Subject: Re: [PATCH 3/7] bridge: Add addresses from static fdbs to bridge address list

From: "Michael S. Tsirkin" <[email-masked]>
Date: Wed, 26 Feb 2014 18:23:05 +0200

On Wed, Feb 26, 2014 at 10:18:21AM -0500, Vlad Yasevich wrote:
> When a static fdb entry is created, add the mac address to the bridge
> address list.  This list is used to program the proper port's
> address list.
> 
> Signed-off-by: Vlad Yasevich <[email-masked]>

Hmm won't this mean learned addresses are missing there?
And if so isn't this a problem when we use this list
in the next patch?

I guess we could limit this to configurations where
learning is also disabled (not just flood) -
seems a bit arbitrary but will likely cover
most use-cases.

> ---
>  include/linux/netdevice.h |   6 +++
>  net/bridge/br_device.c    |   2 +
>  net/bridge/br_fdb.c       | 110 +++++++++++++++++++++++++++++++++++++++++++---
>  net/bridge/br_if.c        |  14 ++++--
>  net/bridge/br_private.h   |   3 ++
>  net/core/dev.c            |   1 +
>  net/core/dev_addr_lists.c |  14 +++---
>  7 files changed, 134 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 440a02e..e29cce1 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2881,6 +2881,12 @@ int register_netdev(struct net_device *dev);
>  void unregister_netdev(struct net_device *dev);
>  
>  /* General hardware address lists handling functions */
> +int __hw_addr_add(struct netdev_hw_addr_list *list,
> +		  const unsigned char *addr, int addr_len,
> +		  unsigned char addr_type);
> +int __hw_addr_del(struct netdev_hw_addr_list *list,
> +		  const unsigned char *addr, int addr_len,
> +		  unsigned char addr_type);
>  int __hw_addr_sync(struct netdev_hw_addr_list *to_list,
>  		   struct netdev_hw_addr_list *from_list, int addr_len);
>  void __hw_addr_unsync(struct netdev_hw_addr_list *to_list,
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 63f0455..1521db6 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -100,6 +100,8 @@ static int br_dev_init(struct net_device *dev)
>  		u64_stats_init(&br_dev_stats->syncp);
>  	}
>  
> +	__hw_addr_init(&br->conf_addrs);
> +
>  	return 0;
>  }
>  
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 9203d5a..ef95e81 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -35,6 +35,9 @@ static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
>  static void fdb_notify(struct net_bridge *br,
>  		       const struct net_bridge_fdb_entry *, int);
>  
> +static int br_addr_add(struct net_bridge *br, const unsigned char *addr);
> +static int br_addr_del(struct net_bridge *br, const unsigned char *addr);
> +
>  static u32 fdb_salt __read_mostly;
>  
>  int __init br_fdb_init(void)
> @@ -87,6 +90,9 @@ static void fdb_rcu_free(struct rcu_head *head)
>  
>  static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f)
>  {
> +	if (f->is_static)
> +		br_addr_del(br, f->addr.addr);
> +
>  	hlist_del_rcu(&f->hlist);
>  	fdb_notify(br, f, RTM_DELNEIGH);
>  	call_rcu(&f->rcu, fdb_rcu_free);
> @@ -466,6 +472,7 @@ static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
>  		return -ENOMEM;
>  
>  	fdb->is_local = fdb->is_static = 1;
> +	br_addr_add(br, addr);
>  	fdb_notify(br, fdb, RTM_NEWNEIGH);
>  	return 0;
>  }
> @@ -678,16 +685,29 @@ static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
>  	}
>  
>  	if (fdb_to_nud(fdb) != state) {
> -		if (state & NUD_PERMANENT)
> -			fdb->is_local = fdb->is_static = 1;
> -		else if (state & NUD_NOARP) {
> +		if (state & NUD_PERMANENT) {
> +			fdb->is_local = 1;
> +			if (!fdb->is_static) {
> +				fdb->is_static = 1;
> +				br_addr_add(br, addr);
> +			}
> +		} else if (state & NUD_NOARP) {
> +			fdb->is_local = 0;
> +			if (!fdb->is_static) {
> +				fdb->is_static = 1;
> +				br_addr_add(br, addr);
> +			}
> +		} else {
>  			fdb->is_local = 0;
> -			fdb->is_static = 1;
> -		} else
> -			fdb->is_local = fdb->is_static = 0;
> +			if (fdb->is_static) {
> +				fdb->is_static = 0;
> +				br_addr_del(br, addr);
> +			}
> +		}
>  
>  		modified = true;
>  	}
> +
>  	fdb->added_by_user = 1;
>  
>  	fdb->used = jiffies;
> @@ -874,3 +894,81 @@ int br_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
>  out:
>  	return err;
>  }
> +
> +
> +/* Sync the current list to the correct flood port.  */
> +void br_fdb_addrs_sync(struct net_bridge *br)
> +{
> +	struct net_bridge_port *p;
> +	int err;
> +
> +	/* This function has to run under RTNL.
> +	 * Program the user added addresses into the proper port.  This
> +	 * fuction follows the following algorithm:
> +	 *   a)  If only 1 port is flooding:
> +	 *       - write all the addresses to this one port.
> +	 */
> +	if (br->n_flood_ports == 1) {
> +		p = br->c_flood_port;
> +		netif_addr_lock(p->dev);
> +		err = __hw_addr_sync(&p->dev->uc, &br->conf_addrs,
> +				     p->dev->addr_len);
> +		if (!err)
> +			__dev_set_rx_mode(p->dev);
> +		netif_addr_unlock(p->dev);
> +
> +	}
> +}
> +
> +void br_fdb_addrs_unsync(struct net_bridge *br)
> +{
> +	struct net_bridge_port *p = br->c_flood_port;
> +
> +	if (!p)
> +		return;
> +
> +	netif_addr_lock(p->dev);
> +	__hw_addr_unsync(&p->dev->uc, &br->conf_addrs, p->dev->addr_len);
> +	__dev_set_rx_mode(p->dev);
> +	netif_addr_unlock(p->dev);
> +}
> +
> +/* When a static FDB entry is added, the mac address from the entry is
> + * added to the bridge private HW address list and all required ports
> + * are then updated with the new information.
> + * Called under RTNL.
> + */
> +static int br_addr_add(struct net_bridge *br, const unsigned char *addr)
> +{
> +	int err;
> +
> +	ASSERT_RTNL();
> +
> +	err = __hw_addr_add(&br->conf_addrs, addr, br->dev->addr_len,
> +			    NETDEV_HW_ADDR_T_UNICAST);
> +
> +	if (!err)
> +		br_fdb_addrs_sync(br);
> +
> +	return err;
> +}
> +
> +/* When a static FDB entry is deleted, the HW address from that entry is
> + * also removed from the bridge private HW address list and updates all
> + * the ports with needed information.
> + * Called under RTNL.
> + */
> +static int br_addr_del(struct net_bridge *br, const unsigned char *addr)
> +{
> +	int err;
> +
> +	ASSERT_RTNL();
> +
> +	err = __hw_addr_del(&br->conf_addrs, addr, br->dev->addr_len,
> +			    NETDEV_HW_ADDR_T_UNICAST);
> +	if (!err)
> +		br_fdb_addrs_sync(br);
> +
> +	return err;
> +}
> +
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index f072b34..e782c2e 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -144,7 +144,7 @@ static void del_nbp(struct net_bridge_port *p)
>  
>  	br_ifinfo_notify(RTM_DELLINK, p);
>  
> -	list_del_rcu(&p->list);
> +	dev->priv_flags &= ~IFF_BRIDGE_PORT;
>  
>  	if (p->flags & BR_FLOOD)
>  		br_del_flood_port(p, br);
> @@ -152,7 +152,7 @@ static void del_nbp(struct net_bridge_port *p)
>  	nbp_vlan_flush(p);
>  	br_fdb_delete_by_port(br, p, 1);
>  
> -	dev->priv_flags &= ~IFF_BRIDGE_PORT;
> +	list_del_rcu(&p->list);
>  
>  	netdev_rx_handler_unregister(dev);
>  
> @@ -473,6 +473,8 @@ static void br_add_flood_port(struct net_bridge_port *p, struct net_bridge *br)
>  	br->n_flood_ports++;
>  	if (br->n_flood_ports == 1)
>  		br->c_flood_port = p;
> +
> +	br_fdb_addrs_sync(br);
>  }
>  
>  static void br_del_flood_port(struct net_bridge_port *p, struct net_bridge *br)
> @@ -485,13 +487,17 @@ static void br_del_flood_port(struct net_bridge_port *p, struct net_bridge *br)
>  	 * set it if it is not set.
>  	 */
>  	br->n_flood_ports--;
> -	if (p == br->c_flood_port)
> +	if (p == br->c_flood_port) {
> +		br_fdb_addrs_unsync(br);
>  		br->c_flood_port = NULL;
> +	}
>  
>  	if (br->n_flood_ports == 1) {
>  		list_for_each_entry(port, &p->br->port_list, list) {
> -			if (port->flags & BR_FLOOD) {
> +			if (br_port_exists(port->dev) &&
> +			    (port->flags & BR_FLOOD)) {
>  				br->c_flood_port = port;
> +				br_fdb_addrs_sync(br);
>  				break;
>  			}
>  		}
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 26a3987..40a6927 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -296,6 +296,7 @@ struct net_bridge
>  	u8				vlan_enabled;
>  	struct net_port_vlans __rcu	*vlan_info;
>  #endif
> +	struct netdev_hw_addr_list	conf_addrs;
>  };
>  
>  struct br_input_skb_cb {
> @@ -397,6 +398,8 @@ int br_fdb_add(struct ndmsg *nlh, struct nlattr *tb[], struct net_device *dev,
>  	       const unsigned char *addr, u16 nlh_flags);
>  int br_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
>  		struct net_device *dev, int idx);
> +void br_fdb_addrs_sync(struct net_bridge *br);
> +void br_fdb_addrs_unsync(struct net_bridge *br);
>  
>  /* br_forward.c */
>  void br_deliver(const struct net_bridge_port *to, struct sk_buff *skb);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 4ad1b78..eca4d476 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5212,6 +5212,7 @@ void __dev_set_rx_mode(struct net_device *dev)
>  	if (ops->ndo_set_rx_mode)
>  		ops->ndo_set_rx_mode(dev);
>  }
> +EXPORT_SYMBOL(__dev_set_rx_mode);
>  
>  void dev_set_rx_mode(struct net_device *dev)
>  {
> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
> index 329d579..3de44a3 100644
> --- a/net/core/dev_addr_lists.c
> +++ b/net/core/dev_addr_lists.c
> @@ -81,13 +81,14 @@ static int __hw_addr_add_ex(struct netdev_hw_addr_list *list,
>  				   sync);
>  }
>  
> -static int __hw_addr_add(struct netdev_hw_addr_list *list,
> -			 const unsigned char *addr, int addr_len,
> -			 unsigned char addr_type)
> +int __hw_addr_add(struct netdev_hw_addr_list *list,
> +		  const unsigned char *addr, int addr_len,
> +		  unsigned char addr_type)
>  {
>  	return __hw_addr_add_ex(list, addr, addr_len, addr_type, false, false,
>  				0);
>  }
> +EXPORT_SYMBOL(__hw_addr_add);
>  
>  static int __hw_addr_del_entry(struct netdev_hw_addr_list *list,
>  			       struct netdev_hw_addr *ha, bool global,
> @@ -127,12 +128,13 @@ static int __hw_addr_del_ex(struct netdev_hw_addr_list *list,
>  	return -ENOENT;
>  }
>  
> -static int __hw_addr_del(struct netdev_hw_addr_list *list,
> -			 const unsigned char *addr, int addr_len,
> -			 unsigned char addr_type)
> +int __hw_addr_del(struct netdev_hw_addr_list *list,
> +		  const unsigned char *addr, int addr_len,
> +		  unsigned char addr_type)
>  {
>  	return __hw_addr_del_ex(list, addr, addr_len, addr_type, false, false);
>  }
> +EXPORT_SYMBOL(__hw_addr_del);
>  
>  static int __hw_addr_sync_one(struct netdev_hw_addr_list *to_list,
>  			       struct netdev_hw_addr *ha,
> -- 
> 1.8.5.3