Re: [PATCH] net: bridge: suppress broadcast when multicast flood is disabled

[Nikolay Aleksandrov]

On 25/04/17 16:32, Mike Manning wrote:
> On 24/04/17 20:52, Nikolay Aleksandrov wrote:
>> On 24/04/17 17:09, Mike Manning wrote:
>>> Flood suppression for packets that are not unicast needs to be handled
>>> consistently by also not flooding broadcast packets. As broadcast is a
>>> special case of multicast, the same kernel parameter should be used to
>>> suppress flooding for both of these packet types.
>>>
>>> Fixes: b6cb5ac8331b ("net: bridge: add per-port multicast flood flag")
>>> Cc: Nikolay Aleksandrov 
>>> Signed-off-by: Mike Manning 
>>> ---
>>>  net/bridge/br_forward.c | 17 ++---
>>>  1 file changed, 10 insertions(+), 7 deletions(-)
>>>
>>
>> I do not agree that this is a bug fix, the behaviour was intentional and is 
>> close to how HW
>> handles this flag. It has been like that for a few releases and changing it 
>> may impact setups
>> that use the flag since up until now they've seen the broadcast but not 
>> multicast packets and
>> suddenly their broadcast will stop.
>>
>> I think it would be better to introduce a third flag for bcast in net-next 
>> and use that to
>> filter it since that would give us the ability to program HW that can 
>> distinguish these
>> and have both options available, moreover it will not break any user setups 
>> relying on
>> the current flag behaviour and we have such setups.
>>
>> Thanks,
>>  Nik
>>
>>
> 
> Hi Nik,
> What is the usecase for flooding broadcast but not multicast please? Is the 
> lack of flood
> suppression for broadcast just something that has not been explicitly tested 
> for in those
> setups? This is the case for us, the bug raised only at this stage of the 
> release cycle.
> While adding another kernel param is an option, I would only do so if 
> absolutely necessary
> so as to avoid proliferation of params. Also to justify adding such a flag 
> for broadcast
> suppression, I would need to add a comment to explain that while broadcast is 
> a subset of
> multicast, the multicast flood suppression flag excludes broadcast.
> 
> Thanks
> Mike
> 

Hi Mike,
Stopping non-locally originating ARP requests is a pretty serious change
that affects many setups and changes the intended behaviour of this
option which was introduced specifically for unknown multicast flooding.
There're other options - you could filter the broadcast at the firewall
level, at least now you have that option but with this patch applied it
will be gone. Most network vendors differentiate the same types of
traffic as the ones listed below and allow to control them separately
which is much more flexible, I would like to keep it that way.

Currently the bridge differentiates intentionally between:
- known/unknown unicast controlled via fdbs/BR_FLOOD respectively
- known/unknown multicast controlled via mdbs/BR_MCAST_FLOOD respectively
- broadcast controlled only via firewall at this point

