* [PATCH net] bridge: mdb: fix delmdb state in the notification
@ 2015-07-28 11:10 Nikolay Aleksandrov
2015-07-28 22:38 ` Cong Wang
2015-07-29 22:03 ` David Miller
0 siblings, 2 replies; 4+ messages in thread
From: Nikolay Aleksandrov @ 2015-07-28 11:10 UTC (permalink / raw)
To: netdev; +Cc: amwang, Nikolay Aleksandrov, cwang, bridge, davem
From: Nikolay Aleksandrov <nikolay@cumulusnetworks•com>
Since mdb states were introduced when deleting an entry the state was
left as it was set in the delete request from the user which leads to
the following output when doing a monitor (for example):
$ bridge mdb add dev br0 port eth3 grp 239.0.0.1 permanent
(monitor) dev br0 port eth3 grp 239.0.0.1 permanent
$ bridge mdb del dev br0 port eth3 grp 239.0.0.1 permanent
(monitor) dev br0 port eth3 grp 239.0.0.1 temp
^^^
Note the "temp" state in the delete notification which is wrong since
the entry was permanent, the state in a delete is always reported as
"temp" regardless of the real state of the entry.
After this patch:
$ bridge mdb add dev br0 port eth3 grp 239.0.0.1 permanent
(monitor) dev br0 port eth3 grp 239.0.0.1 permanent
$ bridge mdb del dev br0 port eth3 grp 239.0.0.1 permanent
(monitor) dev br0 port eth3 grp 239.0.0.1 permanent
There's one important note to make here that the state is actually not
matched when doing a delete, so one can delete a permanent entry by
stating "temp" in the end of the command, I've chosen this fix in order
not to break user-space tools which rely on this (incorrect) behaviour.
So to give an example after this patch and using the wrong state:
$ bridge mdb add dev br0 port eth3 grp 239.0.0.1 permanent
(monitor) dev br0 port eth3 grp 239.0.0.1 permanent
$ bridge mdb del dev br0 port eth3 grp 239.0.0.1 temp
(monitor) dev br0 port eth3 grp 239.0.0.1 permanent
Note the state of the entry that got deleted is correct in the
notification.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks•com>
Fixes: ccb1c31a7a87 ("bridge: add flags to distinguish permanent mdb entires")
---
I propose to fix the state matching in net-next but we may risk breaking
some user-space tools which rely on this behaviour.
net/bridge/br_mdb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 1198a3dbad95..c94321955db7 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -445,6 +445,7 @@ static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry)
if (p->port->state == BR_STATE_DISABLED)
goto unlock;
+ entry->state = p->state;
rcu_assign_pointer(*pp, p->next);
hlist_del_init(&p->mglist);
del_timer(&p->timer);
--
2.4.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net] bridge: mdb: fix delmdb state in the notification
2015-07-28 11:10 [PATCH net] bridge: mdb: fix delmdb state in the notification Nikolay Aleksandrov
@ 2015-07-28 22:38 ` Cong Wang
2015-07-28 22:43 ` Nikolay Aleksandrov
2015-07-29 22:03 ` David Miller
1 sibling, 1 reply; 4+ messages in thread
From: Cong Wang @ 2015-07-28 22:38 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: netdev, David Miller, bridge@lists•linux-foundation.org,
Stephen Hemminger, Cong Wang, Nikolay Aleksandrov
On Tue, Jul 28, 2015 at 4:10 AM, Nikolay Aleksandrov
<razor@blackwall•org> wrote:
> From: Nikolay Aleksandrov <nikolay@cumulusnetworks•com>
>
> Since mdb states were introduced when deleting an entry the state was
> left as it was set in the delete request from the user which leads to
> the following output when doing a monitor (for example):
> $ bridge mdb add dev br0 port eth3 grp 239.0.0.1 permanent
> (monitor) dev br0 port eth3 grp 239.0.0.1 permanent
> $ bridge mdb del dev br0 port eth3 grp 239.0.0.1 permanent
> (monitor) dev br0 port eth3 grp 239.0.0.1 temp
> ^^^
> Note the "temp" state in the delete notification which is wrong since
> the entry was permanent, the state in a delete is always reported as
> "temp" regardless of the real state of the entry.
>
Hmm?
I think it is iproute2 who forgets to set entry->state when deleting it?
} else if (strcmp(*argv, "permanent") == 0) {
if (cmd == RTM_NEWMDB)
entry.state |= MDB_PERMANENT;
Kernel simply returns what you pass to it.
Please fix iproute2.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] bridge: mdb: fix delmdb state in the notification
2015-07-28 22:38 ` Cong Wang
@ 2015-07-28 22:43 ` Nikolay Aleksandrov
0 siblings, 0 replies; 4+ messages in thread
From: Nikolay Aleksandrov @ 2015-07-28 22:43 UTC (permalink / raw)
To: Cong Wang, Nikolay Aleksandrov
Cc: netdev, bridge@lists•linux-foundation.org, David Miller,
Cong Wang
On 07/29/2015 12:38 AM, Cong Wang wrote:
> On Tue, Jul 28, 2015 at 4:10 AM, Nikolay Aleksandrov
> <razor@blackwall•org> wrote:
>> From: Nikolay Aleksandrov <nikolay@cumulusnetworks•com>
>>
>> Since mdb states were introduced when deleting an entry the state was
>> left as it was set in the delete request from the user which leads to
>> the following output when doing a monitor (for example):
>> $ bridge mdb add dev br0 port eth3 grp 239.0.0.1 permanent
>> (monitor) dev br0 port eth3 grp 239.0.0.1 permanent
>> $ bridge mdb del dev br0 port eth3 grp 239.0.0.1 permanent
>> (monitor) dev br0 port eth3 grp 239.0.0.1 temp
>> ^^^
>> Note the "temp" state in the delete notification which is wrong since
>> the entry was permanent, the state in a delete is always reported as
>> "temp" regardless of the real state of the entry.
>>
>
> Hmm?
>
> I think it is iproute2 who forgets to set entry->state when deleting it?
>
> } else if (strcmp(*argv, "permanent") == 0) {
> if (cmd == RTM_NEWMDB)
> entry.state |= MDB_PERMANENT;
>
> Kernel simply returns what you pass to it.
>
> Please fix iproute2.
>
Hi Cong,
Please read the full commit log, I've explained that the state is not honored in the kernel
so it doesn't matter if iproute2 sets the correct state that you give on the command
line, that is if I give it "temp" and the entry is permanent - it will still get
deleted and the notification will have the wrong state as "temp" because I've set
it, while this way it'll at least return the correct state of the entry being deleted.
Again I'm saying that I chose this solution over a check for the entry state because
it may break some user-space tools that rely on the behaviour that the state is
not checked in the kernel.
Cheers,
Nik
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] bridge: mdb: fix delmdb state in the notification
2015-07-28 11:10 [PATCH net] bridge: mdb: fix delmdb state in the notification Nikolay Aleksandrov
2015-07-28 22:38 ` Cong Wang
@ 2015-07-29 22:03 ` David Miller
1 sibling, 0 replies; 4+ messages in thread
From: David Miller @ 2015-07-29 22:03 UTC (permalink / raw)
To: razor; +Cc: netdev, bridge, stephen, cwang, amwang, nikolay
From: Nikolay Aleksandrov <razor@blackwall•org>
Date: Tue, 28 Jul 2015 13:10:44 +0200
> From: Nikolay Aleksandrov <nikolay@cumulusnetworks•com>
>
> Since mdb states were introduced when deleting an entry the state was
> left as it was set in the delete request from the user which leads to
> the following output when doing a monitor (for example):
> $ bridge mdb add dev br0 port eth3 grp 239.0.0.1 permanent
> (monitor) dev br0 port eth3 grp 239.0.0.1 permanent
> $ bridge mdb del dev br0 port eth3 grp 239.0.0.1 permanent
> (monitor) dev br0 port eth3 grp 239.0.0.1 temp
> ^^^
> Note the "temp" state in the delete notification which is wrong since
> the entry was permanent, the state in a delete is always reported as
> "temp" regardless of the real state of the entry.
>
> After this patch:
> $ bridge mdb add dev br0 port eth3 grp 239.0.0.1 permanent
> (monitor) dev br0 port eth3 grp 239.0.0.1 permanent
> $ bridge mdb del dev br0 port eth3 grp 239.0.0.1 permanent
> (monitor) dev br0 port eth3 grp 239.0.0.1 permanent
>
> There's one important note to make here that the state is actually not
> matched when doing a delete, so one can delete a permanent entry by
> stating "temp" in the end of the command, I've chosen this fix in order
> not to break user-space tools which rely on this (incorrect) behaviour.
>
> So to give an example after this patch and using the wrong state:
> $ bridge mdb add dev br0 port eth3 grp 239.0.0.1 permanent
> (monitor) dev br0 port eth3 grp 239.0.0.1 permanent
> $ bridge mdb del dev br0 port eth3 grp 239.0.0.1 temp
> (monitor) dev br0 port eth3 grp 239.0.0.1 permanent
>
> Note the state of the entry that got deleted is correct in the
> notification.
>
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks•com>
> Fixes: ccb1c31a7a87 ("bridge: add flags to distinguish permanent mdb entires")
Applied and queued up for -stable, thanks.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-07-29 22:03 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-28 11:10 [PATCH net] bridge: mdb: fix delmdb state in the notification Nikolay Aleksandrov
2015-07-28 22:38 ` Cong Wang
2015-07-28 22:43 ` Nikolay Aleksandrov
2015-07-29 22:03 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox