[ovs-dev] [PATCH] datapath: Do not clear key in ovs_match_init()

Simon Horman horms at verge.net.au
Thu Jun 27 04:53:00 UTC 2013


On Mon, Jun 24, 2013 at 12:29:55PM -0700, Jesse Gross wrote:
> On Sun, Jun 23, 2013 at 7:29 PM, Simon Horman <horms at verge.net.au> wrote:
> > It appears that in the case where vs_match_init() is called from
> > ovs_flow_metadata_from_nlattrs() it is undesirable to set the flow key as
> > some of its values are set earlier on in ovs_flow_metadata_from_nlattrs().
> > Furthermore in the case where ovs_flow_metadata_from_nlattrs() is called
> > from ovs_packet_cmd_execute() key has been partially initialised by
> > ovs_flow_extract().
> >
> > This manifests in a problem when executing actions as elements of key are
> > used when verifying some actions. For example a dec_ttl action verifies the
> > proto of the flow. An example of a flow that fails as a result of this
> > problem is:
> >
> >         ovs-ofctl add-flow br0 "ip actions=dec_ttl,normal"
> >
> > This patch resolves this problem by not clearing key in ovs_match_init()
> > and instead clearing it before calling ovs_match_init() when it is called
> > other than by ovs_flow_metadata_from_nlattrs().
> >
> > This appears to be a regression added by "datapath: Mega flow
> > implementation", a1c564be1e2ffc31f8da09ab654c8ed987907fe5.
> >
> > Cc: Andy Zhou <azhou at nicira.com>
> > Signed-off-by: Simon Horman <horms at verge.net.au>
> >
> > ---
> >
> > I am unsure of the exact intention of clearing the key and thus the
> > correctness of this patch. However, it does appear to resolve
> > the problem that I describe above.
> 
> Thanks, I think this can simplified a little bit. I applied the following patch:

Thanks, I have confirmed that resolves the problem that I observed.

> commit c121c7ce259016fb5630c382bf99f9ac4d5900fc
> Author: Jesse Gross <jesse at nicira.com>
> Date:   Mon Jun 24 12:21:29 2013 -0700
> 
>     datapath: Do not clear key in ovs_match_init()
> 
>     When executing packets sent from userspace, the majority of the
>     flow information is extracted from the packet itself and a small
>     amount of metadata supplied by userspace is added. However, when
>     adding this metadata, the extracted flow information is currently
>     being cleared.
> 
>     This manifests in a problem when executing actions as elements of key are
>     used when verifying some actions. For example a dec_ttl action verifies th
>     proto of the flow. An example of a flow that fails as a result of this
>     problem is:
> 
>             ovs-ofctl add-flow br0 "ip actions=dec_ttl,normal"
> 
>     This is a regression added by "datapath: Mega flow implementation",
>     a1c564be1e2ffc31f8da09ab654c8ed987907fe5.
> 
>     CC: Andy Zhou <azhou at nicira.com>
>     Reported-by: Simon Horman <horms at verge.net.au>
>     Signed-off-by: Jesse Gross <jesse at nicira.com>
> 
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 499e3e2..39de931 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -1601,7 +1601,8 @@ int ovs_flow_metadata_from_nlattrs(struct sw_flow *flow,
>         if (err)
>                 return -EINVAL;
> 
> -       ovs_match_init(&match, &flow->key, NULL);
> +       memset(&match, 0, sizeof(match));
> +       match.key = &flow->key;
> 
>         err = metadata_from_nlattrs(&match, &attrs, a, false);
>         if (err)
> 



More information about the dev mailing list