[PATCH net v2 0/9] bridge: Fix corner case problems around local fdb entries

[PATCH net v2 0/9] bridge: Fix corner case problems around local fdb entries

[Toshiaki Makita]

There are so many corner cases that are not handled properly around local
fdb entries.
- We might fail to delete the old entry and might delete an arbitrary local
  entry when changing mac address of a bridge port.
- We always fail to delete the old entry when changing mac address of the
  bridge device.
- We might incorrectly delete a necessary entry when detaching a bridge port.
- We might incorrectly delete a necessary entry when deleting a vlan.
and so on.

This is a patch series to fix these issues.

v2:
- Change the way to find the old entry in br_fdb_changeaddr() from memorizing
  previous port address to introducing a new flag indicating whether a fdb
  entry is added by user or not, commented by Stephen Hemminger.

- Add a fix for the way to insert a new address in br_fdb_changeaddr().

- Prevent creating an entry such that its dst is NULL in br_add_if() to
  preserve old behavior, commented by Vlad Yasevich.

- Add more comments about slight behavior change, where the bridge device
  come to be able to receive traffic to an address it has during short
  window, to changelogs, commented by Vlad Yasevich.

- Add a fix for possible race in br_fdb_change_mac_address().

Toshiaki Makita (9):
  bridge: Fix the way to find old local fdb entries in br_fdb_changeaddr
  bridge: Fix the way to insert new local fdb entries in
    br_fdb_changeaddr
  bridge: Fix the way to find old local fdb entries in
    br_fdb_change_mac_address
  bridge: Change local fdb entries whenever mac address of bridge device
    changes
  bridge: Fix the way to check if a local fdb entry can be deleted
  bridge: Properly check if local fdb entry can be deleted in
    br_fdb_change_mac_address
  bridge: Properly check if local fdb entry can be deleted in
    br_fdb_delete_by_port
  bridge: Properly check if local fdb entry can be deleted when deleting
    vlan
  bridge: Prevent possible race condition in br_fdb_change_mac_address

 net/bridge/br_device.c  |   3 +-
 net/bridge/br_fdb.c     | 126 +++++++++++++++++++++++++++++++-----------------
 net/bridge/br_if.c      |   6 +--
 net/bridge/br_private.h |  11 ++++-
 net/bridge/br_stp_if.c  |   2 +
 net/bridge/br_vlan.c    |  27 ++++++++---
 6 files changed, 118 insertions(+), 57 deletions(-)

-- 
1.8.1.2

[PATCH net v2 1/9] bridge: Fix the way to find old local fdb entries in br_fdb_changeaddr

[Toshiaki Makita]

br_fdb_changeaddr() assumes that there is at most one local entry per port
per vlan. It used to be true, but since commit 36fd2b63e3b4 ("bridge: allow
creating/deleting fdb entries via netlink"), it has not been so.
Therefore, the function might fail to search a correct previous address
to be deleted and delete an arbitrary local entry if user has added local
entries manually.

Example of problematic case:
  ip link set eth0 address ee:ff:12:34:56:78
  brctl addif br0 eth0
  bridge fdb add 12:34:56:78:90:ab dev eth0 master
  ip link set eth0 address aa:bb:cc:dd:ee:ff
Then, the address 12:34:56:78:90:ab might be deleted instead of
ee:ff:12:34:56:78, the original mac address of eth0.

Address this issue by introducing a new flag, added_by_user, to struct
net_bridge_fdb_entry.

Note that br_fdb_delete_by_port() has to set added_by_user to 0 in case
like:
  ip link set eth0 address 12:34:56:78:90:ab
  ip link set eth1 address aa:bb:cc:dd:ee:ff
  brctl addif br0 eth0
  bridge fdb add aa:bb:cc:dd:ee:ff dev eth0 master
  brctl addif br0 eth1
  brctl delif br0 eth0
In this case, kernel should delete the user-added entry aa:bb:cc:dd:ee:ff,
but it also should have been added by "brctl addif br0 eth1" originally,
so we don't delete it and treat it a new kernel-created entry.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_fdb.c     | 5 ++++-
 net/bridge/br_private.h | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 33e8f23..5dab230 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -104,7 +104,7 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
 			struct net_bridge_fdb_entry *f;
 
 			f = hlist_entry(h, struct net_bridge_fdb_entry, hlist);
-			if (f->dst == p && f->is_local) {
+			if (f->dst == p && f->is_local && !f->added_by_user) {
 				/* maybe another port has same hw addr? */
 				struct net_bridge_port *op;
 				u16 vid = f->vlan_id;
@@ -247,6 +247,7 @@ void br_fdb_delete_by_port(struct net_bridge *br,
 					    ether_addr_equal(op->dev->dev_addr,
 							     f->addr.addr)) {
 						f->dst = op;
+						f->added_by_user = 0;
 						goto skip_delete;
 					}
 				}
@@ -397,6 +398,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
 		fdb->vlan_id = vid;
 		fdb->is_local = 0;
 		fdb->is_static = 0;
+		fdb->added_by_user = 0;
 		fdb->updated = fdb->used = jiffies;
 		hlist_add_head_rcu(&fdb->hlist, head);
 	}
@@ -648,6 +650,7 @@ static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
 
 		modified = true;
 	}
+	fdb->added_by_user = 1;
 
 	fdb->used = jiffies;
 	if (modified) {
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 045d56e..91fb2c2 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -104,6 +104,7 @@ struct net_bridge_fdb_entry
 	mac_addr			addr;
 	unsigned char			is_local;
 	unsigned char			is_static;
+	unsigned char			added_by_user;
 	__u16				vlan_id;
 };
 
-- 
1.8.1.2

Re: [PATCH net v2 1/9] bridge: Fix the way to find old local fdb entries in br_fdb_changeaddr

[Vlad Yasevich]

On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> br_fdb_changeaddr() assumes that there is at most one local entry per port
> per vlan. It used to be true, but since commit 36fd2b63e3b4 ("bridge: allow
> creating/deleting fdb entries via netlink"), it has not been so.
> Therefore, the function might fail to search a correct previous address
> to be deleted and delete an arbitrary local entry if user has added local
> entries manually.
> 
> Example of problematic case:
>   ip link set eth0 address ee:ff:12:34:56:78
>   brctl addif br0 eth0
>   bridge fdb add 12:34:56:78:90:ab dev eth0 master
>   ip link set eth0 address aa:bb:cc:dd:ee:ff
> Then, the address 12:34:56:78:90:ab might be deleted instead of
> ee:ff:12:34:56:78, the original mac address of eth0.
> 
> Address this issue by introducing a new flag, added_by_user, to struct
> net_bridge_fdb_entry.
> 
> Note that br_fdb_delete_by_port() has to set added_by_user to 0 in case
> like:
>   ip link set eth0 address 12:34:56:78:90:ab
>   ip link set eth1 address aa:bb:cc:dd:ee:ff
>   brctl addif br0 eth0
>   bridge fdb add aa:bb:cc:dd:ee:ff dev eth0 master
>   brctl addif br0 eth1
>   brctl delif br0 eth0
> In this case, kernel should delete the user-added entry aa:bb:cc:dd:ee:ff,
> but it also should have been added by "brctl addif br0 eth1" originally,
> so we don't delete it and treat it a new kernel-created entry.
> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>

Acked-by: Vlad Yasevich <vyasevic@redhat.com>

-vlad

> ---
>  net/bridge/br_fdb.c     | 5 ++++-
>  net/bridge/br_private.h | 1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 33e8f23..5dab230 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -104,7 +104,7 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
>  			struct net_bridge_fdb_entry *f;
>  
>  			f = hlist_entry(h, struct net_bridge_fdb_entry, hlist);
> -			if (f->dst == p && f->is_local) {
> +			if (f->dst == p && f->is_local && !f->added_by_user) {
>  				/* maybe another port has same hw addr? */
>  				struct net_bridge_port *op;
>  				u16 vid = f->vlan_id;
> @@ -247,6 +247,7 @@ void br_fdb_delete_by_port(struct net_bridge *br,
>  					    ether_addr_equal(op->dev->dev_addr,
>  							     f->addr.addr)) {
>  						f->dst = op;
> +						f->added_by_user = 0;
>  						goto skip_delete;
>  					}
>  				}
> @@ -397,6 +398,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
>  		fdb->vlan_id = vid;
>  		fdb->is_local = 0;
>  		fdb->is_static = 0;
> +		fdb->added_by_user = 0;
>  		fdb->updated = fdb->used = jiffies;
>  		hlist_add_head_rcu(&fdb->hlist, head);
>  	}
> @@ -648,6 +650,7 @@ static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
>  
>  		modified = true;
>  	}
> +	fdb->added_by_user = 1;
>  
>  	fdb->used = jiffies;
>  	if (modified) {
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 045d56e..91fb2c2 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -104,6 +104,7 @@ struct net_bridge_fdb_entry
>  	mac_addr			addr;
>  	unsigned char			is_local;
>  	unsigned char			is_static;
> +	unsigned char			added_by_user;
>  	__u16				vlan_id;
>  };
>  
> 

Re: [PATCH net v2 1/9] bridge: Fix the way to find old local fdb entries in br_fdb_changeaddr

[Vlad Yasevich]

On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> br_fdb_changeaddr() assumes that there is at most one local entry per port
> per vlan. It used to be true, but since commit 36fd2b63e3b4 ("bridge: allow
> creating/deleting fdb entries via netlink"), it has not been so.
> Therefore, the function might fail to search a correct previous address
> to be deleted and delete an arbitrary local entry if user has added local
> entries manually.
> 
> Example of problematic case:
>   ip link set eth0 address ee:ff:12:34:56:78
>   brctl addif br0 eth0
>   bridge fdb add 12:34:56:78:90:ab dev eth0 master
>   ip link set eth0 address aa:bb:cc:dd:ee:ff
> Then, the address 12:34:56:78:90:ab might be deleted instead of
> ee:ff:12:34:56:78, the original mac address of eth0.
> 
> Address this issue by introducing a new flag, added_by_user, to struct
> net_bridge_fdb_entry.
> 
> Note that br_fdb_delete_by_port() has to set added_by_user to 0 in case
> like:
>   ip link set eth0 address 12:34:56:78:90:ab
>   ip link set eth1 address aa:bb:cc:dd:ee:ff
>   brctl addif br0 eth0
>   bridge fdb add aa:bb:cc:dd:ee:ff dev eth0 master
>   brctl addif br0 eth1
>   brctl delif br0 eth0
> In this case, kernel should delete the user-added entry aa:bb:cc:dd:ee:ff,
> but it also should have been added by "brctl addif br0 eth1" originally,
> so we don't delete it and treat it a new kernel-created entry.
> 

I was looking over my patch series that adds something similar to this
and noticed that you are not handing the NTF_USE case.  That case was
always troublesome for me as it allows for 2 different way to create
the same FDB: one through br_fdb_update() and one through fdb_add_entry().

It is possible, though I haven't found any users yet, that NTF_USE
may be used and in that case, bridge will create a dynamic fdb and
disregard all NUD flags.  In case case, add_by_user will not be set
either.

I think that the above is broken and plan to submit a fix shortly.

Thanks
-vlad

> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>  net/bridge/br_fdb.c     | 5 ++++-
>  net/bridge/br_private.h | 1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 33e8f23..5dab230 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -104,7 +104,7 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
>  			struct net_bridge_fdb_entry *f;
>  
>  			f = hlist_entry(h, struct net_bridge_fdb_entry, hlist);
> -			if (f->dst == p && f->is_local) {
> +			if (f->dst == p && f->is_local && !f->added_by_user) {
>  				/* maybe another port has same hw addr? */
>  				struct net_bridge_port *op;
>  				u16 vid = f->vlan_id;
> @@ -247,6 +247,7 @@ void br_fdb_delete_by_port(struct net_bridge *br,
>  					    ether_addr_equal(op->dev->dev_addr,
>  							     f->addr.addr)) {
>  						f->dst = op;
> +						f->added_by_user = 0;
>  						goto skip_delete;
>  					}
>  				}
> @@ -397,6 +398,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
>  		fdb->vlan_id = vid;
>  		fdb->is_local = 0;
>  		fdb->is_static = 0;
> +		fdb->added_by_user = 0;
>  		fdb->updated = fdb->used = jiffies;
>  		hlist_add_head_rcu(&fdb->hlist, head);
>  	}
> @@ -648,6 +650,7 @@ static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
>  
>  		modified = true;
>  	}
> +	fdb->added_by_user = 1;
>  
>  	fdb->used = jiffies;
>  	if (modified) {
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 045d56e..91fb2c2 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -104,6 +104,7 @@ struct net_bridge_fdb_entry
>  	mac_addr			addr;
>  	unsigned char			is_local;
>  	unsigned char			is_static;
> +	unsigned char			added_by_user;
>  	__u16				vlan_id;
>  };
>  
> 

Re: [PATCH net v2 1/9] bridge: Fix the way to find old local fdb entries in br_fdb_changeaddr

[Vlad Yasevich]

On 01/03/2014 02:28 PM, Vlad Yasevich wrote:
> On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
>> br_fdb_changeaddr() assumes that there is at most one local entry per port
>> per vlan. It used to be true, but since commit 36fd2b63e3b4 ("bridge: allow
>> creating/deleting fdb entries via netlink"), it has not been so.
>> Therefore, the function might fail to search a correct previous address
>> to be deleted and delete an arbitrary local entry if user has added local
>> entries manually.
>>
>> Example of problematic case:
>>   ip link set eth0 address ee:ff:12:34:56:78
>>   brctl addif br0 eth0
>>   bridge fdb add 12:34:56:78:90:ab dev eth0 master
>>   ip link set eth0 address aa:bb:cc:dd:ee:ff
>> Then, the address 12:34:56:78:90:ab might be deleted instead of
>> ee:ff:12:34:56:78, the original mac address of eth0.
>>
>> Address this issue by introducing a new flag, added_by_user, to struct
>> net_bridge_fdb_entry.
>>
>> Note that br_fdb_delete_by_port() has to set added_by_user to 0 in case
>> like:
>>   ip link set eth0 address 12:34:56:78:90:ab
>>   ip link set eth1 address aa:bb:cc:dd:ee:ff
>>   brctl addif br0 eth0
>>   bridge fdb add aa:bb:cc:dd:ee:ff dev eth0 master
>>   brctl addif br0 eth1
>>   brctl delif br0 eth0
>> In this case, kernel should delete the user-added entry aa:bb:cc:dd:ee:ff,
>> but it also should have been added by "brctl addif br0 eth1" originally,
>> so we don't delete it and treat it a new kernel-created entry.
>>
> 
> I was looking over my patch series that adds something similar to this
> and noticed that you are not handing the NTF_USE case.  That case was
> always troublesome for me as it allows for 2 different way to create
> the same FDB: one through br_fdb_update() and one through fdb_add_entry().
> 
> It is possible, though I haven't found any users yet, that NTF_USE
> may be used and in that case, bridge will create a dynamic fdb and
> disregard all NUD flags.  In case case, add_by_user will not be set
> either.
> 
> I think that the above is broken and plan to submit a fix shortly.

Just looked again at my NTF_USE patch and while it seems ok, the whole
NTF_USE usage is racy to begin with and I am really starting to question
it's validity.

Presently, br_fdb_update() will not update local fdb entries.   Instead
it will log a misleading warning...  It will only let you update
non-local entries.  This is fine for user-created entries, but any
operation on dynamically created entries will only persist until
the next packet.  It also races against the packet, so there is
absolutely no guarantee that the values of fdb->dst and fdb->updated
will be consistent..

It seems to me that the update capability of NTF_USE would actually be
of more value on local or user-created fdb entries.

The fdb creation capability of NTF_USE should be disabled.

Thoughts?

-vlad


> 
> Thanks
> -vlad
> 
>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>> ---
>>  net/bridge/br_fdb.c     | 5 ++++-
>>  net/bridge/br_private.h | 1 +
>>  2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>> index 33e8f23..5dab230 100644
>> --- a/net/bridge/br_fdb.c
>> +++ b/net/bridge/br_fdb.c
>> @@ -104,7 +104,7 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
>>  			struct net_bridge_fdb_entry *f;
>>  
>>  			f = hlist_entry(h, struct net_bridge_fdb_entry, hlist);
>> -			if (f->dst == p && f->is_local) {
>> +			if (f->dst == p && f->is_local && !f->added_by_user) {
>>  				/* maybe another port has same hw addr? */
>>  				struct net_bridge_port *op;
>>  				u16 vid = f->vlan_id;
>> @@ -247,6 +247,7 @@ void br_fdb_delete_by_port(struct net_bridge *br,
>>  					    ether_addr_equal(op->dev->dev_addr,
>>  							     f->addr.addr)) {
>>  						f->dst = op;
>> +						f->added_by_user = 0;
>>  						goto skip_delete;
>>  					}
>>  				}
>> @@ -397,6 +398,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
>>  		fdb->vlan_id = vid;
>>  		fdb->is_local = 0;
>>  		fdb->is_static = 0;
>> +		fdb->added_by_user = 0;
>>  		fdb->updated = fdb->used = jiffies;
>>  		hlist_add_head_rcu(&fdb->hlist, head);
>>  	}
>> @@ -648,6 +650,7 @@ static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
>>  
>>  		modified = true;
>>  	}
>> +	fdb->added_by_user = 1;
>>  
>>  	fdb->used = jiffies;
>>  	if (modified) {
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 045d56e..91fb2c2 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -104,6 +104,7 @@ struct net_bridge_fdb_entry
>>  	mac_addr			addr;
>>  	unsigned char			is_local;
>>  	unsigned char			is_static;
>> +	unsigned char			added_by_user;
>>  	__u16				vlan_id;
>>  };
>>  
>>
> 

Re: [PATCH net v2 1/9] bridge: Fix the way to find old local fdb entries in br_fdb_changeaddr

[Toshiaki Makita]

