| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 25 Apr 2017 20:51:00 +0800
From: "Gao Feng" <gfree.wind@...mail.com>
To: <gfree.wind@...mail.com>, <jiri@...nulli.us>,
<davem@...emloft.net>, <kuznet@....inr.ac.ru>, <jmorris@...ei.org>,
<yoshfuji@...ux-ipv6.org>, <kaber@...sh.net>,
<steffen.klassert@...unet.com>, <herbert@...dor.apana.org.au>,
<netdev@...r.kernel.org>
Subject: RE: [PATCH net] driver/net: Fix possible memleaks when fail to register_netdevice
> From: Gao Feng <fgao@...ai8.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@...ai8.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
Powered by blists - more mailing lists