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

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

[Nikolay Aleksandrov]

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

Hi,
I've queued all of these patches to net-next since they're tightly related
and the bugs that get fixed are not critical, so it's better to soak the
changes a bit. Also I'd much rather fix this with the slave list change in.
If you decide that they need to be posted for -net, let me know.

Patch 01 - fixes vlan 0 addition/removal since when we have 8021q module
loaded it gets added to all network devices, this causes the bonding to
output false warnings, output tagged learning packets (though with 0 tag)
The fix is by denying the addition/removal of vlan 0 through the ndo_vlan
functions and do it ourselves in the bond's netdev event handler when a
vlan 0 is added/removed from a bonding device. Also a small comment fix
which adds the missing proto parameter.

Patch 02 - switches the bonding to the now standard vlan syncing functions
vlan_vids_add/del_by_dev() and removes the bonding specific ones.

Patch 03 - reverts vlan addition in case of bond_add_vlan failure because
otherwise we may get bad vlan refcounts in the slaves.

Best regards,
 Nikolay Aleksandrov

Nikolay Aleksandrov (3):
  bonding: fix vlan 0 addition and removal
  bonding: change the bond's vlan syncing functions with the standard
    ones
  bonding: unwind on bond_add_vlan add failure

 drivers/net/bonding/bond_main.c | 86 +++++++++++++++++++++++++----------------
 1 file changed, 52 insertions(+), 34 deletions(-)

-- 
1.8.1.4

[PATCH net-next 1/3] bonding: fix vlan 0 addition and removal

[Nikolay Aleksandrov]

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

Right now if we have 8021q module loaded vlan 0 is added to all network
devices but since the bonding is a software device (and doesn't have hw
filters) it doesn't really need it, and moreover it causes the bond to
output warnings when a slave is being released if the slave's mac is in
use by the bonding like:
Aug  5 14:02:53 localhost kernel: [163030.313576] bonding: bond0:
Warning: clearing HW address of bond0 while it still has VLANs.
Aug  5 14:02:53 localhost kernel: [163030.313584] bonding: bond0: When
re-adding slaves, make sure the bond's HW address matches its VLANs'.

Although there aren't any real vlans on the bonding, it also causes the
ALB/TLB modes to transmit their learning packets tagged with vlan 0, and
bond_vlan_used() always evaluates to true when 8021q is loaded without
adding any vlans on the bonding.

This is fixed by forbidding the addition/removal of vlan 0 through the
bond's ndo_vlan_rx_add/kill_vid functions, and adding/removing it only when
vlan 0 is in fact being created (or destroyed) on top of a bond interface
in the bond's netdev handling function.

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

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 476df7d..5df8bcd 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -434,12 +434,13 @@ int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb,
 */
 
 /**
- * bond_vlan_rx_add_vid - Propagates adding an id to slaves
+ * __bond_vlan_rx_add_vid - Propagates adding an id to slaves
  * @bond_dev: bonding net device that got called
+ * @proto: vlan protocol
  * @vid: vlan id being added
  */
-static int bond_vlan_rx_add_vid(struct net_device *bond_dev,
-				__be16 proto, u16 vid)
+static int __bond_vlan_rx_add_vid(struct net_device *bond_dev,
+				  __be16 proto, u16 vid)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct slave *slave;
@@ -468,13 +469,27 @@ unwind:
 	return res;
 }
 
+/* Wrapper for ndo_vlan_rx_add_vid */
+static int bond_vlan_rx_add_vid(struct net_device *bond_dev,
+				__be16 proto, u16 vid)
+{
+	/* don't add vlan 0 through ndo_rx_vlan_add_vid
+	 * it's taken care of in the netdev event code upon NETDEV_REGISTER
+	 */
+	if (!vid)
+		return 0;
+
+	return __bond_vlan_rx_add_vid(bond_dev, proto, vid);
+}
+
 /**
- * bond_vlan_rx_kill_vid - Propagates deleting an id to slaves
+ * __bond_vlan_rx_kill_vid - Propagates deleting an id to slaves
  * @bond_dev: bonding net device that got called
+ * @proto: vlan protocol
  * @vid: vlan id being removed
  */