On Fri, 2014-01-03 at 15:46 -0500, Vlad Yasevich wrote:
> On 01/03/2014 02:28 PM, Vlad Yasevich wrote:
> > On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> >> br_fdb_changeaddr() assumes that there is at most one local entry per port
> >> per vlan. It used to be true, but since commit 36fd2b63e3b4 ("bridge: allow
> >> creating/deleting fdb entries via netlink"), it has not been so.
> >> Therefore, the function might fail to search a correct previous address
> >> to be deleted and delete an arbitrary local entry if user has added local
> >> entries manually.
> >>
> >> Example of problematic case:
> >>   ip link set eth0 address ee:ff:12:34:56:78
> >>   brctl addif br0 eth0
> >>   bridge fdb add 12:34:56:78:90:ab dev eth0 master
> >>   ip link set eth0 address aa:bb:cc:dd:ee:ff
> >> Then, the address 12:34:56:78:90:ab might be deleted instead of
> >> ee:ff:12:34:56:78, the original mac address of eth0.
> >>
> >> Address this issue by introducing a new flag, added_by_user, to struct
> >> net_bridge_fdb_entry.
> >>
> >> Note that br_fdb_delete_by_port() has to set added_by_user to 0 in case
> >> like:
> >>   ip link set eth0 address 12:34:56:78:90:ab
> >>   ip link set eth1 address aa:bb:cc:dd:ee:ff
> >>   brctl addif br0 eth0
> >>   bridge fdb add aa:bb:cc:dd:ee:ff dev eth0 master
> >>   brctl addif br0 eth1
> >>   brctl delif br0 eth0
> >> In this case, kernel should delete the user-added entry aa:bb:cc:dd:ee:ff,
> >> but it also should have been added by "brctl addif br0 eth1" originally,
> >> so we don't delete it and treat it a new kernel-created entry.
> >>
> > 
> > I was looking over my patch series that adds something similar to this
> > and noticed that you are not handing the NTF_USE case.  That case was
> > always troublesome for me as it allows for 2 different way to create
> > the same FDB: one through br_fdb_update() and one through fdb_add_entry().
> > 
> > It is possible, though I haven't found any users yet, that NTF_USE
> > may be used and in that case, bridge will create a dynamic fdb and
> > disregard all NUD flags.  In case case, add_by_user will not be set
> > either.
> > 
> > I think that the above is broken and plan to submit a fix shortly.
> 
> Just looked again at my NTF_USE patch and while it seems ok, the whole
> NTF_USE usage is racy to begin with and I am really starting to question
> it's validity.
> 
> Presently, br_fdb_update() will not update local fdb entries.   Instead
> it will log a misleading warning...  It will only let you update
> non-local entries.  This is fine for user-created entries, but any
> operation on dynamically created entries will only persist until
> the next packet.  It also races against the packet, so there is
> absolutely no guarantee that the values of fdb->dst and fdb->updated
> will be consistent..
> 
> It seems to me that the update capability of NTF_USE would actually be
> of more value on local or user-created fdb entries.
> 
> The fdb creation capability of NTF_USE should be disabled.
> 
> Thoughts?

I ignored NTF_USE in this patch because I regard it as emulating kernel
creating entries after investigating git log.

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0c5c2d3089068d4aa378f7a40d2b5ad9d4f52ce8
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=292d1398983f3514a0eab13b7606df7f4730b498

So I think NTF_USE shouldn't set added_by_user.
And to emulate kernel creating entries, simply calling br_fdb_update()
is the right way, isn't it?

Thanks,
Toshiaki Makita

Re: [PATCH net v2 1/9] bridge: Fix the way to find old local fdb entries in br_fdb_changeaddr

[Vlad Yasevich]

On 01/05/2014 10:26 AM, Toshiaki Makita wrote:
> On Fri, 2014-01-03 at 15:46 -0500, Vlad Yasevich wrote:
>> On 01/03/2014 02:28 PM, Vlad Yasevich wrote:
>>> On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
>>>> br_fdb_changeaddr() assumes that there is at most one local entry per port
>>>> per vlan. It used to be true, but since commit 36fd2b63e3b4 ("bridge: allow
>>>> creating/deleting fdb entries via netlink"), it has not been so.
>>>> Therefore, the function might fail to search a correct previous address
>>>> to be deleted and delete an arbitrary local entry if user has added local
>>>> entries manually.
>>>>
>>>> Example of problematic case:
>>>>   ip link set eth0 address ee:ff:12:34:56:78
>>>>   brctl addif br0 eth0
>>>>   bridge fdb add 12:34:56:78:90:ab dev eth0 master
>>>>   ip link set eth0 address aa:bb:cc:dd:ee:ff
>>>> Then, the address 12:34:56:78:90:ab might be deleted instead of
>>>> ee:ff:12:34:56:78, the original mac address of eth0.
>>>>
>>>> Address this issue by introducing a new flag, added_by_user, to struct
>>>> net_bridge_fdb_entry.
>>>>
>>>> Note that br_fdb_delete_by_port() has to set added_by_user to 0 in case
>>>> like:
>>>>   ip link set eth0 address 12:34:56:78:90:ab
>>>>   ip link set eth1 address aa:bb:cc:dd:ee:ff
>>>>   brctl addif br0 eth0
>>>>   bridge fdb add aa:bb:cc:dd:ee:ff dev eth0 master
>>>>   brctl addif br0 eth1
>>>>   brctl delif br0 eth0
>>>> In this case, kernel should delete the user-added entry aa:bb:cc:dd:ee:ff,
>>>> but it also should have been added by "brctl addif br0 eth1" originally,
>>>> so we don't delete it and treat it a new kernel-created entry.
>>>>
>>>
>>> I was looking over my patch series that adds something similar to this
>>> and noticed that you are not handing the NTF_USE case.  That case was
>>> always troublesome for me as it allows for 2 different way to create
>>> the same FDB: one through br_fdb_update() and one through fdb_add_entry().
>>>
>>> It is possible, though I haven't found any users yet, that NTF_USE
>>> may be used and in that case, bridge will create a dynamic fdb and
>>> disregard all NUD flags.  In case case, add_by_user will not be set
>>> either.
>>>
>>> I think that the above is broken and plan to submit a fix shortly.
>>
>> Just looked again at my NTF_USE patch and while it seems ok, the whole
>> NTF_USE usage is racy to begin with and I am really starting to question
>> it's validity.
>>
>> Presently, br_fdb_update() will not update local fdb entries.   Instead
>> it will log a misleading warning...  It will only let you update
>> non-local entries.  This is fine for user-created entries, but any
>> operation on dynamically created entries will only persist until
>> the next packet.  It also races against the packet, so there is
>> absolutely no guarantee that the values of fdb->dst and fdb->updated
>> will be consistent..
>>
>> It seems to me that the update capability of NTF_USE would actually be
>> of more value on local or user-created fdb entries.
>>
>> The fdb creation capability of NTF_USE should be disabled.
>>
>> Thoughts?
> 
> I ignored NTF_USE in this patch because I regard it as emulating kernel
> creating entries after investigating git log.
> 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0c5c2d3089068d4aa378f7a40d2b5ad9d4f52ce8
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=292d1398983f3514a0eab13b7606df7f4730b498
> 
> So I think NTF_USE shouldn't set added_by_user.
> And to emulate kernel creating entries, simply calling br_fdb_update()
> is the right way, isn't it?

You can create dynamic entries (emulating the kernel) without NTF_USE.
Just set the NUD_REACHABLE.  Notice that arp cache only uses NTF_USE
to trigger and arp notification.  The creation is still triggered via
other netlink flags.

The more I look at this the more I think NTF_USE should not create
an entry all by itself.

-vlad

> 
> Thanks,
> Toshiaki Makita
> 

Re: [PATCH net v2 1/9] bridge: Fix the way to find old local fdb entries in br_fdb_changeaddr

[Toshiaki Makita]

On Mon, 2014-01-06 at 06:29 -0500, Vlad Yasevich wrote:
> On 01/05/2014 10:26 AM, Toshiaki Makita wrote:
> > On Fri, 2014-01-03 at 15:46 -0500, Vlad Yasevich wrote:
> >> On 01/03/2014 02:28 PM, Vlad Yasevich wrote:
> >>> On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> >>>> br_fdb_changeaddr() assumes that there is at most one local entry per port
> >>>> per vlan. It used to be true, but since commit 36fd2b63e3b4 ("bridge: allow
> >>>> creating/deleting fdb entries via netlink"), it has not been so.
> >>>> Therefore, the function might fail to search a correct previous address
> >>>> to be deleted and delete an arbitrary local entry if user has added local
> >>>> entries manually.
> >>>>
> >>>> Example of problematic case:
> >>>>   ip link set eth0 address ee:ff:12:34:56:78
> >>>>   brctl addif br0 eth0
> >>>>   bridge fdb add 12:34:56:78:90:ab dev eth0 master
> >>>>   ip link set eth0 address aa:bb:cc:dd:ee:ff
> >>>> Then, the address 12:34:56:78:90:ab might be deleted instead of
> >>>> ee:ff:12:34:56:78, the original mac address of eth0.
> >>>>
> >>>> Address this issue by introducing a new flag, added_by_user, to struct
> >>>> net_bridge_fdb_entry.
> >>>>
> >>>> Note that br_fdb_delete_by_port() has to set added_by_user to 0 in case
> >>>> like:
> >>>>   ip link set eth0 address 12:34:56:78:90:ab
> >>>>   ip link set eth1 address aa:bb:cc:dd:ee:ff
> >>>>   brctl addif br0 eth0
> >>>>   bridge fdb add aa:bb:cc:dd:ee:ff dev eth0 master
> >>>>   brctl addif br0 eth1
> >>>>   brctl delif br0 eth0
> >>>> In this case, kernel should delete the user-added entry aa:bb:cc:dd:ee:ff,
> >>>> but it also should have been added by "brctl addif br0 eth1" originally,
> >>>> so we don't delete it and treat it a new kernel-created entry.
> >>>>
> >>>
> >>> I was looking over my patch series that adds something similar to this
> >>> and noticed that you are not handing the NTF_USE case.  That case was
> >>> always troublesome for me as it allows for 2 different way to create
> >>> the same FDB: one through br_fdb_update() and one through fdb_add_entry().
> >>>
> >>> It is possible, though I haven't found any users yet, that NTF_USE
> >>> may be used and in that case, bridge will create a dynamic fdb and
> >>> disregard all NUD flags.  In case case, add_by_user will not be set
> >>> either.
> >>>
> >>> I think that the above is broken and plan to submit a fix shortly.
> >>
> >> Just looked again at my NTF_USE patch and while it seems ok, the whole
> >> NTF_USE usage is racy to begin with and I am really starting to question
> >> it's validity.
> >>
> >> Presently, br_fdb_update() will not update local fdb entries.   Instead
> >> it will log a misleading warning...  It will only let you update
> >> non-local entries.  This is fine for user-created entries, but any
> >> operation on dynamically created entries will only persist until
> >> the next packet.  It also races against the packet, so there is
> >> absolutely no guarantee that the values of fdb->dst and fdb->updated
> >> will be consistent..
> >>
> >> It seems to me that the update capability of NTF_USE would actually be
> >> of more value on local or user-created fdb entries.
> >>
> >> The fdb creation capability of NTF_USE should be disabled.
> >>
> >> Thoughts?
> > 
> > I ignored NTF_USE in this patch because I regard it as emulating kernel
> > creating entries after investigating git log.
> > 
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0c5c2d3089068d4aa378f7a40d2b5ad9d4f52ce8
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=292d1398983f3514a0eab13b7606df7f4730b498
> > 
> > So I think NTF_USE shouldn't set added_by_user.
> > And to emulate kernel creating entries, simply calling br_fdb_update()
> > is the right way, isn't it?
> 
> You can create dynamic entries (emulating the kernel) without NTF_USE.
> Just set the NUD_REACHABLE.  Notice that arp cache only uses NTF_USE
> to trigger and arp notification.  The creation is still triggered via
> other netlink flags.
> 
> The more I look at this the more I think NTF_USE should not create
> an entry all by itself.

I haven't fully understood you yet.
Currently NTF_USE behaves as if the port receives a frame and it seems
to work, though the ability to create entries is different from neigh
subsystem.
Why do you want to change the behavior?
Are you worried about inconsistency of NLM-flags/NUD-state with NTF_USE
between neigh and bridge?

Thanks,
Toshiaki Makita

Re: [PATCH net v2 1/9] bridge: Fix the way to find old local fdb entries in br_fdb_changeaddr

[Vlad Yasevich]

On 01/07/2014 07:42 AM, Toshiaki Makita wrote:
> On Mon, 2014-01-06 at 06:29 -0500, Vlad Yasevich wrote:
>> On 01/05/2014 10:26 AM, Toshiaki Makita wrote:
>>> On Fri, 2014-01-03 at 15:46 -0500, Vlad Yasevich wrote:
>>>> On 01/03/2014 02:28 PM, Vlad Yasevich wrote:
>>>>> On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
>>>>>> br_fdb_changeaddr() assumes that there is at most one local entry per port
>>>>>> per vlan. It used to be true, but since commit 36fd2b63e3b4 ("bridge: allow
>>>>>> creating/deleting fdb entries via netlink"), it has not been so.
>>>>>> Therefore, the function might fail to search a correct previous address
>>>>>> to be deleted and delete an arbitrary local entry if user has added local
>>>>>> entries manually.
>>>>>>
>>>>>> Example of problematic case:
>>>>>>   ip link set eth0 address ee:ff:12:34:56:78
>>>>>>   brctl addif br0 eth0
>>>>>>   bridge fdb add 12:34:56:78:90:ab dev eth0 master
>>>>>>   ip link set eth0 address aa:bb:cc:dd:ee:ff
>>>>>> Then, the address 12:34:56:78:90:ab might be deleted instead of
>>>>>> ee:ff:12:34:56:78, the original mac address of eth0.
>>>>>>
>>>>>> Address this issue by introducing a new flag, added_by_user, to struct
>>>>>> net_bridge_fdb_entry.
>>>>>>
>>>>>> Note that br_fdb_delete_by_port() has to set added_by_user to 0 in case
>>>>>> like:
>>>>>>   ip link set eth0 address 12:34:56:78:90:ab
>>>>>>   ip link set eth1 address aa:bb:cc:dd:ee:ff
>>>>>>   brctl addif br0 eth0
>>>>>>   bridge fdb add aa:bb:cc:dd:ee:ff dev eth0 master
>>>>>>   brctl addif br0 eth1
>>>>>>   brctl delif br0 eth0
>>>>>> In this case, kernel should delete the user-added entry aa:bb:cc:dd:ee:ff,
>>>>>> but it also should have been added by "brctl addif br0 eth1" originally,
>>>>>> so we don't delete it and treat it a new kernel-created entry.
>>>>>>
>>>>>
>>>>> I was looking over my patch series that adds something similar to this
>>>>> and noticed that you are not handing the NTF_USE case.  That case was
>>>>> always troublesome for me as it allows for 2 different way to create
>>>>> the same FDB: one through br_fdb_update() and one through fdb_add_entry().
>>>>>
>>>>> It is possible, though I haven't found any users yet, that NTF_USE
>>>>> may be used and in that case, bridge will create a dynamic fdb and
>>>>> disregard all NUD flags.  In case case, add_by_user will not be set
>>>>> either.
>>>>>
>>>>> I think that the above is broken and plan to submit a fix shortly.
>>>>
>>>> Just looked again at my NTF_USE patch and while it seems ok, the whole
>>>> NTF_USE usage is racy to begin with and I am really starting to question
>>>> it's validity.
>>>>
>>>> Presently, br_fdb_update() will not update local fdb entries.   Instead
>>>> it will log a misleading warning...  It will only let you update
>>>> non-local entries.  This is fine for user-created entries, but any
>>>> operation on dynamically created entries will only persist until
>>>> the next packet.  It also races against the packet, so there is
>>>> absolutely no guarantee that the values of fdb->dst and fdb->updated
>>>> will be consistent..
>>>>
>>>> It seems to me that the update capability of NTF_USE would actually be
>>>> of more value on local or user-created fdb entries.
>>>>
>>>> The fdb creation capability of NTF_USE should be disabled.
>>>>
>>>> Thoughts?
>>>
>>> I ignored NTF_USE in this patch because I regard it as emulating kernel
>>> creating entries after investigating git log.
>>>
>>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0c5c2d3089068d4aa378f7a40d2b5ad9d4f52ce8
>>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=292d1398983f3514a0eab13b7606df7f4730b498
>>>
>>> So I think NTF_USE shouldn't set added_by_user.
>>> And to emulate kernel creating entries, simply calling br_fdb_update()
>>> is the right way, isn't it?
>>
>> You can create dynamic entries (emulating the kernel) without NTF_USE.
>> Just set the NUD_REACHABLE.  Notice that arp cache only uses NTF_USE
>> to trigger and arp notification.  The creation is still triggered via
>> other netlink flags.
>>
>> The more I look at this the more I think NTF_USE should not create
>> an entry all by itself.
> 
> I haven't fully understood you yet.
> Currently NTF_USE behaves as if the port receives a frame and it seems
> to work, though the ability to create entries is different from neigh
> subsystem.
> Why do you want to change the behavior?
> Are you worried about inconsistency of NLM-flags/NUD-state with NTF_USE
> between neigh and bridge?

No, it is inconsistent with other NLM/NUD-state within bridge.  As
an fdb creation flag NTF_USE is confusing.  It will create an entry
without NLM_F_CREATE being set.  It will ignore NLM_F_EXCL flag as
well.  It will additionally ignore any NUD-state flags that may be set
in the netlink message.  So it may not be doing what the user wishes.

It also provides duplicate functionality.  The same results are achieved
by setting NLM_F_CREATE flag and NUD_REACHABLE state in the message.

-vlad
> 
> Thanks,
> Toshiaki Makita
> 

Re: [PATCH net v2 1/9] bridge: Fix the way to find old local fdb entries in br_fdb_changeaddr

[Toshiaki Makita]

On Tue, 2014-01-07 at 09:44 -0500, Vlad Yasevich wrote:
> On 01/07/2014 07:42 AM, Toshiaki Makita wrote:
> > On Mon, 2014-01-06 at 06:29 -0500, Vlad Yasevich wrote:
> >> On 01/05/2014 10:26 AM, Toshiaki Makita wrote:
> >>> On Fri, 2014-01-03 at 15:46 -0500, Vlad Yasevich wrote:
> >>>> On 01/03/2014 02:28 PM, Vlad Yasevich wrote:
> >>>>> On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> >>>>>> br_fdb_changeaddr() assumes that there is at most one local entry per port
> >>>>>> per vlan. It used to be true, but since commit 36fd2b63e3b4 ("bridge: allow
> >>>>>> creating/deleting fdb entries via netlink"), it has not been so.
> >>>>>> Therefore, the function might fail to search a correct previous address
> >>>>>> to be deleted and delete an arbitrary local entry if user has added local
> >>>>>> entries manually.
> >>>>>>
> >>>>>> Example of problematic case:
> >>>>>>   ip link set eth0 address ee:ff:12:34:56:78
> >>>>>>   brctl addif br0 eth0
> >>>>>>   bridge fdb add 12:34:56:78:90:ab dev eth0 master
> >>>>>>   ip link set eth0 address aa:bb:cc:dd:ee:ff
> >>>>>> Then, the address 12:34:56:78:90:ab might be deleted instead of
> >>>>>> ee:ff:12:34:56:78, the original mac address of eth0.
> >>>>>>
> >>>>>> Address this issue by introducing a new flag, added_by_user, to struct
> >>>>>> net_bridge_fdb_entry.
> >>>>>>
> >>>>>> Note that br_fdb_delete_by_port() has to set added_by_user to 0 in case
> >>>>>> like:
> >>>>>>   ip link set eth0 address 12:34:56:78:90:ab
> >>>>>>   ip link set eth1 address aa:bb:cc:dd:ee:ff
> >>>>>>   brctl addif br0 eth0
> >>>>>>   bridge fdb add aa:bb:cc:dd:ee:ff dev eth0 master
> >>>>>>   brctl addif br0 eth1
> >>>>>>   brctl delif br0 eth0
> >>>>>> In this case, kernel should delete the user-added entry aa:bb:cc:dd:ee:ff,
> >>>>>> but it also should have been added by "brctl addif br0 eth1" originally,
> >>>>>> so we don't delete it and treat it a new kernel-created entry.
> >>>>>>
> >>>>>
> >>>>> I was looking over my patch series that adds something similar to this
> >>>>> and noticed that you are not handing the NTF_USE case.  That case was
> >>>>> always troublesome for me as it allows for 2 different way to create
> >>>>> the same FDB: one through br_fdb_update() and one through fdb_add_entry().
> >>>>>
> >>>>> It is possible, though I haven't found any users yet, that NTF_USE
> >>>>> may be used and in that case, bridge will create a dynamic fdb and
> >>>>> disregard all NUD flags.  In case case, add_by_user will not be set
> >>>>> either.
> >>>>>
> >>>>> I think that the above is broken and plan to submit a fix shortly.
> >>>>
> >>>> Just looked again at my NTF_USE patch and while it seems ok, the whole
> >>>> NTF_USE usage is racy to begin with and I am really starting to question
> >>>> it's validity.
> >>>>
> >>>> Presently, br_fdb_update() will not update local fdb entries.   Instead
> >>>> it will log a misleading warning...  It will only let you update
> >>>> non-local entries.  This is fine for user-created entries, but any
> >>>> operation on dynamically created entries will only persist until
> >>>> the next packet.  It also races against the packet, so there is
> >>>> absolutely no guarantee that the values of fdb->dst and fdb->updated
> >>>> will be consistent..
> >>>>
> >>>> It seems to me that the update capability of NTF_USE would actually be
> >>>> of more value on local or user-created fdb entries.
> >>>>
> >>>> The fdb creation capability of NTF_USE should be disabled.
> >>>>
> >>>> Thoughts?
> >>>
> >>> I ignored NTF_USE in this patch because I regard it as emulating kernel
> >>> creating entries after investigating git log.
> >>>
> >>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0c5c2d3089068d4aa378f7a40d2b5ad9d4f52ce8
> >>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=292d1398983f3514a0eab13b7606df7f4730b498
> >>>
> >>> So I think NTF_USE shouldn't set added_by_user.
> >>> And to emulate kernel creating entries, simply calling br_fdb_update()
> >>> is the right way, isn't it?
> >>
> >> You can create dynamic entries (emulating the kernel) without NTF_USE.
> >> Just set the NUD_REACHABLE.  Notice that arp cache only uses NTF_USE
> >> to trigger and arp notification.  The creation is still triggered via
> >> other netlink flags.
> >>
> >> The more I look at this the more I think NTF_USE should not create
> >> an entry all by itself.
> > 
> > I haven't fully understood you yet.
> > Currently NTF_USE behaves as if the port receives a frame and it seems
> > to work, though the ability to create entries is different from neigh
> > subsystem.
> > Why do you want to change the behavior?
> > Are you worried about inconsistency of NLM-flags/NUD-state with NTF_USE
> > between neigh and bridge?
> 
> No, it is inconsistent with other NLM/NUD-state within bridge.  As
> an fdb creation flag NTF_USE is confusing.  It will create an entry
> without NLM_F_CREATE being set.  It will ignore NLM_F_EXCL flag as
> well.  It will additionally ignore any NUD-state flags that may be set
> in the netlink message.  So it may not be doing what the user wishes.

I don't know which of NTF-flags and NLM-flags/NUD-state should be given
high priority on setting. For now, in bridge, NTF_USE masks any other
flags. If this is not proper way for netlink/neighbour, I will agree
with you. If not sure, I have no motivation to change existing behavior
that might be expected by some users.

> 
> It also provides duplicate functionality.  The same results are achieved
> by setting NLM_F_CREATE flag and NUD_REACHABLE state in the message.

br_fdb_update() never updates fdb->used, which is visible by user,
unlike fdb_add_entry().

If it is duplicate functionality, isn't NTF_USE itself no use?
What can be achieved by changing capability of creation and update of
local entries?

Thanks,
Toshiaki Makita

Re: [PATCH net v2 1/9] bridge: Fix the way to find old local fdb entries in br_fdb_changeaddr

[Vlad Yasevich]

On 01/07/2014 11:33 AM, Toshiaki Makita wrote:
> On Tue, 2014-01-07 at 09:44 -0500, Vlad Yasevich wrote:
>> On 01/07/2014 07:42 AM, Toshiaki Makita wrote:
>>> On Mon, 2014-01-06 at 06:29 -0500, Vlad Yasevich wrote:
>>>> On 01/05/2014 10:26 AM, Toshiaki Makita wrote:
>>>>> On Fri, 2014-01-03 at 15:46 -0500, Vlad Yasevich wrote:
>>>>>> On 01/03/2014 02:28 PM, Vlad Yasevich wrote:
>>>>>>> On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
>>>>>>>> br_fdb_changeaddr() assumes that there is at most one local entry per port
>>>>>>>> per vlan. It used to be true, but since commit 36fd2b63e3b4 ("bridge: allow
>>>>>>>> creating/deleting fdb entries via netlink"), it has not been so.
>>>>>>>> Therefore, the function might fail to search a correct previous address
>>>>>>>> to be deleted and delete an arbitrary local entry if user has added local
>>>>>>>> entries manually.
>>>>>>>>
>>>>>>>> Example of problematic case:
>>>>>>>>   ip link set eth0 address ee:ff:12:34:56:78
>>>>>>>>   brctl addif br0 eth0
>>>>>>>>   bridge fdb add 12:34:56:78:90:ab dev eth0 master
>>>>>>>>   ip link set eth0 address aa:bb:cc:dd:ee:ff
>>>>>>>> Then, the address 12:34:56:78:90:ab might be deleted instead of
>>>>>>>> ee:ff:12:34:56:78, the original mac address of eth0.
>>>>>>>>
>>>>>>>> Address this issue by introducing a new flag, added_by_user, to struct
>>>>>>>> net_bridge_fdb_entry.
>>>>>>>>
>>>>>>>> Note that br_fdb_delete_by_port() has to set added_by_user to 0 in case
>>>>>>>> like:
>>>>>>>>   ip link set eth0 address 12:34:56:78:90:ab
>>>>>>>>   ip link set eth1 address aa:bb:cc:dd:ee:ff
>>>>>>>>   brctl addif br0 eth0
>>>>>>>>   bridge fdb add aa:bb:cc:dd:ee:ff dev eth0 master
>>>>>>>>   brctl addif br0 eth1
>>>>>>>>   brctl delif br0 eth0
>>>>>>>> In this case, kernel should delete the user-added entry aa:bb:cc:dd:ee:ff,
>>>>>>>> but it also should have been added by "brctl addif br0 eth1" originally,
>>>>>>>> so we don't delete it and treat it a new kernel-created entry.
>>>>>>>>
>>>>>>>
>>>>>>> I was looking over my patch series that adds something similar to this
>>>>>>> and noticed that you are not handing the NTF_USE case.  That case was
>>>>>>> always troublesome for me as it allows for 2 different way to create
>>>>>>> the same FDB: one through br_fdb_update() and one through fdb_add_entry().
>>>>>>>
>>>>>>> It is possible, though I haven't found any users yet, that NTF_USE
>>>>>>> may be used and in that case, bridge will create a dynamic fdb and
>>>>>>> disregard all NUD flags.  In case case, add_by_user will not be set
>>>>>>> either.
>>>>>>>
>>>>>>> I think that the above is broken and plan to submit a fix shortly.
>>>>>>
>>>>>> Just looked again at my NTF_USE patch and while it seems ok, the whole
>>>>>> NTF_USE usage is racy to begin with and I am really starting to question
>>>>>> it's validity.
>>>>>>
>>>>>> Presently, br_fdb_update() will not update local fdb entries.   Instead
>>>>>> it will log a misleading warning...  It will only let you update
>>>>>> non-local entries.  This is fine for user-created entries, but any
>>>>>> operation on dynamically created entries will only persist until
>>>>>> the next packet.  It also races against the packet, so there is
>>>>>> absolutely no guarantee that the values of fdb->dst and fdb->updated
>>>>>> will be consistent..
>>>>>>
>>>>>> It seems to me that the update capability of NTF_USE would actually be
>>>>>> of more value on local or user-created fdb entries.
>>>>>>
>>>>>> The fdb creation capability of NTF_USE should be disabled.
>>>>>>
>>>>>> Thoughts?
>>>>>
>>>>> I ignored NTF_USE in this patch because I regard it as emulating kernel
>>>>> creating entries after investigating git log.
>>>>>
>>>>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0c5c2d3089068d4aa378f7a40d2b5ad9d4f52ce8
>>>>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=292d1398983f3514a0eab13b7606df7f4730b498
>>>>>
>>>>> So I think NTF_USE shouldn't set added_by_user.
>>>>> And to emulate kernel creating entries, simply calling br_fdb_update()
>>>>> is the right way, isn't it?
>>>>
>>>> You can create dynamic entries (emulating the kernel) without NTF_USE.
>>>> Just set the NUD_REACHABLE.  Notice that arp cache only uses NTF_USE
>>>> to trigger and arp notification.  The creation is still triggered via
>>>> other netlink flags.
>>>>
>>>> The more I look at this the more I think NTF_USE should not create
>>>> an entry all by itself.
>>>
>>> I haven't fully understood you yet.
>>> Currently NTF_USE behaves as if the port receives a frame and it seems
>>> to work, though the ability to create entries is different from neigh
>>> subsystem.
>>> Why do you want to change the behavior?
>>> Are you worried about inconsistency of NLM-flags/NUD-state with NTF_USE
>>> between neigh and bridge?
>>
>> No, it is inconsistent with other NLM/NUD-state within bridge.  As
>> an fdb creation flag NTF_USE is confusing.  It will create an entry
>> without NLM_F_CREATE being set.  It will ignore NLM_F_EXCL flag as
>> well.  It will additionally ignore any NUD-state flags that may be set
>> in the netlink message.  So it may not be doing what the user wishes.
> 
> I don't know which of NTF-flags and NLM-flags/NUD-state should be given
> high priority on setting. For now, in bridge, NTF_USE masks any other
> flags. If this is not proper way for netlink/neighbour, I will agree
> with you. If not sure, I have no motivation to change existing behavior
> that might be expected by some users.
> 
>>
>> It also provides duplicate functionality.  The same results are achieved
>> by setting NLM_F_CREATE flag and NUD_REACHABLE state in the message.
> 
> br_fdb_update() never updates fdb->used, which is visible by user,
> unlike fdb_add_entry().

Thanks for pointing this out.  It looks like there are some
inconsistencies in the fdb->used markings as well.

> 
> If it is duplicate functionality, isn't NTF_USE itself no use?
> What can be achieved by changing capability of creation and update of
> local entries?

Update of local entries gives you port redirection, but doing it under
rcu.  Not sure if it really makes much sense though...

NTF_USE makes since for neighbor cache as it triggers an ARP and a
refresh of the entry.  Suppose, NTF_USE on the fdb entry should
trigger a refresh as well, but causing a create has to be explicit.

I think I'll just send my patch and we can continue this discussion
there.

-vlad
> 
> Thanks,
> Toshiaki Makita
> 

Re: [PATCH net v2 1/9] bridge: Fix the way to find old local fdb entries in br_fdb_changeaddr

[Toshiaki Makita]

On Tue, 2014-01-07 at 12:45 -0500, Vlad Yasevich wrote:
> On 01/07/2014 11:33 AM, Toshiaki Makita wrote:
> > On Tue, 2014-01-07 at 09:44 -0500, Vlad Yasevich wrote:
> >> On 01/07/2014 07:42 AM, Toshiaki Makita wrote:
> >>> On Mon, 2014-01-06 at 06:29 -0500, Vlad Yasevich wrote:
> >>>> On 01/05/2014 10:26 AM, Toshiaki Makita wrote:
> >>>>> On Fri, 2014-01-03 at 15:46 -0500, Vlad Yasevich wrote:
> >>>>>> On 01/03/2014 02:28 PM, Vlad Yasevich wrote:
> >>>>>>> On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> >>>>>>>> br_fdb_changeaddr() assumes that there is at most one local entry per port
> >>>>>>>> per vlan. It used to be true, but since commit 36fd2b63e3b4 ("bridge: allow
> >>>>>>>> creating/deleting fdb entries via netlink"), it has not been so.
> >>>>>>>> Therefore, the function might fail to search a correct previous address
> >>>>>>>> to be deleted and delete an arbitrary local entry if user has added local
> >>>>>>>> entries manually.
> >>>>>>>>
> >>>>>>>> Example of problematic case:
> >>>>>>>>   ip link set eth0 address ee:ff:12:34:56:78
> >>>>>>>>   brctl addif br0 eth0
> >>>>>>>>   bridge fdb add 12:34:56:78:90:ab dev eth0 master
> >>>>>>>>   ip link set eth0 address aa:bb:cc:dd:ee:ff
> >>>>>>>> Then, the address 12:34:56:78:90:ab might be deleted instead of
> >>>>>>>> ee:ff:12:34:56:78, the original mac address of eth0.
> >>>>>>>>
> >>>>>>>> Address this issue by introducing a new flag, added_by_user, to struct
> >>>>>>>> net_bridge_fdb_entry.
> >>>>>>>>
> >>>>>>>> Note that br_fdb_delete_by_port() has to set added_by_user to 0 in case
> >>>>>>>> like:
> >>>>>>>>   ip link set eth0 address 12:34:56:78:90:ab
> >>>>>>>>   ip link set eth1 address aa:bb:cc:dd:ee:ff
> >>>>>>>>   brctl addif br0 eth0
> >>>>>>>>   bridge fdb add aa:bb:cc:dd:ee:ff dev eth0 master
> >>>>>>>>   brctl addif br0 eth1
> >>>>>>>>   brctl delif br0 eth0
> >>>>>>>> In this case, kernel should delete the user-added entry aa:bb:cc:dd:ee:ff,
> >>>>>>>> but it also should have been added by "brctl addif br0 eth1" originally,
> >>>>>>>> so we don't delete it and treat it a new kernel-created entry.
> >>>>>>>>
> >>>>>>>
> >>>>>>> I was looking over my patch series that adds something similar to this
> >>>>>>> and noticed that you are not handing the NTF_USE case.  That case was
> >>>>>>> always troublesome for me as it allows for 2 different way to create
> >>>>>>> the same FDB: one through br_fdb_update() and one through fdb_add_entry().
> >>>>>>>
> >>>>>>> It is possible, though I haven't found any users yet, that NTF_USE
> >>>>>>> may be used and in that case, bridge will create a dynamic fdb and
> >>>>>>> disregard all NUD flags.  In case case, add_by_user will not be set
> >>>>>>> either.
> >>>>>>>
> >>>>>>> I think that the above is broken and plan to submit a fix shortly.
> >>>>>>
> >>>>>> Just looked again at my NTF_USE patch and while it seems ok, the whole
> >>>>>> NTF_USE usage is racy to begin with and I am really starting to question
> >>>>>> it's validity.
> >>>>>>
> >>>>>> Presently, br_fdb_update() will not update local fdb entries.   Instead
> >>>>>> it will log a misleading warning...  It will only let you update
> >>>>>> non-local entries.  This is fine for user-created entries, but any
> >>>>>> operation on dynamically created entries will only persist until
> >>>>>> the next packet.  It also races against the packet, so there is
> >>>>>> absolutely no guarantee that the values of fdb->dst and fdb->updated
> >>>>>> will be consistent..
> >>>>>>
> >>>>>> It seems to me that the update capability of NTF_USE would actually be
> >>>>>> of more value on local or user-created fdb entries.
> >>>>>>
> >>>>>> The fdb creation capability of NTF_USE should be disabled.
> >>>>>>
> >>>>>> Thoughts?
> >>>>>
> >>>>> I ignored NTF_USE in this patch because I regard it as emulating kernel
> >>>>> creating entries after investigating git log.
> >>>>>
> >>>>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0c5c2d3089068d4aa378f7a40d2b5ad9d4f52ce8
> >>>>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=292d1398983f3514a0eab13b7606df7f4730b498
> >>>>>
> >>>>> So I think NTF_USE shouldn't set added_by_user.
> >>>>> And to emulate kernel creating entries, simply calling br_fdb_update()
> >>>>> is the right way, isn't it?
> >>>>
> >>>> You can create dynamic entries (emulating the kernel) without NTF_USE.
> >>>> Just set the NUD_REACHABLE.  Notice that arp cache only uses NTF_USE
> >>>> to trigger and arp notification.  The creation is still triggered via
> >>>> other netlink flags.
> >>>>
> >>>> The more I look at this the more I think NTF_USE should not create
> >>>> an entry all by itself.
> >>>
> >>> I haven't fully understood you yet.
> >>> Currently NTF_USE behaves as if the port receives a frame and it seems
> >>> to work, though the ability to create entries is different from neigh
> >>> subsystem.
> >>> Why do you want to change the behavior?
> >>> Are you worried about inconsistency of NLM-flags/NUD-state with NTF_USE
> >>> between neigh and bridge?
> >>
> >> No, it is inconsistent with other NLM/NUD-state within bridge.  As
> >> an fdb creation flag NTF_USE is confusing.  It will create an entry
> >> without NLM_F_CREATE being set.  It will ignore NLM_F_EXCL flag as
> >> well.  It will additionally ignore any NUD-state flags that may be set
> >> in the netlink message.  So it may not be doing what the user wishes.
> > 
> > I don't know which of NTF-flags and NLM-flags/NUD-state should be given
> > high priority on setting. For now, in bridge, NTF_USE masks any other
> > flags. If this is not proper way for netlink/neighbour, I will agree
> > with you. If not sure, I have no motivation to change existing behavior
> > that might be expected by some users.
> > 
> >>
> >> It also provides duplicate functionality.  The same results are achieved
> >> by setting NLM_F_CREATE flag and NUD_REACHABLE state in the message.
> > 
> > br_fdb_update() never updates fdb->used, which is visible by user,
> > unlike fdb_add_entry().
> 
> Thanks for pointing this out.  It looks like there are some
> inconsistencies in the fdb->used markings as well.
> 
> > 
> > If it is duplicate functionality, isn't NTF_USE itself no use?
> > What can be achieved by changing capability of creation and update of
> > local entries?
> 
> Update of local entries gives you port redirection, but doing it under
> rcu.  Not sure if it really makes much sense though...
> 
> NTF_USE makes since for neighbor cache as it triggers an ARP and a
> refresh of the entry.  Suppose, NTF_USE on the fdb entry should
> trigger a refresh as well, but causing a create has to be explicit.
> 
> I think I'll just send my patch and we can continue this discussion
> there.

OK, I'll wait for your patch.

Thanks,
Toshiaki Makita

[PATCH net v2 2/9] bridge: Fix the way to insert new local fdb entries in br_fdb_changeaddr

[Toshiaki Makita]

Since commit bc9a25d21ef8 ("bridge: Add vlan support for local fdb entries"),
br_fdb_changeaddr() has inserted a new local fdb entry only if it can
find old one. But if we have two ports where they have the same address
or user has deleted a local entry, there will be no entry for one of the
ports.

Example of problematic case:
  ip link set eth0 address aa:bb:cc:dd:ee:ff
  ip link set eth1 address aa:bb:cc:dd:ee:ff
  brctl addif br0 eth0
  brctl addif br0 eth1 # eth1 will not have a local entry due to dup.
  ip link set eth1 address 12:34:56:78:90:ab
Then, the new entry for the address 12:34:56:78:90:ab will not be
created, and the bridge device will not be able to communicate.

Insert new entries regardless of whether we can find old entries or not.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_fdb.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 5dab230..5f1bd11 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -92,8 +92,10 @@ static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f)
 void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
 {
 	struct net_bridge *br = p->br;
-	bool no_vlan = (nbp_get_vlan_info(p) == NULL) ? true : false;
+	struct net_port_vlans *pv = nbp_get_vlan_info(p);
+	bool no_vlan = !pv;
 	int i;
+	u16 vid;
 
 	spin_lock_bh(&br->hash_lock);
 
@@ -114,28 +116,37 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
 							     f->addr.addr) &&
 					    nbp_vlan_find(op, vid)) {
 						f->dst = op;
-						goto insert;
+						goto skip_delete;
 					}
 				}
 
 				/* delete old one */
 				fdb_delete(br, f);
-insert:
-				/* insert new address,  may fail if invalid
-				 * address or dup.
-				 */
-				fdb_insert(br, p, newaddr, vid);
-
+skip_delete:
 				/* if this port has no vlan information
 				 * configured, we can safely be done at
 				 * this point.
 				 */
 				if (no_vlan)
-					goto done;
+					goto insert;
 			}
 		}
 	}
 
+insert:
+	/* insert new address,  may fail if invalid address or dup. */
+	fdb_insert(br, p, newaddr, 0);
+
+	if (no_vlan)
+		goto done;
+
+	/* Now add entries for every VLAN configured on the port.
+	 * This function runs under RTNL so the bitmap will not change
+	 * from under us.
+	 */
+	for_each_set_bit(vid, pv->vlan_bitmap, VLAN_N_VID)
+		fdb_insert(br, p, newaddr, vid);
+
 done:
 	spin_unlock_bh(&br->hash_lock);
 }
