* [PATCH net v2 1/2] wireguard: device: don't call free_netdev() in priv_destructor()
@ 2020-12-01 9:29 Yang Yingliang
2020-12-01 9:29 ` [PATCH net v2 2/2] net: fix memory leak in register_netdevice() on error path Yang Yingliang
2020-12-01 9:46 ` [PATCH net v2 1/2] wireguard: device: don't call free_netdev() in priv_destructor() Jason A. Donenfeld
0 siblings, 2 replies; 4+ messages in thread
From: Yang Yingliang @ 2020-12-01 9:29 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, toshiaki.makita1, rkovhaev, Jason, yangyingliang
After commit cf124db566e6 ("net: Fix inconsistent teardown and..."),
priv_destruct() doesn't call free_netdev() in driver, we use
dev->needs_free_netdev to indicate whether free_netdev() should be
called on release path.
This patch remove free_netdev() from priv_destructor() and set
dev->needs_free_netdev to true.
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
drivers/net/wireguard/device.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireguard/device.c b/drivers/net/wireguard/device.c
index c9f65e96ccb0..578ac6097d7e 100644
--- a/drivers/net/wireguard/device.c
+++ b/drivers/net/wireguard/device.c
@@ -247,7 +247,6 @@ static void wg_destruct(struct net_device *dev)
mutex_unlock(&wg->device_update_lock);
pr_debug("%s: Interface destroyed\n", dev->name);
- free_netdev(dev);
}
static const struct device_type device_type = { .name = KBUILD_MODNAME };
@@ -360,6 +359,7 @@ static int wg_newlink(struct net *src_net, struct net_device *dev,
* register_netdevice doesn't call it for us if it fails.
*/
dev->priv_destructor = wg_destruct;
+ dev->needs_free_netdev = true;
pr_debug("%s: Interface created\n", dev->name);
return ret;
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH net v2 2/2] net: fix memory leak in register_netdevice() on error path
2020-12-01 9:29 [PATCH net v2 1/2] wireguard: device: don't call free_netdev() in priv_destructor() Yang Yingliang
@ 2020-12-01 9:29 ` Yang Yingliang
2020-12-01 9:46 ` [PATCH net v2 1/2] wireguard: device: don't call free_netdev() in priv_destructor() Jason A. Donenfeld
1 sibling, 0 replies; 4+ messages in thread
From: Yang Yingliang @ 2020-12-01 9:29 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, toshiaki.makita1, rkovhaev, Jason, yangyingliang
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>
---
net/core/dev.c | 10 ++++++++++
1 file changed, 10 insertions(+)
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
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net v2 1/2] wireguard: device: don't call free_netdev() in priv_destructor()
2020-12-01 9:29 [PATCH net v2 1/2] wireguard: device: don't call free_netdev() in priv_destructor() Yang Yingliang
2020-12-01 9:29 ` [PATCH net v2 2/2] net: fix memory leak in register_netdevice() on error path Yang Yingliang
@ 2020-12-01 9:46 ` Jason A. Donenfeld
2020-12-01 13:47 ` Yang Yingliang
1 sibling, 1 reply; 4+ messages in thread
From: Jason A. Donenfeld @ 2020-12-01 9:46 UTC (permalink / raw)
To: yangyingliang
Cc: Netdev, David Miller, Jakub Kicinski, toshiaki.makita1, rkovhaev
Hi Yang,
On Tue, Dec 1, 2020 at 10:31 AM Yang Yingliang <yangyingliang@huawei.com> wrote:
>
> After commit cf124db566e6 ("net: Fix inconsistent teardown and..."),
> priv_destruct() doesn't call free_netdev() in driver, we use
> dev->needs_free_netdev to indicate whether free_netdev() should be
> called on release path.
> This patch remove free_netdev() from priv_destructor() and set
> dev->needs_free_netdev to true.
For now, nack.
I remember when cf124db566e6 came out and carefully looking at the
construction of device.c in WireGuard. priv_destructor is only
assigned after register_device, with the various error paths in
wg_newlink responsible for cleaning up other earlier failures, and
trying to move to needs_free_netdev would have introduced more
complexity in this particular case, if my memory serves. I do not
think there's a memory leak here, and I worry about too hastily
changing the state machine "just because".
In other words, could you point out how to generate a memory leak? If
you're correct, then we can start dissecting and refactoring this. But
off the bat, I'm not sure I'm exactly seeing whatever you're seeing.
Jason
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net v2 1/2] wireguard: device: don't call free_netdev() in priv_destructor()
2020-12-01 9:46 ` [PATCH net v2 1/2] wireguard: device: don't call free_netdev() in priv_destructor() Jason A. Donenfeld
@ 2020-12-01 13:47 ` Yang Yingliang
0 siblings, 0 replies; 4+ messages in thread
From: Yang Yingliang @ 2020-12-01 13:47 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Netdev, David Miller, Jakub Kicinski, toshiaki.makita1, rkovhaev
On 2020/12/1 17:46, Jason A. Donenfeld wrote:
> Hi Yang,
>
> On Tue, Dec 1, 2020 at 10:31 AM Yang Yingliang <yangyingliang@huawei.com> wrote:
>> After commit cf124db566e6 ("net: Fix inconsistent teardown and..."),
>> priv_destruct() doesn't call free_netdev() in driver, we use
>> dev->needs_free_netdev to indicate whether free_netdev() should be
>> called on release path.
>> This patch remove free_netdev() from priv_destructor() and set
>> dev->needs_free_netdev to true.
> For now, nack.
>
> I remember when cf124db566e6 came out and carefully looking at the
> construction of device.c in WireGuard. priv_destructor is only
> assigned after register_device, with the various error paths in
> wg_newlink responsible for cleaning up other earlier failures, and
> trying to move to needs_free_netdev would have introduced more
> complexity in this particular case, if my memory serves. I do not
> think there's a memory leak here, and I worry about too hastily
> changing the state machine "just because".
>
> In other words, could you point out how to generate a memory leak? If
> you're correct, then we can start dissecting and refactoring this. But
> off the bat, I'm not sure I'm exactly seeing whatever you're seeing.
Yes, I missed that priv_destructor is only assigned after
register_netdevice(),
so, it will not lead a double free in my patch#2, so this patch can be
dropped and
send v3.
>
> Jason
> .
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-12-01 13:48 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-01 9:29 [PATCH net v2 1/2] wireguard: device: don't call free_netdev() in priv_destructor() Yang Yingliang
2020-12-01 9:29 ` [PATCH net v2 2/2] net: fix memory leak in register_netdevice() on error path Yang Yingliang
2020-12-01 9:46 ` [PATCH net v2 1/2] wireguard: device: don't call free_netdev() in priv_destructor() Jason A. Donenfeld
2020-12-01 13:47 ` Yang Yingliang
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.