All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net] driver/net: Fix possible memleaks when fail to register_netdevice
@ 2017-04-25 12:01 gfree.wind
  2017-04-25 12:51 ` Gao Feng
  2017-04-27  8:15 ` Herbert Xu
  0 siblings, 2 replies; 5+ messages in thread
From: gfree.wind @ 2017-04-25 12:01 UTC (permalink / raw)
  To: jiri, davem, kuznet, jmorris, yoshfuji, kaber, steffen.klassert,
	herbert, netdev
  Cc: Gao Feng

From: Gao Feng <fgao@ikuai8.com>

These drivers allocate kinds of resources in init routine, and free
some resources in the destructor of net_device. It may cause memleak
when some errors happen after register_netdevice invokes the init
callback. Because only the uninit callback is invoked in the error
handler of register_netdevice, but the destructor not. As a result,
some resources are lost forever.

Now invokes the destructor instead of free_netdev somewhere, and free
the left resources in the newlink func when fail to register_netdevice.

Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
 drivers/net/dummy.c      |  2 +-
 drivers/net/ifb.c        |  2 +-
 drivers/net/loopback.c   |  2 +-
 drivers/net/team/team.c  | 11 ++++++++++-
 drivers/net/veth.c       |  4 ++--
 net/8021q/vlan_netlink.c |  6 +++++-
 net/ipv4/ip_tunnel.c     |  9 ++++++++-
 net/ipv6/ip6_gre.c       |  6 +++++-
 net/ipv6/ip6_tunnel.c    | 12 ++++++++++--
 net/ipv6/ip6_vti.c       |  7 ++++++-
 net/ipv6/sit.c           |  5 ++++-
 11 files changed, 53 insertions(+), 13 deletions(-)

diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c
index 2c80611..55b8a50 100644
--- a/drivers/net/dummy.c
+++ b/drivers/net/dummy.c
@@ -383,7 +383,7 @@ static int __init dummy_init_one(void)
 	return 0;
 
 err:
-	free_netdev(dev_dummy);
+	dummy_free_netdev(dev_dummy);
 	return err;
 }
 
diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index 312fce7..a298371 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -318,7 +318,7 @@ static int __init ifb_init_one(int index)
 	return 0;
 
 err:
-	free_netdev(dev_ifb);
+	ifb_dev_free(dev_ifb);
 	return err;
 }
 
diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index b23b719..c4e1d4c 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -208,7 +208,7 @@ static __net_init int loopback_net_init(struct net *net)
 
 
 out_free_netdev:
-	free_netdev(dev);
+	loopback_dev_free(dev);
 out:
 	if (net_eq(net, &init_net))
 		panic("loopback: Failed to register netdevice: %d\n", err);
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index f8c81f1..0bc80fb 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2109,10 +2109,19 @@ static void team_setup(struct net_device *dev)
 static int team_newlink(struct net *src_net, struct net_device *dev,
 			struct nlattr *tb[], struct nlattr *data[])
 {
+	int ret;
+
 	if (tb[IFLA_ADDRESS] == NULL)
 		eth_hw_addr_random(dev);
 
-	return register_netdevice(dev);
+	ret = register_netdevice(dev);
+	if (ret) {
+		struct team *team = netdev_priv(dev);
+
+		free_percpu(team->pcpu_stats);
+	}
+
+	return ret;
 }
 
 static int team_validate(struct nlattr *tb[], struct nlattr *data[])
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 8c39d6d..f60f5ee 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -457,13 +457,13 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 	return 0;
 
 err_register_dev:
-	/* nothing to do */
+	free_percpu(dev->vstats);
 err_configure_peer:
 	unregister_netdevice(peer);
 	return err;
 
 err_register_peer:
-	free_netdev(peer);
+	veth_dev_free(peer);
 	return err;
 }
 
diff --git a/net/8021q/vlan_netlink.c b/net/8021q/vlan_netlink.c
index 1270207..a15826a 100644
--- a/net/8021q/vlan_netlink.c
+++ b/net/8021q/vlan_netlink.c
@@ -156,7 +156,11 @@ static int vlan_newlink(struct net *src_net, struct net_device *dev,
 	if (err < 0)
 		return err;
 
-	return register_vlan_dev(dev);
+	err = register_vlan_dev(dev);
+	if (err)
+		free_percpu(vlan->vlan_pcpu_stats);
+
+	return err;
 }
 
 static inline size_t vlan_qos_map_size(unsigned int n)
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 823abae..4acb296 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -63,6 +63,8 @@
 #include <net/ip6_route.h>
 #endif
 
