diff mbox

pppoe: fix race at init time

Message ID 4A6F397D.6010606@gmail.com
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet July 28, 2009, 5:46 p.m. UTC 
Eric Dumazet a écrit :
> Eric Dumazet a écrit :
>> 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

So Igor still has a panic... lets try a third patch then :)

[PATCH] pppoe: fix race at init time

I believe we have a race in ppoe_init() :

As soon as dev_add_pack(&pppoes_ptype); and/or dev_add_pack(&pppoed_ptype); 
are called, we can receive packets while nets not yet fully ready
(ie : pppoe_init_net() not yet called)

This means we should be prepared to get a NULL pointer
from net_generic(net, pppoe_net_id) call.

We miss this NULL check in get_item() and possibly crash if this nets 
has no struct pppoe_net attached yet. Other subroutines
are safe.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Cyrill Gorcunov July 28, 2009, 6:48 p.m. UTC | #1 
[Eric Dumazet - Tue, Jul 28, 2009 at 07:46:37PM +0200]
... 
| So Igor still has a panic... lets try a third patch then :)
| 
| [PATCH] pppoe: fix race at init time
| 
| I believe we have a race in ppoe_init() :
| 
| As soon as dev_add_pack(&pppoes_ptype); and/or dev_add_pack(&pppoed_ptype); 
| are called, we can receive packets while nets not yet fully ready
| (ie : pppoe_init_net() not yet called)
| 
| This means we should be prepared to get a NULL pointer
| from net_generic(net, pppoe_net_id) call.
| 
| We miss this NULL check in get_item() and possibly crash if this nets 
| has no struct pppoe_net attached yet. Other subroutines
| are safe.

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)

	-- Cyrill
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Igor M Podlesny July 29, 2009, 3:55 a.m. UTC | #2 
2009/7/29 Cyrill Gorcunov <gorcunov@gmail.com>:
> [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)

	At last the 3rd patch was also unable to fix the bug.
Eric Dumazet July 29, 2009, 4:33 a.m. UTC | #3 
Igor M Podlesny a écrit :
> 2009/7/29 Cyrill Gorcunov <gorcunov@gmail.com>:
>> [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)
> 
> 	At last the 3rd patch was also unable to fix the bug.
> 

Yes, Cyril is right, my patch was not necessary.


Any chance you give us CR2 value of fault ?

Do you still have a corruption by a 2 value somewhere ?

(High 32 bits order in first jpeg you sent)
00000002.3dcc5244  instead of ffffXXXX.3dcc5244


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c
index f0031f1..e50af8c 100644
--- a/drivers/net/pppoe.c
+++ b/drivers/net/pppoe.c
@@ -237,14 +237,15 @@  static struct pppox_sock *__delete_item(struct pppoe_net *pn, __be16 sid,
 static inline struct pppox_sock *get_item(struct pppoe_net *pn, __be16 sid,
 					unsigned char *addr, int ifindex)
 {
-	struct pppox_sock *po;
-
-	read_lock_bh(&pn->hash_lock);
-	po = __get_item(pn, sid, addr, ifindex);
-	if (po)
-		sock_hold(sk_pppox(po));
-	read_unlock_bh(&pn->hash_lock);
-
+	struct pppox_sock *po = NULL;
+
+	if (pn) {
+		read_lock_bh(&pn->hash_lock);
+		po = __get_item(pn, sid, addr, ifindex);
+		if (po)
+			sock_hold(sk_pppox(po));
+		read_unlock_bh(&pn->hash_lock);
+	}
 	return po;
 }