[ovs-dev] [PATCH] nx-match: Correct writing of value and length in set_field_to_ofast()

Simon Horman horms at verge.net.au
Thu Feb 28 05:00:22 UTC 2013


On Wed, Feb 27, 2013 at 09:41:41AM -0800, Ben Pfaff wrote:
> On Wed, Feb 27, 2013 at 04:12:16PM +0900, Simon Horman wrote:
> > ofpbuf_put_* may reallocate the underlying buffer of the ofpbuf and
> > thus writing data after a ofpbuf_put_* call must write to memory
> > relative to the pointer returned by the call.
> > 
> > Prior to this change the length and trailing value would not be written to
> > the set_field action if ofpbuf_put_* may reallocated the underlying buffer.
> > 
> > Also make use of ofpbuf_put_zero() to avoid calling memset() directly.
> > 
> > Signed-off-by: Simon Horman <horms at verge.net.au>
> 
> Good catch!
> 
> I think that we don't really need to reload oasf at all, because we
> can just initialize oasf->len earlier and then not refer back to oasf
> again after the ofpbuf_put().  That seems a little nicer.  So how
> about this:

Indeed it does, how silly of me not to notice that.

I have verified the patch below passes the test case I have which
is to add 120k flows and then run the following:

ovs-ofctl --protocols OpenFlow12 dump-flows br0

On the off chance that it is of value to you:

Tested-by: Simon Horman <horms at verge.net.au>

> --8<--------------------------cut here-------------------------->8--
> 
> From: Simon Horman <horms at verge.net.au>
> Date: Wed, 27 Feb 2013 16:12:16 +0900
> Subject: [PATCH] nx-match: Correct writing of value and length in set_field_to_ofast()
> 
> ofpbuf_put_* may reallocate the underlying buffer of the ofpbuf and
> thus writing data after a ofpbuf_put_* call must write to memory
> relative to the pointer returned by the call.
> 
> Prior to this change the length and trailing value would not be written to
> the set_field action if ofpbuf_put_* may reallocated the underlying buffer.
> 
> Also make use of ofpbuf_put_zero() to avoid calling memset() directly.
> 
> Signed-off-by: Simon Horman <horms at verge.net.au>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  lib/nx-match.c |   15 ++++++---------
>  1 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/nx-match.c b/lib/nx-match.c
> index 4ff516e..e5545de 100644
> --- a/lib/nx-match.c
> +++ b/lib/nx-match.c
> @@ -1217,22 +1217,19 @@ set_field_to_ofast(const struct ofpact_reg_load *load,
>                        struct ofpbuf *openflow)
>  {
>      const struct mf_field *mf = load->dst.field;
> +    uint16_t padded_value_len = ROUND_UP(mf->n_bytes, 8);
>      struct ofp12_action_set_field *oasf;
> -    uint16_t padded_value_len;
> -
> -    oasf = ofputil_put_OFPAT12_SET_FIELD(openflow);
> -    oasf->dst = htonl(mf->oxm_header);
> +    char *value;
>  
>      /* Set field is the only action of variable length (so far),
>       * so handling the variable length portion is open-coded here */
> -    padded_value_len = ROUND_UP(mf->n_bytes, 8);
> -    ofpbuf_put_uninit(openflow, padded_value_len);
> +    oasf = ofputil_put_OFPAT12_SET_FIELD(openflow);
> +    oasf->dst = htonl(mf->oxm_header);
>      oasf->len = htons(ntohs(oasf->len) + padded_value_len);
> -    memset(oasf + 1, 0, padded_value_len);
>  
> +    value = ofpbuf_put_zeros(openflow, padded_value_len);
>      bitwise_copy(&load->subvalue, sizeof load->subvalue, load->dst.ofs,
> -                 oasf + 1, mf->n_bytes, load->dst.ofs, load->dst.n_bits);
> -    return;
> +                 value, mf->n_bytes, load->dst.ofs, load->dst.n_bits);
>  }
>  
>  void
> -- 
> 1.7.2.5
> 



More information about the dev mailing list