Re: [PATCH net] bridge: mdb: fix delmdb state in the notification
From: Nikolay Aleksandrov ra...@blackwall.org Date: Tue, 28 Jul 2015 13:10:44 +0200 From: Nikolay Aleksandrov niko...@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 niko...@cumulusnetworks.com Fixes: ccb1c31a7a87 (bridge: add flags to distinguish permanent mdb entires) Applied and queued up for -stable, thanks. -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net] bridge: mdb: fix delmdb state in the notification
On Tue, Jul 28, 2015 at 4:10 AM, Nikolay Aleksandrov
ra...@blackwall.org wrote:
From: Nikolay Aleksandrov niko...@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.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net] bridge: mdb: fix delmdb state in the notification
On 07/29/2015 12:38 AM, Cong Wang wrote:
On Tue, Jul 28, 2015 at 4:10 AM, Nikolay Aleksandrov
ra...@blackwall.org wrote:
From: Nikolay Aleksandrov niko...@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
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net] bridge: mdb: fix delmdb state in the notification
From: Nikolay Aleksandrov niko...@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 niko...@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 -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
