* [PATCH net] net: bridge: mcast: don't delete permanent entries when fast leave is enabled
@ 2019-07-30 11:21 ` Nikolay Aleksandrov
0 siblings, 0 replies; 14+ messages in thread
From: Nikolay Aleksandrov @ 2019-07-30 11:21 UTC (permalink / raw)
To: netdev; +Cc: davem, roopa, bridge, Nikolay Aleksandrov
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@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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Bridge] [PATCH net] net: bridge: mcast: don't delete permanent entries when fast leave is enabled
@ 2019-07-30 11:21 ` Nikolay Aleksandrov
0 siblings, 0 replies; 14+ messages in thread
From: Nikolay Aleksandrov @ 2019-07-30 11:21 UTC (permalink / raw)
To: netdev; +Cc: Nikolay Aleksandrov, roopa, bridge, davem
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@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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH net] net: bridge: mcast: don't delete permanent entries when fast leave is enabled
2019-07-30 11:21 ` [Bridge] " Nikolay Aleksandrov
@ 2019-07-30 13:58 ` David Ahern
-1 siblings, 0 replies; 14+ messages in thread
From: David Ahern @ 2019-07-30 13:58 UTC (permalink / raw)
To: Nikolay Aleksandrov, netdev; +Cc: davem, roopa, bridge
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))
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Bridge] [PATCH net] net: bridge: mcast: don't delete permanent entries when fast leave is enabled
@ 2019-07-30 13:58 ` David Ahern
0 siblings, 0 replies; 14+ messages in thread
From: David Ahern @ 2019-07-30 13:58 UTC (permalink / raw)
To: Nikolay Aleksandrov, netdev; +Cc: roopa, bridge, davem
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))
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] net: bridge: mcast: don't delete permanent entries when fast leave is enabled
2019-07-30 13:58 ` [Bridge] " David Ahern
@ 2019-07-30 14:00 ` Nikolay Aleksandrov
-1 siblings, 0 replies; 14+ messages in thread
From: Nikolay Aleksandrov @ 2019-07-30 14:00 UTC (permalink / raw)
To: David Ahern, netdev; +Cc: davem, roopa, bridge
On 30/07/2019 16:58, David Ahern wrote:
> 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))
>
Because we'll hit the goto out after this hunk always, no point in continuing
if we matched a group and it's permanent, the break might as well be a goto out.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Bridge] [PATCH net] net: bridge: mcast: don't delete permanent entries when fast leave is enabled
@ 2019-07-30 14:00 ` Nikolay Aleksandrov
0 siblings, 0 replies; 14+ messages in thread
From: Nikolay Aleksandrov @ 2019-07-30 14:00 UTC (permalink / raw)
To: David Ahern, netdev; +Cc: roopa, bridge, davem
On 30/07/2019 16:58, David Ahern wrote:
> 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))
>
Because we'll hit the goto out after this hunk always, no point in continuing
if we matched a group and it's permanent, the break might as well be a goto out.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] net: bridge: mcast: don't delete permanent entries when fast leave is enabled
2019-07-30 11:21 ` [Bridge] " Nikolay Aleksandrov
@ 2019-07-30 17:18 ` David Miller
-1 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2019-07-30 17:18 UTC (permalink / raw)
To: nikolay; +Cc: netdev, roopa, bridge
From: Nikolay Aleksandrov <nikolay@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?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Bridge] [PATCH net] net: bridge: mcast: don't delete permanent entries when fast leave is enabled
@ 2019-07-30 17:18 ` David Miller
0 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2019-07-30 17:18 UTC (permalink / raw)
To: nikolay; +Cc: netdev, roopa, bridge
From: Nikolay Aleksandrov <nikolay@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?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] net: bridge: mcast: don't delete permanent entries when fast leave is enabled
2019-07-30 17:18 ` [Bridge] " David Miller
@ 2019-07-30 17:21 ` Nikolay Aleksandrov
-1 siblings, 0 replies; 14+ messages in thread
From: Nikolay Aleksandrov @ 2019-07-30 17:21 UTC (permalink / raw)
To: David Miller; +Cc: netdev, roopa, bridge
On 30/07/2019 20:18, David Miller wrote:
> From: Nikolay Aleksandrov <nikolay@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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Bridge] [PATCH net] net: bridge: mcast: don't delete permanent entries when fast leave is enabled
@ 2019-07-30 17:21 ` Nikolay Aleksandrov
0 siblings, 0 replies; 14+ messages in thread
From: Nikolay Aleksandrov @ 2019-07-30 17:21 UTC (permalink / raw)
To: David Miller; +Cc: netdev, roopa, bridge
On 30/07/2019 20:18, David Miller wrote:
> From: Nikolay Aleksandrov <nikolay@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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] net: bridge: mcast: don't delete permanent entries when fast leave is enabled
2019-07-30 17:21 ` [Bridge] " Nikolay Aleksandrov
@ 2019-07-30 17:23 ` Nikolay Aleksandrov
-1 siblings, 0 replies; 14+ messages in thread
From: Nikolay Aleksandrov @ 2019-07-30 17:23 UTC (permalink / raw)
To: David Miller; +Cc: netdev, roopa, bridge
On 30/07/2019 20:21, Nikolay Aleksandrov wrote:
> On 30/07/2019 20:18, David Miller wrote:
>> From: Nikolay Aleksandrov <nikolay@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.
>
Like I wrote in the patch I plan to re-work that code in net-next to remove the
duplication and make it more understandable to avoid such confusions. This code
will be functionally equivalent if I put continue there, we'll just walk over
all of them even after a match or permanent are found. There can only be one
match though as I said, so walking the rest of the ports is a waste.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Bridge] [PATCH net] net: bridge: mcast: don't delete permanent entries when fast leave is enabled
@ 2019-07-30 17:23 ` Nikolay Aleksandrov
0 siblings, 0 replies; 14+ messages in thread
From: Nikolay Aleksandrov @ 2019-07-30 17:23 UTC (permalink / raw)
To: David Miller; +Cc: netdev, roopa, bridge
On 30/07/2019 20:21, Nikolay Aleksandrov wrote:
> On 30/07/2019 20:18, David Miller wrote:
>> From: Nikolay Aleksandrov <nikolay@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.
>
Like I wrote in the patch I plan to re-work that code in net-next to remove the
duplication and make it more understandable to avoid such confusions. This code
will be functionally equivalent if I put continue there, we'll just walk over
all of them even after a match or permanent are found. There can only be one
match though as I said, so walking the rest of the ports is a waste.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] net: bridge: mcast: don't delete permanent entries when fast leave is enabled
2019-07-30 11:21 ` [Bridge] " Nikolay Aleksandrov
@ 2019-07-31 23:04 ` David Miller
-1 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2019-07-31 23:04 UTC (permalink / raw)
To: nikolay; +Cc: netdev, roopa, bridge
From: Nikolay Aleksandrov <nikolay@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@cumulusnetworks.com>
Applied and queued up for -stable.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Bridge] [PATCH net] net: bridge: mcast: don't delete permanent entries when fast leave is enabled
@ 2019-07-31 23:04 ` David Miller
0 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2019-07-31 23:04 UTC (permalink / raw)
To: nikolay; +Cc: netdev, roopa, bridge
From: Nikolay Aleksandrov <nikolay@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@cumulusnetworks.com>
Applied and queued up for -stable.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-07-31 23:04 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-30 11:21 [PATCH net] net: bridge: mcast: don't delete permanent entries when fast leave is enabled Nikolay Aleksandrov
2019-07-30 11:21 ` [Bridge] " Nikolay Aleksandrov
2019-07-30 13:58 ` David Ahern
2019-07-30 13:58 ` [Bridge] " David Ahern
2019-07-30 14:00 ` Nikolay Aleksandrov
2019-07-30 14:00 ` [Bridge] " Nikolay Aleksandrov
2019-07-30 17:18 ` David Miller
2019-07-30 17:18 ` [Bridge] " David Miller
2019-07-30 17:21 ` Nikolay Aleksandrov
2019-07-30 17:21 ` [Bridge] " Nikolay Aleksandrov
2019-07-30 17:23 ` Nikolay Aleksandrov
2019-07-30 17:23 ` [Bridge] " Nikolay Aleksandrov
2019-07-31 23:04 ` David Miller
2019-07-31 23:04 ` [Bridge] " David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.