Linux Ethernet Bridging - Jul 2019 - [Bridge] [PATCH net] net: bridge: mcast: don't delete permanent entries when fast leave is enabled

When permanent entries were introduced by the commit below, they were
exempt from timing out and thus igmp leave wouldn't affect them unless
fast leave was enabled on the port which was added before permanent
entries existed. It shouldn't matter if fast leave is enabled or not
if the user added a permanent entry it shouldn't be deleted on igmp
leave.

Before:
$ echo 1 > /sys/class/net/eth4/brport/multicast_fast_leave
$ bridge mdb add dev br0 port eth4 grp 229.1.1.1 permanent
$ bridge mdb show
dev br0 port eth4 grp 229.1.1.1 permanent

< join and leave 229.1.1.1 on eth4 >

$ bridge mdb show
$

After:
$ echo 1 > /sys/class/net/eth4/brport/multicast_fast_leave
$ bridge mdb add dev br0 port eth4 grp 229.1.1.1 permanent
$ bridge mdb show
dev br0 port eth4 grp 229.1.1.1 permanent

< join and leave 229.1.1.1 on eth4 >

$ bridge mdb show
dev br0 port eth4 grp 229.1.1.1 permanent

Fixes: ccb1c31a7a87 ("bridge: add flags to distinguish permanent mdb
entires")
Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
---
I'll re-work this code in net-next as there's a lot of duplication.

 net/bridge/br_multicast.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 3d8deac2353d..f8cac3702712 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1388,6 +1388,9 @@ br_multicast_leave_group(struct net_bridge *br,
 			if (!br_port_group_equal(p, port, src))
 				continue;
 
+			if (p->flags & MDB_PG_FLAGS_PERMANENT)
+				break;
+
 			rcu_assign_pointer(*pp, p->next);
 			hlist_del_init(&p->mglist);
 			del_timer(&p->timer);
-- 
2.21.0

On 7/30/19 5:21 AM, Nikolay Aleksandrov wrote:
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 3d8deac2353d..f8cac3702712 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -1388,6 +1388,9 @@ br_multicast_leave_group(struct net_bridge *br,
>  			if (!br_port_group_equal(p, port, src))
>  				continue;
>  
> +			if (p->flags & MDB_PG_FLAGS_PERMANENT)
> +				break;
> +
>  			rcu_assign_pointer(*pp, p->next);
>  			hlist_del_init(&p->mglist);
>  			del_timer(&p->timer);
Why 'break' and not 'continue' like you have with
	if (!br_port_group_equal(p, port, src))

From: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
Date: Tue, 30 Jul 2019 14:21:00 +0300
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 3d8deac2353d..f8cac3702712 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -1388,6 +1388,9 @@ br_multicast_leave_group(struct net_bridge *br,
>  			if (!br_port_group_equal(p, port, src))
>  				continue;
>  
> +			if (p->flags & MDB_PG_FLAGS_PERMANENT)
> +				break;
> +
Like David, I also don't understand why this can be a break.  Is it because
permanent entries are always the last on the list?  Why will there be no
other entries that might need to be processed on the list?

On 30/07/2019 20:18, David Miller wrote:
> From: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
> Date: Tue, 30 Jul 2019 14:21:00 +0300
> 
>> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
>> index 3d8deac2353d..f8cac3702712 100644
>> --- a/net/bridge/br_multicast.c
>> +++ b/net/bridge/br_multicast.c
>> @@ -1388,6 +1388,9 @@ br_multicast_leave_group(struct net_bridge *br,
>>  			if (!br_port_group_equal(p, port, src))
>>  				continue;
>>  
>> +			if (p->flags & MDB_PG_FLAGS_PERMANENT)
>> +				break;
>> +
> 
> Like David, I also don't understand why this can be a break.  Is it
because
> permanent entries are always the last on the list?  Why will there be no
> other entries that might need to be processed on the list?
> 
The reason is that only one port can match. See the first clause of
br_port_group_equal,
that port can participate only once. We could easily add a break statement in
the end
when a match is found and it will be correct. Even in the presence of
MULTICAST_TO_UNICAST
flag, the port must match and can be added only once.

From: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
Date: Tue, 30 Jul 2019 14:21:00 +0300
> When permanent entries were introduced by the commit below, they were
> exempt from timing out and thus igmp leave wouldn't affect them unless
> fast leave was enabled on the port which was added before permanent
> entries existed. It shouldn't matter if fast leave is enabled or not
> if the user added a permanent entry it shouldn't be deleted on igmp
> leave.
 ...
> Fixes: ccb1c31a7a87 ("bridge: add flags to distinguish permanent mdb
entires")
> Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
Applied and queued up for -stable.