| Message ID | 20200830131336.275844-1-rkovhaev@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | veth: fix memory leak in veth_newlink() | expand |
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
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
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
From: Rustam Kovhaev <rkovhaev@gmail.com> 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+59ef240dd8f0ed7598a8@syzkaller.appspotmail.com > Link: https://syzkaller.appspot.com/bug?extid=59ef240dd8f0ed7598a8 > Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com> 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.
On Tue, Sep 01, 2020 at 01:01:27PM -0700, David Miller wrote: > From: Rustam Kovhaev <rkovhaev@gmail.com> > 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+59ef240dd8f0ed7598a8@syzkaller.appspotmail.com > > Link: https://syzkaller.appspot.com/bug?extid=59ef240dd8f0ed7598a8 > > Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com> > > 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
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;
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+59ef240dd8f0ed7598a8@syzkaller.appspotmail.com Link: https://syzkaller.appspot.com/bug?extid=59ef240dd8f0ed7598a8 Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com> --- drivers/net/veth.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)