diff mbox

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

Message ID 1387281821-21342-8-git-send-email-makita.toshiaki@lab.ntt.co.jp
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Toshiaki Makita Dec. 17, 2013, 12:03 p.m. UTC
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(-)

Comments

Vlad Yasevich Dec. 17, 2013, 7:12 p.m. UTC | #1
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);
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Toshiaki Makita Dec. 18, 2013, 2:27 a.m. UTC | #2
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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vlad Yasevich Dec. 18, 2013, 5:50 p.m. UTC | #3
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
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Toshiaki Makita Dec. 19, 2013, 12:33 p.m. UTC | #4
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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);