[ovs-dev] Issue with one of your old patches, commit c645550b

Ben Pfaff blp at ovn.org
Mon Oct 4 23:55:25 UTC 2021


This code in odp-util is basically unmaintainable.  I've never been able
to figure out how to properly test or reason about it.  It's a huge
reason that I want to get rid of the kernel module.

On Mon, Oct 04, 2021 at 11:49:41AM +0200, Eelco Chaudron wrote:
> 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 <blp at 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 <hanxueluo at gmail.com>
> >     Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/343665.html
> >     Signed-off-by: Ben Pfaff <blp at 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) {
> 


More information about the dev mailing list