[PATCH 0/5] bonding: enslave and locking bug fixes

[PATCH 0/5] bonding: enslave and locking bug fixes

[Nikolay Aleksandrov]

Hi,
This patch-set fixes mainly bugs on enslave failure and one occasion
of a needed locking. The patches are:

	1. On enslave failure mc addresses are not flushed from the slave
	2. On enslave failure vlans are not cleaned up from the slave
	3. On enslave failure the bond's primary and curr_active_slave
	   are not cleaned up (which might result in use of freed memory)
	4. On enslave failure netpoll is not disabled which might result in
	   a memory leak
	5. In bond_mc_swap() the bond's mc addr list is walked without
	   netif_addr_lock, since it can be called without rtnl, add it

Best regards,
 Nik

Nikolay Aleksandrov (5):
  bonding: mc addresses don't get deleted on enslave failure
  bonding: vlans don't get deleted on enslave failure
  bonding: primary_slave & curr_active_slave are not cleaned on enslave
    failure
  bonding: disable netpoll on enslave failure
  bonding: in bond_mc_swap() bond's mc addr list is walked without lock

 drivers/net/bonding/bond_main.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

-- 
1.8.1.4

[PATCH 1/5] bonding: mc addresses don't get deleted on enslave failure

[Nikolay Aleksandrov]

Add dev_mc_del after err_detach as that's the first error path
after they're added. The main issue is the mc addresses' refcount
which only gets bumped up.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
 drivers/net/bonding/bond_main.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index a61a760..dc0153b 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1523,6 +1523,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 	struct sockaddr addr;
 	int link_reporting;
 	int res = 0;
+	u8 lacpdu_multicast[ETH_ALEN] = MULTICAST_LACPDU_ADDR;
 
 	if (!bond->params.use_carrier &&
 	    slave_dev->ethtool_ops->get_link == NULL &&
@@ -1726,12 +1727,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		netif_addr_unlock_bh(bond_dev);
 	}
 
-	if (bond->params.mode == BOND_MODE_8023AD) {
+	if (bond->params.mode == 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_add_vlans_on_slave(bond, slave_dev);
 
@@ -1901,6 +1899,11 @@ err_dest_symlinks:
 	bond_destroy_slave_symlinks(bond_dev, slave_dev);
 
 err_detach:
+	if (!USES_PRIMARY(bond->params.mode)) {
+		netif_addr_lock_bh(bond_dev);
+		bond_mc_list_flush(bond_dev, slave_dev);
+		netif_addr_unlock_bh(bond_dev);
+	}
 	write_lock_bh(&bond->lock);
 	bond_detach_slave(bond, new_slave);
 	write_unlock_bh(&bond->lock);
-- 
1.8.1.4

[PATCH 2/5] bonding: vlans don't get deleted on enslave failure

[Nikolay Aleksandrov]

The main problem is with vid refcount which only gets bumped up.
Delete the vlans after err_detach as that's the first error path
after the vlans are added.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
 drivers/net/bonding/bond_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index dc0153b..bf5a9df 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1904,6 +1904,7 @@ err_detach:
 		bond_mc_list_flush(bond_dev, slave_dev);
 		netif_addr_unlock_bh(bond_dev);
 	}
+	bond_del_vlans_from_slave(bond, slave_dev);
 	write_lock_bh(&bond->lock);
 	bond_detach_slave(bond, new_slave);
 	write_unlock_bh(&bond->lock);
-- 
1.8.1.4

[PATCH 3/5] bonding: primary_slave & curr_active_slave are not cleaned on enslave failure

[Nikolay Aleksandrov]

On enslave failure primary_slave can point to new_slave which is to be
freed, and the same applies to curr_active_slave. So check if this is
the case and clean up properly after err_detach because that's the first
error code path after they're set.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
 drivers/net/bonding/bond_main.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index bf5a9df..a04a018 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1907,7 +1907,17 @@ err_detach:
 	bond_del_vlans_from_slave(bond, slave_dev);
 	write_lock_bh(&bond->lock);
 	bond_detach_slave(bond, new_slave);
+	if (bond->primary_slave == new_slave)
+		bond->primary_slave = NULL;
 	write_unlock_bh(&bond->lock);
+	if (bond->curr_active_slave == new_slave) {
+		read_lock(&bond->lock);
+		write_lock_bh(&bond->curr_slave_lock);
+		bond_change_active_slave(bond, NULL);
+		bond_select_active_slave(bond);
+		write_unlock_bh(&bond->curr_slave_lock);
+		read_unlock(&bond->lock);
+	}
 
 err_close:
 	slave_dev->priv_flags &= ~IFF_BONDING;
-- 
1.8.1.4

[PATCH 4/5] bonding: disable netpoll on enslave failure

[Nikolay Aleksandrov]

slave_disable_netpoll() is not called upon enslave failure which would
lead to a memory leak. Call slave_disable_netpoll() after err_detach as
that's the first error path after enabling netpoll on that slave.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
 drivers/net/bonding/bond_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index a04a018..62bfde7 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1918,6 +1918,7 @@ err_detach:
 		write_unlock_bh(&bond->curr_slave_lock);
 		read_unlock(&bond->lock);
 	}
