* [PATCH] net: caif: Don't act on notification for non-caif devices @ 2012-01-24 7:30 Sasha Levin 2012-01-24 10:52 ` Sjur Brændeland 0 siblings, 1 reply; 27+ messages in thread From: Sasha Levin @ 2012-01-24 7:30 UTC (permalink / raw) To: sjur.brandeland, davem, davej; +Cc: netdev, linux-kernel, Sasha Levin Currently we assume every notification happens within a network namespace in which CAIF was already initialized. This is not true when we're copying the namespace and the notifier is being called before the initialization code runs. Since the list of CAIF devices is stored in the net generic struct in each net namespace, which is not initialized at that point, we see the following BUG(): [ 200.752016] kernel BUG at include/net/netns/generic.h:40! [ 200.752016] invalid opcode: 0000 [#1] PREEMPT SMP [ 200.752016] CPU 0 [ 200.752016] Pid: 18013, comm: trinity Not tainted 3.3.0-rc1-next-20120123-sasha-dirty #134 [ 200.752016] RIP: 0010:[<ffffffff825c3dd6>] [<ffffffff825c3dd6>] get_cfcnfg+0x126/0x180 [ 200.752016] RSP: 0018:ffff88000fbabb00 EFLAGS: 00010202 [ 200.752016] RAX: 0000000000000001 RBX: 0000000000000016 RCX: 0000000000000000 [ 200.752016] RDX: 0000000000000001 RSI: ffffffff8323c620 RDI: 0000000000000286 [ 200.752016] RBP: ffff88000fbabb20 R08: 0000000000000003 R09: 0000000000000001 [ 200.752016] R10: 0000000000000000 R11: 0000000000000001 R12: ffff88000502b480 [ 200.752016] R13: ffffffff836b9440 R14: 0000000000000000 R15: 0000000000000010 [ 200.752016] FS: 00007f6c3af86700(0000) GS:ffff880013a00000(0000) knlGS:0000000000000000 [ 200.752016] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b [ 200.752016] CR2: 00007f7a60186f60 CR3: 000000000fb3b000 CR4: 00000000000406f0 [ 200.752016] DR0: ffffffff810ab5e0 DR1: 0000000000000000 DR2: 0000000000000000 [ 200.752016] DR3: 0000000000000000 DR6: 00000000ffff4ff0 DR7: 0000000000000600 [ 200.752016] Process trinity (pid: 18013, threadinfo ffff88000fbaa000, task ffff880005002000) [ 200.752016] Stack: [ 200.752016] ffffffff825c3cea ffffffff821cf0b0 ffff88000504d000 00000000ffffffd2 [ 200.752016] ffff88000fbabb80 ffffffff825c41be ffff88000fbabb80 0000000000000001 [ 200.752016] 0000000000000001 ffff880005002000 ffff88000fbabb80 ffff88000504d000 [ 200.752016] Call Trace: [ 200.752016] [<ffffffff825c3cea>] ? get_cfcnfg+0x3a/0x180 [ 200.752016] [<ffffffff821cf0b0>] ? lockdep_rtnl_is_held+0x10/0x20 [ 200.752016] [<ffffffff825c41be>] caif_device_notify+0x2e/0x530 [ 200.752016] [<ffffffff810d61b7>] notifier_call_chain+0x67/0x110 [ 200.752016] [<ffffffff810d67c1>] raw_notifier_call_chain+0x11/0x20 [ 200.752016] [<ffffffff821bae82>] call_netdevice_notifiers+0x32/0x60 [ 200.752016] [<ffffffff821c2b26>] register_netdevice+0x196/0x300 [ 200.752016] [<ffffffff821c2ca9>] register_netdev+0x19/0x30 [ 200.752016] [<ffffffff81c1c67a>] loopback_net_init+0x4a/0xa0 [ 200.752016] [<ffffffff821b5e62>] ops_init+0x42/0x180 [ 200.752016] [<ffffffff821b600b>] setup_net+0x6b/0x100 [ 200.752016] [<ffffffff821b6466>] copy_net_ns+0x86/0x110 [ 200.752016] [<ffffffff810d5789>] create_new_namespaces+0xd9/0x190 [ 200.752016] [<ffffffff810d5964>] copy_namespaces+0x84/0xc0 [ 200.752016] [<ffffffff810aab0f>] copy_process+0xa2f/0x14c0 [ 200.752016] [<ffffffff810d54de>] ? up_read+0x1e/0x40 [ 200.752016] [<ffffffff810ab653>] do_fork+0x73/0x340 [ 200.752016] [<ffffffff8265f5fc>] ? __mutex_unlock_slowpath+0x10c/0x200 [ 200.752016] [<ffffffff8110c7bd>] ? trace_hardirqs_on+0xd/0x10 [ 200.752016] [<ffffffff82662add>] ? retint_swapgs+0x13/0x1b [ 200.752016] [<ffffffff810554b3>] sys_clone+0x23/0x30 [ 200.752016] [<ffffffff82663743>] stub_clone+0x13/0x20 [ 200.752016] [<ffffffff826633b9>] ? system_call_fastpath+0x16/0x1b [ 200.752016] Code: dc 82 c6 05 71 b1 32 02 01 e8 47 dc b4 fe e9 6c ff ff ff 66 90 48 c7 c7 20 c6 23 83 e8 14 9e b4 fe 85 c0 0f 85 56 ff ff ff eb c4 <0f> 0b 80 3d 45 b1 32 02 01 90 0f 84 04 ff ff ff be f2 00 00 00 [ 200.752016] RIP [<ffffffff825c3dd6>] get_cfcnfg+0x126/0x180 [ 200.752016] RSP <ffff88000fbabb00> Instead, we'll first check if the device in the notification is a CAIF device: - If it is - the net generic struct in that namespace must have been already initialized. - If not - just ignore it as we don't care about other devices. Signed-off-by: Sasha Levin <levinsasha928@gmail.com> --- net/caif/caif_dev.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c index 673728a..75b9803 100644 --- a/net/caif/caif_dev.c +++ b/net/caif/caif_dev.c @@ -372,13 +372,16 @@ static int caif_device_notify(struct notifier_block *me, unsigned long what, int head_room = 0; struct caif_device_entry_list *caifdevs; + if (dev->type != ARPHRD_CAIF) + return 0; + cfg = get_cfcnfg(dev_net(dev)); caifdevs = caif_device_list(dev_net(dev)); if (!cfg || !caifdevs) return 0; caifd = caif_get(dev); - if (caifd == NULL && dev->type != ARPHRD_CAIF) + if (caifd == NULL) return 0; switch (what) { -- 1.7.8.3 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] net: caif: Don't act on notification for non-caif devices 2012-01-24 7:30 [PATCH] net: caif: Don't act on notification for non-caif devices Sasha Levin @ 2012-01-24 10:52 ` Sjur Brændeland 2012-01-24 14:49 ` Sasha Levin 0 siblings, 1 reply; 27+ messages in thread From: Sjur Brændeland @ 2012-01-24 10:52 UTC (permalink / raw) To: Sasha Levin; +Cc: davem, davej, netdev, linux-kernel Hi Sasha, > Since the list of CAIF devices is stored in the net generic struct in each > net namespace, which is not initialized at that point, we see the following > BUG(): > > [ 200.752016] kernel BUG at include/net/netns/generic.h:40! ... > [ 200.752016] Call Trace: > [ 200.752016] [<ffffffff825c3cea>] ? get_cfcnfg+0x3a/0x180 > [ 200.752016] [<ffffffff821cf0b0>] ? lockdep_rtnl_is_held+0x10/0x20 > [ 200.752016] [<ffffffff825c41be>] caif_device_notify+0x2e/0x530 Argh, my bad. This issue has been identified and fixed by David Woodhouse earlier, but was reintroduced again by me when adding support for CAIF over NCM. The CAIF code is handling if net_generic() returns NULL, but I missed that net_generic() does BUG_ON(). > Instead, we'll first check if the device in the notification is a CAIF device: > - If it is - the net generic struct in that namespace must have been already > initialized. > - If not - just ignore it as we don't care about other devices. > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com> Nack, we have to handle other device types than just ARPHDR_CAIF after introducing CAIF over USB/NCM. I'd rather fix this in netns by removing the BUG_ON and return NULL. How about this instead: diff --git a/include/net/netns/generic.h b/include/net/netns/generic.h index 3419bf5..0fc2eea 100644 --- a/include/net/netns/generic.h +++ b/include/net/netns/generic.h @@ -37,8 +37,10 @@ static inline void *net_generic(const struct net *net, int id rcu_read_lock(); ng = rcu_dereference(net->gen); - BUG_ON(id == 0 || id > ng->len); - ptr = ng->ptr[id - 1]; + if (id == 0 || id > ng->len) + ptr = NULL; + else + ptr = ng->ptr[id - 1]; rcu_read_unlock(); return ptr; I'll post a patch for this soon. Regards, Sjur ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] net: caif: Don't act on notification for non-caif devices 2012-01-24 10:52 ` Sjur Brændeland @ 2012-01-24 14:49 ` Sasha Levin 2012-01-24 15:06 ` Sjur Brændeland 0 siblings, 1 reply; 27+ messages in thread From: Sasha Levin @ 2012-01-24 14:49 UTC (permalink / raw) To: Sjur Brændeland; +Cc: davem, davej, netdev, linux-kernel On Tue, 2012-01-24 at 11:52 +0100, Sjur Brændeland wrote: > > Nack, we have to handle other device types than just ARPHDR_CAIF after > introducing > CAIF over USB/NCM. I'd rather fix this in netns by removing the BUG_ON > and return > NULL. How about this instead: [snip] I think that doing it this way is wrong for two reasons: 1. The code in net/ assumes net_generic is a trivial dereference and doesn't check that it's not NULL. This means that if anything goes wrong there you'll have a more dangerous NULL deref instead of a BUG(). 2. You'll need to add other device to that if() statement anyway, as it currently looks like this: cfg = get_cfcnfg(dev_net(dev)); caifdevs = caif_device_list(dev_net(dev)); if (!cfg || !caifdevs) return 0; caifd = caif_get(dev); if (caifd == NULL && dev->type != ARPHRD_CAIF) return 0; What my patch did was simply move the type check to above the net_generic call, it didn't add any new checks - which according to what you said, you'll need to do anyway. -- Sasha. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] net: caif: Don't act on notification for non-caif devices 2012-01-24 14:49 ` Sasha Levin @ 2012-01-24 15:06 ` Sjur Brændeland 2012-01-24 15:23 ` Sasha Levin 0 siblings, 1 reply; 27+ messages in thread From: Sjur Brændeland @ 2012-01-24 15:06 UTC (permalink / raw) To: Sasha Levin; +Cc: davem, davej, netdev, linux-kernel Hi Sasha, >> Nack, we have to handle other device types than just ARPHDR_CAIF after >> introducing CAIF over USB/NCM. > What my patch did was simply move the type check to above the net_generic call, > it didn't add any new checks - which according to what you said, you'll need to do anyway. As I said I, don't think your patch would work. Try to see what happens if dev->type != ARPHDR_CAIF and caifd != NULL. Then the statement: if (caifd == NULL && dev->type != ARPHRD_CAIF) return 0; is very different from: if (dev->type != ARPHRD_CAIF) return 0; ... if (caifd == NULL) return 0; Anyway, another option could be to explicitly check if name space is initialized, similar to what net_generic() does,e.g. something like: diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c index 673728a..3197bc2 100644 --- a/net/caif/caif_dev.c +++ b/net/caif/caif_dev.c @@ -371,6 +371,13 @@ static int caif_device_notify(struct notifier_block *me, un struct cflayer *layer, *link_support; int head_room = 0; struct caif_device_entry_list *caifdevs; + int len; + + rcu_read_lock(); + len = rcu_dereference(dev_net(dev)->gen)->len; + rcu_read_unlock(); + if (caif_net_id > len) + return 0; cfg = get_cfcnfg(dev_net(dev)); caifdevs = caif_device_list(dev_net(dev)); -- Regards, Sjur ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] net: caif: Don't act on notification for non-caif devices 2012-01-24 15:06 ` Sjur Brændeland @ 2012-01-24 15:23 ` Sasha Levin 2012-01-24 22:27 ` [PATCH net] caif: Fix crash due to uninitialized net name-space Sjur Brændeland 0 siblings, 1 reply; 27+ messages in thread From: Sasha Levin @ 2012-01-24 15:23 UTC (permalink / raw) To: Sjur Brændeland; +Cc: davem, davej, netdev, linux-kernel On Tue, Jan 24, 2012 at 10:06 AM, Sjur Brændeland <sjurbren@gmail.com> wrote: > Hi Sasha, > >>> Nack, we have to handle other device types than just ARPHDR_CAIF after >>> introducing CAIF over USB/NCM. >> What my patch did was simply move the type check to above the net_generic call, >> it didn't add any new checks - which according to what you said, you'll need to do anyway. > > As I said I, don't think your patch would work. Try to see what happens if > dev->type != ARPHDR_CAIF and caifd != NULL. Then the statement: > > if (caifd == NULL && dev->type != ARPHRD_CAIF) > return 0; > is very different from: > > if (dev->type != ARPHRD_CAIF) > return 0; > ... > if (caifd == NULL) > return 0; Right. > Anyway, another option could be to explicitly check if name space is > initialized, > similar to what net_generic() does,e.g. something like: > > diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c > index 673728a..3197bc2 100644 > --- a/net/caif/caif_dev.c > +++ b/net/caif/caif_dev.c > @@ -371,6 +371,13 @@ static int caif_device_notify(struct notifier_block *me, un > struct cflayer *layer, *link_support; > int head_room = 0; > struct caif_device_entry_list *caifdevs; > + int len; > + > + rcu_read_lock(); > + len = rcu_dereference(dev_net(dev)->gen)->len; > + rcu_read_unlock(); > + if (caif_net_id > len) > + return 0; > > cfg = get_cfcnfg(dev_net(dev)); > caifdevs = caif_device_list(dev_net(dev)); We could, in that case we'd just need to handle the case where it was initialized by a device with higher id than CAIF (which we already do I think), and do it without touching net_generic structure directly. btw, Why do we store the devices per-namespace instead of globally? Is it such a big benefit in performance? ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH net] caif: Fix crash due to uninitialized net name-space. 2012-01-24 15:23 ` Sasha Levin @ 2012-01-24 22:27 ` Sjur Brændeland 2012-01-24 22:44 ` David Miller ` (2 more replies) 0 siblings, 3 replies; 27+ messages in thread From: Sjur Brændeland @ 2012-01-24 22:27 UTC (permalink / raw) To: levinsasha928; +Cc: linux-kernel, davem, davej, sjurbren, Sjur Brændeland net_generic() calls BUG_ON() if called with uninitialized network name-space. Add check if net is initialized before calling net_generic(). This fixes the following oops: [ 200.752016] kernel BUG at include/net/netns/generic.h:40! ... [ 200.752016] [<ffffffff825c3cea>] ? get_cfcnfg+0x3a/0x180 [ 200.752016] [<ffffffff821cf0b0>] ? lockdep_rtnl_is_held+0x10/0x20 [ 200.752016] [<ffffffff825c41be>] caif_device_notify+0x2e/0x530 [ 200.752016] [<ffffffff810d61b7>] notifier_call_chain+0x67/0x110 [ 200.752016] [<ffffffff810d67c1>] raw_notifier_call_chain+0x11/0x20 [ 200.752016] [<ffffffff821bae82>] call_netdevice_notifiers+0x32/0x60 [ 200.752016] [<ffffffff821c2b26>] register_netdevice+0x196/0x300 [ 200.752016] [<ffffffff821c2ca9>] register_netdev+0x19/0x30 [ 200.752016] [<ffffffff81c1c67a>] loopback_net_init+0x4a/0xa0 [ 200.752016] [<ffffffff821b5e62>] ops_init+0x42/0x180 [ 200.752016] [<ffffffff821b600b>] setup_net+0x6b/0x100 [ 200.752016] [<ffffffff821b6466>] copy_net_ns+0x86/0x110 [ 200.752016] [<ffffffff810d5789>] create_new_namespaces+0xd9/0x190 Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com> --- Hi Sasha, Do you have any chance to review and test this patch? I'd like to get the net namespace handling this right this time ... Thanks, Sjur net/caif/caif_dev.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-) diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c index 673728a..6110ade 100644 --- a/net/caif/caif_dev.c +++ b/net/caif/caif_dev.c @@ -371,6 +371,14 @@ static int caif_device_notify(struct notifier_block *me, unsigned long what, struct cflayer *layer, *link_support; int head_room = 0; struct caif_device_entry_list *caifdevs; + int len; + + rcu_read_lock(); + len = rcu_dereference(dev_net(dev)->gen)->len; + rcu_read_unlock(); + + if (caif_net_id == 0 || caif_net_id > len) + return 0; cfg = get_cfcnfg(dev_net(dev)); caifdevs = caif_device_list(dev_net(dev)); -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH net] caif: Fix crash due to uninitialized net name-space. 2012-01-24 22:27 ` [PATCH net] caif: Fix crash due to uninitialized net name-space Sjur Brændeland @ 2012-01-24 22:44 ` David Miller 2012-01-25 16:13 ` Sasha Levin 2012-01-25 20:33 ` Sjur Brændeland 2 siblings, 0 replies; 27+ messages in thread From: David Miller @ 2012-01-24 22:44 UTC (permalink / raw) To: sjur.brandeland; +Cc: levinsasha928, linux-kernel, davej, sjurbren Please post all networking patches CC:'d to netdev@vger.kernel.org ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net] caif: Fix crash due to uninitialized net name-space. 2012-01-24 22:27 ` [PATCH net] caif: Fix crash due to uninitialized net name-space Sjur Brændeland 2012-01-24 22:44 ` David Miller @ 2012-01-25 16:13 ` Sasha Levin 2012-01-25 20:33 ` Sjur Brændeland 2 siblings, 0 replies; 27+ messages in thread From: Sasha Levin @ 2012-01-25 16:13 UTC (permalink / raw) To: Sjur Brændeland; +Cc: linux-kernel, davem, davej, sjurbren, netdev On Tue, Jan 24, 2012 at 5:27 PM, Sjur Brændeland <sjur.brandeland@stericsson.com> wrote: > net_generic() calls BUG_ON() if called with uninitialized > network name-space. Add check if net is initialized before > calling net_generic(). This fixes the following oops: > > [ 200.752016] kernel BUG at include/net/netns/generic.h:40! > ... > [ 200.752016] [<ffffffff825c3cea>] ? get_cfcnfg+0x3a/0x180 > [ 200.752016] [<ffffffff821cf0b0>] ? lockdep_rtnl_is_held+0x10/0x20 > [ 200.752016] [<ffffffff825c41be>] caif_device_notify+0x2e/0x530 > [ 200.752016] [<ffffffff810d61b7>] notifier_call_chain+0x67/0x110 > [ 200.752016] [<ffffffff810d67c1>] raw_notifier_call_chain+0x11/0x20 > [ 200.752016] [<ffffffff821bae82>] call_netdevice_notifiers+0x32/0x60 > [ 200.752016] [<ffffffff821c2b26>] register_netdevice+0x196/0x300 > [ 200.752016] [<ffffffff821c2ca9>] register_netdev+0x19/0x30 > [ 200.752016] [<ffffffff81c1c67a>] loopback_net_init+0x4a/0xa0 > [ 200.752016] [<ffffffff821b5e62>] ops_init+0x42/0x180 > [ 200.752016] [<ffffffff821b600b>] setup_net+0x6b/0x100 > [ 200.752016] [<ffffffff821b6466>] copy_net_ns+0x86/0x110 > [ 200.752016] [<ffffffff810d5789>] create_new_namespaces+0xd9/0x190 > > Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com> > --- > > Hi Sasha, > > Do you have any chance to review and test this patch? > I'd like to get the net namespace handling this right this time ... > > Thanks, > Sjur Works for me. Tested-by: Sasha Levin <levinsasha928@gmail.com> ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH net] caif: Fix crash due to uninitialized net name-space. 2012-01-24 22:27 ` [PATCH net] caif: Fix crash due to uninitialized net name-space Sjur Brændeland 2012-01-24 22:44 ` David Miller 2012-01-25 16:13 ` Sasha Levin @ 2012-01-25 20:33 ` Sjur Brændeland 2012-01-26 6:14 ` Eric Dumazet 2012-01-26 10:41 ` [PATCH] netns: fix net_alloc_generic() Eric Dumazet 2 siblings, 2 replies; 27+ messages in thread From: Sjur Brændeland @ 2012-01-25 20:33 UTC (permalink / raw) To: levinsasha928, netdev, davem Cc: linux-kernel, davej, sjurbren, Sjur Brændeland net_generic() calls BUG_ON() if called with uninitialized network name-space. Add check if net is initialized before calling net_generic(). This fixes the following oops: [ 200.752016] kernel BUG at include/net/netns/generic.h:40! ... [ 200.752016] [<ffffffff825c3cea>] ? get_cfcnfg+0x3a/0x180 [ 200.752016] [<ffffffff821cf0b0>] ? lockdep_rtnl_is_held+0x10/0x20 [ 200.752016] [<ffffffff825c41be>] caif_device_notify+0x2e/0x530 [ 200.752016] [<ffffffff810d61b7>] notifier_call_chain+0x67/0x110 [ 200.752016] [<ffffffff810d67c1>] raw_notifier_call_chain+0x11/0x20 [ 200.752016] [<ffffffff821bae82>] call_netdevice_notifiers+0x32/0x60 [ 200.752016] [<ffffffff821c2b26>] register_netdevice+0x196/0x300 [ 200.752016] [<ffffffff821c2ca9>] register_netdev+0x19/0x30 [ 200.752016] [<ffffffff81c1c67a>] loopback_net_init+0x4a/0xa0 [ 200.752016] [<ffffffff821b5e62>] ops_init+0x42/0x180 [ 200.752016] [<ffffffff821b600b>] setup_net+0x6b/0x100 [ 200.752016] [<ffffffff821b6466>] copy_net_ns+0x86/0x110 [ 200.752016] [<ffffffff810d5789>] create_new_namespaces+0xd9/0x190 Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com> Tested-by: Sasha Levin <levinsasha928@gmail.com> --- Hi Sasha and Dave, [Sasha] >Works for me. Thank you Sasha for reporting this bug and testing my patch, I appreciate it. [Dave] >Please post all networking patches CC:'d Sorry, I missed the obvious. I'm resending the same patch as yesterday, this time to:netdev and with "Tested-by: Sasha". Please apply to net. Thanks, Sjur net/caif/caif_dev.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-) diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c index 673728a..6110ade 100644 --- a/net/caif/caif_dev.c +++ b/net/caif/caif_dev.c @@ -371,6 +371,14 @@ static int caif_device_notify(struct notifier_block *me, unsigned long what, struct cflayer *layer, *link_support; int head_room = 0; struct caif_device_entry_list *caifdevs; + int len; + + rcu_read_lock(); + len = rcu_dereference(dev_net(dev)->gen)->len; + rcu_read_unlock(); + + if (caif_net_id == 0 || caif_net_id > len) + return 0; cfg = get_cfcnfg(dev_net(dev)); caifdevs = caif_device_list(dev_net(dev)); -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH net] caif: Fix crash due to uninitialized net name-space. 2012-01-25 20:33 ` Sjur Brændeland @ 2012-01-26 6:14 ` Eric Dumazet 2012-01-26 10:41 ` [PATCH] netns: fix net_alloc_generic() Eric Dumazet 1 sibling, 0 replies; 27+ messages in thread From: Eric Dumazet @ 2012-01-26 6:14 UTC (permalink / raw) To: Sjur Brændeland Cc: levinsasha928, netdev, davem, linux-kernel, davej, sjurbren Le mercredi 25 janvier 2012 à 21:33 +0100, Sjur Brændeland a écrit : > net_generic() calls BUG_ON() if called with uninitialized > network name-space. Add check if net is initialized before > calling net_generic(). This fixes the following oops: > > [ 200.752016] kernel BUG at include/net/netns/generic.h:40! > ... > [ 200.752016] [<ffffffff825c3cea>] ? get_cfcnfg+0x3a/0x180 > [ 200.752016] [<ffffffff821cf0b0>] ? lockdep_rtnl_is_held+0x10/0x20 > [ 200.752016] [<ffffffff825c41be>] caif_device_notify+0x2e/0x530 > [ 200.752016] [<ffffffff810d61b7>] notifier_call_chain+0x67/0x110 > [ 200.752016] [<ffffffff810d67c1>] raw_notifier_call_chain+0x11/0x20 > [ 200.752016] [<ffffffff821bae82>] call_netdevice_notifiers+0x32/0x60 > [ 200.752016] [<ffffffff821c2b26>] register_netdevice+0x196/0x300 > [ 200.752016] [<ffffffff821c2ca9>] register_netdev+0x19/0x30 > [ 200.752016] [<ffffffff81c1c67a>] loopback_net_init+0x4a/0xa0 > [ 200.752016] [<ffffffff821b5e62>] ops_init+0x42/0x180 > [ 200.752016] [<ffffffff821b600b>] setup_net+0x6b/0x100 > [ 200.752016] [<ffffffff821b6466>] copy_net_ns+0x86/0x110 > [ 200.752016] [<ffffffff810d5789>] create_new_namespaces+0xd9/0x190 > > Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com> > Tested-by: Sasha Levin <levinsasha928@gmail.com> > > --- > > Hi Sasha and Dave, > > [Sasha] > >Works for me. > Thank you Sasha for reporting this bug and testing my patch, > I appreciate it. > > [Dave] > >Please post all networking patches CC:'d > Sorry, I missed the obvious. > I'm resending the same patch as yesterday, this time to:netdev > and with "Tested-by: Sasha". Please apply to net. > > Thanks, > Sjur > > net/caif/caif_dev.c | 8 ++++++++ > 1 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c > index 673728a..6110ade 100644 > --- a/net/caif/caif_dev.c > +++ b/net/caif/caif_dev.c > @@ -371,6 +371,14 @@ static int caif_device_notify(struct notifier_block *me, unsigned long what, > struct cflayer *layer, *link_support; > int head_room = 0; > struct caif_device_entry_list *caifdevs; > + int len; > + > + rcu_read_lock(); > + len = rcu_dereference(dev_net(dev)->gen)->len; > + rcu_read_unlock(); > + > + if (caif_net_id == 0 || caif_net_id > len) > + return 0; > > cfg = get_cfcnfg(dev_net(dev)); > caifdevs = caif_device_list(dev_net(dev)); This looks wrong. This should not be needed, something is broken elsewhere. ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH] netns: fix net_alloc_generic() 2012-01-25 20:33 ` Sjur Brændeland 2012-01-26 6:14 ` Eric Dumazet @ 2012-01-26 10:41 ` Eric Dumazet 2012-01-26 10:44 ` Pavel Emelyanov ` (2 more replies) 1 sibling, 3 replies; 27+ messages in thread From: Eric Dumazet @ 2012-01-26 10:41 UTC (permalink / raw) To: Sjur Brændeland Cc: levinsasha928, netdev, davem, linux-kernel, davej, sjurbren, Eric W. Biederman, Pavel Emelyanov Le mercredi 25 janvier 2012 à 21:33 +0100, Sjur Brændeland a écrit : > net_generic() calls BUG_ON() if called with uninitialized > network name-space. Add check if net is initialized before > calling net_generic(). This fixes the following oops: > > [ 200.752016] kernel BUG at include/net/netns/generic.h:40! > ... > [ 200.752016] [<ffffffff825c3cea>] ? get_cfcnfg+0x3a/0x180 > [ 200.752016] [<ffffffff821cf0b0>] ? lockdep_rtnl_is_held+0x10/0x20 > [ 200.752016] [<ffffffff825c41be>] caif_device_notify+0x2e/0x530 > [ 200.752016] [<ffffffff810d61b7>] notifier_call_chain+0x67/0x110 > [ 200.752016] [<ffffffff810d67c1>] raw_notifier_call_chain+0x11/0x20 > [ 200.752016] [<ffffffff821bae82>] call_netdevice_notifiers+0x32/0x60 > [ 200.752016] [<ffffffff821c2b26>] register_netdevice+0x196/0x300 > [ 200.752016] [<ffffffff821c2ca9>] register_netdev+0x19/0x30 > [ 200.752016] [<ffffffff81c1c67a>] loopback_net_init+0x4a/0xa0 > [ 200.752016] [<ffffffff821b5e62>] ops_init+0x42/0x180 > [ 200.752016] [<ffffffff821b600b>] setup_net+0x6b/0x100 > [ 200.752016] [<ffffffff821b6466>] copy_net_ns+0x86/0x110 > [ 200.752016] [<ffffffff810d5789>] create_new_namespaces+0xd9/0x190 > > Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com> > Tested-by: Sasha Levin <levinsasha928@gmail.com> > > --- > > Hi Sasha and Dave, > > [Sasha] > >Works for me. > Thank you Sasha for reporting this bug and testing my patch, > I appreciate it. > > [Dave] > >Please post all networking patches CC:'d > Sorry, I missed the obvious. > I'm resending the same patch as yesterday, this time to:netdev > and with "Tested-by: Sasha". Please apply to net. > > Thanks, > Sjur > > net/caif/caif_dev.c | 8 ++++++++ > 1 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c > index 673728a..6110ade 100644 > --- a/net/caif/caif_dev.c > +++ b/net/caif/caif_dev.c > @@ -371,6 +371,14 @@ static int caif_device_notify(struct notifier_block *me, unsigned long what, > struct cflayer *layer, *link_support; > int head_room = 0; > struct caif_device_entry_list *caifdevs; > + int len; > + > + rcu_read_lock(); > + len = rcu_dereference(dev_net(dev)->gen)->len; > + rcu_read_unlock(); > + > + if (caif_net_id == 0 || caif_net_id > len) > + return 0; > > cfg = get_cfcnfg(dev_net(dev)); > caifdevs = caif_device_list(dev_net(dev)); I believe the problem is in net_namespace infrastructure, not in CAIF. Could you test following patch instead ? [PATCH] netns: fix net_alloc_generic() When a new net namespace is created, we should attach to it a "struct net_generic" with enough slots (even empty), or we can hit the following BUG_ON() : [ 200.752016] kernel BUG at include/net/netns/generic.h:40! ... [ 200.752016] [<ffffffff825c3cea>] ? get_cfcnfg+0x3a/0x180 [ 200.752016] [<ffffffff821cf0b0>] ? lockdep_rtnl_is_held+0x10/0x20 [ 200.752016] [<ffffffff825c41be>] caif_device_notify+0x2e/0x530 [ 200.752016] [<ffffffff810d61b7>] notifier_call_chain+0x67/0x110 [ 200.752016] [<ffffffff810d67c1>] raw_notifier_call_chain+0x11/0x20 [ 200.752016] [<ffffffff821bae82>] call_netdevice_notifiers+0x32/0x60 [ 200.752016] [<ffffffff821c2b26>] register_netdevice+0x196/0x300 [ 200.752016] [<ffffffff821c2ca9>] register_netdev+0x19/0x30 [ 200.752016] [<ffffffff81c1c67a>] loopback_net_init+0x4a/0xa0 [ 200.752016] [<ffffffff821b5e62>] ops_init+0x42/0x180 [ 200.752016] [<ffffffff821b600b>] setup_net+0x6b/0x100 [ 200.752016] [<ffffffff821b6466>] copy_net_ns+0x86/0x110 [ 200.752016] [<ffffffff810d5789>] create_new_namespaces+0xd9/0x190 net_alloc_generic() should take into account the maximum index into the ptr array, as a subsystem might use net_generic() anytime. This also reduces number of reallocations in net_assign_generic() Reported-by: Sasha Levin <levinsasha928@gmail.com> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> Cc: Sjur Brændeland <sjur.brandeland@stericsson.com> Cc: Eric W. Biederman <ebiederm@xmission.com> Cc: Pavel Emelyanov <xemul@openvz.org> --- net/core/net_namespace.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c index aefcd7a..0e950fd 100644 --- a/net/core/net_namespace.c +++ b/net/core/net_namespace.c @@ -30,6 +30,20 @@ EXPORT_SYMBOL(init_net); #define INITIAL_NET_GEN_PTRS 13 /* +1 for len +2 for rcu_head */ +static unsigned int max_gen_ptrs = INITIAL_NET_GEN_PTRS; + +static struct net_generic *net_alloc_generic(void) +{ + struct net_generic *ng; + size_t generic_size = offsetof(struct net_generic, ptr[max_gen_ptrs]); + + ng = kzalloc(generic_size, GFP_KERNEL); + if (ng) + ng->len = max_gen_ptrs; + + return ng; +} + static int net_assign_generic(struct net *net, int id, void *data) { struct net_generic *ng, *old_ng; @@ -43,8 +57,7 @@ static int net_assign_generic(struct net *net, int id, void *data) if (old_ng->len >= id) goto assign; - ng = kzalloc(sizeof(struct net_generic) + - id * sizeof(void *), GFP_KERNEL); + ng = net_alloc_generic(); if (ng == NULL) return -ENOMEM; @@ -59,7 +72,6 @@ static int net_assign_generic(struct net *net, int id, void *data) * the old copy for kfree after a grace period. */ - ng->len = id; memcpy(&ng->ptr, &old_ng->ptr, old_ng->len * sizeof(void*)); rcu_assign_pointer(net->gen, ng); @@ -161,18 +173,6 @@ out_undo: goto out; } -static struct net_generic *net_alloc_generic(void) -{ - struct net_generic *ng; - size_t generic_size = sizeof(struct net_generic) + - INITIAL_NET_GEN_PTRS * sizeof(void *); - - ng = kzalloc(generic_size, GFP_KERNEL); - if (ng) - ng->len = INITIAL_NET_GEN_PTRS; - - return ng; -} #ifdef CONFIG_NET_NS static struct kmem_cache *net_cachep; @@ -483,6 +483,7 @@ again: } return error; } + max_gen_ptrs = max_t(unsigned int, max_gen_ptrs, *ops->id); } error = __register_pernet_operations(list, ops); if (error) { ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] netns: fix net_alloc_generic() 2012-01-26 10:41 ` [PATCH] netns: fix net_alloc_generic() Eric Dumazet @ 2012-01-26 10:44 ` Pavel Emelyanov 2012-01-26 10:51 ` Eric Dumazet 2012-01-26 14:40 ` Sasha Levin 2012-01-26 18:37 ` David Miller 2 siblings, 1 reply; 27+ messages in thread From: Pavel Emelyanov @ 2012-01-26 10:44 UTC (permalink / raw) To: Eric Dumazet Cc: Sjur Brændeland, levinsasha928, netdev, davem, linux-kernel, davej, sjurbren, Eric W. Biederman > I believe the problem is in net_namespace infrastructure, not in CAIF. > > Could you test following patch instead ? > > [PATCH] netns: fix net_alloc_generic() > > When a new net namespace is created, we should attach to it a "struct > net_generic" with enough slots (even empty), or we can hit the following > BUG_ON() : > > [ 200.752016] kernel BUG at include/net/netns/generic.h:40! > ... > [ 200.752016] [<ffffffff825c3cea>] ? get_cfcnfg+0x3a/0x180 > [ 200.752016] [<ffffffff821cf0b0>] ? lockdep_rtnl_is_held+0x10/0x20 > [ 200.752016] [<ffffffff825c41be>] caif_device_notify+0x2e/0x530 > [ 200.752016] [<ffffffff810d61b7>] notifier_call_chain+0x67/0x110 > [ 200.752016] [<ffffffff810d67c1>] raw_notifier_call_chain+0x11/0x20 > [ 200.752016] [<ffffffff821bae82>] call_netdevice_notifiers+0x32/0x60 > [ 200.752016] [<ffffffff821c2b26>] register_netdevice+0x196/0x300 > [ 200.752016] [<ffffffff821c2ca9>] register_netdev+0x19/0x30 > [ 200.752016] [<ffffffff81c1c67a>] loopback_net_init+0x4a/0xa0 > [ 200.752016] [<ffffffff821b5e62>] ops_init+0x42/0x180 > [ 200.752016] [<ffffffff821b600b>] setup_net+0x6b/0x100 > [ 200.752016] [<ffffffff821b6466>] copy_net_ns+0x86/0x110 > [ 200.752016] [<ffffffff810d5789>] create_new_namespaces+0xd9/0x190 > > net_alloc_generic() should take into account the maximum index into the > ptr array, as a subsystem might use net_generic() anytime. I'm not sure I understand it correctly, but subsystem can only use the net_generic() only (!) after the net_assign_generic() is performed. > This also reduces number of reallocations in net_assign_generic() > > Reported-by: Sasha Levin <levinsasha928@gmail.com> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > Cc: Sjur Brændeland <sjur.brandeland@stericsson.com> > Cc: Eric W. Biederman <ebiederm@xmission.com> > Cc: Pavel Emelyanov <xemul@openvz.org> > --- > net/core/net_namespace.c | 31 ++++++++++++++++--------------- > 1 file changed, 16 insertions(+), 15 deletions(-) > > diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c > index aefcd7a..0e950fd 100644 > --- a/net/core/net_namespace.c > +++ b/net/core/net_namespace.c > @@ -30,6 +30,20 @@ EXPORT_SYMBOL(init_net); > > #define INITIAL_NET_GEN_PTRS 13 /* +1 for len +2 for rcu_head */ > > +static unsigned int max_gen_ptrs = INITIAL_NET_GEN_PTRS; > + > +static struct net_generic *net_alloc_generic(void) > +{ > + struct net_generic *ng; > + size_t generic_size = offsetof(struct net_generic, ptr[max_gen_ptrs]); > + > + ng = kzalloc(generic_size, GFP_KERNEL); > + if (ng) > + ng->len = max_gen_ptrs; > + > + return ng; > +} > + > static int net_assign_generic(struct net *net, int id, void *data) > { > struct net_generic *ng, *old_ng; > @@ -43,8 +57,7 @@ static int net_assign_generic(struct net *net, int id, void *data) > if (old_ng->len >= id) > goto assign; > > - ng = kzalloc(sizeof(struct net_generic) + > - id * sizeof(void *), GFP_KERNEL); > + ng = net_alloc_generic(); > if (ng == NULL) > return -ENOMEM; > > @@ -59,7 +72,6 @@ static int net_assign_generic(struct net *net, int id, void *data) > * the old copy for kfree after a grace period. > */ > > - ng->len = id; > memcpy(&ng->ptr, &old_ng->ptr, old_ng->len * sizeof(void*)); > > rcu_assign_pointer(net->gen, ng); > @@ -161,18 +173,6 @@ out_undo: > goto out; > } > > -static struct net_generic *net_alloc_generic(void) > -{ > - struct net_generic *ng; > - size_t generic_size = sizeof(struct net_generic) + > - INITIAL_NET_GEN_PTRS * sizeof(void *); > - > - ng = kzalloc(generic_size, GFP_KERNEL); > - if (ng) > - ng->len = INITIAL_NET_GEN_PTRS; > - > - return ng; > -} > > #ifdef CONFIG_NET_NS > static struct kmem_cache *net_cachep; > @@ -483,6 +483,7 @@ again: > } > return error; > } > + max_gen_ptrs = max_t(unsigned int, max_gen_ptrs, *ops->id); > } > error = __register_pernet_operations(list, ops); > if (error) { > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] netns: fix net_alloc_generic() 2012-01-26 10:44 ` Pavel Emelyanov @ 2012-01-26 10:51 ` Eric Dumazet 2012-01-26 22:57 ` Eric W. Biederman 0 siblings, 1 reply; 27+ messages in thread From: Eric Dumazet @ 2012-01-26 10:51 UTC (permalink / raw) To: Pavel Emelyanov Cc: Sjur Brændeland, levinsasha928, netdev, davem, linux-kernel, davej, sjurbren, Eric W. Biederman Le jeudi 26 janvier 2012 à 14:44 +0400, Pavel Emelyanov a écrit : > > I believe the problem is in net_namespace infrastructure, not in CAIF. > > > > Could you test following patch instead ? > > > > [PATCH] netns: fix net_alloc_generic() > > > > When a new net namespace is created, we should attach to it a "struct > > net_generic" with enough slots (even empty), or we can hit the following > > BUG_ON() : > > > > [ 200.752016] kernel BUG at include/net/netns/generic.h:40! > > ... > > [ 200.752016] [<ffffffff825c3cea>] ? get_cfcnfg+0x3a/0x180 > > [ 200.752016] [<ffffffff821cf0b0>] ? lockdep_rtnl_is_held+0x10/0x20 > > [ 200.752016] [<ffffffff825c41be>] caif_device_notify+0x2e/0x530 > > [ 200.752016] [<ffffffff810d61b7>] notifier_call_chain+0x67/0x110 > > [ 200.752016] [<ffffffff810d67c1>] raw_notifier_call_chain+0x11/0x20 > > [ 200.752016] [<ffffffff821bae82>] call_netdevice_notifiers+0x32/0x60 > > [ 200.752016] [<ffffffff821c2b26>] register_netdevice+0x196/0x300 > > [ 200.752016] [<ffffffff821c2ca9>] register_netdev+0x19/0x30 > > [ 200.752016] [<ffffffff81c1c67a>] loopback_net_init+0x4a/0xa0 > > [ 200.752016] [<ffffffff821b5e62>] ops_init+0x42/0x180 > > [ 200.752016] [<ffffffff821b600b>] setup_net+0x6b/0x100 > > [ 200.752016] [<ffffffff821b6466>] copy_net_ns+0x86/0x110 > > [ 200.752016] [<ffffffff810d5789>] create_new_namespaces+0xd9/0x190 > > > > net_alloc_generic() should take into account the maximum index into the > > ptr array, as a subsystem might use net_generic() anytime. > > I'm not sure I understand it correctly, but subsystem can only use the > net_generic() only (!) after the net_assign_generic() is performed. Yes, but here, loopback_net_init() calls register_netdev() So every subsystems _notify are called, even if subsystem _init_net() was not yet called. Its a chicken and egg problem. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] netns: fix net_alloc_generic() 2012-01-26 10:51 ` Eric Dumazet @ 2012-01-26 22:57 ` Eric W. Biederman 2012-01-26 23:07 ` David Miller 2012-01-27 6:09 ` [PATCH] netns: fix net_alloc_generic() Eric Dumazet 0 siblings, 2 replies; 27+ messages in thread From: Eric W. Biederman @ 2012-01-26 22:57 UTC (permalink / raw) To: Eric Dumazet Cc: Pavel Emelyanov, Sjur Brændeland, levinsasha928, netdev, davem, linux-kernel, davej, sjurbren Eric Dumazet <eric.dumazet@gmail.com> writes: > Le jeudi 26 janvier 2012 à 14:44 +0400, Pavel Emelyanov a écrit : >> > I believe the problem is in net_namespace infrastructure, not in CAIF. >> > >> > Could you test following patch instead ? >> > >> > [PATCH] netns: fix net_alloc_generic() >> > >> > When a new net namespace is created, we should attach to it a "struct >> > net_generic" with enough slots (even empty), or we can hit the following >> > BUG_ON() : >> > >> > [ 200.752016] kernel BUG at include/net/netns/generic.h:40! >> > ... >> > [ 200.752016] [<ffffffff825c3cea>] ? get_cfcnfg+0x3a/0x180 >> > [ 200.752016] [<ffffffff821cf0b0>] ? lockdep_rtnl_is_held+0x10/0x20 >> > [ 200.752016] [<ffffffff825c41be>] caif_device_notify+0x2e/0x530 >> > [ 200.752016] [<ffffffff810d61b7>] notifier_call_chain+0x67/0x110 >> > [ 200.752016] [<ffffffff810d67c1>] raw_notifier_call_chain+0x11/0x20 >> > [ 200.752016] [<ffffffff821bae82>] call_netdevice_notifiers+0x32/0x60 >> > [ 200.752016] [<ffffffff821c2b26>] register_netdevice+0x196/0x300 >> > [ 200.752016] [<ffffffff821c2ca9>] register_netdev+0x19/0x30 >> > [ 200.752016] [<ffffffff81c1c67a>] loopback_net_init+0x4a/0xa0 >> > [ 200.752016] [<ffffffff821b5e62>] ops_init+0x42/0x180 >> > [ 200.752016] [<ffffffff821b600b>] setup_net+0x6b/0x100 >> > [ 200.752016] [<ffffffff821b6466>] copy_net_ns+0x86/0x110 >> > [ 200.752016] [<ffffffff810d5789>] create_new_namespaces+0xd9/0x190 >> > >> > net_alloc_generic() should take into account the maximum index into the >> > ptr array, as a subsystem might use net_generic() anytime. >> >> I'm not sure I understand it correctly, but subsystem can only use the >> net_generic() only (!) after the net_assign_generic() is performed. > > Yes, but here, loopback_net_init() calls register_netdev() > > So every subsystems _notify are called, even if subsystem _init_net() > was not yet called. > > Its a chicken and egg problem. It is not a chicken and egg problem. It is a bug in caif. caif is claiming to be a network device when it is acting as a subsytem. That means it is being initialized too late. Untested but this should trivially fix the problem, and a bunch of others of the same ilk. It is not safe to shutdown subsystems until all of the devices are gone, otherwise there will be problems with packets in flight. diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c index 673728a..cf5bdd3 100644 --- a/net/caif/caif_dev.c +++ b/net/caif/caif_dev.c @@ -569,7 +569,7 @@ static int __init caif_device_init(void) { int result; - result = register_pernet_device(&caif_net_ops); + result = register_pernet_subsys(&caif_net_ops); if (result) return result; @@ -582,7 +582,7 @@ static int __init caif_device_init(void) static void __exit caif_device_exit(void) { - unregister_pernet_device(&caif_net_ops); + unregister_pernet_subsys(&caif_net_ops); unregister_netdevice_notifier(&caif_device_notifier); dev_remove_pack(&caif_packet_type); } Eric ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] netns: fix net_alloc_generic() 2012-01-26 22:57 ` Eric W. Biederman @ 2012-01-26 23:07 ` David Miller 2012-01-26 23:57 ` Eric W. Biederman 2012-01-27 0:02 ` [PATCH 1/2] netns: Fail conspicously if someone uses net_generic at an inappropriate time Eric W. Biederman 2012-01-27 6:09 ` [PATCH] netns: fix net_alloc_generic() Eric Dumazet 1 sibling, 2 replies; 27+ messages in thread From: David Miller @ 2012-01-26 23:07 UTC (permalink / raw) To: ebiederm Cc: eric.dumazet, xemul, sjur.brandeland, levinsasha928, netdev, linux-kernel, davej, sjurbren From: ebiederm@xmission.com (Eric W. Biederman) Date: Thu, 26 Jan 2012 14:57:02 -0800 > It is not a chicken and egg problem. It is a bug in caif. > caif is claiming to be a network device when it is acting as a subsytem. > That means it is being initialized too late. > > Untested but this should trivially fix the problem, and a bunch > of others of the same ilk. > > It is not safe to shutdown subsystems until all of the devices > are gone, otherwise there will be problems with packets in flight. If you guys can agree that this is the better fix, and we can get testing showing that it works, I'll revert Eric D.'s patch and apply the final version of this one. Thanks. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] netns: fix net_alloc_generic() 2012-01-26 23:07 ` David Miller @ 2012-01-26 23:57 ` Eric W. Biederman 2012-01-27 0:02 ` [PATCH 1/2] netns: Fail conspicously if someone uses net_generic at an inappropriate time Eric W. Biederman 1 sibling, 0 replies; 27+ messages in thread From: Eric W. Biederman @ 2012-01-26 23:57 UTC (permalink / raw) To: David Miller Cc: eric.dumazet, xemul, sjur.brandeland, levinsasha928, netdev, linux-kernel, davej, sjurbren David Miller <davem@davemloft.net> writes: > From: ebiederm@xmission.com (Eric W. Biederman) > Date: Thu, 26 Jan 2012 14:57:02 -0800 > >> It is not a chicken and egg problem. It is a bug in caif. >> caif is claiming to be a network device when it is acting as a subsytem. >> That means it is being initialized too late. >> >> Untested but this should trivially fix the problem, and a bunch >> of others of the same ilk. >> >> It is not safe to shutdown subsystems until all of the devices >> are gone, otherwise there will be problems with packets in flight. > > If you guys can agree that this is the better fix, and we can > get testing showing that it works, I'll revert Eric D.'s patch > and apply the final version of this one. Eric D.'s change is correct, but it is just an optimization of net_assign_generic. Eric D.'s change is not a bug fix. Tested fix in a moment. Eric ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 1/2] netns: Fail conspicously if someone uses net_generic at an inappropriate time. 2012-01-26 23:07 ` David Miller 2012-01-26 23:57 ` Eric W. Biederman @ 2012-01-27 0:02 ` Eric W. Biederman 2012-01-27 0:04 ` [PATCH 2/2] net caif: Register properly as a pernet subsystem Eric W. Biederman 2012-01-28 2:07 ` [PATCH 1/2] netns: Fail conspicously if someone uses net_generic at an inappropriate time David Miller 1 sibling, 2 replies; 27+ messages in thread From: Eric W. Biederman @ 2012-01-27 0:02 UTC (permalink / raw) To: David Miller Cc: eric.dumazet, xemul, sjur.brandeland, levinsasha928, netdev, linux-kernel, davej, sjurbren By definition net_generic should never be called when it can return NULL. Fail conspicously with a BUG_ON to make it clear when people mess up that a NULL return should never happen. Recently there was a bug in the CAIF subsystem where it was registered with register_pernet_device instead of register_pernet_subsys. It was erroneously concluded that net_generic could validly return NULL and that net_assign_generic was buggy (when it was just inefficient). Hopefully this BUG_ON will prevent people to coming to similar erroneous conclusions in the futrue. Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- include/net/netns/generic.h | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/include/net/netns/generic.h b/include/net/netns/generic.h index 3419bf5..d55f434 100644 --- a/include/net/netns/generic.h +++ b/include/net/netns/generic.h @@ -41,6 +41,7 @@ static inline void *net_generic(const struct net *net, int id) ptr = ng->ptr[id - 1]; rcu_read_unlock(); + BUG_ON(!ptr); return ptr; } #endif -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 2/2] net caif: Register properly as a pernet subsystem. 2012-01-27 0:02 ` [PATCH 1/2] netns: Fail conspicously if someone uses net_generic at an inappropriate time Eric W. Biederman @ 2012-01-27 0:04 ` Eric W. Biederman 2012-01-27 13:24 ` Sasha Levin 2012-01-28 2:07 ` David Miller 2012-01-28 2:07 ` [PATCH 1/2] netns: Fail conspicously if someone uses net_generic at an inappropriate time David Miller 1 sibling, 2 replies; 27+ messages in thread From: Eric W. Biederman @ 2012-01-27 0:04 UTC (permalink / raw) To: David Miller Cc: eric.dumazet, xemul, sjur.brandeland, levinsasha928, netdev, linux-kernel, davej, sjurbren caif is a subsystem and as such it needs to register with register_pernet_subsys instead of register_pernet_device. Among other problems using register_pernet_device was resulting in net_generic being called before the caif_net structure was allocated. Which has been causing net_generic to fail with either BUG_ON's or by return NULL pointers. A more ugly problem that could be caused is packets in flight why the subsystem is shutting down. To remove confusion also remove the cruft cause by inappropriately trying to fix this bug. With the aid of the previous patch I have tested this patch and confirmed that using register_pernet_subsys makes the failure go away as it should. Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- net/caif/caif_dev.c | 22 ++-------------------- net/caif/cfcnfg.c | 1 - 2 files changed, 2 insertions(+), 21 deletions(-) diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c index 673728a..82c5706 100644 --- a/net/caif/caif_dev.c +++ b/net/caif/caif_dev.c @@ -59,8 +59,6 @@ struct cfcnfg *get_cfcnfg(struct net *net) { struct caif_net *caifn; caifn = net_generic(net, caif_net_id); - if (!caifn) - return NULL; return caifn->cfg; } EXPORT_SYMBOL(get_cfcnfg); @@ -69,8 +67,6 @@ static struct caif_device_entry_list *caif_device_list(struct net *net) { struct caif_net *caifn; caifn = net_generic(net, caif_net_id); - if (!caifn) - return NULL; return &caifn->caifdevs; } @@ -99,8 +95,6 @@ static struct caif_device_entry *caif_device_alloc(struct net_device *dev) struct caif_device_entry *caifd; caifdevs = caif_device_list(dev_net(dev)); - if (!caifdevs) - return NULL; caifd = kzalloc(sizeof(*caifd), GFP_KERNEL); if (!caifd) @@ -120,8 +114,6 @@ static struct caif_device_entry *caif_get(struct net_device *dev) struct caif_device_entry_list *caifdevs = caif_device_list(dev_net(dev)); struct caif_device_entry *caifd; - if (!caifdevs) - return NULL; list_for_each_entry_rcu(caifd, &caifdevs->list, list) { if (caifd->netdev == dev) @@ -321,8 +313,6 @@ void caif_enroll_dev(struct net_device *dev, struct caif_dev_common *caifdev, struct caif_device_entry_list *caifdevs; caifdevs = caif_device_list(dev_net(dev)); - if (!cfg || !caifdevs) - return; caifd = caif_device_alloc(dev); if (!caifd) return; @@ -374,8 +364,6 @@ static int caif_device_notify(struct notifier_block *me, unsigned long what, cfg = get_cfcnfg(dev_net(dev)); caifdevs = caif_device_list(dev_net(dev)); - if (!cfg || !caifdevs) - return 0; caifd = caif_get(dev); if (caifd == NULL && dev->type != ARPHRD_CAIF) @@ -507,9 +495,6 @@ static struct notifier_block caif_device_notifier = { static int caif_init_net(struct net *net) { struct caif_net *caifn = net_generic(net, caif_net_id); - if (WARN_ON(!caifn)) - return -EINVAL; - INIT_LIST_HEAD(&caifn->caifdevs.list); mutex_init(&caifn->caifdevs.lock); @@ -527,9 +512,6 @@ static void caif_exit_net(struct net *net) caif_device_list(net); struct cfcnfg *cfg = get_cfcnfg(net); - if (!cfg || !caifdevs) - return; - rtnl_lock(); mutex_lock(&caifdevs->lock); @@ -569,7 +551,7 @@ static int __init caif_device_init(void) { int result; - result = register_pernet_device(&caif_net_ops); + result = register_pernet_subsys(&caif_net_ops); if (result) return result; @@ -582,7 +564,7 @@ static int __init caif_device_init(void) static void __exit caif_device_exit(void) { - unregister_pernet_device(&caif_net_ops); + unregister_pernet_subsys(&caif_net_ops); unregister_netdevice_notifier(&caif_device_notifier); dev_remove_pack(&caif_packet_type); } diff --git a/net/caif/cfcnfg.c b/net/caif/cfcnfg.c index 598aafb..ba9cfd4 100644 --- a/net/caif/cfcnfg.c +++ b/net/caif/cfcnfg.c @@ -309,7 +309,6 @@ int caif_connect_client(struct net *net, struct caif_connect_request *conn_req, int err; struct cfctrl_link_param param; struct cfcnfg *cfg = get_cfcnfg(net); - caif_assert(cfg != NULL); rcu_read_lock(); err = caif_connect_req_to_link_param(cfg, conn_req, ¶m); -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] net caif: Register properly as a pernet subsystem. 2012-01-27 0:04 ` [PATCH 2/2] net caif: Register properly as a pernet subsystem Eric W. Biederman @ 2012-01-27 13:24 ` Sasha Levin 2012-01-27 14:48 ` Sjur BRENDELAND 2012-01-28 2:07 ` David Miller 1 sibling, 1 reply; 27+ messages in thread From: Sasha Levin @ 2012-01-27 13:24 UTC (permalink / raw) To: Eric W. Biederman Cc: David Miller, eric.dumazet, xemul, sjur.brandeland, netdev, linux-kernel, davej, sjurbren On Thu, 2012-01-26 at 16:04 -0800, Eric W. Biederman wrote: > caif is a subsystem and as such it needs to register with > register_pernet_subsys instead of register_pernet_device. > > Among other problems using register_pernet_device was resulting in > net_generic being called before the caif_net structure was allocated. > Which has been causing net_generic to fail with either BUG_ON's or by > return NULL pointers. > > A more ugly problem that could be caused is packets in flight why the > subsystem is shutting down. > > To remove confusion also remove the cruft cause by inappropriately > trying to fix this bug. > > With the aid of the previous patch I have tested this patch and > confirmed that using register_pernet_subsys makes the failure go away > as it should. > > Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> I've tested these two patches as well, and they also work for me. Tested-by: Sasha Levin <levinsasha928@gmail.com> -- Sasha. ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH 2/2] net caif: Register properly as a pernet subsystem. 2012-01-27 13:24 ` Sasha Levin @ 2012-01-27 14:48 ` Sjur BRENDELAND 0 siblings, 0 replies; 27+ messages in thread From: Sjur BRENDELAND @ 2012-01-27 14:48 UTC (permalink / raw) To: Sasha Levin, Eric W. Biederman Cc: David Miller, eric.dumazet, xemul, netdev, linux-kernel, davej, sjurbren > > caif is a subsystem and as such it needs to register with > > register_pernet_subsys instead of register_pernet_device. > > > > Among other problems using register_pernet_device was resulting in > > net_generic being called before the caif_net structure was allocated. > > Which has been causing net_generic to fail with either BUG_ON's or by > > return NULL pointers. > > > > A more ugly problem that could be caused is packets in flight why the > > subsystem is shutting down. > > > > To remove confusion also remove the cruft cause by inappropriately > > trying to fix this bug. > > > > With the aid of the previous patch I have tested this patch and > > confirmed that using register_pernet_subsys makes the failure go away > > as it should. > > > > Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> > > I've tested these two patches as well, and they also work for me. > > Tested-by: Sasha Levin <levinsasha928@gmail.com> This looks very good to me, thank you Eric. Acked-by: Sjur Brændeland <sjur.brandeland@stericsson.com> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] net caif: Register properly as a pernet subsystem. 2012-01-27 0:04 ` [PATCH 2/2] net caif: Register properly as a pernet subsystem Eric W. Biederman 2012-01-27 13:24 ` Sasha Levin @ 2012-01-28 2:07 ` David Miller 1 sibling, 0 replies; 27+ messages in thread From: David Miller @ 2012-01-28 2:07 UTC (permalink / raw) To: ebiederm Cc: eric.dumazet, xemul, sjur.brandeland, levinsasha928, netdev, linux-kernel, davej, sjurbren From: ebiederm@xmission.com (Eric W. Biederman) Date: Thu, 26 Jan 2012 16:04:53 -0800 > > caif is a subsystem and as such it needs to register with > register_pernet_subsys instead of register_pernet_device. > > Among other problems using register_pernet_device was resulting in > net_generic being called before the caif_net structure was allocated. > Which has been causing net_generic to fail with either BUG_ON's or by > return NULL pointers. > > A more ugly problem that could be caused is packets in flight why the > subsystem is shutting down. > > To remove confusion also remove the cruft cause by inappropriately > trying to fix this bug. > > With the aid of the previous patch I have tested this patch and > confirmed that using register_pernet_subsys makes the failure go away as > it should. > > Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> Applied. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] netns: Fail conspicously if someone uses net_generic at an inappropriate time. 2012-01-27 0:02 ` [PATCH 1/2] netns: Fail conspicously if someone uses net_generic at an inappropriate time Eric W. Biederman 2012-01-27 0:04 ` [PATCH 2/2] net caif: Register properly as a pernet subsystem Eric W. Biederman @ 2012-01-28 2:07 ` David Miller 1 sibling, 0 replies; 27+ messages in thread From: David Miller @ 2012-01-28 2:07 UTC (permalink / raw) To: ebiederm Cc: eric.dumazet, xemul, sjur.brandeland, levinsasha928, netdev, linux-kernel, davej, sjurbren From: ebiederm@xmission.com (Eric W. Biederman) Date: Thu, 26 Jan 2012 16:02:55 -0800 > By definition net_generic should never be called when it can return > NULL. Fail conspicously with a BUG_ON to make it clear when people mess > up that a NULL return should never happen. > > Recently there was a bug in the CAIF subsystem where it was registered > with register_pernet_device instead of register_pernet_subsys. It was > erroneously concluded that net_generic could validly return NULL and > that net_assign_generic was buggy (when it was just inefficient). > Hopefully this BUG_ON will prevent people to coming to similar erroneous > conclusions in the futrue. > > Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> Applied. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] netns: fix net_alloc_generic() 2012-01-26 22:57 ` Eric W. Biederman 2012-01-26 23:07 ` David Miller @ 2012-01-27 6:09 ` Eric Dumazet 2012-01-27 6:54 ` Eric W. Biederman 1 sibling, 1 reply; 27+ messages in thread From: Eric Dumazet @ 2012-01-27 6:09 UTC (permalink / raw) To: Eric W. Biederman Cc: Pavel Emelyanov, Sjur Brændeland, levinsasha928, netdev, davem, linux-kernel, davej, sjurbren Le jeudi 26 janvier 2012 à 14:57 -0800, Eric W. Biederman a écrit : > It is not a chicken and egg problem. It is a bug in caif. > caif is claiming to be a network device when it is acting as a subsytem. > That means it is being initialized too late. > Ah ok ! > Untested but this should trivially fix the problem, and a bunch > of others of the same ilk. > Hmm, please refrain from using "trivially" or "trivial", you're not fooling anyone. Truth is this netns layer is horribly complex, since this CAIF bug needed no more than four patch attempts and lastly your own work before finding the root cause. Thanks ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] netns: fix net_alloc_generic() 2012-01-27 6:09 ` [PATCH] netns: fix net_alloc_generic() Eric Dumazet @ 2012-01-27 6:54 ` Eric W. Biederman 2012-01-27 7:07 ` Eric Dumazet 0 siblings, 1 reply; 27+ messages in thread From: Eric W. Biederman @ 2012-01-27 6:54 UTC (permalink / raw) To: Eric Dumazet Cc: Pavel Emelyanov, Sjur Brændeland, levinsasha928, netdev, davem, linux-kernel, davej, sjurbren Eric Dumazet <eric.dumazet@gmail.com> writes: > Le jeudi 26 janvier 2012 à 14:57 -0800, Eric W. Biederman a écrit : > >> It is not a chicken and egg problem. It is a bug in caif. >> caif is claiming to be a network device when it is acting as a subsytem. >> That means it is being initialized too late. >> > > Ah ok ! > >> Untested but this should trivially fix the problem, and a bunch >> of others of the same ilk. >> > > Hmm, please refrain from using "trivially" or "trivial", you're not > fooling anyone. All I meant is that the change was trivial. > Truth is this netns layer is horribly complex, since this CAIF bug > needed no more than four patch attempts and lastly your own work before > finding the root cause. As for the complexity I don't know that it is noticeably worse than the initialization complexity of the network stack in general. I do think that it is non-obvious that serious initialization ordering problems can be caused by such a small difference. The non-locality and of cause and effect, combined with unfamiliarity of the code seems to be what hides problems like this. Once you know that initialization ordering problems tend to registering the wrong way. Aka as a device instead of a subsys the solution to problems like this tend to jump out at you. Now the common plumbing in net/core/net_namespace.c does count as complex. The fact we missed such an obvious optimization opportunity for so long seems to confirm that. I am open for ideas on how to simply things. Eric ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] netns: fix net_alloc_generic() 2012-01-27 6:54 ` Eric W. Biederman @ 2012-01-27 7:07 ` Eric Dumazet 0 siblings, 0 replies; 27+ messages in thread From: Eric Dumazet @ 2012-01-27 7:07 UTC (permalink / raw) To: Eric W. Biederman Cc: Pavel Emelyanov, Sjur Brændeland, levinsasha928, netdev, davem, linux-kernel, davej, sjurbren Le jeudi 26 janvier 2012 à 22:54 -0800, Eric W. Biederman a écrit : > Now the common plumbing in net/core/net_namespace.c does count as > complex. The fact we missed such an obvious optimization opportunity > for so long seems to confirm that. This was probably masked by the 13 pointers initial value. I find hard to believe some real setups need more than that ! ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] netns: fix net_alloc_generic() 2012-01-26 10:41 ` [PATCH] netns: fix net_alloc_generic() Eric Dumazet 2012-01-26 10:44 ` Pavel Emelyanov @ 2012-01-26 14:40 ` Sasha Levin 2012-01-26 18:37 ` David Miller 2 siblings, 0 replies; 27+ messages in thread From: Sasha Levin @ 2012-01-26 14:40 UTC (permalink / raw) To: Eric Dumazet Cc: Sjur Brændeland, netdev, davem, linux-kernel, davej, sjurbren, Eric W. Biederman, Pavel Emelyanov On Thu, 2012-01-26 at 11:41 +0100, Eric Dumazet wrote: > Could you test following patch instead ? Works for me. -- Sasha. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] netns: fix net_alloc_generic() 2012-01-26 10:41 ` [PATCH] netns: fix net_alloc_generic() Eric Dumazet 2012-01-26 10:44 ` Pavel Emelyanov 2012-01-26 14:40 ` Sasha Levin @ 2012-01-26 18:37 ` David Miller 2 siblings, 0 replies; 27+ messages in thread From: David Miller @ 2012-01-26 18:37 UTC (permalink / raw) To: eric.dumazet Cc: sjur.brandeland, levinsasha928, netdev, linux-kernel, davej, sjurbren, ebiederm, xemul From: Eric Dumazet <eric.dumazet@gmail.com> Date: Thu, 26 Jan 2012 11:41:38 +0100 > [PATCH] netns: fix net_alloc_generic() > > When a new net namespace is created, we should attach to it a "struct > net_generic" with enough slots (even empty), or we can hit the following > BUG_ON() : > ... > net_alloc_generic() should take into account the maximum index into the > ptr array, as a subsystem might use net_generic() anytime. > > This also reduces number of reallocations in net_assign_generic() > > Reported-by: Sasha Levin <levinsasha928@gmail.com> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> Applied and queued up for -stable, thanks. ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2012-01-28 2:07 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-01-24 7:30 [PATCH] net: caif: Don't act on notification for non-caif devices Sasha Levin 2012-01-24 10:52 ` Sjur Brændeland 2012-01-24 14:49 ` Sasha Levin 2012-01-24 15:06 ` Sjur Brændeland 2012-01-24 15:23 ` Sasha Levin 2012-01-24 22:27 ` [PATCH net] caif: Fix crash due to uninitialized net name-space Sjur Brændeland 2012-01-24 22:44 ` David Miller 2012-01-25 16:13 ` Sasha Levin 2012-01-25 20:33 ` Sjur Brændeland 2012-01-26 6:14 ` Eric Dumazet 2012-01-26 10:41 ` [PATCH] netns: fix net_alloc_generic() Eric Dumazet 2012-01-26 10:44 ` Pavel Emelyanov 2012-01-26 10:51 ` Eric Dumazet 2012-01-26 22:57 ` Eric W. Biederman 2012-01-26 23:07 ` David Miller 2012-01-26 23:57 ` Eric W. Biederman 2012-01-27 0:02 ` [PATCH 1/2] netns: Fail conspicously if someone uses net_generic at an inappropriate time Eric W. Biederman 2012-01-27 0:04 ` [PATCH 2/2] net caif: Register properly as a pernet subsystem Eric W. Biederman 2012-01-27 13:24 ` Sasha Levin 2012-01-27 14:48 ` Sjur BRENDELAND 2012-01-28 2:07 ` David Miller 2012-01-28 2:07 ` [PATCH 1/2] netns: Fail conspicously if someone uses net_generic at an inappropriate time David Miller 2012-01-27 6:09 ` [PATCH] netns: fix net_alloc_generic() Eric Dumazet 2012-01-27 6:54 ` Eric W. Biederman 2012-01-27 7:07 ` Eric Dumazet 2012-01-26 14:40 ` Sasha Levin 2012-01-26 18:37 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).