[PATCH RFC] net: bridge: don't flood known multicast traffic when snooping is enabled

[PATCH RFC] net: bridge: don't flood known multicast traffic when snooping is enabled

[Nikolay Aleksandrov]

The behaviour since b00589af3b04 ("bridge: disable snooping if there is
no querier") is wrong, we shouldn't be flooding multicast traffic when
there is an mdb entry and we know where it should be forwarded to when
multicast snooping is enabled. This patch changes the behaviour to not
flood known unicast traffic. I'll give two obviously broken cases:
 - most obvious: static mdb created by the user with snooping enabled
 - user-space daemon controlling the mdb table (e.g. MLAG)

Every user would expect to have traffic forwarded only to the configured
mdb destination when snooping is enabled, instead now to get that one
needs to enable both snooping and querier. Enabling querier on all
switches could be problematic and is not a good solution, for example
as summarized by our multicast experts:
"every switch would send an IGMP query for any random multicast traffic it
received across the entire domain and it would send it forever as long as a
host exists wanting that stream even if it has no downstream/directly
connected receivers"

Sending as an RFC to get the discussion going, but I'm strongly for
removing this behaviour and would like to send this patch officially.

We could make this behaviour possible via a knob if necessary, but
it really should not be the default.

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/bridge/br_device.c | 3 +--
 net/bridge/br_input.c  | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 013323b6dbe4..2aa8a6509924 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -96,8 +96,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 		}
 
 		mdst = br_mdb_get(br, skb, vid);
