Message-ID: <20230321172415.3ccdi4j226d5qa7h@skbuf>
Date:   Tue, 21 Mar 2023 19:24:15 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Jan Hoffmann <jan@....eu>
Cc:     Arınç ÜNAL <arinc.unal@...nc9.com>,
        netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        openwrt-devel@...ts.openwrt.org,
        Sander Vanheule <sander@...nheule.net>,
        erkin.bozoglu@...ont.com
Subject: Re: [PATCH 0/6] realtek: fix management of mdb entries

On Fri, Mar 10, 2023 at 07:37:34PM +0100, Jan Hoffmann wrote:
> Hi Vladimir,
> 
> Thank you for having a look at this!
> 
> On 06.03.2023 14:46, Vladimir Oltean wrote:
> > On Sat, Mar 04, 2023 at 01:52:32PM +0300, Arınç ÜNAL wrote:
> > > On 4.03.2023 00:48, Jan Hoffmann wrote:
> > > > This series fixes multiple issues related to the L2 table and multicast
> > > > table. That includes an issue that causes corruption of the port mask
> > > > for unknown multicast forwarding, which can occur even when multicast
> > > > snooping is disabled.
> > > > 
> > > > With these patches, multicast snooping should be mostly working.
> > > > However, one important missing piece is forwarding of all multicast
> > > > traffic to multicast router ports (as specified in section 2.1.2-1 of
> > > > RFC4541). As far as I can see, this is a general issue that affects all
> > > > DSA switches, and cannot be fixed without changes to the DSA subsystem.
> > > 
> > > Do you plan to discuss this on the netdev mailing list with Vladimir?
> > > 
> > > Arınç
> > 
> > I searched for the patches and found them at
> > https://patchwork.ozlabs.org/project/openwrt/cover/20230303214846.410414-1-jan@3e8.eu/
> > 
> > I guess that what should be discussed is the topic of switch ports
> > attached to multicast routers, yes?
> > 
> > DSA does not (and has never) listen(ed) to the switchdev notifications
> > emitted for bridge ports regarding multicast routers (SWITCHDEV_ATTR_ID_PORT_MROUTER).
> > It has only listened for a while to the switchdev notifications for the
> > bridge itself as a multicast router (SWITCHDEV_ATTR_ID_BRIDGE_MROUTER),
> > and even that was done for a fairly strange reason and eventually got
> > reverted for breaking something - see commits 08cc83cc7fd8 and c73c57081b3d.
> > 
> > I personally don't have use cases for IP multicast routing / snooping,
> > so I would need some guidance regarding what is needed for things to
> > work smoothly. Also, to make sure that they keep working in the future,
> > one of the tests from tools/testing/selftests/net/forwarding/ which
> > exercises flooding towards multicast ports (if it exists) should be
> > symlinked to tools/testing/selftests/drivers/net/dsa/ and adapted until
> > it works. That's a pretty good way to get maintainers' attention on a
> > feature that they don't normally test.
> > 
> > It's not the first time I'm reading RFC4541, but due to a lack of any
> > practical applications surrounding me (and therefore also partial lack
> > of understanding), I keep forgetting what it says :)
> > 
> > Section 2.1.1.  IGMP Forwarding Rules (for the control path) says
> > 
> >     1) A snooping switch should forward IGMP Membership Reports only to
> >        those ports where multicast routers are attached.
> > 
> > how? I guess IGMP/MLD packets should reach the CPU via packet traps
> > (which cause skb->offload_fwd_mark to be unset), and from there, the
> > bridge software data path identifies the mrouter ports and forwards
> > control packets only there? What happens if the particular switch
> > hardware doesn't support IGMP/MLD packet identification and trapping?
> 
> I wasn't really thinking about this potential issue, as that already works
> for switches that trap IGMP/MLD reports to the CPU. But multicast snooping
> is definitely going to be broken for DSA drivers that implement the MDB
> methods but don't trap IGMP/MLD to the CPU port.
> 
> > Should the driver install a normal multicast forwarding rule for
> > all 224.0.0.X traffic (translated to MAC), and patch the tagging
> > protocol driver to set skb->offload_fwd_mark = 0 based on
> > eth_hdr(skb)->h_dest?
> Doing something like that should work for IGMPv3/MLDv2, as reports have the
> fixed destination addresses 224.0.0.22 and ff02:16. However, for these
> protocol versions, it should also be okay to flood reports, because clients
> don't do report suppression in that case.
> 
> For IGMPv1/IGMPv2/MLDv1, this approach isn't practical, as reports are sent
> to the group address itself, i.e. are not limited to 224.0.0.X. Detecting
> them requires looking further into the packets (there are a few details on
> this in section 3 "IPv6 Considerations" of RFC4515).
> 
> So, I don't think there really is a good way to do multicast snooping on
> hardware that doesn't support detecting and trapping IGMP/MLD reports
> specifically. Of course, trapping all multicast to the CPU (as you suggested
> below) should always be an option, but the additional CPU load might be an
> issue.
> 
> Another possibility would be to just not support multicast snooping on such
> devices and always flood multicast (i.e. how a driver without
> port_mdb_add/port_mdb_del works right now). However, that opens the question
> about what should happen when multicast snooping is enabled on a bridge
> (which is the default), but the DSA switch does not support it. I guess a
> clean solution would be to just not allow this in the first place. If this
> is allowed, then making sure that any multicast originating from the CPU is
> flooded to all switch ports should also avoid breaking multicast (but having
> a bridge that performs multicast snooping only partially, i.e. for the
> non-offloaded ports, is probably a confusing design).
> 
> > Then for the data path we have:
> > 
> > 2.1.2.  Data Forwarding Rules
> > 
> >     1) Packets with a destination IP address outside 224.0.0.X which are
> >        not IGMP should be forwarded according to group-based port
> >        membership tables and must also be forwarded on router ports.
> > 
> >        This is the main IGMP snooping functionality for the data path.
> >        One approach that an implementation could take would be to
> >        maintain separate membership and multicast router tables in
> >        software and then "merge" these tables into a forwarding cache.
> > 
> > For my clarity, this means that *all* IP multicast packets must be
> > forwarded to the multicast router ports, be their addresses known
> > (bridge mdb entries exist for them) or unknown (flooded)?
> 
> Yes, that is how I understand this section as well.
> 
> > What does the software bridge implementation do? Does it perform this
> > "merging" of tables for us? (for known MDB entries, does it also notify
> > them through switchdev on the mrouter ports?) Looking superficially at
> > the first-order callers of br_mdb_notify(), I don't get the impression
> > that the bridge has this logic?
> 
> Unfortunately, the software bridge implementation doesn't do this kind of
> merging ahead of time, otherwise there wouldn't be any need to handle this
> in DSA. It only takes place in br_multicast_flood, the function that does
> the actual forwarding of registered multicast:
> 
> https://elixir.bootlin.com/linux/v6.2.3/source/net/bridge/br_forward.c#L277
> 
> > Or am I completely off with my reading of RFC4541?
> > 
> > It's not obvious to me, after looking at the few implementations of
> > SWITCHDEV_ATTR_ID_PORT_MROUTER handlers in drivers, that this merging of
> > forwarding tables would be done anywhere within sight.
> 
> It looks to me like all three switchdev drivers with handlers for
> SWITCHDEV_ATTR_ID_PORT_MROUTER actually add the mrouter ports to mdb
> entries:
> 
> https://elixir.bootlin.com/linux/v6.2.3/source/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c#L1022
> 
> https://elixir.bootlin.com/linux/v6.2.3/source/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c#L2163
> 
> https://elixir.bootlin.com/linux/v6.2.3/source/drivers/net/ethernet/microchip/sparx5/sparx5_switchdev.c#L105
> 
> > If someone could explain to me what are the expectations (we don't seem
> > to have a lot of good documentation) and how do the existing drivers work,
> > we can start discussing more concretely how the layering and API within
> > DSA should look like.
> 
> I experimented a bit with the rtl83xx switch driver in OpenWrt by passing
> through the switchdev events to the DSA driver:
> https://github.com/janh/openwrt/commit/80b677fe04292570d7e304e184fd7d4ac8397a4f
> 
> The disadvantage with this approach is that the DSA driver has to do the
> "merging" of mrouter ports and mdb entries. As a port can be a group member
> and multicast router port at the same time, this means the driver now has to
> keep track of all mdb entries, to be able to properly handle when a port
> leaves a group or stops being a multicast router.
> 
> If the DSA subsystem could handle the "merging" instead and also call
> port_mdb_add/port_mdb_del as appropriate for multicast router ports, the
> individual drivers wouldn't have to deal with this particular issue at all.
> 
> > As a way to fix a bug quickly and get correct behavior, I guess there's
> > also the option of stopping to process multicast packets in hardware,
> > and configure the switch to always send any multicast to the CPU port
> > only. As long as the tagger knows to leave skb->offload_fwd_mark unset,
> > the bridge driver should know how to deal with those packets in
> > software, and forward them only to whom is interested. But the drawback
> > is that there is no forwarding acceleration involved. Maybe DSA should
> > have done that from the get go for drivers which didn't care about
> > multicast in particular, instead of ending up with this current situation
> > which appears to be slightly chaotic.
> 
> Thanks,
> Jan

Andrew, Florian, do you have any additional comments here?

Powered by blists - more mailing lists