Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Linus Lüssing]

On Sat, Jun 29, 2019 at 07:29:45PM +0300, Ido Schimmel wrote:
> I would like to avoid having drivers take the querier state into account
> as it will only complicate things further.

I absolutely share your pain. Initially in the early prototypes of
multicast awareness in batman-adv we did not consider the querier state.
And doing so later did indeed complicate the code a good bit in batman-adv
(together with the IGMP/MLD suppression issues). I would have loved to
avoid that.


> Is there anything we can do about it? Enable the bridge querier if no
> other querier was detected? Commit c5c23260594c ("bridge: Add
> multicast_querier toggle and disable queries by default") disabled
> queries by default, but I'm only suggesting to turn them on if no other
> querier was detected on the link. Do you think it's still a problem?

As soon as you start becoming the querier, you will not be able to reliably
detect anymore whether you are the only querier candidate.

If any random Linux host using a bridge device were potentially becoming
a querier, that would cause quite some trouble when this host is
behind some bad, bottleneck connection. This host will receive
all multicast traffic, not just IGMP/MLD reports. And with a
congested connection and then unreliable IGMP/MLD, multicast would
become unreliable overall in this domain. So it's important that
your querier is not running in the "dark, remote, dusty closet" of
your network (topologically speaking).

> On Sun, Jun 23, 2019 at 10:44:27AM +0300, Ido Schimmel wrote:
> > See commit b00589af3b04 ("bridge: disable snooping if there is no
> > querier"). I think that's unfortunate behavior that we need because
> > multicast snooping is enabled by default. If it weren't enabled by
> > default, then anyone enabling it would also make sure there's a querier
> > in the network.

I do not quite understand that point. In a way, that's what we
have right now, isn't it? By default it's disabled, because by
default there is no querier on the link. So anyone wanting to use
multicast snooping will need to make sure there's a querier in the
network.


Overall I think the querier (election) mechanism in the standards could
need an update. While the lowest-address first might have
worked well back then, in uniform, fully wired networks where the
position of the querier did not matter, this is not a good
solution anymore in networks involving wireless, dynamic connections.
Especially in wireless mesh networks this is a bit of an issue for
us. Ideally, the querier mechanism were dismissed in favour of simply
unsolicited, periodic IGMP/MLD reports...

But of course, updating IETF standards is no solution for now. 

While more complicated, it would not be impossible to consider the
querier state, would it? I mean you probably already need to
consider the case of a user disabling multicast snooping during
runtime, right? So similarly, you could react to appearing or
disappearing queriers?

Cheers, Linus

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Nikolay Aleksandrov]

On 30/06/2019 19:56, Linus Lüssing wrote:
> On Sat, Jun 29, 2019 at 07:29:45PM +0300, Ido Schimmel wrote:
>> I would like to avoid having drivers take the querier state into account
>> as it will only complicate things further.
> 
> I absolutely share your pain. Initially in the early prototypes of
> multicast awareness in batman-adv we did not consider the querier state.
> And doing so later did indeed complicate the code a good bit in batman-adv
> (together with the IGMP/MLD suppression issues). I would have loved to
> avoid that.
> 
> 
>> Is there anything we can do about it? Enable the bridge querier if no
>> other querier was detected? Commit c5c23260594c ("bridge: Add
>> multicast_querier toggle and disable queries by default") disabled
>> queries by default, but I'm only suggesting to turn them on if no other
>> querier was detected on the link. Do you think it's still a problem?
> 
> As soon as you start becoming the querier, you will not be able to reliably
> detect anymore whether you are the only querier candidate.
> 
> If any random Linux host using a bridge device were potentially becoming
> a querier, that would cause quite some trouble when this host is
> behind some bad, bottleneck connection. This host will receive
> all multicast traffic, not just IGMP/MLD reports. And with a
> congested connection and then unreliable IGMP/MLD, multicast would
> become unreliable overall in this domain. So it's important that
> your querier is not running in the "dark, remote, dusty closet" of
> your network (topologically speaking).
> 

+1
We definitely don't want random hosts becoming queriers

>> On Sun, Jun 23, 2019 at 10:44:27AM +0300, Ido Schimmel wrote:
>>> See commit b00589af3b04 ("bridge: disable snooping if there is no
>>> querier"). I think that's unfortunate behavior that we need because
>>> multicast snooping is enabled by default. If it weren't enabled by
>>> default, then anyone enabling it would also make sure there's a querier
>>> in the network.
> 
> I do not quite understand that point. In a way, that's what we
> have right now, isn't it? By default it's disabled, because by
> default there is no querier on the link. So anyone wanting to use
> multicast snooping will need to make sure there's a querier in the
> network.
> 

Indeed, also you could create the bridge with explicit mcast parameters if you need
different behaviour on start. Unfortunately I think you'll have to handle
the querier state.

> 
> Overall I think the querier (election) mechanism in the standards could
> need an update. While the lowest-address first might have
> worked well back then, in uniform, fully wired networks where the
> position of the querier did not matter, this is not a good
> solution anymore in networks involving wireless, dynamic connections.
> Especially in wireless mesh networks this is a bit of an issue for
> us. Ideally, the querier mechanism were dismissed in favour of simply
> unsolicited, periodic IGMP/MLD reports...
> 
> But of course, updating IETF standards is no solution for now. 
> 
> While more complicated, it would not be impossible to consider the
> querier state, would it? I mean you probably already need to
> consider the case of a user disabling multicast snooping during
> runtime, right? So similarly, you could react to appearing or
> disappearing queriers?
> 
> Cheers, Linus
> 

Thanks,
 Nik

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Ido Schimmel]

On Sun, Jun 30, 2019 at 06:56:01PM +0200, Linus Lüssing wrote:
> > On Sun, Jun 23, 2019 at 10:44:27AM +0300, Ido Schimmel wrote:
> > > See commit b00589af3b04 ("bridge: disable snooping if there is no
> > > querier"). I think that's unfortunate behavior that we need because
> > > multicast snooping is enabled by default. If it weren't enabled by
> > > default, then anyone enabling it would also make sure there's a querier
> > > in the network.
> 
> I do not quite understand that point. In a way, that's what we
> have right now, isn't it? By default it's disabled, because by
> default there is no querier on the link. So anyone wanting to use
> multicast snooping will need to make sure there's a querier in the
> network.

Hi Linus,

Querier state is not reflected to drivers ATM, so drivers believe the
bridge is multicast aware and unregistered multicast packets are only
flooded to mrouter ports. Hosts that are silent (because there is no
querier) never get the traffic addressed to them (f.e., IPv6 neighbour
solicitation).

> Overall I think the querier (election) mechanism in the standards could
> need an update. While the lowest-address first might have
> worked well back then, in uniform, fully wired networks where the
> position of the querier did not matter, this is not a good
> solution anymore in networks involving wireless, dynamic connections.
> Especially in wireless mesh networks this is a bit of an issue for
> us. Ideally, the querier mechanism were dismissed in favour of simply
> unsolicited, periodic IGMP/MLD reports...
> 
> But of course, updating IETF standards is no solution for now. 
> 
> While more complicated, it would not be impossible to consider the
> querier state, would it? I mean you probably already need to
> consider the case of a user disabling multicast snooping during
> runtime, right? 

Sure, this is implemented.

> So similarly, you could react to appearing or disappearing queriers?

Yes, but it's a bit more complicated since we need to differentiate
between IPv4 and IPv6. If the bridge is multicast aware, but there is
only IPv4 querier on the link, then:

1. All the IPv6 MDB entries need to be removed from the device. At least
in mlxsw, we do not have a way to ignore only IPv6 entries. From the
device's perspective, an MDB entry is just a multicast DMAC with a
bitmap of ports packets should be replicated to.

2. We need to split the flood tables used for IPv4 and IPv6 unregistered
multicast packets. For IPv4, packets should only be flooded to mrouter
ports whereas for IPv6 packets should be flooded to all the member
ports.

Do you differentiate between IPv4 and IPv6 in batman-adv?

> Cheers, Linus

Thanks for the feedback!

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Linus Lüssing]

Hi Ido,

> Do you differentiate between IPv4 and IPv6 in batman-adv?

For most things, yes: The querier state is kept separately for
IPv4 and IPv6. And we do have something like a "router node"
flag to signalize that a node needs all multicast traffic, which
is split into IPv4 and IPv6.

The "MDB" equivalent in batman-adv (multicast entries in our "TT",
translation table) are on MAC address base right now, not on an IP
address base yet, so that sounds similar to what you do in mlxsw?

Regarding querier state, we periodically ask the
bridge via "br_multicast_has_querier_anywhere(dev, ETH_P_IP)"
and "br_multicast_has_querier_anywhere(dev, ETH_P_IPV6)".

(Something more event based with handler functions would probably
be nicer, but this was the easier thing to start with.)


> 1. All the IPv6 MDB entries need to be removed from the device. At least
> in mlxsw, we do not have a way to ignore only IPv6 entries. From the
> device's perspective, an MDB entry is just a multicast DMAC with a
> bitmap of ports packets should be replicated to.

Ah, I see, yes. We had a similar "issue". Initially we just always
added any multicast entry into our translation table offered by
the IP stack or bridge, no matter what a querier state or "router
node" state said. Which would have led to a node receiving two
copies of a multicast packet if it were both a querier or router
and were also having a listener announced via IGMP/MLD.

So there we also just recently changed that, to filter out
IPv6 multicast TT entries if this node were also announcing itself as
an MLD querier or IPv6 router. And same, independently for
IPv4/IGMP.

> 2. We need to split the flood tables used for IPv4 and IPv6 unregistered
> multicast packets. For IPv4, packets should only be flooded to mrouter
> ports whereas for IPv6 packets should be flooded to all the member
> ports.

This one I do not fully understand yet. Why don't you apply the
"flood to all ports" (in the no IGMP querier present case)
for IPv4, too?

Sure, for IPv4 nothing "essential" will break, as IPv4 unicast
does not rely on multicast (contrary to IPv6, due to NDP, as you
mentioned). But still there would be potential multicast packet loss
for a 239.x.x.x listener on the local link, for instance, wouldn't
there?


If I'm not mistaken, we do not apply differing behaviour for IPv4
vs. IPv6 in the bridge either and would flood on all ports for IPv4
with no querier present, too.

Regards, Linus

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Ido Schimmel]

On Wed, Jul 03, 2019 at 01:13:08AM +0200, Linus Lüssing wrote:
> Hi Ido,
> 
> > Do you differentiate between IPv4 and IPv6 in batman-adv?
> 
> For most things, yes: The querier state is kept separately for
> IPv4 and IPv6. And we do have something like a "router node"
> flag to signalize that a node needs all multicast traffic, which
> is split into IPv4 and IPv6.
> 
> The "MDB" equivalent in batman-adv (multicast entries in our "TT",
> translation table) are on MAC address base right now, not on an IP
> address base yet, so that sounds similar to what you do in mlxsw?

Yes.

> Regarding querier state, we periodically ask the
> bridge via "br_multicast_has_querier_anywhere(dev, ETH_P_IP)"
> and "br_multicast_has_querier_anywhere(dev, ETH_P_IPV6)".
> 
> (Something more event based with handler functions would probably
> be nicer, but this was the easier thing to start with.)

Thanks for the reference. Will check the code. I believe that we will
add switchdev notifications for querier state change, so it might be
useful for you as well.

> > 1. All the IPv6 MDB entries need to be removed from the device. At least
> > in mlxsw, we do not have a way to ignore only IPv6 entries. From the
> > device's perspective, an MDB entry is just a multicast DMAC with a
> > bitmap of ports packets should be replicated to.
> 
> Ah, I see, yes. We had a similar "issue". Initially we just always
> added any multicast entry into our translation table offered by
> the IP stack or bridge, no matter what a querier state or "router
> node" state said. Which would have led to a node receiving two
> copies of a multicast packet if it were both a querier or router
> and were also having a listener announced via IGMP/MLD.
> 
> So there we also just recently changed that, to filter out
> IPv6 multicast TT entries if this node were also announcing itself as
> an MLD querier or IPv6 router. And same, independently for
> IPv4/IGMP.

This is actually not a problem with mlxsw. The ports a packet should be
replicated to are represented using a bitmap. It does not matter if we
set the bit because it has an MDB entry or because it is an mrouter
port. And obviously it does not matter if we set it twice :)

