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

[Vlad Yasevich <vyasevic@redhat.com>]

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