From: Gao Feng <redacted>
These following drivers allocate kinds of resources in its ndo_init
func, free some of them or all in the destructor func. Then there is
one memleak that some errors happen after register_netdevice invokes
the ndo_init callback. Because only the ndo_uninit callback is invoked
in the error handler of register_netdevice, but destructor not.
In my original approach, I tried to free the resources in the newlink
func when fail to register_netdevice, like destructor did except not
free the net_dev. This method is not good when destructor is changed,
and the memleak could be not fixed when there is no newlink callback.
Now create one new func used to free the resources in the destructor,
and the ndo_uninit func also could invokes it when fail to register
the net_device by comparing the dev->reg_state with NETREG_UNINITIALIZED.
If there is no existing ndo_uninit, just add one.
This solution doesn't only make sure free all resources in any case,
but also follows the original desgin that some resources could be kept
until the destructor executes normally after register the device
successfully.
Gao Feng (12):
driver: dummy: Fix one possbile memleak when fail to
register_netdevice
driver: ifb: Fix one possbile memleak when fail to register_netdevice
driver: loopback: Fix one possbile memleak when fail to
register_netdevice
driver: team: Fix one possbile memleak when fail to register_netdevice
driver: veth: Fix one possbile memleak when fail to register_netdevice
net: ip6_gre: Fix one possbile memleak when fail to register_netdevice
ip6_tunnel: Fix one possbile memleak when fail to register_netdevice
net: ip6_vti: Fix one possbile memleak when fail to register_netdevice
net: ip_tunnel: Fix one possbile memleak when fail to
register_netdevice
net: sit: Fix one possbile memleak when fail to register_netdevice
net: vlan: Fix one possbile memleak when fail to register_netdevice
net: batman-adv: Fix one possbile memleak when fail to
register_netdevice
drivers/net/dummy.c | 14 +++++++++++---
drivers/net/ifb.c | 33 +++++++++++++++++++++++----------
drivers/net/loopback.c | 15 ++++++++++++++-
drivers/net/team/team.c | 15 ++++++++++++---
drivers/net/veth.c | 15 ++++++++++++++-
net/8021q/vlan_dev.c | 17 +++++++++++++----
net/batman-adv/soft-interface.c | 18 +++++++++++++++---
net/ipv4/ip_tunnel.c | 11 ++++++++++-
net/ipv6/ip6_gre.c | 17 +++++++++++++----
net/ipv6/ip6_tunnel.c | 11 ++++++++++-
net/ipv6/ip6_vti.c | 11 ++++++++++-
net/ipv6/sit.c | 17 +++++++++++++----
12 files changed, 158 insertions(+), 36 deletions(-)
From: Gao Feng <redacted>
The dummy driver allocates dev->dstats and priv->vfinfo in its
ndo_init func dummy_dev_init, free the dev->dstats in the ndo_uninit
and free the priv->vfinfo in its destructor func. Then there is one
memleak that some errors happen after register_netdevice invokes the
ndo_init callback. Because only the ndo_uninit callback is invoked in
the error handler of register_netdevice, but destructor not.
Now create one new func dummy_destructor_free to free the mem in the
destructor, and the ndo_uninit func also invokes it when fail to
register the dummy device.
It's not only free all resources, but also follow the original desgin
that the priv->vfinfo is freed in the destructor normally after
register the device successfully.
Signed-off-by: Gao Feng <redacted>
---
drivers/net/dummy.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
@@ -153,9 +153,19 @@ static int dummy_dev_init(struct net_device *dev)return0;}+staticvoiddummy_destructor_free(structnet_device*dev)+{+structdummy_priv*priv=netdev_priv(dev);++kfree(priv->vfinfo);+}+staticvoiddummy_dev_uninit(structnet_device*dev){free_percpu(dev->dstats);+/* dev is not registered, perform the free instead of destructor */+if(dev->reg_state==NETREG_UNINITIALIZED)+dummy_destructor_free(dev);}staticintdummy_change_carrier(structnet_device*dev,boolnew_carrier)
From: Gao Feng <redacted>
The ip6_gre allocates some resources in its ndo_init func, and
free some of them in its destructor func. Then there is one memleak
that some errors happen after register_netdevice invokes the ndo_init
callback. Because only the ndo_uninit callback is invoked in the error
handler of register_netdevice, but destructor not.
Now create one new func ip6_gre_destructor_free to free the mem in
the destructor, and ndo_uninit func also invokes it when fail to
register the ip6_gre 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 <redacted>
---
net/ipv6/ip6_gre.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
@@ -355,6 +355,14 @@ static struct ip6_tnl *ip6gre_tunnel_locate(struct net *net,returnNULL;}+staticvoidip6gre_destructor_free(structnet_device*dev)+{+structip6_tnl*t=netdev_priv(dev);++dst_cache_destroy(&t->dst_cache);+free_percpu(dev->tstats);+}+staticvoidip6gre_tunnel_uninit(structnet_device*dev){structip6_tnl*t=netdev_priv(dev);
@@ -363,6 +371,10 @@ static void ip6gre_tunnel_uninit(struct net_device *dev)ip6gre_tunnel_unlink(ign,t);dst_cache_reset(&t->dst_cache);dev_put(dev);++/* dev is not registered, perform the free instead of destructor */+if(dev->reg_state==NETREG_UNINITIALIZED)+ip6gre_destructor_free(dev);}
From: Gao Feng <redacted>
The ifb 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 ifb_destructor_free to free the mem in the
destructor, and add ndo_uninit func also invokes it when fail to register
the ifb 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 <redacted>
---
drivers/net/ifb.c | 33 +++++++++++++++++++++++----------
1 file changed, 23 insertions(+), 10 deletions(-)
@@ -180,6 +180,27 @@ static int ifb_dev_init(struct net_device *dev)return0;}+staticvoidifb_destructor_free(structnet_device*dev)+{+structifb_dev_private*dp=netdev_priv(dev);+structifb_q_private*txp=dp->tx_private;+inti;++for(i=0;i<dev->num_tx_queues;i++,txp++){+tasklet_kill(&txp->ifb_tasklet);+__skb_queue_purge(&txp->rq);+__skb_queue_purge(&txp->tq);+}+kfree(dp->tx_private);+}++staticvoidifb_dev_uninit(structnet_device*dev)+{+/* dev is not registered, perform the free instead of destructor */+if(dev->reg_state==NETREG_UNINITIALIZED)+ifb_destructor_free(dev);+}+staticconststructnet_device_opsifb_netdev_ops={.ndo_open=ifb_open,.ndo_stop=ifb_close,
From: Gao Feng <redacted>
The team driver allocates some resources in its ndo_init func, and
free some of them in its destructor func. Then there is one memleak
that some errors happen after register_netdevice invokes the ndo_init
callback. Because only the ndo_uninit callback is invoked in the error
handler of register_netdevice, but destructor not.
Now create one new func team_destructor_free to free the mem in the
destructor, and ndo_uninit func also invokes it when fail to register
the team 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 <redacted>
---
drivers/net/team/team.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
@@ -1619,6 +1619,13 @@ static int team_init(struct net_device *dev)returnerr;}+staticvoidteam_destructor_free(structnet_device*dev)+{+structteam*team=netdev_priv(dev);++free_percpu(team->pcpu_stats);+}+staticvoidteam_uninit(structnet_device*dev){structteam*team=netdev_priv(dev);
@@ -1636,13 +1643,15 @@ static void team_uninit(struct net_device *dev)team_queue_override_fini(team);mutex_unlock(&team->lock);netdev_change_features(dev);++/* dev is not registered, perform the free instead of destructor */+if(dev->reg_state==NETREG_UNINITIALIZED)+team_destructor_free(dev);}staticvoidteam_destructor(structnet_device*dev){-structteam*team=netdev_priv(dev);--free_percpu(team->pcpu_stats);+team_destructor_free(dev);free_netdev(dev);}
From: Gao Feng <redacted>
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 <redacted>
---
drivers/net/veth.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
@@ -224,9 +224,21 @@ static int veth_dev_init(struct net_device *dev)return0;}-staticvoidveth_dev_free(structnet_device*dev)+staticvoidveth_destructor_free(structnet_device*dev){free_percpu(dev->vstats);+}++staticvoidveth_dev_uninit(structnet_device*dev)+{+/* dev is not registered, perform the free instead of destructor */+if(dev->reg_state==NETREG_UNINITIALIZED)+veth_destructor_free(dev);+}++staticvoidveth_dev_free(structnet_device*dev)+{+veth_destructor_free(dev);free_netdev(dev);}
From: Gao Feng <redacted>
The loopback 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 loopback_destructor_free to free the mem in
the destructor, and add ndo_uninit func also invokes it when fail to
register the loopback 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 <redacted>
---
drivers/net/loopback.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
@@ -141,15 +141,28 @@ static int loopback_dev_init(struct net_device *dev)return0;}-staticvoidloopback_dev_free(structnet_device*dev)+staticvoidloopback_destructor_free(structnet_device*dev){dev_net(dev)->loopback_dev=NULL;free_percpu(dev->lstats);+}++staticvoidloopback_dev_uninit(structnet_device*dev)+{+/* dev is not registered, perform the free instead of destructor */+if(dev->reg_state==NETREG_UNINITIALIZED)+loopback_destructor_free(dev);+}++staticvoidloopback_dev_free(structnet_device*dev)+{+loopback_destructor_free(dev);free_netdev(dev);}staticconststructnet_device_opsloopback_ops={.ndo_init=loopback_dev_init,+.ndo_uninit=loopback_dev_uninit,.ndo_start_xmit=loopback_xmit,.ndo_get_stats64=loopback_get_stats64,.ndo_set_mac_address=eth_mac_addr,
From: Gao Feng <redacted>
The ip6_tunnel allocates some resources in its ndo_init func, and
free some of them in its destructor func. Then there is one memleak
that some errors happen after register_netdevice invokes the ndo_init
callback. Because only the ndo_uninit callback is invoked in the error
handler of register_netdevice, but destructor not.
Now create one new func ip6_tnl_destructor_free to free the mem in
the destructor, and ndo_uninit func also invokes it when fail to
register the ip6_tunnel 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 <redacted>
---
net/ipv6/ip6_tunnel.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
@@ -387,6 +392,10 @@ static struct ip6_tnl *ip6_tnl_locate(struct net *net,ip6_tnl_unlink(ip6n,t);dst_cache_reset(&t->dst_cache);dev_put(dev);++/* dev is not registered, perform the free instead of destructor */+if(dev->reg_state==NETREG_UNINITIALIZED)+ip6_tnl_destructor_free(dev);}/**
From: gfree.wind@foxmail.com [mailto:gfree.wind@foxmail.com]
Sent: Tuesday, May 2, 2017 1:59 PM
drivers/net/dummy.c | 14 +++++++++++---
drivers/net/ifb.c | 33 +++++++++++++++++++++++----------
drivers/net/loopback.c | 15 ++++++++++++++-
drivers/net/team/team.c | 15 ++++++++++++---
drivers/net/veth.c | 15 ++++++++++++++-
net/8021q/vlan_dev.c | 17 +++++++++++++----
net/batman-adv/soft-interface.c | 18 +++++++++++++++---
net/ipv4/ip_tunnel.c | 11 ++++++++++-
net/ipv6/ip6_gre.c | 17 +++++++++++++----
net/ipv6/ip6_tunnel.c | 11 ++++++++++-
net/ipv6/ip6_vti.c | 11 ++++++++++-
net/ipv6/sit.c | 17 +++++++++++++----
12 files changed, 158 insertions(+), 36 deletions(-)
--
v4: Make patches as one sery of patches, per David Miler
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
Because I sent these patches too fast today, so that my email server blocks
my account today.
The left patches (08~12) would be sent after my account is unlocked.
Maybe tomorrow.
I am very sorry about this, and try to change another mail server next time.
Best Regards
Feng
From: Gao Feng <redacted>
The ip6_vti allocates some resources in its ndo_init func, and
free some of them in its destructor func. Then there is one memleak
that some errors happen after register_netdevice invokes the ndo_init
callback. Because only the ndo_uninit callback is invoked in the error
handler of register_netdevice, but destructor not.
Now create one new func vti6_destructor_free to free the mem in
the destructor, and ndo_uninit func also invokes it when fail to
register the vti6 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 <redacted>
---
net/ipv6/ip6_vti.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
@@ -296,6 +301,10 @@ static void vti6_dev_uninit(struct net_device *dev)elsevti6_tnl_unlink(ip6n,t);dev_put(dev);++/* dev is not registered, perform the free instead of destructor */+if(dev->reg_state==NETREG_UNINITIALIZED)+vti6_destructor_free(dev);}staticintvti6_rcv(structsk_buff*skb)
From: Gao Feng <redacted>
The ip_tunnel allocates some resources in its ndo_init func, and
free some of them in its destructor func. Then there is one memleak
that some errors happen after register_netdevice invokes the ndo_init
callback. Because only the ndo_uninit callback is invoked in the error
handler of register_netdevice, but destructor not.
Now create one new func ip_tunnel_destructor_free to free the mem in
the destructor, and ndo_uninit func also invokes it when fail to
register the ip_tunnel 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 <redacted>
---
net/ipv4/ip_tunnel.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
@@ -954,13 +954,18 @@ int ip_tunnel_change_mtu(struct net_device *dev, int new_mtu)}EXPORT_SYMBOL_GPL(ip_tunnel_change_mtu);-staticvoidip_tunnel_dev_free(structnet_device*dev)+staticvoidip_tunnel_destructor_free(structnet_device*dev){structip_tunnel*tunnel=netdev_priv(dev);gro_cells_destroy(&tunnel->gro_cells);dst_cache_destroy(&tunnel->dst_cache);free_percpu(dev->tstats);+}++staticvoidip_tunnel_dev_free(structnet_device*dev)+{+ip_tunnel_destructor_free(dev);free_netdev(dev);}
@@ -1192,6 +1197,10 @@ void ip_tunnel_uninit(struct net_device *dev)ip_tunnel_del(itn,netdev_priv(dev));dst_cache_reset(&tunnel->dst_cache);++/* dev is not registered, perform the free instead of destructor */+if(dev->reg_state==NETREG_UNINITIALIZED)+ip_tunnel_destructor_free(dev);}EXPORT_SYMBOL_GPL(ip_tunnel_uninit);
From: Gao Feng <redacted>
The ipip6 allocates some resources in its ndo_init func, and
free some of them in its destructor func. Then there is one memleak
that some errors happen after register_netdevice invokes the ndo_init
callback. Because only the ndo_uninit callback is invoked in the error
handler of register_netdevice, but destructor not.
Now create one new func ipip6_destructor_free to free the mem in
the destructor, and ndo_uninit func also invokes it when fail to
register the ipip6 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 <redacted>
---
net/ipv6/sit.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
@@ -477,6 +485,10 @@ static void ipip6_tunnel_uninit(struct net_device *dev)}dst_cache_reset(&tunnel->dst_cache);dev_put(dev);++/* dev is not registered, perform the free instead of destructor */+if(dev->reg_state==NETREG_UNINITIALIZED)+ipip6_destructor_free(dev);}staticintipip6_err(structsk_buff*skb,u32info)
From: Gao Feng <redacted>
The vlan driver allocates some resources in its ndo_init func, and
free some of them in its destructor func. Then there is one memleak
that some errors happen after register_netdevice invokes the ndo_init
callback. Because only the ndo_uninit callback is invoked in the error
handler of register_netdevice, but destructor not.
Now create one new func vlan_destructor_free to free the mem in the
destructor, and ndo_uninit func also invokes it when fail to register
the vlan 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 <redacted>
---
net/8021q/vlan_dev.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
@@ -608,6 +608,14 @@ static int vlan_dev_init(struct net_device *dev)return0;}+staticvoidvlan_destructor_free(structnet_device*dev)+{+structvlan_dev_priv*vlan=vlan_dev_priv(dev);++free_percpu(vlan->vlan_pcpu_stats);+vlan->vlan_pcpu_stats=NULL;+}+staticvoidvlan_dev_uninit(structnet_device*dev){structvlan_priority_tci_mapping*pm;
@@ -620,6 +628,10 @@ static void vlan_dev_uninit(struct net_device *dev)kfree(pm);}}++/* dev is not registered, perform the free instead of destructor */+if(dev->reg_state==NETREG_UNINITIALIZED)+vlan_destructor_free(dev);}staticnetdev_features_tvlan_dev_fix_features(structnet_device*dev,
@@ -803,10 +815,7 @@ static int vlan_dev_get_iflink(const struct net_device *dev)staticvoidvlan_dev_free(structnet_device*dev){-structvlan_dev_priv*vlan=vlan_dev_priv(dev);--free_percpu(vlan->vlan_pcpu_stats);-vlan->vlan_pcpu_stats=NULL;+vlan_destructor_free(dev);free_netdev(dev);}
From: Gao Feng <redacted>
The batman-adv 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 batadv_destructor_free to free the mem in
the destructor, and add ndo_uninit func also invokes it when fail to
register the batman-adv 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 <redacted>
---
net/batman-adv/soft-interface.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
@@ -878,6 +878,19 @@ static int batadv_softif_init_late(struct net_device *dev)returnret;}+staticvoidbatadv_destructor_free(structnet_device*dev)+{+batadv_debugfs_del_meshif(dev);+batadv_mesh_free(dev);+}++staticvoidbatadv_softif_uninit(structnet_device*dev)+{+/* dev is not registered, perform the free instead of destructor */+if(dev->reg_state==NETREG_UNINITIALIZED)+batadv_destructor_free(dev);+}+/***batadv_softif_slave_add-Addaslaveinterfacetoabatadv_soft_interface*@dev:batadv_soft_interfaceusedasmasterinterface
@@ -933,6 +946,7 @@ static int batadv_softif_slave_del(struct net_device *dev,staticconststructnet_device_opsbatadv_netdev_ops={.ndo_init=batadv_softif_init_late,+.ndo_uninit=batadv_softif_uninit,.ndo_open=batadv_interface_open,.ndo_stop=batadv_interface_release,.ndo_get_stats=batadv_interface_stats,
@@ -953,9 +967,7 @@ static int batadv_softif_slave_del(struct net_device *dev,*/staticvoidbatadv_softif_free(structnet_device*dev){-batadv_debugfs_del_meshif(dev);-batadv_mesh_free(dev);-+batadv_destructor_free(dev);/* some scheduled RCU callbacks need the bat_priv struct to accomplish*theirtasks.Waitforthemalltobefinishedbeforefreeingthe*netdevanditsprivatedata(bat_priv)
From: David Miller <davem@davemloft.net> Date: 2017-05-02 19:30:32
From: gfree.wind@foxmail.com
Date: Tue, 2 May 2017 13:58:42 +0800
These following drivers allocate kinds of resources in its ndo_init
func, free some of them or all in the destructor func. Then there is
one memleak that some errors happen after register_netdevice invokes
the ndo_init callback. Because only the ndo_uninit callback is invoked
in the error handler of register_netdevice, but destructor not.
In my original approach, I tried to free the resources in the newlink
func when fail to register_netdevice, like destructor did except not
free the net_dev. This method is not good when destructor is changed,
and the memleak could be not fixed when there is no newlink callback.
Now create one new func used to free the resources in the destructor,
and the ndo_uninit func also could invokes it when fail to register
the net_device by comparing the dev->reg_state with NETREG_UNINITIALIZED.
If there is no existing ndo_uninit, just add one.
This solution doesn't only make sure free all resources in any case,
but also follows the original desgin that some resources could be kept
until the destructor executes normally after register the device
successfully.
I want to think about this some more.
It is really unfortunate that resources are allocated strictly from
the ndo_init() yet released in two different callbacks which are
invoked only in certain (different) situations.
Just the fact that we have to make an internal netdev state test
in the ndo_uninit callback to get this right is a big red flag
to me.
From: David Miller [mailto:davem@davemloft.net]
Sent: Wednesday, May 3, 2017 3:30 AM
From: gfree.wind@foxmail.com
Date: Tue, 2 May 2017 13:58:42 +0800
[...]
quoted
This solution doesn't only make sure free all resources in any case,
but also follows the original desgin that some resources could be kept
until the destructor executes normally after register the device
successfully.
I want to think about this some more.
It is really unfortunate that resources are allocated strictly from the
ndo_init()
yet released in two different callbacks which are invoked only in certain
(different) situations.
Just the fact that we have to make an internal netdev state test in the
ndo_uninit callback to get this right is a big red flag to me.
Yes, I am very agree with you.
This fix is just like a workaround under current framework.
The root is that allocate in one spot, but free them at two spots.
It means all ndo_uninit need to handle this case if allocate some resource
and free them in the destructor.
It should be done by the framework.
I thought about if there was a better solution to fix it.
But I think it need to modify the framework of net_device, it seems not good
as a bug fix to net.git.
Best Regards
Feng
From: David Miller <davem@davemloft.net> Date: 2017-05-07 22:25:55
From: gfree.wind@foxmail.com
Date: Tue, 2 May 2017 13:58:42 +0800
These following drivers allocate kinds of resources in its ndo_init
func, free some of them or all in the destructor func. Then there is
one memleak that some errors happen after register_netdevice invokes
the ndo_init callback. Because only the ndo_uninit callback is invoked
in the error handler of register_netdevice, but destructor not.
In my original approach, I tried to free the resources in the
newlink func when fail to register_netdevice, like destructor did
except not free the net_dev. This method is not good when destructor
is changed, and the memleak could be not fixed when there is no
newlink callback.
Now create one new func used to free the resources in the
destructor, and the ndo_uninit func also could invokes it when fail
to register the net_device by comparing the dev->reg_state with
NETREG_UNINITIALIZED. If there is no existing ndo_uninit, just add
one.
This solution doesn't only make sure free all resources in any case,
but also follows the original desgin that some resources could be
kept until the destructor executes normally after register the
device successfully.
Device private teardown is in two stages for the following reason.
The issue is that netdev_ops->ndo_init() allocates two types of
resources.
One type is OK to release during destruction before the netdev refs
goes to zero. This is what netdev_ops->ndo_uninit() is for.
The second type is for releasing things which are not safe to drop
until the very last netdev reference disappears. This is what
netdev->destructor() is for.
If you look around there are hacks in place all over to try and deal
with this issue. Basically, look for code that checks the return
value of register_netdev() and if an error is indicated it does
some local driver state freeing. Bonding is one example. It is
trying to deal with the problem this patch set is targetting.
What really needs to happen is we must divorce the logic of
dev->destructor() from free_netdev().
That way we can do the free_netdev in the unregister netdevice path
after calling netdev->destructor().
Then the only issue callers of register_netdevice() need to be aware
of is that if an error is returned, that caller must call
free_netdev(). Which has been the case for decades.
So I would fix this as follows:
1) Rename netdev->destructor() into "netdev->priv_destructor()" It
performs all post ndo_ops->ndo_uninit() cleanups, _except_
free_netdev(). Update all drivers with netdev->destructor().
2) Add a boolean state to netdev, which indicates if free_netdev()
should be performed after netdev->priv_destructor() during
unregister.
That provides all of the flexibility necessary to fix this bug in
the core.
In register_netdevice() if something after ndo_ops->ndo_init()
succeeeds, we invoke _both_ ndo_ops->ndo_uninit() and
netdev->priv_destructor(). We do not look at the netdev "needs
free_netdev()" boolean.
In netdev_run_todo(), where we have the one and only
netdev->destructor() call, change it to:
if (dev->priv_destructor)
dev->priv_destructor(dev);
if (dev->needs_free_netdev)
free_netdev(dev);
That fixes the bug in all cases. And makes the purpose and logic
extremely clear. Also, no internal state tests leak into the
drivers.
Finally, drivers that try to cover up this issue, such as bonding,
need to be changed to no try and free up device private state if
their invocation of register_netdevice() fails.
Thanks.
From: David Miller [mailto:davem@davemloft.net]
From: gfree.wind@foxmail.com
Date: Tue, 2 May 2017 13:58:42 +0800
quoted
These following drivers allocate kinds of resources in its ndo_init
func, free some of them or all in the destructor func. Then there is
one memleak that some errors happen after register_netdevice invokes
the ndo_init callback. Because only the ndo_uninit callback is invoked
in the error handler of register_netdevice, but destructor not.
In my original approach, I tried to free the resources in the newlink
func when fail to register_netdevice, like destructor did except not
free the net_dev. This method is not good when destructor is changed,
and the memleak could be not fixed when there is no newlink callback.
Now create one new func used to free the resources in the destructor,
and the ndo_uninit func also could invokes it when fail to register
the net_device by comparing the dev->reg_state with
NETREG_UNINITIALIZED. If there is no existing ndo_uninit, just add
one.
This solution doesn't only make sure free all resources in any case,
but also follows the original desgin that some resources could be kept
until the destructor executes normally after register the device
successfully.
Device private teardown is in two stages for the following reason.
The issue is that netdev_ops->ndo_init() allocates two types of resources.
One type is OK to release during destruction before the netdev refs goes
to
zero. This is what netdev_ops->ndo_uninit() is for.
The second type is for releasing things which are not safe to drop until
the very
last netdev reference disappears. This is what
netdev->destructor() is for.
If you look around there are hacks in place all over to try and deal with
this
issue. Basically, look for code that checks the return value of
register_netdev() and if an error is indicated it does some local driver
state
freeing. Bonding is one example. It is trying to deal with the problem
this
patch set is targetting.
Yes. When I was fixing this bug, I had found the bond had deal with this
issue, frees the resources by itself.
What really needs to happen is we must divorce the logic of
dev->destructor() from free_netdev().
That way we can do the free_netdev in the unregister netdevice path after
calling netdev->destructor().
Then the only issue callers of register_netdevice() need to be aware of is
that if
an error is returned, that caller must call free_netdev(). Which has been
the
case for decades.
So I would fix this as follows:
1) Rename netdev->destructor() into "netdev->priv_destructor()" It
performs all post ndo_ops->ndo_uninit() cleanups, _except_
free_netdev(). Update all drivers with netdev->destructor().
2) Add a boolean state to netdev, which indicates if free_netdev()
should be performed after netdev->priv_destructor() during
unregister.
That provides all of the flexibility necessary to fix this bug in the
core.
In register_netdevice() if something after ndo_ops->ndo_init() succeeeds,
we
invoke _both_ ndo_ops->ndo_uninit() and
netdev->priv_destructor(). We do not look at the netdev "needs
free_netdev()" boolean.
In netdev_run_todo(), where we have the one and only
netdev->destructor() call, change it to:
if (dev->priv_destructor)
dev->priv_destructor(dev);
if (dev->needs_free_netdev)
free_netdev(dev);
That fixes the bug in all cases. And makes the purpose and logic
extremely
clear. Also, no internal state tests leak into the drivers.
Finally, drivers that try to cover up this issue, such as bonding, need to
be
changed to no try and free up device private state if their invocation of
register_netdevice() fails.
Thanks.
Ok. It is better to fix it by changing the framework of net_dev.
I was afraid that I would bring other bugs if I changed it.
Because it would affect too many codes.
I would like to see you fix it by yourself.
Best Regards
Feng