[PATCH net-next v2 0/2] bonding: vlan handling changes

[PATCH net-next v2 0/2] bonding: vlan handling changes

[Nikolay Aleksandrov]

From: Nikolay Aleksandrov <Nikolay Aleksandrov nikolay@redhat.com>

Hi,
In v2 of these patchset I've dropped the previous patch 01 (vlan 0 fix)
because it'll be taken care of later and these two patches are not
dependent on it.

Patch 01 - switches the bonding to the now standard vlan syncing functions
vlan_vids_add/del_by_dev() and removes the bonding specific ones. In v2
upon vlan addition failure we fail the enslavement with an error message
because otherwise we might delete vlans that weren't added by the bonding.
The only way this may happen is with ENOMEM currently, so we're in trouble
anyway.

Patch 02 - reverts vlan addition in case of bond_add_vlan failure because
otherwise we may get bad vlan refcounts in the slaves.
This patch is queued for net-next because a fix for -net without the list
conversion would mean taking care of special cases and would be really
ugly. Since this bug is not critical, I'd rather have the fix for net-next.
It can happen only with ENOMEM, so again we're already in trouble.

Patches 01 and 02 are not connected and if you decide to drop patch 02
please leave 01 (if accepted).

v2: See the notes, I've explained the changes together and by patch.

Nikolay Aleksandrov (2):
  bonding: change the bond's vlan syncing functions with the standard
    ones
  bonding: unwind on bond_add_vlan failure

 drivers/net/bonding/bond_main.c | 41 +++++++++--------------------------------
 1 file changed, 9 insertions(+), 32 deletions(-)

-- 
1.8.1.4

[PATCH net-next v2 1/2] bonding: change the bond's vlan syncing functions with the standard ones

[Nikolay Aleksandrov]

From: Nikolay Aleksandrov <Nikolay Aleksandrov nikolay@redhat.com>

Now we have vlan_vids_add/del_by_dev() which serve the same purpose as
bond's bond_add/del_vlans_on_slave() with the good side effect of
reverting the changes if one of the additions fails.
There's only 1 change in the behaviour of enslave: if adding of the
vlans to the slave fails, we'll fail the enslaving because otherwise we
might delete some vlan that wasn't added by the bonding.
The only way this may happen is with ENOMEM currently, so we're in trouble
anyway.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
v2: Fail enslavement with an error message if syncing of vlans fails
because otherwise we might delete vlans that weren't added by us.

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

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 5697043..78b0aeb 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -493,33 +493,6 @@ static int bond_vlan_rx_kill_vid(struct net_device *bond_dev,
 	return 0;
 }
 
-static void bond_add_vlans_on_slave(struct bonding *bond, struct net_device *slave_dev)
-{
-	struct vlan_entry *vlan;
-	int res;
-
-	list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
-		res = vlan_vid_add(slave_dev, htons(ETH_P_8021Q),
-				   vlan->vlan_id);
-		if (res)
-			pr_warning("%s: Failed to add vlan id %d to device %s\n",
-				   bond->dev->name, vlan->vlan_id,
-				   slave_dev->name);
-	}
-}
-
-static void bond_del_vlans_from_slave(struct bonding *bond,
-				      struct net_device *slave_dev)
-{
-	struct vlan_entry *vlan;
-
-	list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
-		if (!vlan->vlan_id)
-			continue;
-		vlan_vid_del(slave_dev, htons(ETH_P_8021Q), vlan->vlan_id);
-	}
-}
-
 /*------------------------------- Link status -------------------------------*/
 
 /*
@@ -1630,7 +1603,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		dev_mc_add(slave_dev, lacpdu_multicast);
 	}
 
-	bond_add_vlans_on_slave(bond, slave_dev);
+	if (vlan_vids_add_by_dev(slave_dev, bond_dev)) {
+		pr_err("%s: Error: Couldn't add bond vlan ids to %s\n",
+		       bond_dev->name, slave_dev->name);
+		goto err_close;
+	}
 
 	write_lock_bh(&bond->lock);
 
@@ -1806,7 +1783,7 @@ err_detach:
 	if (!USES_PRIMARY(bond->params.mode))
 		bond_hw_addr_flush(bond_dev, slave_dev);
 
-	bond_del_vlans_from_slave(bond, slave_dev);
+	vlan_vids_del_by_dev(slave_dev, bond_dev);
 	write_lock_bh(&bond->lock);
 	bond_detach_slave(bond, new_slave);
 	if (bond->primary_slave == new_slave)
@@ -2002,7 +1979,7 @@ static int __bond_release_one(struct net_device *bond_dev,
 	/* must do this from outside any spinlocks */
 	bond_destroy_slave_symlinks(bond_dev, slave_dev);
 
