| 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
| ||
|
Message-ID: <20180326154616.GB92743@C02RW35GFVH8>
Date: Mon, 26 Mar 2018 11:46:16 -0400
From: Andy Gospodarek <andrew.gospodarek@...adcom.com>
To: Xin Long <lucien.xin@...il.com>
Cc: network dev <netdev@...r.kernel.org>, davem@...emloft.net,
Jiri Pirko <jiri@...nulli.us>,
Wang Chen <wangchen@...fujitsu.com>,
Veaceslav Falico <vfalico@...hat.com>,
Nikolay Aleksandrov <nikolay@...hat.com>
Subject: Re: [PATCH net 2/3] bonding: move dev_mc_sync after
master_upper_dev_link in bond_enslave
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@...hat.com>
> Signed-off-by: Xin Long <lucien.xin@...il.com>
Acked-by: Andy Gospodarek <andy@...yhouse.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);
Powered by blists - more mailing lists