-static int bond_vlan_rx_kill_vid(struct net_device *bond_dev,
-				 __be16 proto, u16 vid)
+static int __bond_vlan_rx_kill_vid(struct net_device *bond_dev,
+				   __be16 proto, u16 vid)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct slave *slave;
@@ -493,6 +508,19 @@ static int bond_vlan_rx_kill_vid(struct net_device *bond_dev,
 	return 0;
 }
 
+/* Wrapper for ndo_vlan_rx_kill_vid */
+static int bond_vlan_rx_kill_vid(struct net_device *bond_dev,
+				 __be16 proto, u16 vid)
+{
+	/* don't remove vlan 0 through ndo_rx_vlan_kill_vid
+	 * it's taken care of in the netdev event code upon NETDEV_UNREGISTER
+	 */
+	if (!vid)
+		return 0;
+
+	return __bond_vlan_rx_kill_vid(bond_dev, proto, vid);
+}
+
 static void bond_add_vlans_on_slave(struct bonding *bond, struct net_device *slave_dev)
 {
 	struct vlan_entry *vlan;
@@ -3175,11 +3203,26 @@ static int bond_netdev_event(struct notifier_block *this,
 			     unsigned long event, void *ptr)
 {
 	struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
+	struct net_device *real_dev;
 
 	pr_debug("event_dev: %s, event: %lx\n",
 		 event_dev ? event_dev->name : "None",
 		 event);
 
+	/* Take care of addition/removal of vlan 0 on top of a bond interface */
+	if (is_vlan_dev(event_dev)) {
+		real_dev = vlan_dev_real_dev(event_dev);
+		if (netif_is_bond_master(real_dev) &&
+		    vlan_dev_vlan_id(event_dev) == 0) {
+			__be16 proto = htons(ETH_P_8021Q);
+
+			if (event == NETDEV_REGISTER)
+				__bond_vlan_rx_add_vid(real_dev, proto, 0);
+			else if (event == NETDEV_UNREGISTER)
+				__bond_vlan_rx_kill_vid(real_dev, proto, 0);
+		}
+	}
+
 	if (!(event_dev->priv_flags & IFF_BONDING))
 		return NOTIFY_DONE;
 
-- 
1.8.1.4

Re: [net-next,1/3] bonding: fix vlan 0 addition and removal

[Veaceslav Falico]

On Mon, Aug 05, 2013 at 03:28:22PM +0200, nikolay@redhat.com wrote:
...snip...
>This is fixed by forbidding the addition/removal of vlan 0 through the
>bond's ndo_vlan_rx_add/kill_vid functions, and adding/removing it only when
>vlan 0 is in fact being created (or destroyed) on top of a bond interface
>in the bond's netdev handling function.

Isn't that a bit too intrusive/hacky? I don't think we should treat vlan id
0 somehow differently in terms of adding/removing, though I might be
wrong...

Maybe we should just fix the bond_vlan_used() function? Something like
this (I've done only basic testing, can do more thorough if needed), though
it's also not a really clean fix:

>From 1c89abefebe90568ed52d2df59fcfdd650bc4696 Mon Sep 17 00:00:00 2001
From: Veaceslav Falico <vfalico@redhat.com>
Date: Mon, 5 Aug 2013 23:29:12 +0200
Subject: [PATCH] bonding: add vlan_uses_dev_rcu() and make bond_vlan_used() use it

Currently, bond_vlan_used() looks for any vlan, including the pseudo-vlan
id 0, and always returns true if 8021q is loaded. This creates several bad
situations - some warnings in __bond_release_one() because it thinks that
we still have vlans while removing, sending LB packets with vlan id 0 and,
possibly, other caused by vlan id 0.

Fix it by adding a new call, vlan_uses_dev_rcu(), which is the same as
vlan_uses_dev(), but uses rcu_dereference() instead of rtnl, and thus we
can use it in bond_vlan_used() wrapped in rcu_read_lock().

Also, use the pure vlan_uses_dev() in __bond_release_one() cause the rtnl
lock is held there.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: Patrick McHardy <kaber@trash.net>
CC: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
  drivers/net/bonding/bond_alb.c  |  1 -
  drivers/net/bonding/bond_main.c |  3 +--
  drivers/net/bonding/bonding.h   | 10 +++++++++-
  include/linux/if_vlan.h         |  6 ++++++
  net/8021q/vlan_core.c           | 11 +++++++++++
  5 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 3a5db7b..2684329 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -34,7 +34,6 @@
  #include <linux/if_arp.h>
  #include <linux/if_ether.h>
  #include <linux/if_bonding.h>
-#include <linux/if_vlan.h>
  #include <linux/in.h>
  #include <net/ipx.h>
  #include <net/arp.h>
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 5697043..b8c36ac 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -69,7 +69,6 @@
  #include <net/arp.h>
  #include <linux/mii.h>
  #include <linux/ethtool.h>
-#include <linux/if_vlan.h>
  #include <linux/if_bonding.h>
  #include <linux/jiffies.h>
  #include <linux/preempt.h>
@@ -1976,7 +1975,7 @@ static int __bond_release_one(struct net_device *bond_dev,
  		bond_set_carrier(bond);
  		eth_hw_addr_random(bond_dev);
  
-		if (bond_vlan_used(bond)) {
+		if (vlan_uses_dev(bond_dev)) {
  			pr_warning("%s: Warning: clearing HW address of %s while it still has VLANs.\n",
  				   bond_dev->name, bond_dev->name);
  			pr_warning("%s: When re-adding slaves, make sure the bond's HW address matches its VLANs'.\n",
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 4bf52d5..cb49313 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -23,6 +23,7 @@
  #include <linux/netpoll.h>
  #include <linux/inetdevice.h>
  #include <linux/etherdevice.h>
+#include <linux/if_vlan.h>
  #include "bond_3ad.h"
  #include "bond_alb.h"
  
@@ -267,9 +268,16 @@ struct bonding {
  #endif /* CONFIG_DEBUG_FS */
  };
  
+/* use vlan_uses_dev() if under rtnl */
  static inline bool bond_vlan_used(struct bonding *bond)
  {
-	return !list_empty(&bond->vlan_list);
+	bool ret;
+
+	rcu_read_lock();
+	ret = vlan_uses_dev_rcu(bond->dev);
+	rcu_read_unlock();
+
+	return ret;
  }
  
  #define bond_slave_get_rcu(dev) \
diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 715c343..1fcea36 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -101,6 +101,7 @@ extern void vlan_vids_del_by_dev(struct net_device *dev,
  				 const struct net_device *by_dev);
  
  extern bool vlan_uses_dev(const struct net_device *dev);
+extern bool vlan_uses_dev_rcu(const struct net_device *dev);
  #else
  static inline struct net_device *
  __vlan_find_dev_deep(struct net_device *real_dev,
@@ -155,6 +156,11 @@ static inline bool vlan_uses_dev(const struct net_device *dev)
  {
  	return false;
  }
+
+static inline bool vlan_uses_dev_rcu(const struct net_device *dev)
+{
+	return false;
+}
  #endif
  
  static inline bool vlan_hw_offload_capable(netdev_features_t features,
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 4a78c4d..52e3fb3 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -399,3 +399,14 @@ bool vlan_uses_dev(const struct net_device *dev)
  	return vlan_info->grp.nr_vlan_devs ? true : false;
  }
  EXPORT_SYMBOL(vlan_uses_dev);
+
+bool vlan_uses_dev_rcu(const struct net_device *dev)
+{
+	struct vlan_info *vlan_info;
+
+	vlan_info = rcu_dereference(dev->vlan_info);
+	if (!vlan_info)
+		return false;
+	return vlan_info->grp.nr_vlan_devs ? true : false;
+}
+EXPORT_SYMBOL(vlan_uses_dev_rcu);
-- 
1.8.1.4

Re: [net-next,1/3] bonding: fix vlan 0 addition and removal

[David Miller]

From: Veaceslav Falico <vfalico@redhat.com>
Date: Mon, 5 Aug 2013 23:51:26 +0200

> @@ -69,7 +69,6 @@
>  #include <net/arp.h>
>  #include <linux/mii.h>
>  #include <linux/ethtool.h>
> -#include <linux/if_vlan.h>
>  #include <linux/if_bonding.h>
>  #include <linux/jiffies.h>
>  #include <linux/preempt.h>
> @@ -1976,7 +1975,7 @@ static int __bond_release_one(struct net_device
> *bond_dev,
>  		bond_set_carrier(bond);
>  		eth_hw_addr_random(bond_dev);
>  -		if (bond_vlan_used(bond)) {
> +		if (vlan_uses_dev(bond_dev)) {

If you're adding a use of vlan_uses_dev(), you should retain the
if_vlan.h include, not remove it.

Re: [net-next,1/3] bonding: fix vlan 0 addition and removal

[Veaceslav Falico]

On Mon, Aug 05, 2013 at 04:08:22PM -0700, David Miller wrote:
>From: Veaceslav Falico <vfalico@redhat.com>
>Date: Mon, 5 Aug 2013 23:51:26 +0200
>
>> @@ -69,7 +69,6 @@
>>  #include <net/arp.h>
>>  #include <linux/mii.h>
>>  #include <linux/ethtool.h>
>> -#include <linux/if_vlan.h>
>>  #include <linux/if_bonding.h>
>>  #include <linux/jiffies.h>
>>  #include <linux/preempt.h>
>> @@ -1976,7 +1975,7 @@ static int __bond_release_one(struct net_device
>> *bond_dev,
>>  		bond_set_carrier(bond);
>>  		eth_hw_addr_random(bond_dev);
>>  -		if (bond_vlan_used(bond)) {
>> +		if (vlan_uses_dev(bond_dev)) {
>
>If you're adding a use of vlan_uses_dev(), you should retain the
>if_vlan.h include, not remove it.

I've added the usage of vlan_uses_dev_rcu() to bonding.h, so bonding.h
already includes linux/if_vlan.h, and thus any other bonding file doesn't
need it, cause it always includes bonding.h.

Sorry, my bad, should have added it to the commit message.

Re: [net-next,1/3] bonding: fix vlan 0 addition and removal

[Nikolay Aleksandrov]

On 08/05/2013 11:51 PM, Veaceslav Falico wrote:
> On Mon, Aug 05, 2013 at 03:28:22PM +0200, nikolay@redhat.com wrote:
> ...snip...
>> This is fixed by forbidding the addition/removal of vlan 0 through the
>> bond's ndo_vlan_rx_add/kill_vid functions, and adding/removing it only when
>> vlan 0 is in fact being created (or destroyed) on top of a bond interface
>> in the bond's netdev handling function.
> 
> Isn't that a bit too intrusive/hacky? I don't think we should treat vlan id
> 0 somehow differently in terms of adding/removing, though I might be
> wrong...
> 
I didn't want to touch the vlan code, so I solved the problem entirely in
the bonding, mind you there's still a bug when loading the 8021q module
we'll bump up every slave's vlan 0 refcnt without adding the vlan, and it
won't get bumped down.
In my patch that problem still persists but only when an actual vlan 0 is
being created.

> Maybe we should just fix the bond_vlan_used() function? Something like
> this (I've done only basic testing, can do more thorough if needed), though
> it's also not a really clean fix:
> 
Yes, that would be optimal and I agree.

> From 1c89abefebe90568ed52d2df59fcfdd650bc4696 Mon Sep 17 00:00:00 2001
> From: Veaceslav Falico <vfalico@redhat.com>
> Date: Mon, 5 Aug 2013 23:29:12 +0200
> Subject: [PATCH] bonding: add vlan_uses_dev_rcu() and make bond_vlan_used()
> use it
> 
> Currently, bond_vlan_used() looks for any vlan, including the pseudo-vlan
> id 0, and always returns true if 8021q is loaded. This creates several bad
> situations - some warnings in __bond_release_one() because it thinks that
> we still have vlans while removing, sending LB packets with vlan id 0 and,
> possibly, other caused by vlan id 0.
> 
> Fix it by adding a new call, vlan_uses_dev_rcu(), which is the same as
> vlan_uses_dev(), but uses rcu_dereference() instead of rtnl, and thus we
> can use it in bond_vlan_used() wrapped in rcu_read_lock().
> 
> Also, use the pure vlan_uses_dev() in __bond_release_one() cause the rtnl
> lock is held there.
> 
> CC: Jay Vosburgh <fubar@us.ibm.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> CC: Patrick McHardy <kaber@trash.net>
> CC: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
I'm okay with this fix, if Dave is also okay with this version then you can
submit it as a patch, I'll re-base my third one later and resubmit.

Re: [net-next,1/3] bonding: fix vlan 0 addition and removal

[Veaceslav Falico]

On Tue, Aug 06, 2013 at 10:16:04AM +0200, Nikolay Aleksandrov wrote:
>On 08/05/2013 11:51 PM, Veaceslav Falico wrote:
>> On Mon, Aug 05, 2013 at 03:28:22PM +0200, nikolay@redhat.com wrote:
>> ...snip...
>>> This is fixed by forbidding the addition/removal of vlan 0 through the
>>> bond's ndo_vlan_rx_add/kill_vid functions, and adding/removing it only when
>>> vlan 0 is in fact being created (or destroyed) on top of a bond interface
>>> in the bond's netdev handling function.
>>
>> Isn't that a bit too intrusive/hacky? I don't think we should treat vlan id
>> 0 somehow differently in terms of adding/removing, though I might be
>> wrong...
>>
>I didn't want to touch the vlan code, so I solved the problem entirely in
>the bonding, mind you there's still a bug when loading the 8021q module
>we'll bump up every slave's vlan 0 refcnt without adding the vlan, and it
>won't get bumped down.
>In my patch that problem still persists but only when an actual vlan 0 is
>being created.

I'll look into that, though maybe we'll require another patch to fix it.

>
>> Maybe we should just fix the bond_vlan_used() function? Something like
>> this (I've done only basic testing, can do more thorough if needed), though
>> it's also not a really clean fix:
>>
>Yes, that would be optimal and I agree.
>
>> From 1c89abefebe90568ed52d2df59fcfdd650bc4696 Mon Sep 17 00:00:00 2001
>> From: Veaceslav Falico <vfalico@redhat.com>
>> Date: Mon, 5 Aug 2013 23:29:12 +0200
>> Subject: [PATCH] bonding: add vlan_uses_dev_rcu() and make bond_vlan_used()
>> use it
>>
>> Currently, bond_vlan_used() looks for any vlan, including the pseudo-vlan
>> id 0, and always returns true if 8021q is loaded. This creates several bad
>> situations - some warnings in __bond_release_one() because it thinks that
>> we still have vlans while removing, sending LB packets with vlan id 0 and,
>> possibly, other caused by vlan id 0.
>>
>> Fix it by adding a new call, vlan_uses_dev_rcu(), which is the same as
>> vlan_uses_dev(), but uses rcu_dereference() instead of rtnl, and thus we
>> can use it in bond_vlan_used() wrapped in rcu_read_lock().
>>
>> Also, use the pure vlan_uses_dev() in __bond_release_one() cause the rtnl
>> lock is held there.
>>
>> CC: Jay Vosburgh <fubar@us.ibm.com>
>> CC: Andy Gospodarek <andy@greyhouse.net>
>> CC: Patrick McHardy <kaber@trash.net>
>> CC: "David S. Miller" <davem@davemloft.net>
>> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
>> ---
>I'm okay with this fix, if Dave is also okay with this version then you can
>submit it as a patch, I'll re-base my third one later and resubmit.

Ok, great, then I'll get some testing with this patch and submit it.

Re: [net-next,1/3] bonding: fix vlan 0 addition and removal

[Nikolay Aleksandrov]

On 08/06/2013 10:51 AM, Veaceslav Falico wrote:
> On Tue, Aug 06, 2013 at 10:16:04AM +0200, Nikolay Aleksandrov wrote:
>> On 08/05/2013 11:51 PM, Veaceslav Falico wrote:
>>> On Mon, Aug 05, 2013 at 03:28:22PM +0200, nikolay@redhat.com wrote:
>>> ...snip...
>>>> This is fixed by forbidding the addition/removal of vlan 0 through the
>>>> bond's ndo_vlan_rx_add/kill_vid functions, and adding/removing it only
>>>> when
>>>> vlan 0 is in fact being created (or destroyed) on top of a bond interface
>>>> in the bond's netdev handling function.
>>>
>>> Isn't that a bit too intrusive/hacky? I don't think we should treat vlan id
>>> 0 somehow differently in terms of adding/removing, though I might be
>>> wrong...
>>>
>> I didn't want to touch the vlan code, so I solved the problem entirely in
>> the bonding, mind you there's still a bug when loading the 8021q module
>> we'll bump up every slave's vlan 0 refcnt without adding the vlan, and it
>> won't get bumped down.
>> In my patch that problem still persists but only when an actual vlan 0 is
>> being created.
> 
> I'll look into that, though maybe we'll require another patch to fix it.
> 
You can take a look at:
http://www.spinics.net/lists/netdev/msg241858.html#.UgC5rxoW2Y4
and
http://www.spinics.net/lists/netdev/msg241999.html#.UgC5_BoW2Y4

I think I've explained the refcnt bug well there.


Thanks,
 Nik

Re: [net-next,1/3] bonding: fix vlan 0 addition and removal

[Nikolay Aleksandrov]

> From 1c89abefebe90568ed52d2df59fcfdd650bc4696 Mon Sep 17 00:00:00 2001
> From: Veaceslav Falico <vfalico@redhat.com>
> Date: Mon, 5 Aug 2013 23:29:12 +0200
> Subject: [PATCH] bonding: add vlan_uses_dev_rcu() and make bond_vlan_used()
> use it
> 
> Currently, bond_vlan_used() looks for any vlan, including the pseudo-vlan
> id 0, and always returns true if 8021q is loaded. This creates several bad
> situations - some warnings in __bond_release_one() because it thinks that
> we still have vlans while removing, sending LB packets with vlan id 0 and,
> possibly, other caused by vlan id 0.
> 
> Fix it by adding a new call, vlan_uses_dev_rcu(), which is the same as
> vlan_uses_dev(), but uses rcu_dereference() instead of rtnl, and thus we
> can use it in bond_vlan_used() wrapped in rcu_read_lock().
> 
> Also, use the pure vlan_uses_dev() in __bond_release_one() cause the rtnl
> lock is held there.
> 
Just 1 more note, you can't trust nr_vlan_devs under RCU.

Re: [net-next,1/3] bonding: fix vlan 0 addition and removal

[Veaceslav Falico]

On Tue, Aug 06, 2013 at 10:39:22AM +0200, Nikolay Aleksandrov wrote:
>> From 1c89abefebe90568ed52d2df59fcfdd650bc4696 Mon Sep 17 00:00:00 2001
>> From: Veaceslav Falico <vfalico@redhat.com>
>> Date: Mon, 5 Aug 2013 23:29:12 +0200
>> Subject: [PATCH] bonding: add vlan_uses_dev_rcu() and make bond_vlan_used()
>> use it
>>
>> Currently, bond_vlan_used() looks for any vlan, including the pseudo-vlan
>> id 0, and always returns true if 8021q is loaded. This creates several bad
>> situations - some warnings in __bond_release_one() because it thinks that
>> we still have vlans while removing, sending LB packets with vlan id 0 and,
>> possibly, other caused by vlan id 0.
>>
>> Fix it by adding a new call, vlan_uses_dev_rcu(), which is the same as
>> vlan_uses_dev(), but uses rcu_dereference() instead of rtnl, and thus we
>> can use it in bond_vlan_used() wrapped in rcu_read_lock().
>>
>> Also, use the pure vlan_uses_dev() in __bond_release_one() cause the rtnl
>> lock is held there.
>>
>Just 1 more note, you can't trust nr_vlan_devs under RCU.

Yes, you're right, however we actually don't care anyway if we race with
(un)register_vlan_dev() - we'll end up either in using the (un)registered
vlan or not, and in both cases it's ok. So I don't see a real problem here,
tbh, though I'll look into this also.

Re: [net-next,1/3] bonding: fix vlan 0 addition and removal

[Nikolay Aleksandrov]

On 08/06/2013 10:59 AM, Veaceslav Falico wrote:
> On Tue, Aug 06, 2013 at 10:39:22AM +0200, Nikolay Aleksandrov wrote:
>>> From 1c89abefebe90568ed52d2df59fcfdd650bc4696 Mon Sep 17 00:00:00 2001
>>> From: Veaceslav Falico <vfalico@redhat.com>
>>> Date: Mon, 5 Aug 2013 23:29:12 +0200
>>> Subject: [PATCH] bonding: add vlan_uses_dev_rcu() and make bond_vlan_used()
>>> use it
>>>
>>> Currently, bond_vlan_used() looks for any vlan, including the pseudo-vlan
>>> id 0, and always returns true if 8021q is loaded. This creates several bad
>>> situations - some warnings in __bond_release_one() because it thinks that
>>> we still have vlans while removing, sending LB packets with vlan id 0 and,
>>> possibly, other caused by vlan id 0.
>>>
>>> Fix it by adding a new call, vlan_uses_dev_rcu(), which is the same as
>>> vlan_uses_dev(), but uses rcu_dereference() instead of rtnl, and thus we
>>> can use it in bond_vlan_used() wrapped in rcu_read_lock().
>>>
>>> Also, use the pure vlan_uses_dev() in __bond_release_one() cause the rtnl
>>> lock is held there.
>>>
>> Just 1 more note, you can't trust nr_vlan_devs under RCU.
> 
> Yes, you're right, however we actually don't care anyway if we race with
> (un)register_vlan_dev() - we'll end up either in using the (un)registered
> vlan or not, and in both cases it's ok. So I don't see a real problem here,
> tbh, though I'll look into this also.
You might have stale value in the cache, the implications don't stop there.
I'd like to avoid inconsistent behaviour if there's a way.
A solution that can be relied on and works always would be much more
preferable.

Re: [net-next,1/3] bonding: fix vlan 0 addition and removal

[Veaceslav Falico]

On Tue, Aug 06, 2013 at 11:07:30AM +0200, Nikolay Aleksandrov wrote:
>On 08/06/2013 10:59 AM, Veaceslav Falico wrote:
>> On Tue, Aug 06, 2013 at 10:39:22AM +0200, Nikolay Aleksandrov wrote:
...snip...
>>> Just 1 more note, you can't trust nr_vlan_devs under RCU.
>>
>> Yes, you're right, however we actually don't care anyway if we race with
>> (un)register_vlan_dev() - we'll end up either in using the (un)registered
>> vlan or not, and in both cases it's ok. So I don't see a real problem here,
>> tbh, though I'll look into this also.
>You might have stale value in the cache, the implications don't stop there.
>I'd like to avoid inconsistent behaviour if there's a way.
>A solution that can be relied on and works always would be much more
>preferable.

Sure :). I currently don't see it, except of using quite offensive locking
strategies, but I'll try to figure something out.

[PATCH net-next 2/3] 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.

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

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 5df8bcd..ed1d261 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -521,33 +521,6 @@ static int bond_vlan_rx_kill_vid(struct net_device *bond_dev,
 	return __bond_vlan_rx_kill_vid(bond_dev, proto, vid);
 }
 
-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 -------------------------------*/
 
 /*
@@ -1656,7 +1629,9 @@ 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_warn("%s: couldn't add bond vlan ids to %s",
+			bond_dev->name, slave_dev->name);
 
 	write_lock_bh(&bond->lock);
 
@@ -1832,7 +1807,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)
@@ -2028,7 +2003,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

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

[Veaceslav Falico]

On Mon, Aug 05, 2013 at 03:28:23PM +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.
>
>Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
>
>---
>drivers/net/bonding/bond_main.c | 35 +++++------------------------------
> 1 file changed, 5 insertions(+), 30 deletions(-)

-25, awesome, great one :)

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

>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 5df8bcd..ed1d261 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -521,33 +521,6 @@ static int bond_vlan_rx_kill_vid(struct net_device *bond_dev,
> 	return __bond_vlan_rx_kill_vid(bond_dev, proto, vid);
> }
>
>-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 -------------------------------*/
>
> /*
>@@ -1656,7 +1629,9 @@ 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_warn("%s: couldn't add bond vlan ids to %s",
>+			bond_dev->name, slave_dev->name);
>
> 	write_lock_bh(&bond->lock);
>
>@@ -1832,7 +1807,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)
>@@ -2028,7 +2003,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)

[PATCH net-next 3/3] bonding: unwind on bond_add_vlan add 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>
---
 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 ed1d261..0f9ca7e 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -456,13 +456,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,3/3] bonding: unwind on bond_add_vlan add failure

[Veaceslav Falico]

On Mon, Aug 05, 2013 at 03:28:24PM +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>

In case patch 1/3 from series goes in... Otherwise I think it will be
needed to patch bond_vlan_rx_add_vid().

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

>
>---
>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 ed1d261..0f9ca7e 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -456,13 +456,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: [net-next,3/3] bonding: unwind on bond_add_vlan add failure

[David Miller]

From: Veaceslav Falico <vfalico@redhat.com>
Date: Mon, 5 Aug 2013 23:59:21 +0200

> On Mon, Aug 05, 2013 at 03:28:24PM +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>
> 
> In case patch 1/3 from series goes in... Otherwise I think it will be
> needed to patch bond_vlan_rx_add_vid().

I'm not happy with patch #1 and it needs more discussion between you
two.