-- 
1.8.1.2

Re: [PATCH net v2 2/9] bridge: Fix the way to insert new local fdb entries in br_fdb_changeaddr

[Vlad Yasevich]

On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> Since commit bc9a25d21ef8 ("bridge: Add vlan support for local fdb entries"),
> br_fdb_changeaddr() has inserted a new local fdb entry only if it can
> find old one. But if we have two ports where they have the same address
> or user has deleted a local entry, there will be no entry for one of the
> ports.
> 
> Example of problematic case:
>   ip link set eth0 address aa:bb:cc:dd:ee:ff
>   ip link set eth1 address aa:bb:cc:dd:ee:ff
>   brctl addif br0 eth0
>   brctl addif br0 eth1 # eth1 will not have a local entry due to dup.
>   ip link set eth1 address 12:34:56:78:90:ab
> Then, the new entry for the address 12:34:56:78:90:ab will not be
> created, and the bridge device will not be able to communicate.
> 
> Insert new entries regardless of whether we can find old entries or not.
> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>

Acked-by: Vlad Yasevich <vyasevic@redhat.com>

-vlad
> ---
>  net/bridge/br_fdb.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 5dab230..5f1bd11 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -92,8 +92,10 @@ static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f)
>  void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
>  {
>  	struct net_bridge *br = p->br;
> -	bool no_vlan = (nbp_get_vlan_info(p) == NULL) ? true : false;
> +	struct net_port_vlans *pv = nbp_get_vlan_info(p);
> +	bool no_vlan = !pv;
>  	int i;
> +	u16 vid;
>  
>  	spin_lock_bh(&br->hash_lock);
>  
> @@ -114,28 +116,37 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
>  							     f->addr.addr) &&
>  					    nbp_vlan_find(op, vid)) {
>  						f->dst = op;
> -						goto insert;
> +						goto skip_delete;
>  					}
>  				}
>  
>  				/* delete old one */
>  				fdb_delete(br, f);
> -insert:
> -				/* insert new address,  may fail if invalid
> -				 * address or dup.
> -				 */
> -				fdb_insert(br, p, newaddr, vid);
> -
> +skip_delete:
>  				/* if this port has no vlan information
>  				 * configured, we can safely be done at
>  				 * this point.
>  				 */
>  				if (no_vlan)
> -					goto done;
> +					goto insert;
>  			}
>  		}
>  	}
>  
> +insert:
> +	/* insert new address,  may fail if invalid address or dup. */
> +	fdb_insert(br, p, newaddr, 0);
> +
> +	if (no_vlan)
> +		goto done;
> +
> +	/* Now add entries for every VLAN configured on the port.
> +	 * This function runs under RTNL so the bitmap will not change
> +	 * from under us.
> +	 */
> +	for_each_set_bit(vid, pv->vlan_bitmap, VLAN_N_VID)
> +		fdb_insert(br, p, newaddr, vid);
> +
>  done:
>  	spin_unlock_bh(&br->hash_lock);
>  }
> 

[PATCH net v2 3/9] bridge: Fix the way to find old local fdb entries in br_fdb_change_mac_address

[Toshiaki Makita]

We have been always failed to delete the old entry at
br_fdb_change_mac_address() because br_set_mac_address() updates
dev->dev_addr before calling br_fdb_change_mac_address() and
br_fdb_change_mac_address() uses dev->dev_addr to find the old entry.

That update of dev_addr is completely unnecessary because the same work
is done in br_stp_change_bridge_id() which is called right away after
calling br_fdb_change_mac_address().

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index f00cfd2..d967773 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -187,8 +187,8 @@ static int br_set_mac_address(struct net_device *dev, void *p)
 
 	spin_lock_bh(&br->lock);
 	if (!ether_addr_equal(dev->dev_addr, addr->sa_data)) {
-		memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
 		br_fdb_change_mac_address(br, addr->sa_data);
+		/* Mac address will be changed in br_stp_change_bridge_id(). */
 		br_stp_change_bridge_id(br, addr->sa_data);
 	}
 	spin_unlock_bh(&br->lock);
-- 
1.8.1.2

Re: [PATCH net v2 3/9] bridge: Fix the way to find old local fdb entries in br_fdb_change_mac_address

[Vlad Yasevich]

On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> We have been always failed to delete the old entry at
> br_fdb_change_mac_address() because br_set_mac_address() updates
> dev->dev_addr before calling br_fdb_change_mac_address() and
> br_fdb_change_mac_address() uses dev->dev_addr to find the old entry.
> 
> That update of dev_addr is completely unnecessary because the same work
> is done in br_stp_change_bridge_id() which is called right away after
> calling br_fdb_change_mac_address().
> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>

Acked-by: Vlad Yasevich <vyasevic@redhat.com>

-vlad

> ---
>  net/bridge/br_device.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index f00cfd2..d967773 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -187,8 +187,8 @@ static int br_set_mac_address(struct net_device *dev, void *p)
>  
>  	spin_lock_bh(&br->lock);
>  	if (!ether_addr_equal(dev->dev_addr, addr->sa_data)) {
> -		memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
>  		br_fdb_change_mac_address(br, addr->sa_data);
> +		/* Mac address will be changed in br_stp_change_bridge_id(). */
>  		br_stp_change_bridge_id(br, addr->sa_data);
>  	}
>  	spin_unlock_bh(&br->lock);
> 

[PATCH net v2 4/9] bridge: Change local fdb entries whenever mac address of bridge device changes

[Toshiaki Makita]

Vlan code may need fdb change when changing mac address of bridge device
even if it is caused by the mac address changing of a bridge port.

Example configuration:
  ip link set eth0 address 12:34:56:78:90:ab
  ip link set eth1 address aa:bb:cc:dd:ee:ff
  brctl addif br0 eth0
  brctl addif br0 eth1 # br0 will have mac address 12:34:56:78:90:ab
  bridge vlan add dev br0 vid 10 self
  bridge vlan add dev eth0 vid 10
We will have fdb entry such that f->dst == NULL, f->vlan_id == 10 and
f->addr == 12:34:56:78:90:ab at this time.
Next, change the mac address of eth0 to greater value.
  ip link set eth0 address ee:ff:12:34:56:78
Then, mac address of br0 will be recalculated and set to aa:bb:cc:dd:ee:ff.
However, an entry aa:bb:cc:dd:ee:ff will not be created and we will be not
able to communicate using br0 on vlan 10.

Address this issue by deleting and adding local entries whenever
changing the mac address of the bridge device.

If there already exists an entry that has the same address, for example,
in case that br_fdb_changeaddr() has already inserted it,
br_fdb_change_mac_address() will simply fail to insert it and no
duplicated entry will be made, as it was.

This approach also needs br_add_if() to call br_fdb_insert() before
br_stp_recalculate_bridge_id() so that we don't create an entry whose
dst == NULL in this function to preserve previous behavior.

