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 <b...@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 <b...@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 <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?
> > > >>
> > > >> 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@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 <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 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 <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
