Ilya Maximets <[email protected]> writes: > On 2/23/22 19:58, Aaron Conole wrote: >> Ilya Maximets <[email protected]> writes: >> >>> On 1/6/22 19:07, Aaron Conole wrote: >>>> 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. >>>> >>>> lib/odp-util.c | 5 --- >>>> ofproto/ofproto-dpif-xlate.c | 1 - >>>> tests/mcast-snooping.at | 68 ++++++++++++++++++++++++++++++ >>>> tests/ofproto-macros.at | 15 +++++++ >>>> tests/system-traffic.at | 80 ++++++++++++++++++++++++++++++++++++ >>>> 5 files changed, 163 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 cafcd014ad..29d26cf9ac 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..e24f293a0c 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>) >>>> +]) >>> >>> I tried to backport the patch to earlier branches, but this test fails >>> on 2.14 and 2.13 (works on 2.15+) with the following result: >>> >>> --- - 2022-01-26 21:33:15.713282778 +0000 >>> +++ >>> /home/runner/work/ovs/ovs/openvswitch-2.13.7/_build/sub/tests/testsuite.dir/at-groups/2302/stdout >>> 2022-01-26 21:33:15.708679313 +0000 >>> @@ -1,4 +1,4 @@ >>> +ct_state(+inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(frag=no), >>> packets:0, bytes:0, used:never, actions:drop >>> >>> 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>) >>> >>> Packets seems to be marked +inv by conntrack. Is it some kind >>> of conntrack issue that got fixed in 2.15 that we should backport? >>> Can we modify the test somehow to make it work on 2.13 ? >> >> I'll look into it. Thanks. >> >>>> + >>>> +AT_CLEANUP >>>> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at >>>> index 736d9809cb..f906b5c3b5 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 d79753a99a..5d9449fada 100644 >>>> --- a/tests/system-traffic.at >>>> +++ b/tests/system-traffic.at >>>> @@ -6287,6 +6287,86 @@ 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 dpif/show | grep -v missed:], [0], [dnl >>>> + br0: >>>> + br0 65534/1: (internal) >>> >>> This line is failing the system-userspace testsuite, because userspace >>> datapath reports 'tap' instead of 'internal'. Suggesting a following >>> change: >>> >>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >>> index 1ba28f7ee..7711f14b0 100644 >>> --- a/tests/system-traffic.at >>> +++ b/tests/system-traffic.at >>> @@ -6681,18 +6681,11 @@ 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 dpif/show | grep -v missed:], [0], [dnl >>> - br0: >>> - br0 65534/1: (internal) >>> - ovs-p1 1/2: (system) >>> - ovs-p2 2/3: (system) >>> -]) >>> - >>> -AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl >>> +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(2),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:1,3 >>> +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 >>> --- >>> >>> What do you think? >> >> ACK - I'll pull that in. >> >>> >>>> + ovs-p1 1/2: (system) >>>> + ovs-p2 2/3: (system) >>>> +]) >>>> + >>>> +AT_CHECK([ovs-appctl dpctl/dump-flows | 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(2),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:1,3 >>>> +]) >>>> +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 >>>> + >>>> +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) >>>> +]) >>> >>> These are OpenFlow rules. Shouldn't we dump datapath flows, or >>> am I missing something? >> >> IIRC, we use the openflow rules because they are wildly different >> datapath rules between the two, so we rely on these rules being hit. >> Additionally, in the kernel DP case, if ipv6 is enabled then some ND >> will occur and that will tamper with n_packets/n_bytes counters (which >> is why I clear it before doing the comparison). >> >> I will look at the datapath flows again, but I believe the datapath >> action set is different because of the way controller() is implemented >> between the two (in the userspace DP, it can be a direct call, but in >> the kerneldp it needs to be a userspace call first). >> >> So I guess I will look at this, but expect to keep it as-is. > > OK. I guess, the main check here is n_packets=[[1-9]]. Is that correct? > Maybe just a comment would be enough in that case. It's not clear what > exactly we're testing here otherwise.
Makes sense. Yes, that is the main check. Perhaps we should explicitly add a check like that as well. I'll see what I can do. > Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
