[NETNS] Oops in register_pernet_operations() with CONFIG_NET_NS=n [message #22323] |
Thu, 25 October 2007 12:59  |
Benjamin Thery
Messages: 79 Registered: March 2007
|
Member |
|
|
Hello Pavel,
I've found a problem with one of your patch related to netns:
* [NETNS] Move some code into __init section when CONFIG_NET_NS=n (v2)
http://www.spinics.net/lists/netdev/msg43310.html
This patch introduces the __net_init/__net_exit/__net_initdata
defines to save some memory when CONFIG_NET_NS is not set.
Cedric Le Goater reported he had a *non-fatal* oops when booting
a 2.6.23-mm1-lxc1 kernel with CONFIG_NET_NS=n. (2.6.23-mm1-lxc1
contains the NETNS49 patchset). The oops occured when modules
related to iptables were loaded after the boot completes.
The problem is the following:
- Your patch adds the __net_initdata attribute to pernet_operations
structures.
- pernet_operations are registered via register_pernet_subsys() and
linked in the pernet_list during boot.
- At the end of boot, pernet_operations are freed (because of the
__net_initdata attribute), and the pernet_list (or first_device list)
points to freed memory.
- After boot, network modules which are netns-aware try to register
themselves with register_pernet_subsys() and ...KABOOM... page
fault when accessing pernet_list (or first_device list).
(I reproduce Cedric's oops with the command: iptables --list)
This is not a problem right now in 2.6.23-mm1 or net-2.6 because
there are very few netns-aware network subsystems merged and they
are all initialized during boot. But it will be problematic when
we will merge netns code for subsystems which can be built as
modules (eg. iptables, ...). I'm not sure we can use
__net_init_data for pernet_operations then.
Maybe we can add some checks in register_pernet_operations
when CONFIG_NET_NS=n.
I haven't found a fix yet.
Regards,
Benjamin
--
B e n j a m i n T h e r y - BULL/DT/Open Software R&D
http://www.bull.com
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers
|
|
|
Re: [NETNS] Oops in register_pernet_operations() with CONFIG_NET_NS=n [message #22325 is a reply to message #22323] |
Thu, 25 October 2007 14:00   |
den
Messages: 494 Registered: December 2005
|
Senior Member |
|
|
The patch attached should help. The idea is simple. The "init" should be
called only once without NETNS. Period. No need for any lists.
I'll resend it to Dave after the ACK.
Regards,
Den
Benjamin Thery wrote:
> Hello Pavel,
>
> I've found a problem with one of your patch related to netns:
>
> * [NETNS] Move some code into __init section when CONFIG_NET_NS=n (v2)
> http://www.spinics.net/lists/netdev/msg43310.html
>
> This patch introduces the __net_init/__net_exit/__net_initdata
> defines to save some memory when CONFIG_NET_NS is not set.
>
> Cedric Le Goater reported he had a *non-fatal* oops when booting
> a 2.6.23-mm1-lxc1 kernel with CONFIG_NET_NS=n. (2.6.23-mm1-lxc1
> contains the NETNS49 patchset). The oops occured when modules
> related to iptables were loaded after the boot completes.
>
> The problem is the following:
>
> - Your patch adds the __net_initdata attribute to pernet_operations
> structures.
>
> - pernet_operations are registered via register_pernet_subsys() and
> linked in the pernet_list during boot.
>
> - At the end of boot, pernet_operations are freed (because of the
> __net_initdata attribute), and the pernet_list (or first_device list)
> points to freed memory.
>
> - After boot, network modules which are netns-aware try to register
> themselves with register_pernet_subsys() and ...KABOOM... page
> fault when accessing pernet_list (or first_device list).
> (I reproduce Cedric's oops with the command: iptables --list)
>
> This is not a problem right now in 2.6.23-mm1 or net-2.6 because
> there are very few netns-aware network subsystems merged and they
> are all initialized during boot. But it will be problematic when
> we will merge netns code for subsystems which can be built as
> modules (eg. iptables, ...). I'm not sure we can use
> __net_init_data for pernet_operations then.
> Maybe we can add some checks in register_pernet_operations
> when CONFIG_NET_NS=n.
>
> I haven't found a fix yet.
>
> Regards,
> Benjamin
>
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 6f71db8..9ba4809 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -186,7 +186,11 @@ static int register_pernet_operations(struct list_head *list,
int error;
error = 0;
+#ifdef CONFIG_NET_NS
list_add_tail(&ops->list, list);
+#endif
+ INIT_LIST_HEAD(&ops->list);
+#endif
for_each_net(net) {
if (ops->init) {
error = ops->init(net);
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers
|
|
|
Re: Re: [NETNS] Oops in register_pernet_operations() with CONFIG_NET_NS=n (resend, was wrong patch) [message #22326 is a reply to message #22325] |
Thu, 25 October 2007 14:14   |
den
Messages: 494 Registered: December 2005
|
Senior Member |
|
|
Denis V. Lunev wrote:
> The patch attached should help. The idea is simple. The "init" should be
> called only once without NETNS. Period. No need for any lists.
>
> I'll resend it to Dave after the ACK.
>
> Regards,
> Den
>
> Benjamin Thery wrote:
>> Hello Pavel,
>>
>> I've found a problem with one of your patch related to netns:
>>
>> * [NETNS] Move some code into __init section when CONFIG_NET_NS=n (v2)
>> http://www.spinics.net/lists/netdev/msg43310.html
>>
>> This patch introduces the __net_init/__net_exit/__net_initdata
>> defines to save some memory when CONFIG_NET_NS is not set.
>>
>> Cedric Le Goater reported he had a *non-fatal* oops when booting
>> a 2.6.23-mm1-lxc1 kernel with CONFIG_NET_NS=n. (2.6.23-mm1-lxc1
>> contains the NETNS49 patchset). The oops occured when modules
>> related to iptables were loaded after the boot completes.
>>
>> The problem is the following:
>>
>> - Your patch adds the __net_initdata attribute to pernet_operations
>> structures.
>>
>> - pernet_operations are registered via register_pernet_subsys() and
>> linked in the pernet_list during boot.
>>
>> - At the end of boot, pernet_operations are freed (because of the
>> __net_initdata attribute), and the pernet_list (or first_device list)
>> points to freed memory.
>>
>> - After boot, network modules which are netns-aware try to register
>> themselves with register_pernet_subsys() and ...KABOOM... page
>> fault when accessing pernet_list (or first_device list).
>> (I reproduce Cedric's oops with the command: iptables --list)
>>
>> This is not a problem right now in 2.6.23-mm1 or net-2.6 because
>> there are very few netns-aware network subsystems merged and they
>> are all initialized during boot. But it will be problematic when
>> we will merge netns code for subsystems which can be built as
>> modules (eg. iptables, ...). I'm not sure we can use
>> __net_init_data for pernet_operations then.
>> Maybe we can add some checks in register_pernet_operations
>> when CONFIG_NET_NS=n.
>>
>> I haven't found a fix yet.
>>
>> Regards,
>> Benjamin
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 6f71db8..cc306dc 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -186,7 +186,11 @@ static int register_pernet_operations(struct list_head *list,
int error;
error = 0;
+#ifdef CONFIG_NET_NS
list_add_tail(&ops->list, list);
+#else
+ INIT_LIST_HEAD(&ops->list);
+#endif
for_each_net(net) {
if (ops->init) {
error = ops->init(net);
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers
|
|
|
|
|
|
|
|
|
|
|
|
|
|
[PATCH] net: Marking struct pernet_operations __net_initdata was inappropriate [message #22398 is a reply to message #22368] |
Fri, 26 October 2007 23:45   |
ebiederm
Messages: 1354 Registered: February 2006
|
Senior Member |
|
|
It is not safe to to place struct pernet_operations in a special section.
We need struct pernet_operations to last until we call unregister_pernet_subsys.
Which doesn't happen until module unload.
So marking struct pernet_operations is a disaster for modules in two ways.
- We discard it before we call the exit method it points to.
- Because I keep struct pernet_operations on a linked list discarding
it for compiled in code removes elements in the middle of a linked
list and does horrible things for linked insert.
So this looks safe assuming __exit_refok is not discarded
for modules.
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/net/loopback.c | 2 +-
fs/proc/proc_net.c | 2 +-
include/net/net_namespace.h | 2 --
net/core/dev.c | 6 +++---
net/core/dev_mcast.c | 2 +-
net/netlink/af_netlink.c | 2 +-
6 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index 662b8d1..45f30a2 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -284,7 +284,7 @@ static __net_exit void loopback_net_exit(struct net *net)
unregister_netdev(dev);
}
-static struct pernet_operations __net_initdata loopback_net_ops = {
+static struct pernet_operations loopback_net_ops = {
.init = loopback_net_init,
.exit = loopback_net_exit,
};
diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c
index 4edaad0..749def0 100644
--- a/fs/proc/proc_net.c
+++ b/fs/proc/proc_net.c
@@ -185,7 +185,7 @@ static __net_exit void proc_net_ns_exit(struct net *net)
kfree(net->proc_net_root);
}
-static struct pernet_operations __net_initdata proc_net_ns_ops = {
+static struct pernet_operations proc_net_ns_ops = {
.init = proc_net_ns_init,
.exit = proc_net_ns_exit,
};
diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 93aa87d..5279466 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -102,11 +102,9 @@ static inline void release_net(struct net *net)
#ifdef CONFIG_NET_NS
#define __net_init
#define __net_exit
-#define __net_initdata
#else
#define __net_init __init
#define __net_exit __exit_refok
-#define __net_initdata __initdata
#endif
struct pernet_operations {
diff --git a/net/core/dev.c b/net/core/dev.c
index ddfef3b..853c8b5 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2668,7 +2668,7 @@ static void __net_exit dev_proc_net_exit(struct net *net)
proc_net_remove(net, "dev");
}
-static struct pernet_operations __net_initdata dev_proc_ops = {
+static struct pernet_operations dev_proc_ops = {
.init = dev_proc_net_init,
.exit = dev_proc_net_exit,
};
@@ -4328,7 +4328,7 @@ static void __net_exit netdev_exit(struct net *net)
kfree(net->dev_index_head);
}
-static struct pernet_operations __net_initdata netdev_net_ops = {
+static struct pernet_operations netdev_net_ops = {
.init = netdev_init,
.exit = netdev_exit,
};
@@ -4359,7 +4359,7 @@ static void __net_exit default_device_exit(struct net *net)
rtnl_unlock();
}
-static struct pernet_operations __net_initdata default_device_ops = {
+static struct pernet_operations default_device_ops = {
.exit = default_device_exit,
};
diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c
index 15241cf..ae35405 100644
--- a/net/core/dev_mcast.c
+++ b/net/core/dev_mcast.c
@@ -285,7 +285,7 @@ static void __net_exit dev_mc_net_exit(struct net *net)
proc_net_remove(net, "dev_mcast");
}
-static struct pernet_operations __net_initdata dev_mc_net_ops = {
+static struct pernet_operations dev_mc_net_ops = {
.init = dev_mc_net_init,
.exit = dev_mc_net_exit,
};
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 3252729..4f994c0 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1888,7 +1888,7 @@ static void __net_exit netlink_net_exit(struct net *net)
#endif
}
-static struct pernet_operations __net_initdata netlink_net_ops = {
+static struct pernet_operations netlink_net_ops = {
.init = netlink_net_init,
.exit = netlink_net_exit,
};
--
1.5.3.rc6.17.g1911
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers
|
|
|
|
|
Re: [PATCH] net: Marking struct pernet_operations __net_initdata was inappropriate [message #22410 is a reply to message #22408] |
Sat, 27 October 2007 07:29  |
davem
Messages: 463 Registered: February 2006
|
Senior Member |
|
|
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Sat, 27 Oct 2007 00:07:12 -0600
> This patch is minimal stupid and should just work. Doubtless the
> clever patch can be applied on top, once the details are figured
> out.
That is true and that's why I applied your patch.
Thanks!
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers
|
|
|