Hi Ben, Any chance you can take a peek at the email below? I hope you can remember this from 2018 :)
Thanks, Eelco On 15 Sep 2021, at 15:30, Eelco Chaudron wrote: > Hi Ben, > > The mentioned fix below is causing a regression, actually two related ones: > > 1) If IGMP snooping is disabled, IGMP related traffic is still sent slow path > for processing with the NORMAL rule. In addition, even without the NORMAL > rule, the DP rule gets modified. > > Here are some examples: > > a) default config, one bridge (kernel dp), two ports (i.e. only a single > NORMAL rule, and mcast_snooping_enable: false), send an IGMP join: > > - With your fix the dp rule looks like this: > > > recirc_id(0),in_port(2),eth(src=04:f4:bc:28:57:01,dst=01:00:5e:00:00:16),eth_type(0x0800),ipv4(frag=no), > packets:8, bytes:576, used:0.169s, > actions:userspace(pid=4220974525,slow_path(match)) > > - Without the fix, which is correct: > > > recirc_id(0),in_port(2),eth(src=04:f4:bc:28:57:01,dst=01:00:5e:00:00:16),eth_type(0x0800),ipv4(frag=no), > packets:8, bytes:576, used:0.004s, actions:1 > > > b) with a more specific OpenFlow entry, no DP entry gets created: > > ovs-ofctl del-flows ovs_pvp_br0 && ovs-ofctl add-flow ovs_pvp_br0 > "in_port=enp5s0f0,eth_type(0x800),ip_proto=2,action=drop" > ovs-ofctl del-flows ovs_pvp_br0 && ovs-ofctl add-flow ovs_pvp_br0 > "in_port=enp5s0f0,eth_type(0x800),ip_proto=2,action=vnet6" > > > 2) With the netdev (DPDK) interface all seems to work as expected, i.e., the > flow gets installed correctly without the userspace upcall in both cases > above. However, the verifier generates an error as it would like to update > the dp rule with a slow_path variant. Which is failing as netdev is using the > odp_flow_key_to_flow() utility function, which has the IGMP check. > > Looking at the change it assumes you filter on igmp type/code when you filter > on the protocol type igmp. But this is not true, you can just filter on the > protocol type only, and this is what is actually causing the problems above. > So I think your patch needs to be reversed! > > However, looking at fixing Huanle's original problem, > https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/343665.html, > looking at his patch, this might be the right solution. All IGMP packets > independent of their type need to be processed by a SLOW_ACTION upcall. > > > I've copied in Flavio, who worked on the IGMP snooping part before, as he > might still remember what I argue above is true :) > > > If you have any other ideas, thoughts please let me know. > > > Cheers, > > Eelco > > ============================================================ > > commit c645550bb2498fb3816b6a39b22bffeb3154dca3 > Author: Ben Pfaff <b...@ovn.org> > Date: Wed Jan 24 11:40:20 2018 -0800 > > odp-util: Always report ODP_FIT_TOO_LITTLE for IGMP. > > OVS datapaths don't understand or parse IGMP fields, but OVS userspace > does, so this commit updates odp_flow_key_to_flow() to report that > properly > to the caller. > > Reported-by: Huanle Han <hanxue...@gmail.com> > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/343665.html > Signed-off-by: Ben Pfaff <b...@ovn.org> > > diff --git a/lib/odp-util.c b/lib/odp-util.c > index 14d5af097..5da83b4c6 100644 > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -6210,6 +6210,11 @@ 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) { _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