+static void ip_tunnel_dev_free(struct net_device *dev);
+
 static unsigned int ip_tunnel_hash(__be32 key, __be32 remote)
 {
 	return hash_32((__force u32)key ^ (__force u32)remote,
@@ -285,7 +287,7 @@ static struct net_device *__ip_tunnel_create(struct net *net,
 	return dev;
 
 failed_free:
-	free_netdev(dev);
+	ip_tunnel_dev_free(dev);
 failed:
 	return ERR_PTR(err);
 }
@@ -1099,7 +1101,12 @@ int ip_tunnel_newlink(struct net_device *dev, struct nlattr *tb[],
 		dev->mtu = mtu;
 
 	ip_tunnel_add(itn, nt);
+
+	return 0;
 out:
+	gro_cells_destroy(&nt->gro_cells);
+	dst_cache_destroy(&nt->dst_cache);
+	free_percpu(dev->tstats);
 	return err;
 }
 EXPORT_SYMBOL_GPL(ip_tunnel_newlink);
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 6fcb7cb..d409ad1 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -77,6 +77,7 @@ struct ip6gre_net {
 static void ip6gre_tunnel_setup(struct net_device *dev);
 static void ip6gre_tunnel_link(struct ip6gre_net *ign, struct ip6_tnl *t);
 static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu);
+static void ip6gre_dev_free(struct net_device *dev);
 
 /* Tunnel hash table */
 
@@ -351,7 +352,7 @@ static struct ip6_tnl *ip6gre_tunnel_locate(struct net *net,
 	return nt;
 
 failed_free:
-	free_netdev(dev);
+	ip6gre_dev_free(dev);
 	return NULL;
 }
 
@@ -1388,7 +1389,10 @@ static int ip6gre_newlink(struct net *src_net, struct net_device *dev,
 	dev_hold(dev);
 	ip6gre_tunnel_link(ign, nt);
 
+	return 0;
 out:
+	dst_cache_destroy(&nt->dst_cache);
+	free_percpu(dev->tstats);
 	return err;
 }
 
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 75fac93..95f512c 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1960,11 +1960,12 @@ static int ip6_tnl_newlink(struct net *src_net, struct net_device *dev,
 	struct ip6_tnl_net *ip6n = net_generic(net, ip6_tnl_net_id);
 	struct ip6_tnl *nt, *t;
 	struct ip_tunnel_encap ipencap;
+	int err;
 
 	nt = netdev_priv(dev);
 
 	if (ip6_tnl_netlink_encap_parms(data, &ipencap)) {
-		int err = ip6_tnl_encap_setup(nt, &ipencap);
+		err = ip6_tnl_encap_setup(nt, &ipencap);
 
 		if (err < 0)
 			return err;
@@ -1981,7 +1982,14 @@ static int ip6_tnl_newlink(struct net *src_net, struct net_device *dev,
 			return -EEXIST;
 	}
 
-	return ip6_tnl_create2(dev);
+	err = ip6_tnl_create2(dev);
+	if (err) {
+		gro_cells_destroy(&t->gro_cells);
+		dst_cache_destroy(&t->dst_cache);
+		free_percpu(dev->tstats);
+	}
+
+	return err;
 }
 
 static int ip6_tnl_changelink(struct net_device *dev, struct nlattr *tb[],
diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c
index 3d8a3b6..b201eef 100644
--- a/net/ipv6/ip6_vti.c
+++ b/net/ipv6/ip6_vti.c
@@ -940,6 +940,7 @@ static int vti6_newlink(struct net *src_net, struct net_device *dev,
 {
 	struct net *net = dev_net(dev);
 	struct ip6_tnl *nt;
+	int ret;
 
 	nt = netdev_priv(dev);
 	vti6_netlink_parms(data, &nt->parms);
@@ -949,7 +950,11 @@ static int vti6_newlink(struct net *src_net, struct net_device *dev,
 	if (vti6_locate(net, &nt->parms, 0))
 		return -EEXIST;
 
-	return vti6_tnl_create2(dev);
+	ret = vti6_tnl_create2(dev);
+	if (ret)
+		free_percpu(dev->tstats);
+
+	return ret;
 }
 
 static void vti6_dellink(struct net_device *dev, struct list_head *head)
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index 99853c6..f45dc4a 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -1555,8 +1555,11 @@ static int ipip6_newlink(struct net *src_net, struct net_device *dev,
 		return -EEXIST;
 
 	err = ipip6_tunnel_create(dev);
-	if (err < 0)
+	if (err < 0) {
+		dst_cache_destroy(&nt->dst_cache);
+		free_percpu(dev->tstats);
 		return err;
+	}
 
 #ifdef CONFIG_IPV6_SIT_6RD
 	if (ipip6_netlink_6rd_parms(data, &ip6rd))
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* RE: [PATCH net] driver/net: Fix possible memleaks when fail to register_netdevice
  2017-04-25 12:01 [PATCH net] driver/net: Fix possible memleaks when fail to register_netdevice gfree.wind
@ 2017-04-25 12:51 ` Gao Feng
  2017-04-27  8:15 ` Herbert Xu
  1 sibling, 0 replies; 5+ messages in thread
From: Gao Feng @ 2017-04-25 12:51 UTC (permalink / raw)
  To: gfree.wind, jiri, davem, kuznet, jmorris, yoshfuji, kaber,
	steffen.klassert, herbert, netdev

> From: Gao Feng <fgao@ikuai8.com>
> 
> These drivers allocate kinds of resources in init routine, and free some
> resources in the destructor of net_device. It may cause memleak when some
> errors happen after register_netdevice invokes the init callback. Because
only
> the uninit callback is invoked in the error handler of register_netdevice,
but the
> destructor not. As a result, some resources are lost forever.
> 
> Now invokes the destructor instead of free_netdev somewhere, and free the
> left resources in the newlink func when fail to register_netdevice.
> 
> Signed-off-by: Gao Feng <fgao@ikuai8.com>
> ---
>  drivers/net/dummy.c      |  2 +-
>  drivers/net/ifb.c        |  2 +-
>  drivers/net/loopback.c   |  2 +-
>  drivers/net/team/team.c  | 11 ++++++++++-
>  drivers/net/veth.c       |  4 ++--
>  net/8021q/vlan_netlink.c |  6 +++++-
>  net/ipv4/ip_tunnel.c     |  9 ++++++++-
>  net/ipv6/ip6_gre.c       |  6 +++++-
>  net/ipv6/ip6_tunnel.c    | 12 ++++++++++--
>  net/ipv6/ip6_vti.c       |  7 ++++++-
>  net/ipv6/sit.c           |  5 ++++-
>  11 files changed, 53 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c index
> 2c80611..55b8a50 100644
> --- a/drivers/net/dummy.c
> +++ b/drivers/net/dummy.c
> @@ -383,7 +383,7 @@ static int __init dummy_init_one(void)
>  	return 0;
> 
>  err:
> -	free_netdev(dev_dummy);
> +	dummy_free_netdev(dev_dummy);
>  	return err;
>  }
> 
> diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c index 312fce7..a298371
100644
> --- a/drivers/net/ifb.c
> +++ b/drivers/net/ifb.c
> @@ -318,7 +318,7 @@ static int __init ifb_init_one(int index)
>  	return 0;
> 
>  err:
> -	free_netdev(dev_ifb);
> +	ifb_dev_free(dev_ifb);
>  	return err;
>  }
> 
> diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c index
> b23b719..c4e1d4c 100644
> --- a/drivers/net/loopback.c
> +++ b/drivers/net/loopback.c
> @@ -208,7 +208,7 @@ static __net_init int loopback_net_init(struct net
*net)
> 
> 
>  out_free_netdev:
> -	free_netdev(dev);
> +	loopback_dev_free(dev);
>  out:
>  	if (net_eq(net, &init_net))
>  		panic("loopback: Failed to register netdevice: %d\n", err);
diff --git
> a/drivers/net/team/team.c b/drivers/net/team/team.c index f8c81f1..0bc80fb
> 100644
> --- a/drivers/net/team/team.c
> +++ b/drivers/net/team/team.c
> @@ -2109,10 +2109,19 @@ static void team_setup(struct net_device *dev)
> static int team_newlink(struct net *src_net, struct net_device *dev,
>  			struct nlattr *tb[], struct nlattr *data[])  {
> +	int ret;
> +
>  	if (tb[IFLA_ADDRESS] == NULL)
>  		eth_hw_addr_random(dev);
> 
> -	return register_netdevice(dev);
> +	ret = register_netdevice(dev);
> +	if (ret) {
> +		struct team *team = netdev_priv(dev);
> +
> +		free_percpu(team->pcpu_stats);
> +	}
> +
> +	return ret;
>  }
> 
>  static int team_validate(struct nlattr *tb[], struct nlattr *data[]) diff
--git
> a/drivers/net/veth.c b/drivers/net/veth.c index 8c39d6d..f60f5ee 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -457,13 +457,13 @@ static int veth_newlink(struct net *src_net, struct
> net_device *dev,
>  	return 0;
> 
>  err_register_dev:
> -	/* nothing to do */
> +	free_percpu(dev->vstats);
>  err_configure_peer:
>  	unregister_netdevice(peer);
>  	return err;
> 
>  err_register_peer:
> -	free_netdev(peer);
> +	veth_dev_free(peer);
>  	return err;
>  }
> 
> diff --git a/net/8021q/vlan_netlink.c b/net/8021q/vlan_netlink.c index
> 1270207..a15826a 100644
> --- a/net/8021q/vlan_netlink.c
> +++ b/net/8021q/vlan_netlink.c
> @@ -156,7 +156,11 @@ static int vlan_newlink(struct net *src_net, struct
> net_device *dev,
>  	if (err < 0)
>  		return err;
> 
> -	return register_vlan_dev(dev);
> +	err = register_vlan_dev(dev);
> +	if (err)
> +		free_percpu(vlan->vlan_pcpu_stats);
> +
> +	return err;
>  }
> 
>  static inline size_t vlan_qos_map_size(unsigned int n) diff --git
> a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c index 823abae..4acb296
> 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -63,6 +63,8 @@
>  #include <net/ip6_route.h>
>  #endif
> 
> +static void ip_tunnel_dev_free(struct net_device *dev);
> +
>  static unsigned int ip_tunnel_hash(__be32 key, __be32 remote)  {
>  	return hash_32((__force u32)key ^ (__force u32)remote, @@ -285,7
> +287,7 @@ static struct net_device *__ip_tunnel_create(struct net *net,
>  	return dev;
> 
>  failed_free:
> -	free_netdev(dev);
> +	ip_tunnel_dev_free(dev);
>  failed:
>  	return ERR_PTR(err);
>  }
> @@ -1099,7 +1101,12 @@ int ip_tunnel_newlink(struct net_device *dev,
> struct nlattr *tb[],
>  		dev->mtu = mtu;
> 
>  	ip_tunnel_add(itn, nt);
> +
> +	return 0;
>  out:
> +	gro_cells_destroy(&nt->gro_cells);
> +	dst_cache_destroy(&nt->dst_cache);
> +	free_percpu(dev->tstats);
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(ip_tunnel_newlink);
> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c index
6fcb7cb..d409ad1
> 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -77,6 +77,7 @@ struct ip6gre_net {
>  static void ip6gre_tunnel_setup(struct net_device *dev);  static void
> ip6gre_tunnel_link(struct ip6gre_net *ign, struct ip6_tnl *t);  static
void
> ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu);
> +static void ip6gre_dev_free(struct net_device *dev);
> 
>  /* Tunnel hash table */
> 
> @@ -351,7 +352,7 @@ static struct ip6_tnl *ip6gre_tunnel_locate(struct net
> *net,
>  	return nt;
> 
>  failed_free:
> -	free_netdev(dev);
> +	ip6gre_dev_free(dev);
>  	return NULL;
>  }
> 
> @@ -1388,7 +1389,10 @@ static int ip6gre_newlink(struct net *src_net,
struct
> net_device *dev,
>  	dev_hold(dev);
>  	ip6gre_tunnel_link(ign, nt);
> 
> +	return 0;
>  out:
> +	dst_cache_destroy(&nt->dst_cache);
> +	free_percpu(dev->tstats);
>  	return err;
>  }
> 
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c index
> 75fac93..95f512c 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -1960,11 +1960,12 @@ static int ip6_tnl_newlink(struct net *src_net,
> struct net_device *dev,
>  	struct ip6_tnl_net *ip6n = net_generic(net, ip6_tnl_net_id);
>  	struct ip6_tnl *nt, *t;
>  	struct ip_tunnel_encap ipencap;
> +	int err;
> 
>  	nt = netdev_priv(dev);
> 
>  	if (ip6_tnl_netlink_encap_parms(data, &ipencap)) {
> -		int err = ip6_tnl_encap_setup(nt, &ipencap);
> +		err = ip6_tnl_encap_setup(nt, &ipencap);
> 
>  		if (err < 0)
>  			return err;
> @@ -1981,7 +1982,14 @@ static int ip6_tnl_newlink(struct net *src_net,
> struct net_device *dev,
>  			return -EEXIST;
>  	}
> 
> -	return ip6_tnl_create2(dev);
> +	err = ip6_tnl_create2(dev);
> +	if (err) {
> +		gro_cells_destroy(&t->gro_cells);
> +		dst_cache_destroy(&t->dst_cache);
> +		free_percpu(dev->tstats);
> +	}
> +
> +	return err;
>  }
> 
>  static int ip6_tnl_changelink(struct net_device *dev, struct nlattr
*tb[], diff
> --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c index 3d8a3b6..b201eef
100644
> --- a/net/ipv6/ip6_vti.c
> +++ b/net/ipv6/ip6_vti.c
> @@ -940,6 +940,7 @@ static int vti6_newlink(struct net *src_net, struct
> net_device *dev,  {
>  	struct net *net = dev_net(dev);
>  	struct ip6_tnl *nt;
> +	int ret;
> 
>  	nt = netdev_priv(dev);
>  	vti6_netlink_parms(data, &nt->parms);
> @@ -949,7 +950,11 @@ static int vti6_newlink(struct net *src_net, struct
> net_device *dev,
>  	if (vti6_locate(net, &nt->parms, 0))
>  		return -EEXIST;
> 
> -	return vti6_tnl_create2(dev);
> +	ret = vti6_tnl_create2(dev);
> +	if (ret)
> +		free_percpu(dev->tstats);
> +
> +	return ret;
>  }
> 
>  static void vti6_dellink(struct net_device *dev, struct list_head *head)
diff
> --git a/net/ipv6/sit.c b/net/ipv6/sit.c index 99853c6..f45dc4a 100644
> --- a/net/ipv6/sit.c
> +++ b/net/ipv6/sit.c
> @@ -1555,8 +1555,11 @@ static int ipip6_newlink(struct net *src_net,
struct
> net_device *dev,
>  		return -EEXIST;
> 
>  	err = ipip6_tunnel_create(dev);
> -	if (err < 0)
> +	if (err < 0) {
> +		dst_cache_destroy(&nt->dst_cache);
> +		free_percpu(dev->tstats);
>  		return err;
> +	}
> 
>  #ifdef CONFIG_IPV6_SIT_6RD
>  	if (ipip6_netlink_6rd_parms(data, &ip6rd))
> --
> 1.9.1

Actually I have another simpler solution to fix it. 
When newlink failed, its caller "rtnl_newlink" invokes the destructor if it
exists like the following:
	if (dev->reg_state == NETREG_UNINITIALIZED)
		if (dev->destructor)
			dev->destructor(dev);
		else
			free_netdev(dev);

There are two reasons I don't adopt this solution.
1. I don't know if it is against the original purpose of dev->destructor and
rtnl_newlink.
2. It breaks the design rule that "who malloc, who free".

But it is one more simple fix, then what's your opinion?

Best Regards
Feng

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] driver/net: Fix possible memleaks when fail to register_netdevice
  2017-04-25 12:01 [PATCH net] driver/net: Fix possible memleaks when fail to register_netdevice gfree.wind
  2017-04-25 12:51 ` Gao Feng
@ 2017-04-27  8:15 ` Herbert Xu
  2017-04-27  8:33   ` Gao Feng
  2017-04-28  0:27   ` Gao Feng
  1 sibling, 2 replies; 5+ messages in thread
From: Herbert Xu @ 2017-04-27  8:15 UTC (permalink / raw)
  To: gfree.wind
  Cc: jiri, davem, kuznet, jmorris, yoshfuji, kaber, steffen.klassert,
	netdev, Gao Feng

On Tue, Apr 25, 2017 at 08:01:50PM +0800, gfree.wind@foxmail.com wrote:
> From: Gao Feng <fgao@ikuai8.com>
> 
> These drivers allocate kinds of resources in init routine, and free
> some resources in the destructor of net_device. It may cause memleak
> when some errors happen after register_netdevice invokes the init
> callback. Because only the uninit callback is invoked in the error
> handler of register_netdevice, but the destructor not. As a result,
> some resources are lost forever.
> 
> Now invokes the destructor instead of free_netdev somewhere, and free
> the left resources in the newlink func when fail to register_netdevice.
> 
> Signed-off-by: Gao Feng <fgao@ikuai8.com>

This has the potential of creating future bugs, because there
is no guarantee that the ndo_init function has been invoked at
all.

Wouldn't it be safer to move the freeing from the destructors
into their ndo_uninit functions instead?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH net] driver/net: Fix possible memleaks when fail to register_netdevice
  2017-04-27  8:15 ` Herbert Xu
@ 2017-04-27  8:33   ` Gao Feng
  2017-04-28  0:27   ` Gao Feng
  1 sibling, 0 replies; 5+ messages in thread
From: Gao Feng @ 2017-04-27  8:33 UTC (permalink / raw)
  To: 'Herbert Xu'
  Cc: jiri, davem, kuznet, jmorris, yoshfuji, kaber, steffen.klassert,
	netdev, 'Gao Feng'

> From: Herbert Xu [mailto:herbert@gondor.apana.org.au]
> Sent: Thursday, April 27, 2017 4:16 PM
> On Tue, Apr 25, 2017 at 08:01:50PM +0800, gfree.wind@foxmail.com wrote:
> > From: Gao Feng <fgao@ikuai8.com>
> >
[...]
> 
> This has the potential of creating future bugs, because there is no
guarantee
> that the ndo_init function has been invoked at all.
> 
> Wouldn't it be safer to move the freeing from the destructors into their
> ndo_uninit functions instead?

I considered about this solution, I am not sure if it is safe to move the
freeing from destructors into ndo_uninit.
Because when the free action is done in ndo_uninit, it is earlier than
destructor. 
I am not sure if it break the design of original driver.

I just tested the team driver before. It is ok to free all mems in
ndo_uninit.
Is it possible that anyone are using the net_dev after ndo_uninit ?

If no one, i would like to update the patch. 
Could you give me some guide please?

Regards
Feng

> 
> Thanks,
> --

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH net] driver/net: Fix possible memleaks when fail to register_netdevice
  2017-04-27  8:15 ` Herbert Xu
  2017-04-27  8:33   ` Gao Feng
@ 2017-04-28  0:27   ` Gao Feng
  1 sibling, 0 replies; 5+ messages in thread
From: Gao Feng @ 2017-04-28  0:27 UTC (permalink / raw)
  To: 'Herbert Xu'
  Cc: jiri, davem, kuznet, jmorris, yoshfuji, kaber, steffen.klassert,
	netdev, 'Gao Feng'

> From: Gao Feng [mailto:gfree.wind@foxmail.com]
> Sent: Thursday, April 27, 2017 4:33 PM
> > From: Herbert Xu [mailto:herbert@gondor.apana.org.au]
> > Sent: Thursday, April 27, 2017 4:16 PM On Tue, Apr 25, 2017 at
> > 08:01:50PM +0800, gfree.wind@foxmail.com wrote:
> > > From: Gao Feng <fgao@ikuai8.com>
> > >
> [...]
> >
[...]

I think I get one method which could be safe to free the mem in ndo_uninit.
Thanks, I would send the v2 patch later.

Best Regards
Feng

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-04-28  0:27 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-25 12:01 [PATCH net] driver/net: Fix possible memleaks when fail to register_netdevice gfree.wind
2017-04-25 12:51 ` Gao Feng
2017-04-27  8:15 ` Herbert Xu
2017-04-27  8:33   ` Gao Feng
2017-04-28  0:27   ` Gao Feng

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.