-		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
-		    br_multicast_querier_exists(br, eth_hdr(skb)))
+		if (mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb))
 			br_multicast_flood(mdst, skb, false, true);
 		else
 			br_flood(br, skb, BR_PKT_MULTICAST, false, true);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 5ea7e56119c1..aae78095cf67 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -136,8 +136,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 	switch (pkt_type) {
 	case BR_PKT_MULTICAST:
 		mdst = br_mdb_get(br, skb, vid);
-		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
-		    br_multicast_querier_exists(br, eth_hdr(skb))) {
+		if (mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) {
 			if ((mdst && mdst->host_joined) ||
 			    br_multicast_is_router(br)) {
 				local_rcv = true;
-- 
2.17.2

Re: [PATCH RFC] net: bridge: don't flood known multicast traffic when snooping is enabled

[Nikolay Aleksandrov]

On 15/02/2019 15:04, Nikolay Aleksandrov wrote:
> The behaviour since b00589af3b04 ("bridge: disable snooping if there is
> no querier") is wrong, we shouldn't be flooding multicast traffic when
> there is an mdb entry and we know where it should be forwarded to when
> multicast snooping is enabled. This patch changes the behaviour to not
> flood known unicast traffic. I'll give two obviously broken cases:
>  - most obvious: static mdb created by the user with snooping enabled
>  - user-space daemon controlling the mdb table (e.g. MLAG)
> 

I had to mention: when snooping is enabled and there is *no querier
configured*, that is the important bit here. In most setups the querier
is explicitly configured when there is no mcast router, but it shouldn't
be required to have the intuitive and normal behaviour.

> Every user would expect to have traffic forwarded only to the configured
> mdb destination when snooping is enabled, instead now to get that one
> needs to enable both snooping and querier. Enabling querier on all
> switches could be problematic and is not a good solution, for example
> as summarized by our multicast experts:
> "every switch would send an IGMP query for any random multicast traffic it
> received across the entire domain and it would send it forever as long as a
> host exists wanting that stream even if it has no downstream/directly
> connected receivers"
> 
> Sending as an RFC to get the discussion going, but I'm strongly for
> removing this behaviour and would like to send this patch officially.
> 
> We could make this behaviour possible via a knob if necessary, but
> it really should not be the default.
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> ---
>  net/bridge/br_device.c | 3 +--
>  net/bridge/br_input.c  | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 013323b6dbe4..2aa8a6509924 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -96,8 +96,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>  		}
>  
>  		mdst = br_mdb_get(br, skb, vid);
> -		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
> -		    br_multicast_querier_exists(br, eth_hdr(skb)))
> +		if (mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb))
>  			br_multicast_flood(mdst, skb, false, true);
>  		else
>  			br_flood(br, skb, BR_PKT_MULTICAST, false, true);
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 5ea7e56119c1..aae78095cf67 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -136,8 +136,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  	switch (pkt_type) {
>  	case BR_PKT_MULTICAST:
>  		mdst = br_mdb_get(br, skb, vid);
> -		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
> -		    br_multicast_querier_exists(br, eth_hdr(skb))) {
> +		if (mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) {
>  			if ((mdst && mdst->host_joined) ||
>  			    br_multicast_is_router(br)) {
>  				local_rcv = true;
> 

Re: [PATCH RFC] net: bridge: don't flood known multicast traffic when snooping is enabled

[Ido Schimmel]

On Fri, Feb 15, 2019 at 03:04:27PM +0200, Nikolay Aleksandrov wrote:
> The behaviour since b00589af3b04 ("bridge: disable snooping if there is
> no querier") is wrong, we shouldn't be flooding multicast traffic when
> there is an mdb entry and we know where it should be forwarded to when
> multicast snooping is enabled. This patch changes the behaviour to not
> flood known unicast traffic. I'll give two obviously broken cases:
>  - most obvious: static mdb created by the user with snooping enabled
>  - user-space daemon controlling the mdb table (e.g. MLAG)
> 
> Every user would expect to have traffic forwarded only to the configured
> mdb destination when snooping is enabled, instead now to get that one
> needs to enable both snooping and querier. Enabling querier on all
> switches could be problematic and is not a good solution, for example
> as summarized by our multicast experts:
> "every switch would send an IGMP query for any random multicast traffic it
> received across the entire domain and it would send it forever as long as a
> host exists wanting that stream even if it has no downstream/directly
> connected receivers"
> 
> Sending as an RFC to get the discussion going, but I'm strongly for
> removing this behaviour and would like to send this patch officially.
> 
> We could make this behaviour possible via a knob if necessary, but
> it really should not be the default.
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Yes. This is great! :)

I had to enable a multicast querier when testing static mdb entries only
because the test was "too long" and this check kicked in. Never made
sense to me. Lets drop it if we can.

Reviewed-by: Ido Schimmel <idosch@mellanox.com>

Re: [PATCH RFC] net: bridge: don't flood known multicast traffic when snooping is enabled

[Linus Lüssing]

On Fri, Feb 15, 2019 at 03:04:27PM +0200, Nikolay Aleksandrov wrote:
> Every user would expect to have traffic forwarded only to the configured
> mdb destination when snooping is enabled, instead now to get that one
> needs to enable both snooping and querier. Enabling querier on all
> switches could be problematic and is not a good solution,

There is no need to set the querier on all snooping switches.
br_multicast_querier_exists() checks if a querier exists on the
link in general, not if this particular host/bridge is a querier.


> for example as summarized by our multicast experts:
> "every switch would send an IGMP query

What? RFC3810, section 7.1 says:

"If it is the case, a querier election mechanism (described in
 section 7.6.2) is used to elect a single multicast router to be
 in Querier state. [...] Nevertheless, it is only the [elected] Querier
 that sends periodical or triggered query messages on the subnet."

> for any random multicast traffic it
> received across the entire domain and it would send it forever as long as a
> host exists wanting that stream even if it has no downstream/directly
> connected receivers"

Queries are not send for "any random multicast traffic", but
either periodically (general query) or in response to changes on
multicast address listener state (multicast address specific query).
More precisely, if a host leaves a group and sends an IGMPv3/MLDv2 report
or IGMPv2/MLDv1 "Leave Group"/"Done" message for that.

> Sending as an RFC to get the discussion going, but I'm strongly for
> removing this behaviour and would like to send this patch officially.

While reading the code I'm getting confused with the mrouters_only
flag again... (if I remember correctly it never did what its name
implied) I'd have to test / play with your change to check,
but maybe you have tested these scenarios already:

What happens if:

- no querier exists on the link
- you have added static MDB entries from userspace

=> will only ports with statically configured MDB entries receive
   multicast traffic? what happens to other ports?
   => with no queries, those other ports will stay rather silent,
      they will not send any reports
      => do they miss multicast traffic / will IPv6 (ND) break for
         them?

And what happens if:

- no querier exists on the link
- one port gets an unsolicited MLD report, i.e. because a host has just
  started to listen to a particular multicast address
=> will only this port receive multicast traffic? what happens to
   other ports that have listeners for the same multicast group?


(and what currently confuses me while reading the code if a
- a querier exists
- but no listener for a particular IPv6 group / no mdst
- for IPv6 link-local multicast traffic (so mrouters_only = 0?)
=> will this result in always flooding multicast traffic for
   this particular IPv6 link-local multicast group to all ports?
   => reading the code it seems like, but I had remembered it
      differently; for IPv4 this makes sense, as IGMP is not
      mandatory for link-local addresse, however for IPv6 this
      seems unnecessary, dropping should be the correct approach
      if an MLD querier exists)


Have you done some tests with this change yet, Nikolay?

Regards, Linus

Re: [PATCH RFC] net: bridge: don't flood known multicast traffic when snooping is enabled

[Nikolay Aleksandrov]

On 15/02/2019 19:13, Linus Lüssing wrote:
> On Fri, Feb 15, 2019 at 03:04:27PM +0200, Nikolay Aleksandrov wrote:
>> Every user would expect to have traffic forwarded only to the configured
>> mdb destination when snooping is enabled, instead now to get that one
>> needs to enable both snooping and querier. Enabling querier on all
>> switches could be problematic and is not a good solution,
> 
> There is no need to set the querier on all snooping switches.
> br_multicast_querier_exists() checks if a querier exists on the
> link in general, not if this particular host/bridge is a querier.
> 

We need a generic solution for the case of existing mdst and no querier.
More below.

> 
>> for example as summarized by our multicast experts:
>> "every switch would send an IGMP query
> 
> What? RFC3810, section 7.1 says:
> 
> "If it is the case, a querier election mechanism (described in
>  section 7.6.2) is used to elect a single multicast router to be
>  in Querier state. [...] Nevertheless, it is only the [elected] Querier
>  that sends periodical or triggered query messages on the subnet."
> >> for any random multicast traffic it
>> received across the entire domain and it would send it forever as long as a
>> host exists wanting that stream even if it has no downstream/directly
>> connected receivers"
> 

This was taken out of context and it's my bad, I think everyone is aware
of the election process, please nevermind the above statement.

[snip]> 
> 
> Have you done some tests with this change yet, Nikolay?
> 

You've raised good questions, IPv6 indeed needs more work - we'll have to flood
link-local packets etc. but I wanted to have a discussion about no querier/existing mdst.
To simplify we can modify the patch and have traffic forwarded to the proper ports when an
mdst exists and there is no querier for both unsolicited report and user-added entry.
We can keep the current behaviour for unknown traffic with and without querier.
This would align it closer to what other vendors currently do as well IIRC.
What do you think ?

Thanks,
 Nik

Re: [PATCH RFC] net: bridge: don't flood known multicast traffic when snooping is enabled

[Nikolay Aleksandrov]

On 16/02/2019 10:05, Nikolay Aleksandrov wrote:
> On 15/02/2019 19:13, Linus Lüssing wrote:
>> On Fri, Feb 15, 2019 at 03:04:27PM +0200, Nikolay Aleksandrov wrote:
>>> Every user would expect to have traffic forwarded only to the configured
>>> mdb destination when snooping is enabled, instead now to get that one
>>> needs to enable both snooping and querier. Enabling querier on all
>>> switches could be problematic and is not a good solution,
>>
>> There is no need to set the querier on all snooping switches.
>> br_multicast_querier_exists() checks if a querier exists on the
>> link in general, not if this particular host/bridge is a querier.
>>
> 
> We need a generic solution for the case of existing mdst and no querier.
> More below.
> 
>>
>>> for example as summarized by our multicast experts:
>>> "every switch would send an IGMP query
>>
>> What? RFC3810, section 7.1 says:
>>
>> "If it is the case, a querier election mechanism (described in
>>  section 7.6.2) is used to elect a single multicast router to be
>>  in Querier state. [...] Nevertheless, it is only the [elected] Querier
>>  that sends periodical or triggered query messages on the subnet."
>>>> for any random multicast traffic it
>>> received across the entire domain and it would send it forever as long as a
>>> host exists wanting that stream even if it has no downstream/directly
>>> connected receivers"
>>
> 
> This was taken out of context and it's my bad, I think everyone is aware
> of the election process, please nevermind the above statement.
> 
> [snip]> 
>>
>> Have you done some tests with this change yet, Nikolay?
>>
> 
> You've raised good questions, IPv6 indeed needs more work - we'll have to flood
> link-local packets etc. but I wanted to have a discussion about no querier/existing mdst.
> To simplify we can modify the patch and have traffic forwarded to the proper ports when an
> mdst exists and there is no querier for both unsolicited report and user-added entry.

To add a bit more:
"- no querier exists on the link
- one port gets an unsolicited MLD report, i.e. because a host has just
  started to listen to a particular multicast address
=> will only this port receive multicast traffic? what happens to
   other ports that have listeners for the same multicast group?"

Correct, only the interested ports (where reports have been seen or the user has
added them) will get the traffic. We could also consider having this only
for user-added mdsts, I'll have to think more about that.

> We can keep the current behaviour for unknown traffic with and without querier.
> This would align it closer to what other vendors currently do as well IIRC.
> What do you think ?
> 
> Thanks,
>  Nik
> 

Re: [PATCH RFC] net: bridge: don't flood known multicast traffic when snooping is enabled

[Ido Schimmel]

On Sat, Feb 16, 2019 at 10:05:40AM +0200, Nikolay Aleksandrov wrote:
> On 15/02/2019 19:13, Linus Lüssing wrote:
> > On Fri, Feb 15, 2019 at 03:04:27PM +0200, Nikolay Aleksandrov wrote:
> >> Every user would expect to have traffic forwarded only to the configured
> >> mdb destination when snooping is enabled, instead now to get that one
> >> needs to enable both snooping and querier. Enabling querier on all
> >> switches could be problematic and is not a good solution,
> > 
> > There is no need to set the querier on all snooping switches.
> > br_multicast_querier_exists() checks if a querier exists on the
> > link in general, not if this particular host/bridge is a querier.
> > 
> 
> We need a generic solution for the case of existing mdst and no querier.
> More below.
> 
> > 
> >> for example as summarized by our multicast experts:
> >> "every switch would send an IGMP query
> > 
> > What? RFC3810, section 7.1 says:
> > 
> > "If it is the case, a querier election mechanism (described in
> >  section 7.6.2) is used to elect a single multicast router to be
> >  in Querier state. [...] Nevertheless, it is only the [elected] Querier
> >  that sends periodical or triggered query messages on the subnet."
> > >> for any random multicast traffic it
> >> received across the entire domain and it would send it forever as long as a
> >> host exists wanting that stream even if it has no downstream/directly
> >> connected receivers"
> > 
> 
> This was taken out of context and it's my bad, I think everyone is aware
> of the election process, please nevermind the above statement.
> 
> [snip]> 
> > 
> > Have you done some tests with this change yet, Nikolay?
> > 
> 
> You've raised good questions, IPv6 indeed needs more work - we'll have to flood
> link-local packets etc. but I wanted to have a discussion about no querier/existing mdst.
> To simplify we can modify the patch and have traffic forwarded to the proper ports when an
> mdst exists and there is no querier for both unsolicited report and user-added entry.
> We can keep the current behaviour for unknown traffic with and without querier.
> This would align it closer to what other vendors currently do as well IIRC.
> What do you think ?

