[ovs-dev] [PATCH v2 07/16] ofp-util: Fix table features decoding of action and instruction fields.

Jarno Rajahalme jrajahalme at nicira.com
Fri Aug 8 19:31:51 UTC 2014


Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>

On Aug 7, 2014, at 4:13 PM, Ben Pfaff <blp at nicira.com> wrote:

> Parsing of actions was wrong because OF1.3 says that non-experimenter
> actions are 4 bytes and don't include padding.  This fixes the problem.
> 
> Parsing of instructions seems wrong too because OF1.3 says that
> non-experimenter instructions are 4 bytes (though it is not explicit about
> padding as it is for actions).  This fixes the problem there too.
> 
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
> lib/ofp-util.c |   47 +++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 1e641bd..1ba3970 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -61,12 +61,17 @@ struct ofp_prop_header {
> /* Pulls a property, beginning with struct ofp_prop_header, from the beginning
>  * of 'msg'.  Stores the type of the property in '*typep' and, if 'property' is
>  * nonnull, the entire property, including the header, in '*property'.  Returns
> - * 0 if successful, otherwise an error code. */
> + * 0 if successful, otherwise an error code.
> + *
> + * This function pulls the property's stated size padded out to a multiple of
> + * 'alignment' bytes.  The common case in OpenFlow is an 'alignment' of 8, so
> + * you can use ofputil_pull_property() for that case. */
> static enum ofperr
> -ofputil_pull_property(struct ofpbuf *msg, struct ofpbuf *property,
> -                      uint16_t *typep)
> +ofputil_pull_property__(struct ofpbuf *msg, struct ofpbuf *property,
> +                        unsigned int alignment, uint16_t *typep)
> {
>     struct ofp_prop_header *oph;
> +    unsigned int padded_len;
>     unsigned int len;
> 
>     if (ofpbuf_size(msg) < sizeof *oph) {
> @@ -75,7 +80,8 @@ ofputil_pull_property(struct ofpbuf *msg, struct ofpbuf *property,
> 
>     oph = ofpbuf_data(msg);
>     len = ntohs(oph->len);
> -    if (len < sizeof *oph || ROUND_UP(len, 8) > ofpbuf_size(msg)) {
> +    padded_len = ROUND_UP(len, alignment);
> +    if (len < sizeof *oph || padded_len > ofpbuf_size(msg)) {
>         return OFPERR_OFPBPC_BAD_LEN;
>     }
> 
> @@ -83,10 +89,24 @@ ofputil_pull_property(struct ofpbuf *msg, struct ofpbuf *property,
>     if (property) {
>         ofpbuf_use_const(property, ofpbuf_data(msg), len);
>     }
> -    ofpbuf_pull(msg, ROUND_UP(len, 8));
> +    ofpbuf_pull(msg, padded_len);
>     return 0;
> }
> 
> +/* Pulls a property, beginning with struct ofp_prop_header, from the beginning
> + * of 'msg'.  Stores the type of the property in '*typep' and, if 'property' is
> + * nonnull, the entire property, including the header, in '*property'.  Returns
> + * 0 if successful, otherwise an error code.
> + *
> + * This function pulls the property's stated size padded out to a multiple of
> + * 8 bytes, which is the common case for OpenFlow properties. */
> +static enum ofperr
> +ofputil_pull_property(struct ofpbuf *msg, struct ofpbuf *property,
> +                      uint16_t *typep)
> +{
> +    return ofputil_pull_property__(msg, property, 8, typep);
> +}
> +
> static void PRINTF_FORMAT(2, 3)
> log_property(bool loose, const char *message, ...)
> {
> @@ -4443,10 +4463,12 @@ ofputil_encode_port_mod(const struct ofputil_port_mod *pm,
> 
>     return b;
> }
> +
> +/* Table features. */
> 
> static enum ofperr
> pull_table_feature_property(struct ofpbuf *msg, struct ofpbuf *payload,
> -                        uint16_t *typep)
> +                            uint16_t *typep)
> {
>     enum ofperr error;
> 
> @@ -4467,7 +4489,7 @@ parse_action_bitmap(struct ofpbuf *payload, enum ofp_version ofp_version,
>         uint16_t type;
>         enum ofperr error;
> 
> -        error = pull_table_feature_property(payload, NULL, &type);
> +        error = ofputil_pull_property__(payload, NULL, 1, &type);
>         if (error) {
>             return error;
>         }
> @@ -4489,7 +4511,16 @@ parse_instruction_ids(struct ofpbuf *payload, bool loose, uint32_t *insts)
>         enum ofperr error;
>         uint16_t ofpit;
> 
> -        error = pull_table_feature_property(payload, NULL, &ofpit);
> +        /* OF1.3 and OF1.4 aren't clear about padding in the instruction IDs.
> +         * It seems clear that they aren't padded to 8 bytes, though, because
> +         * both standards say that "non-experimenter instructions are 4 bytes"
> +         * and do not mention any padding before the first instruction ID.
> +         * (There wouldn't be any point in padding to 8 bytes if the IDs were
> +         * aligned on an odd 4-byte boundary.)
> +         *
> +         * Anyway, we just assume they're all glommed together on byte
> +         * boundaries. */
> +        error = ofputil_pull_property__(payload, NULL, 1, &ofpit);
>         if (error) {
>             return error;
>         }
> -- 
> 1.7.10.4
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev




More information about the dev mailing list