[PATCH net v3] driver: veth: Fix one possbile memleak when fail to register_netdevice

[PATCH net v3] driver: veth: Fix one possbile memleak when fail to register_netdevice

[gfree.wind]

From: Gao Feng <gfree.wind@foxmail.com>

The veth driver allocates some resources in its ndo_init func, and
free them in its destructor func. Then there is one memleak that some
errors happen after register_netdevice invokes the ndo_init callback.
Because the destructor would not be invoked to free the resources.

Now create one new func veth_destructor_free to free the mem in the
destructor, and add ndo_uninit func also invokes it when fail to register
the veth device.

It's not only free all resources, but also follow the original desgin
that the resources are freed in the destructor normally after
register the device successfully.

Signed-off-by: Gao Feng <gfree.wind@foxmail.com>
---
 v3: Split one patch to multiple commits, per David Ahern
 v2: Move the free in ndo_uninit when fail to register, per Herbert Xu
 v1: initial version

 drivers/net/veth.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 8c39d6d..418376a 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -224,9 +224,21 @@ static int veth_dev_init(struct net_device *dev)
 	return 0;
 }
 
-static void veth_dev_free(struct net_device *dev)
+static void veth_destructor_free(struct net_device *dev)
 {
 	free_percpu(dev->vstats);
+}
+
+static void veth_dev_uninit(struct net_device *dev)
+{
+	/* dev is not registered, perform the free instead of destructor */
+	if (dev->reg_state == NETREG_UNINITIALIZED)
+		veth_destructor_free(dev);
+}
+
+static void veth_dev_free(struct net_device *dev)
+{
+	veth_destructor_free(dev);
 	free_netdev(dev);
 }
 
@@ -284,6 +296,7 @@ static void veth_set_rx_headroom(struct net_device *dev, int new_hr)
 
 static const struct net_device_ops veth_netdev_ops = {
 	.ndo_init            = veth_dev_init,
+	.ndo_uninit          = veth_dev_uninit,
 	.ndo_open            = veth_open,
 	.ndo_stop            = veth_close,
 	.ndo_start_xmit      = veth_xmit,
-- 
2.7.4

Re: [PATCH net v3] driver: veth: Fix one possbile memleak when fail to register_netdevice

[David Ahern]

On 4/28/17 9:51 PM, gfree.wind@foxmail.com wrote:
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index 8c39d6d..418376a 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -224,9 +224,21 @@ static int veth_dev_init(struct net_device *dev)
>  	return 0;
>  }
>  
> -static void veth_dev_free(struct net_device *dev)
> +static void veth_destructor_free(struct net_device *dev)

_destructor in the name is confusing since veth_dev_free is the
dev->destructor. Perhaps that should be veth_free_stats.


>  {
>  	free_percpu(dev->vstats);
> +}
> +
> +static void veth_dev_uninit(struct net_device *dev)
> +{
> +	/* dev is not registered, perform the free instead of destructor */
> +	if (dev->reg_state == NETREG_UNINITIALIZED)
> +		veth_destructor_free(dev);
> +}
> +
> +static void veth_dev_free(struct net_device *dev)
> +{
> +	veth_destructor_free(dev);
>  	free_netdev(dev);
>  }
>  
> @@ -284,6 +296,7 @@ static void veth_set_rx_headroom(struct net_device *dev, int new_hr)
>  
>  static const struct net_device_ops veth_netdev_ops = {
>  	.ndo_init            = veth_dev_init,
> +	.ndo_uninit          = veth_dev_uninit,
>  	.ndo_open            = veth_open,
>  	.ndo_stop            = veth_close,
>  	.ndo_start_xmit      = veth_xmit,
> 

Functionally, it looks good to me.

Acked-by: David Ahern <dsa@cumulusnetworks.com>

Re: [PATCH net v3] driver: veth: Fix one possbile memleak when fail to register_netdevice

[Xin Long]

On Sat, Apr 29, 2017 at 11:51 AM,  <gfree.wind@foxmail.com> wrote:
> From: Gao Feng <gfree.wind@foxmail.com>
>
> The veth driver allocates some resources in its ndo_init func, and
> free them in its destructor func. Then there is one memleak that some
> errors happen after register_netdevice invokes the ndo_init callback.
> Because the destructor would not be invoked to free the resources.
>
> Now create one new func veth_destructor_free to free the mem in the
> destructor, and add ndo_uninit func also invokes it when fail to register
> the veth device.
>
> It's not only free all resources, but also follow the original desgin
> that the resources are freed in the destructor normally after
> register the device successfully.
>
> Signed-off-by: Gao Feng <gfree.wind@foxmail.com>
> ---
>  v3: Split one patch to multiple commits, per David Ahern
>  v2: Move the free in ndo_uninit when fail to register, per Herbert Xu
>  v1: initial version
>
>  drivers/net/veth.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index 8c39d6d..418376a 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -224,9 +224,21 @@ static int veth_dev_init(struct net_device *dev)
>         return 0;
>  }
>
> -static void veth_dev_free(struct net_device *dev)
> +static void veth_destructor_free(struct net_device *dev)
>  {
>         free_percpu(dev->vstats);
> +}
not sure why you needed to add this function.
to use free_percpu() directly may be clearer.

