Message-ID: <Z793qqMMvxKuFxbM@sellars>
Date: Wed, 26 Feb 2025 21:20:58 +0100
From: Linus Lüssing <linus.luessing@...3.blue>
To: Nikolay Aleksandrov <razor@...ckwall.org>
Cc: Joseph Huang <joseph.huang.2024@...il.com>,
	Vladimir Oltean <olteanv@...il.com>,
	Joseph Huang <Joseph.Huang@...min.com>, netdev@...r.kernel.org,
	Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Roopa Prabhu <roopa@...dia.com>, linux-kernel@...r.kernel.org,
	bridge@...ts.linux.dev, Jan Hoffmann <jan@....eu>,
	Birger Koblitz <git@...ger-koblitz.de>,
	Sebastian Gottschall <s.gottschall@...wrt.com>
Subject: Re: [PATCH RFC net-next 00/10] MC Flood disable and snooping

Sorry for chiming in so late in this conversation, missed it due
to the mailing list address change and how my procmail sorts
things...

First of all, many thanks Joseph for looking into this! I think I
agree with you that there is an actual RFC4541 compatiblity
issue for multicast offloading switches which (try to) use
the kernel space Linux bridge IGMP/MLD snooping code. For both
routable and non-routable multicast traffic.


I see a lot of confusion and misunderstandings in this thread. And
initially got very confused by the cover letter, too, and needed to
recheck the MCAST_FLOOD flag behaviour in the code and in practice :-).

> There is a use case where one would like to enable multicast snooping
> on a bridge but disable multicast flooding on all bridge ports so that
> registered multicast traffic will only reach the intended recipients and
> unregistered multicast traffic will be dropped.

To clarify for others: This is exactly what the Linux bridge does
right now. With bridge multicast snooping enabled and active
(a querier is present) any snoopable, unregistered (no MDB entry)
IP multicast payload traffic will only be forwarded to ports which
were either manually set to a multicast router port or where a
multicast router was detected via IGMP/MLD query, MRD or PIM
snooping. And if no such port exists, will be dropped.

The current per port MCAST_FLOOD flag implementation does not change this
behaviour. Its current (unfortunately not very well documented)
behaviour is basically, mainly to decide on what to do with packets
which the bridge multicast snooping code *can not* deal with / can
not learn about. Which can be because multicast snooping is
disabled, because there is no IGMP/MLD querier, because they are
IGMPv1/v2/MLDv1 packets, because they are not in the RFC4541
defined snoopable address ranges - or because it is a multicast
packet which is not an IP packet after all. The last case should
make it clear why the MCAST_FLOOD is 1/enabled by default. Which
might be a bit confusing/counterintuitive initially when
coming and thinking from the other, the IP snooping direction.

> bridge ports' mcast_flood flag implementation, it doesn't work as desired.

The important context to the "it doesn't work as desired"
can be found some lines later:
"3. A hardware offloading switch is involved".

The main issue seems that the learned or manually set multicast
router ports in the Linux bridge are not propagated down to the
actual multicast offloading switches. And therefore these switches
won't be able to follow RFC4541, which will potentially lead to
packet loss for multicast packets, both routable ones - as
multicast routers are not detected - but also for non-routable ones
due to potential IGMPv1/v2/MLDv1 report suppression.
This is the main issue this patchset tries to tackle, making
multicast offloading switches with kernelspace IGMP/MLD snooping
RFC4541 compliant, I believe?

-----

> [PATCH RFC net-next 03/10] net: bridge: Always flood local subnet mc packets
> ...
> If multicast flooding is disabled on a bridge port, local subnet multicast
> packets from the bridge will not be forwarded out of that port, even if
> IGMP snooping is running and the hosts beyond the bridge port are sending
> Reports to join these groups (e.g., 224.0.0.251)

This is a fix for a regression which patch 01/10 introduced in the first place

> [PATCH RFC net-next 02/10] net: bridge: Always multicast_flood Reports
> ...
> Fixes: b00589af3b04 ("bridge: disable snooping if there is no querier")

This is a regression of patch 01/10, the "Fixes" line is
incorrect.


> [PATCH RFC net-next 09/10] net: dsa: mv88e6xxx: Enable mc flood for mrouter port

So this is what the idea of repurposing/redefining of MCAST_FLOOD
ultimately comes down to, I guess?

I'm wondering, shouldn't the information you're looking for already
be available inside the Linux bridge as is? As it
right now correctly knows when to flood on a port?
Hm, would it maybe alternatively be possible to somehow call the
new dsa_switch_ops.port_mrouter you're adding - which
seems to be a missing, key part to fix this in the DSA API -
from for instance br_port_mc_router_state_change() instead?
Without needing any new or redefined state or knob in the
Linux bridge?

I'm also not quite sure yet what effect your proposed changes to
the MCAST_FLOOD knob would have to non-IP multicast packets -
could it break them?

-----

For the discussion regarding more knobs for a more fine-grained
control for flooding various types of packets: I agree that would
be nice to have. But I don't think it's necessary to fix the
original issue which Joseph tries to address.

Also I think they maybe should be added afterwards, after fixing
the issue with offloading switches? As already as is the port
MCAST_FLOOD and BR_INPUT_SKB_CB(skb)->mrouters_only flags
interactions are quite hard to follow and confusing in the
Linux bridge code (qed.?).

Regards, Linus


PS: Also adding Jan Hoffmann, Birger Koblitz and
Sebastian Gottschall to CC, as they seem to have been working on
getting such a feature running in OpenWrt/DD-Wrt for rtl83xx
DSA switches, too.

Powered by blists - more mailing lists