-	bond_del_vlans_from_slave(bond, slave_dev);
+	vlan_vids_del_by_dev(slave_dev, bond_dev);
 
 	/* If the mode USES_PRIMARY, then this cases was handled above by
 	 * bond_change_active_slave(..., NULL)
-- 
1.8.1.4

[PATCH net-next v2 2/2] bonding: unwind on bond_add_vlan failure

[Nikolay Aleksandrov]

From: Nikolay Aleksandrov <Nikolay Aleksandrov nikolay@redhat.com>

In case of bond_add_vlan() failure currently we'll have the vlan's
refcnt bumped up in all slaves, but it will never go down because it
failed to get added to the bond, so properly unwind the added vlan if
bond_add_vlan fails.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
v2: no changes

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

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 78b0aeb..4264a76 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -455,13 +455,13 @@ static int bond_vlan_rx_add_vid(struct net_device *bond_dev,
 	if (res) {
 		pr_err("%s: Error: Failed to add vlan id %d\n",
 		       bond_dev->name, vid);
-		return res;
+		goto unwind;
 	}
 
 	return 0;
 
 unwind:
-	/* unwind from head to the slave that failed */
+	/* unwind from the slave that failed */
 	bond_for_each_slave_continue_reverse(bond, slave)
 		vlan_vid_del(slave->dev, proto, vid);
 
-- 
1.8.1.4

Re: [net-next, v2, 1/2] bonding: change the bond's vlan syncing functions with the standard ones

[Veaceslav Falico]

On Tue, Aug 06, 2013 at 12:40:15PM +0200, nikolay@redhat.com wrote:
>From: Nikolay Aleksandrov <Nikolay Aleksandrov nikolay@redhat.com>
>
>Now we have vlan_vids_add/del_by_dev() which serve the same purpose as
>bond's bond_add/del_vlans_on_slave() with the good side effect of
>reverting the changes if one of the additions fails.
>There's only 1 change in the behaviour of enslave: if adding of the
>vlans to the slave fails, we'll fail the enslaving because otherwise we
>might delete some vlan that wasn't added by the bonding.
>The only way this may happen is with ENOMEM currently, so we're in trouble
>anyway.
>
>Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>

Acked-by: Veaceslav Falico <vfalico@redhat.com>