> +
> +static void veth_dev_uninit(struct net_device *dev)
> +{
call free_percpu() here, no need to check dev->reg_state.
free_percpu will just return if dev->vstats is NULL.

> +       /* dev is not registered, perform the free instead of destructor */
> +       if (dev->reg_state == NETREG_UNINITIALIZED)
> +               veth_destructor_free(dev);
> +}
> +
> +static void veth_dev_free(struct net_device *dev)
> +{
> +       veth_destructor_free(dev);
use free_percpu here as well.

>         free_netdev(dev);
>  }
>
> @@ -284,6 +296,7 @@ static void veth_set_rx_headroom(struct net_device *dev, int new_hr)
>
>  static const struct net_device_ops veth_netdev_ops = {
>         .ndo_init            = veth_dev_init,
> +       .ndo_uninit          = veth_dev_uninit,
>         .ndo_open            = veth_open,
>         .ndo_stop            = veth_close,
>         .ndo_start_xmit      = veth_xmit,
> --
> 2.7.4
>

RE: [PATCH net v3] driver: veth: Fix one possbile memleak when fail to register_netdevice

[Gao Feng]

> From: David Ahern [mailto:dsa@cumulusnetworks.com]
> Sent: Monday, May 1, 2017 11:08 PM
> On 4/28/17 9:51 PM, gfree.wind@foxmail.com wrote:
> > diff --git a/drivers/net/veth.c b/drivers/net/veth.c index
> > 8c39d6d..418376a 100644
> > --- a/drivers/net/veth.c
> > +++ b/drivers/net/veth.c
> > @@ -224,9 +224,21 @@ static int veth_dev_init(struct net_device *dev)
> >  	return 0;
> >  }
> >
> > -static void veth_dev_free(struct net_device *dev)
> > +static void veth_destructor_free(struct net_device *dev)
> 
> _destructor in the name is confusing since veth_dev_free is the
> dev->destructor. Perhaps that should be veth_free_stats.

Because I want to emphasize it should be invoked in the destructor.
What's your opinion ?


[...]
> Functionally, it looks good to me.
> 
> Acked-by: David Ahern <dsa@cumulusnetworks.com>

Thanks David.
I have sent the v4 patches with a series according to David's advice.

BTW, because I send multiple patches too fast today, the email server blocks
my account.
So I have to reply you with a different email account. Sorry.

Regards
Feng

RE: [PATCH net v3] driver: veth: Fix one possbile memleak when fail to register_netdevice

[Gao Feng]

> From: Xin Long [mailto:lucien.xin@gmail.com]
> Sent: Tuesday, May 2, 2017 3:56 PM
> On Sat, Apr 29, 2017 at 11:51 AM,  <gfree.wind@foxmail.com> wrote:
> > From: Gao Feng <gfree.wind@foxmail.com>
[...]
> > -static void veth_dev_free(struct net_device *dev)
> > +static void veth_destructor_free(struct net_device *dev)
> >  {
> >         free_percpu(dev->vstats);
> > +}
> not sure why you needed to add this function.
> to use free_percpu() directly may be clearer.

