[PATCH 1/2] netns: fix NULL-dereference in dev_net()

[PATCH 1/2] netns: fix NULL-dereference in dev_net()

[Brian Haley]

[-- Attachment #1: Type: text/plain, Size: 133 bytes --]

Change dev_net() to handle a NULL argument - return &init_net instead.

-Brian

Signed-off-by: Brian Haley <brian.haley@hp.com>
---


[-- Attachment #2: dev_net1.patch --]
[-- Type: text/x-patch, Size: 405 bytes --]

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 488c56e..baf6517 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -781,10 +781,10 @@ static inline
 struct net *dev_net(const struct net_device *dev)
 {
 #ifdef CONFIG_NET_NS
-	return dev->nd_net;
-#else
-	return &init_net;
+	if (dev)
+		return dev->nd_net;
 #endif
+	return &init_net;
 }
 
 static inline

Re: [Devel] [PATCH 1/2] netns: fix NULL-dereference in dev_net()

[Alexey Dobriyan]

On Thu, Aug 14, 2008 at 03:27:25PM -0400, Brian Haley wrote:
> Change dev_net() to handle a NULL argument - return &init_net instead.

> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -781,10 +781,10 @@ static inline
>  struct net *dev_net(const struct net_device *dev)
>  {
>  #ifdef CONFIG_NET_NS
> -	return dev->nd_net;
> -#else
> -	return &init_net;
> +	if (dev)
> +		return dev->nd_net;
>  #endif
> +	return &init_net;
>  }

This is ugly and wrong.

The only assymetry between init_net and dynamically created netns is
that some data structures are created only when init_net is initialized
like some kmem caches and so on.

And some sysctls that some people may ban in netns.

Modulo that, init_net is no different from the rest.

Re: [Devel] [PATCH 1/2] netns: fix NULL-dereference in dev_net()

[Brian Haley]

Alexey Dobriyan wrote:
> On Thu, Aug 14, 2008 at 03:27:25PM -0400, Brian Haley wrote:
>> Change dev_net() to handle a NULL argument - return &init_net instead.
> 
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -781,10 +781,10 @@ static inline
>>  struct net *dev_net(const struct net_device *dev)
>>  {
>>  #ifdef CONFIG_NET_NS
>> -	return dev->nd_net;
>> -#else
>> -	return &init_net;
>> +	if (dev)
>> +		return dev->nd_net;
>>  #endif
>> +	return &init_net;
>>  }
> 
> This is ugly and wrong.
> 
> The only assymetry between init_net and dynamically created netns is
> that some data structures are created only when init_net is initialized
> like some kmem caches and so on.
> 
> And some sysctls that some people may ban in netns.
> 
> Modulo that, init_net is no different from the rest.

So would you rather have all the callers that aren't net-namespace aware 
pass &init_net?  That seemed like a worse solution to me.

-Brian

Re: [PATCH 1/2] netns: fix NULL-dereference in dev_net()

[Denis V. Lunev]

On Thu, 2008-08-14 at 15:27 -0400, Brian Haley wrote:
> Change dev_net() to handle a NULL argument - return &init_net instead.
> 
> -Brian
> 
> Signed-off-by: Brian Haley <brian.haley@hp.com>
> ---
> 
the code is designed keeping in mind that there are no checks for
	net != NULL
in any place. They are too expensive. We need to invent other way :(

Regards,
	Den

Re: [PATCH 1/2] netns: fix NULL-dereference in dev_net()

[Brian Haley]

Denis V. Lunev wrote:
> On Thu, 2008-08-14 at 15:27 -0400, Brian Haley wrote:
>> Change dev_net() to handle a NULL argument - return &init_net instead.
>>
>> -Brian
>>
>> Signed-off-by: Brian Haley <brian.haley@hp.com>
>> ---
>>
> the code is designed keeping in mind that there are no checks for
> 	net != NULL
> in any place. They are too expensive. We need to invent other way :(

Ok, I'll fix the other callers to pass *net, I was just trying to be 
overly safe in dev_net() to avoid a crash.

-Brian

Re: [PATCH 1/2] netns: fix NULL-dereference in dev_net()

[Daniel Lezcano]

Brian Haley wrote:
> Change dev_net() to handle a NULL argument - return &init_net instead.
> 
> -Brian
> 
> Signed-off-by: Brian Haley <brian.haley@hp.com>

Did you fall into the case where the argument was NULL ?

If it is the case, I think it is not the proper way to handle that. 
IMHO, this is the symptom the code which calls this function has a 
problem with the network namespace and it should be changed to be 
correct. eg, the code is assuming the network device is never null and 
so the network namespace can be retrieved from it, a correct fix may be 
to pass the network namespace as parameter of the function.

Returning &init_net if the network device is null will gracefully avoid 
the kernel oopsing but will lead to some inconsistent behaviour and 
confusion with what is happening with the namespace.

IMO, we should be radical and let the kernel oopsing, giving us only one 
action which is to fix asap the bug correctly.

Thanks.
   -- Daniel

Re: [PATCH 1/2] netns: fix NULL-dereference in dev_net()

[David Miller]

From: Daniel Lezcano <dlezcano@fr.ibm.com>
Date: Sat, 16 Aug 2008 23:30:09 +0200

> Brian Haley wrote:
> > Change dev_net() to handle a NULL argument - return &init_net instead.
> > 
> > -Brian
> > 
> > Signed-off-by: Brian Haley <brian.haley@hp.com>
> 
> Did you fall into the case where the argument was NULL ?

Brian retracted this patch and we checked in a different fix.