New: 2.6.30 kernel locks up with pppoe in back trace (regression)

http://bugzilla.kernel.org/show_bug.cgi?id=13760

           Summary: 2.6.30 kernel locks up with pppoe in back trace
                    (regression)
           Product: Networking
           Version: 2.5
    Kernel Version: 2.6.30
          Platform: All
        OS/Version: Linux
              Tree: Mainline
            Status: NEW
          Severity: normal
          Priority: P1
         Component: Other
        AssignedTo: [email protected]
        ReportedBy: [email protected]
        Regression: Yes

Having updated kernel to 2.6.30 (and 2.6.30.1) I've faced several lock-ups during boot sequence (and even during shutdown), when pppoe connection had been initiated (and interrupted on shutdown). The lock-ups aren't Alt_SysRq_B breakable.

I still not have backtraces saved but as soon as pppoe connection initiation runs before running X-server (and after its shutdown), I have seen parts of dmesg's output and to my surprise it was regarding pppoe's functions.

(switched to email.  Please respond via emailed reply-to-all, not via the bugzilla web interface).

Quoted 19 lines · On Sat, 11 Jul 2009 06:10:00 GMT [email protected] wrote:

http://bugzilla.kernel.org/show_bug.cgi?id=13760

           Summary: 2.6.30 kernel locks up with pppoe in back trace (regression)
           Product: Networking
           Version: 2.5
    Kernel Version: 2.6.30
          Platform: All
        OS/Version: Linux
              Tree: Mainline
            Status: NEW
          Severity: normal
          Priority: P1
         Component: Other
        AssignedTo: [email protected]
        ReportedBy: [email protected]
        Regression: Yes

Quoted 9 lines

Having updated kernel to 2.6.30 (and 2.6.30.1) I've faced several lock-ups during boot sequence (and even during shutdown), when pppoe connection had been initiated (and interrupted on shutdown). The lock-ups aren't Alt_SysRq_B breakable.

I still not have backtraces saved but as soon as pppoe connection initiation runs before running X-server (and after its shutdown), I have seen parts of dmesg's output and to my surprise it was regarding pppoe's functions.

This regression report is nearly two weeks old and I think there has been a bit of ppp-related fixing going on in that time.

Can you please tell us whether the regression is still present in 2.6.30.2?  If so, I expect we'll be wanting to see those traces, please.

2009/7/23 Andrew Morton <[email protected]>:

Quoted 39 lines

(switched to email. __Please respond via emailed reply-to-all, not via the bugzilla web interface).

Quoted 19 lines · On Sat, 11 Jul 2009 06:10:00 GMT [email protected] wrote:

http://bugzilla.kernel.org/show_bug.cgi?id=13760

__ __ __ __ __ __Summary: 2.6.30 kernel locks up with pppoe in back trace __ __ __ __ __ __ __ __ __ __ (regression)
__ __ __ __ __ __Product: Networking
__ __ __ __ __ __Version: 2.5
__ __ Kernel Version: 2.6.30
__ __ __ __ __ Platform: All
__ __ __ __ OS/Version: Linux
__ __ __ __ __ __ __ Tree: Mainline
__ __ __ __ __ __ Status: NEW
__ __ __ __ __ Severity: normal
__ __ __ __ __ Priority: P1
__ __ __ __ __Component: Other
__ __ __ __ AssignedTo: [email protected]
__ __ __ __ ReportedBy: [email protected] __ __ __ __ Regression: Yes

Quoted 9 lines

Having updated kernel to 2.6.30 (and 2.6.30.1) I've faced several lock-ups during boot sequence (and even during shutdown), when pppoe connection had been initiated (and interrupted on shutdown). The lock-ups aren't Alt_SysRq_B breakable.

I still not have backtraces saved but as soon as pppoe connection initiation runs before running X-server (and after its shutdown), I have seen parts of dmesg's output and to my surprise it was regarding pppoe's functions.

