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

[Jesse Gross]

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:

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)