[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(-)
  

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