[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1348555011.26828.2031.camel@edumazet-glaptop>
Date: Tue, 25 Sep 2012 08:36:51 +0200
From: Eric Dumazet <eric.dumazet@...il.com>
To: Gao feng <gaofeng@...fujitsu.com>
Cc: davem@...emloft.net, stephen.hemminger@...tta.com, jengelh@...i.de,
kuznet@....inr.ac.ru, netdev@...r.kernel.org
Subject: Re: [PATCH] inet_diag: fix panic when unload inet_diag
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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists