| Message ID | 1348372154-20663-1-git-send-email-gaofeng@cn.fujitsu.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
> when inet_diag being compiled as module, inet_diag_handler_dump > set netlink_dump_control.dump to inet_diag_dump,so if module > inet_diag is unloaded,netlink will still try to call this function > in netlink_dump. this will cause kernel panic. > Another way to handle this to just get rid of inet_diag_exit so that module can be loaded but not unloaded. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sunday 2012-09-23 06:36, Stephen Hemminger wrote: >> when inet_diag being compiled as module, inet_diag_handler_dump >> set netlink_dump_control.dump to inet_diag_dump,so if module >> inet_diag is unloaded,netlink will still try to call this function >> in netlink_dump. this will cause kernel panic. > >Another way to handle this to just get rid of inet_diag_exit >so that module can be loaded but not unloaded. Why don't we unset netlink_dump_control.dump on exit? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
于 2012年09月23日 21:40, Jan Engelhardt 写道: > On Sunday 2012-09-23 06:36, Stephen Hemminger wrote: > >>> when inet_diag being compiled as module, inet_diag_handler_dump >>> set netlink_dump_control.dump to inet_diag_dump,so if module >>> inet_diag is unloaded,netlink will still try to call this function >>> in netlink_dump. this will cause kernel panic. >> >> Another way to handle this to just get rid of inet_diag_exit >> so that module can be loaded but not unloaded. > > Why don't we unset netlink_dump_control.dump on exit? Though I like this idea,but it may cause dead lock. netlink_dimp [mutex_lock(netlink_sock->cb_mutex) here] |->inet_diag_dump |->__inet_diag_dump |->inet_diag_lock_handler [may try to load tcp_diag here, so we need module_mutex lock] And on module exit,we already get module_mutex lock, if we unset netlink_sock->cb,we need to get mutex lock of netlink_sock->cb_mutex. I think this will cause dead lock. I don't know if I should change this patch as Stephen said,because I don't know witch one is better. Any comments? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-09-24 at 16:48 +0800, Gao feng wrote: > 于 2012年09月23日 21:40, Jan Engelhardt 写道: > > On Sunday 2012-09-23 06:36, Stephen Hemminger wrote: > > > >>> when inet_diag being compiled as module, inet_diag_handler_dump > >>> set netlink_dump_control.dump to inet_diag_dump,so if module > >>> inet_diag is unloaded,netlink will still try to call this function > >>> in netlink_dump. this will cause kernel panic. > >> > >> Another way to handle this to just get rid of inet_diag_exit > >> so that module can be loaded but not unloaded. > > > > Why don't we unset netlink_dump_control.dump on exit? > > > Though I like this idea,but it may cause dead lock. > netlink_dimp [mutex_lock(netlink_sock->cb_mutex) here] > |->inet_diag_dump > |->__inet_diag_dump > |->inet_diag_lock_handler [may try to load tcp_diag here, > so we need module_mutex lock] > > And on module exit,we already get module_mutex lock, > if we unset netlink_sock->cb,we need to get mutex lock of netlink_sock->cb_mutex. > > I think this will cause dead lock. > > I don't know if I should change this patch as Stephen said,because I don't know > witch one is better. > > Any comments? In fact I didnt fully understand the problem you try to address. If you want to prevent module being unloaded, you need to add proper module_get()/module_put() So I would add a "struct module *module;" in struct sock_diag_handler and use it appropriately. But then, I might have totally misunderstood the problem. Care to explain how you trigger the bug ? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
于 2012年09月24日 17:42, Eric Dumazet 写道: > In fact I didnt fully understand the problem you try to address. > > If you want to prevent module being unloaded, you need to add proper > module_get()/module_put() > > So I would add a "struct module *module;" in struct sock_diag_handler > and use it appropriately. Yes, I try to add reference of the module,but I can't find a proper location to call module_get and module_put. module_get should be called when userspace program use netlink to send dump request to the kernel,and module_put should be called when the dump is completed. I am right? BUT the userspace program may only call netlink_sendmsg without call netlink_recvmsg.so the reference of the module will be incorrect. > > But then, I might have totally misunderstood the problem. > > Care to explain how you trigger the bug ? It's very easy to trigger this bug, you can exec "while :; do ss -a ; done" in one terminal and exec "while :; do rmmod tcp_diag && rmmod inet_diag; done" in another terminal. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-09-24 at 18:17 +0800, Gao feng wrote: > 于 2012年09月24日 17:42, Eric Dumazet 写道: > > In fact I didnt fully understand the problem you try to address. > > > > If you want to prevent module being unloaded, you need to add proper > > module_get()/module_put() > > > > So I would add a "struct module *module;" in struct sock_diag_handler > > and use it appropriately. > > Yes, I try to add reference of the module,but I can't find a proper > location to call module_get and module_put. > > module_get should be called when userspace program use netlink to > send dump request to the kernel,and module_put should be called when > the dump is completed. I am right? > > BUT the userspace program may only call netlink_sendmsg without call > netlink_recvmsg.so the reference of the module will be incorrect. check ->dump() and ->done() methods -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
于 2012年09月24日 19:32, Eric Dumazet 写道: > On Mon, 2012-09-24 at 18:17 +0800, Gao feng wrote: >> 于 2012年09月24日 17:42, Eric Dumazet 写道: >>> In fact I didnt fully understand the problem you try to address. >>> >>> If you want to prevent module being unloaded, you need to add proper >>> module_get()/module_put() >>> >>> So I would add a "struct module *module;" in struct sock_diag_handler >>> and use it appropriately. >> >> Yes, I try to add reference of the module,but I can't find a proper >> location to call module_get and module_put. >> >> module_get should be called when userspace program use netlink to >> send dump request to the kernel,and module_put should be called when >> the dump is completed. I am right? >> >> BUT the userspace program may only call netlink_sendmsg without call >> netlink_recvmsg.so the reference of the module will be incorrect. > > check ->dump() and ->done() methods > I miss that cb->done will be called when netlink sock being destructed. so add a reference of the inet_diag module is doable. I will send a v2 patch. Thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/Kconfig b/net/ipv4/Kconfig index 5a19aeb..da56f84 100644 --- a/net/ipv4/Kconfig +++ b/net/ipv4/Kconfig @@ -403,7 +403,7 @@ config INET_LRO If unsure, say Y. config INET_DIAG - tristate "INET: socket monitoring interface" + bool "INET: socket monitoring interface" default y ---help--- Support for INET (TCP, DCCP, etc) socket monitoring interface used by
when inet_diag being compiled as module, inet_diag_handler_dump set netlink_dump_control.dump to inet_diag_dump,so if module inet_diag is unloaded,netlink will still try to call this function in netlink_dump. this will cause kernel panic. this patch makes config INET_DIAG bool to avoid inet_diag being compiled as module. Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> --- net/ipv4/Kconfig | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)