>
>---
>v2: Fail enslavement with an error message if syncing of vlans fails
>because otherwise we might delete vlans that weren't added by us.
>
> drivers/net/bonding/bond_main.c | 37 +++++++------------------------------
> 1 file changed, 7 insertions(+), 30 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 5697043..78b0aeb 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -493,33 +493,6 @@ static int bond_vlan_rx_kill_vid(struct net_device *bond_dev,
> 	return 0;
> }
>
>-static void bond_add_vlans_on_slave(struct bonding *bond, struct net_device *slave_dev)
>-{
>-	struct vlan_entry *vlan;
>-	int res;
>-
>-	list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
>-		res = vlan_vid_add(slave_dev, htons(ETH_P_8021Q),
>-				   vlan->vlan_id);
>-		if (res)
>-			pr_warning("%s: Failed to add vlan id %d to device %s\n",
>-				   bond->dev->name, vlan->vlan_id,
>-				   slave_dev->name);
>-	}
>-}
>-
>-static void bond_del_vlans_from_slave(struct bonding *bond,
>-				      struct net_device *slave_dev)
>-{
>-	struct vlan_entry *vlan;
>-
>-	list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
>-		if (!vlan->vlan_id)
>-			continue;
>-		vlan_vid_del(slave_dev, htons(ETH_P_8021Q), vlan->vlan_id);
>-	}
>-}
>-
> /*------------------------------- Link status -------------------------------*/
>
> /*
>@@ -1630,7 +1603,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> 		dev_mc_add(slave_dev, lacpdu_multicast);
> 	}
>
>-	bond_add_vlans_on_slave(bond, slave_dev);
>+	if (vlan_vids_add_by_dev(slave_dev, bond_dev)) {
>+		pr_err("%s: Error: Couldn't add bond vlan ids to %s\n",
>+		       bond_dev->name, slave_dev->name);
>+		goto err_close;
>+	}
>
> 	write_lock_bh(&bond->lock);
>
>@@ -1806,7 +1783,7 @@ err_detach:
> 	if (!USES_PRIMARY(bond->params.mode))
> 		bond_hw_addr_flush(bond_dev, slave_dev);
>
>-	bond_del_vlans_from_slave(bond, slave_dev);
>+	vlan_vids_del_by_dev(slave_dev, bond_dev);
> 	write_lock_bh(&bond->lock);
> 	bond_detach_slave(bond, new_slave);
> 	if (bond->primary_slave == new_slave)
>@@ -2002,7 +1979,7 @@ static int __bond_release_one(struct net_device *bond_dev,
> 	/* must do this from outside any spinlocks */
> 	bond_destroy_slave_symlinks(bond_dev, slave_dev);
>
>-	bond_del_vlans_from_slave(bond, slave_dev);
>+	vlan_vids_del_by_dev(slave_dev, bond_dev);
>
> 	/* If the mode USES_PRIMARY, then this cases was handled above by
> 	 * bond_change_active_slave(..., NULL)

Re: [net-next,v2,2/2] bonding: unwind on bond_add_vlan failure

[Veaceslav Falico]

On Tue, Aug 06, 2013 at 12:40:16PM +0200, nikolay@redhat.com wrote:
>From: Nikolay Aleksandrov <Nikolay Aleksandrov nikolay@redhat.com>
>
>In case of bond_add_vlan() failure currently we'll have the vlan's
>refcnt bumped up in all slaves, but it will never go down because it
>failed to get added to the bond, so properly unwind the added vlan if
>bond_add_vlan fails.
>
>Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>

Acked-by: Veaceslav Falico <vfalico@redhat.com>

>
>---
>v2: no changes
>
> drivers/net/bonding/bond_main.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 78b0aeb..4264a76 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -455,13 +455,13 @@ static int bond_vlan_rx_add_vid(struct net_device *bond_dev,
> 	if (res) {
> 		pr_err("%s: Error: Failed to add vlan id %d\n",
> 		       bond_dev->name, vid);
>-		return res;
>+		goto unwind;
> 	}
>
> 	return 0;
>
> unwind:
>-	/* unwind from head to the slave that failed */
>+	/* unwind from the slave that failed */
> 	bond_for_each_slave_continue_reverse(bond, slave)
> 		vlan_vid_del(slave->dev, proto, vid);
>

Re: [PATCH net-next v2 0/2] bonding: vlan handling changes

[David Miller]

From: Nikolay Aleksandrov <nikolay@redhat.com>
Date: Tue,  6 Aug 2013 12:40:14 +0200

> From: Nikolay Aleksandrov <Nikolay Aleksandrov nikolay@redhat.com>
> 
> Hi,
> In v2 of these patchset I've dropped the previous patch 01 (vlan 0 fix)
> because it'll be taken care of later and these two patches are not
> dependent on it.
> 
> Patch 01 - switches the bonding to the now standard vlan syncing functions
> vlan_vids_add/del_by_dev() and removes the bonding specific ones. In v2
> upon vlan addition failure we fail the enslavement with an error message
> because otherwise we might delete vlans that weren't added by the bonding.
> The only way this may happen is with ENOMEM currently, so we're in trouble
> anyway.
> 
> Patch 02 - reverts vlan addition in case of bond_add_vlan failure because
> otherwise we may get bad vlan refcounts in the slaves.
> This patch is queued for net-next because a fix for -net without the list
> conversion would mean taking care of special cases and would be really
> ugly. Since this bug is not critical, I'd rather have the fix for net-next.
> It can happen only with ENOMEM, so again we're already in trouble.
> 
> Patches 01 and 02 are not connected and if you decide to drop patch 02
> please leave 01 (if accepted).
> 
> v2: See the notes, I've explained the changes together and by patch.

Series applied, thanks.