The no querier condition is not currently reflected via switchdev, so
the behavior you're proposing in your patch is what actually happens in
the data plane.

We already hit the problem Linus mentioned in commit b00589af3b04
("bridge: disable snooping if there is no querier"). Namely, IPv6 ND
broke because a port joined before the bridge was created.

I introduced a workaround in commit 9d45deb04c59 ("mlxsw: spectrum:
Treat IPv6 unregistered multicast as broadcast"). I'm interested to know
what other vendors are doing. Can you elaborate?

We can trap IPv6 ND packets at L2 (we'll eventually need to do for ND
suppression) and let the bridge take care of flooding them correctly.
I'm not sure it's good enough.

Re: [PATCH RFC] net: bridge: don't flood known multicast traffic when snooping is enabled

[nikolay]

On 16 February 2019 20:43:53 EET, Ido Schimmel <idosch@idosch.org> wrote:
>On Sat, Feb 16, 2019 at 10:05:40AM +0200, Nikolay Aleksandrov wrote:
>> On 15/02/2019 19:13, Linus Lüssing wrote:
>> > On Fri, Feb 15, 2019 at 03:04:27PM +0200, Nikolay Aleksandrov
>wrote:
>> >> Every user would expect to have traffic forwarded only to the
>configured
>> >> mdb destination when snooping is enabled, instead now to get that
>one
>> >> needs to enable both snooping and querier. Enabling querier on all
>> >> switches could be problematic and is not a good solution,
>> > 
>> > There is no need to set the querier on all snooping switches.
>> > br_multicast_querier_exists() checks if a querier exists on the
>> > link in general, not if this particular host/bridge is a querier.
>> > 
>> 
>> We need a generic solution for the case of existing mdst and no
>querier.
>> More below.
>> 
>> > 
>> >> for example as summarized by our multicast experts:
>> >> "every switch would send an IGMP query
>> > 
>> > What? RFC3810, section 7.1 says:
>> > 
>> > "If it is the case, a querier election mechanism (described in
>> >  section 7.6.2) is used to elect a single multicast router to be
>> >  in Querier state. [...] Nevertheless, it is only the [elected]
>Querier
>> >  that sends periodical or triggered query messages on the subnet."
>> > >> for any random multicast traffic it
>> >> received across the entire domain and it would send it forever as
>long as a
>> >> host exists wanting that stream even if it has no
>downstream/directly
>> >> connected receivers"
>> > 
>> 
>> This was taken out of context and it's my bad, I think everyone is
>aware
>> of the election process, please nevermind the above statement.
>> 
>> [snip]> 
>> > 
>> > Have you done some tests with this change yet, Nikolay?
>> > 
>> 
>> You've raised good questions, IPv6 indeed needs more work - we'll
>have to flood
>> link-local packets etc. but I wanted to have a discussion about no
>querier/existing mdst.
>> To simplify we can modify the patch and have traffic forwarded to the
>proper ports when an
>> mdst exists and there is no querier for both unsolicited report and
>user-added entry.
>> We can keep the current behaviour for unknown traffic with and
>without querier.
>> This would align it closer to what other vendors currently do as well
>IIRC.
>> What do you think ?
>
>The no querier condition is not currently reflected via switchdev, so
>the behavior you're proposing in your patch is what actually happens in
>the data plane.
>
>We already hit the problem Linus mentioned in commit b00589af3b04
>("bridge: disable snooping if there is no querier"). Namely, IPv6 ND
>broke because a port joined before the bridge was created.
>
>I introduced a workaround in commit 9d45deb04c59 ("mlxsw: spectrum:
>Treat IPv6 unregistered multicast as broadcast"). I'm interested to
>know
>what other vendors are doing. Can you elaborate?
>

Exactly like your fix. :) Flood it, this patch unfortunately breaks it 
because of mrouters flag, but we can retain the behaviour
by forwarding only known mdsts to their ports and flooding
unregistered mcast when there is no querier. I think that is 
what others do by default too, actually I think they flood with querier as well. Maybe unknown mcast flooding should be controlled by a flag when a querier is present
because we've had this behaviour for a long time, personally I'd have it on
by default. 
I am currently away and will be able to prepare a rfc patch after the weekend. 


>We can trap IPv6 ND packets at L2 (we'll eventually need to do for ND
>suppression) and let the bridge take care of flooding them correctly.
>I'm not sure it's good enough.

Re: [PATCH RFC] net: bridge: don't flood known multicast traffic when snooping is enabled

[nikolay]

