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

[Nikolay Aleksandrov <nikolay@cumulusnetworks.com>]

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