Nested VLAN causes recursive locking error

From:
https://bugzilla.redhat.com/show_bug.cgi?id=426164

kernel version is 2.6.24-0.107.rc5.git3.fc9

(full log attached)

Added VLAN with VID == 2 to IF -:eth0.1568:-

=============================================
[ INFO: possible recursive locking detected ] 2.6.24-0.107.rc5.git3.fc9 #1
---------------------------------------------
ifconfig/15011 is trying to acquire lock:
(&vlan_netdev_xmit_lock_key){-+..}, at: [<c05d9450>] dev_mc_sync+0x1c/0x102

but task is already holding lock:
(&vlan_netdev_xmit_lock_key){-+..}, at: [<c05d51bd>] dev_set_rx_mode+0x14/0x3c

other info that might help us debug this:
2 locks held by ifconfig/15011:
#0: (rtnl_mutex){--..}, at: [<c05de4f7>] rtnl_lock+0xf/0x11
#1: (&vlan_netdev_xmit_lock_key){-+..}, at: [<c05d51bd>] dev_set_rx_mode+0x14/0x3c

stack backtrace:
Pid: 15011, comm: ifconfig Not tainted 2.6.24-0.107.rc5.git3.fc9 #1
 [<c040649a>] show_trace_log_lvl+0x1a/0x2f
 [<c0406d41>] show_trace+0x12/0x14
 [<c0407061>] dump_stack+0x6c/0x72
 [<c044cfb3>] __lock_acquire+0x815/0xb5f
 [<c044d6f5>] lock_acquire+0x7b/0x9e
 [<c063dcf5>] _spin_lock_bh+0x33/0x5d
 [<c05d9450>] dev_mc_sync+0x1c/0x102
 [<f8b24c96>] vlan_dev_set_multicast_list+0x15/0x17 [8021q]
 [<c05d5107>] __dev_set_rx_mode+0x7e/0x81
 [<c05d51d0>] dev_set_rx_mode+0x27/0x3c
 [<c05d749e>] dev_open+0x61/0x7b
 [<c05d60b0>] dev_change_flags+0xa4/0x152
 [<c0616b8d>] devinet_ioctl+0x211/0x518
 [<c061723b>] inet_ioctl+0x86/0xa4
 [<c05caf0b>] sock_ioctl+0x1ca/0x1eb
 [<c049cdf2>] do_ioctl+0x22/0x67
 [<c049d080>] vfs_ioctl+0x249/0x25c
 [<c049d0d5>] sys_ioctl+0x42/0x5d
 [<c0405252>] syscall_call+0x7/0xb
 =======================

...

Subject: [PATCH] nested VLAN: fix lockdep's recursive locking warning

Allow vlans nesting other vlans without lockdep's warnings (max. 8 levels).

---

diff -Nurp linux-2.6.24-rc5-/net/8021q/vlan.c linux-2.6.24-rc5+/net/8021q/vlan.c
@@ -307,12 +307,15 @@ int unregister_vlan_device(struct net_de
 	return ret;
 }
 
+#ifdef CONFIG_LOCKDEP
 /*
  * vlan network devices have devices nesting below it, and are a special
  * "super class" of normal network devices; split their locks off into a
  * separate class since they always nest.
  */
 static struct lock_class_key vlan_netdev_xmit_lock_key;
+static int subclass; /* vlan nesting vlan */
+#endif
 
 static const struct header_ops vlan_header_ops = {
 	.create	 = vlan_dev_hard_header,

@@ -349,7 +352,14 @@ static int vlan_dev_init(struct net_devi
 		dev->hard_start_xmit = vlan_dev_hard_start_xmit;
 	}
 
-	lockdep_set_class(&dev->_xmit_lock, &vlan_netdev_xmit_lock_key);
+#ifdef CONFIG_LOCKDEP
+	if ((real_dev->priv_flags & IFF_802_1Q_VLAN) &&
+	    subclass < MAX_LOCKDEP_SUBCLASSES - 1)
+		subclass++;
+
+	lockdep_set_class_and_subclass(&dev->_xmit_lock,
+				&vlan_netdev_xmit_lock_key, subclass);
+#endif
 	return 0;
 }
 

