Message ID | 1348541310-31913-1-git-send-email-gaofeng@cn.fujitsu.com |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
On Tue, 2012-09-25 at 10:48 +0800, Gao feng 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. > > fix this by adding a reference of inet_diag module before > setting netlink_callback, and release this reference in > netlink_callback.done. > > Thanks for all help from Stephen,Jan and Eric. ... > > @@ -1001,8 +1025,26 @@ static int inet_diag_handler_dump(struct sk_buff *skb, struct nlmsghdr *h) > { > struct netlink_dump_control c = { > .dump = inet_diag_dump, > + .done = inet_diag_done, > }; > - return netlink_dump_start(net->diag_nlsk, skb, h, &c); > + int err; > + /* > + * netlink_dump will call inet_diag_dump, > + * so we need a reference of THIS_MODULE. > + */ > + if (!try_module_get(THIS_MODULE)) > + return -EPROTONOSUPPORT; > + > + err = netlink_dump_start(net->diag_nlsk, skb, h, &c); > + > + if ((err != -EINTR) && (err != -ENOBUFS)) { > + /* > + * netlink_callback set failed, release the > + * referenct of THIS_MODULE. > + */ > + module_put(THIS_MODULE); > + } > + return err; > } > } > Hmm... this seems error prone... In the future, netlink_dump_start() could be changed to return other errors than EINTR or ENOBUFS that need the module_put() I would change netlink_dump_start() to __netlink_dump_start() and add a module param to it, so that this module stuff is centralized in __netlink_dump_start() Then, instead of calling (from inet_diag) netlink_dump_start(net->diag_nlsk, skb, nlh, &c); you would use : __netlink_dump_start(net->diag_nlsk, skb, nlh, &c, THIS_MODULE); I wonder if this fix is not needed elsewhere eventually (net/unix/af_unix.c for example ?) -- 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月25日 14:36, Eric Dumazet 写道: > On Tue, 2012-09-25 at 10:48 +0800, Gao feng 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. >> >> fix this by adding a reference of inet_diag module before >> setting netlink_callback, and release this reference in >> netlink_callback.done. >> >> Thanks for all help from Stephen,Jan and Eric. > ... > >> >> @@ -1001,8 +1025,26 @@ static int inet_diag_handler_dump(struct sk_buff *skb, struct nlmsghdr *h) >> { >> struct netlink_dump_control c = { >> .dump = inet_diag_dump, >> + .done = inet_diag_done, >> }; >> - return netlink_dump_start(net->diag_nlsk, skb, h, &c); >> + int err; >> + /* >> + * netlink_dump will call inet_diag_dump, >> + * so we need a reference of THIS_MODULE. >> + */ >> + if (!try_module_get(THIS_MODULE)) >> + return -EPROTONOSUPPORT; >> + >> + err = netlink_dump_start(net->diag_nlsk, skb, h, &c); >> + >> + if ((err != -EINTR) && (err != -ENOBUFS)) { >> + /* >> + * netlink_callback set failed, release the >> + * referenct of THIS_MODULE. >> + */ >> + module_put(THIS_MODULE); >> + } >> + return err; >> } >> } >> > > Hmm... this seems error prone... > > In the future, netlink_dump_start() could be changed to return other > errors than EINTR or ENOBUFS that need the module_put() > EINTR and ENOBUFS is returned by netlink_dump, netlink_dump is called by netlink_dump_start after netlink_callback being set successfully. so this checking of EINTR and ENOBUFS here is to determinate if we set netlink_callback successfully. I think in order to reduce error prone,we have to change netlink_dump_start to determinate if we set netlink_callback successfully. > I would change netlink_dump_start() to __netlink_dump_start() and add a > module param to it, so that this module stuff is centralized in > __netlink_dump_start() > > Then, instead of calling (from inet_diag) > > netlink_dump_start(net->diag_nlsk, skb, nlh, &c); > > you would use : > > __netlink_dump_start(net->diag_nlsk, skb, nlh, &c, THIS_MODULE); > > I wonder if this fix is not needed elsewhere eventually > (net/unix/af_unix.c for example ?) > do you mean net/unix/unix_diag.c ? I test nfnetlink module,it has the same problem. It's need to modify netlink_dump_start not only wrap netlink_dump_start. -- 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月25日 15:20, Gao feng 写道:
> It's need to modify netlink_dump_start not only wrap netlink_dump_start.
maybe it's better to add a struct module *module in struct netlink_callback..
--
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/inet_diag.c b/net/ipv4/inet_diag.c index 570e61f..e573090 100644 --- a/net/ipv4/inet_diag.c +++ b/net/ipv4/inet_diag.c @@ -903,6 +903,12 @@ static int inet_diag_dump(struct sk_buff *skb, struct netlink_callback *cb) return __inet_diag_dump(skb, cb, nlmsg_data(cb->nlh), bc); } +static int inet_diag_done(struct netlink_callback *cb) +{ + module_put(THIS_MODULE); + return 0; +} + static inline int inet_diag_type2proto(int type) { switch (type) { @@ -972,8 +978,26 @@ static int inet_diag_rcv_msg_compat(struct sk_buff *skb, struct nlmsghdr *nlh) { struct netlink_dump_control c = { .dump = inet_diag_dump_compat, + .done = inet_diag_done, }; - return netlink_dump_start(net->diag_nlsk, skb, nlh, &c); + int err; + /* + * netlink_dump will call inet_diag_dump_compat, + * so we need a reference of THIS_MODULE. + */ + if (!try_module_get(THIS_MODULE)) + return -EPROTONOSUPPORT; + + err = netlink_dump_start(net->diag_nlsk, skb, nlh, &c); + + if ((err != -EINTR) && (err != -ENOBUFS)) { + /* + * netlink_callback set failed, release the + * referenct of THIS_MODULE. + */ + module_put(THIS_MODULE); + } + return err; } } @@ -1001,8 +1025,26 @@ static int inet_diag_handler_dump(struct sk_buff *skb, struct nlmsghdr *h) { struct netlink_dump_control c = { .dump = inet_diag_dump, + .done = inet_diag_done, }; - return netlink_dump_start(net->diag_nlsk, skb, h, &c); + int err; + /* + * netlink_dump will call inet_diag_dump, + * so we need a reference of THIS_MODULE. + */ + if (!try_module_get(THIS_MODULE)) + return -EPROTONOSUPPORT; + + err = netlink_dump_start(net->diag_nlsk, skb, h, &c); + + if ((err != -EINTR) && (err != -ENOBUFS)) { + /* + * netlink_callback set failed, release the + * referenct of THIS_MODULE. + */ + module_put(THIS_MODULE); + } + return err; } }
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. fix this by adding a reference of inet_diag module before setting netlink_callback, and release this reference in netlink_callback.done. Thanks for all help from Stephen,Jan and Eric. Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> --- net/ipv4/inet_diag.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 44 insertions(+), 2 deletions(-)