All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net 0/3] bonding: a bunch of fixes for dev hwaddr sync in bond_enslave
@ 2018-03-25 17:16 Xin Long
  2018-03-25 17:16 ` [PATCH net 1/3] bonding: fix the err path " Xin Long
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Xin Long @ 2018-03-25 17:16 UTC (permalink / raw)
  To: network dev
  Cc: davem, Jiri Pirko, Wang Chen, Veaceslav Falico, Nikolay Aleksandrov

This patchset is mainly to fix a crash when adding vlan as slave of
bond which is also the parent link in patch 2/3,  and also fix some
err process problems in bond_enslave in patch 1/3 and 3/3.

Xin Long (3):
  bonding: fix the err path for dev hwaddr sync in bond_enslave
  bonding: move dev_mc_sync after master_upper_dev_link in bond_enslave
  bonding: process the err returned by dev_set_allmulti properly in
    bond_enslave

 drivers/net/bonding/bond_main.c | 73 +++++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 36 deletions(-)

-- 
2.1.0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH net 1/3] bonding: fix the err path for dev hwaddr sync in bond_enslave
  2018-03-25 17:16 [PATCH net 0/3] bonding: a bunch of fixes for dev hwaddr sync in bond_enslave Xin Long
@ 2018-03-25 17:16 ` Xin Long
  2018-03-25 17:16   ` [PATCH net 2/3] bonding: move dev_mc_sync after master_upper_dev_link " Xin Long
                     ` (2 more replies)
  2018-03-26 14:16 ` [PATCH net 0/3] bonding: a bunch of fixes " Nikolay Aleksandrov
  2018-03-26 16:52 ` David Miller
  2 siblings, 3 replies; 10+ messages in thread
From: Xin Long @ 2018-03-25 17:16 UTC (permalink / raw)
  To: network dev
  Cc: davem, Jiri Pirko, Wang Chen, Veaceslav Falico, Nikolay Aleksandrov

vlan_vids_add_by_dev is called right after dev hwaddr sync, so on
the err path it should unsync dev hwaddr. Otherwise, the slave
dev's hwaddr will never be unsync when this err happens.

Fixes: 1ff412ad7714 ("bonding: change the bond's vlan syncing functions with the standard ones")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index c669554..0c299de 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1565,7 +1565,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 	if (res) {
 		netdev_err(bond_dev, "Couldn't add bond vlan ids to %s\n",
 			   slave_dev->name);
-		goto err_close;
+		goto err_hwaddr_unsync;
 	}
 
 	prev_slave = bond_last_slave(bond);
@@ -1755,9 +1755,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 	netdev_rx_handler_unregister(slave_dev);
 
 err_detach:
-	if (!bond_uses_primary(bond))
-		bond_hw_addr_flush(bond_dev, slave_dev);
-
 	vlan_vids_del_by_dev(slave_dev, bond_dev);
 	if (rcu_access_pointer(bond->primary_slave) == new_slave)
 		RCU_INIT_POINTER(bond->primary_slave, NULL);
@@ -1771,6 +1768,10 @@ 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);
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net 2/3] bonding: move dev_mc_sync after master_upper_dev_link in bond_enslave
  2018-03-25 17:16 ` [PATCH net 1/3] bonding: fix the err path " Xin Long
@ 2018-03-25 17:16   ` Xin Long
  2018-03-25 17:16     ` [PATCH net 3/3] bonding: process the err returned by dev_set_allmulti properly " Xin Long
  2018-03-26 15:46     ` [PATCH net 2/3] bonding: move dev_mc_sync after master_upper_dev_link " Andy Gospodarek
  2018-03-26 14:06   ` [PATCH net 1/3] bonding: fix the err path for dev hwaddr sync " Nikolay Aleksandrov
  2018-03-26 15:16   ` Andy Gospodarek
  2 siblings, 2 replies; 10+ messages in thread
From: Xin Long @ 2018-03-25 17:16 UTC (permalink / raw)
  To: network dev
  Cc: davem, Jiri Pirko, Wang Chen, Veaceslav Falico, Nikolay Aleksandrov

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.

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>
---
 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);
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net 3/3] bonding: process the err returned by dev_set_allmulti properly in bond_enslave
  2018-03-25 17:16   ` [PATCH net 2/3] bonding: move dev_mc_sync after master_upper_dev_link " Xin Long
@ 2018-03-25 17:16     ` Xin Long
  2018-03-26 16:07       ` Andy Gospodarek
  2018-03-26 15:46     ` [PATCH net 2/3] bonding: move dev_mc_sync after master_upper_dev_link " Andy Gospodarek
  1 sibling, 1 reply; 10+ messages in thread
From: Xin Long @ 2018-03-25 17:16 UTC (permalink / raw)
  To: network dev
  Cc: davem, Jiri Pirko, Wang Chen, Veaceslav Falico, Nikolay Aleksandrov

When dev_set_promiscuity(1) succeeds but dev_set_allmulti(1) fails,
dev_set_promiscuity(-1) should be done before going to the err path.
Otherwise, dev->promiscuity will leak.