This regression report is nearly two weeks old and I think there has been a bit of ppp-related fixing going on in that time.

Quoted 4 lines

Can you please tell us whether the regression is still present in 2.6.30.2? __If so, I expect we'll be wanting to see those traces, please.

Just repeating the same thing I had written in the Bugzilla: I
haven't noticed any changes to pppoe during last commits to 2.6.30. Why do you think there's a chance that bug(s) would have been fixed?

Just repeating the same thing I had written in the Bugzilla: I
haven't noticed any changes to pppoe during last commits to 2.6.30. Why do you think there's a chance that bug(s) would have been fixed?

Below is a ppp fix from 2.6.31, but it seems unlikely to fix your problem.

From: Andrew Morton <[email protected]>
Date: Thu, 23 Jul 2009 00:01:00 -0700

Quoted 1 line

Below is a ppp fix from 2.6.31, but it seems unlikely to fix your problem.

Andrew, that change was reverted, it's a NOP in the current tree.

There were various TTY layer fixups from Alan that could effect PPP and related drivers.

I wouldn't have thought that pppoe would be affected by tty changes?

As opposed to pppotty :)

From: Andrew Morton <[email protected]>
Date: Thu, 23 Jul 2009 10:51:41 -0700

Quoted 3 lines

I wouldn't have thought that pppoe would be affected by tty changes?

As opposed to pppotty :)

Good point. So it's unlikely any of the changes we're discussing will influence this bug.

Here is a link showing around pppoe problem quite similar to the one fixed later in pty.c by Alan Cox, so IMHO the current 2.6.31-rc is worth trying.

Well it depends on whether he's using the kernel pppoe stack or doing pppoe in user-space.  If the latter then yes those fixes

may help.  Otherwise we really need to see the backtraces.

Could have been a problem in net core, perhaps.

Below is a ppp fix from 2.6.31, but it seems unlikely to fix your problem.

It would help if we could see that trace, please.  A digital photo would suit.

[...]

Quoted 4 lines

Could have been a problem in net core, perhaps.

Below is a ppp fix from 2.6.31, but it seems unlikely to fix your problem.

Quoted 2 lines

