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