| Message ID | 20201201092903.3269202-1-yangyingliang@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net,v2,1/2] wireguard: device: don't call free_netdev() in priv_destructor() | expand |
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
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 > .
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;
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(-)