Note that this is a slight change in behavior where the bridge device can
receive the traffic to the new address before calling
br_stp_recalculate_bridge_id() in br_add_if().
However, it is not a problem because we have already the address on the
new port and such a way to insert new one before recalculating bridge id
is taken in br_device_event() as well.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_device.c | 1 -
 net/bridge/br_if.c     | 6 +++---
 net/bridge/br_stp_if.c | 2 ++
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index d967773..1cbdbf1 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -187,7 +187,6 @@ static int br_set_mac_address(struct net_device *dev, void *p)
 
 	spin_lock_bh(&br->lock);
 	if (!ether_addr_equal(dev->dev_addr, addr->sa_data)) {
-		br_fdb_change_mac_address(br, addr->sa_data);
 		/* Mac address will be changed in br_stp_change_bridge_id(). */
 		br_stp_change_bridge_id(br, addr->sa_data);
 	}
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 4bf02ad..779d91c 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -389,6 +389,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 	if (br->dev->needed_headroom < dev->needed_headroom)
 		br->dev->needed_headroom = dev->needed_headroom;
 
+	if (br_fdb_insert(br, p, dev->dev_addr, 0))
+		netdev_err(dev, "failed insert local address bridge forwarding table\n");
+
 	spin_lock_bh(&br->lock);
 	changed_addr = br_stp_recalculate_bridge_id(br);
 
@@ -404,9 +407,6 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 
 	dev_set_mtu(br->dev, br_min_mtu(br));
 
-	if (br_fdb_insert(br, p, dev->dev_addr, 0))
-		netdev_err(dev, "failed insert local address bridge forwarding table\n");
-
 	kobject_uevent(&p->kobj, KOBJ_ADD);
 
 	return 0;
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index 656a6f3..189ba1e 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -194,6 +194,8 @@ void br_stp_change_bridge_id(struct net_bridge *br, const unsigned char *addr)
 
 	wasroot = br_is_root_bridge(br);
 
+	br_fdb_change_mac_address(br, addr);
+
 	memcpy(oldaddr, br->bridge_id.addr, ETH_ALEN);
 	memcpy(br->bridge_id.addr, addr, ETH_ALEN);
 	memcpy(br->dev->dev_addr, addr, ETH_ALEN);
-- 
1.8.1.2

Re: [PATCH net v2 4/9] bridge: Change local fdb entries whenever mac address of bridge device changes

[Vlad Yasevich]

On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> Vlan code may need fdb change when changing mac address of bridge device
> even if it is caused by the mac address changing of a bridge port.
> 
> Example configuration:
>   ip link set eth0 address 12:34:56:78:90:ab
>   ip link set eth1 address aa:bb:cc:dd:ee:ff
>   brctl addif br0 eth0
>   brctl addif br0 eth1 # br0 will have mac address 12:34:56:78:90:ab
>   bridge vlan add dev br0 vid 10 self
>   bridge vlan add dev eth0 vid 10
> We will have fdb entry such that f->dst == NULL, f->vlan_id == 10 and
> f->addr == 12:34:56:78:90:ab at this time.
> Next, change the mac address of eth0 to greater value.
>   ip link set eth0 address ee:ff:12:34:56:78
> Then, mac address of br0 will be recalculated and set to aa:bb:cc:dd:ee:ff.
> However, an entry aa:bb:cc:dd:ee:ff will not be created and we will be not
> able to communicate using br0 on vlan 10.
> 
> Address this issue by deleting and adding local entries whenever
> changing the mac address of the bridge device.
> 
> If there already exists an entry that has the same address, for example,
> in case that br_fdb_changeaddr() has already inserted it,
> br_fdb_change_mac_address() will simply fail to insert it and no
> duplicated entry will be made, as it was.
> 
> This approach also needs br_add_if() to call br_fdb_insert() before
> br_stp_recalculate_bridge_id() so that we don't create an entry whose
> dst == NULL in this function to preserve previous behavior.

Aren't we still creating this entry even with this change?

Looking at br_fdb_change_mac_address(), it will always add
an fdb with NULL port.

May be we can update br_fdb_change_mac_address() to not add
a new entry if we have a local fdb already for the address.

-vlad

> 
> Note that this is a slight change in behavior where the bridge device can
> receive the traffic to the new address before calling
> br_stp_recalculate_bridge_id() in br_add_if().
> However, it is not a problem because we have already the address on the
> new port and such a way to insert new one before recalculating bridge id
> is taken in br_device_event() as well.
> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>  net/bridge/br_device.c | 1 -
>  net/bridge/br_if.c     | 6 +++---
>  net/bridge/br_stp_if.c | 2 ++
>  3 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index d967773..1cbdbf1 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -187,7 +187,6 @@ static int br_set_mac_address(struct net_device *dev, void *p)
>  
>  	spin_lock_bh(&br->lock);
>  	if (!ether_addr_equal(dev->dev_addr, addr->sa_data)) {
> -		br_fdb_change_mac_address(br, addr->sa_data);
>  		/* Mac address will be changed in br_stp_change_bridge_id(). */
>  		br_stp_change_bridge_id(br, addr->sa_data);
>  	}
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 4bf02ad..779d91c 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -389,6 +389,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
>  	if (br->dev->needed_headroom < dev->needed_headroom)
>  		br->dev->needed_headroom = dev->needed_headroom;
>  
> +	if (br_fdb_insert(br, p, dev->dev_addr, 0))
> +		netdev_err(dev, "failed insert local address bridge forwarding table\n");
> +
>  	spin_lock_bh(&br->lock);
>  	changed_addr = br_stp_recalculate_bridge_id(br);
>  
> @@ -404,9 +407,6 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
>  
>  	dev_set_mtu(br->dev, br_min_mtu(br));
>  
> -	if (br_fdb_insert(br, p, dev->dev_addr, 0))
> -		netdev_err(dev, "failed insert local address bridge forwarding table\n");
> -
>  	kobject_uevent(&p->kobj, KOBJ_ADD);
>  
>  	return 0;
> diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
> index 656a6f3..189ba1e 100644
> --- a/net/bridge/br_stp_if.c
> +++ b/net/bridge/br_stp_if.c
> @@ -194,6 +194,8 @@ void br_stp_change_bridge_id(struct net_bridge *br, const unsigned char *addr)
>  
>  	wasroot = br_is_root_bridge(br);
>  
> +	br_fdb_change_mac_address(br, addr);
> +
>  	memcpy(oldaddr, br->bridge_id.addr, ETH_ALEN);
>  	memcpy(br->bridge_id.addr, addr, ETH_ALEN);
>  	memcpy(br->dev->dev_addr, addr, ETH_ALEN);
> 

Re: [PATCH net v2 4/9] bridge: Change local fdb entries whenever mac address of bridge device changes

[Vlad Yasevich]

On 12/17/2013 11:22 AM, Vlad Yasevich wrote:
> On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
>> Vlan code may need fdb change when changing mac address of bridge device
>> even if it is caused by the mac address changing of a bridge port.
>>
>> Example configuration:
>>   ip link set eth0 address 12:34:56:78:90:ab
>>   ip link set eth1 address aa:bb:cc:dd:ee:ff
>>   brctl addif br0 eth0
>>   brctl addif br0 eth1 # br0 will have mac address 12:34:56:78:90:ab
>>   bridge vlan add dev br0 vid 10 self
>>   bridge vlan add dev eth0 vid 10
>> We will have fdb entry such that f->dst == NULL, f->vlan_id == 10 and
>> f->addr == 12:34:56:78:90:ab at this time.
>> Next, change the mac address of eth0 to greater value.
>>   ip link set eth0 address ee:ff:12:34:56:78
>> Then, mac address of br0 will be recalculated and set to aa:bb:cc:dd:ee:ff.
>> However, an entry aa:bb:cc:dd:ee:ff will not be created and we will be not
>> able to communicate using br0 on vlan 10.
>>
>> Address this issue by deleting and adding local entries whenever
>> changing the mac address of the bridge device.
>>
>> If there already exists an entry that has the same address, for example,
>> in case that br_fdb_changeaddr() has already inserted it,
>> br_fdb_change_mac_address() will simply fail to insert it and no
>> duplicated entry will be made, as it was.
>>
>> This approach also needs br_add_if() to call br_fdb_insert() before
>> br_stp_recalculate_bridge_id() so that we don't create an entry whose
>> dst == NULL in this function to preserve previous behavior.
> 
> Aren't we still creating this entry even with this change?
> 
> Looking at br_fdb_change_mac_address(), it will always add
> an fdb with NULL port.
>

Never mind.  br_fdb_insert() here will return 0 since a local
entry already exists...

Acked-by: Vlad Yasevich <vyasevic@redhat.com>

-vlad


> May be we can update br_fdb_change_mac_address() to not add
> a new entry if we have a local fdb already for the address.
> 
> -vlad
> 
>>
>> Note that this is a slight change in behavior where the bridge device can
>> receive the traffic to the new address before calling
>> br_stp_recalculate_bridge_id() in br_add_if().
>> However, it is not a problem because we have already the address on the
>> new port and such a way to insert new one before recalculating bridge id
>> is taken in br_device_event() as well.
>>
>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>> ---
>>  net/bridge/br_device.c | 1 -
>>  net/bridge/br_if.c     | 6 +++---
>>  net/bridge/br_stp_if.c | 2 ++
>>  3 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
>> index d967773..1cbdbf1 100644
>> --- a/net/bridge/br_device.c
>> +++ b/net/bridge/br_device.c
>> @@ -187,7 +187,6 @@ static int br_set_mac_address(struct net_device *dev, void *p)
>>  
>>  	spin_lock_bh(&br->lock);
>>  	if (!ether_addr_equal(dev->dev_addr, addr->sa_data)) {
>> -		br_fdb_change_mac_address(br, addr->sa_data);
>>  		/* Mac address will be changed in br_stp_change_bridge_id(). */
>>  		br_stp_change_bridge_id(br, addr->sa_data);
>>  	}
>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>> index 4bf02ad..779d91c 100644
>> --- a/net/bridge/br_if.c
>> +++ b/net/bridge/br_if.c
>> @@ -389,6 +389,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
>>  	if (br->dev->needed_headroom < dev->needed_headroom)
>>  		br->dev->needed_headroom = dev->needed_headroom;
>>  
>> +	if (br_fdb_insert(br, p, dev->dev_addr, 0))
>> +		netdev_err(dev, "failed insert local address bridge forwarding table\n");
>> +
>>  	spin_lock_bh(&br->lock);
>>  	changed_addr = br_stp_recalculate_bridge_id(br);
>>  
>> @@ -404,9 +407,6 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
>>  
>>  	dev_set_mtu(br->dev, br_min_mtu(br));
>>  
>> -	if (br_fdb_insert(br, p, dev->dev_addr, 0))
>> -		netdev_err(dev, "failed insert local address bridge forwarding table\n");
>> -
>>  	kobject_uevent(&p->kobj, KOBJ_ADD);
>>  
>>  	return 0;
>> diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
>> index 656a6f3..189ba1e 100644
>> --- a/net/bridge/br_stp_if.c
>> +++ b/net/bridge/br_stp_if.c
>> @@ -194,6 +194,8 @@ void br_stp_change_bridge_id(struct net_bridge *br, const unsigned char *addr)
>>  
>>  	wasroot = br_is_root_bridge(br);
>>  
>> +	br_fdb_change_mac_address(br, addr);
>> +
>>  	memcpy(oldaddr, br->bridge_id.addr, ETH_ALEN);
>>  	memcpy(br->bridge_id.addr, addr, ETH_ALEN);
>>  	memcpy(br->dev->dev_addr, addr, ETH_ALEN);
>>
> 

[PATCH net v2 5/9] bridge: Fix the way to check if a local fdb entry can be deleted

[Toshiaki Makita]

We should take into account the followings when deleting a local fdb
entry.

- nbp_vlan_find() can be used only when vid != 0 to check if an entry is
  deletable, because a fdb entry with vid 0 can exist at any time while
  nbp_vlan_find() always return false with vid 0.

  Example of problematic case:
    ip link set eth0 address 12:34:56:78:90:ab
    ip link set eth1 address 12:34:56:78:90:ab
    brctl addif br0 eth0
    brctl addif br0 eth1
    ip link set eth0 address aa:bb:cc:dd:ee:ff
  Then, the fdb entry 12:34:56:78:90:ab will be deleted even though the
  bridge port eth1 still has that address.

- The port to which the bridge device is attached might needs a local entry
  if its mac address is set manually.

  Example of problematic case:
    ip link set eth0 address 12:34:56:78:90:ab
    brctl addif br0 eth0
    ip link set br0 address 12:34:56:78:90:ab
    ip link set eth0 address aa:bb:cc:dd:ee:ff
  Then, the fdb still must have the entry 12:34:56:78:90:ab, but it will be
  deleted.

We can use br->dev->addr_assign_type to check if the address is manually
set or not, but I propose another approach.

Since we delete and insert local entries whenever changing mac address
of the bridge device, we can change dst of the entry to NULL regardless of
addr_assign_type when deleting an entry associated with a certain port,
and if it is found to be unnecessary later, then delete it.
That is, if changing mac address of a port, the entry might be changed
to its dst being NULL first, but is eventually deleted when recalculating
and changing bridge id.

This approach is especially useful when we want to share the code with
deleting vlan in which the bridge device might want such an entry regardless
of addr_assign_type, and makes things easy because we don't have to consider
if mac address of the bridge device will be changed or not at the time we
delete a local entry of a port, which meas fdb code will not be bothered even
if the bridge id calculating logic is changed in the future.

Note that this is a slight change in behavior where the bridge device can
receive the traffic to the old address during the short window between
calling br_fdb_changeaddr() and br_stp_recalculate_bridge_id() in
br_device_event(). However, it is not a problem because we still have the
address on the bridge device.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_fdb.c     | 10 +++++++++-
 net/bridge/br_private.h |  6 ++++++
 net/bridge/br_vlan.c    | 19 +++++++++++++++++++
 3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 5f1bd11..cf8b64e 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -114,12 +114,20 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
 					if (op != p &&
 					    ether_addr_equal(op->dev->dev_addr,
 							     f->addr.addr) &&
-					    nbp_vlan_find(op, vid)) {
+					    (!vid || nbp_vlan_find(op, vid))) {
 						f->dst = op;
 						goto skip_delete;
 					}
 				}
 
+				/* maybe bridge device has same hw addr? */
+				if (ether_addr_equal(br->dev->dev_addr,
+						     f->addr.addr) &&
+				    (!vid || br_vlan_find(br, vid))) {
+					f->dst = NULL;
+					goto skip_delete;
+				}
+
 				/* delete old one */
 				fdb_delete(br, f);
 skip_delete:
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 91fb2c2..868c059 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -594,6 +594,7 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
 int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags);
 int br_vlan_delete(struct net_bridge *br, u16 vid);
 void br_vlan_flush(struct net_bridge *br);
+bool br_vlan_find(struct net_bridge *br, u16 vid);
 int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val);
 int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags);
 int nbp_vlan_delete(struct net_bridge_port *port, u16 vid);
@@ -675,6 +676,11 @@ static inline void br_vlan_flush(struct net_bridge *br)
 {
 }
 
+static inline bool br_vlan_find(struct net_bridge *br, u16 vid)
+{
+	return false;
+}
+
 static inline int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags)
 {
 	return -EOPNOTSUPP;
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index af5ebd1..f87ab88f 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -316,6 +316,25 @@ void br_vlan_flush(struct net_bridge *br)
 	__vlan_flush(pv);
 }
 
+bool br_vlan_find(struct net_bridge *br, u16 vid)
+{
+	struct net_port_vlans *pv;
+	bool found = false;
+
+	rcu_read_lock();
+	pv = rcu_dereference(br->vlan_info);
+
+	if (!pv)
+		goto out;
+
+	if (test_bit(vid, pv->vlan_bitmap))
+		found = true;
+
+out:
+	rcu_read_unlock();
+	return found;
+}
+
 int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val)
 {
 	if (!rtnl_trylock())
-- 
1.8.1.2

Re: [PATCH net v2 5/9] bridge: Fix the way to check if a local fdb entry can be deleted

[Vlad Yasevich]

On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> We should take into account the followings when deleting a local fdb
> entry.
> 
> - nbp_vlan_find() can be used only when vid != 0 to check if an entry is
>   deletable, because a fdb entry with vid 0 can exist at any time while
>   nbp_vlan_find() always return false with vid 0.
> 
>   Example of problematic case:
>     ip link set eth0 address 12:34:56:78:90:ab
>     ip link set eth1 address 12:34:56:78:90:ab
>     brctl addif br0 eth0
>     brctl addif br0 eth1
>     ip link set eth0 address aa:bb:cc:dd:ee:ff
>   Then, the fdb entry 12:34:56:78:90:ab will be deleted even though the
>   bridge port eth1 still has that address.
> 
> - The port to which the bridge device is attached might needs a local entry
>   if its mac address is set manually.
> 
>   Example of problematic case:
>     ip link set eth0 address 12:34:56:78:90:ab
>     brctl addif br0 eth0
>     ip link set br0 address 12:34:56:78:90:ab
>     ip link set eth0 address aa:bb:cc:dd:ee:ff
>   Then, the fdb still must have the entry 12:34:56:78:90:ab, but it will be
>   deleted.
> 
> We can use br->dev->addr_assign_type to check if the address is manually
> set or not, but I propose another approach.
> 
> Since we delete and insert local entries whenever changing mac address
> of the bridge device, we can change dst of the entry to NULL regardless of
> addr_assign_type when deleting an entry associated with a certain port,
> and if it is found to be unnecessary later, then delete it.
> That is, if changing mac address of a port, the entry might be changed
> to its dst being NULL first, but is eventually deleted when recalculating
> and changing bridge id.
> 
> This approach is especially useful when we want to share the code with
> deleting vlan in which the bridge device might want such an entry regardless
> of addr_assign_type, and makes things easy because we don't have to consider
> if mac address of the bridge device will be changed or not at the time we
> delete a local entry of a port, which meas fdb code will not be bothered even
> if the bridge id calculating logic is changed in the future.
> 
> Note that this is a slight change in behavior where the bridge device can
> receive the traffic to the old address during the short window between
> calling br_fdb_changeaddr() and br_stp_recalculate_bridge_id() in
> br_device_event(). However, it is not a problem because we still have the
> address on the bridge device.

I think you are understating the significance here a little bit.  The
change is that for a short period of time after a port has been removed,
packets addressed to the MAC of that port may be delivered only to
bridge device instead of being flooded.

> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>  net/bridge/br_fdb.c     | 10 +++++++++-
>  net/bridge/br_private.h |  6 ++++++
>  net/bridge/br_vlan.c    | 19 +++++++++++++++++++
>  3 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 5f1bd11..cf8b64e 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -114,12 +114,20 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
>  					if (op != p &&
>  					    ether_addr_equal(op->dev->dev_addr,
>  							     f->addr.addr) &&
> -					    nbp_vlan_find(op, vid)) {
> +					    (!vid || nbp_vlan_find(op, vid))) {
>  						f->dst = op;
>  						goto skip_delete;
>  					}
>  				}
>  
> +				/* maybe bridge device has same hw addr? */
> +				if (ether_addr_equal(br->dev->dev_addr,
> +						     f->addr.addr) &&

I think this really needs a
                                    br->dev->addr_assign_type ==
NET_ADDR_SET &&
> +				    (!vid || br_vlan_find(br, vid))) {

That way we'll only do this if the user actively set the bridge mac
to one be the same as one of the ports.


-vlad

> +					f->dst = NULL;
> +					goto skip_delete;
> +				}
> +
>  				/* delete old one */
>  				fdb_delete(br, f);
>  skip_delete:
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 91fb2c2..868c059 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -594,6 +594,7 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
>  int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags);
>  int br_vlan_delete(struct net_bridge *br, u16 vid);
>  void br_vlan_flush(struct net_bridge *br);
> +bool br_vlan_find(struct net_bridge *br, u16 vid);
>  int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val);
>  int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags);
>  int nbp_vlan_delete(struct net_bridge_port *port, u16 vid);
> @@ -675,6 +676,11 @@ static inline void br_vlan_flush(struct net_bridge *br)
>  {
>  }
>  
> +static inline bool br_vlan_find(struct net_bridge *br, u16 vid)
> +{
> +	return false;
> +}
> +
>  static inline int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags)
>  {
>  	return -EOPNOTSUPP;
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index af5ebd1..f87ab88f 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -316,6 +316,25 @@ void br_vlan_flush(struct net_bridge *br)
>  	__vlan_flush(pv);
>  }
>  
> +bool br_vlan_find(struct net_bridge *br, u16 vid)
> +{
> +	struct net_port_vlans *pv;
> +	bool found = false;
> +
> +	rcu_read_lock();
> +	pv = rcu_dereference(br->vlan_info);
> +
> +	if (!pv)
> +		goto out;
> +
> +	if (test_bit(vid, pv->vlan_bitmap))
> +		found = true;
> +
> +out:
> +	rcu_read_unlock();
> +	return found;
> +}
> +
>  int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val)
>  {
>  	if (!rtnl_trylock())
> 

Re: [PATCH net v2 5/9] bridge: Fix the way to check if a local fdb entry can be deleted

[Toshiaki Makita]

On Tue, 2013-12-17 at 13:53 -0500, Vlad Yasevich wrote:
> On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
...
> > 
> > Note that this is a slight change in behavior where the bridge device can
> > receive the traffic to the old address during the short window between
> > calling br_fdb_changeaddr() and br_stp_recalculate_bridge_id() in
> > br_device_event(). However, it is not a problem because we still have the
> > address on the bridge device.
> 
> I think you are understating the significance here a little bit.  The
> change is that for a short period of time after a port has been removed,
> packets addressed to the MAC of that port may be delivered only to
> bridge device instead of being flooded.
> 
> > 
> > Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> > ---
> >  net/bridge/br_fdb.c     | 10 +++++++++-
> >  net/bridge/br_private.h |  6 ++++++
> >  net/bridge/br_vlan.c    | 19 +++++++++++++++++++
> >  3 files changed, 34 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> > index 5f1bd11..cf8b64e 100644
> > --- a/net/bridge/br_fdb.c
> > +++ b/net/bridge/br_fdb.c
> > @@ -114,12 +114,20 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
> >  					if (op != p &&
> >  					    ether_addr_equal(op->dev->dev_addr,
> >  							     f->addr.addr) &&
> > -					    nbp_vlan_find(op, vid)) {
> > +					    (!vid || nbp_vlan_find(op, vid))) {
> >  						f->dst = op;
> >  						goto skip_delete;
> >  					}
> >  				}
> >  
> > +				/* maybe bridge device has same hw addr? */
> > +				if (ether_addr_equal(br->dev->dev_addr,
> > +						     f->addr.addr) &&
> 
> I think this really needs a
>                                     br->dev->addr_assign_type ==
> NET_ADDR_SET &&
> > +				    (!vid || br_vlan_find(br, vid))) {
> 
> That way we'll only do this if the user actively set the bridge mac
> to one be the same as one of the ports.

Indeed I can do it but it affects patch 8...
If we do, the final condition will be like

static void fdb_delete_local(..., bool check_br_addr)
...
  if (p && ether_addr_equal(br->dev->dev_addr, addr) &&
      (!check_br_addr || br->dev->addr_assign_type == NET_ADDR_SET) &&
      (!vid || br_vlan_find(br, vid)))

void br_fdb_delete_by_port(...)
...
  fdb_delete_local(br, p, f, true);

void nbp_vlan_delete(...)
...
  fdb_delete_local(br, p, f, false);


And we can't change the order of function calls in br_add_if() in patch
4, which changes the behavior of traffic as well.
Instead of whole patch 4, we can do something like

int br_add_if(...)
...
  memcpy(oldaddr, br->dev->dev_addr, ETH_ALEN);
  changed_addr = br_stp_recalculate_bridge_id(br);
...
  if (br_fdb_insert(...))
    ...
  if (changed_addr)
    br_fdb_change_mac_address(br, br->dev->dev_addr, oldaddr);

void br_fdb_change_mac_address(..., const u8 *oldaddr)
(Signature changing)


I honestly can't understand all these changes are really reasonable.
I'm thinking the bridge should receive the traffic to the address
instead of flooding during it has the address.
Can't we change any existing behavior in bug fixes at all?

Thanks,
Toshiaki Makita

Re: [PATCH net v2 5/9] bridge: Fix the way to check if a local fdb entry can be deleted

[Vlad Yasevich]

On 12/17/2013 11:46 PM, Toshiaki Makita wrote:
> On Tue, 2013-12-17 at 13:53 -0500, Vlad Yasevich wrote:
>> On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> ...
>>>
>>> Note that this is a slight change in behavior where the bridge device can
>>> receive the traffic to the old address during the short window between
>>> calling br_fdb_changeaddr() and br_stp_recalculate_bridge_id() in
>>> br_device_event(). However, it is not a problem because we still have the
>>> address on the bridge device.
>>
>> I think you are understating the significance here a little bit.  The
>> change is that for a short period of time after a port has been removed,
>> packets addressed to the MAC of that port may be delivered only to
>> bridge device instead of being flooded.
>>
>>>
>>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>>> ---
>>>  net/bridge/br_fdb.c     | 10 +++++++++-
>>>  net/bridge/br_private.h |  6 ++++++
>>>  net/bridge/br_vlan.c    | 19 +++++++++++++++++++
>>>  3 files changed, 34 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>>> index 5f1bd11..cf8b64e 100644
>>> --- a/net/bridge/br_fdb.c
>>> +++ b/net/bridge/br_fdb.c
>>> @@ -114,12 +114,20 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
>>>  					if (op != p &&
>>>  					    ether_addr_equal(op->dev->dev_addr,
>>>  							     f->addr.addr) &&
>>> -					    nbp_vlan_find(op, vid)) {
>>> +					    (!vid || nbp_vlan_find(op, vid))) {
>>>  						f->dst = op;
>>>  						goto skip_delete;
>>>  					}
>>>  				}
>>>  
>>> +				/* maybe bridge device has same hw addr? */
>>> +				if (ether_addr_equal(br->dev->dev_addr,
>>> +						     f->addr.addr) &&
>>
>> I think this really needs a
>>                                     br->dev->addr_assign_type ==
>> NET_ADDR_SET &&
>>> +				    (!vid || br_vlan_find(br, vid))) {
>>
>> That way we'll only do this if the user actively set the bridge mac
>> to one be the same as one of the ports.
> 
> Indeed I can do it but it affects patch 8...
> If we do, the final condition will be like
> 
> static void fdb_delete_local(..., bool check_br_addr)
> ...
>   if (p && ether_addr_equal(br->dev->dev_addr, addr) &&
>       (!check_br_addr || br->dev->addr_assign_type == NET_ADDR_SET) &&
>       (!vid || br_vlan_find(br, vid)))
> 
> void br_fdb_delete_by_port(...)
> ...
>   fdb_delete_local(br, p, f, true);
> 
> void nbp_vlan_delete(...)
> ...
>   fdb_delete_local(br, p, f, false);
>

Yes, you are right.  It does impact later patches.  I've been trying
to think of anyway to avoid that, but haven't found one so far.

> 
> And we can't change the order of function calls in br_add_if() in patch
> 4, which changes the behavior of traffic as well.

This one is different. First, the window here is a lot shorter
since there is no rcu grace period and the notifier to worry
about.  Second, the change here doesn't result in wrongful delivery
of packets.  Any packets matching the new fdb are dropped until the
port is enabled.

So, patch 4 causes a 'drop rather then flood' change and the
window in which the change is visible is very small.
This patch causes 'deliver to bridge rather then flood' change and the
window is much larger (synchronize_net + netdev notifier chain overhead).

I'll let Stephen or David weigh in with their opinions.

Thanks
-vlad




> Instead of whole patch 4, we can do something like
> 
> int br_add_if(...)
> ...
>   memcpy(oldaddr, br->dev->dev_addr, ETH_ALEN);
>   changed_addr = br_stp_recalculate_bridge_id(br);
> ...
>   if (br_fdb_insert(...))
>     ...
>   if (changed_addr)
>     br_fdb_change_mac_address(br, br->dev->dev_addr, oldaddr);
> 
> void br_fdb_change_mac_address(..., const u8 *oldaddr)
> (Signature changing)
> 
> 
> I honestly can't understand all these changes are really reasonable.
> I'm thinking the bridge should receive the traffic to the address
> instead of flooding during it has the address.
> Can't we change any existing behavior in bug fixes at all?
> 
> Thanks,
> Toshiaki Makita
> 
> 

Re: [PATCH net v2 5/9] bridge: Fix the way to check if a local fdb entry can be deleted

[Stephen Hemminger]

On Wed, 18 Dec 2013 12:22:46 -0500
Vlad Yasevich <vyasevic@redhat.com> wrote:

> I'll let Stephen or David weigh in with their opinions.

I am fine with how you guys are working out the solution.
When you come to a final conclusion, I will give it one last look.

Re: [PATCH net v2 5/9] bridge: Fix the way to check if a local fdb entry can be deleted

[Toshiaki Makita]

On Wed, 2013-12-18 at 12:22 -0500, Vlad Yasevich wrote:
> On 12/17/2013 11:46 PM, Toshiaki Makita wrote:
> > On Tue, 2013-12-17 at 13:53 -0500, Vlad Yasevich wrote:
> >> On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> > ...
> >>>
> >>> Note that this is a slight change in behavior where the bridge device can
> >>> receive the traffic to the old address during the short window between
> >>> calling br_fdb_changeaddr() and br_stp_recalculate_bridge_id() in
> >>> br_device_event(). However, it is not a problem because we still have the
> >>> address on the bridge device.
> >>
> >> I think you are understating the significance here a little bit.  The
> >> change is that for a short period of time after a port has been removed,
> >> packets addressed to the MAC of that port may be delivered only to
> >> bridge device instead of being flooded.
> >>
> >>>
> >>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> >>> ---
> >>>  net/bridge/br_fdb.c     | 10 +++++++++-
> >>>  net/bridge/br_private.h |  6 ++++++
> >>>  net/bridge/br_vlan.c    | 19 +++++++++++++++++++
> >>>  3 files changed, 34 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> >>> index 5f1bd11..cf8b64e 100644
> >>> --- a/net/bridge/br_fdb.c
> >>> +++ b/net/bridge/br_fdb.c
> >>> @@ -114,12 +114,20 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
> >>>  					if (op != p &&
> >>>  					    ether_addr_equal(op->dev->dev_addr,
> >>>  							     f->addr.addr) &&
> >>> -					    nbp_vlan_find(op, vid)) {
> >>> +					    (!vid || nbp_vlan_find(op, vid))) {
> >>>  						f->dst = op;
> >>>  						goto skip_delete;
> >>>  					}
> >>>  				}
> >>>  
> >>> +				/* maybe bridge device has same hw addr? */
> >>> +				if (ether_addr_equal(br->dev->dev_addr,
> >>> +						     f->addr.addr) &&
> >>
> >> I think this really needs a
> >>                                     br->dev->addr_assign_type ==
> >> NET_ADDR_SET &&
> >>> +				    (!vid || br_vlan_find(br, vid))) {
> >>
> >> That way we'll only do this if the user actively set the bridge mac
> >> to one be the same as one of the ports.
> > 
> > Indeed I can do it but it affects patch 8...
> > If we do, the final condition will be like
> > 
> > static void fdb_delete_local(..., bool check_br_addr)
> > ...
> >   if (p && ether_addr_equal(br->dev->dev_addr, addr) &&
> >       (!check_br_addr || br->dev->addr_assign_type == NET_ADDR_SET) &&
> >       (!vid || br_vlan_find(br, vid)))
> > 
> > void br_fdb_delete_by_port(...)
> > ...
> >   fdb_delete_local(br, p, f, true);
> > 
> > void nbp_vlan_delete(...)
> > ...
> >   fdb_delete_local(br, p, f, false);
> >
> 
> Yes, you are right.  It does impact later patches.  I've been trying
> to think of anyway to avoid that, but haven't found one so far.
> 
> > 
> > And we can't change the order of function calls in br_add_if() in patch
> > 4, which changes the behavior of traffic as well.
> 
> This one is different. First, the window here is a lot shorter
> since there is no rcu grace period 

Yes, there is no rcu grace period to wait for.

> and the notifier to worry
> about.  

There is call_netdevice_notifiers() between original position of
br_fdb_insert() and changed position of it.

> Second, the change here doesn't result in wrongful delivery
> of packets.  Any packets matching the new fdb are dropped until the
> port is enabled.

Incoming frames from another port will be affected if they dereference
the new entry during the window. They will be delivered to the bridge
device instead of flooding.

> 
> So, patch 4 causes a 'drop rather then flood' change and the
> window in which the change is visible is very small.
> This patch causes 'deliver to bridge rather then flood' change and the
> window is much larger (synchronize_net + netdev notifier chain overhead).

As you say, synchronize_net() and notifiers relatively can take longer
time than other functions in that window.

So are you worried about the time length of the window?
At least, we are in RTNL and the window should be short enough, observed
by human.

And what do you think is wrong as a bridge?
It has that address. I can't find the reason flooding is better.

I'm afraid the code get considerably complicated or ugly if we try to
get rid of the window, as I showed by pseudo code in previous mail.

Thanks,
Toshiaki Makita

Re: [PATCH net v2 5/9] bridge: Fix the way to check if a local fdb entry can be deleted

[Stephen Hemminger]

On Thu, 19 Dec 2013 21:23:54 +0900
Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:

> On Wed, 2013-12-18 at 12:22 -0500, Vlad Yasevich wrote:
> > On 12/17/2013 11:46 PM, Toshiaki Makita wrote:
> > > On Tue, 2013-12-17 at 13:53 -0500, Vlad Yasevich wrote:
> > >> On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> > > ...
> > >>>
> > >>> Note that this is a slight change in behavior where the bridge device can
> > >>> receive the traffic to the old address during the short window between
> > >>> calling br_fdb_changeaddr() and br_stp_recalculate_bridge_id() in
> > >>> br_device_event(). However, it is not a problem because we still have the
> > >>> address on the bridge device.
> > >>
> > >> I think you are understating the significance here a little bit.  The
> > >> change is that for a short period of time after a port has been removed,
> > >> packets addressed to the MAC of that port may be delivered only to
> > >> bridge device instead of being flooded.
> > >>
> > >>>
> > >>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> > >>> ---
> > >>>  net/bridge/br_fdb.c     | 10 +++++++++-
> > >>>  net/bridge/br_private.h |  6 ++++++
> > >>>  net/bridge/br_vlan.c    | 19 +++++++++++++++++++
> > >>>  3 files changed, 34 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> > >>> index 5f1bd11..cf8b64e 100644
> > >>> --- a/net/bridge/br_fdb.c
> > >>> +++ b/net/bridge/br_fdb.c
> > >>> @@ -114,12 +114,20 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
> > >>>  					if (op != p &&
> > >>>  					    ether_addr_equal(op->dev->dev_addr,
> > >>>  							     f->addr.addr) &&
> > >>> -					    nbp_vlan_find(op, vid)) {
> > >>> +					    (!vid || nbp_vlan_find(op, vid))) {
> > >>>  						f->dst = op;
> > >>>  						goto skip_delete;
> > >>>  					}
> > >>>  				}
> > >>>  
> > >>> +				/* maybe bridge device has same hw addr? */
> > >>> +				if (ether_addr_equal(br->dev->dev_addr,
> > >>> +						     f->addr.addr) &&
> > >>
> > >> I think this really needs a
> > >>                                     br->dev->addr_assign_type ==
> > >> NET_ADDR_SET &&
> > >>> +				    (!vid || br_vlan_find(br, vid))) {
> > >>
> > >> That way we'll only do this if the user actively set the bridge mac
> > >> to one be the same as one of the ports.
> > > 
> > > Indeed I can do it but it affects patch 8...
> > > If we do, the final condition will be like
> > > 
> > > static void fdb_delete_local(..., bool check_br_addr)
> > > ...
> > >   if (p && ether_addr_equal(br->dev->dev_addr, addr) &&
> > >       (!check_br_addr || br->dev->addr_assign_type == NET_ADDR_SET) &&
> > >       (!vid || br_vlan_find(br, vid)))
> > > 
> > > void br_fdb_delete_by_port(...)
> > > ...
> > >   fdb_delete_local(br, p, f, true);
> > > 
> > > void nbp_vlan_delete(...)
> > > ...
> > >   fdb_delete_local(br, p, f, false);
> > >
> > 
> > Yes, you are right.  It does impact later patches.  I've been trying
> > to think of anyway to avoid that, but haven't found one so far.
> > 
> > > 
> > > And we can't change the order of function calls in br_add_if() in patch
> > > 4, which changes the behavior of traffic as well.
> > 
> > This one is different. First, the window here is a lot shorter
> > since there is no rcu grace period 
> 
> Yes, there is no rcu grace period to wait for.
> 
> > and the notifier to worry
> > about.  
> 
> There is call_netdevice_notifiers() between original position of
> br_fdb_insert() and changed position of it.
> 
> > Second, the change here doesn't result in wrongful delivery
> > of packets.  Any packets matching the new fdb are dropped until the
> > port is enabled.
> 
> Incoming frames from another port will be affected if they dereference
> the new entry during the window. They will be delivered to the bridge
> device instead of flooding.
> 
> > 
> > So, patch 4 causes a 'drop rather then flood' change and the
> > window in which the change is visible is very small.
> > This patch causes 'deliver to bridge rather then flood' change and the
> > window is much larger (synchronize_net + netdev notifier chain overhead).
> 
> As you say, synchronize_net() and notifiers relatively can take longer
> time than other functions in that window.
> 
> So are you worried about the time length of the window?
> At least, we are in RTNL and the window should be short enough, observed
> by human.
> 
> And what do you think is wrong as a bridge?
> It has that address. I can't find the reason flooding is better.
> 
> I'm afraid the code get considerably complicated or ugly if we try to
> get rid of the window, as I showed by pseudo code in previous mail.
> 
> Thanks,
> Toshiaki Makita
> 
> 

Could we make up as set of test case scripts to validate these changes.
Now that FDB table can be manipulated by iproute tools, should be possible
to have set of cases for validation.

Re: [PATCH net v2 5/9] bridge: Fix the way to check if a local fdb entry can be deleted

[Toshiaki Makita]

On Thu, 2013-12-19 at 09:39 -0800, Stephen Hemminger wrote:
...
> Could we make up as set of test case scripts to validate these changes.
> Now that FDB table can be manipulated by iproute tools, should be possible
> to have set of cases for validation.

Thank you for your suggestion.
Maybe is it enough to make some test of port attach/detach and confirm
that data/control plane doesn't result in any inconsistency or odd
situation by tcpdump/bridge commands?

Thanks,
Toshiaki Makita

Re: [PATCH net v2 5/9] bridge: Fix the way to check if a local fdb entry can be deleted

[Toshiaki Makita]

On Fri, 2013-12-20 at 17:02 +0900, Toshiaki Makita wrote:
> On Thu, 2013-12-19 at 09:39 -0800, Stephen Hemminger wrote:
> ...
> > Could we make up as set of test case scripts to validate these changes.
> > Now that FDB table can be manipulated by iproute tools, should be possible
> > to have set of cases for validation.
> 
> Thank you for your suggestion.
> Maybe is it enough to make some test of port attach/detach and confirm
> that data/control plane doesn't result in any inconsistency or odd
> situation by tcpdump/bridge commands?

Sorry for replying an old thread..

I tested traffic and fdb entries when attaching/detaching a bridge port,
and couldn't find any problem with this patch.

Instead, I found an additional undesirable behavior without this patch:
ping to a bridge device (with arp resolution) sometimes fails while
detaching a port.
The bridge device continues to have its mac address for a while when a
port is detached, but in current implementation we immediately delete
corresponding fdb entry even though that mac address is used by the
bridge device. I think this caused ping fails because the replied mac
address actually can't reach the bridge device due to the premature fdb
entry deletion.

I need to rearrange this patch set, but I'm going to keep this change as
is.


The test I did is:
- Attach/detach a port while sending traffic, and confirm tat the
traffic is changed to being [flooded/delivered to the bridge] as
expected. Also, confirm that corresponding fdb entry is added/deleted as
expected.
- Attach/detach a port while flushing arp entry and pinging, and confirm
that ping doesn't fail.

The test script I used and the result is below.

---- Test script begin ----
#!/bin/sh

# This is a test script for validating fdb and traffic consistency
# when a bridge port is attached/detached.
#
# This script does two types of tests.
# Forwarding test:
#   Confirm that traffic to mac address of PORT0 is delivered to bridge
#   when PORT0 is attached and flooded when PORT0 is detached.
#   And measure the time window between "ip set master" and actual traffic
#   change by "date" command (clock_gettime) and timestamp in tcpdump.
# ARP/ICMP test:
#   Confirm that ping can always suceed with arp resolution each time when
#   PORT0 gets attached/detached.
# In each test, this validates fdb entries as well, i.e. if attaching PORT0,
# we must have a local entry where its dst and address is those of PORT0,
# and if detaching PORT0, we must not have such an entry.
#
# This test script assumes environment below.
# BR0 has three ports. Two of them (PORT1/2) are veths to namespaces.
# PORT0 is subject for attaching/detaching.
#
# +----------------------------------------------+
# | +-------------+                              |
# | |     +-------|        +-----+  +---+  +-----|
# | | NS0 |NS0_IF0|--VETH--|PORT1|--|BR0|--|PORT0|
# | |     +-------|        +-----+  +---+  +-----|
# | +-------------+                   |          |
# |                                   |          |
# | +-------------+                   |          |
# | |     +-------|        +-----+    |          |
# | | NS1 |NS1_IF0|--VETH--|PORT2|----+          |
# | |     +-------|        +-----+               |
# | +-------------+                              |
# |                             Physical machine |
# +----------------------------------------------+
#
# Test scenarios are
# Forwarding test:
#   Send traffic from NS0 to the mac address of PORT0.
#   The traffic should be delivered to BR0 when attached, and to NS1 when
#   detached.
# ARP/ICMP test:
#   Invalidate the arp entry of the mac address of PORT0 and do "ping -c 1"
#   to the ip address of BR0.
#   Ping should always succeed even if attach/detach occurs.

export LC_ALL=C

# Number of tests
ITERATIONS=10
[ -n "$1" ] && ITERATIONS=$1

# Namespace
NS0=ns0
NS1=ns1
NS0CMD="ip netns exec $NS0"

# Veth interface
VETH_TO_NS0_IF=veth0
NS0_IF=ns0-eth0
VETH_TO_NS1_IF=veth1
NS1_IF=ns1-eth0

# Bridge
BR0=br0

# Bridge interfaces
PORT0=em1
PORT1=$VETH_TO_NS0_IF
PORT2=$VETH_TO_NS1_IF

# MAC addresses
MAC0=12:34:56:78:90:ab # for PORT0, smallest among all bridge ports
NS0_MAC=aa:bb:cc:dd:ee:00
NS1_MAC=aa:bb:cc:dd:ee:01
FLOOD_MAC=aa:bb:cc:dd:ee:02

# IP addresses
BR0_IP=192.168.0.1
NS0_IP=192.168.0.2
PREFIX=24

# Pktgen parameters
PGTHREAD=/proc/net/pktgen/kpktgend_0
PGDEV=/proc/net/pktgen/$NS0_IF
PGCTRL=/proc/net/pktgen/pgctrl

# Directories to store logs
RESULT_DIR=/tmp

# Test statistics
SEND_FAILS_ATTACH=0
NO_ENTRY_ATTACH=0
WRONG_ENTRY_ATTACH=0
UNDELETED_ENTRY_ATTACH=0
CAPTURE_DROPS_ATTACH=0
NO_CAPTURE_ATTACH=0
DELAYED_FLOW_CHANGE_ATTACH=0
DROPS_UNKNOWN_REASON_ATTACH=0

SEND_FAILS_DETACH=0
NO_ENTRY_DETACH=0
WRONG_ENTRY_DETACH=0
UNDELETED_ENTRY_DETACH=0
CAPTURE_DROPS_DETACH=0
NO_CAPTURE_DETACH=0
DELAYED_FLOW_CHANGE_DETACH=0
DROPS_UNKNOWN_REASON_DETACH=0

WINDOW_SUM_ATTACH=0
WINDOW_MAX_ATTACH=0
WINDOW_MIN_ATTACH=-1

WINDOW_SUM_DETACH=0
WINDOW_MAX_DETACH=0
WINDOW_MIN_DETACH=-1

PING_FAIL_COUNT_ATTACH=0
PING_ERROR_COUNT_ATTACH=0
PING_FAIL_COUNT_DETACH=0
PING_ERROR_COUNT_DETACH=0

FORWARD_TEST_COUNT_ATTACH=0
FORWARD_TEST_COUNT_DETACH=0
REPLY_TEST_COUNT_ATTACH=0
REPLY_TEST_COUNT_DETACH=0

prepare ()
{
	modprobe pktgen

	# Namespace settings
	for i in 0 1; do
		eval NS=\$NS$i
		eval NS_IF=\$NS${i}_IF
		eval VETH_TO_NS_IF=\$VETH_TO_NS${i}_IF
		eval NS_MAC=\$NS${i}_MAC

		if ip netns | grep -q $NS; then
			ip netns exec $NS ip link show dev $NS_IF > /dev/null 2>&1 && \
			ip netns exec $NS ip link del $NS_IF
			ip netns del $NS
		fi

		ip netns add $NS

		ip link show dev $VETH_TO_NS_IF > /dev/null 2>&1 && \
		ip link del $VETH_TO_NS_IF
		ip link show dev $NS_IF > /dev/null 2>&1 && ip link del $NS_IF
		ip link add $VETH_TO_NS_IF type veth peer name $NS_IF

		ip link set $NS_IF netns $NS
		ip netns exec $NS ip link set $NS_IF up
		ip link set $VETH_TO_NS_IF address $NS_MAC 
	done

	# PORT0 address setting
	ip link set $PORT0 address $MAC0

	# Bridge settings
	ip link show dev $BR0 > /dev/null 2>&1 && ip link del $BR0
	ip link add $BR0 type bridge

	ip link set $PORT1 master $BR0 # NS0
	ip link set $PORT2 master $BR0 # NS1

	# IP address settings
	ip addr add ${BR0_IP}/$PREFIX dev $BR0
	$NS0CMD ip addr add ${NS0_IP}/$PREFIX dev $NS0_IF

	# Enable bridge and ports
	ip link set $PORT0 up
	ip link set $PORT1 up
	ip link set $PORT2 up
	ip link set $BR0 up
}

# Set a pktgen parameter
pgset ()
{
	PGPATH=$2
	if [ x"$3" == x"bg" -a x"$PGPATH" == x"$PGCTRL" ]; then
		$NS0CMD sh -c "echo $1 > $PGPATH" &
		PG_PID=$!
		ERROR=$?
	else
		$NS0CMD sh -c "echo $1 > $PGPATH"
		ERROR=$?
		RESULT=`$NS0CMD cat $PGPATH`
		if ! echo "$RESULT" | fgrep -q "Result: OK:"; then
			echo "$RESULT" | fgrep Result: 1>&2
			ERROR=1
		fi
	fi
	[ $ERROR -ne 0 ] && return 1
	return 0
}

# Send frames using pktgen
send_frames ()
{
	COUNT=$1
	DST=$2
	BG=$3

	pgset "rem_device_all" $PGTHREAD || return 1
	pgset "add_device $NS0_IF" $PGTHREAD || return 1
	
	pgset "count $COUNT" $PGDEV || return 1
	pgset "pkt_size 60" $PGDEV || return 1
	pgset "dst_mac $DST" $PGDEV || return 1
	pgset "delay 0" $PGDEV || return 1

	pgset "start" $PGCTRL $BG || return 1
	return 0
}

# Send frames and attach/detach PORT0
do_forward_test ()
{
	ATTACH=$1
	if [ x"$ATTACH" == x"attach" ]; then
		MASTER="master $BR0"
	else
		MASTER="nomaster"
	fi

	DUMP_PROG=tcpdump
	DATE=`date +%H%M%S%N`
	BR0_DUMP=${RESULT_DIR}/${BR0}_${DATE}.dump
	BR0_DUMPLOG=${RESULT_DIR}/${BR0}_${DATE}.log
	NS1_IF_DUMP=${RESULT_DIR}/${NS1_IF}_${DATE}.dump
	NS1_IF_DUMPLOG=${RESULT_DIR}/${NS1_IF}_${DATE}.log
	FILTER="udp and dst port 9"

	# Start capturing
	$DUMP_PROG -p -i $BR0 -f "$FILTER" -s 64 \
	-w $BR0_DUMP 2> $BR0_DUMPLOG &
	DUMP_PIDS=$!

	ip netns exec $NS1 $DUMP_PROG -p -i $NS1_IF -f "$FILTER" -s 64 \
	-w $NS1_IF_DUMP 2> $NS1_IF_DUMPLOG &
	DUMP_PIDS="$DUMP_PIDS $!"

	# Wait for capturing start
	BR0_CAPTURE_OK=0
	NS1_IF_CAPTURE_OK=0
	while [ $BR0_CAPTURE_OK -eq 0 -o $NS1_IF_CAPTURE_OK -eq 0 ]; do
		[ $BR0_CAPTURE_OK -eq 0 ] &&  send_frames 1000 "$NS0_MAC" fg
		[ $NS1_IF_CAPTURE_OK -eq 0 ] && send_frames 1000 "$FLOOD_MAC" fg
		sleep 0.2
		[ $BR0_CAPTURE_OK -eq 0 ] && \
		[ `tshark -n -r $BR0_DUMP 2> /dev/null | wc -l` -ne 0 ] && \
		BR0_CAPTURE_OK=1
		[ $NS1_IF_CAPTURE_OK -eq 0 ] && \
		[ `tshark -n -r $NS1_IF_DUMP 2> /dev/null | wc -l` -ne 0 ] && \
		NS1_IF_CAPTURE_OK=1
	done

	# Do test
	PG_PID=""
	if send_frames 0 "$MAC0" bg; then
		# Wait for frame sending start
		sleep 0.2

		# Change traffic destination
		ADD_DEL_IF_TIME=`date +%H%M%S%N`
		ip link set $PORT0 $MASTER

		# Wait for traffic flow change
		sleep 0.1

		SEND_FAILED=0
	else
		SEND_FAILED=1
	fi

	# Stop capturing
	kill $DUMP_PIDS $PG_PID
	wait $DUMP_PIDS $PG_PID 2> /dev/null
	DUMP_PIDS=""
	PG_PID=""

	if [ $SEND_FAILED -eq 0 ]; then
		SEND_ERROR_COUNT=`$NS0CMD tail -1 $PGDEV | \
		sed -n 's/.*errors: \([0-9]\+\)/\1/p'`
		[ "$SEND_ERROR_COUNT" -ne 0 ] && SEND_FAILED=1
	fi
}

validate_fdb ()
{
	local AT_TYPE=$1

	FDB_ENTRY=`bridge fdb show | grep -v self | grep $MAC0`
	if [ -z "$FDB_ENTRY" ]; then
		# Retrieving a particular fdb entry might fail because
		# "bridge fdb show" could consist of multiple netlink
		# recvmsg()s and fdb might changes between each recvmsg().
		# Check fdb again to make sure.
		FDB_ENTRY=`bridge fdb show | grep -v self | grep $MAC0`
	fi

	if [ x"$AT_TYPE" == x"ATTACH" ]; then
		if [ -z "$FDB_ENTRY" ]; then
			NO_ENTRY_ATTACH=`expr $NO_ENTRY_ATTACH + 1`
			return 1
		fi
	
		if ! echo $FDB_ENTRY | grep -q "permanent"; then
			WRONG_ENTRY_ATTACH=`expr $WRONG_ENTRY_ATTACH + 1`
			return 1
		fi
		if ! echo $FDB_ENTRY | grep -q "$PORT0"; then
			WRONG_ENTRY_ATTACH=`expr $WRONG_ENTRY_ATTACH + 1`
			return 1
		fi
	else
		if [ -n "$FDB_ENTRY" ]; then
			if ! echo $FDB_ENTRY | grep -q "$PORT0"; then
				WRONG_ENTRY_DETACH=`expr $WRONG_ENTRY_DETACH + 1`
				return 1
			fi
			UNDELETED_ENTRY_DETACH=`expr $UNDELETED_ENTRY_DETACH + 1`
			return 1
		fi
	fi

	return 0
}

validate_forward ()
{
	if [ x"$1" == x"attach" ]; then
		DUMP_BEFORE_CHANGE=$NS1_IF_DUMP
		DUMP_AFTER_CHANGE=$BR0_DUMP
		local AT_TYPE="ATTACH"
	else
		DUMP_BEFORE_CHANGE=$BR0_DUMP
		DUMP_AFTER_CHANGE=$NS1_IF_DUMP
		local AT_TYPE="DETACH"
	fi

	if [ $SEND_FAILED -eq 1 ]; then
		eval local SEND_FAILS=\$SEND_FAILS_$AT_TYPE
		eval SEND_FAILS_$AT_TYPE=`expr $SEND_FAILS + 1`
		return 2
	fi

	validate_fdb $AT_TYPE
	local RET=$?
	[ $RET -ne 0 ] && return $RET

	# Validate captured data
	BR0_CAPTURE_DROPS=`tail -1 $BR0_DUMPLOG | awk '{print $1}'`
	NS1_IF_CAPTURE_DROPS=`tail -1 $NS1_IF_DUMPLOG | awk '{print $1}'`
	if [ "$BR0_CAPTURE_DROPS" -ne 0 -o \
	     "$NS1_IF_CAPTURE_DROPS" -ne 0 ]; then
		# Kernel failed to store captured frames due to no space
		eval local CAPTURE_DROPS=\$CAPTURE_DROPS_$AT_TYPE
		eval CAPTURE_DROPS_$AT_TYPE=`expr $CAPTURE_DROPS + 1`
		return 2
	fi

	CAPTURED_BEFORE_CHANGE=`tshark -n -r $DUMP_BEFORE_CHANGE -T fields \
	-e frame.number -Y "eth.dst eq $MAC0" 2> /dev/null | wc -l`
	CAPTURED_AFTER_CHANGE=`tshark -n -r $DUMP_AFTER_CHANGE -T fields \
	-e frame.number -Y "eth.dst eq $MAC0" 2> /dev/null | wc -l`
	if [ $CAPTURED_BEFORE_CHANGE -eq 0 ]; then
		# Couldn't captured expected traffic
		# Maybe one of 'sleep's was too short
		eval local NO_CAPTURE=\$NO_CAPTURE_$AT_TYPE
		eval NO_CAPTURE_$AT_TYPE=`expr $NO_CAPTURE + 1`
		return 2
	fi
	if [ $CAPTURED_AFTER_CHANGE -eq 0 ]; then
		# This implies too late traffic flow change
		eval local DELAYED_FLOW_CHANGE=\$DELAYED_FLOW_CHANGE_$AT_TYPE
		eval DELAYED_FLOW_CHANGE_$AT_TYPE=`expr $DELAYED_FLOW_CHANGE + 1`
		return 1
	fi

	LAST_SEQ_BEFORE_CHANGE=`tshark -n -r $DUMP_BEFORE_CHANGE -T fields \
	-e pktgen.seqnum -Y "eth.dst eq $MAC0" 2> /dev/null | tail -1`
	FIRST_SEQ_AFTER_CHANGE=`tshark -n -r $DUMP_AFTER_CHANGE -T fields \
	-e pktgen.seqnum -Y "eth.dst eq $MAC0" 2> /dev/null | head -1`
	LAST_SEQ_AFTER_CHANGE=`tshark -n -r $DUMP_AFTER_CHANGE -T fields \
	-e pktgen.seqnum -Y "eth.dst eq $MAC0" 2> /dev/null | tail -1`

	# Check drops by unknown reason
	if [ `expr $FIRST_SEQ_AFTER_CHANGE - $LAST_SEQ_BEFORE_CHANGE` -ne 1 -o \
	     "$CAPTURED_BEFORE_CHANGE" -ne "$LAST_SEQ_BEFORE_CHANGE" -o \
	     "$CAPTURED_AFTER_CHANGE" -ne \
	     `expr $LAST_SEQ_AFTER_CHANGE - $LAST_SEQ_BEFORE_CHANGE` ]; then
		eval local DROPS_UNKNOWN_REASON=\$DROPS_UNKNOWN_REASON_$AT_TYPE
		eval DROPS_UNKNOWN_REASON_$AT_TYPE=`expr $DROPS_UNKNOWN_REASON + 1`
		return 1
	fi

	# Calculate window time
	TRAFFIC_CHANGED_TIME=`tshark -n -r $DUMP_AFTER_CHANGE -T fields \
	-e frame.time -Y "eth.dst eq $MAC0" 2> /dev/null | head -1 | \
	awk '{print $4}' | sed s/[:.]//g`

	h1=`echo $ADD_DEL_IF_TIME | cut -b 1-2`
	m1=`echo $ADD_DEL_IF_TIME | cut -b 3-4`
	s1=`echo $ADD_DEL_IF_TIME | cut -b 5-6`
	us1=`echo $ADD_DEL_IF_TIME | cut -b 7-12`
	TIME1=`expr $h1 \* 3600 + $m1 \* 60 + $s1`
	TIME1=`expr $TIME1 \* 1000000 + $us1`

	h2=`echo $TRAFFIC_CHANGED_TIME | cut -b 1-2`
	[ "$h2" -lt "$h1" ] && h2=`expr $h2 + 24`
	m2=`echo $TRAFFIC_CHANGED_TIME | cut -b 3-4`
	s2=`echo $TRAFFIC_CHANGED_TIME | cut -b 5-6`
	us2=`echo $TRAFFIC_CHANGED_TIME | cut -b 7-12`
	TIME2=`expr $h2 \* 3600 + $m2 \* 60 + $s2`
	TIME2=`expr $TIME2 \* 1000000 + $us2`

	WINDOW=`expr $TIME2 - $TIME1`
	eval local WINDOW_SUM=\$WINDOW_SUM_$AT_TYPE
	eval WINDOW_SUM_$AT_TYPE=`expr $WINDOW_SUM + $WINDOW`
	eval local WINDOW_MAX=\$WINDOW_MAX_$AT_TYPE
	[ "$WINDOW" -gt "$WINDOW_MAX" ] && eval WINDOW_MAX_$AT_TYPE=$WINDOW
	eval local WINDOW_MIN=\$WINDOW_MIN_$AT_TYPE
	[ "$WINDOW_MIN" -eq -1 -o "$WINDOW" -lt "$WINDOW_MIN" ] && \
	eval WINDOW_MIN_$AT_TYPE=$WINDOW

	return 0
}

cleanup_forward_logs ()
{
	/bin/rm -f "$BR0_DUMP"
	/bin/rm -f "$BR0_DUMPLOG"
	/bin/rm -f "$NS1_IF_DUMP"
	/bin/rm -f "$NS1_IF_DUMPLOG"
}

send_probes ()
{
	: > $PROBE_RESULT
	while :; do
		# Invalidate arp cache
		$NS0CMD ip neigh del $BR0_IP dev $NS0_IF 2> /dev/null
		# Send arp request and echo request.
		# This may fail when mac address of BR0 changes
		# between arp reply and echo request.
		$NS0CMD ping -c 1 -w 1 $BR0_IP > /dev/null 2>&1 || \
		echo >> $PROBE_RESULT
	done
}

do_reply_test ()
{
	ATTACH=$1
	if [ x"$ATTACH" == x"attach" ]; then
		MASTER="master $BR0"
	else
		MASTER="nomaster"
	fi

	DATE=`date +%H%M%S%N`
	PROBE_RESULT=${RESULT_DIR}/PROBE_${DATE}.log

	send_probes &
	PROBE_PID=$!
	sleep 0.1

	# Change traffic destination
	ip link set $PORT0 $MASTER

	# Wait for traffic flow change
	sleep 1.2

	# Stop probing
	kill $PROBE_PID
	wait $PROBE_PID 2> /dev/null
	PROBE_PID=""
}

validate_reply ()
{
	if [ x"$1" == x"attach" ]; then
		local AT_TYPE="ATTACH"
	else
		local AT_TYPE="DETACH"
	fi

	validate_fdb $AT_TYPE
	local RET=$?
	[ $RET -ne 0 ] && return $RET

	# validate ping result
	local PING_FAIL=`cat $PROBE_RESULT | wc -l`
	if [ $PING_FAIL -eq 1 ]; then
		# ping may fail if mac address changes between arp reply and
		# echo request.
		eval local PING_FAIL_COUNT=\$PING_FAIL_COUNT_$AT_TYPE
		eval PING_FAIL_COUNT_$AT_TYPE=`expr $PING_FAIL_COUNT + 1`
		return 2
	elif [ $PING_FAIL -gt 1 ]; then
		# ping should not fail two or more times.
		eval local PING_ERROR_COUNT=\$PING_ERROR_COUNT_$AT_TYPE
		eval PING_ERROR_COUNT_$AT_TYPE=`expr $PING_ERROR_COUNT + 1`
		return 1
	fi

	return 0
}

cleanup_reply_logs ()
{
	/bin/rm -f "$PROBE_RESULT"
}

output_results ()
{
	echo "" 1>&2

	local AT_TYPE
	for AT_TYPE in ATTACH DETACH; do
		eval local NO_ENTRY=\$NO_ENTRY_$AT_TYPE
		eval local WRONG_ENTRY=\$WRONG_ENTRY_$AT_TYPE
		eval local UNDELETED_ENTRY=\$UNDELETED_ENTRY_$AT_TYPE
		eval local DELAYED_FLOW_CHANGE=\$DELAYED_FLOW_CHANGE_$AT_TYPE
		eval local DROPS_UNKNOWN_REASON=\$DROPS_UNKNOWN_REASON_$AT_TYPE
		eval local PING_ERROR_COUNT=\$PING_ERROR_COUNT_$AT_TYPE
		ERROR_COUNT=`expr $NO_ENTRY + $WRONG_ENTRY + $UNDELETED_ENTRY + $DELAYED_FLOW_CHANGE + $DROPS_UNKNOWN_REASON + $PING_ERROR_COUNT`

		eval local SEND_FAILS=\$SEND_FAILS_$AT_TYPE
		eval local CAPTURE_DROPS=\$CAPTURE_DROPS_$AT_TYPE
		eval local NO_CAPTURE=\$NO_CAPTURE_$AT_TYPE
		eval local PING_FAIL_COUNT=\$PING_FAIL_COUNT_$AT_TYPE
		INFO_COUNT=`expr $SEND_FAILS + $CAPTURE_DROPS + $NO_CAPTURE + $PING_FAIL_COUNT`

		eval local FORWARD_TEST_COUNT=\$FORWARD_TEST_COUNT_$AT_TYPE
		eval local REPLY_TEST_COUNT=\$REPLY_TEST_COUNT_$AT_TYPE
		local TEST_COUNT=`expr $FORWARD_TEST_COUNT + $REPLY_TEST_COUNT`

		echo "$AT_TYPE test result:"
		if [ "$ERROR_COUNT" -ne 0 ]; then
			echo "Validation failed: $ERROR_COUNT"
			if [ "$NO_ENTRY" -ne 0 ]; then
				echo -e "\tExpected fdb entry not found: $NO_ENTRY"
			fi
			if [ "$WRONG_ENTRY" -ne 0 ]; then
				echo -e "\tExpected fdb entry had wrong attribute: $WRONG_ENTRY"
			fi
			if [ "$UNDELETED_ENTRY" -ne 0 ]; then
				echo -e "\tUnexpected fdb entry found: $UNDELETED_ENTRY"
			fi
			if [ "$DELAYED_FLOW_CHANGE" -ne 0 ]; then
				echo -e "\tWindow time was too long (over 100 ms): $DELAYED_FLOW_CHANGE"
			fi
			if [ "$DROPS_UNKNOWN_REASON" -ne 0 ]; then
				echo -e "\tCouldn't capture due to unknown reason: $DROPS_UNKNOWN_REASON"
			fi
			if [ "$PING_ERROR_COUNT" -ne 0 ]; then
				echo -e "\tPing failed two or more times: $PING_ERROR_COUNT"
			fi
		else
			if [ "$TEST_COUNT" -ne 0 ]; then
				echo "All validations succeeded."
				echo "Number of valid tests: $TEST_COUNT"
				echo -e "\tForwarding tests: $FORWARD_TEST_COUNT"
				echo -e "\tARP/ICMP tests: $REPLY_TEST_COUNT"
			else
				echo "No valid test was done."
			fi
		fi

		if [ "$FORWARD_TEST_COUNT" -ne 0 ]; then
			eval local WINDOW_SUM=\$WINDOW_SUM_$AT_TYPE
			eval local WINDOW_MAX=\$WINDOW_MAX_$AT_TYPE
			eval local WINDOW_MIN=\$WINDOW_MIN_$AT_TYPE

			echo "Window time summary:"

			WINDOW_AVG=`expr $WINDOW_SUM / $FORWARD_TEST_COUNT`
			WINDOW_AVG_MSEC=`expr $WINDOW_AVG / 1000`
			WINDOW_AVG_USEC=`expr $WINDOW_AVG % 1000 | xargs printf %03d`
			echo -e "\tAverage window time: ${WINDOW_AVG_MSEC}.$WINDOW_AVG_USEC msec"

			WINDOW_MAX_MSEC=`expr $WINDOW_MAX / 1000`
			WINDOW_MAX_USEC=`expr $WINDOW_MAX % 1000 | xargs printf %03d`
			echo -e "\tMax window time: ${WINDOW_MAX_MSEC}.$WINDOW_MAX_USEC msec"

			WINDOW_MIN_MSEC=`expr $WINDOW_MIN / 1000`
			WINDOW_MIN_USEC=`expr $WINDOW_MIN % 1000 | xargs printf %03d`
			echo -e "\tMin window time: ${WINDOW_MIN_MSEC}.$WINDOW_MIN_USEC msec"
		fi

		if [ "$INFO_COUNT" -ne 0 ]; then
			echo "INFO: Some tests failed to execute commands normally."
			echo "      These are not validation failures but test environment issues."
			if [ "$SEND_FAILS" -ne 0 ]; then
				echo -e "\tFrame send fails: $SEND_FAILS"
			fi
			if [ "$CAPTURE_DROPS" -ne 0 ]; then
				echo -e "\tCouldn't capture due to buffer overflow: $CAPTURE_DROPS"
			fi
			if [ "$NO_CAPTURE" -ne 0 ]; then
				echo -e "\tCouldn't capture due to delayed pktgen start: $NO_CAPTURE"
			fi
			if [ "$PING_FAIL_COUNT" -ne 0 ]; then
				echo -e "\tPing failed (only once): $PING_FAIL_COUNT"
				echo -e "\t  Maybe this happened due to timing issue where mac address changes between arp reply and echo request."
			fi
		fi

		echo ""
	done
}

cleanup_settings ()
{
	ip link del $BR0

	local i
	for i in 0 1; do
		eval NS=\$NS$i
		eval VETH_TO_NS_IF=\$VETH_TO_NS${i}_IF

		ip link del $VETH_TO_NS_IF
		ip netns del $NS
	done
}

output_exit ()
{
	output_results
	if [ -n "$DUMP_PIDS$PG_PID$PROBE_PID" ]; then
		kill $DUMP_PIDS $PG_PID $PROBE_PID
		wait $DUMP_PIDS $PG_PID $PROBE_PID 2> /dev/null
	fi
	cleanup_forward_logs
	cleanup_reply_logs
	cleanup_settings
	exit
}

forward_test ()
{
	local j
	for j in attach detach; do
		if [ x"$j" == x"attach" ]; then
			local AT_TYPE=ATTACH
		else
			local AT_TYPE=DETACH
		fi
		do_forward_test $j
		validate_forward $j
		local RET=$?
		if [ $RET -eq 0 ]; then
			echo -n "." 1>&2
			eval local TEST_COUNT=\$FORWARD_TEST_COUNT_$AT_TYPE
			eval FORWARD_TEST_COUNT_$AT_TYPE=`expr $TEST_COUNT + 1`
		elif [ $RET -eq 1 ]; then
			echo -n "!" 1>&2
		else
			echo -n "?" 1>&2
		fi
		cleanup_forward_logs
	done
}

reply_test ()
{
	local j
	for j in attach detach; do
		if [ x"$j" == x"attach" ]; then
			local AT_TYPE=ATTACH
		else
			local AT_TYPE=DETACH
		fi
		do_reply_test $j
		validate_reply $j
		local RET=$?
		if [ $RET -eq 0 ]; then
			echo -n "." 1>&2
			eval local TEST_COUNT=\$REPLY_TEST_COUNT_$AT_TYPE
			eval REPLY_TEST_COUNT_$AT_TYPE=`expr $TEST_COUNT + 1`
		elif [ $RET -eq 1 ]; then
			echo -n "!" 1>&2
		else
			echo -n "?" 1>&2
		fi
		cleanup_reply_logs
	done
}

prepare

trap output_exit INT TERM
trap "output_results 1>&2" USR1

for i in `seq $ITERATIONS`; do
	forward_test
	reply_test
done

output_exit

---- Test script end ----

---- Test result begin ----

* before this patch set

ATTACH test result:
All validations succeeded.
Number of valid tests: 2000
        Forwarding tests: 1000
        ARP/ICMP tests: 1000
Window time summary:
        Average window time: 1.547 msec
        Max window time: 2.899 msec
        Min window time: 1.328 msec

DETACH test result:
All validations succeeded.
Number of valid tests: 1982
        Forwarding tests: 1000
        ARP/ICMP tests: 982
Window time summary:
        Average window time: 1.276 msec
        Max window time: 4.030 msec
        Min window time: 1.056 msec
INFO: Some tests failed to execute commands normally.
      These are not validation failures but test environment issues.
        Ping failed (only once): 18
          Maybe this happened due to timing issue where mac address changes between arp reply and echo request.


* after this patch set

ATTACH test result:
All validations succeeded.
Number of valid tests: 2000
        Forwarding tests: 1000
        ARP/ICMP tests: 1000
Window time summary:
        Average window time: 1.455 msec
        Max window time: 2.651 msec
        Min window time: 1.256 msec

DETACH test result:
All validations succeeded.
Number of valid tests: 1999
        Forwarding tests: 1000
        ARP/ICMP tests: 999
Window time summary:
        Average window time: 1.382 msec
        Max window time: 4.015 msec
        Min window time: 1.159 msec
INFO: Some tests failed to execute commands normally.
      These are not validation failures but test environment issues.
        Ping failed (only once): 1
          Maybe this happened due to timing issue where mac address changes between arp reply and echo request.

---- Test result end ----

Note: ping failed only once even after applying this patch set.
I'm thinking mac address was changed between arp reply and echo request.
This can happen on not only bridge device but also in any network
environment, and not a bug.

Thanks,
Toshiaki Makita

[PATCH net v2 6/9] bridge: Properly check if local fdb entry can be deleted in br_fdb_change_mac_address

[Toshiaki Makita]

br_fdb_change_mac_address() doesn't check if the local entry has the
same address as any of bridge ports.
Although I'm not sure when it is beneficial, current implementation allow
the bridge device to receive any mac address of its ports.
To preserve this behavior, we have to check if the mac address of the
entry we are deleting is identical to that of any port.

As this check is almost the same as that in br_fdb_changeaddr(), create
a common function fdb_delete_local() and call it from
br_fdb_changeadddr() and br_fdb_change_mac_address().

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_fdb.c | 57 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 32 insertions(+), 25 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index cf8b64e..817f138 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -89,6 +89,34 @@ static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f)
 	call_rcu(&f->rcu, fdb_rcu_free);
 }
 