On 16 February 2019 21:15:21 EET, nikolay@cumulusnetworks.com wrote:
>On 16 February 2019 20:43:53 EET, Ido Schimmel <idosch@idosch.org>
>wrote:
>>On Sat, Feb 16, 2019 at 10:05:40AM +0200, Nikolay Aleksandrov wrote:
>>> On 15/02/2019 19:13, Linus Lüssing wrote:
>>> > On Fri, Feb 15, 2019 at 03:04:27PM +0200, Nikolay Aleksandrov
>>wrote:
>>> >> Every user would expect to have traffic forwarded only to the
>>configured
>>> >> mdb destination when snooping is enabled, instead now to get that
>>one
>>> >> needs to enable both snooping and querier. Enabling querier on
>all
>>> >> switches could be problematic and is not a good solution,
>>> > 
>>> > There is no need to set the querier on all snooping switches.
>>> > br_multicast_querier_exists() checks if a querier exists on the
>>> > link in general, not if this particular host/bridge is a querier.
>>> > 
>>> 
>>> We need a generic solution for the case of existing mdst and no
>>querier.
>>> More below.
>>> 
>>> > 
>>> >> for example as summarized by our multicast experts:
>>> >> "every switch would send an IGMP query
>>> > 
>>> > What? RFC3810, section 7.1 says:
>>> > 
>>> > "If it is the case, a querier election mechanism (described in
>>> >  section 7.6.2) is used to elect a single multicast router to be
>>> >  in Querier state. [...] Nevertheless, it is only the [elected]
>>Querier
>>> >  that sends periodical or triggered query messages on the subnet."
>>> > >> for any random multicast traffic it
>>> >> received across the entire domain and it would send it forever as
>>long as a
>>> >> host exists wanting that stream even if it has no
>>downstream/directly
>>> >> connected receivers"
>>> > 
>>> 
>>> This was taken out of context and it's my bad, I think everyone is
>>aware
>>> of the election process, please nevermind the above statement.
>>> 
>>> [snip]> 
>>> > 
>>> > Have you done some tests with this change yet, Nikolay?
>>> > 
>>> 
>>> You've raised good questions, IPv6 indeed needs more work - we'll
>>have to flood
>>> link-local packets etc. but I wanted to have a discussion about no
>>querier/existing mdst.
>>> To simplify we can modify the patch and have traffic forwarded to
>the
>>proper ports when an
>>> mdst exists and there is no querier for both unsolicited report and
>>user-added entry.
>>> We can keep the current behaviour for unknown traffic with and
>>without querier.
>>> This would align it closer to what other vendors currently do as
>well
>>IIRC.
>>> What do you think ?
>>
>>The no querier condition is not currently reflected via switchdev, so
>>the behavior you're proposing in your patch is what actually happens
>in
>>the data plane.
>>
>>We already hit the problem Linus mentioned in commit b00589af3b04
>>("bridge: disable snooping if there is no querier"). Namely, IPv6 ND
>>broke because a port joined before the bridge was created.
>>
>>I introduced a workaround in commit 9d45deb04c59 ("mlxsw: spectrum:
>>Treat IPv6 unregistered multicast as broadcast"). I'm interested to
>>know
>>what other vendors are doing. Can you elaborate?
>>
>
>Exactly like your fix. :) Flood it, this patch unfortunately breaks it 
>because of mrouters flag, but we can retain the behaviour
>by forwarding only known mdsts to their ports and flooding
>unregistered mcast when there is no querier. I think that is 
>what others do by default too, actually I think they flood with querier
>as well. Maybe unknown mcast flooding should be controlled by a flag
>when a querier is present
>because we've had this behaviour for a long time, personally I'd have
>it on
>by default. 

Ugh, mispoke please read the above statement to be only about no querier. 
I meant flooding v6 link-local always. 

>I am currently away and will be able to prepare a rfc patch after the
>weekend. 
>
>
>>We can trap IPv6 ND packets at L2 (we'll eventually need to do for ND
>>suppression) and let the bridge take care of flooding them correctly.
>>I'm not sure it's good enough.


-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH RFC] net: bridge: don't flood known multicast traffic when snooping is enabled

[Linus Lüssing]

Hi Nik, hi Ido,

By the way speaking about the IGMP/MLD querier mechanism. Not sure if
you are subscribed to the pim@ietf.org or mcast-wifi@ietf.org mailing lists.

There was a call for volunteers to progress IGMP/MLD to standards
track not that long ago:

"[pim] Volunteers needed for work on progressing IGMP/MLD on the standards track"
https://mailarchive.ietf.org/arch/msg/pim/F5y_w6jnrtzBpEiC7F_TxBv9PXI

I had voiced some concerns regarding the IGMPv2/MLDv1 report
suppression and the querier mechanism and their unfavourable
implications, as well as some issues (or ambiguities at least) with RFC4541:

https://mailarchive.ietf.org/arch/msg/pim/DWWYpnsZsbqHtafwYZ_swhDpbcQ
https://mailarchive.ietf.org/arch/msg/pim/omI4-pg-12vO88YuZcRP5TCz4wY


It seems that some bigger vendors had gathered for a phone call:

https://mailarchive.ietf.org/arch/msg/pim/jPAbf-PHgtgpwe8lgyjOzCHlhc0

But I haven't heard anything back from that meeting on those
lists.


Just wanted to forward this, in case you guys might maybe be
interested to participate there in some way. :-)

Cheers, Linus

Re: [PATCH RFC] net: bridge: don't flood known multicast traffic when snooping is enabled

[Linus Lüssing]

On Sat, Feb 16, 2019 at 09:27:26PM +0200, nikolay@cumulusnetworks.com wrote:
> >>The no querier condition is not currently reflected via switchdev, so
> >>the behavior you're proposing in your patch is what actually happens
> >in
> >>the data plane.
> >>
> >>We already hit the problem Linus mentioned in commit b00589af3b04
> >>("bridge: disable snooping if there is no querier"). Namely, IPv6 ND
> >>broke because a port joined before the bridge was created.
> >>
> >>I introduced a workaround in commit 9d45deb04c59 ("mlxsw: spectrum:
> >>Treat IPv6 unregistered multicast as broadcast"). I'm interested to
> >>know
> >>what other vendors are doing. Can you elaborate?
> >>
> >
> >Exactly like your fix. :) Flood it, this patch unfortunately breaks it 
> >because of mrouters flag, but we can retain the behaviour
> >by forwarding only known mdsts to their ports and flooding
> >unregistered mcast when there is no querier. I think that is 
> >what others do by default too, actually I think they flood with querier
> >as well. Maybe unknown mcast flooding should be controlled by a flag
> >when a querier is present
> >because we've had this behaviour for a long time, personally I'd have
> >it on
> >by default. 
> 
> Ugh, mispoke please read the above statement to be only about no querier. 
> I meant flooding v6 link-local always. 

And for routable IPv6 multicast, how would you detect multicast
listeners on the local link in absence of a querier?

Re: [PATCH RFC] net: bridge: don't flood known multicast traffic when snooping is enabled

[Florian Fainelli]

