| Message ID | 20201201135457.3549435-1-yangyingliang@huawei.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] net: fix memory leak in register_netdevice() on error path | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | fail | Series targets non-next tree, but doesn't contain any Fixes tags |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 10 this patch: 10 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 16 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 10 this patch: 10 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Tue, 1 Dec 2020 21:54:57 +0800 Yang Yingliang wrote: > I got a memleak report when doing fault-inject test: > > unreferenced object 0xffff88810ace9000 (size 1024): > comm "ip", pid 4622, jiffies 4295457037 (age 43.378s) > hex dump (first 32 bytes): > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > [<00000000008abe41>] __kmalloc+0x10f/0x210 > [<000000005d3533a6>] veth_dev_init+0x140/0x310 > [<0000000088353c64>] register_netdevice+0x496/0x7a0 > [<000000001324d322>] veth_newlink+0x40b/0x960 > [<00000000d0799866>] __rtnl_newlink+0xd8c/0x1360 > [<00000000d616040a>] rtnl_newlink+0x6b/0xa0 > [<00000000e0a1600d>] rtnetlink_rcv_msg+0x3cc/0x9e0 > [<000000009eeff98b>] netlink_rcv_skb+0x130/0x3a0 > [<00000000500f8be1>] netlink_unicast+0x4da/0x700 > [<00000000666c03b3>] netlink_sendmsg+0x7fe/0xcb0 > [<0000000073b28103>] sock_sendmsg+0x143/0x180 > [<00000000ad746a30>] ____sys_sendmsg+0x677/0x810 > [<0000000087dd98e5>] ___sys_sendmsg+0x105/0x180 > [<00000000028dd365>] __sys_sendmsg+0xf0/0x1c0 > [<00000000a6bfbae6>] do_syscall_64+0x33/0x40 > [<00000000e00521b4>] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > It seems ifb and loopback may also hit the leak, so I try to fix this in > register_netdevice(). > > In common case, priv_destructor() will be called in netdev_run_todo() > after calling ndo_uninit() in rollback_registered(), on other error > path in register_netdevice(), ndo_uninit() and priv_destructor() are > called before register_netdevice() return, but in this case, > priv_destructor() will never be called, then it causes memory leak, > so we should call priv_destructor() here. > > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > --- > v2 -> v3: In wireguard driver, priv_destructor() will call > free_netdev(), but it is assigned after register_netdevice(), > so it will not lead a double free, drop patch#1. Also I've > test wireguard device, it's no memory leak on this error path. Sorry I don't want to apply yet another wobbly workaround to this path. I started hacking on a rework of the registration / free which will solve this and all the other corner cases which are broken around here. Stay tuned.
diff --git a/net/core/dev.c b/net/core/dev.c index 82dc6b48e45f..51b9cf1ff6a1 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -10003,6 +10003,16 @@ int register_netdevice(struct net_device *dev) rcu_barrier(); dev->reg_state = NETREG_UNREGISTERED; + /* In common case, priv_destructor() will be + * called in netdev_run_todo() after calling + * ndo_uninit() in rollback_registered(). + * But in this case, priv_destructor() will + * never be called, then it causes memory + * leak, so we should call priv_destructor() + * here. + */ + if (dev->priv_destructor) + dev->priv_destructor(dev); /* We should put the kobject that hold in * netdev_unregister_kobject(), otherwise * the net device cannot be freed when
I got a memleak report when doing fault-inject test: unreferenced object 0xffff88810ace9000 (size 1024): comm "ip", pid 4622, jiffies 4295457037 (age 43.378s) hex dump (first 32 bytes): 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<00000000008abe41>] __kmalloc+0x10f/0x210 [<000000005d3533a6>] veth_dev_init+0x140/0x310 [<0000000088353c64>] register_netdevice+0x496/0x7a0 [<000000001324d322>] veth_newlink+0x40b/0x960 [<00000000d0799866>] __rtnl_newlink+0xd8c/0x1360 [<00000000d616040a>] rtnl_newlink+0x6b/0xa0 [<00000000e0a1600d>] rtnetlink_rcv_msg+0x3cc/0x9e0 [<000000009eeff98b>] netlink_rcv_skb+0x130/0x3a0 [<00000000500f8be1>] netlink_unicast+0x4da/0x700 [<00000000666c03b3>] netlink_sendmsg+0x7fe/0xcb0 [<0000000073b28103>] sock_sendmsg+0x143/0x180 [<00000000ad746a30>] ____sys_sendmsg+0x677/0x810 [<0000000087dd98e5>] ___sys_sendmsg+0x105/0x180 [<00000000028dd365>] __sys_sendmsg+0xf0/0x1c0 [<00000000a6bfbae6>] do_syscall_64+0x33/0x40 [<00000000e00521b4>] entry_SYSCALL_64_after_hwframe+0x44/0xa9 It seems ifb and loopback may also hit the leak, so I try to fix this in register_netdevice(). In common case, priv_destructor() will be called in netdev_run_todo() after calling ndo_uninit() in rollback_registered(), on other error path in register_netdevice(), ndo_uninit() and priv_destructor() are called before register_netdevice() return, but in this case, priv_destructor() will never be called, then it causes memory leak, so we should call priv_destructor() here. Reported-by: Hulk Robot <hulkci@huawei.com> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- v2 -> v3: In wireguard driver, priv_destructor() will call free_netdev(), but it is assigned after register_netdevice(), so it will not lead a double free, drop patch#1. Also I've test wireguard device, it's no memory leak on this error path. --- net/core/dev.c | 10 ++++++++++ 1 file changed, 10 insertions(+)