Skip to contentSkip to conversation

netns: fix NULL-dereference in dev_net()

PATCH In review Brian Haley 5 participants 7 messages Aug 14, 2008 – Aug 17, 2008

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Brian Haley

author
#0 lore raw

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

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

-Brian

Signed-off-by Brian Haley <[email protected]>

---

include/linux/netdevice.h +3 -3
index 488c56e..baf6517 100644
@@ -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
#1 lore raw
Quoted 6 lines · Brian Haley

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

include/linux/netdevice.h +0 -0
@@ -781,10 +781,10 @@ static inline
 struct net *dev_net(const struct net_device *dev)

include/linux/netdevice.h static inline · line 788

Quoted 10 lines
include/linux/netdevice.h +3 -3
 {
 #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.

author
#2 lore raw
Quoted 20 lines · Alexey Dobriyan
Quoted 6 lines · Brian Haley

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

include/linux/netdevice.h +0 -0
@@ -781,10 +781,10 @@ static inline
 struct net *dev_net(const struct net_device *dev)

include/linux/netdevice.h static inline · line 788

Quoted 10 lines
include/linux/netdevice.h +3 -3
 {
 #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.

Quoted 7 lines

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

author
#4 lore raw
Quoted 11 lines · Denis V. Lunev
Quoted 7 lines · Brian Haley

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

-Brian

Signed-off-by Brian Haley <[email protected]>

---

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

#5 lore raw
In reply to Brian Haley
Quoted 5 lines · Brian Haley

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

-Brian

Signed-off-by Brian Haley <[email protected]>

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

maintainer
#6 lore raw

From: Daniel Lezcano <[email protected]>
Date: Sat, 16 Aug 2008 23:30:09 +0200

Quoted 8 lines
Quoted 5 lines · Brian Haley

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

-Brian

Signed-off-by Brian Haley <[email protected]>

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

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