On 2/15/2019 5:04 AM, Nikolay Aleksandrov wrote:
> The behaviour since b00589af3b04 ("bridge: disable snooping if there is
> no querier") is wrong, we shouldn't be flooding multicast traffic when
> there is an mdb entry and we know where it should be forwarded to when
> multicast snooping is enabled. This patch changes the behaviour to not
> flood known unicast traffic.

You mean multicast traffic in the last part of the sentence, right?

> I'll give two obviously broken cases:
>  - most obvious: static mdb created by the user with snooping enabled
>  - user-space daemon controlling the mdb table (e.g. MLAG)
> 
> Every user would expect to have traffic forwarded only to the configured
> mdb destination when snooping is enabled, instead now to get that one
> needs to enable both snooping and querier. Enabling querier on all
> switches could be problematic and is not a good solution, for example
> as summarized by our multicast experts:
> "every switch would send an IGMP query for any random multicast traffic it
> received across the entire domain and it would send it forever as long as a
> host exists wanting that stream even if it has no downstream/directly
> connected receivers"
> 
> Sending as an RFC to get the discussion going, but I'm strongly for
> removing this behaviour and would like to send this patch officially.
> 
> We could make this behaviour possible via a knob if necessary, but
> it really should not be the default.
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> ---
>  net/bridge/br_device.c | 3 +--
>  net/bridge/br_input.c  | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 013323b6dbe4..2aa8a6509924 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -96,8 +96,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>  		}
>  
>  		mdst = br_mdb_get(br, skb, vid);
> -		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
> -		    br_multicast_querier_exists(br, eth_hdr(skb)))
> +		if (mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb))
>  			br_multicast_flood(mdst, skb, false, true);
>  		else
>  			br_flood(br, skb, BR_PKT_MULTICAST, false, true);
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 5ea7e56119c1..aae78095cf67 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -136,8 +136,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  	switch (pkt_type) {
>  	case BR_PKT_MULTICAST:
>  		mdst = br_mdb_get(br, skb, vid);
> -		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
> -		    br_multicast_querier_exists(br, eth_hdr(skb))) {
> +		if (mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) {
>  			if ((mdst && mdst->host_joined) ||
>  			    br_multicast_is_router(br)) {
>  				local_rcv = true;
> 

-- 
Florian

Re: [PATCH RFC] net: bridge: don't flood known multicast traffic when snooping is enabled

[Nikolay Aleksandrov]

On 17/02/2019 05:05, Florian Fainelli wrote:
> 
> 
> On 2/15/2019 5:04 AM, Nikolay Aleksandrov wrote:
>> The behaviour since b00589af3b04 ("bridge: disable snooping if there is
>> no querier") is wrong, we shouldn't be flooding multicast traffic when
>> there is an mdb entry and we know where it should be forwarded to when
>> multicast snooping is enabled. This patch changes the behaviour to not
>> flood known unicast traffic.
> 
> You mean multicast traffic in the last part of the sentence, right?
> 

Right. The change I wanted to discuss is when there is no querier and we have
a known mdst - forward only to its registered ports. The rest of the traffic
follows the current rules (i.e. no querier - flood unknown mcast). I'll send
an updated RFC version since this patch has issues as noted in the discussion
and we can continue from there.

>> I'll give two obviously broken cases:
>>  - most obvious: static mdb created by the user with snooping enabled
>>  - user-space daemon controlling the mdb table (e.g. MLAG)
>>
>> Every user would expect to have traffic forwarded only to the configured
>> mdb destination when snooping is enabled, instead now to get that one
>> needs to enable both snooping and querier. Enabling querier on all
>> switches could be problematic and is not a good solution, for example
>> as summarized by our multicast experts:
>> "every switch would send an IGMP query for any random multicast traffic it
>> received across the entire domain and it would send it forever as long as a
>> host exists wanting that stream even if it has no downstream/directly
>> connected receivers"
>>
>> Sending as an RFC to get the discussion going, but I'm strongly for
>> removing this behaviour and would like to send this patch officially.
>>
>> We could make this behaviour possible via a knob if necessary, but
>> it really should not be the default.
>>
>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> ---
>>  net/bridge/br_device.c | 3 +--
>>  net/bridge/br_input.c  | 3 +--
>>  2 files changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
>> index 013323b6dbe4..2aa8a6509924 100644
>> --- a/net/bridge/br_device.c
>> +++ b/net/bridge/br_device.c
>> @@ -96,8 +96,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>>  		}
>>  
>>  		mdst = br_mdb_get(br, skb, vid);
>> -		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
>> -		    br_multicast_querier_exists(br, eth_hdr(skb)))
>> +		if (mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb))
>>  			br_multicast_flood(mdst, skb, false, true);
>>  		else
>>  			br_flood(br, skb, BR_PKT_MULTICAST, false, true);
>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>> index 5ea7e56119c1..aae78095cf67 100644
>> --- a/net/bridge/br_input.c
>> +++ b/net/bridge/br_input.c
>> @@ -136,8 +136,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>  	switch (pkt_type) {
>>  	case BR_PKT_MULTICAST:
>>  		mdst = br_mdb_get(br, skb, vid);
>> -		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
>> -		    br_multicast_querier_exists(br, eth_hdr(skb))) {
>> +		if (mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) {
>>  			if ((mdst && mdst->host_joined) ||
>>  			    br_multicast_is_router(br)) {
>>  				local_rcv = true;
>>
> 

[RFC v2] net: bridge: don't flood known multicast traffic when snooping is enabled

[Nikolay Aleksandrov]

This is v2 of the RFC patch which aims to forward packets to known
mdsts' ports only (the no querier case). After v1 I've kept
the previous behaviour when it comes to unregistered traffic or when
a querier is present. All of this is of course only with snooping
enabled. So with this patch the following changes should occur:
 - No querier: forward known mdst traffic to its registered ports,
               no change about unknown mcast (flood)
 - Querier present: no change

The reason to do this is simple - we want to respect the user's mdb
configuration in both cases, that is if the user adds static mdb entries
manually then we should use that information about forwarding traffic.

What do you think ?

* Notes
Traffic that is currently marked as mrouters_only:
 - IPv4: non-local mcast traffic, igmp reports
 - IPv6: non-all-nodes-dst mcast traffic, mldv1 reports

Simple use case:
 $ echo 1 > /sys/class/net/bridge/bridge/multicast_snooping
 $ bridge mdb add dev bridge port swp1 grp 239.0.0.1
 - without a querier currently traffic for 239.0.0.1 will still be flooded,
   with this change it will be forwarded only to swp1

Ido, I know this doesn't solve the issue you brought up, maybe we should
have a separate discussion about acting on querier changes in the switch
driver or alternative solutions (e.g. always-flood-unknown-mcast knob).
Perhaps the bridge can notify the drivers on querier state changes.

This patch is meant about discussing the best way to solve the issue,
it's not thoroughly tested, in case we settle about the details I'll run
more tests.

Thanks,

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/bridge/br_device.c | 5 +++--
 net/bridge/br_input.c  | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 013323b6dbe4..e8c01409a7e7 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -96,8 +96,9 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 		}
 
 		mdst = br_mdb_get(br, skb, vid);
-		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
-		    br_multicast_querier_exists(br, eth_hdr(skb)))
+		if (mdst ||
+		    (BR_INPUT_SKB_CB_MROUTERS_ONLY(skb) &&
+		     br_multicast_querier_exists(br, eth_hdr(skb))))
 			br_multicast_flood(mdst, skb, false, true);
 		else
 			br_flood(br, skb, BR_PKT_MULTICAST, false, true);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 5ea7e56119c1..8777566f7b6d 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -136,8 +136,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 	switch (pkt_type) {
 	case BR_PKT_MULTICAST:
 		mdst = br_mdb_get(br, skb, vid);
-		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
-		    br_multicast_querier_exists(br, eth_hdr(skb))) {
+		if (mdst ||
+		    (BR_INPUT_SKB_CB_MROUTERS_ONLY(skb) &&
+		     br_multicast_querier_exists(br, eth_hdr(skb)))) {
 			if ((mdst && mdst->host_joined) ||
 			    br_multicast_is_router(br)) {
 				local_rcv = true;
-- 
2.20.1

Re: [RFC v2] net: bridge: don't flood known multicast traffic when snooping is enabled

[Ido Schimmel]

Hi Nik,

On Mon, Feb 18, 2019 at 02:21:07PM +0200, Nikolay Aleksandrov wrote:
> This is v2 of the RFC patch which aims to forward packets to known
> mdsts' ports only (the no querier case). After v1 I've kept
> the previous behaviour when it comes to unregistered traffic or when
> a querier is present. All of this is of course only with snooping
> enabled. So with this patch the following changes should occur:
>  - No querier: forward known mdst traffic to its registered ports,
>                no change about unknown mcast (flood)
>  - Querier present: no change
> 
> The reason to do this is simple - we want to respect the user's mdb
> configuration in both cases, that is if the user adds static mdb entries
> manually then we should use that information about forwarding traffic.
> 
> What do you think ?
> 
> * Notes
> Traffic that is currently marked as mrouters_only:
>  - IPv4: non-local mcast traffic, igmp reports
>  - IPv6: non-all-nodes-dst mcast traffic, mldv1 reports
> 
> Simple use case:
>  $ echo 1 > /sys/class/net/bridge/bridge/multicast_snooping
>  $ bridge mdb add dev bridge port swp1 grp 239.0.0.1
>  - without a querier currently traffic for 239.0.0.1 will still be flooded,
>    with this change it will be forwarded only to swp1

Looks good to me.

> 
> Ido, I know this doesn't solve the issue you brought up, maybe we should
> have a separate discussion about acting on querier changes in the switch
> driver or alternative solutions (e.g. always-flood-unknown-mcast knob).
> Perhaps the bridge can notify the drivers on querier state changes.

I think we need to reflect querier state to drivers. That's the only way
to sync the two data paths. We can flood different packet types using
different tables. The IPv6 packet types are like in the Linux bridge:

* IPv6 all-hosts (ff02::1)
* IPv6 unregistered multicast

Currently we treat both as broadcast, but in case querier is present, we
can switch the second to use the multicast flood table where we only
flood to mrouter ports.

But I wonder if we can simplify it. I believe that the main reason we
take querier state into account in the data path is because multicast is
enabled by default on the bridge. Users that would otherwise explicitly
enable it would probably make sure they also have a querier in the
network.

The workaround I did in commit 9d45deb04c59 ("mlxsw: spectrum: Treat
IPv6 unregistered multicast as broadcast") was in response to blocked
IPv6 neighbour solicitation packets sent to the solicited-node multicast
address. I now see that Russel (Cc-ed) bumped into the same problem [1].

If we trap such packets at L2 (we need it for ND suppression anyway) and
let the Linux bridge take care of flooding them correctly, will it fix
the problem? I'm basically asking if after the proposed fix we will
still have basic scenarios broken by not taking querier state into
account.

Thanks

[1] https://patchwork.ozlabs.org/patch/1043694/

> 
> This patch is meant about discussing the best way to solve the issue,
> it's not thoroughly tested, in case we settle about the details I'll run
> more tests.
> 
> Thanks,
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> ---
>  net/bridge/br_device.c | 5 +++--
>  net/bridge/br_input.c  | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 013323b6dbe4..e8c01409a7e7 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -96,8 +96,9 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>  		}
>  
>  		mdst = br_mdb_get(br, skb, vid);
> -		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
> -		    br_multicast_querier_exists(br, eth_hdr(skb)))
> +		if (mdst ||
> +		    (BR_INPUT_SKB_CB_MROUTERS_ONLY(skb) &&
> +		     br_multicast_querier_exists(br, eth_hdr(skb))))
>  			br_multicast_flood(mdst, skb, false, true);
>  		else
>  			br_flood(br, skb, BR_PKT_MULTICAST, false, true);
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 5ea7e56119c1..8777566f7b6d 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -136,8 +136,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  	switch (pkt_type) {
>  	case BR_PKT_MULTICAST:
>  		mdst = br_mdb_get(br, skb, vid);
> -		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
> -		    br_multicast_querier_exists(br, eth_hdr(skb))) {
> +		if (mdst ||
> +		    (BR_INPUT_SKB_CB_MROUTERS_ONLY(skb) &&
> +		     br_multicast_querier_exists(br, eth_hdr(skb)))) {
>  			if ((mdst && mdst->host_joined) ||
>  			    br_multicast_is_router(br)) {
>  				local_rcv = true;
> -- 
> 2.20.1
> 

Re: [RFC v2] net: bridge: don't flood known multicast traffic when snooping is enabled

[Linus Lüssing]

On Mon, Feb 18, 2019 at 02:21:07PM +0200, Nikolay Aleksandrov wrote:
> This is v2 of the RFC patch which aims to forward packets to known
> mdsts' ports only (the no querier case). After v1 I've kept
> the previous behaviour when it comes to unregistered traffic or when
> a querier is present. All of this is of course only with snooping
> enabled. So with this patch the following changes should occur:
>  - No querier: forward known mdst traffic to its registered ports,
>                no change about unknown mcast (flood)
>  - Querier present: no change
> 
> The reason to do this is simple - we want to respect the user's mdb
> configuration in both cases, that is if the user adds static mdb entries
> manually then we should use that information about forwarding traffic.
> 
> What do you think ?
> 
> * Notes
> Traffic that is currently marked as mrouters_only:
>  - IPv4: non-local mcast traffic, igmp reports
>  - IPv6: non-all-nodes-dst mcast traffic, mldv1 reports
> 
> Simple use case:
>  $ echo 1 > /sys/class/net/bridge/bridge/multicast_snooping
>  $ bridge mdb add dev bridge port swp1 grp 239.0.0.1
>  - without a querier currently traffic for 239.0.0.1 will still be flooded,
>    with this change it will be forwarded only to swp1

There is still the issue with unsolicited reports adding mdst
entries here, too. Leading to unwanted packet loss and connectivity issues.

If I understand correctly, then the goal is to give the user
slightly more control over specific, dedicated multicast addresses, right?
Could you achieve the same via netfilter? Or is that rather unfeasible
with switchdev drivers? (sorry, don't have much
knowledge/experience with switchdev yet)

Regards, Linus

Re: [Bridge] [RFC v2] net: bridge: don't flood known multicast traffic when snooping is enabled

[Linus Lüssing]

On Tue, Feb 19, 2019 at 09:57:16AM +0100, Linus Lüssing wrote:
> On Mon, Feb 18, 2019 at 02:21:07PM +0200, Nikolay Aleksandrov wrote:
> > This is v2 of the RFC patch which aims to forward packets to known
> > mdsts' ports only (the no querier case). After v1 I've kept
> > the previous behaviour when it comes to unregistered traffic or when
> > a querier is present. All of this is of course only with snooping
> > enabled. So with this patch the following changes should occur:
> >  - No querier: forward known mdst traffic to its registered ports,
> >                no change about unknown mcast (flood)
> >  - Querier present: no change
> > 
> > The reason to do this is simple - we want to respect the user's mdb
> > configuration in both cases, that is if the user adds static mdb entries
> > manually then we should use that information about forwarding traffic.
> > 
> > What do you think ?
> > 
> > * Notes
> > Traffic that is currently marked as mrouters_only:
> >  - IPv4: non-local mcast traffic, igmp reports
> >  - IPv6: non-all-nodes-dst mcast traffic, mldv1 reports
> > 
> > Simple use case:
> >  $ echo 1 > /sys/class/net/bridge/bridge/multicast_snooping
> >  $ bridge mdb add dev bridge port swp1 grp 239.0.0.1
> >  - without a querier currently traffic for 239.0.0.1 will still be flooded,
> >    with this change it will be forwarded only to swp1
> 
> There is still the issue with unsolicited reports adding mdst
> entries here, too. Leading to unwanted packet loss and connectivity issues.

Or in other words, an unsolicited report will turn a previously
unregistered multicast group into a registered one. However in the
absence of a querier the knowledge about this newly registered multicast group
will be incomplete. And therefore still needs to be flooded to avoid packet
loss.

Re: [Bridge] [RFC v2] net: bridge: don't flood known multicast traffic when snooping is enabled

[Nikolay Aleksandrov]

On 19/02/2019 11:21, Linus Lüssing wrote:
> On Tue, Feb 19, 2019 at 09:57:16AM +0100, Linus Lüssing wrote:
>> On Mon, Feb 18, 2019 at 02:21:07PM +0200, Nikolay Aleksandrov wrote:
>>> This is v2 of the RFC patch which aims to forward packets to known
>>> mdsts' ports only (the no querier case). After v1 I've kept
>>> the previous behaviour when it comes to unregistered traffic or when
>>> a querier is present. All of this is of course only with snooping
>>> enabled. So with this patch the following changes should occur:
>>>  - No querier: forward known mdst traffic to its registered ports,
>>>                no change about unknown mcast (flood)
>>>  - Querier present: no change
>>>
>>> The reason to do this is simple - we want to respect the user's mdb
>>> configuration in both cases, that is if the user adds static mdb entries
>>> manually then we should use that information about forwarding traffic.
>>>
>>> What do you think ?
>>>
>>> * Notes
>>> Traffic that is currently marked as mrouters_only:
>>>  - IPv4: non-local mcast traffic, igmp reports
>>>  - IPv6: non-all-nodes-dst mcast traffic, mldv1 reports
>>>
>>> Simple use case:
>>>  $ echo 1 > /sys/class/net/bridge/bridge/multicast_snooping
>>>  $ bridge mdb add dev bridge port swp1 grp 239.0.0.1
>>>  - without a querier currently traffic for 239.0.0.1 will still be flooded,
>>>    with this change it will be forwarded only to swp1
>>
>> There is still the issue with unsolicited reports adding mdst
>> entries here, too. Leading to unwanted packet loss and connectivity issues.
> 
> Or in other words, an unsolicited report will turn a previously
> unregistered multicast group into a registered one. However in the
> absence of a querier the knowledge about this newly registered multicast group
> will be incomplete. And therefore still needs to be flooded to avoid packet
> loss.
> 

Right, this is expected. If the user has enabled igmp snooping and doesn't have
a querier present then such behaviour is to be expected. What is surprising is
the user explicitly enabling igmp snooping, adding an mdst and then still
getting it flooded. :)
An alternative is to drop all unregistered traffic when a querier is not present.
But that will surely break setups and at best should be a configurable option that
is disabled by default.

So in effect and to try and make everybody happy we can add an option to control
this behaviour with keeping the current as default and adding the following options:
 - no querier: flood all (default, current)
 - no querier: flood unregistered, forward registered
 - no querier: drop unregistered, forward registered

Re: [Bridge] [RFC v2] net: bridge: don't flood known multicast traffic when snooping is enabled

[Linus Lüssing]

On Tue, Feb 19, 2019 at 03:31:42PM +0200, Nikolay Aleksandrov wrote:
> On 19/02/2019 11:21, Linus Lüssing wrote:
> > On Tue, Feb 19, 2019 at 09:57:16AM +0100, Linus Lüssing wrote:
> >> On Mon, Feb 18, 2019 at 02:21:07PM +0200, Nikolay Aleksandrov wrote:
> >>> This is v2 of the RFC patch which aims to forward packets to known
> >>> mdsts' ports only (the no querier case). After v1 I've kept
> >>> the previous behaviour when it comes to unregistered traffic or when
> >>> a querier is present. All of this is of course only with snooping
> >>> enabled. So with this patch the following changes should occur:
> >>>  - No querier: forward known mdst traffic to its registered ports,
> >>>                no change about unknown mcast (flood)
> >>>  - Querier present: no change
> >>>
> >>> The reason to do this is simple - we want to respect the user's mdb
> >>> configuration in both cases, that is if the user adds static mdb entries
> >>> manually then we should use that information about forwarding traffic.
> >>>
> >>> What do you think ?
> >>>
> >>> * Notes
> >>> Traffic that is currently marked as mrouters_only:
> >>>  - IPv4: non-local mcast traffic, igmp reports
> >>>  - IPv6: non-all-nodes-dst mcast traffic, mldv1 reports
> >>>
> >>> Simple use case:
> >>>  $ echo 1 > /sys/class/net/bridge/bridge/multicast_snooping
> >>>  $ bridge mdb add dev bridge port swp1 grp 239.0.0.1
> >>>  - without a querier currently traffic for 239.0.0.1 will still be flooded,
> >>>    with this change it will be forwarded only to swp1
> >>
> >> There is still the issue with unsolicited reports adding mdst
> >> entries here, too. Leading to unwanted packet loss and connectivity issues.
> > 
> > Or in other words, an unsolicited report will turn a previously
> > unregistered multicast group into a registered one. However in the
> > absence of a querier the knowledge about this newly registered multicast group
> > will be incomplete. And therefore still needs to be flooded to avoid packet
> > loss.
> > 
> 
> Right, this is expected. If the user has enabled igmp snooping and doesn't have
> a querier present then such behaviour is to be expected.

IGMP snooping is currently enabled by default. And IGMP/MLD
querier is disabled by default. I wouldn't want packet loss to be
the expected behaviour in a default setup.

This default setup currently works. However with this change it
will introduce packet loss, as you acknowledged (if I understood
you correctly?).

> What is surprising is
> the user explicitly enabling igmp snooping, adding an mdst and then still
> getting it flooded. :)

Hm, for me that does not seem surprising. I would not expect igmp
snooping to work without a querier on the link. Why don't you just
add/enable a querier in your setups then if you want to avoid
flooding?


> An alternative is to drop all unregistered traffic when a querier is not present.
> But that will surely break setups and at best should be a configurable option that
> is disabled by default.

Absolutely right. Always dropping with no querier is no option. That's why I'd say
you should always flood multicast packets if there is no querier.


> So in effect and to try and make everybody happy we can add an option to control
> this behaviour with keeping the current as default and adding the following options:
>  - no querier: flood all (default, current)

ACK

For the other options maybe I do not understand your scenario yet.
Wouldn't these two options result in eratic behaviour?

>  - no querier: flood unregistered, forward registered
>  - no querier: drop unregistered, forward registered

Let's call these two cases A) - flood unregistered, forward
registered and B) - drop unregistered, forward registered.


Let's say you have a bridge with two ports:
(1)<-[br]->(2). And no querier.

Behind (2) there is a listener for group M. M is not
registered by the bridge because either that listener joined
before the bridge came up or the entry was registered once but had
timed out. Or there was packet loss and the report did not arrive
at the bridge (for instance bc. listener is behind a wireless
connection).

For case B) we can immediately see that the listener at (2) will
not receive the traffic it signed up for. And this is a permanent
issue as the listener will not send any further reports.

Case A) is ok, the listener behind port (2) receives its traffic.


Now, a listener for M joins at (1). It sends an unsolicited
report. Group M becomes registered by the bridge. Both for
cases (A) and (B) this new listener at (1) will receive its
traffic. However, not only in case B) now, but in case A), too,
the listener at (2) will rceive no more traffic for M.