Fixes: 7e1a1ac1fbaa ("bonding: Check return of dev_set_promiscuity/allmulti")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 55e1985..b7b1130 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1706,8 +1706,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 		/* set allmulti level to new slave */
 		if (bond_dev->flags & IFF_ALLMULTI) {
 			res = dev_set_allmulti(slave_dev, 1);
-			if (res)
+			if (res) {
+				if (bond_dev->flags & IFF_PROMISC)
+					dev_set_promiscuity(slave_dev, -1);
 				goto err_sysfs_del;
+			}
 		}
 
 		netif_addr_lock_bh(bond_dev);
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH net 1/3] bonding: fix the err path for dev hwaddr sync in bond_enslave
  2018-03-25 17:16 ` [PATCH net 1/3] bonding: fix the err path " Xin Long
  2018-03-25 17:16   ` [PATCH net 2/3] bonding: move dev_mc_sync after master_upper_dev_link " Xin Long
@ 2018-03-26 14:06   ` Nikolay Aleksandrov
  2018-03-26 15:16   ` Andy Gospodarek
  2 siblings, 0 replies; 10+ messages in thread
From: Nikolay Aleksandrov @ 2018-03-26 14:06 UTC (permalink / raw)
  To: Xin Long, network dev
  Cc: davem, Jiri Pirko, Wang Chen, Veaceslav Falico, Nikolay Aleksandrov

On 25/03/18 20:16, Xin Long wrote:
> vlan_vids_add_by_dev is called right after dev hwaddr sync, so on
> the err path it should unsync dev hwaddr. Otherwise, the slave
> dev's hwaddr will never be unsync when this err happens.
> 
> Fixes: 1ff412ad7714 ("bonding: change the bond's vlan syncing functions with the standard ones")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>   drivers/net/bonding/bond_main.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 

Seems I've missed the err path back then.
Thanks,
Reviewed-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net 0/3] bonding: a bunch of fixes for dev hwaddr sync in bond_enslave
  2018-03-25 17:16 [PATCH net 0/3] bonding: a bunch of fixes for dev hwaddr sync in bond_enslave Xin Long
  2018-03-25 17:16 ` [PATCH net 1/3] bonding: fix the err path " Xin Long
@ 2018-03-26 14:16 ` Nikolay Aleksandrov
  2018-03-26 16:52 ` David Miller
  2 siblings, 0 replies; 10+ messages in thread
From: Nikolay Aleksandrov @ 2018-03-26 14:16 UTC (permalink / raw)
  To: Xin Long, network dev
  Cc: davem, Jiri Pirko, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek

On 25/03/18 20:16, Xin Long wrote:
> This patchset is mainly to fix a crash when adding vlan as slave of
> bond which is also the parent link in patch 2/3,  and also fix some
> err process problems in bond_enslave in patch 1/3 and 3/3.
> 
> Xin Long (3):
>    bonding: fix the err path for dev hwaddr sync in bond_enslave
>    bonding: move dev_mc_sync after master_upper_dev_link in bond_enslave
>    bonding: process the err returned by dev_set_allmulti properly in
>      bond_enslave
> 
>   drivers/net/bonding/bond_main.c | 73 +++++++++++++++++++++--------------------
>   1 file changed, 37 insertions(+), 36 deletions(-)
> 

I have no objections to the patches but you've missed to add the bonding maintainers
to the CC list (added now and removed the bouncing emails).

Thanks,
  Nik

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net 1/3] bonding: fix the err path for dev hwaddr sync in bond_enslave
  2018-03-25 17:16 ` [PATCH net 1/3] bonding: fix the err path " Xin Long
  2018-03-25 17:16   ` [PATCH net 2/3] bonding: move dev_mc_sync after master_upper_dev_link " Xin Long
  2018-03-26 14:06   ` [PATCH net 1/3] bonding: fix the err path for dev hwaddr sync " Nikolay Aleksandrov
@ 2018-03-26 15:16   ` Andy Gospodarek
  2 siblings, 0 replies; 10+ messages in thread
From: Andy Gospodarek @ 2018-03-26 15:16 UTC (permalink / raw)
  To: Xin Long
  Cc: network dev, davem, Jiri Pirko, Wang Chen, Veaceslav Falico,
	Nikolay Aleksandrov

On Mon, Mar 26, 2018 at 01:16:45AM +0800, Xin Long wrote:
> vlan_vids_add_by_dev is called right after dev hwaddr sync, so on
> the err path it should unsync dev hwaddr. Otherwise, the slave
> dev's hwaddr will never be unsync when this err happens.
> 
> Fixes: 1ff412ad7714 ("bonding: change the bond's vlan syncing functions with the standard ones")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

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

> ---
>  drivers/net/bonding/bond_main.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index c669554..0c299de 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1565,7 +1565,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  	if (res) {
>  		netdev_err(bond_dev, "Couldn't add bond vlan ids to %s\n",
>  			   slave_dev->name);
> -		goto err_close;
> +		goto err_hwaddr_unsync;
>  	}
>  
>  	prev_slave = bond_last_slave(bond);
> @@ -1755,9 +1755,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  	netdev_rx_handler_unregister(slave_dev);
>  
>  err_detach:
> -	if (!bond_uses_primary(bond))
> -		bond_hw_addr_flush(bond_dev, slave_dev);
> -
>  	vlan_vids_del_by_dev(slave_dev, bond_dev);
>  	if (rcu_access_pointer(bond->primary_slave) == new_slave)
>  		RCU_INIT_POINTER(bond->primary_slave, NULL);
> @@ -1771,6 +1768,10 @@ 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);

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net 2/3] bonding: move dev_mc_sync after master_upper_dev_link in bond_enslave
  2018-03-25 17:16   ` [PATCH net 2/3] bonding: move dev_mc_sync after master_upper_dev_link " Xin Long
  2018-03-25 17:16     ` [PATCH net 3/3] bonding: process the err returned by dev_set_allmulti properly " Xin Long
@ 2018-03-26 15:46     ` Andy Gospodarek
  1 sibling, 0 replies; 10+ messages in thread