Because both of ndo_uninit and destructor need to perform same free statements.
It is good at maintain the codes with the common function.
> 
> > +
> > +static void veth_dev_uninit(struct net_device *dev) {
> call free_percpu() here, no need to check dev->reg_state.
> free_percpu will just return if dev->vstats is NULL.

It would break the original design if don't check the reg_state.
The original logic is that free the resources in the destructor, not in ndo_init.

BTW, because I send multiple patches too fast today, the email server blocks my account.
So I have to reply you with a different email account. Sorry.

Best Regards
Feng

> 
[...]

Re: [PATCH net v3] driver: veth: Fix one possbile memleak when fail to register_netdevice

[Xin Long]

On Tue, May 2, 2017 at 7:03 PM, Gao Feng <gfree.wind@vip.163.com> wrote:
>> From: Xin Long [mailto:lucien.xin@gmail.com]
>> Sent: Tuesday, May 2, 2017 3:56 PM
>> On Sat, Apr 29, 2017 at 11:51 AM,  <gfree.wind@foxmail.com> wrote:
>> > From: Gao Feng <gfree.wind@foxmail.com>
> [...]
>> > -static void veth_dev_free(struct net_device *dev)
>> > +static void veth_destructor_free(struct net_device *dev)
>> >  {
>> >         free_percpu(dev->vstats);
>> > +}
>> not sure why you needed to add this function.
>> to use free_percpu() directly may be clearer.
>
> Because both of ndo_uninit and destructor need to perform same free statements.
> It is good at maintain the codes with the common function.
>>
>> > +
>> > +static void veth_dev_uninit(struct net_device *dev) {
>> call free_percpu() here, no need to check dev->reg_state.
>> free_percpu will just return if dev->vstats is NULL.
>
> It would break the original design if don't check the reg_state.
> The original logic is that free the resources in the destructor, not in ndo_init.
I got what you're doing now, can you pls try to fix this with:

--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -219,10 +219,9 @@ static int veth_dev_init(struct net_device *dev)
        return 0;
 }

-static void veth_dev_free(struct net_device *dev)
+static void veth_dev_uninit(struct net_device *dev)
 {
        free_percpu(dev->vstats);
-       free_netdev(dev);
 }

 #ifdef CONFIG_NET_POLL_CONTROLLER
@@ -279,6 +278,7 @@ static void veth_set_rx_headroom(struct net_device
*dev, int new_hr)

 static const struct net_device_ops veth_netdev_ops = {
        .ndo_init            = veth_dev_init,
+       .ndo_uninit          = veth_dev_uninit,
        .ndo_open            = veth_open,
        .ndo_stop            = veth_close,
        .ndo_start_xmit      = veth_xmit,
@@ -317,7 +317,7 @@ static void veth_setup(struct net_device *dev)
                               NETIF_F_HW_VLAN_STAG_TX |
                               NETIF_F_HW_VLAN_CTAG_RX |
                               NETIF_F_HW_VLAN_STAG_RX);
-       dev->destructor = veth_dev_free;
+       dev->destructor = free_netdev;
        dev->max_mtu = ETH_MAX_MTU;

        dev->hw_features = VETH_FEATURES;


just as what other virtual nic drivers do (vxlan, geneve, macsec, bridge ....)

>
> BTW, because I send multiple patches too fast today, the email server blocks my account.
> So I have to reply you with a different email account. Sorry.
>
> Best Regards
> Feng
>
>>
> [...]
>
>

RE: [PATCH net v3] driver: veth: Fix one possbile memleak when fail to register_netdevice

[Gao Feng]

> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Xin Long
> Sent: Wednesday, May 3, 2017 12:59 AM
> On Tue, May 2, 2017 at 7:03 PM, Gao Feng <gfree.wind@vip.163.com> wrote:
> >> From: Xin Long [mailto:lucien.xin@gmail.com]
> >> Sent: Tuesday, May 2, 2017 3:56 PM
> >> On Sat, Apr 29, 2017 at 11:51 AM,  <gfree.wind@foxmail.com> wrote:
> >> > From: Gao Feng <gfree.wind@foxmail.com>
> > [...]
> >> > -static void veth_dev_free(struct net_device *dev)
> >> > +static void veth_destructor_free(struct net_device *dev)
> >> >  {
> >> >         free_percpu(dev->vstats);
> >> > +}
> >> not sure why you needed to add this function.
> >> to use free_percpu() directly may be clearer.
> >
> > Because both of ndo_uninit and destructor need to perform same free
> statements.
> > It is good at maintain the codes with the common function.
> >>
> >> > +
> >> > +static void veth_dev_uninit(struct net_device *dev) {
> >> call free_percpu() here, no need to check dev->reg_state.
> >> free_percpu will just return if dev->vstats is NULL.
> >
> > It would break the original design if don't check the reg_state.
> > The original logic is that free the resources in the destructor, not in ndo_init.
> I got what you're doing now, can you pls try to fix this with:
> 
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -219,10 +219,9 @@ static int veth_dev_init(struct net_device *dev)
>         return 0;
>  }
> 
> -static void veth_dev_free(struct net_device *dev)
> +static void veth_dev_uninit(struct net_device *dev)
>  {
>         free_percpu(dev->vstats);
> -       free_netdev(dev);
>  }
> 
>  #ifdef CONFIG_NET_POLL_CONTROLLER
> @@ -279,6 +278,7 @@ static void veth_set_rx_headroom(struct net_device
> *dev, int new_hr)
> 
>  static const struct net_device_ops veth_netdev_ops = {
>         .ndo_init            = veth_dev_init,
> +       .ndo_uninit          = veth_dev_uninit,
>         .ndo_open            = veth_open,
>         .ndo_stop            = veth_close,
>         .ndo_start_xmit      = veth_xmit,
> @@ -317,7 +317,7 @@ static void veth_setup(struct net_device *dev)
>                                NETIF_F_HW_VLAN_STAG_TX |
>                                NETIF_F_HW_VLAN_CTAG_RX |
>                                NETIF_F_HW_VLAN_STAG_RX);
> -       dev->destructor = veth_dev_free;
> +       dev->destructor = free_netdev;
>         dev->max_mtu = ETH_MAX_MTU;
> 
>         dev->hw_features = VETH_FEATURES;
> 
> 
> just as what other virtual nic drivers do (vxlan, geneve, macsec, bridge ....)
> 

The fix you mentioned change the original logic.
The dev->vstats is freed in advance in the ndo_uninit, not destructor.
It may break the backward.

Regards
Feng

Re: [PATCH net v3] driver: veth: Fix one possbile memleak when fail to register_netdevice

[Xin Long]

On Wed, May 3, 2017 at 10:07 AM, Gao Feng <gfree.wind@foxmail.com> wrote:
>> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
>> On Behalf Of Xin Long
>> Sent: Wednesday, May 3, 2017 12:59 AM
>> On Tue, May 2, 2017 at 7:03 PM, Gao Feng <gfree.wind@vip.163.com> wrote:
>> >> From: Xin Long [mailto:lucien.xin@gmail.com]
>> >> Sent: Tuesday, May 2, 2017 3:56 PM
>> >> On Sat, Apr 29, 2017 at 11:51 AM,  <gfree.wind@foxmail.com> wrote:
>> >> > From: Gao Feng <gfree.wind@foxmail.com>
>> > [...]
>> >> > -static void veth_dev_free(struct net_device *dev)
>> >> > +static void veth_destructor_free(struct net_device *dev)
>> >> >  {
>> >> >         free_percpu(dev->vstats);
>> >> > +}
>> >> not sure why you needed to add this function.
>> >> to use free_percpu() directly may be clearer.
>> >
>> > Because both of ndo_uninit and destructor need to perform same free
>> statements.
>> > It is good at maintain the codes with the common function.
>> >>
>> >> > +
>> >> > +static void veth_dev_uninit(struct net_device *dev) {
>> >> call free_percpu() here, no need to check dev->reg_state.
>> >> free_percpu will just return if dev->vstats is NULL.
>> >
>> > It would break the original design if don't check the reg_state.
>> > The original logic is that free the resources in the destructor, not in ndo_init.
>> I got what you're doing now, can you pls try to fix this with:
>>
>> --- a/drivers/net/veth.c
>> +++ b/drivers/net/veth.c
>> @@ -219,10 +219,9 @@ static int veth_dev_init(struct net_device *dev)
>>         return 0;
>>  }
>>
>> -static void veth_dev_free(struct net_device *dev)
>> +static void veth_dev_uninit(struct net_device *dev)
>>  {
>>         free_percpu(dev->vstats);
>> -       free_netdev(dev);
>>  }
>>
>>  #ifdef CONFIG_NET_POLL_CONTROLLER
>> @@ -279,6 +278,7 @@ static void veth_set_rx_headroom(struct net_device
>> *dev, int new_hr)
>>
>>  static const struct net_device_ops veth_netdev_ops = {
>>         .ndo_init            = veth_dev_init,
>> +       .ndo_uninit          = veth_dev_uninit,
>>         .ndo_open            = veth_open,
>>         .ndo_stop            = veth_close,
>>         .ndo_start_xmit      = veth_xmit,
>> @@ -317,7 +317,7 @@ static void veth_setup(struct net_device *dev)
>>                                NETIF_F_HW_VLAN_STAG_TX |
>>                                NETIF_F_HW_VLAN_CTAG_RX |
>>                                NETIF_F_HW_VLAN_STAG_RX);
>> -       dev->destructor = veth_dev_free;
>> +       dev->destructor = free_netdev;
>>         dev->max_mtu = ETH_MAX_MTU;
>>
>>         dev->hw_features = VETH_FEATURES;
>>
>>
>> just as what other virtual nic drivers do (vxlan, geneve, macsec, bridge ....)
>>
>
> The fix you mentioned change the original logic.
> The dev->vstats is freed in advance in the ndo_uninit, not destructor.
> It may break the backward.
Sorry, I didn't get your "backward"
I can't see there will be any problem caused by it.

can you say this patch also break the 'backward' ?
https://patchwork.ozlabs.org/patch/748964/

It's really weird to do dev->reg_state check in ndo_unint
ndo_unint is supposed to free the memory alloced in ndo_init.

>
> Regards
> Feng
>
>

RE: [PATCH net v3] driver: veth: Fix one possbile memleak when fail to register_netdevice

[Gao Feng]

> From: Xin Long [mailto:lucien.xin@gmail.com]
> Sent: Wednesday, May 3, 2017 1:38 PM
> On Wed, May 3, 2017 at 10:07 AM, Gao Feng <gfree.wind@foxmail.com>
> wrote:
> >> From: netdev-owner@vger.kernel.org
> >> [mailto:netdev-owner@vger.kernel.org]
> >> On Behalf Of Xin Long
> >> Sent: Wednesday, May 3, 2017 12:59 AM On Tue, May 2, 2017 at 7:03 PM,
> >> Gao Feng <gfree.wind@vip.163.com> wrote:
> >> >> From: Xin Long [mailto:lucien.xin@gmail.com]
> >> >> Sent: Tuesday, May 2, 2017 3:56 PM On Sat, Apr 29, 2017 at 11:51
> >> >> AM,  <gfree.wind@foxmail.com> wrote:
> >> >> > From: Gao Feng <gfree.wind@foxmail.com>
[...]
> > The fix you mentioned change the original logic.
> > The dev->vstats is freed in advance in the ndo_uninit, not destructor.
> > It may break the backward.
> Sorry, I didn't get your "backward"
> I can't see there will be any problem caused by it.
> can you say this patch also break the 'backward' ?
> https://patchwork.ozlabs.org/patch/748964/
> 
> It's really weird to do dev->reg_state check in ndo_unint ndo_unint is supposed
> to free the memory alloced in ndo_init.
> 

I am not sure if it would break the backward, so I said it MAY break.
I assumed there may be someone would access the dev->vstats after ndo_uninit,
because current veth driver free the mem in the destructor.
I selected this approach because I don't want to bring new bugs during fix bug.

If you're sure it is safe to free dev->vstats in ndo_uninit, I would like to update it.

BTW there are too many drivers which have possible memleak.
You could find the list by https://www.mail-archive.com/netdev@vger.kernel.org/msg166629.html.

Some drivers allocate the resources in ndo_init, free some in ndo_uninit and free left in destructor.
I think there are some reasons. 
We could not move all free in the ndo_uninit from destructor. What's your opinion?

Best Regards
Feng

Re: [PATCH net v3] driver: veth: Fix one possbile memleak when fail to register_netdevice

[Xin Long]

On Wed, May 3, 2017 at 2:37 PM, Gao Feng <gfree.wind@foxmail.com> wrote:
>> From: Xin Long [mailto:lucien.xin@gmail.com]
>> Sent: Wednesday, May 3, 2017 1:38 PM
>> On Wed, May 3, 2017 at 10:07 AM, Gao Feng <gfree.wind@foxmail.com>
>> wrote:
>> >> From: netdev-owner@vger.kernel.org
>> >> [mailto:netdev-owner@vger.kernel.org]
>> >> On Behalf Of Xin Long
>> >> Sent: Wednesday, May 3, 2017 12:59 AM On Tue, May 2, 2017 at 7:03 PM,
>> >> Gao Feng <gfree.wind@vip.163.com> wrote:
>> >> >> From: Xin Long [mailto:lucien.xin@gmail.com]
>> >> >> Sent: Tuesday, May 2, 2017 3:56 PM On Sat, Apr 29, 2017 at 11:51
>> >> >> AM,  <gfree.wind@foxmail.com> wrote:
>> >> >> > From: Gao Feng <gfree.wind@foxmail.com>
> [...]
>> > The fix you mentioned change the original logic.
>> > The dev->vstats is freed in advance in the ndo_uninit, not destructor.
>> > It may break the backward.
>> Sorry, I didn't get your "backward"
>> I can't see there will be any problem caused by it.
>> can you say this patch also break the 'backward' ?
>> https://patchwork.ozlabs.org/patch/748964/
>>
>> It's really weird to do dev->reg_state check in ndo_unint ndo_unint is supposed
>> to free the memory alloced in ndo_init.
>>
>
> I am not sure if it would break the backward, so I said it MAY break.
> I assumed there may be someone would access the dev->vstats after ndo_uninit,
> because current veth driver free the mem in the destructor.
> I selected this approach because I don't want to bring new bugs during fix bug.
>
> If you're sure it is safe to free dev->vstats in ndo_uninit, I would like to update it.
yes, stats are accessed in .ndo_start_xmit waited by synchronize_net() and
.ndo_get_stats64 protected by rtnl_lock().


>
> BTW there are too many drivers which have possible memleak.
> You could find the list by https://www.mail-archive.com/netdev@vger.kernel.org/msg166629.html.
ah, cool.
I'm not sure about other dev's stuff, have to check them for sure later.

>
> Some drivers allocate the resources in ndo_init, free some in ndo_uninit and free left in destructor.
> I think there are some reasons.
> We could not move all free in the ndo_uninit from destructor. What's your opinion?
>
> Best Regards
> Feng
>
>
>

RE: [PATCH net v3] driver: veth: Fix one possbile memleak when fail to register_netdevice

[Gao Feng]

> From: Xin Long [mailto:lucien.xin@gmail.com]
> Sent: Wednesday, May 3, 2017 7:26 PM
> On Wed, May 3, 2017 at 2:37 PM, Gao Feng <gfree.wind@foxmail.com> wrote:
> >> From: Xin Long [mailto:lucien.xin@gmail.com]
> >> Sent: Wednesday, May 3, 2017 1:38 PM
> >> On Wed, May 3, 2017 at 10:07 AM, Gao Feng <gfree.wind@foxmail.com>
> >> wrote:
> >> >> From: netdev-owner@vger.kernel.org
> >> >> [mailto:netdev-owner@vger.kernel.org]
> >> >> On Behalf Of Xin Long
> >> >> Sent: Wednesday, May 3, 2017 12:59 AM On Tue, May 2, 2017 at 7:03
> >> >> PM, Gao Feng <gfree.wind@vip.163.com> wrote:
> >> >> >> From: Xin Long [mailto:lucien.xin@gmail.com]
> >> >> >> Sent: Tuesday, May 2, 2017 3:56 PM On Sat, Apr 29, 2017 at
> >> >> >> 11:51 AM,  <gfree.wind@foxmail.com> wrote:
> >> >> >> > From: Gao Feng <gfree.wind@foxmail.com>
> > [...]
> >> > The fix you mentioned change the original logic.
> >> > The dev->vstats is freed in advance in the ndo_uninit, not destructor.
> >> > It may break the backward.
> >> Sorry, I didn't get your "backward"
> >> I can't see there will be any problem caused by it.
> >> can you say this patch also break the 'backward' ?
> >> https://patchwork.ozlabs.org/patch/748964/
> >>
> >> It's really weird to do dev->reg_state check in ndo_unint ndo_unint
> >> is supposed to free the memory alloced in ndo_init.
> >>
> >
> > I am not sure if it would break the backward, so I said it MAY break.
> > I assumed there may be someone would access the dev->vstats after
> > ndo_uninit, because current veth driver free the mem in the destructor.
> > I selected this approach because I don't want to bring new bugs during fix
> bug.
> >
> > If you're sure it is safe to free dev->vstats in ndo_uninit, I would like to
> update it.
> yes, stats are accessed in .ndo_start_xmit waited by synchronize_net() and
> .ndo_get_stats64 protected by rtnl_lock().

Thanks, I would update the series later with your advice.
I need to wait for a while to collect more comments.
 
> >
> > BTW there are too many drivers which have possible memleak.
> > You could find the list by
> https://www.mail-archive.com/netdev@vger.kernel.org/msg166629.html.
> ah, cool.
> I'm not sure about other dev's stuff, have to check them for sure later.

Expect and thanks your reviews:)

Best Regards
Feng

> 
> >
> > Some drivers allocate the resources in ndo_init, free some in ndo_uninit and
> free left in destructor.
> > I think there are some reasons.
> > We could not move all free in the ndo_uninit from destructor. What's your
> opinion?
> >
> > Best Regards
> > Feng
> >
> >
> >