Now 260 seconds pass (multicast_membership_interval). The entry
for M times out and is deleted on the bridge. It becomes
unregistered.

Now for case (A) things would be ok again, both listeners at (1)
and (2) would receive traffic. For now - as long as no new listener
joins again.

For case (B), both the listener at port (1) and (2) will fail to
receive the traffic they signed up for.


---

I hope this illustrates a bit what I'm afraid of? If you have any
measures to prevent such behavior in your setup, I'd be curious to
know.

Regards, Linus

Re: [Bridge] [RFC v2] net: bridge: don't flood known multicast traffic when snooping is enabled

[Nikolay Aleksandrov]

On 19/02/2019 17:42, Linus Lüssing wrote:
> On Tue, Feb 19, 2019 at 03:31:42PM +0200, Nikolay Aleksandrov wrote:
>> On 19/02/2019 11:21, Linus Lüssing wrote:
>>> On Tue, Feb 19, 2019 at 09:57:16AM +0100, Linus Lüssing wrote:
>>>> On Mon, Feb 18, 2019 at 02:21:07PM +0200, Nikolay Aleksandrov wrote:
>>>>> This is v2 of the RFC patch which aims to forward packets to known
>>>>> mdsts' ports only (the no querier case). After v1 I've kept
>>>>> the previous behaviour when it comes to unregistered traffic or when
>>>>> a querier is present. All of this is of course only with snooping
>>>>> enabled. So with this patch the following changes should occur:
>>>>>  - No querier: forward known mdst traffic to its registered ports,
>>>>>                no change about unknown mcast (flood)
>>>>>  - Querier present: no change
>>>>>
>>>>> The reason to do this is simple - we want to respect the user's mdb
>>>>> configuration in both cases, that is if the user adds static mdb entries
>>>>> manually then we should use that information about forwarding traffic.
>>>>>
>>>>> What do you think ?
>>>>>
>>>>> * Notes
>>>>> Traffic that is currently marked as mrouters_only:
>>>>>  - IPv4: non-local mcast traffic, igmp reports
>>>>>  - IPv6: non-all-nodes-dst mcast traffic, mldv1 reports
>>>>>
>>>>> Simple use case:
>>>>>  $ echo 1 > /sys/class/net/bridge/bridge/multicast_snooping
>>>>>  $ bridge mdb add dev bridge port swp1 grp 239.0.0.1
>>>>>  - without a querier currently traffic for 239.0.0.1 will still be flooded,
>>>>>    with this change it will be forwarded only to swp1
>>>>
>>>> There is still the issue with unsolicited reports adding mdst
>>>> entries here, too. Leading to unwanted packet loss and connectivity issues.
>>>
>>> Or in other words, an unsolicited report will turn a previously
>>> unregistered multicast group into a registered one. However in the
>>> absence of a querier the knowledge about this newly registered multicast group
>>> will be incomplete. And therefore still needs to be flooded to avoid packet
>>> loss.
>>>
>>
>> Right, this is expected. If the user has enabled igmp snooping and doesn't have
>> a querier present then such behaviour is to be expected.
> 
> IGMP snooping is currently enabled by default. And IGMP/MLD
> querier is disabled by default. I wouldn't want packet loss to be
> the expected behaviour in a default setup.
> 
> This default setup currently works. However with this change it
> will introduce packet loss, as you acknowledged (if I understood
> you correctly?).
> 

