Re: [PATCH net-next] net: bridge: use device address list insteadof dev_addr (repost)

[Eric Dumazet]

Jiri Pirko a écrit :
> (repost, no modifications)

Well, some changes are welcome :)

> 
> This patch changes the handling of mac addresses of bridge port devices. Now
> it uses previously introduced list of device addresses. It allows the bridge to
> know more then one local mac address per port which is mandatory for the right
> work in some cases.
> 
> Signed-off-by: Jiri Pirko <jpirko@xxxxxxxxxx>
> ---
>  net/bridge/br_fdb.c     |  101 ++++++++++++++++++++++++++++++----------------
>  net/bridge/br_if.c      |    2 +-
>  net/bridge/br_notify.c  |    2 +-
>  net/bridge/br_private.h |    4 +-
>  4 files changed, 70 insertions(+), 39 deletions(-)
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index a48f5ef..1e63f76 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -77,10 +77,26 @@ static inline void fdb_delete(struct net_bridge_fdb_entry *f)
>  	br_fdb_put(f);
>  }
>  
> -void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
> +static bool another_port_has_addr(const struct net_bridge_port *p,
> +				  struct net_bridge_fdb_entry *f)
> +{
> +	struct net_bridge *br = p->br;
> +	struct net_bridge_port *op;
> +
> +	list_for_each_entry(op, &br->port_list, list) {
> +		if (op != p && is_etherdev_addr(op->dev, f->addr.addr)) {
> +			f->dst = op;
> +			return 1;
> +		}
> +	}
> +	return 0;
> +}
> +
> +void br_fdb_changeaddr(struct net_bridge_port *p, struct net_device *dev)
>  {
>  	struct net_bridge *br = p->br;
>  	int i;
> +	struct netdev_hw_addr *ha;
>  
>  	spin_lock_bh(&br->hash_lock);
>  
> @@ -92,26 +108,23 @@ 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) {
> -				/* maybe another port has same hw addr? */
> -				struct net_bridge_port *op;
> -				list_for_each_entry(op, &br->port_list, list) {
> -					if (op != p &&
> -					    !compare_ether_addr(op->dev->dev_addr,
> -								f->addr.addr)) {
> -						f->dst = op;
> -						goto insert;
> -					}
> -				}
> -
> -				/* delete old one */
> -				fdb_delete(f);
> -				goto insert;
> +				/*
> +				 * maybe another port has same hw addr?,
> +				 * if not then delete it
> +				 */
> +				if (!another_port_has_addr(p, f))
> +					fdb_delete(f);
>  			}
>  		}
>  	}
> - insert:
> -	/* insert new address,  may fail if invalid address or dup. */
> -	fdb_insert(br, p, newaddr);
> +
> +	/* insert device addresses, may fail if invalid address. */
> +
> +	rcu_read_lock();

Same problem than a previous patch Jiri.

You should not use rcu_read_lock()/rcu_read_unlock() at all in this context,
since you already own a lock.


> +	for_each_dev_addr(dev, ha) {
> +		fdb_insert(br, p, ha->addr);
> +	}
> +	rcu_read_unlock();
>  
>  	spin_unlock_bh(&br->hash_lock);
>  }
> @@ -189,20 +202,9 @@ void br_fdb_delete_by_port(struct net_bridge *br,
>  			 * 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 &&
> -					    !compare_ether_addr(op->dev->dev_addr,
> -								f->addr.addr)) {
> -						f->dst = op;
> -						goto skip_delete;
> -					}
> -				}
> -			}
> -
> -			fdb_delete(f);
> -		skip_delete: ;
> +			if (!f->is_local ||
> +			    !another_port_has_addr(p, f))
> +				fdb_delete(f);
>  		}
>  	}
>  	spin_unlock_bh(&br->hash_lock);
> @@ -338,7 +340,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
>  }
>  
>  static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
> -		  const unsigned char *addr)
> +		      const unsigned char *addr)
>  {
>  	struct hlist_head *head = &br->hash[br_mac_hash(addr)];
>  	struct net_bridge_fdb_entry *fdb;
> @@ -366,13 +368,42 @@ static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
>  	return 0;
>  }
>  
> +static int fdb_insert_dev(struct net_bridge *br, struct net_bridge_port *source,
> +			  struct net_device *dev)
> +{
> +	struct netdev_hw_addr *ha, *ha2;
> +	struct net_bridge_fdb_entry *fdb;
> +	struct hlist_head *head;
> +	int ret = 0;
> +
> +	rcu_read_lock();


You should not use rcu_read_lock()/rcu_read_unlock() at all in this context


> +	for_each_dev_addr(dev, ha) {
> +		ret = fdb_insert(br, source, ha->addr);
> +		if (ret)
> +			goto unroll;
> +	}
> +	goto unlock;
> +unroll:
> +	for_each_dev_addr(dev, ha2) {
> +		if (ha2 == ha)
> +			break;
> +		head = &br->hash[br_mac_hash(ha2->addr)];
> +		fdb = fdb_find(head, ha2->addr);
> +		if (fdb && fdb->is_local)
> +			fdb_delete(fdb);
> +	}
> +unlock:
> +	rcu_read_unlock();
> +	return ret;
> +}
> +
>  int br_fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
> -		  const unsigned char *addr)
> +		  struct net_device *dev)
>  {
>  	int ret;
>  
>  	spin_lock_bh(&br->hash_lock);
> -	ret = fdb_insert(br, source, addr);
> +	ret = fdb_insert_dev(br, source, dev);
>  	spin_unlock_bh(&br->hash_lock);
>  	return ret;
>  }
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 8a96672..789cb30 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -392,7 +392,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
>  	if (err)
>  		goto err0;
>  
> -	err = br_fdb_insert(br, p, dev->dev_addr);
> +	err = br_fdb_insert(br, p, dev);
>  	if (err)
>  		goto err1;
>  
> diff --git a/net/bridge/br_notify.c b/net/bridge/br_notify.c
> index 763a3ec..1423541 100644
> --- a/net/bridge/br_notify.c
> +++ b/net/bridge/br_notify.c
> @@ -48,7 +48,7 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
>  
>  	case NETDEV_CHANGEADDR:
>  		spin_lock_bh(&br->lock);
> -		br_fdb_changeaddr(p, dev->dev_addr);
> +		br_fdb_changeaddr(p, dev);
>  		br_stp_recalculate_bridge_id(br);
>  		spin_unlock_bh(&br->lock);
>  		break;
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index b6c3b71..65ffe3d 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -148,7 +148,7 @@ extern int br_fdb_init(void);
>  extern void br_fdb_fini(void);
>  extern void br_fdb_flush(struct net_bridge *br);
>  extern void br_fdb_changeaddr(struct net_bridge_port *p,
> -			      const unsigned char *newaddr);
> +			      struct net_device *dev);
>  extern void br_fdb_cleanup(unsigned long arg);
>  extern void br_fdb_delete_by_port(struct net_bridge *br,
>  				  const struct net_bridge_port *p, int do_all);
> @@ -161,7 +161,7 @@ extern int br_fdb_fillbuf(struct net_bridge *br, void *buf,
>  			  unsigned long count, unsigned long off);
>  extern int br_fdb_insert(struct net_bridge *br,
>  			 struct net_bridge_port *source,
> -			 const unsigned char *addr);
> +			 struct net_device *dev);
>  extern void br_fdb_update(struct net_bridge *br,
>  			  struct net_bridge_port *source,
>  			  const unsigned char *addr);
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/