Very good point! (As usual...) Actually, this shouldn't fail but work with this one, last subclass only, so similarly like now, without this patch.

Patric, currently I'm not sure this patch is very necessary... Since I don't use vlans, I don't know how much it's real or theoretical only problem. Probably the easiest solution would be limiting this to two subclasses - any nested vlan gets second. Otherwise, it seems there is some place needed to store these subclasses or use some unofficial checks on lockdep's structures?

So, if you think this is useful and have better idea how this should be done, I would be very glad if you re-do this your way (plus there should be no problem with testing). Otherwise, give me some hint...

Thanks & Happy New Year!
Jarek P.

OOPS... Sorry Patrick!

Jarek P.

...

...

...It seems there is simply needed additional macro in lockdep's API to retrieve a subclass number from a lock! It would be at least 'strange' to have to save this doubly.

Jarek P.

The box that shows the problem is "almost-production", so it doesn't have a build system. Is there a chance you could get the patch into Rawhide or some other testing repository? It takes forever and a lot of disk space to rebuild a kernel RPM, so if I could get someone else to do the hard work, that would be lovely...

If not, I'll probably find the time to do the RPM rebuild sometime within the next week or so.

/Benny

Benny Amorsen wrote, On 01/02/2008 05:40 PM:
...

...I guess this is to Chuck?

As a matter of fact I started to doubt it's a real problem: 2 vlan headers in the row - is it working?

Anyway, as Patrick pointed, the previous patch was a bit buggy, and deeper nesting needs a little more (if it's can work too...). So, here is something minimal.

Patrick, if you think about something else, then of course don't care about this patch.

Regards,
Jarek P.

------------------> (take 2)

Subject: [PATCH] nested VLAN: fix lockdep's recursive locking warning

Allow vlans nesting other vlans without lockdep's warnings (max. 2 levels i.e. parent + child). Thanks to Patrick McHardy for pointing a bug in the first version of this patch.

---

diff -Nurp 2.6.24-rc6-mm1-/net/8021q/vlan.c 2.6.24-rc6-mm1+/net/8021q/vlan.c
@@ -323,6 +323,7 @@ static const struct header_ops vlan_head
 static int vlan_dev_init(struct net_device *dev)
 {
 	struct net_device *real_dev = VLAN_DEV_INFO(dev)->real_dev;
+	int subclass = 0;
 
 	/* IFF_BROADCAST|IFF_MULTICAST; ??? */
 	dev->flags  = real_dev->flags & ~IFF_UP;

@@ -349,7 +350,11 @@ static int vlan_dev_init(struct net_devi
 		dev->hard_start_xmit = vlan_dev_hard_start_xmit;
 	}
 
-	lockdep_set_class(&dev->_xmit_lock, &vlan_netdev_xmit_lock_key);
+	if (real_dev->priv_flags & IFF_802_1Q_VLAN)
+		subclass = 1;
+
+	lockdep_set_class_and_subclass(&dev->_xmit_lock,
+				&vlan_netdev_xmit_lock_key, subclass);
 	return 0;
 }
 

Yes, apparently some people are using this.

No, this seems fine, thanks. Even better would be a way to get the last lockdep subclass through lockdep somehow, but I couldn't find a clean way for this. So I've applied your patch and also fixed macvlan.

On Thu, Jan 10, 2008 at 04:31:22PM +0100, Patrick McHardy wrote:
...

As a matter of fact this simplified version was done mainly to remove this bad looking effect of a never decreased global. Of course, your proposal with using parent's subclass + 1 would be better, if deeper nestings are required: so, I could try to enhance this (probably with such additional lockdep macro) after some hint.

But still some 'quirks' are possible there: removing and adding devices 'properly' would often require resetting of many subclasses, so quite a lot of activities if more devices. And probably not very common if not requested until now...

Thanks,
Jarek P.

On Thu, Jan 10, 2008 at 10:08:16PM +0100, Jarek Poplawski wrote:
...

...Hmm, probably they are always removed from/with the children, then no problem! (I know, I could've checked...)

Jarek P.