It would help if we could see that trace, please. A digital photo would suit.

	Here it is:

		http://bugzilla.kernel.org/attachment.cgi?id=22516

	(It's 2.6.30.3)

Quoted 5 lines · Igor M Podlesny

[...]

Quoted 4 lines

Could have been a problem in net core, perhaps.

Below is a ppp fix from 2.6.31, but it seems unlikely to fix your problem.

Quoted 8 lines

Quoted 2 lines

It would help if we could see that trace, please. A digital photo would suit.

	Here it is:

		http://bugzilla.kernel.org/attachment.cgi?id=22516

	(It's 2.6.30.3)

Looking at this, I believe net_assign_generic() is not safe.

Two cpus could try to expand/update the array at same time, one update could be lost.

register_pernet_gen_device() has a mutex to guard against concurrent calls, but net_assign_generic() has no locking at all.

I doubt this is the reason of the crash, still worth to mention it...

[PATCH] net: net_assign_generic() is not SMP safe

Two cpus could try to expand/update the array at same time, one update could be lost during the copy of old array.

Re-using net_mutex is an easy way to fix this, it was used right before to allocate the 'id'

Signed-off-by Eric Dumazet <[email protected]>

---

net/core/net_namespace.c +5 -2

index b7292a2..9c31ad1 100644
@@ -467,15 +467,17 @@ int net_assign_generic(struct net *net, int id, void *data)
 	BUG_ON(!mutex_is_locked(&net_mutex));
 	BUG_ON(id == 0);
 
+	mutex_lock(&net_mutex);
 	ng = old_ng = net->gen;
 	if (old_ng->len >= id)
 		goto assign;
 
 	ng = kzalloc(sizeof(struct net_generic) +
 			id * sizeof(void *), GFP_KERNEL);
-	if (ng == NULL)
+	if (ng == NULL) {
+		mutex_unlock(&net_mutex);
 		return -ENOMEM;
-
+	}
 	/*
 	 * Some synchronisation notes:
 	 *

@@ -494,6 +496,7 @@ int net_assign_generic(struct net *net, int id, void *data)
 	call_rcu(&old_ng->rcu, net_generic_release);
 assign:
 	ng->ptr[id - 1] = data;
+	mutex_unlock(&net_mutex);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(net_assign_generic);

Quoted 25 lines · Eric Dumazet

Quoted 5 lines · Igor M Podlesny

[...]

Quoted 4 lines

Could have been a problem in net core, perhaps.

Below is a ppp fix from 2.6.31, but it seems unlikely to fix your problem.

Quoted 8 lines

Quoted 2 lines

It would help if we could see that trace, please. A digital photo would suit.

	Here it is:

		http://bugzilla.kernel.org/attachment.cgi?id=22516

	(It's 2.6.30.3)

Looking at this, I believe net_assign_generic() is not safe.

Two cpus could try to expand/update the array at same time, one update could be lost.

register_pernet_gen_device() has a mutex to guard against concurrent calls, but net_assign_generic() has no locking at all.

I doubt this is the reason of the crash, still worth to mention it...

[PATCH] net: net_assign_generic() is not SMP safe

Quoted 2 lines

Two cpus could try to expand/update the array at same time, one update could be lost during the copy of old array.

How can this happen? The array is updated only during ->init routines of the pernet_operations, which are called from under the net_mutex.

Do I miss anything?

Quoted 15 lines

Re-using net_mutex is an easy way to fix this, it was used right before to allocate the 'id'

Signed-off-by Eric Dumazet <[email protected]>

---

net/core/net_namespace.c +1 -0

index b7292a2..9c31ad1 100644
@@ -467,15 +467,17 @@ int net_assign_generic(struct net *net, int id, void *data)
 	BUG_ON(!mutex_is_locked(&net_mutex));
 	BUG_ON(id == 0);
 
+	mutex_lock(&net_mutex);

Quoted 31 lines · Pavel Emelyanov

Quoted 21 lines · Eric Dumazet

Quoted 5 lines · Igor M Podlesny

[...]

Quoted 4 lines

Could have been a problem in net core, perhaps.

Below is a ppp fix from 2.6.31, but it seems unlikely to fix your problem.

Quoted 8 lines

Quoted 2 lines

It would help if we could see that trace, please. A digital photo would suit.

	Here it is:

		http://bugzilla.kernel.org/attachment.cgi?id=22516

	(It's 2.6.30.3)

Looking at this, I believe net_assign_generic() is not safe.

Two cpus could try to expand/update the array at same time, one update could be lost.

register_pernet_gen_device() has a mutex to guard against concurrent calls, but net_assign_generic() has no locking at all.

Quoted 6 lines

I doubt this is the reason of the crash, still worth to mention it...

[PATCH] net: net_assign_generic() is not SMP safe

Two cpus could try to expand/update the array at same time, one update could be lost during the copy of old array.

How can this happen? The array is updated only during ->init routines of the pernet_operations, which are called from under the net_mutex.

Quoted 2 lines

Do I miss anything?

Oops, I missed the obvious "BUG_ON(!mutex_is_locked(&net_mutex));"

Sorry for the noise and untested patch as well :)

Hmm...

Real bug may be fixed by followed patch ? (yet untested, sorry...)

[PATCH] net: net_assign_generic() fix

memcpy() should take into account size of pointers, not only number of pointers to copy.

Signed-off-by Eric Dumazet <[email protected]>

---

net/core/net_namespace.c +1 -1

index b7292a2..1972830 100644
@@ -488,7 +488,7 @@ int net_assign_generic(struct net *net, int id, void *data)
 	 */
 
 	ng->len = id;
-	memcpy(&ng->ptr, &old_ng->ptr, old_ng->len);
+	memcpy(&ng->ptr, &old_ng->ptr, old_ng->len * sizeof(void*));
 
 	rcu_assign_pointer(net->gen, ng);
 	call_rcu(&old_ng->rcu, net_generic_release);

Quoted 4 lines

Hmm...

Real bug may be fixed by followed patch ? (yet untested, sorry...)

Quoted 6 lines

[PATCH] net: net_assign_generic() fix

memcpy() should take into account size of pointers, not only number of pointers to copy.

Signed-off-by Eric Dumazet <[email protected]>

Acked-by Pavel Emelyanov <[email protected]>

Quoted 11 lines · Pavel Emelyanov

Quoted 10 lines

Hmm...

Real bug may be fixed by followed patch ? (yet untested, sorry...)

[PATCH] net: net_assign_generic() fix

memcpy() should take into account size of pointers, not only number of pointers to copy.

Signed-off-by Eric Dumazet <[email protected]>

Acked-by Pavel Emelyanov <[email protected]>

Thanks.

Still this doesnt explain the crash, because initial number of pointers is 13 (INITIAL_NET_GEN_PTRS)

We probably never realloc this array, unless a module forgets to unregister_pernet_gen_device() and we load/unload it many times ?

Seems drivers/net/pppol2tp.c is a suspect...

It uses register_pernet_gen_device() from pppol2tp_init() but doesnt call unregister_pernet_gen_device()

Quoted 4 lines · Eric Dumazet

Seems drivers/net/pppol2tp.c is a suspect...

It uses register_pernet_gen_device() from pppol2tp_init() but doesnt call unregister_pernet_gen_device()

OK patch seems really easy...

This bug was added in commit 4e9fb8016a351b5b9da7fea32bcfdbc9d836e421
net: pppol2tp - introduce net-namespace functionality

So this is a stable candidate I guess ?

Thank you

[Eric Dumazet - Tue, Jul 28, 2009 at 07:46:37PM +0200]
...
| So Igor still has a panic... lets try a third patch then :)

Hmm. It seems the problem is not in pppoe_init_net since it's called *before* dev_add_pack via register_pernet_gen_device (which is protected by a global net mutex). Or I miss something?

(sorry guys I have quite a limited internet connection this week)

2009/7/29 Cyrill Gorcunov <[email protected]>:

Quoted 3 lines

[Eric Dumazet - Tue, Jul 28, 2009 at 07:46:37PM +0200]
...
| So Igor still has a panic... lets try a third patch then :)

