diff mbox series

[net,v2,1/2] wireguard: device: don't call free_netdev() in priv_destructor()

Message ID 20201201092903.3269202-1-yangyingliang@huawei.com
State New
Headers show
Series [net,v2,1/2] wireguard: device: don't call free_netdev() in priv_destructor() | expand

Commit Message

Yang Yingliang Dec. 1, 2020, 9:29 a.m. UTC
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(-)

Comments

Jason A. Donenfeld Dec. 1, 2020, 9:46 a.m. UTC | #1
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
Yang Yingliang Dec. 1, 2020, 1:47 p.m. UTC | #2
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 mbox series

Patch

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;