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

[PATCHv2 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

v2: patch 01 - fix log message and remove unnecessary code move

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

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

[Nikolay Aleksandrov]

Add bond_mc_list_flush() after err_detach as that's the first error path
after the addresses are added. The main issue is the mc addresses' refcount
which only gets bumped up.

v2: update log message and don't move code unnecessarily

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

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index a61a760..6fe1544 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1901,6 +1901,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

[PATCHv2 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 6fe1544..2e7fce0 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1906,6 +1906,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

[PATCHv3 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 2e7fce0..c56e132 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1909,7 +1909,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

[PATCHv2 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 c56e132..1f3b1cc 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1920,6 +1920,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

[PATCHv2 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 1f3b1cc..5a2b5f1 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: [PATCHv3 3/5] bonding: primary_slave & curr_active_slave are not cleaned on enslave failure

[Nikolay Aleksandrov]

On 18/04/13 19:33, Nikolay Aleksandrov wrote:
> 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 2e7fce0..c56e132 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1909,7 +1909,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;

I've had too much coffee today, this should've been
v2 although there aren't any changes :-)

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

[Jay Vosburgh]

Nikolay Aleksandrov <nikolay@redhat.com> wrote:

>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
>
>v2: patch 01 - fix log message and remove unnecessary code move

	All look good to me.

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>

	-J

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

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

[David Miller]

From: Jay Vosburgh <fubar@us.ibm.com>
Date: Thu, 18 Apr 2013 11:58:06 -0700

> Nikolay Aleksandrov <nikolay@redhat.com> wrote:
> 
>>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
>>
>>v2: patch 01 - fix log message and remove unnecessary code move
> 
> 	All look good to me.
> 
> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>

Series applied, thanks everyone.