From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Gospodarek Subject: Re: [PATCH net 2/3] bonding: move dev_mc_sync after master_upper_dev_link in bond_enslave Date: Mon, 26 Mar 2018 11:46:16 -0400 Message-ID: <20180326154616.GB92743@C02RW35GFVH8> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: network dev , davem@davemloft.net, Jiri Pirko , Wang Chen , Veaceslav Falico , Nikolay Aleksandrov To: Xin Long Return-path: Received: from mail-qt0-f194.google.com ([209.85.216.194]:46306 "EHLO mail-qt0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751907AbeCZPqU (ORCPT ); Mon, 26 Mar 2018 11:46:20 -0400 Received: by mail-qt0-f194.google.com with SMTP id h4so20029463qtn.13 for ; Mon, 26 Mar 2018 08:46:19 -0700 (PDT) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: 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: > > [] queued_spin_lock_slowpath+0xb/0xf > [] _raw_spin_lock+0x20/0x30 > [] dev_mc_sync+0x37/0x80 > [] vlan_dev_set_rx_mode+0x1c/0x30 [8021q] > [] __dev_set_rx_mode+0x5a/0xa0 > [] dev_mc_sync_multiple+0x78/0x80 > [] bond_enslave+0x67c/0x1190 [bonding] > [] do_setlink+0x9c9/0xe50 > [] rtnl_newlink+0x522/0x880 > [] rtnetlink_rcv_msg+0xa7/0x260 > [] netlink_rcv_skb+0xab/0xc0 > [] rtnetlink_rcv+0x28/0x30 > [] netlink_unicast+0x170/0x210 > [] netlink_sendmsg+0x308/0x420 > [] 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 > Signed-off-by: Xin Long Acked-by: Andy Gospodarek > --- > 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);