[ovs-dev] [PATCH] [RFC] flow: Do not clear L3+ fields of flow in flow_push_mpls()

[Simon Horman]

On Thu, Mar 20, 2014 at 10:05:44AM -0700, Ben Pfaff wrote:
> On Fri, Mar 14, 2014 at 04:19:52PM +0900, Simon Horman wrote:
> > When creating a flow in the datapath as the result of an upcall
> > the match itself is the match supplied in the upcall while
> > the mask of the match, if supplied, is generated based on the
> > flow and mask composed during action translation.
> > 
> > In the case of, for example a UDP packet, the match will include
> > of L2, L3 and L4 fields. However, if the flow is cleared in
> > flow_push_mpls() then the mask that is synthesised from it will
> > not include L3 and L4 fields. This seems incorrect and the kernel
> > datapath complains about this mismatch.
> > 
> > Signed-off-by: Simon Horman <horms at verge.net.au>
> 
> The goal of clearing the fields is to ensure that later flow tables
> can't match on fields that aren't visible anymore.  That's important for
> accurate OpenFlow implementation, so I'd rather not change it.  On the
> other hand, I see the point you're making, but I don't immediately
> understand why it happens that way.  After all, I can change fields with
> OpenFlow actions and the datapath flows work out OK, why doesn't this
> work out OK too?  Do you understand the reason?

Hi Ben,

I have taken a closer look at this and I think that I now understand the
problem. I would like to propose the following:


From: Simon Horman <horms at verge.net.au>

odp-util: Do not set port mask of non-IP packets

In the case that an flow for an IP packet has an mpls_push action applied
the L3 and L4 portions of the flow will be cleared in flow_push_mpls().

Without this change commit_set_port_action() will set the tp_src and tp_dst
mask for the flow to all-ones because the base and flow port values no
longer match. Even though there will be no corresponding set action for the
ports; because the flow is no longer IP.

In this case where nw_proto is not part of the match this manifests
in a problem because the userspace datapath rejects flows whose masks
have non-zero values for tp_src or dp_dst if the nw_proto mask is
not all-ones.

This patch resolves this problem by having commit_set_port_action() return
without doing anything if flow->nw_proto is zero. The same logic is present
in commit_set_nw_action().

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

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 956fef1..b40f9bc 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -3778,6 +3778,11 @@ static void
 commit_set_port_action(const struct flow *flow, struct flow *base,
                        struct ofpbuf *odp_actions, struct flow_wildcards *wc)
 {
+    /* Check if 'flow' really has an L3 header. */
+    if (!flow->nw_proto) {
+        return;
+    }
+
     if (!is_ip_any(base) || (!base->tp_src && !base->tp_dst)) {
         return;
     }