| Message ID | 20240605235952.21320-1-cpaasch@apple.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: Don't warn on ENOMEM in __dev_change_net_namespace | expand |
Christoph Paasch <cpaasch@apple.com> writes: > syzkaller found a WARN in __dev_change_net_namespace when > device_rename() returns ENOMEM: So device_rename() is returning ENOMEM. Which mean the rename fails. Which means the machine has entered an inconsistent state. Actions have already been taken that essentially can not be rolled back. That warrants a warning. If device_rename is going to return ENOMEM in some case that isn't error injection __dev_change_net_namespace needs to be updated to deal with this case. This patch looks like it is shooting the messenger rather than doing something useful. The fact that the warning has not happened until now suggests that this is not a real world failure scenario but instead this failure is some erroneous error injection triggered by syzkaller. I think there was a version of __dev_change_net_namespace that did not have this failure mode that was never merged into the main tree. At about the same time as this code was being merged, sysfs support was merged and added a number of new failure cases that were frankly ABI breaking if the were to be allowed to happen. A real tail wagging the dog scenario. Eventually dev_change_name was updated to deal with it, but that effort has not been put into __dev_change_net_namespace. If you want to fix it please fix it, but let's not remove the warning that tells us there is a problem in the first place. Thank you, Eric > > ------------[ cut here ]------------ > WARNING: CPU: 1 PID: 6395 at net/core/dev.c:11430 __dev_change_net_namespace+0xba7/0xbf0 > Modules linked in: > CPU: 1 PID: 6395 Comm: syz-executor.1 Not tainted 6.9.0-g4eea1d874bbf #66 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-2.el7 04/01/2014 > RIP: 0010:__dev_change_net_namespace+0xba7/0xbf0 net/core/dev.c:11430 > Code: 05 d6 72 34 01 01 48 c7 c7 ea e8 c4 82 48 c7 c6 21 d2 cb 82 ba bf 07 00 00 e8 25 cc 39 ff 0f 0b e9 5b f8 ff ff e8 c9 b3 4f ff <0f> 0b e9 ca fc ff ff e8 bd b3 4f ff 0f 0b e9 5f fe ff ff e8 b1 b3 > RSP: 0018:ffffc90000d1f410 EFLAGS: 00010293 > RAX: ffffffff81d1d487 RBX: ffff8881213b5000 RCX: ffff888115f9adc0 > RDX: 0000000000000000 RSI: 00000000fffffff4 RDI: 0000000000000000 > RBP: ffffc90000d1f4e0 R08: ffffffff81d1d14a R09: 205d393032343136 > R10: 3e4b5341542f3c20 R11: ffffffff81a40d20 R12: ffff88811f57af40 > R13: 0000000000000000 R14: 00000000fffffff4 R15: ffff8881213b5000 > FS: 00007fc93618e640(0000) GS:ffff88813bd00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000000000468d90 CR3: 0000000126252003 CR4: 0000000000170ef0 > Call Trace: > <TASK> > do_setlink+0x154/0x1c70 net/core/rtnetlink.c:2805 > __rtnl_newlink net/core/rtnetlink.c:3696 [inline] > rtnl_newlink+0xe60/0x1210 net/core/rtnetlink.c:3743 > rtnetlink_rcv_msg+0x689/0x720 net/core/rtnetlink.c:6595 > netlink_rcv_skb+0xea/0x1c0 net/netlink/af_netlink.c:2564 > netlink_unicast_kernel net/netlink/af_netlink.c:1335 [inline] > netlink_unicast+0x430/0x500 net/netlink/af_netlink.c:1361 > netlink_sendmsg+0x43d/0x540 net/netlink/af_netlink.c:1905 > sock_sendmsg_nosec net/socket.c:730 [inline] > __sock_sendmsg+0xa4/0xd0 net/socket.c:745 > ____sys_sendmsg+0x22a/0x320 net/socket.c:2585 > ___sys_sendmsg+0x143/0x190 net/socket.c:2639 > __sys_sendmsg net/socket.c:2668 [inline] > __do_sys_sendmsg net/socket.c:2677 [inline] > __se_sys_sendmsg net/socket.c:2675 [inline] > __x64_sys_sendmsg+0xd8/0x150 net/socket.c:2675 > do_syscall_x64 arch/x86/entry/common.c:52 [inline] > do_syscall_64+0x54/0xf0 arch/x86/entry/common.c:83 > entry_SYSCALL_64_after_hwframe+0x76/0x7e > RIP: 0033:0x7fc936e686a9 > > The WARN is there because device_rename() is indeed not meant to > fail in __dev_change_net_namespace(), except for the case where > it can't allocated memory. > > So, let's special-case the scenario where err == ENOMEM to silence the > warning. > > AFAICS, this has been there since the initial implementation. > > Cc: Eric W. Biederman <ebiederm@xmission.com> > Fixes: ce286d327341 ("[NET]: Implement network device movement between namespaces") > Signed-off-by: Christoph Paasch <cpaasch@apple.com> > --- > net/core/dev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 4d4de9008f6f..ba0c9f705ddb 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -11428,7 +11428,7 @@ int __dev_change_net_namespace(struct net_device *dev, struct net *net, > dev_set_uevent_suppress(&dev->dev, 1); > err = device_rename(&dev->dev, dev->name); > dev_set_uevent_suppress(&dev->dev, 0); > - WARN_ON(err); > + WARN_ON(err && err != -ENOMEM); > > /* Send a netdev-add uevent to the new namespace */ > kobject_uevent(&dev->dev.kobj, KOBJ_ADD);
diff --git a/net/core/dev.c b/net/core/dev.c index 4d4de9008f6f..ba0c9f705ddb 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -11428,7 +11428,7 @@ int __dev_change_net_namespace(struct net_device *dev, struct net *net, dev_set_uevent_suppress(&dev->dev, 1); err = device_rename(&dev->dev, dev->name); dev_set_uevent_suppress(&dev->dev, 0); - WARN_ON(err); + WARN_ON(err && err != -ENOMEM); /* Send a netdev-add uevent to the new namespace */ kobject_uevent(&dev->dev.kobj, KOBJ_ADD);
syzkaller found a WARN in __dev_change_net_namespace when device_rename() returns ENOMEM: ------------[ cut here ]------------ WARNING: CPU: 1 PID: 6395 at net/core/dev.c:11430 __dev_change_net_namespace+0xba7/0xbf0 Modules linked in: CPU: 1 PID: 6395 Comm: syz-executor.1 Not tainted 6.9.0-g4eea1d874bbf #66 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-2.el7 04/01/2014 RIP: 0010:__dev_change_net_namespace+0xba7/0xbf0 net/core/dev.c:11430 Code: 05 d6 72 34 01 01 48 c7 c7 ea e8 c4 82 48 c7 c6 21 d2 cb 82 ba bf 07 00 00 e8 25 cc 39 ff 0f 0b e9 5b f8 ff ff e8 c9 b3 4f ff <0f> 0b e9 ca fc ff ff e8 bd b3 4f ff 0f 0b e9 5f fe ff ff e8 b1 b3 RSP: 0018:ffffc90000d1f410 EFLAGS: 00010293 RAX: ffffffff81d1d487 RBX: ffff8881213b5000 RCX: ffff888115f9adc0 RDX: 0000000000000000 RSI: 00000000fffffff4 RDI: 0000000000000000 RBP: ffffc90000d1f4e0 R08: ffffffff81d1d14a R09: 205d393032343136 R10: 3e4b5341542f3c20 R11: ffffffff81a40d20 R12: ffff88811f57af40 R13: 0000000000000000 R14: 00000000fffffff4 R15: ffff8881213b5000 FS: 00007fc93618e640(0000) GS:ffff88813bd00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000468d90 CR3: 0000000126252003 CR4: 0000000000170ef0 Call Trace: <TASK> do_setlink+0x154/0x1c70 net/core/rtnetlink.c:2805 __rtnl_newlink net/core/rtnetlink.c:3696 [inline] rtnl_newlink+0xe60/0x1210 net/core/rtnetlink.c:3743 rtnetlink_rcv_msg+0x689/0x720 net/core/rtnetlink.c:6595 netlink_rcv_skb+0xea/0x1c0 net/netlink/af_netlink.c:2564 netlink_unicast_kernel net/netlink/af_netlink.c:1335 [inline] netlink_unicast+0x430/0x500 net/netlink/af_netlink.c:1361 netlink_sendmsg+0x43d/0x540 net/netlink/af_netlink.c:1905 sock_sendmsg_nosec net/socket.c:730 [inline] __sock_sendmsg+0xa4/0xd0 net/socket.c:745 ____sys_sendmsg+0x22a/0x320 net/socket.c:2585 ___sys_sendmsg+0x143/0x190 net/socket.c:2639 __sys_sendmsg net/socket.c:2668 [inline] __do_sys_sendmsg net/socket.c:2677 [inline] __se_sys_sendmsg net/socket.c:2675 [inline] __x64_sys_sendmsg+0xd8/0x150 net/socket.c:2675 do_syscall_x64 arch/x86/entry/common.c:52 [inline] do_syscall_64+0x54/0xf0 arch/x86/entry/common.c:83 entry_SYSCALL_64_after_hwframe+0x76/0x7e RIP: 0033:0x7fc936e686a9 The WARN is there because device_rename() is indeed not meant to fail in __dev_change_net_namespace(), except for the case where it can't allocated memory. So, let's special-case the scenario where err == ENOMEM to silence the warning. AFAICS, this has been there since the initial implementation. Cc: Eric W. Biederman <ebiederm@xmission.com> Fixes: ce286d327341 ("[NET]: Implement network device movement between namespaces") Signed-off-by: Christoph Paasch <cpaasch@apple.com> --- net/core/dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)