+/* Delete a local entry if no other port had the same address. */
+static void fdb_delete_local(struct net_bridge *br,
+			     const struct net_bridge_port *p,
+			     struct net_bridge_fdb_entry *f)
+{
+	const unsigned char *addr = f->addr.addr;
+	u16 vid = f->vlan_id;
+	struct net_bridge_port *op;
+
+	/* Maybe another port has same hw addr? */
+	list_for_each_entry(op, &br->port_list, list) {
+		if (op != p && ether_addr_equal(op->dev->dev_addr, addr) &&
+		    (!vid || nbp_vlan_find(op, vid))) {
+			f->dst = op;
+			return;
+		}
+	}
+
+	/* Maybe bridge device has same hw addr? */
+	if (p && ether_addr_equal(br->dev->dev_addr, addr) &&
+	    (!vid || br_vlan_find(br, vid))) {
+		f->dst = NULL;
+		return;
+	}
+
+	fdb_delete(br, f);
+}
+
 void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
 {
 	struct net_bridge *br = p->br;
@@ -107,30 +135,9 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
 
 			f = hlist_entry(h, struct net_bridge_fdb_entry, hlist);
 			if (f->dst == p && f->is_local && !f->added_by_user) {
-				/* maybe another port has same hw addr? */
-				struct net_bridge_port *op;
-				u16 vid = f->vlan_id;
-				list_for_each_entry(op, &br->port_list, list) {
-					if (op != p &&
-					    ether_addr_equal(op->dev->dev_addr,
-							     f->addr.addr) &&
-					    (!vid || nbp_vlan_find(op, vid))) {
-						f->dst = op;
-						goto skip_delete;
-					}
-				}
-
-				/* maybe bridge device has same hw addr? */
-				if (ether_addr_equal(br->dev->dev_addr,
-						     f->addr.addr) &&
-				    (!vid || br_vlan_find(br, vid))) {
-					f->dst = NULL;
-					goto skip_delete;
-				}
-
 				/* delete old one */
-				fdb_delete(br, f);
-skip_delete:
+				fdb_delete_local(br, p, f);
+
 				/* if this port has no vlan information
 				 * configured, we can safely be done at
 				 * this point.
@@ -168,7 +175,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
 	/* If old entry was unassociated with any port, then delete it. */
 	f = __br_fdb_get(br, br->dev->dev_addr, 0);
 	if (f && f->is_local && !f->dst)
-		fdb_delete(br, f);
+		fdb_delete_local(br, NULL, f);
 
 	fdb_insert(br, NULL, newaddr, 0);
 
@@ -183,7 +190,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
 	for_each_set_bit_from(vid, pv->vlan_bitmap, VLAN_N_VID) {
 		f = __br_fdb_get(br, br->dev->dev_addr, vid);
 		if (f && f->is_local && !f->dst)
-			fdb_delete(br, f);
+			fdb_delete_local(br, NULL, f);
 		fdb_insert(br, NULL, newaddr, vid);
 	}
 }
-- 
1.8.1.2

Re: [PATCH net v2 6/9] bridge: Properly check if local fdb entry can be deleted in br_fdb_change_mac_address

[Vlad Yasevich]

On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> br_fdb_change_mac_address() doesn't check if the local entry has the
> same address as any of bridge ports.
> Although I'm not sure when it is beneficial, current implementation allow
> the bridge device to receive any mac address of its ports.
> To preserve this behavior, we have to check if the mac address of the
> entry we are deleting is identical to that of any port.
> 
> As this check is almost the same as that in br_fdb_changeaddr(), create
> a common function fdb_delete_local() and call it from
> br_fdb_changeadddr() and br_fdb_change_mac_address().
> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>  net/bridge/br_fdb.c | 57 ++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 32 insertions(+), 25 deletions(-)
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index cf8b64e..817f138 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -89,6 +89,34 @@ static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f)
>  	call_rcu(&f->rcu, fdb_rcu_free);
>  }
>  
> +/* Delete a local entry if no other port had the same address. */
> +static void fdb_delete_local(struct net_bridge *br,
> +			     const struct net_bridge_port *p,
> +			     struct net_bridge_fdb_entry *f)
> +{
> +	const unsigned char *addr = f->addr.addr;
> +	u16 vid = f->vlan_id;
> +	struct net_bridge_port *op;
> +
> +	/* Maybe another port has same hw addr? */
> +	list_for_each_entry(op, &br->port_list, list) {
> +		if (op != p && ether_addr_equal(op->dev->dev_addr, addr) &&
> +		    (!vid || nbp_vlan_find(op, vid))) {
> +			f->dst = op;
> +			return;
> +		}
> +	}
> +
> +	/* Maybe bridge device has same hw addr? */
> +	if (p && ether_addr_equal(br->dev->dev_addr, addr) &&
> +	    (!vid || br_vlan_find(br, vid))) {
> +		f->dst = NULL;
> +		return;
> +	}
> +
> +	fdb_delete(br, f);
> +}
> +
>  void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
>  {
>  	struct net_bridge *br = p->br;
> @@ -107,30 +135,9 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
>  
>  			f = hlist_entry(h, struct net_bridge_fdb_entry, hlist);
>  			if (f->dst == p && f->is_local && !f->added_by_user) {
> -				/* maybe another port has same hw addr? */
> -				struct net_bridge_port *op;
> -				u16 vid = f->vlan_id;
> -				list_for_each_entry(op, &br->port_list, list) {
> -					if (op != p &&
> -					    ether_addr_equal(op->dev->dev_addr,
> -							     f->addr.addr) &&
> -					    (!vid || nbp_vlan_find(op, vid))) {
> -						f->dst = op;
> -						goto skip_delete;
> -					}
> -				}
> -
> -				/* maybe bridge device has same hw addr? */
> -				if (ether_addr_equal(br->dev->dev_addr,
> -						     f->addr.addr) &&
> -				    (!vid || br_vlan_find(br, vid))) {
> -					f->dst = NULL;
> -					goto skip_delete;
> -				}
> -
>  				/* delete old one */
> -				fdb_delete(br, f);
> -skip_delete:
> +				fdb_delete_local(br, p, f);
> +
>  				/* if this port has no vlan information
>  				 * configured, we can safely be done at
>  				 * this point.
> @@ -168,7 +175,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
>  	/* If old entry was unassociated with any port, then delete it. */
>  	f = __br_fdb_get(br, br->dev->dev_addr, 0);
>  	if (f && f->is_local && !f->dst)
> -		fdb_delete(br, f);
> +		fdb_delete_local(br, NULL, f);

In this series, br_fdb_change_mac_address() is called before
br->dev->dev_addr is set to the new value (patch 4).  As a result,
the above fdb_delete_local() call will not delete the entry because
br->dev->dev_addr will match the f->addr and the function will
return before calling fdb_delete().

-vlad

>  
>  	fdb_insert(br, NULL, newaddr, 0);
>  
> @@ -183,7 +190,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
>  	for_each_set_bit_from(vid, pv->vlan_bitmap, VLAN_N_VID) {
>  		f = __br_fdb_get(br, br->dev->dev_addr, vid);
>  		if (f && f->is_local && !f->dst)
> -			fdb_delete(br, f);
> +			fdb_delete_local(br, NULL, f);
>  		fdb_insert(br, NULL, newaddr, vid);
>  	}
>  }
> 

Re: [PATCH net v2 6/9] bridge: Properly check if local fdb entry can be deleted in br_fdb_change_mac_address

[Vlad Yasevich]

On 12/17/2013 02:00 PM, Vlad Yasevich wrote:
> On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
>> br_fdb_change_mac_address() doesn't check if the local entry has the
>> same address as any of bridge ports.
>> Although I'm not sure when it is beneficial, current implementation allow
>> the bridge device to receive any mac address of its ports.
>> To preserve this behavior, we have to check if the mac address of the
>> entry we are deleting is identical to that of any port.
>>
>> As this check is almost the same as that in br_fdb_changeaddr(), create
>> a common function fdb_delete_local() and call it from
>> br_fdb_changeadddr() and br_fdb_change_mac_address().
>>
>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>> ---
>>  net/bridge/br_fdb.c | 57 ++++++++++++++++++++++++++++++-----------------------
>>  1 file changed, 32 insertions(+), 25 deletions(-)
>>
>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>> index cf8b64e..817f138 100644
>> --- a/net/bridge/br_fdb.c
>> +++ b/net/bridge/br_fdb.c
>> @@ -89,6 +89,34 @@ static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f)
>>  	call_rcu(&f->rcu, fdb_rcu_free);
>>  }
>>  
>> +/* Delete a local entry if no other port had the same address. */
>> +static void fdb_delete_local(struct net_bridge *br,
>> +			     const struct net_bridge_port *p,
>> +			     struct net_bridge_fdb_entry *f)
>> +{
>> +	const unsigned char *addr = f->addr.addr;
>> +	u16 vid = f->vlan_id;
>> +	struct net_bridge_port *op;
>> +
>> +	/* Maybe another port has same hw addr? */
>> +	list_for_each_entry(op, &br->port_list, list) {
>> +		if (op != p && ether_addr_equal(op->dev->dev_addr, addr) &&
>> +		    (!vid || nbp_vlan_find(op, vid))) {
>> +			f->dst = op;
>> +			return;
>> +		}
>> +	}
>> +
>> +	/* Maybe bridge device has same hw addr? */
>> +	if (p && ether_addr_equal(br->dev->dev_addr, addr) &&
>> +	    (!vid || br_vlan_find(br, vid))) {
>> +		f->dst = NULL;
>> +		return;
>> +	}
>> +
>> +	fdb_delete(br, f);
>> +}
>> +
>>  void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
>>  {
>>  	struct net_bridge *br = p->br;
>> @@ -107,30 +135,9 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
>>  
>>  			f = hlist_entry(h, struct net_bridge_fdb_entry, hlist);
>>  			if (f->dst == p && f->is_local && !f->added_by_user) {
>> -				/* maybe another port has same hw addr? */
>> -				struct net_bridge_port *op;
>> -				u16 vid = f->vlan_id;
>> -				list_for_each_entry(op, &br->port_list, list) {
>> -					if (op != p &&
>> -					    ether_addr_equal(op->dev->dev_addr,
>> -							     f->addr.addr) &&
>> -					    (!vid || nbp_vlan_find(op, vid))) {
>> -						f->dst = op;
>> -						goto skip_delete;
>> -					}
>> -				}
>> -
>> -				/* maybe bridge device has same hw addr? */
>> -				if (ether_addr_equal(br->dev->dev_addr,
>> -						     f->addr.addr) &&
>> -				    (!vid || br_vlan_find(br, vid))) {
>> -					f->dst = NULL;
>> -					goto skip_delete;
>> -				}
>> -
>>  				/* delete old one */
>> -				fdb_delete(br, f);
>> -skip_delete:
>> +				fdb_delete_local(br, p, f);
>> +
>>  				/* if this port has no vlan information
>>  				 * configured, we can safely be done at
>>  				 * this point.
>> @@ -168,7 +175,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
>>  	/* If old entry was unassociated with any port, then delete it. */
>>  	f = __br_fdb_get(br, br->dev->dev_addr, 0);
>>  	if (f && f->is_local && !f->dst)
>> -		fdb_delete(br, f);
>> +		fdb_delete_local(br, NULL, f);
> 
> In this series, br_fdb_change_mac_address() is called before
> br->dev->dev_addr is set to the new value (patch 4).  As a result,
> the above fdb_delete_local() call will not delete the entry because
> br->dev->dev_addr will match the f->addr and the function will
> return before calling fdb_delete().

Sorry, missed the check for port pointer in br_delete_local, so this
would work correctly.

Thanks
-vlad

> 
> -vlad
> 
>>  
>>  	fdb_insert(br, NULL, newaddr, 0);
>>  
>> @@ -183,7 +190,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
>>  	for_each_set_bit_from(vid, pv->vlan_bitmap, VLAN_N_VID) {
>>  		f = __br_fdb_get(br, br->dev->dev_addr, vid);
>>  		if (f && f->is_local && !f->dst)
>> -			fdb_delete(br, f);
>> +			fdb_delete_local(br, NULL, f);
>>  		fdb_insert(br, NULL, newaddr, vid);
>>  	}
>>  }
>>
> 

[PATCH net v2 7/9] bridge: Properly check if local fdb entry can be deleted in br_fdb_delete_by_port

[Toshiaki Makita]

br_fdb_delete_by_port() doesn't care about vlan and mac address of the
bridge device.

As the check is almost the same as mac address changing, slightly modify
fdb_delete_local() and use it.

Note:
- We change the dst of a local entry when the same address is found.
  This occurs in the case kernel has inserted the same address for another
  port but has failed due to dup. We can regard changing dst as deleting
  old one and inserting new one that should have been added by the dup
  port, so we can always set its added_by_user to 0 in fdb_delete_local().

- This is a slight change in behavior where the bridge device can receive
  the traffic to the old address during the short window between calling
  del_nbp() and br_stp_recalculate_bridge_id() in br_del_if(). However,
  it is not a problem because we still have the address on the bridge device.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_fdb.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 817f138..bd43cb1 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -103,6 +103,7 @@ static void fdb_delete_local(struct net_bridge *br,
 		if (op != p && ether_addr_equal(op->dev->dev_addr, addr) &&
 		    (!vid || nbp_vlan_find(op, vid))) {
 			f->dst = op;
+			f->added_by_user = 0;
 			return;
 		}
 	}
@@ -111,6 +112,7 @@ static void fdb_delete_local(struct net_bridge *br,
 	if (p && ether_addr_equal(br->dev->dev_addr, addr) &&
 	    (!vid || br_vlan_find(br, vid))) {
 		f->dst = NULL;
+		f->added_by_user = 0;
 		return;
 	}
 
@@ -261,26 +263,11 @@ void br_fdb_delete_by_port(struct net_bridge *br,
 
 			if (f->is_static && !do_all)
 				continue;
-			/*
-			 * if multiple ports all have the same device address
-			 * then when one port is deleted, assign
-			 * the local entry to other port
-			 */
-			if (f->is_local) {
-				struct net_bridge_port *op;
-				list_for_each_entry(op, &br->port_list, list) {
-					if (op != p &&
-					    ether_addr_equal(op->dev->dev_addr,
-							     f->addr.addr)) {
-						f->dst = op;
-						f->added_by_user = 0;
-						goto skip_delete;
-					}
-				}
-			}
 
-			fdb_delete(br, f);
-		skip_delete: ;
+			if (f->is_local)
+				fdb_delete_local(br, p, f);
+			else
+				fdb_delete(br, f);
 		}
 	}
 	spin_unlock_bh(&br->hash_lock);
-- 
1.8.1.2

Re: [PATCH net v2 7/9] bridge: Properly check if local fdb entry can be deleted in br_fdb_delete_by_port

[Vlad Yasevich]

On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> br_fdb_delete_by_port() doesn't care about vlan and mac address of the
> bridge device.
> 
> As the check is almost the same as mac address changing, slightly modify
> fdb_delete_local() and use it.
> 
> Note:
> - We change the dst of a local entry when the same address is found.
>   This occurs in the case kernel has inserted the same address for another
>   port but has failed due to dup. We can regard changing dst as deleting
>   old one and inserting new one that should have been added by the dup
>   port, so we can always set its added_by_user to 0 in fdb_delete_local().

I disagree.  What happens if the user tries add a duplicate fdb with
the local bit set?  That is permitted and in fact a default because in
iproute right now.  That fdb should persist until the port is removed or
user removes the fdb.

added_by_user flag should only be changed in the netlink code since the
user has full control of it.

-vlad
> 
> - This is a slight change in behavior where the bridge device can receive
>   the traffic to the old address during the short window between calling
>   del_nbp() and br_stp_recalculate_bridge_id() in br_del_if(). However,
>   it is not a problem because we still have the address on the bridge device.
> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>  net/bridge/br_fdb.c | 25 ++++++-------------------
>  1 file changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 817f138..bd43cb1 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -103,6 +103,7 @@ static void fdb_delete_local(struct net_bridge *br,
>  		if (op != p && ether_addr_equal(op->dev->dev_addr, addr) &&
>  		    (!vid || nbp_vlan_find(op, vid))) {
>  			f->dst = op;
> +			f->added_by_user = 0;
>  			return;
>  		}
>  	}
> @@ -111,6 +112,7 @@ static void fdb_delete_local(struct net_bridge *br,
>  	if (p && ether_addr_equal(br->dev->dev_addr, addr) &&
>  	    (!vid || br_vlan_find(br, vid))) {
>  		f->dst = NULL;
> +		f->added_by_user = 0;
>  		return;
>  	}
>  
> @@ -261,26 +263,11 @@ void br_fdb_delete_by_port(struct net_bridge *br,
>  
>  			if (f->is_static && !do_all)
>  				continue;
> -			/*
> -			 * if multiple ports all have the same device address
> -			 * then when one port is deleted, assign
> -			 * the local entry to other port
> -			 */
> -			if (f->is_local) {
> -				struct net_bridge_port *op;
> -				list_for_each_entry(op, &br->port_list, list) {
> -					if (op != p &&
> -					    ether_addr_equal(op->dev->dev_addr,
> -							     f->addr.addr)) {
> -						f->dst = op;
> -						f->added_by_user = 0;
> -						goto skip_delete;
> -					}
> -				}
> -			}
>  
> -			fdb_delete(br, f);
> -		skip_delete: ;
> +			if (f->is_local)
> +				fdb_delete_local(br, p, f);
> +			else
> +				fdb_delete(br, f);
>  		}
>  	}
>  	spin_unlock_bh(&br->hash_lock);
> 

Re: [PATCH net v2 7/9] bridge: Properly check if local fdb entry can be deleted in br_fdb_delete_by_port

[Toshiaki Makita]

On Tue, 2013-12-17 at 14:12 -0500, Vlad Yasevich wrote:
> On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> > br_fdb_delete_by_port() doesn't care about vlan and mac address of the
> > bridge device.
> > 
> > As the check is almost the same as mac address changing, slightly modify
> > fdb_delete_local() and use it.
> > 
> > Note:
> > - We change the dst of a local entry when the same address is found.
> >   This occurs in the case kernel has inserted the same address for another
> >   port but has failed due to dup. We can regard changing dst as deleting
> >   old one and inserting new one that should have been added by the dup
> >   port, so we can always set its added_by_user to 0 in fdb_delete_local().
> 
> I disagree.  What happens if the user tries add a duplicate fdb with
> the local bit set?  

If the user add a dup local entry, the existent entry will be
overwritten and its add_by_user is set to 1 (if !NLM_F_EXCL).
The user never fails to add an entry due to dup in !NLM_F_EXCL case.

> That is permitted and in fact a default because in
> iproute right now.  That fdb should persist until the port is removed or
> user removes the fdb.
> 
> added_by_user flag should only be changed in the netlink code since the
> user has full control of it.

Maybe my changelog is misleading.

br_fdb_delete_by_port() calls fdb_delete_local() for local entries
regardless of its added_by_user. In this case, we have to check if
another port has the same address and vlan, and if found, we have to
create the entry (by changing dst). This is kernel-added entry, not
user-added.

br_fdb_changeaddr()/nbp_vlan_delete() doesn't call fdb_delete_local()
for user-added entry.

So it is safe to set added_by_user to 0 in fdb_delete_local().

will reword the changelog.

Thanks,
Toshiaki Makita

Re: [PATCH net v2 7/9] bridge: Properly check if local fdb entry can be deleted in br_fdb_delete_by_port

[Vlad Yasevich]

On 12/17/2013 09:27 PM, Toshiaki Makita wrote:
> On Tue, 2013-12-17 at 14:12 -0500, Vlad Yasevich wrote:
>> On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
>>> br_fdb_delete_by_port() doesn't care about vlan and mac address of the
>>> bridge device.
>>>
>>> As the check is almost the same as mac address changing, slightly modify
>>> fdb_delete_local() and use it.
>>>
>>> Note:
>>> - We change the dst of a local entry when the same address is found.
>>>   This occurs in the case kernel has inserted the same address for another
>>>   port but has failed due to dup. We can regard changing dst as deleting
>>>   old one and inserting new one that should have been added by the dup
>>>   port, so we can always set its added_by_user to 0 in fdb_delete_local().
>>
>> I disagree.  What happens if the user tries add a duplicate fdb with
>> the local bit set?  
> 
> If the user add a dup local entry, the existent entry will be
> overwritten and its add_by_user is set to 1 (if !NLM_F_EXCL).
> The user never fails to add an entry due to dup in !NLM_F_EXCL case.

You are right.  This is actually a very interesting situation.  User may
over-write the current entry on add, but a delete will remove the entry
instead of restoring original configuration.  I wonder if this was done
on purpose...

> 
>> That is permitted and in fact a default because in
>> iproute right now.  That fdb should persist until the port is removed or
>> user removes the fdb.
>>
>> added_by_user flag should only be changed in the netlink code since the
>> user has full control of it.
> 
> Maybe my changelog is misleading.
> 
> br_fdb_delete_by_port() calls fdb_delete_local() for local entries
> regardless of its added_by_user. In this case, we have to check if
> another port has the same address and vlan, and if found, we have to
> create the entry (by changing dst). This is kernel-added entry, not
> user-added.
> 
> br_fdb_changeaddr()/nbp_vlan_delete() doesn't call fdb_delete_local()
> for user-added entry.
> 
> So it is safe to set added_by_user to 0 in fdb_delete_local().
> 
> will reword the changelog.

Ok.  Thanks for clearing this up.  Looking at patch 6 made it a bit
more clear.  Yes, updating the changelog makes sense since I don't see
this patch introducing the the "change in behavior" you note in the
log.

-vlad

> 
> Thanks,
> Toshiaki Makita
> 
> 

Re: [PATCH net v2 7/9] bridge: Properly check if local fdb entry can be deleted in br_fdb_delete_by_port

[Toshiaki Makita]

On Wed, 2013-12-18 at 12:50 -0500, Vlad Yasevich wrote:
> On 12/17/2013 09:27 PM, Toshiaki Makita wrote:
> > On Tue, 2013-12-17 at 14:12 -0500, Vlad Yasevich wrote:
> >> On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> >>> br_fdb_delete_by_port() doesn't care about vlan and mac address of the
> >>> bridge device.
> >>>
> >>> As the check is almost the same as mac address changing, slightly modify
> >>> fdb_delete_local() and use it.
> >>>
> >>> Note:
> >>> - We change the dst of a local entry when the same address is found.
> >>>   This occurs in the case kernel has inserted the same address for another
> >>>   port but has failed due to dup. We can regard changing dst as deleting
> >>>   old one and inserting new one that should have been added by the dup
> >>>   port, so we can always set its added_by_user to 0 in fdb_delete_local().
> >>
> >> I disagree.  What happens if the user tries add a duplicate fdb with
> >> the local bit set?  
> > 
> > If the user add a dup local entry, the existent entry will be
> > overwritten and its add_by_user is set to 1 (if !NLM_F_EXCL).
> > The user never fails to add an entry due to dup in !NLM_F_EXCL case.
> 
> You are right.  This is actually a very interesting situation.  User may
> over-write the current entry on add, but a delete will remove the entry
> instead of restoring original configuration.  I wonder if this was done
> on purpose...
> 
> > 
> >> That is permitted and in fact a default because in
> >> iproute right now.  That fdb should persist until the port is removed or
> >> user removes the fdb.
> >>
> >> added_by_user flag should only be changed in the netlink code since the
> >> user has full control of it.
> > 
> > Maybe my changelog is misleading.
> > 
> > br_fdb_delete_by_port() calls fdb_delete_local() for local entries
> > regardless of its added_by_user. In this case, we have to check if
> > another port has the same address and vlan, and if found, we have to
> > create the entry (by changing dst). This is kernel-added entry, not
> > user-added.
> > 
> > br_fdb_changeaddr()/nbp_vlan_delete() doesn't call fdb_delete_local()
> > for user-added entry.
> > 
> > So it is safe to set added_by_user to 0 in fdb_delete_local().
> > 
> > will reword the changelog.
> 
> Ok.  Thanks for clearing this up.  Looking at patch 6 made it a bit
> more clear.  Yes, updating the changelog makes sense since I don't see
> this patch introducing the the "change in behavior" you note in the
> log.

This patch actually introduces the behavior change because
br_fdb_delete_by_port() starts to use fdb_delete_local().
Without this patch, del_nbp() never delay the fdb deleting.

Sorry for my confusing logs.

Thanks,
Toshiaki Makita

[PATCH net v2 8/9] bridge: Properly check if local fdb entry can be deleted when deleting vlan

[Toshiaki Makita]

Vlan codes unconditionally delete local fdb entries.
We should consider the possibility that other ports have the same
address and vlan.

Example of problematic case:
  ip link set eth0 address 12:34:56:78:90:ab
  ip link set eth1 address aa:bb:cc:dd:ee:ff
  brctl addif br0 eth0
  brctl addif br0 eth1 # br0 will have mac address 12:34:56:78:90:ab
  bridge vlan add dev eth0 vid 10
  bridge vlan add dev eth1 vid 10
  bridge vlan add dev br0 vid 10 self
We will have fdb entry such that f->dst == eth0, f->vlan_id == 10 and
f->addr == 12:34:56:78:90:ab at this time.
Next, delete eth0 vlan 10.
  bridge vlan del dev eth0 vid 10
In this case, we still need the entry for br0, but it will be deleted.

Note that br0 needs the entry even though its mac address is not set
manually. To delete the entry with proper condition checking,
fdb_delete_local() is suitable to use.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_fdb.c     | 20 ++++++++++++++++++--
 net/bridge/br_private.h |  4 +++-
 net/bridge/br_vlan.c    |  8 ++------
 3 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index bd43cb1..38cd29d 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -27,6 +27,9 @@
 #include "br_private.h"
 
 static struct kmem_cache *br_fdb_cache __read_mostly;
+static struct net_bridge_fdb_entry *fdb_find(struct hlist_head *head,
+					     const unsigned char *addr,
+					     __u16 vid);
 static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
 		      const unsigned char *addr, u16 vid);
 static void fdb_notify(struct net_bridge *br,
@@ -119,6 +122,20 @@ static void fdb_delete_local(struct net_bridge *br,
 	fdb_delete(br, f);
 }
 
+void br_fdb_find_delete_local(struct net_bridge *br,
+			      const struct net_bridge_port *p,
+			      const unsigned char *addr, u16 vid)
+{
+	struct hlist_head *head = &br->hash[br_mac_hash(addr, vid)];
+	struct net_bridge_fdb_entry *f;
+
+	spin_lock_bh(&br->hash_lock);
+	f = fdb_find(head, addr, vid);
+	if (f && f->is_local && !f->added_by_user && f->dst == p)
+		fdb_delete_local(br, p, f);
+	spin_unlock_bh(&br->hash_lock);
+}
+
 void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
 {
 	struct net_bridge *br = p->br;
@@ -766,8 +783,7 @@ out:
 	return err;
 }
 
-int fdb_delete_by_addr(struct net_bridge *br, const u8 *addr,
-		       u16 vlan)
+static int fdb_delete_by_addr(struct net_bridge *br, const u8 *addr, u16 vlan)
 {
 	struct hlist_head *head = &br->hash[br_mac_hash(addr, vlan)];
 	struct net_bridge_fdb_entry *fdb;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 868c059..14c74c2 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -379,6 +379,9 @@ static inline void br_netpoll_disable(struct net_bridge_port *p)
 int br_fdb_init(void);
 void br_fdb_fini(void);
 void br_fdb_flush(struct net_bridge *br);
+void br_fdb_find_delete_local(struct net_bridge *br,
+			      const struct net_bridge_port *p,
+			      const unsigned char *addr, u16 vid);
 void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr);
 void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr);
 void br_fdb_cleanup(unsigned long arg);
