linux-kernel - Re: [PATCH 3/4] net: bridge: use device address list instead of dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090413075400.34604e72@nehalam>
Date:	Mon, 13 Apr 2009 07:54:00 -0700
From:	Stephen Hemminger <shemminger@...tta.com>
To:	Jiri Pirko <jpirko@...hat.com>
Cc:	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	jgarzik@...ox.com, davem@...emloft.net,
	bridge@...ts.linux-foundation.org, fubar@...ibm.com,
	bonding-devel@...ts.sourceforge.net, kaber@...sh.net,
	mschmidt@...hat.com, dada1@...mosbay.com, ivecera@...hat.com
Subject: Re: [PATCH 3/4] net: bridge: use device address list instead of
 dev_addr

On Mon, 13 Apr 2009 10:44:08 +0200
Jiri Pirko <jpirko@...hat.com> wrote:

> 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@...hat.com>
> ---
>  net/bridge/br_fdb.c     |  120 +++++++++++++++++++++++++++++++++--------------
>  net/bridge/br_if.c      |    2 +-
>  net/bridge/br_notify.c  |    2 +-
>  net/bridge/br_private.h |    4 +-
>  4 files changed, 89 insertions(+), 39 deletions(-)
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index a48f5ef..6efc556 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -77,10 +77,45 @@ 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)
> +/*
> + * Finds out if passed address is one of the addresses assigned to the device.
> + * Returns 1 on positive result
> + */
> +static inline int is_dev_addr(struct net_device *dev, unsigned char *addr)

Why not a general version in net_device.h or etherdevice.h?

static inline bool is_etherdev_addr(const struct net_device *dev, const unsigned char addr[ETH_ALEN])

> +{
> +	struct hw_addr *ha;
> +	int ret = 1;
> +
> +	netif_dev_addr_lock_bh(dev);
> +	for_each_dev_addr(dev, ha) {
User RCU

> +		ret = compare_ether_addr(addr, ha->addr);
> +		if (!ret)
> +			break;
> +	}
> +	netif_dev_addr_unlock_bh(dev);
> +	return !ret ? 1 : 0;
> +}
> +
> +static int 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_dev_addr(op->dev, f->addr.addr)) {
> +			f->dst = op;
> +			return 1;
> +		}
> +	}
> +	return 0;
> +}

Forwarding database is hot path, people sometimes run lots of devices
on single bridge, doesn't this scale worse?

> +void br_fdb_changeaddr(struct net_bridge_port *p, struct net_device *dev)
>  {
>  	struct net_bridge *br = p->br;
>  	int i;
> +	struct hw_addr *ha;
>  
>  	spin_lock_bh(&br->hash_lock);
>  
> @@ -92,26 +127,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. */
> +
> +	netif_dev_addr_lock_bh(dev);
> +	for_each_dev_addr(dev, ha) {
> +		fdb_insert(br, p, ha->addr);
> +	}
> +	netif_dev_addr_unlock_bh(dev);
>

You added another layer of locking on the already hot bridge
fast path.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/