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

Simon Horman horms at verge.net.au
Wed May 14 06:41:55 UTC 2014


On Wed, May 07, 2014 at 05:02:06PM +1200, Simon Horman wrote:
> On Thu, May 01, 2014 at 08:24:21AM -0700, Ben Pfaff wrote:
> > On Thu, May 01, 2014 at 08:21:36AM -0700, Jarno Rajahalme wrote:
> > > 
> > > > On May 1, 2014, at 7:53 AM, Ben Pfaff <blp at nicira.com> wrote:
> > > > 
> > > >> On Mon, Apr 28, 2014 at 03:57:58PM -0700, Jarno Rajahalme wrote:
> > > >> 
> > > >>> On Mar 20, 2014, at 10:05 AM, Ben Pfaff <blp at nicira.com> 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?
> > > >> 
> > > >> As the flow?s dl_type is changed to an MPLS type, later non-MPLS rules
> > > >> will not match on the modified flow. AFAIK, you can match on L3/L4
> > > >> fields only by also matching on the corresponding dl_type as a
> > > >> prerequisite, no?
> > > > 
> > > > Yes, that's true.
> > > > 
> > > >> If this holds, I?d rather not clear the fields so we can properly do a
> > > >> set IPv4 action followed by an MPLS push action. Currently the the
> > > >> MPLS action clears the flow values at the translation time set in the
> > > >> preceding action, so that at the commit time the values intended for
> > > >> set IPv4 action are lost.
> > > > 
> > > > Are you sure?  compose_mpls_push_action() call commit_odp_actions() to
> > > > avoid this very problem.
> > > > 
> > > 
> > > I did not notice this, sorry about that.
> > > 
> > > > Assuming I'm right about that, at this point what I really want is an
> > > > example of a situation that's broken in the current code and not broken
> > > > with this patch applied, so that I can understand exactly what we're
> > > > getting at here.
> > > > 
> > > 
> > > It seems that a check on the nw_proto before committing set ports
> > > actions is enough to avoid introducing new problems as part of my
> > > masked set actions patch series.
> > 
> > OK.
> > 
> > Simon, I guess that you still see a problem that we should fix.  Can you
> > provide me an example that I can work through for myself?  I want
> > everything to work well.
> 
> Sure, will do.

Hi,

I had done some further analysis of this problem, which seems to have
ended up in an unloved portion of this thread. That analysis resulted in
the patch below. I hope that its changelog explains the situation.


The problem that I describe in the changelog below only manifests
when using the kernel datapath as the user-space datapath does
not verify flows nearly as strictly (at all?).

The problem also only manifests if an IP packet becomes an MPLS packet.
As such I have used v 2.56 of the MPLS datapath patch to exercise this
problem. You can find it at:

git://github.com/horms/openvswitch.git devel/mpls-v2.56

Although it doesn't seem to make any difference to the discussion at hand
I have also provided that patch rebase on today's master branch here:

git://github.com/horms/openvswitch.git devel/mpls-v2.56.rebase


I have been able to exercise the problem using the following manual test
using the kernel datapath: I'm entirely unsure how to automate tests that
use the kernel datapath.

ovs-vsctl --if-exists del-br br0

ovs-vsctl add-br br0
ovs-vsctl add-port br0 eth0

ip link set up dev br0
ip link set up dev eth0

ovs-ofctl add-flow br0 "ip actions=push_mpls:0x8847,normal"

mz br0 -v -a 0e:73:cc:bd:88:d8 -b 0e:73:cc:bd:88:d9 \
	-A 192.0.0.42 -B 192.168.0.2 -t udp "sp=80 dp=80"
TZ=UTC date
ovs-dpctl dump-flows
grep 0e:73:cc:bd:88:d8 /var/log/openvswitch/ovs-vswitchd.log | tail -1


Using devel/mpls-v2.56.rebase without the patch below applied
I see the following result:

# TZ=UTC date
Wed May 14 06:25:55 UTC 2014
# ovs-dpctl dump-flows
[nothing]
# grep 0e:73:cc:bd:88:d8 /var/log/openvswitch/ovs-vswitchd.log | tail -1
2014-05-14T06:25:55.900Z|00001|dpif(handler13)|WARN|system at ovs-system: failed to put[create][modify] (Invalid argument) dp_hash(0/0),recirc_id(0),skb_priority(0),in_port(1),skb_mark(0/0),eth(src=0e:73:cc:bd:88:d8,dst=0e:73:cc:bd:88:d9),eth_type(0x0800),ipv4(src=192.0.0.42/0.0.0.0,dst=192.168.0.2/0.0.0.0,proto=17/0,tos=0/0xfc,ttl=255/0xff,frag=no/0xff),udp(src=80,dst=0), actions:push_mpls(label=0,tc=0,ttl=255,bos=1,eth_type=0x8847),2

Using devel/mpls-v2.56.rebase
with the patch below applied I see the following result:

# TZ=UTC date
Wed May 14 06:27:20 UTC 2014
# ovs-dpctl dump-flows
recirc_id(0),skb_priority(0),in_port(1),eth(src=0e:73:cc:bd:88:d8,dst=0e:73:cc:bd:88:d9),eth_type(0x0800),ipv4(src=192.0.0.42/0.0.0.0,dst=192.168.0.2/0.0.0.0,proto=17/0,tos=0/0xfc,ttl=255/0xff,frag=no/0xff), packets:0, bytes:0, used:never, actions:push_mpls(label=0,tc=0,ttl=255,bos=1,eth_type=0x8847),2
recirc_id(0),skb_priority(0),in_port(1),eth(src=3e:26:15:0c:06:4b,dst=33:33:00:00:00:02),eth_type(0x86dd),ipv6(src=fe80::3c26:15ff:fe0c:64b/::,dst=ff02::2/::,label=0/0,proto=58/0,tclass=0/0,hlimit=255/0,frag=no/0xff), packets:0, bytes:0, used:never, actions:2
# grep 0e:73:cc:bd:88:d8 /var/log/openvswitch/ovs-vswitchd.log | tail -1
[nothing]


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 kernel 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;
     }






More information about the dev mailing list