@@ -393,7 +396,6 @@ int br_fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
 		  const unsigned char *addr, u16 vid);
 void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 		   const unsigned char *addr, u16 vid);
-int fdb_delete_by_addr(struct net_bridge *br, const u8 *addr, u16 vid);
 
 int br_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
 		  struct net_device *dev, const unsigned char *addr);
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index f87ab88f..24db71d 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -296,9 +296,7 @@ int br_vlan_delete(struct net_bridge *br, u16 vid)
 	if (!pv)
 		return -EINVAL;
 
-	spin_lock_bh(&br->hash_lock);
-	fdb_delete_by_addr(br, br->dev->dev_addr, vid);
-	spin_unlock_bh(&br->hash_lock);
+	br_fdb_find_delete_local(br, NULL, br->dev->dev_addr, vid);
 
 	__vlan_del(pv, vid);
 	return 0;
@@ -399,9 +397,7 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
 	if (!pv)
 		return -EINVAL;
 
-	spin_lock_bh(&port->br->hash_lock);
-	fdb_delete_by_addr(port->br, port->dev->dev_addr, vid);
-	spin_unlock_bh(&port->br->hash_lock);
+	br_fdb_find_delete_local(port->br, port, port->dev->dev_addr, vid);
 
 	return __vlan_del(pv, vid);
 }