+	slave_disable_netpoll(new_slave);
 
 err_close:
 	slave_dev->priv_flags &= ~IFF_BONDING;
-- 
1.8.1.4

[PATCH 5/5] bonding: in bond_mc_swap() bond's mc addr list is walked without lock

[Nikolay Aleksandrov]

Use netif_addr_lock_bh() to acquire the appropriate lock before walking.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
 drivers/net/bonding/bond_main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 62bfde7..d7239c9 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -846,8 +846,10 @@ static void bond_mc_swap(struct bonding *bond, struct slave *new_active,
 		if (bond->dev->flags & IFF_ALLMULTI)
 			dev_set_allmulti(old_active->dev, -1);
 
+		netif_addr_lock_bh(bond->dev);
 		netdev_for_each_mc_addr(ha, bond->dev)
 			dev_mc_del(old_active->dev, ha->addr);
+		netif_addr_unlock_bh(bond->dev);
 	}
 
 	if (new_active) {
@@ -858,8 +860,10 @@ static void bond_mc_swap(struct bonding *bond, struct slave *new_active,
 		if (bond->dev->flags & IFF_ALLMULTI)
 			dev_set_allmulti(new_active->dev, 1);
 
+		netif_addr_lock_bh(bond->dev);
 		netdev_for_each_mc_addr(ha, bond->dev)
 			dev_mc_add(new_active->dev, ha->addr);
+		netif_addr_unlock_bh(bond->dev);
 	}
 }
 
-- 
1.8.1.4

Re: [PATCH 1/5] bonding: mc addresses don't get deleted on enslave failure

[Jay Vosburgh]

Nikolay Aleksandrov <nikolay@redhat.com> wrote:

>Add dev_mc_del after err_detach as that's the first error path
>after they're added. The main issue is the mc addresses' refcount
>which only gets bumped up.
>
>Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>

	All 5 of these patches look good to me.  The only minor nits are
that the above description says "dev_mc_del," but the actual function
call added is bond_mc_list_flush (which in turn does call dev_mc_dev),
and that this patch (#1) modifies the lacpdu_multicast variable
initialization, which isn't really necessary for the bug fix.

	-J


>---
> drivers/net/bonding/bond_main.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index a61a760..dc0153b 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1523,6 +1523,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> 	struct sockaddr addr;
> 	int link_reporting;
> 	int res = 0;
>+	u8 lacpdu_multicast[ETH_ALEN] = MULTICAST_LACPDU_ADDR;
>
> 	if (!bond->params.use_carrier &&
> 	    slave_dev->ethtool_ops->get_link == NULL &&
>@@ -1726,12 +1727,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> 		netif_addr_unlock_bh(bond_dev);
> 	}
>
>-	if (bond->params.mode == BOND_MODE_8023AD) {
>+	if (bond->params.mode == 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_add_vlans_on_slave(bond, slave_dev);
>
>@@ -1901,6 +1899,11 @@ err_dest_symlinks:
> 	bond_destroy_slave_symlinks(bond_dev, slave_dev);
>
> err_detach:
>+	if (!USES_PRIMARY(bond->params.mode)) {
>+		netif_addr_lock_bh(bond_dev);
>+		bond_mc_list_flush(bond_dev, slave_dev);
>+		netif_addr_unlock_bh(bond_dev);
>+	}
> 	write_lock_bh(&bond->lock);
> 	bond_detach_slave(bond, new_slave);
> 	write_unlock_bh(&bond->lock);
>-- 
>1.8.1.4

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH 1/5] bonding: mc addresses don't get deleted on enslave failure

[Nikolay Aleksandrov]

On 18/04/13 18:58, Jay Vosburgh wrote:
> Nikolay Aleksandrov <nikolay@redhat.com> wrote:
> 
>> Add dev_mc_del after err_detach as that's the first error path
>> after they're added. The main issue is the mc addresses' refcount
>> which only gets bumped up.
>>
>> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
> 
> 	All 5 of these patches look good to me.  The only minor nits are
> that the above description says "dev_mc_del," but the actual function
> call added is bond_mc_list_flush (which in turn does call dev_mc_dev),
> and that this patch (#1) modifies the lacpdu_multicast variable
> initialization, which isn't really necessary for the bug fix.
> 
> 	-J
> 
> 

Yes, that all is because initially I wrote it directly with dev_mc_del
where I needed the variable and then I changed it to use
bond_mc_list_flush. I'll re-post v2 with updated log message and
without that initialization, sorry about this.

Nik