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 <ho...@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 <ho...@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 <ho...@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;
}
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