-- 
1.8.1.2

Re: [PATCH net v2 8/9] bridge: Properly check if local fdb entry can be deleted when deleting vlan

[Vlad Yasevich]

On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> Vlan codes unconditionally delete local fdb entries.
> We should consider the possibility that other ports have the same
> address and vlan.
> 
> Example of problematic case:
>   ip link set eth0 address 12:34:56:78:90:ab
>   ip link set eth1 address aa:bb:cc:dd:ee:ff
>   brctl addif br0 eth0
>   brctl addif br0 eth1 # br0 will have mac address 12:34:56:78:90:ab
>   bridge vlan add dev eth0 vid 10
>   bridge vlan add dev eth1 vid 10
>   bridge vlan add dev br0 vid 10 self
> We will have fdb entry such that f->dst == eth0, f->vlan_id == 10 and
> f->addr == 12:34:56:78:90:ab at this time.
> Next, delete eth0 vlan 10.
>   bridge vlan del dev eth0 vid 10
> In this case, we still need the entry for br0, but it will be deleted.
> 
> Note that br0 needs the entry even though its mac address is not set
> manually. To delete the entry with proper condition checking,
> fdb_delete_local() is suitable to use.
> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>

Acked-by: Vlad Yasevich <vyasevic@redhat.com>

Note: All these special cases are begging for something in the fdb
to track if this fdb refers to multiple ports or not.  May be
a list of pointers to ports?  Then we can simply check to see if
the list is not empty and assign to the first one in the list..

-vlad

> ---
>  net/bridge/br_fdb.c     | 20 ++++++++++++++++++--
>  net/bridge/br_private.h |  4 +++-
>  net/bridge/br_vlan.c    |  8 ++------
>  3 files changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index bd43cb1..38cd29d 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -27,6 +27,9 @@
>  #include "br_private.h"
>  
>  static struct kmem_cache *br_fdb_cache __read_mostly;
> +static struct net_bridge_fdb_entry *fdb_find(struct hlist_head *head,
> +					     const unsigned char *addr,
> +					     __u16 vid);
>  static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
>  		      const unsigned char *addr, u16 vid);
>  static void fdb_notify(struct net_bridge *br,
> @@ -119,6 +122,20 @@ static void fdb_delete_local(struct net_bridge *br,
>  	fdb_delete(br, f);
>  }
>  
> +void br_fdb_find_delete_local(struct net_bridge *br,
> +			      const struct net_bridge_port *p,
> +			      const unsigned char *addr, u16 vid)
> +{
> +	struct hlist_head *head = &br->hash[br_mac_hash(addr, vid)];
> +	struct net_bridge_fdb_entry *f;
> +
> +	spin_lock_bh(&br->hash_lock);
> +	f = fdb_find(head, addr, vid);
> +	if (f && f->is_local && !f->added_by_user && f->dst == p)
> +		fdb_delete_local(br, p, f);
> +	spin_unlock_bh(&br->hash_lock);
> +}
> +
>  void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
>  {
>  	struct net_bridge *br = p->br;
> @@ -766,8 +783,7 @@ out:
>  	return err;
>  }
>  
> -int fdb_delete_by_addr(struct net_bridge *br, const u8 *addr,
> -		       u16 vlan)
> +static int fdb_delete_by_addr(struct net_bridge *br, const u8 *addr, u16 vlan)
>  {
>  	struct hlist_head *head = &br->hash[br_mac_hash(addr, vlan)];
>  	struct net_bridge_fdb_entry *fdb;
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 868c059..14c74c2 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -379,6 +379,9 @@ static inline void br_netpoll_disable(struct net_bridge_port *p)
>  int br_fdb_init(void);
>  void br_fdb_fini(void);
>  void br_fdb_flush(struct net_bridge *br);
> +void br_fdb_find_delete_local(struct net_bridge *br,
> +			      const struct net_bridge_port *p,
> +			      const unsigned char *addr, u16 vid);
>  void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr);
>  void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr);
>  void br_fdb_cleanup(unsigned long arg);
> @@ -393,7 +396,6 @@ int br_fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
>  		  const unsigned char *addr, u16 vid);
>  void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
>  		   const unsigned char *addr, u16 vid);
> -int fdb_delete_by_addr(struct net_bridge *br, const u8 *addr, u16 vid);
>  
>  int br_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
>  		  struct net_device *dev, const unsigned char *addr);
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index f87ab88f..24db71d 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -296,9 +296,7 @@ int br_vlan_delete(struct net_bridge *br, u16 vid)
>  	if (!pv)
>  		return -EINVAL;
>  
> -	spin_lock_bh(&br->hash_lock);
> -	fdb_delete_by_addr(br, br->dev->dev_addr, vid);
> -	spin_unlock_bh(&br->hash_lock);
> +	br_fdb_find_delete_local(br, NULL, br->dev->dev_addr, vid);
>  
>  	__vlan_del(pv, vid);
>  	return 0;
> @@ -399,9 +397,7 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
>  	if (!pv)
>  		return -EINVAL;
>  
> -	spin_lock_bh(&port->br->hash_lock);
> -	fdb_delete_by_addr(port->br, port->dev->dev_addr, vid);
> -	spin_unlock_bh(&port->br->hash_lock);
> +	br_fdb_find_delete_local(port->br, port, port->dev->dev_addr, vid);
>  
>  	return __vlan_del(pv, vid);
>  }
> 

Re: [PATCH net v2 8/9] bridge: Properly check if local fdb entry can be deleted when deleting vlan

[Toshiaki Makita]

On Tue, 2013-12-17 at 14:34 -0500, Vlad Yasevich wrote:
> On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> > Vlan codes unconditionally delete local fdb entries.
> > We should consider the possibility that other ports have the same
> > address and vlan.
> > 
> > Example of problematic case:
> >   ip link set eth0 address 12:34:56:78:90:ab
> >   ip link set eth1 address aa:bb:cc:dd:ee:ff
> >   brctl addif br0 eth0
> >   brctl addif br0 eth1 # br0 will have mac address 12:34:56:78:90:ab
> >   bridge vlan add dev eth0 vid 10
> >   bridge vlan add dev eth1 vid 10
> >   bridge vlan add dev br0 vid 10 self
> > We will have fdb entry such that f->dst == eth0, f->vlan_id == 10 and
> > f->addr == 12:34:56:78:90:ab at this time.
> > Next, delete eth0 vlan 10.
> >   bridge vlan del dev eth0 vid 10
> > In this case, we still need the entry for br0, but it will be deleted.
> > 
> > Note that br0 needs the entry even though its mac address is not set
> > manually. To delete the entry with proper condition checking,
> > fdb_delete_local() is suitable to use.
> > 
> > Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> 
> Acked-by: Vlad Yasevich <vyasevic@redhat.com>
> 
> Note: All these special cases are begging for something in the fdb
> to track if this fdb refers to multiple ports or not.  May be
> a list of pointers to ports?  Then we can simply check to see if
> the list is not empty and assign to the first one in the list..

Maybe the list will simplify fdb_delete_local().
The difficult point is maintaining the list properly.
I will think about it more deeply.

Thanks,
Toshiaki Makita

[PATCH net v2 9/9] bridge: Prevent possible race condition in br_fdb_change_mac_address

[Toshiaki Makita]

br_fdb_change_mac_address() calls fdb_insert()/fdb_delete() without
br->hash_lock.

These hash list updates are racy with br_fdb_update()/br_fdb_cleanup().

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_fdb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 38cd29d..05fb8da 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -191,6 +191,8 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
 	struct net_port_vlans *pv;
 	u16 vid = 0;
 
+	spin_lock_bh(&br->hash_lock);
+
 	/* If old entry was unassociated with any port, then delete it. */
 	f = __br_fdb_get(br, br->dev->dev_addr, 0);
 	if (f && f->is_local && !f->dst)
@@ -204,7 +206,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
 	 */
 	pv = br_get_vlan_info(br);
 	if (!pv)
-		return;
+		goto out;
 
 	for_each_set_bit_from(vid, pv->vlan_bitmap, VLAN_N_VID) {
 		f = __br_fdb_get(br, br->dev->dev_addr, vid);
@@ -212,6 +214,8 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
 			fdb_delete_local(br, NULL, f);
 		fdb_insert(br, NULL, newaddr, vid);
 	}
+out:
+	spin_unlock_bh(&br->hash_lock);
 }
 
 void br_fdb_cleanup(unsigned long _data)
-- 
1.8.1.2

Re: [PATCH net v2 9/9] bridge: Prevent possible race condition in br_fdb_change_mac_address

[Vlad Yasevich]

On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> br_fdb_change_mac_address() calls fdb_insert()/fdb_delete() without
> br->hash_lock.
> 
> These hash list updates are racy with br_fdb_update()/br_fdb_cleanup().
> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>

Good bug fix.

Acked-by: Vlad Yasevich <vyasevic@redhat.com>

-vlad

> ---
>  net/bridge/br_fdb.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 38cd29d..05fb8da 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -191,6 +191,8 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
>  	struct net_port_vlans *pv;
>  	u16 vid = 0;
>  
> +	spin_lock_bh(&br->hash_lock);
> +
>  	/* If old entry was unassociated with any port, then delete it. */
>  	f = __br_fdb_get(br, br->dev->dev_addr, 0);
>  	if (f && f->is_local && !f->dst)
> @@ -204,7 +206,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
>  	 */
>  	pv = br_get_vlan_info(br);
>  	if (!pv)
> -		return;
> +		goto out;
>  
>  	for_each_set_bit_from(vid, pv->vlan_bitmap, VLAN_N_VID) {
>  		f = __br_fdb_get(br, br->dev->dev_addr, vid);
> @@ -212,6 +214,8 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
>  			fdb_delete_local(br, NULL, f);
>  		fdb_insert(br, NULL, newaddr, vid);
>  	}
> +out:
> +	spin_unlock_bh(&br->hash_lock);
>  }
>  
>  void br_fdb_cleanup(unsigned long _data)
>