From: Andy Gospodarek @ 2018-03-26 15:46 UTC (permalink / raw)
  To: Xin Long
  Cc: network dev, davem, Jiri Pirko, Wang Chen, Veaceslav Falico,
	Nikolay Aleksandrov

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);

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net 3/3] bonding: process the err returned by dev_set_allmulti properly in bond_enslave
  2018-03-25 17:16     ` [PATCH net 3/3] bonding: process the err returned by dev_set_allmulti properly " Xin Long
@ 2018-03-26 16:07       ` Andy Gospodarek
  0 siblings, 0 replies; 10+ messages in thread
From: Andy Gospodarek @ 2018-03-26 16:07 UTC (permalink / raw)
  To: Xin Long
  Cc: network dev, davem, Jiri Pirko, Wang Chen, Veaceslav Falico,
	Nikolay Aleksandrov

On Mon, Mar 26, 2018 at 01:16:47AM +0800, Xin Long wrote:
> When dev_set_promiscuity(1) succeeds but dev_set_allmulti(1) fails,
> dev_set_promiscuity(-1) should be done before going to the err path.
> Otherwise, dev->promiscuity will leak.
> 
> Fixes: 7e1a1ac1fbaa ("bonding: Check return of dev_set_promiscuity/allmulti")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

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

> ---
>  drivers/net/bonding/bond_main.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 55e1985..b7b1130 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1706,8 +1706,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  		/* set allmulti level to new slave */
>  		if (bond_dev->flags & IFF_ALLMULTI) {
>  			res = dev_set_allmulti(slave_dev, 1);
> -			if (res)
> +			if (res) {
> +				if (bond_dev->flags & IFF_PROMISC)
> +					dev_set_promiscuity(slave_dev, -1);
>  				goto err_sysfs_del;
> +			}
>  		}
>  
>  		netif_addr_lock_bh(bond_dev);

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net 0/3] bonding: a bunch of fixes for dev hwaddr sync in bond_enslave
  2018-03-25 17:16 [PATCH net 0/3] bonding: a bunch of fixes for dev hwaddr sync in bond_enslave Xin Long
  2018-03-25 17:16 ` [PATCH net 1/3] bonding: fix the err path " Xin Long
  2018-03-26 14:16 ` [PATCH net 0/3] bonding: a bunch of fixes " Nikolay Aleksandrov
@ 2018-03-26 16:52 ` David Miller
  2 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2018-03-26 16:52 UTC (permalink / raw)
  To: lucien.xin; +Cc: netdev, jiri, wangchen, vfalico, nikolay

From: Xin Long <lucien.xin@gmail.com>
Date: Mon, 26 Mar 2018 01:16:44 +0800

> This patchset is mainly to fix a crash when adding vlan as slave of
> bond which is also the parent link in patch 2/3,  and also fix some
> err process problems in bond_enslave in patch 1/3 and 3/3.

Series applied and queued up for -stable.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-03-26 16:52 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-25 17:16 [PATCH net 0/3] bonding: a bunch of fixes for dev hwaddr sync in bond_enslave Xin Long
2018-03-25 17:16 ` [PATCH net 1/3] bonding: fix the err path " Xin Long
2018-03-25 17:16   ` [PATCH net 2/3] bonding: move dev_mc_sync after master_upper_dev_link " Xin Long
2018-03-25 17:16     ` [PATCH net 3/3] bonding: process the err returned by dev_set_allmulti properly " Xin Long
2018-03-26 16:07       ` Andy Gospodarek
2018-03-26 15:46     ` [PATCH net 2/3] bonding: move dev_mc_sync after master_upper_dev_link " Andy Gospodarek
2018-03-26 14:06   ` [PATCH net 1/3] bonding: fix the err path for dev hwaddr sync " Nikolay Aleksandrov
2018-03-26 15:16   ` Andy Gospodarek
2018-03-26 14:16 ` [PATCH net 0/3] bonding: a bunch of fixes " Nikolay Aleksandrov
2018-03-26 16:52 ` David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.