> > 2. We need to split the flood tables used for IPv4 and IPv6 unregistered
> > multicast packets. For IPv4, packets should only be flooded to mrouter
> > ports whereas for IPv6 packets should be flooded to all the member
> > ports.
> 
> This one I do not fully understand yet. Why don't you apply the
> "flood to all ports" (in the no IGMP querier present case)
> for IPv4, too?
> 
> Sure, for IPv4 nothing "essential" will break, as IPv4 unicast
> does not rely on multicast (contrary to IPv6, due to NDP, as you
> mentioned). But still there would be potential multicast packet loss
> for a 239.x.x.x listener on the local link, for instance, wouldn't
> there?
> 
> 
> If I'm not mistaken, we do not apply differing behaviour for IPv4
> vs. IPv6 in the bridge either and would flood on all ports for IPv4
> with no querier present, too.

I think I was not clear, so I will explain again. I was referring to a
situation where IPv4 has a querier, but IPv6 does not. In this case, the
bridge will flood IPv4 unregistered multicast packets to mrouter ports
only. On the other hand, IPv6 unregistered multicast packets will be
flooded to all the ports. Based on my reading of the code, this is
controlled by 'mcast_hit' in br_handle_frame_finish().

In mlxsw, each packet type (e.g., unknown unicast, IPvX unregistered
multicast) is associated with a flood table (basically a huge matrix,
where row corresponds to VLAN and column corresponds to a port). If we
are to handle the case where IPv4 unregistered multicast packets need to
be flooded to mrouter ports only, whereas IPv6 unregistered multicast
packets need to be flooded to all the ports, then we need to use a
separate flood table for each.

Alternatively, we can use the same flood table, but flood to all the
ports if IPv4 or IPv6 querier is missing. I do not think anything will
break, it is just very efficient. This seems to be allowed by RFC 4541
(Section 2.1.2):

"If a switch receives an unregistered packet, it must forward that
packet on all ports to which an IGMP router is attached. A switch may
default to forwarding unregistered packets on all ports."

> Regards, Linus

Linus, thanks a lot for the great feedback. I really appreciate it!