Re: [PATCH] veth: fix memory leak in veth_newlink()
On Tue, Sep 01, 2020 at 01:01:27PM -0700, David Miller wrote: > From: Rustam Kovhaev > Date: Sun, 30 Aug 2020 06:13:36 -0700 > > > when register_netdevice(dev) fails we should check whether struct > > veth_rq has been allocated via ndo_init callback and free it, because, > > depending on the code path, register_netdevice() might not call > > priv_destructor() callback > > > > Reported-and-tested-by: > > syzbot+59ef240dd8f0ed759...@syzkaller.appspotmail.com > > Link: https://syzkaller.appspot.com/bug?extid=59ef240dd8f0ed7598a8 > > Signed-off-by: Rustam Kovhaev > > I think I agree with Toshiaki here. There is no reason why the > rollback_registered() path of register_netdevice() should behave > differently from the normal control flow. > > Any code path that invokes ->ndo_uninit() should probably also > invoke the priv destructor. hi David, thank you for the review! > > The question is why does the err_uninit: label of register_netdevice > behave differently from rollback_registered()? If there is a reason, > it should be documented in a comment or similar. If it is wrong, > it should be corrected. good question, that i do not know, i'll review it
Re: [PATCH] veth: fix memory leak in veth_newlink()
From: Rustam Kovhaev Date: Sun, 30 Aug 2020 06:13:36 -0700 > when register_netdevice(dev) fails we should check whether struct > veth_rq has been allocated via ndo_init callback and free it, because, > depending on the code path, register_netdevice() might not call > priv_destructor() callback > > Reported-and-tested-by: syzbot+59ef240dd8f0ed759...@syzkaller.appspotmail.com > Link: https://syzkaller.appspot.com/bug?extid=59ef240dd8f0ed7598a8 > Signed-off-by: Rustam Kovhaev I think I agree with Toshiaki here. There is no reason why the rollback_registered() path of register_netdevice() should behave differently from the normal control flow. Any code path that invokes ->ndo_uninit() should probably also invoke the priv destructor. The question is why does the err_uninit: label of register_netdevice behave differently from rollback_registered()? If there is a reason, it should be documented in a comment or similar. If it is wrong, it should be corrected.
Re: [PATCH] veth: fix memory leak in veth_newlink()
On 2020/08/31 9:51, Rustam Kovhaev wrote: On Mon, Aug 31, 2020 at 09:16:32AM +0900, Toshiaki Makita wrote: On 2020/08/30 22:13, Rustam Kovhaev wrote: when register_netdevice(dev) fails we should check whether struct veth_rq has been allocated via ndo_init callback and free it, because, depending on the code path, register_netdevice() might not call priv_destructor() callback AFAICS, register_netdevice() always goto err_uninit and calls priv_destructor() on failure after ndo_init() succeeded. So I could not find such a code path. Would you elaborate on it? in net/core/dev.c:9863, where register_netdevice() calls rollback_registered(), which does not call priv_destructor(), then register_netdevice() returns error net/core/dev.c:9884 Thank you, now I see the code path. But then all devices which allocate something in ndo_init() and free them in priv_destructor() are affected? E.g. loopback and ifb seem to do such thing. Why not calling priv_destructor() after invocation of rollback_registered()? It looks weird that only that path does not call priv_destructor(). Toshiaki Makita
Re: [PATCH] veth: fix memory leak in veth_newlink()
On Mon, Aug 31, 2020 at 09:16:32AM +0900, Toshiaki Makita wrote: > On 2020/08/30 22:13, Rustam Kovhaev wrote: > > when register_netdevice(dev) fails we should check whether struct > > veth_rq has been allocated via ndo_init callback and free it, because, > > depending on the code path, register_netdevice() might not call > > priv_destructor() callback > > AFAICS, register_netdevice() always goto err_uninit and calls > priv_destructor() > on failure after ndo_init() succeeded. > So I could not find such a code path. > Would you elaborate on it? in net/core/dev.c:9863, where register_netdevice() calls rollback_registered(), which does not call priv_destructor(), then register_netdevice() returns error net/core/dev.c:9884
Re: [PATCH] veth: fix memory leak in veth_newlink()
On 2020/08/30 22:13, Rustam Kovhaev wrote: when register_netdevice(dev) fails we should check whether struct veth_rq has been allocated via ndo_init callback and free it, because, depending on the code path, register_netdevice() might not call priv_destructor() callback AFAICS, register_netdevice() always goto err_uninit and calls priv_destructor() on failure after ndo_init() succeeded. So I could not find such a code path. Would you elaborate on it? Thanks, Toshiaki Makita
[PATCH] veth: fix memory leak in veth_newlink()
when register_netdevice(dev) fails we should check whether struct veth_rq has been allocated via ndo_init callback and free it, because, depending on the code path, register_netdevice() might not call priv_destructor() callback Reported-and-tested-by: syzbot+59ef240dd8f0ed759...@syzkaller.appspotmail.com Link: https://syzkaller.appspot.com/bug?extid=59ef240dd8f0ed7598a8 Signed-off-by: Rustam Kovhaev --- drivers/net/veth.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/net/veth.c b/drivers/net/veth.c index a475f48d43c4..e40ca62a046a 100644 --- a/drivers/net/veth.c +++ b/drivers/net/veth.c @@ -1394,7 +1394,9 @@ static int veth_newlink(struct net *src_net, struct net_device *dev, return 0; err_register_dev: - /* nothing to do */ + priv = netdev_priv(dev); + if (priv->rq) + veth_dev_free(dev); err_configure_peer: unregister_netdevice(peer); return err; -- 2.28.0
