Nested VLAN causes recursive locking error

Nested VLAN causes recursive locking error

[Chuck Ebbert]

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


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

>From boot log on serial console:
(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
 =======================

[PATCH] Re: Nested VLAN causes recursive locking error

[Jarek Poplawski]

On 19-12-2007 00:03, Chuck Ebbert wrote:
> From:
> https://bugzilla.redhat.com/show_bug.cgi?id=426164
> 
> 
> kernel version is 2.6.24-0.107.rc5.git3.fc9
> 
> From boot log on serial console:
> (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
...


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

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

Reported-by: Benny Amorsen
Tested-by: Benny Amorsen	(?) NEEDS TESTING!

Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

diff -Nurp linux-2.6.24-rc5-/net/8021q/vlan.c linux-2.6.24-rc5+/net/8021q/vlan.c
--- linux-2.6.24-rc5-/net/8021q/vlan.c	2007-12-17 13:29:19.000000000 +0100
+++ linux-2.6.24-rc5+/net/8021q/vlan.c	2007-12-20 14:21:02.000000000 +0100
@@ -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;
 }
 

Re: [PATCH] Re: Nested VLAN causes recursive locking error

[Patrick McHardy]

Jarek Poplawski wrote:
> diff -Nurp linux-2.6.24-rc5-/net/8021q/vlan.c linux-2.6.24-rc5+/net/8021q/vlan.c
> --- linux-2.6.24-rc5-/net/8021q/vlan.c	2007-12-17 13:29:19.000000000 +0100
> +++ linux-2.6.24-rc5+/net/8021q/vlan.c	2007-12-20 14:21:02.000000000 +0100
> @@ -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++;
> +

That will increment the subclass globally, but it should actually just
use real_dev->subclass + 1. Otherwise we'll permenently fail after
registering 8 nested devices and unregistering them again.

Re: [PATCH] Re: Nested VLAN causes recursive locking error

[Jarek Poplawski]

On Mon, Dec 31, 2007 at 04:04:17PM +0100, Patrick McHardy wrote:
> Jarek Poplawski wrote:
>> diff -Nurp linux-2.6.24-rc5-/net/8021q/vlan.c linux-2.6.24-rc5+/net/8021q/vlan.c
>> --- linux-2.6.24-rc5-/net/8021q/vlan.c	2007-12-17 13:29:19.000000000 +0100
>> +++ linux-2.6.24-rc5+/net/8021q/vlan.c	2007-12-20 14:21:02.000000000 +0100
>> @@ -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++;
>> +
>
> That will increment the subclass globally, but it should actually just
> use real_dev->subclass + 1. Otherwise we'll permenently fail after
> registering 8 nested devices and unregistering them again.
>

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.

Re: [PATCH] Re: Nested VLAN causes recursive locking error

[Jarek Poplawski]

On Mon, Dec 31, 2007 at 06:45:55PM +0100, Jarek Poplawski wrote:
> Patric, [...]

OOPS... Sorry Patrick!

Jarek P.

Re: [PATCH] Re: Nested VLAN causes recursive locking error

[Jarek Poplawski]

On Mon, Dec 31, 2007 at 06:45:55PM +0100, Jarek Poplawski wrote:
> On Mon, Dec 31, 2007 at 04:04:17PM +0100, Patrick McHardy wrote:
...
> > That will increment the subclass globally, but it should actually just
> > use real_dev->subclass + 1. [...]
...
> 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?

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

Re: [PATCH] Re: Nested VLAN causes recursive locking error

[Benny Amorsen]

Jarek Poplawski <jarkao2@gmail.com> writes:

> Subject: [PATCH] nested VLAN: fix lockdep's recursive locking warning
>
> Allow vlans nesting other vlans without lockdep's warnings (max. 8 levels).
>
> Reported-by: Benny Amorsen
> Tested-by: Benny Amorsen	(?) NEEDS TESTING!
>
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

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

[PATCH take2] Re: Nested VLAN causes recursive locking error

[Jarek Poplawski]

Benny Amorsen wrote, On 01/02/2008 05:40 PM:
...
> 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.

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

Reported-by: Benny Amorsen
Tested-by: Benny Amorsen	(?) STILL NEEDS TESTING!

Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

diff -Nurp 2.6.24-rc6-mm1-/net/8021q/vlan.c 2.6.24-rc6-mm1+/net/8021q/vlan.c
--- 2.6.24-rc6-mm1-/net/8021q/vlan.c	2007-12-23 14:55:38.000000000 +0100
+++ 2.6.24-rc6-mm1+/net/8021q/vlan.c	2008-01-02 23:50:19.000000000 +0100
@@ -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;
 }
 

Re: [PATCH take2] Re: Nested VLAN causes recursive locking error

[Patrick McHardy]

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

Yes, apparently some people are using this.

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

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.

Re: [PATCH take2] Re: Nested VLAN causes recursive locking error

[Jarek Poplawski]

On Thu, Jan 10, 2008 at 04:31:22PM +0100, Patrick McHardy wrote:
...
> 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.

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.

Re: [PATCH take2] Re: Nested VLAN causes recursive locking error

[Jarek Poplawski]

On Thu, Jan 10, 2008 at 10:08:16PM +0100, Jarek Poplawski wrote:
...
> But still some 'quirks' are possible there: removing and adding
> devices 'properly' would often require resetting of many subclasses,

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

Jarek P.