From 791a09be15d3cd36f6a59086794a6ef2b4a76669 Mon Sep 17 00:00:00 2001 From: Simon Horman Date: Wed, 4 Jun 2014 16:56:09 +0000 Subject: [PATCH] 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(). Also enhance one of the MPLS tests to exercise this logic. The enhanced tests inputs a UDP packet with non-zero ports rather than an IP packet with zeroed ports: zeroed ports cause commit_set_port_action() always return without doing anything.. Commit 691d39b ("upcall: Remove redundant xlate_actions_for_side_effects().") causes xlate_in_init() to be called for every packet that has an upcall. This has the effect of indirectly calling commit_set_port_action() when translating a controller action which may not have previously been the case depending on the flow. The result is that the behaviour described in the changelog above can be exercised via a minor enhancement to one of the existing MPLS tests. This illustrates that the problem exists for the user-space datapath whereas I had previously incorrectly assumed it only manifested when using the kernel datapath because I had only observed it there. Signed-off-by: Simon Horman Acked-by: Jarno Rajahalme --- lib/odp-util.c | 5 +++++ tests/ofproto-dpif.at | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/odp-util.c b/lib/odp-util.c index 280e8a647..3c69adaf4 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -3790,6 +3790,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; } diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index c14d671ed..aea8a93ab 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -1325,7 +1325,7 @@ dnl Modified MPLS controller action. AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log]) for i in 1 2 3; do - ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:44:42,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=16,tos=0,ttl=64,frag=no)' + ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:44:42,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=64,frag=no),udp(src=7777,dst=80)' done OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6]) OVS_APP_EXIT_AND_WAIT(ovs-ofctl) -- 2.20.1