This reverts commit c645550bb249 ("odp-util: Always report
ODP_FIT_TOO_LITTLE for IGMP.")

Always forcing a slow path action can result in some over-broad
flows which swallow all traffic and force them to userspace, as reported
in the thread at
https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387706.html
and at
https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387689.html

Revert the ODP_FIT_TOO_LITTLE return for IGMP specifically.
Additionally, remove the userspace wc mask to prevent revalidator from
cycling flows.

Fixes: c645550bb249 ("odp-util: Always report ODP_FIT_TOO_LITTLE for IGMP.")
Signed-off-by: Aaron Conole <acon...@redhat.com>
Acked-by: Eelco Chaudron <echau...@redhat.com>
---
v1->v2: Add unit test for coverage of mcast traffic when not doing snooping
v2->v3: Add a unit test for the non-dummy datapaths as well to demonstrate the
        matching issue.

 lib/odp-util.c               |  5 ---
 ofproto/ofproto-dpif-xlate.c |  1 -
 tests/mcast-snooping.at      | 72 ++++++++++++++++++++++++++++++++++++
 tests/system-traffic.at      | 65 ++++++++++++++++++++++++++++++++
 4 files changed, 137 insertions(+), 6 deletions(-)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index fbdfc7ad83..0f21ffe639 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -7132,11 +7132,6 @@ parse_l2_5_onward(const struct nlattr 
*attrs[OVS_KEY_ATTR_MAX + 1],
                 }
             }
         }
-    } else if (src_flow->nw_proto == IPPROTO_IGMP
-               && src_flow->dl_type == htons(ETH_TYPE_IP)) {
-        /* OVS userspace parses the IGMP type, code, and group, but its
-         * datapaths do not, so there is always missing information. */
-        return ODP_FIT_TOO_LITTLE;
     }
     if (is_mask && expected_bit != OVS_KEY_ATTR_UNSPEC) {
         if ((flow->tp_src || flow->tp_dst) && flow->nw_proto != 0xff) {
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 88cdc09a0e..dacee0642d 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3048,7 +3048,6 @@ xlate_normal(struct xlate_ctx *ctx)
              */
             ctx->xout->slow |= SLOW_ACTION;
 
-            memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
             if (mcast_snooping_is_membership(flow->tp_src) ||
                 mcast_snooping_is_query(flow->tp_src)) {
                 if (ctx->xin->allow_side_effects && ctx->xin->packet) {
diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at
index 757cf7186e..478aced91d 100644
--- a/tests/mcast-snooping.at
+++ b/tests/mcast-snooping.at
@@ -216,3 +216,75 @@ AT_CHECK([ovs-appctl mdb/show br0], [0], [dnl
 ])
 
 AT_CLEANUP
+
+
+AT_SETUP([mcast - igmp flood for non-snoop enabled])
+OVS_VSWITCHD_START([])
+
+AT_CHECK([
+    ovs-vsctl set bridge br0 \
+    datapath_type=dummy], [0])
+
+AT_CHECK([
+    ovs-vsctl add-port br0 p0 -- set Interface p0 type=dummy \
+    other-config:hwaddr=aa:55:aa:55:00:01 ofport_request=1 \
+    -- add-port br0 p1 \
+    -- set Interface p1 type=dummy other-config:hwaddr=aa:55:aa:55:00:02 
ofport_request=2 \
+], [0])
+
+AT_CHECK([ovs-ofctl add-flow br0 action=normal])
+
+ovs-appctl time/stop
+
+dnl Basic scenario - needs to flood for IGMP followed by unicast ICMP
+dnl in reverse direction
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 \
+    
'0101000c29a0aa55aa550001080046c00028000040000102d3494565eb4ae0000016940400002200f9020000000104000000e00000fb000000000000'])
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 \
+    
'aa55aa5500010101000c29a008004500001c00010000400164dc0a0101010a0101020800f7ffffffffff'])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | dnl
+          sed -e 's/ packets:[[0-9]]*,//' -e 's/ bytes:[[0-9]]*,//' dnl
+              -e 's/ used:[[a-zA-Z0-9\.]]*,//' -e 's/pid=[[0-9]]*,//' dnl
+              -e 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'],
+                     [0], [dnl
+recirc_id(0),in_port(1),eth(src=aa:55:aa:55:00:01,dst=01:01:00:0c:29:a0),eth_type(0x0800),ipv4(frag=no),
 actions:100,2
+recirc_id(0),in_port(2),eth(src=01:01:00:0c:29:a0,dst=aa:55:aa:55:00:01),eth_type(0x0800),ipv4(frag=no),
 actions:1
+])
+
+ovs-appctl time/warp 100000
+
+dnl Next we should clear the flows and install a complex case
+AT_CHECK([ovs-ofctl del-flows br0])
+
+AT_DATA([flows.txt], [dnl
+table=0, arp actions=NORMAL
+table=0, ip,in_port=1 actions=ct(table=1,zone=64000)
+table=0, in_port=2 actions=output:1
+table=1, ip,ct_state=+trk+inv actions=drop
+table=1  ip,in_port=1,icmp,ct_state=+trk+new actions=output:2
+table=1, in_port=1,ip,ct_state=+trk+new 
actions=controller(userdata=00.de.ad.be.ef.ca.fe.01)
+table=1, in_port=1,ip,ct_state=+trk+est actions=output:2
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+
+ovs-appctl time/stop
+
+dnl Send the IGMP, followed by a unicast ICMP - ensure we won't black hole
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 \
+    
'0101000c29a0aa55aa550001080046c00028000040000102d3494565eb4ae0000016940400002200f9020000000104000000e00000fb000000000000'])
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 \
+    
'aa55aa550001aa55aa55000208004500001c00010000400164dc0a0101010a0101020800f7ffffffffff'])
+
+
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | dnl
+          sed -e 's/ packets:[[0-9]]*,//' -e 's/ bytes:[[0-9]]*,//' dnl
+              -e 's/ used:[[a-zA-Z0-9\.]]*,//' -e 's/pid=[[0-9]]*,//' dnl
+              -e 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'],
+                     [0], [dnl
+ct_state(+new-inv+trk),recirc_id(0x1),in_port(1),eth_type(0x0800),ipv4(proto=2,frag=no),
 
actions:userspace(controller(reason=1,dont_send=0,continuation=0,recirc_id=2,rule_cookie=0,controller_id=0,max_len=65535))
+recirc_id(0),in_port(1),eth_type(0x0800),ipv4(frag=no), 
actions:ct(zone=64000),recirc(0x1)
+ct_state(+new-inv+trk),recirc_id(0x1),in_port(1),eth_type(0x0800),ipv4(proto=1,frag=no),
 actions:2
+])
+
+AT_CLEANUP
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index d79753a99a..e77799489d 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -6287,6 +6287,71 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep table=2, | 
OFPROTO_CLEAR_DURATION_IDLE
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_BANNER([IGMP])
+
+AT_SETUP([IGMP - flood under normal action])
+
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01")
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02")
+
+AT_CHECK([ovs-ofctl add-flow br0 "actions=NORMAL"])
+
+NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 f0 00 00 01 01 02 dnl
+f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl
+00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl
+00 00 00 00 > /dev/null])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | dnl
+          sed -e 's/ packets:[[0-9]]*,//' -e 's/ bytes:[[0-9]]*,//' dnl
+              -e 's/ used:[[a-zA-Z0-9]]*,//'],
+                     [0], [dnl
+recirc_id(0),in_port(2),eth(src=f0:00:00:01:01:01,dst=f0:00:00:01:01:02),eth_type(0x0800),ipv4(frag=no),
 actions:1,3
+])
+
+AT_CHECK([ovs-ofctl del-flows br0])
+AT_DATA([flows.txt], [dnl
+table=0, arp actions=NORMAL
+table=0, ip,in_port=1 actions=ct(table=1,zone=64000)
+table=0, in_port=2 actions=output:1
+table=1, ip,ct_state=+trk+inv actions=drop
+table=1  ip,in_port=1,icmp,ct_state=+trk+new actions=output:2
+table=1, in_port=1,ip,ct_state=+trk+new 
actions=controller(userdata=00.de.ad.be.ef.ca.fe.01)
+table=1, in_port=1,ip,ct_state=+trk+est actions=output:2
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+
+OVS_WAIT_UNTIL([ovs-appctl dpctl/dump-flows | wc -l | grep -E '^0$'])
+
+dnl Send the IGMP, followed by a unicast ICMP - ensure we won't black hole
+
+NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 f0 00 00 01 01 02 dnl
+f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl
+00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl
+00 00 00 00 > /dev/null])
+
+NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 f0 00 00 01 01 02 dnl
+f0 00 00 01 01 01 08 00 45 00 00 1c 00 01 00 00 40 01 64 dc 0a 01 01 01 0a dnl
+01 01 02 08 00 f7 ff ff ff ff ff > /dev/null])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | dnl
+          sed -e 's/ packets:[[0-9]]*,//' -e 's/ bytes:[[0-9]]*,//' dnl
+              -e 's/recirc_id([[x0-9]]*)/recirc_id(<recirc>)/' dnl
+              -e 's/recirc_id=[[x0-9]]*/recirc_id=<recirc>/' dnl
+              -e 's/recirc([[x0-9]]*)/recirc(<recirc>)/' dnl
+              -e 's/pid=[[0-9]]*,//' -e 's/ used:[[\.a-zA-Z0-9]]*,//' | sort],
+                     [0], [dnl
+recirc_id(<recirc>),in_port(2),ct_state(+new-inv+trk),eth(),eth_type(0x0800),ipv4(proto=1,frag=no),
 actions:3
+recirc_id(<recirc>),in_port(2),ct_state(+new-inv+trk),eth(),eth_type(0x0800),ipv4(proto=2,frag=no),
 
actions:userspace(controller(reason=1,dont_send=0,continuation=0,recirc_id=<recirc>,rule_cookie=0,controller_id=0,max_len=65535))
+recirc_id(<recirc>),in_port(2),eth(),eth_type(0x0800),ipv4(frag=no), 
actions:ct(zone=64000),recirc(<recirc>)
+recirc_id(<recirc>),in_port(3),eth(),eth_type(0x0800),ipv4(frag=no), actions:2
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_BANNER([802.1ad])
 
 AT_SETUP([802.1ad - vlan_limit])
-- 
2.31.1

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to