Re: [PATCH 0/6] realtek: fix management of mdb entries

Re: [PATCH 0/6] realtek: fix management of mdb entries

[Vladimir Oltean]

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?
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?

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)?

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?

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.

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.

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.

Re: [PATCH 0/6] realtek: fix management of mdb entries

[Jan Hoffmann]

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

Re: [PATCH 0/6] realtek: fix management of mdb entries

[Vladimir Oltean]

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?

Re: [PATCH 0/6] realtek: fix management of mdb entries

[Vladimir Oltean]

On Tue, Mar 21, 2023 at 07:24:15PM +0200, Vladimir Oltean wrote:
> 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?

Left you by mistake in Cc:. Moving to To: ;)

Re: [PATCH 0/6] realtek: fix management of mdb entries

[Andrew Lunn]

> > 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?

I've no real experience with this either. But it sounds like this
merging should be pulled up into the bridge, if all switchdev drivers
need it.

     Andrew

Re: [PATCH 0/6] realtek: fix management of mdb entries

[Bjørn Mork]

Vladimir Oltean <olteanv@gmail.com> writes:

> 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.

I don't think that's an option.  The CPU is dimensioned for switch
management. For the RTL838x, using the current ethernet driver, it tends
to break down completely at 30-40 Mbits/s.

Multicast offloading is necessary to be able to use one of these devices
for e.g IPTV.  Which definitely is a relevant use case.


Bjørn