Re: [PATCH net 2/3] bonding: move dev_mc_sync after master_upper_dev_link in bond_enslave

[Andy Gospodarek <andrew.gospodarek@broadcom.com>]

On Mon, Mar 26, 2018 at 01:16:46AM +0800, Xin Long wrote:
> Beniamino found a crash when adding vlan as slave of bond which is also
> the parent link:
> 
>   ip link add bond1 type bond
>   ip link set bond1 up
>   ip link add link bond1 vlan1 type vlan id 80
>   ip link set vlan1 master bond1
> 
> The call trace is as below:
> 
>   [<ffffffffa850842a>] queued_spin_lock_slowpath+0xb/0xf
>   [<ffffffffa8515680>] _raw_spin_lock+0x20/0x30
>   [<ffffffffa83f6f07>] dev_mc_sync+0x37/0x80
>   [<ffffffffc08687dc>] vlan_dev_set_rx_mode+0x1c/0x30 [8021q]
>   [<ffffffffa83efd2a>] __dev_set_rx_mode+0x5a/0xa0
>   [<ffffffffa83f7138>] dev_mc_sync_multiple+0x78/0x80
>   [<ffffffffc084127c>] bond_enslave+0x67c/0x1190 [bonding]
>   [<ffffffffa8401909>] do_setlink+0x9c9/0xe50
>   [<ffffffffa8403bf2>] rtnl_newlink+0x522/0x880
>   [<ffffffffa8403ff7>] rtnetlink_rcv_msg+0xa7/0x260
>   [<ffffffffa8424ecb>] netlink_rcv_skb+0xab/0xc0
>   [<ffffffffa83fe498>] rtnetlink_rcv+0x28/0x30
>   [<ffffffffa8424850>] netlink_unicast+0x170/0x210
>   [<ffffffffa8424bf8>] netlink_sendmsg+0x308/0x420
>   [<ffffffffa83cc396>] sock_sendmsg+0xb6/0xf0
> 
> This is actually a dead lock caused by sync slave hwaddr from master when
> the master is the slave's 'slave'. This dead loop check is actually done
> by netdev_master_upper_dev_link. However, Commit 1f718f0f4f97 ("bonding:
> populate neighbour's private on enslave") moved it after dev_mc_sync.
> 
> This patch is to fix it by moving dev_mc_sync after master_upper_dev_link,
> so that this loop check would be earlier than dev_mc_sync. It also moves
> if (mode == BOND_MODE_8023AD) into if (!bond_uses_primary) clause as an
> improvement.

Nice optimization.  :-)

> 
> Note team driver also has this issue, I will fix it in another patch.
> 
> Fixes: 1f718f0f4f97 ("bonding: populate neighbour's private on enslave")
> Reported-by: Beniamino Galvani <bgalvani@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Andy Gospodarek <andy@greyhouse.net>

> ---
>  drivers/net/bonding/bond_main.c | 73 ++++++++++++++++++++---------------------
>  1 file changed, 35 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 0c299de..55e1985 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1528,44 +1528,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  			goto err_close;
>  	}
>  
> -	/* If the mode uses primary, then the following is handled by
> -	 * bond_change_active_slave().
> -	 */
> -	if (!bond_uses_primary(bond)) {
> -		/* set promiscuity level to new slave */
> -		if (bond_dev->flags & IFF_PROMISC) {
> -			res = dev_set_promiscuity(slave_dev, 1);
> -			if (res)
> -				goto err_close;
> -		}
> -
> -		/* set allmulti level to new slave */
> -		if (bond_dev->flags & IFF_ALLMULTI) {
> -			res = dev_set_allmulti(slave_dev, 1);
> -			if (res)
> -				goto err_close;
> -		}
> -
> -		netif_addr_lock_bh(bond_dev);
> -
> -		dev_mc_sync_multiple(slave_dev, bond_dev);
> -		dev_uc_sync_multiple(slave_dev, bond_dev);
> -
> -		netif_addr_unlock_bh(bond_dev);
> -	}
> -
> -	if (BOND_MODE(bond) == BOND_MODE_8023AD) {
> -		/* add lacpdu mc addr to mc list */
> -		u8 lacpdu_multicast[ETH_ALEN] = MULTICAST_LACPDU_ADDR;
> -
> -		dev_mc_add(slave_dev, lacpdu_multicast);
> -	}
> -
>  	res = vlan_vids_add_by_dev(slave_dev, bond_dev);
>  	if (res) {
>  		netdev_err(bond_dev, "Couldn't add bond vlan ids to %s\n",
>  			   slave_dev->name);
> -		goto err_hwaddr_unsync;
> +		goto err_close;
>  	}
>  
>  	prev_slave = bond_last_slave(bond);
> @@ -1725,6 +1692,37 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  		goto err_upper_unlink;
>  	}
>  
> +	/* If the mode uses primary, then the following is handled by
> +	 * bond_change_active_slave().
> +	 */
> +	if (!bond_uses_primary(bond)) {
> +		/* set promiscuity level to new slave */
> +		if (bond_dev->flags & IFF_PROMISC) {
> +			res = dev_set_promiscuity(slave_dev, 1);
> +			if (res)
> +				goto err_sysfs_del;
> +		}
> +
> +		/* set allmulti level to new slave */
> +		if (bond_dev->flags & IFF_ALLMULTI) {
> +			res = dev_set_allmulti(slave_dev, 1);
> +			if (res)
> +				goto err_sysfs_del;
> +		}
> +
> +		netif_addr_lock_bh(bond_dev);
> +		dev_mc_sync_multiple(slave_dev, bond_dev);
> +		dev_uc_sync_multiple(slave_dev, bond_dev);
> +		netif_addr_unlock_bh(bond_dev);
> +
> +		if (BOND_MODE(bond) == BOND_MODE_8023AD) {
> +			/* add lacpdu mc addr to mc list */
> +			u8 lacpdu_multicast[ETH_ALEN] = MULTICAST_LACPDU_ADDR;
> +
> +			dev_mc_add(slave_dev, lacpdu_multicast);
> +		}
> +	}
> +
>  	bond->slave_cnt++;
>  	bond_compute_features(bond);
>  	bond_set_carrier(bond);
> @@ -1748,6 +1746,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  	return 0;
>  
>  /* Undo stages on error */
> +err_sysfs_del:
> +	bond_sysfs_slave_del(new_slave);
> +
>  err_upper_unlink:
>  	bond_upper_dev_unlink(bond, new_slave);
>  
> @@ -1768,10 +1769,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  	synchronize_rcu();
>  	slave_disable_netpoll(new_slave);
>  
> -err_hwaddr_unsync:
> -	if (!bond_uses_primary(bond))
> -		bond_hw_addr_flush(bond_dev, slave_dev);
> -
>  err_close:
>  	slave_dev->priv_flags &= ~IFF_BONDING;
>  	dev_close(slave_dev);