[B.A.T.M.A.N.] [PATCH net] net: batman-adv: Fix possible memleaks when fail to register_netdevice
From: Gao Feng fgao@ikuai8.com
Because the func batadv_softif_init_late allocate some resources and it would be invoked in register_netdevice. So we need to invoke the func batadv_softif_free instead of free_netdev to cleanup when fail to register_netdevice.
Signed-off-by: Gao Feng fgao@ikuai8.com --- net/batman-adv/soft-interface.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c index d042c99..90bf990 100644 --- a/net/batman-adv/soft-interface.c +++ b/net/batman-adv/soft-interface.c @@ -1011,7 +1011,7 @@ struct net_device *batadv_softif_create(struct net *net, const char *name) if (ret < 0) { pr_err("Unable to register the batman interface '%s': %i\n", name, ret); - free_netdev(soft_iface); + batadv_softif_free(soft_iface); return NULL; }
On Dienstag, 25. April 2017 20:03:20 CEST gfree.wind@foxmail.com wrote:
From: Gao Feng fgao@ikuai8.com
Because the func batadv_softif_init_late allocate some resources and it would be invoked in register_netdevice. So we need to invoke the func batadv_softif_free instead of free_netdev to cleanup when fail to register_netdevice.
I could be wrong, but shouldn't the destructor be replaced with free_netdevice and the batadv_softif_free (without the free_netdev) used as ndo_uninit? The line you've changed should then be kept as free_netdevice.
At least this seems to be important when using rtnl_newlink() instead of the legacy sysfs netdev stuff from batman-adv. rtnl_newlink() would also only call free_netdevice and thus also not run batadv_softif_free. This seems to be only fixable by calling ndo_uninit.
Kind regards, Sven
From: Sven Eckelmann [mailto:sven@narfation.org] Sent: Tuesday, April 25, 2017 9:53 PM On Dienstag, 25. April 2017 20:03:20 CEST gfree.wind@foxmail.com wrote:
From: Gao Feng fgao@ikuai8.com
Because the func batadv_softif_init_late allocate some resources and it would be invoked in register_netdevice. So we need to invoke the func batadv_softif_free instead of free_netdev to cleanup when fail to register_netdevice.
I could be wrong, but shouldn't the destructor be replaced with
free_netdevice
and the batadv_softif_free (without the free_netdev) used as ndo_uninit?
The
line you've changed should then be kept as free_netdevice.
At least this seems to be important when using rtnl_newlink() instead of
the
legacy sysfs netdev stuff from batman-adv. rtnl_newlink() would also only
call
free_netdevice and thus also not run batadv_softif_free. This seems to be
only
fixable by calling ndo_uninit.
Kind regards, Sven
Sorry, I don't get you. The net_dev is created in this func batadv_softif_create. Why couldn't invoke batadv_softif_free to cleanup when fail to register_netdevice?
Best Regards Feng
On Mittwoch, 26. April 2017 08:41:30 CEST Gao Feng wrote:
On Dienstag, 25. April 2017 20:03:20 CEST gfree.wind@foxmail.com wrote:
From: Gao Feng fgao@ikuai8.com
Because the func batadv_softif_init_late allocate some resources and it would be invoked in register_netdevice. So we need to invoke the func batadv_softif_free instead of free_netdev to cleanup when fail to register_netdevice.
I could be wrong, but shouldn't the destructor be replaced with free_netdevice and the batadv_softif_free (without the free_netdev) used as ndo_uninit? The line you've changed should then be kept as free_netdevice.
At least this seems to be important when using rtnl_newlink() instead of the legacy sysfs netdev stuff from batman-adv. rtnl_newlink() would also only call free_netdevice and thus also not run batadv_softif_free. This seems to be only fixable by calling ndo_uninit.
Sorry, I don't get you. The net_dev is created in this func batadv_softif_create. Why couldn't invoke batadv_softif_free to cleanup when fail to register_netdevice?
Because it is the legacy way to create the batadv interfaces and there is a "new" one. The new way is to use rtnl link (see batadv_link_ops).
The rtnl linke (rtnl_newlink) would not benefit from your fix and therefore still show the old behavior. I think a different fix is necessary to solve the problem for both ways to create an batadv interface.
Kind regards, Sven
From: Sven Eckelmann [mailto:sven@narfation.org] Sent: Wednesday, April 26, 2017 1:58 PM On Mittwoch, 26. April 2017 08:41:30 CEST Gao Feng wrote:
On Dienstag, 25. April 2017 20:03:20 CEST gfree.wind@foxmail.com wrote:
From: Gao Feng fgao@ikuai8.com
Because the func batadv_softif_init_late allocate some resources and it would be invoked in register_netdevice. So we need to invoke the func batadv_softif_free instead of free_netdev to cleanup when fail to register_netdevice.
I could be wrong, but shouldn't the destructor be replaced with free_netdevice and the batadv_softif_free (without the free_netdev) used as ndo_uninit? The line you've changed should then be kept as
free_netdevice.
At least this seems to be important when using rtnl_newlink() instead of the legacy sysfs netdev stuff from batman-adv. rtnl_newlink() would also only call free_netdevice and thus also not run batadv_softif_free. This seems to be only fixable by calling ndo_uninit.
Sorry, I don't get you. The net_dev is created in this func batadv_softif_create. Why couldn't invoke batadv_softif_free to cleanup when fail to register_netdevice?
Because it is the legacy way to create the batadv interfaces and there is
a
"new" one. The new way is to use rtnl link (see batadv_link_ops).
The rtnl linke (rtnl_newlink) would not benefit from your fix and
therefore still
show the old behavior. I think a different fix is necessary to solve the
problem
for both ways to create an batadv interface.
Kind regards, Sven
I get it now, thanks. Actually I sent another patch about the memleaks when invoke newlink and fail to register_netdev. You could refer to it by https://www.mail-archive.com/netdev@vger.kernel.org/msg165253.html.
In this patch, I add the cleanup when fail to register_netdev. It would be more simple if modify the rtnl_newlink and invoke the destructor of netdev when failed. Like the following codes. if (ops->newlink) { err = ops->newlink(link_net ? : net, dev, tb, data); if (err < 0) { if (dev->reg_state == NETREG_UNINITIALIZED) if (dev->destructor) dev->destructor(dev) else free_netdev(dev); goto out; } } else { err = register_netdevice(dev); if (err < 0) { if (dev->destructor) dev->destructor(dev); else free_netdev(dev); goto out; } }
But I don't know if it breaks the design newlink and destructor.
BTW, I think although the batadv_softif_create is legacy, we should fix it when it still exists :)
Regards Feng
On Mittwoch, 26. April 2017 14:44:24 CEST Gao Feng wrote: [...]
I get it now, thanks.
[...]
BTW, I think although the batadv_softif_create is legacy, we should fix it when it still exists :)
I didn't meant that we should not fix it. I just said that it looks to me like the fix should look different to ensure that it actually fixes the sysfs and rtnl link implementation for the batadv interface creation. Right now the ndo_uninit (when it would be set by batadv) is called in the netdev core functions when an error happens during the registration. This is not the case for the destructor. Your patch would not change it. It therefore looks like you simply have to move the current destructor (without the free_netdev) to ndo_uninit and change the destructor to free_netdev.
The batadv ops doesn't have a newlink function. It will therefore use the register_netdevice code path which calls free_netdev on failures. The extra cleanup you've added in https://www.mail-archive.com/netdev@vger.kernel.org/msg165253.html can therefore not work for batman-adv. Actually, it is not touching anything batman-adv related. The suggestion to change the register_netdevice -> free_netdev part in rtnl_newlink was new in the reply to the batadv discussion. It is therefore still an open discussion how it is correctly fixed.
Kind regards, Sven
From: Sven Eckelmann [mailto:sven@narfation.org] Sent: Wednesday, April 26, 2017 3:17 PM On Mittwoch, 26. April 2017 14:44:24 CEST Gao Feng wrote: [...]
I get it now, thanks.
[...]
BTW, I think although the batadv_softif_create is legacy, we should fix it when it still exists :)
I didn't meant that we should not fix it. I just said that it looks to me
like the fix
should look different to ensure that it actually fixes the sysfs and rtnl
link
implementation for the batadv interface creation. Right now the ndo_uninit (when it would be set by batadv) is called in the netdev core functions
when an
error happens during the registration. This is not the case for the
destructor.
Thanks your answer. I assumed the destructor is not for this case before..
Your patch would not change it. It therefore looks like you simply have to
move
the current destructor (without the free_netdev) to ndo_uninit and change
the
destructor to free_netdev.
Yes, that patch didn't touch badman-adv. Because current badman-adv doesn't support newlink now. It would be good that cleanup the resource in ndo_uninit routine.
Best Regards Feng
The batadv ops doesn't have a newlink function. It will therefore use the register_netdevice code path which calls free_netdev on failures. The
extra
cleanup you've added in https://www.mail-archive.com/netdev@vger.kernel.org/msg165253.html can therefore not work for batman-adv. Actually, it is not touching anything batman-adv related. The suggestion to change the register_netdevice -> free_netdev part in rtnl_newlink was new in the reply to the batadv
discussion.
It is therefore still an open discussion how it is correctly fixed.
Kind regards, Sven
On Dienstag, 25. April 2017 20:03:20 CEST gfree.wind@foxmail.com wrote:
From: Gao Feng fgao@ikuai8.com
Because the func batadv_softif_init_late allocate some resources and it would be invoked in register_netdevice. So we need to invoke the func batadv_softif_free instead of free_netdev to cleanup when fail to register_netdevice.
Signed-off-by: Gao Feng fgao@ikuai8.com
net/batman-adv/soft-interface.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c index d042c99..90bf990 100644 --- a/net/batman-adv/soft-interface.c +++ b/net/batman-adv/soft-interface.c @@ -1011,7 +1011,7 @@ struct net_device *batadv_softif_create(struct net *net, const char *name) if (ret < 0) { pr_err("Unable to register the batman interface '%s': %i\n", name, ret);
free_netdev(soft_iface);
batadv_softif_free(soft_iface);
It looks to me like this change is invalid after David's change [1]. Can you confirm that?
Thanks, Sven
[1] https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=cf1...
From: Sven Eckelmann [mailto:sven@narfation.org] Sent: Friday, June 9, 2017 3:23 PM Subject: Re: [B.A.T.M.A.N.] [PATCH net] net: batman-adv: Fix possible
memleaks
when fail to register_netdevice
On Dienstag, 25. April 2017 20:03:20 CEST gfree.wind@foxmail.com wrote:
From: Gao Feng fgao@ikuai8.com
Because the func batadv_softif_init_late allocate some resources and it would be invoked in register_netdevice. So we need to invoke the func batadv_softif_free instead of free_netdev to cleanup when fail to register_netdevice.
Signed-off-by: Gao Feng fgao@ikuai8.com
net/batman-adv/soft-interface.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c index d042c99..90bf990 100644 --- a/net/batman-adv/soft-interface.c +++ b/net/batman-adv/soft-interface.c @@ -1011,7 +1011,7 @@ struct net_device *batadv_softif_create(struct net
*net, const char *name)
if (ret < 0) { pr_err("Unable to register the batman interface '%s': %i\n", name, ret);
free_netdev(soft_iface);
batadv_softif_free(soft_iface);It looks to me like this change is invalid after David's change [1]. Can
you
confirm that?
Thanks, Sven
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=cf1
24db566e6b036b8bcbe8decbed740bdfac8c6
[Gao Feng]
yes, this change is unnecessary.
Best Regards Feng
b.a.t.m.a.n@lists.open-mesh.org
-
Gao Feng -
gfree.wind＠foxmail.com
-
Sven Eckelmann -
高峰