Message ID | 1493121800-28066-1-git-send-email-gfree.wind@foxmail.com (mailing list archive) |
---|---|
State | Rejected, archived |
Delegated to: | Sven Eckelmann |
Headers |
Return-Path: <b.a.t.m.a.n-bounces@lists.open-mesh.org> X-Original-To: patchwork@open-mesh.org Delivered-To: patchwork@open-mesh.org Received: from open-mesh.org (localhost [IPv6:::1]) by open-mesh.org (Postfix) with ESMTP id 3BD088362E; Tue, 25 Apr 2017 14:47:16 +0200 (CEST) Authentication-Results: open-mesh.org; dmarc=none header.from=foxmail.com Authentication-Results: open-mesh.org; dkim=fail reason="verification failed; unprotected key" header.d=foxmail.com header.i=@foxmail.com header.b=adMITcVE; dkim-adsp=none (unprotected policy); dkim-atps=neutral Received: from smtpbg298.qq.com (smtpbg298.qq.com [184.105.67.102]) by open-mesh.org (Postfix) with ESMTPS id 8AFF483323 for <b.a.t.m.a.n@lists.open-mesh.org>; Tue, 25 Apr 2017 14:03:47 +0200 (CEST) Authentication-Results: open-mesh.org; dmarc=pass header.from=foxmail.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foxmail.com; s=s201512; t=1493121814; bh=YDC9HEcUDk0UD3YdwEYqoHYIzUN2QJDzXP8VMmEHrBk=; h=From:To:Cc:Subject:Date:Message-Id; b=adMITcVEHUqEM3td2qiw9Ht7z6EwYa2I7fVqiTqA51oLL8wXW86b1pESOuD+eF+bK tMt/OQISzrIWpdmriI4HbNA46F9Vvz7g8uxjcaQK6QZQSHCQ3Tt7eikVAe2N1L77r4 rW4T1UENsw/DtUX83cGf51VqrC5kQZS5FQLCtqk4= X-QQ-mid: esmtp27t1493121808to5bypyr3 Received: from ikuai8.com (unknown [114.242.17.234]) by esmtp4.qq.com (ESMTP) with id ; Tue, 25 Apr 2017 20:03:22 +0800 (CST) X-QQ-SSF: 01000000000000F0FG600F00000000Q X-QQ-FEAT: tZ7SKDDlSpHlk6ZzfJmRlBzOsarw2BL0CsgswP43X7+cfbufV9hmF9wnGEBSP 4cU+pUn2ZJlPjrThLNNWWggOz/Kip5F+Tj7c0hT3McRy+tuE/+FsZuama5/83hyx1Fy7tn2 Pu9r1Y1qgbT1NP6dpVlMwgQA173/5hKkzwciBxbyjnhBnN8WivGEwIZux3mC6Qj80wXTCVe 2DMvhVa3FNK8CRqKfSUfgO1eeEiLJRqiFW0ohBwaoVDS2/5ale49a4Av81bBM19oMR0AvFP HVZrJh8zVvdXar X-QQ-GoodBg: 0 From: gfree.wind@foxmail.com To: mareklindner@neomailbox.ch, sw@simonwunderlich.de, a@unstable.cc, davem@davemloft.net, b.a.t.m.a.n@lists.open-mesh.org, netdev@vger.kernel.org Date: Tue, 25 Apr 2017 20:03:20 +0800 Message-Id: <1493121800-28066-1-git-send-email-gfree.wind@foxmail.com> X-Mailer: git-send-email 1.9.1 X-QQ-SENDSIZE: 520 X-QQ-FName: C885DE5C534B488BB546C7E5CA355361 X-QQ-LocalIP: 10.130.87.224 X-Mailman-Approved-At: Tue, 25 Apr 2017 14:47:14 +0200 Cc: Gao Feng <fgao@ikuai8.com> Subject: [B.A.T.M.A.N.] [PATCH net] net: batman-adv: Fix possible memleaks when fail to register_netdevice X-BeenThere: b.a.t.m.a.n@lists.open-mesh.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: The list for a Better Approach To Mobile Ad-hoc Networking <b.a.t.m.a.n.lists.open-mesh.org> List-Unsubscribe: <https://lists.open-mesh.org/mm/options/b.a.t.m.a.n>, <mailto:b.a.t.m.a.n-request@lists.open-mesh.org?subject=unsubscribe> List-Archive: <http://lists.open-mesh.org/pipermail/b.a.t.m.a.n/> List-Post: <mailto:b.a.t.m.a.n@lists.open-mesh.org> List-Help: <mailto:b.a.t.m.a.n-request@lists.open-mesh.org?subject=help> List-Subscribe: <https://lists.open-mesh.org/mm/listinfo/b.a.t.m.a.n>, <mailto:b.a.t.m.a.n-request@lists.open-mesh.org?subject=subscribe> Reply-To: The list for a Better Approach To Mobile Ad-hoc Networking <b.a.t.m.a.n@lists.open-mesh.org> Errors-To: b.a.t.m.a.n-bounces@lists.open-mesh.org Sender: "B.A.T.M.A.N" <b.a.t.m.a.n-bounces@lists.open-mesh.org> |
Commit Message
gfree.wind@foxmail.com
April 25, 2017, 12:03 p.m. UTC
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(-)
Comments
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); > return NULL; > } 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=cf124db566e6b036b8bcbe8decbed740bdfac8c6
> 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); > > return NULL; > > } > > 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
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; }