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 <[email protected]> Acked-by: Eelco Chaudron <[email protected]> --- 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. v3->v4: Fix up the unit tests to be consistent when reporting dp flows. Split IGMP unit test into two different tests. Use a multicast eth addr for the FLOOD under NORMAL action case. v4->v5: Address Ilya's comments. NOTE: We need a27d70a89848 ("conntrack: add generic IP protocol support") if we want to backport the test with this change - otherwise the userspace CT will mark IGMP packets as invalid (always) and fail the test. lib/odp-util.c | 5 --- ofproto/ofproto-dpif-xlate.c | 1 - tests/mcast-snooping.at | 67 ++++++++++++++++++++++++++++++ tests/ofproto-macros.at | 15 +++++++ tests/system-traffic.at | 79 ++++++++++++++++++++++++++++++++++++ 5 files changed, 161 insertions(+), 6 deletions(-) diff --git a/lib/odp-util.c b/lib/odp-util.c index 073fc20d9f..40e89a7cf1 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -7170,11 +7170,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 5a770f1713..830d65145a 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3067,7 +3067,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..fe475e7b38 100644 --- a/tests/mcast-snooping.at +++ b/tests/mcast-snooping.at @@ -216,3 +216,70 @@ 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]) + +add_of_ports br0 1 2 + +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 p1 \ + '0101000c29a0aa55aa550001080046c00028000040000102d3494565eb4ae0000016940400002200f9020000000104000000e00000fb000000000000']) +AT_CHECK([ovs-appctl netdev-dummy/receive p2 \ + 'aa55aa5500010101000c29a008004500001c00010000400164dc0a0101010a0101020800f7ffffffffff']) + + +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl + strip_stats | strip_used | strip_recirc | dnl + sed -e 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], + [0], [dnl +recirc_id(<recirc>),in_port(1),eth(src=aa:55:aa:55:00:01,dst=01:01:00:0c:29:a0),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:100,2 +recirc_id(<recirc>),in_port(2),eth(src=01:01:00:0c:29:a0,dst=aa:55:aa:55:00:01),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, 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/warp 100000 + +dnl Send the IGMP, followed by a unicast ICMP - ensure we won't black hole +AT_CHECK([ovs-appctl netdev-dummy/receive p1 \ + '0101000c29a0aa55aa550001080046c00028000040000102d3494565eb4ae0000016940400002200f9020000000104000000e00000fb000000000000']) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 \ + 'aa55aa550001aa55aa55000208004500001c00010000400164dc0a0101010a0101020800f7ffffffffff']) + + +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl + strip_stats | strip_used | strip_recirc | dnl + sed 's/pid=[[0-9]]*,// + s/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], + [0], [dnl +ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=1,frag=no), packets:0, bytes:0, used:never, actions:2 +ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=2,frag=no), packets:0, bytes:0, used:never, 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(1),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:0.0s, actions:ct(zone=64000),recirc(<recirc>) +]) + +AT_CLEANUP diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at index 7051d95396..b18f0fbc1e 100644 --- a/tests/ofproto-macros.at +++ b/tests/ofproto-macros.at @@ -134,6 +134,21 @@ strip_ufid () { sed 's/mega_ufid:[[-0-9a-f]]* // s/ufid:[[-0-9a-f]]* //' } + +# Strips packets: and bytes: from output +strip_stats () { + sed 's/packets:[[0-9]]*/packets:0/ + s/bytes:[[0-9]]*/bytes:0/' +} + +# Changes all 'recirc(...)' and 'recirc=...' to say 'recirc(<recirc_id>)' and +# 'recirc=<recirc_id>' respectively. This should make output easier to +# compare. +strip_recirc() { + sed 's/recirc_id([[x0-9]]*)/recirc_id(<recirc>)/ + s/recirc_id=[[x0-9]]*/recirc_id=<recirc>/ + s/recirc([[x0-9]]*)/recirc(<recirc>)/' +} m4_divert_pop([PREPARE_TESTS]) m4_define([TESTABLE_LOG], [-vPATTERN:ANY:'%c|%p|%m']) diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 4a7fa49fc1..fbc8050876 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -6760,6 +6760,85 @@ 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(p1, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01") +ADD_VETH(p2, 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 p1 01 00 5e 01 01 03 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 --names | grep -e .*ipv4 | sort | dnl + strip_stats | strip_used | strip_recirc | dnl + sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], + [0], [dnl +recirc_id(<recirc>),in_port(ovs-p1),eth(src=f0:00:00:01:01:01,dst=01:00:5e:01:01:03),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:br0,ovs-p2 +]) +OVS_TRAFFIC_VSWITCHD_STOP +AT_CLEANUP + +AT_SETUP([IGMP - forward with ICMP]) + +OVS_TRAFFIC_VSWITCHD_START() +ADD_NAMESPACES(at_ns0, at_ns1) + +ADD_VETH(p1, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01") +ADD_VETH(p2, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02") + +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 del-flows br0]) +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) + +dnl Send the IGMP, followed by a unicast ICMP - ensure we won't black hole + +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p1 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 p1 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]) + +sleep 1 + +dnl Prefer the OpenFlow rules, because different datapaths will behave slightly +dnl differently with respect to the exact dp rules. +dnl +dnl This is also why we clear n_bytes / n_packets - some kernels with ipv6 +dnl enabled will bump some of these counters non-deterministically + +AT_CHECK([ovs-ofctl dump-flows br0 | grep -v NXST | dnl + strip_duration | grep -v arp | grep -v n_packets=0 | dnl + grep -v 'in_port=2 actions=output:1' | dnl + sed 's/n_bytes=[[0-9]]*/n_bytes=0/ + s/idle_age=[[0-9]]*/idle_age=0/ + s/n_packets=[[1-9]]/n_packets=0/' | sort], [0], [dnl + cookie=0x0, table=0, n_packets=0, n_bytes=0, idle_age=0, ip,in_port=1 actions=ct(table=1,zone=64000) + cookie=0x0, table=1, n_packets=0, n_bytes=0, idle_age=0, ct_state=+new+trk,icmp,in_port=1 actions=output:2 + cookie=0x0, table=1, n_packets=0, n_bytes=0, idle_age=0, ct_state=+new+trk,ip,in_port=1 actions=controller(userdata=00.de.ad.be.ef.ca.fe.01) +]) + +OVS_TRAFFIC_VSWITCHD_STOP +AT_CLEANUP + AT_BANNER([802.1ad]) AT_SETUP([802.1ad - vlan_limit]) -- 2.31.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