Yeah, I'm not pushing this change anymore, the only option was to add
a tunable for the behaviour which I described in my previous reply but I
really want to avoid adding yet another bridge option as it has
way too many already.

>> What is surprising is
>> the user explicitly enabling igmp snooping, adding an mdst and then still
>> getting it flooded. :)
> 
> Hm, for me that does not seem surprising. I would not expect igmp
> snooping to work without a querier on the link. Why don't you just
> add/enable a querier in your setups then if you want to avoid
> flooding?
> 
Yep, ack.

> 
>> An alternative is to drop all unregistered traffic when a querier is not present.
>> But that will surely break setups and at best should be a configurable option that
>> is disabled by default.
> 
> Absolutely right. Always dropping with no querier is no option. That's why I'd say
> you should always flood multicast packets if there is no querier.
> 
> 
>> So in effect and to try and make everybody happy we can add an option to control
>> this behaviour with keeping the current as default and adding the following options:
>>  - no querier: flood all (default, current)
> 
> ACK
> 
> For the other options maybe I do not understand your scenario yet.
> Wouldn't these two options result in eratic behaviour?
> 
>>  - no querier: flood unregistered, forward registered
>>  - no querier: drop unregistered, forward registered
> 

To be fair I've seen all 3 options being default on different versions of
network OSes by major vendors.