[...]

Quoted 5 lines

Hmm. It seems the problem is not in pppoe_init_net since it's called *before* dev_add_pack via register_pernet_gen_device (which is protected by a global net mutex). Or I miss something?

(sorry guys I have quite a limited internet connection this week)

At last the 3rd patch was also unable to fix the bug.

If a socket is hashed in last slot of pppoe hash table (PPPOE_HASH_SIZE-1) we report it many times (up to filling seq buffer)
(Only the last socket of last slot)

Signed-off-by Eric Dumazet <[email protected]>

Quoted 6 lines · Pavel Emelyanov

Quoted 6 lines

Hmm...

Real bug may be fixed by followed patch ? (yet untested, sorry...)

[PATCH] net: net_assign_generic() fix

Quoted 6 lines

Quoted 4 lines

memcpy() should take into account size of pointers, not only number of pointers to copy.

Signed-off-by Eric Dumazet <[email protected]>

Acked-by Pavel Emelyanov <[email protected]>

Thanks.

Failure to call unregister_pernet_gen_device() can exhaust memory if module is loaded/unloaded many times.

Signed-off-by Eric Dumazet <[email protected]>

net,pppoe: fixup module init/exit subsequent calls

pernet data should allocated first and freed last on module init/exit routines otherwise it's possible to have unserialized calls to packet handling routines.

Signed-off-by Cyrill Gorcunov <[email protected]>