linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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: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

* 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, &param);
-- 
1.7.2.5


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

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