> Let's call these two cases A) - flood unregistered, forward
> registered and B) - drop unregistered, forward registered.
> 
> 
> Let's say you have a bridge with two ports:
> (1)<-[br]->(2). And no querier.
> 
> Behind (2) there is a listener for group M. M is not
> registered by the bridge because either that listener joined
> before the bridge came up or the entry was registered once but had
> timed out. Or there was packet loss and the report did not arrive
> at the bridge (for instance bc. listener is behind a wireless
> connection).
> 
> For case B) we can immediately see that the listener at (2) will
> not receive the traffic it signed up for. And this is a permanent
> issue as the listener will not send any further reports.
> 
> Case A) is ok, the listener behind port (2) receives its traffic.
> 
> 
> Now, a listener for M joins at (1). It sends an unsolicited
> report. Group M becomes registered by the bridge. Both for
> cases (A) and (B) this new listener at (1) will receive its
> traffic. However, not only in case B) now, but in case A), too,
> the listener at (2) will rceive no more traffic for M.
> 
> 
> Now 260 seconds pass (multicast_membership_interval). The entry
> for M times out and is deleted on the bridge. It becomes
> unregistered.
> 
> Now for case (A) things would be ok again, both listeners at (1)
> and (2) would receive traffic. For now - as long as no new listener
> joins again.
> 
> For case (B), both the listener at port (1) and (2) will fail to
> receive the traffic they signed up for.
> 
> 
> ---
> 
> I hope this illustrates a bit what I'm afraid of? If you have any
> measures to prevent such behavior in your setup, I'd be curious to
> know.
> 

It does and I'm aware of these scenarios, but there're static mcast trees
and also such that are controlled by user-space daemons, that is the daemon
keeps refreshing the mdb entries. They're non-standard for sure and obviously
we suggest having a querier always. That's why I suggested having these
as options, but now I'm retracting that and will table this whole thing for
the time being.
I really don't want to add another option to the bridge lightly.

> Regards, Linus
> 

Thanks for the discussion and all the helpful comments,
 Nik