net: bridge: allow IPv6 when multicast flood is disabled

Even with multicast flooding turned off, IPv6 ND should still work so
that IPv6 connectivity is provided. Allow this by continuing to flood
multicast traffic originated by us. And similar to the unicast case,
set auto-mask if the multicast flood flag is set.

Fixes: b6cb5ac8331b ("net: bridge: add per-port multicast flood flag")
Cc: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Signed-off-by: Mike Manning <mmanning@brocade.com>
---
 include/linux/if_bridge.h | 2 +-
 net/bridge/br_forward.c   | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

> On Feb 27, 2017, at 7:11 PM, Mike Manning <mmanning@brocade.com> wrote:
> 

+CC bridge maintainers

> Even with multicast flooding turned off, IPv6 ND should still work so
> that IPv6 connectivity is provided. Allow this by continuing to flood
> multicast traffic originated by us. And similar to the unicast case,
> set auto-mask if the multicast flood flag is set.
> 
> Fixes: b6cb5ac8331b ("net: bridge: add per-port multicast flood flag")
> Cc: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> Signed-off-by: Mike Manning <mmanning@brocade.com>
> ---
> include/linux/if_bridge.h | 2 +-
> net/bridge/br_forward.c   | 3 ++-
> 2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index c5847dc..7731808 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -40,12 +40,12 @@ struct br_ip_list {
> #define BR_ADMIN_COST		BIT(4)
> #define BR_LEARNING		BIT(5)
> #define BR_FLOOD		BIT(6)
> -#define BR_AUTO_MASK		(BR_FLOOD | BR_LEARNING)
> #define BR_PROMISC		BIT(7)
> #define BR_PROXYARP		BIT(8)
> #define BR_LEARNING_SYNC	BIT(9)
> #define BR_PROXYARP_WIFI	BIT(10)
> #define BR_MCAST_FLOOD		BIT(11)
> +#define BR_AUTO_MASK		(BR_FLOOD | BR_LEARNING | BR_MCAST_FLOOD)

Quoting myself from v2 of the patch-set that introduced the mcast flood flag:
“
Actually there is one potential issue with br_auto_mask. I shouldn't change it 
because that
changes the requirement for a br_auto_port and it may change user-visible 
behaviour (e.g.
having to disable both unicast flood and multicast flood to get the same config 
for
auto ports).”

Source: http://www.mail-archive.com/netdev@vger.kernel.org/msg125724.html

> #define BR_MULTICAST_TO_UNICAST	BIT(12)
> #define BR_VLAN_TUNNEL		BIT(13)
> 
> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> index 6bfac29..7fe7d58 100644
> --- a/net/bridge/br_forward.c
> +++ b/net/bridge/br_forward.c
> @@ -186,8 +186,9 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
> 		/* 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))
> +		    !(p->flags & BR_MCAST_FLOOD) && (skb->dev != br->dev))

Please drop the extra () around the device check

> 			continue;
> 
> 		/* Do not flood to ports that enable proxy ARP */

We are aware of this and have discussed it, but I’m not sure this is the best way to fix it,
it will still allow local IPv4 mcast to be flooded on all ports even with that flag removed and
that definitely changes user-visible behaviour (even if it is okay) and will not be appropriate
for -net.

Let me get back to you on this one.

Thanks,
 Nik

On 28/02/17 09:20, Nikolay Aleksandrov wrote:
> We are aware of this and have discussed it, but I’m not sure this is the best way to fix it,
> it will still allow local IPv4 mcast to be flooded on all ports even with that flag removed and
> that definitely changes user-visible behaviour (even if it is okay) and will not be appropriate
> for -net.
> 
> Let me get back to you on this one.
> 
> Thanks,
>  Nik
> 
Thanks for your comments, I have sent a v2 patch accordingly in case you have no better suggestion.
We need per-port disabling of multicast flooding, but have to apply this patch to allow IPv6
connectivity so as to make it usable. There is no noteworthy impact on IPv4 as the fix only allows
packets originated by the device. As this feature is new to the 4.9 kernel, there are no backwards
compatibility issues with prior kernel versions if this fix is also applied to the 4.9 kernel.

On 01/03/17 11:57, Mike Manning wrote:
> On 28/02/17 09:20, Nikolay Aleksandrov wrote:
>> We are aware of this and have discussed it, but I’m not sure this is the best way to fix it,
>> it will still allow local IPv4 mcast to be flooded on all ports even with that flag removed and
>> that definitely changes user-visible behaviour (even if it is okay) and will not be appropriate
>> for -net.
>>
>> Let me get back to you on this one.
>>
>> Thanks,
>>  Nik
>>
> Thanks for your comments, I have sent a v2 patch accordingly in case you have no better suggestion.
> We need per-port disabling of multicast flooding, but have to apply this patch to allow IPv6
> connectivity so as to make it usable. There is no noteworthy impact on IPv4 as the fix only allows
> packets originated by the device. As this feature is new to the 4.9 kernel, there are no backwards
> compatibility issues with prior kernel versions if this fix is also applied to the 4.9 kernel.
> 

Okay, I agree and have also discussed it with some colleagues so this seems like the right way
to go. I'll review the v2 in a minute.

Thank you,
 Nik