Fortunately the broadcast traffic doesn't have any dependent internal
state and can easily be controlled via the firewall thus rendering such
option unnecessary indeed, but I don't mind having it for completeness.
As for the comment, feel free to add it, I've actually added the exact
same comment some time ago in commit 8addd5e7d3a5 ("net: bridge: change
unicast boolean to exact pkt_type").

Cheers,
 Nik

Re: [PATCH] net: bridge: suppress broadcast when multicast flood is disabled

[Mike Manning]

On 24/04/17 20:52, Nikolay Aleksandrov wrote:
> On 24/04/17 17:09, Mike Manning wrote:
>> Flood suppression for packets that are not unicast needs to be handled
>> consistently by also not flooding broadcast packets. As broadcast is a
>> special case of multicast, the same kernel parameter should be used to
>> suppress flooding for both of these packet types.
>>
>> Fixes: b6cb5ac8331b ("net: bridge: add per-port multicast flood flag")
>> Cc: Nikolay Aleksandrov 
>> Signed-off-by: Mike Manning 
>> ---
>>  net/bridge/br_forward.c | 17 ++---
>>  1 file changed, 10 insertions(+), 7 deletions(-)
>>
> 
> I do not agree that this is a bug fix, the behaviour was intentional and is 
> close to how HW
> handles this flag. It has been like that for a few releases and changing it 
> may impact setups
> that use the flag since up until now they've seen the broadcast but not 
> multicast packets and
> suddenly their broadcast will stop.
> 
> I think it would be better to introduce a third flag for bcast in net-next 
> and use that to
> filter it since that would give us the ability to program HW that can 
> distinguish these
> and have both options available, moreover it will not break any user setups 
> relying on
> the current flag behaviour and we have such setups.
> 
> Thanks,
>  Nik
> 
> 

Hi Nik,
What is the usecase for flooding broadcast but not multicast please? Is the 
lack of flood
suppression for broadcast just something that has not been explicitly tested 
for in those
setups? This is the case for us, the bug raised only at this stage of the 
release cycle.
While adding another kernel param is an option, I would only do so if 
absolutely necessary
so as to avoid proliferation of params. Also to justify adding such a flag for 
broadcast
suppression, I would need to add a comment to explain that while broadcast is a 
subset of
multicast, the multicast flood suppression flag excludes broadcast.

Thanks
Mike

Re: [PATCH] net: bridge: suppress broadcast when multicast flood is disabled

[Nikolay Aleksandrov]

On 24/04/17 17:09, Mike Manning wrote:
> Flood suppression for packets that are not unicast needs to be handled
> consistently by also not flooding broadcast packets. As broadcast is a
> special case of multicast, the same kernel parameter should be used to
> suppress flooding for both of these packet types.
> 
> Fixes: b6cb5ac8331b ("net: bridge: add per-port multicast flood flag")
> Cc: Nikolay Aleksandrov 
> Signed-off-by: Mike Manning 
> ---
>  net/bridge/br_forward.c | 17 ++---
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 

I do not agree that this is a bug fix, the behaviour was intentional and is 
close to how HW
handles this flag. It has been like that for a few releases and changing it may 
impact setups
that use the flag since up until now they've seen the broadcast but not 
multicast packets and
suddenly their broadcast will stop.

I think it would be better to introduce a third flag for bcast in net-next and 
use that to
filter it since that would give us the ability to program HW that can 
distinguish these
and have both options available, moreover it will not break any user setups 
relying on
the current flag behaviour and we have such setups.

Thanks,
 Nik

Re: [PATCH] net: bridge: suppress broadcast when multicast flood is disabled

[Stephen Hemminger]

On Mon, 24 Apr 2017 15:09:04 +0100
Mike Manning  wrote:

> Flood suppression for packets that are not unicast needs to be handled
> consistently by also not flooding broadcast packets. As broadcast is a
> special case of multicast, the same kernel parameter should be used to
> suppress flooding for both of these packet types.
> 
> Fixes: b6cb5ac8331b ("net: bridge: add per-port multicast flood flag")
> Cc: Nikolay Aleksandrov 
> Signed-off-by: Mike Manning 
> ---

Makes sense.

Reviewed-by: Stephen Hemminger 

[PATCH] net: bridge: suppress broadcast when multicast flood is disabled

[Mike Manning]

Flood suppression for packets that are not unicast needs to be handled
consistently by also not flooding broadcast packets. As broadcast is a
special case of multicast, the same kernel parameter should be used to
suppress flooding for both of these packet types.

Fixes: b6cb5ac8331b ("net: bridge: add per-port multicast flood flag")
Cc: Nikolay Aleksandrov 
Signed-off-by: Mike Manning 
---
 net/bridge/br_forward.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index 902af6b..a61c7ad 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -183,13 +183,16 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
struct net_bridge_port *p;
 
list_for_each_entry_rcu(p, >port_list, list) {
-   /* Do not flood unicast traffic to ports that turn it off */
-   if (pkt_type == BR_PKT_UNICAST && !(p->flags & BR_FLOOD))
-   continue;
-   /* Do not flood if mc off, except for traffic we originate */
-   if (pkt_type == BR_PKT_MULTICAST &&
-   !(p->flags & BR_MCAST_FLOOD) && skb->dev != br->dev)
-   continue;
+   /* Do not flood unicast traffic to ports that turn it off, nor
+* other traffic if mc flood off except for traffic we originate
+*/
+   if (pkt_type == BR_PKT_UNICAST) {
+   if (!(p->flags & BR_FLOOD))
+   continue;
+   } else {
+   if (!(p->flags & BR_MCAST_FLOOD) && skb->dev != br->dev)
+   continue;
+   }
 
/* Do not flood to ports that enable proxy ARP */
if (p->flags & BR_PROXYARP)
-- 
2.1.4