| Message ID | 20221108132208.938676-4-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: devlink: move netdev notifier block to dest namespace during reload | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next, async |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/build_clang | success | Errors and warnings before: 5 this patch: 5 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 10 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, Nov 08, 2022 at 02:22:08PM +0100, Jiri Pirko wrote: > From: Jiri Pirko <jiri@nvidia.com> > > As the return value is not 0 only in case there is no such notifier > block registered, add a WARN_ON() to yell about it. > > Suggested-by: Ido Schimmel <idosch@idosch.org> > Signed-off-by: Jiri Pirko <jiri@nvidia.com> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
On Wed, Nov 9, 2022 at 3:49 AM Ido Schimmel <idosch@idosch.org> wrote: > > On Tue, Nov 08, 2022 at 02:22:08PM +0100, Jiri Pirko wrote: > > From: Jiri Pirko <jiri@nvidia.com> > > > > As the return value is not 0 only in case there is no such notifier > > block registered, add a WARN_ON() to yell about it. > > > > Suggested-by: Ido Schimmel <idosch@idosch.org> > > Signed-off-by: Jiri Pirko <jiri@nvidia.com> > > Reviewed-by: Ido Schimmel <idosch@nvidia.com> Please consider WARN_ON_ONCE(), or DEBUG_NET_WARN_ON_ONCE()
On Wed, 9 Nov 2022 08:26:10 -0800 Eric Dumazet wrote: > > On Tue, Nov 08, 2022 at 02:22:08PM +0100, Jiri Pirko wrote: > > > From: Jiri Pirko <jiri@nvidia.com> > > > > > > As the return value is not 0 only in case there is no such notifier > > > block registered, add a WARN_ON() to yell about it. > > > > > > Suggested-by: Ido Schimmel <idosch@idosch.org> > > > Signed-off-by: Jiri Pirko <jiri@nvidia.com> > > > > Reviewed-by: Ido Schimmel <idosch@nvidia.com> > > Please consider WARN_ON_ONCE(), or DEBUG_NET_WARN_ON_ONCE() Do you have any general guidance on when to pick WARN() vs WARN_ONCE()? Or should we always prefer _ONCE() going forward? Let me take the first 2 in, to lower the syzbot volume.
Wed, Nov 09, 2022 at 05:26:10PM CET, edumazet@google.com wrote: >On Wed, Nov 9, 2022 at 3:49 AM Ido Schimmel <idosch@idosch.org> wrote: >> >> On Tue, Nov 08, 2022 at 02:22:08PM +0100, Jiri Pirko wrote: >> > From: Jiri Pirko <jiri@nvidia.com> >> > >> > As the return value is not 0 only in case there is no such notifier >> > block registered, add a WARN_ON() to yell about it. >> > >> > Suggested-by: Ido Schimmel <idosch@idosch.org> >> > Signed-off-by: Jiri Pirko <jiri@nvidia.com> >> >> Reviewed-by: Ido Schimmel <idosch@nvidia.com> > >Please consider WARN_ON_ONCE(), or DEBUG_NET_WARN_ON_ONCE() Well, in this case, I think that plain WARN_ON is fine as this happens only during driver cleanup which is not expected to happen very often (or not at all) in real world scenarios.
Wed, Nov 09, 2022 at 10:45:36PM CET, kuba@kernel.org wrote: >On Wed, 9 Nov 2022 08:26:10 -0800 Eric Dumazet wrote: >> > On Tue, Nov 08, 2022 at 02:22:08PM +0100, Jiri Pirko wrote: >> > > From: Jiri Pirko <jiri@nvidia.com> >> > > >> > > As the return value is not 0 only in case there is no such notifier >> > > block registered, add a WARN_ON() to yell about it. >> > > >> > > Suggested-by: Ido Schimmel <idosch@idosch.org> >> > > Signed-off-by: Jiri Pirko <jiri@nvidia.com> >> > >> > Reviewed-by: Ido Schimmel <idosch@nvidia.com> >> >> Please consider WARN_ON_ONCE(), or DEBUG_NET_WARN_ON_ONCE() > >Do you have any general guidance on when to pick WARN() vs WARN_ONCE()? >Or should we always prefer _ONCE() going forward? Good question. If so, it should be documented or spotted by checkpatch. > >Let me take the first 2 in, to lower the syzbot volume.
On Wed, Nov 9, 2022 at 11:54 PM Jiri Pirko <jiri@resnulli.us> wrote: > > Wed, Nov 09, 2022 at 10:45:36PM CET, kuba@kernel.org wrote: > >On Wed, 9 Nov 2022 08:26:10 -0800 Eric Dumazet wrote: > >> > On Tue, Nov 08, 2022 at 02:22:08PM +0100, Jiri Pirko wrote: > >> > > From: Jiri Pirko <jiri@nvidia.com> > >> > > > >> > > As the return value is not 0 only in case there is no such notifier > >> > > block registered, add a WARN_ON() to yell about it. > >> > > > >> > > Suggested-by: Ido Schimmel <idosch@idosch.org> > >> > > Signed-off-by: Jiri Pirko <jiri@nvidia.com> > >> > > >> > Reviewed-by: Ido Schimmel <idosch@nvidia.com> > >> > >> Please consider WARN_ON_ONCE(), or DEBUG_NET_WARN_ON_ONCE() > > > >Do you have any general guidance on when to pick WARN() vs WARN_ONCE()? > >Or should we always prefer _ONCE() going forward? > > Good question. If so, it should be documented or spotted by checkpatch. > > > > >Let me take the first 2 in, to lower the syzbot volume. Well, I am not sure what you call 'lower syzbot volume' netdevsim netdevsim0 netdevsim3 (unregistering): unset [1, 0] type 2 family 0 port 6081 - 0 ------------[ cut here ]------------ WARNING: CPU: 1 PID: 41 at net/core/devlink.c:10001 devl_port_unregister+0x2f6/0x390 net/core/devlink.c:10001 Modules linked in: CPU: 0 PID: 41 Comm: kworker/u4:2 Not tainted 6.1.0-rc3-syzkaller-00887-g0c9ef08a4d0f #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022 Workqueue: netns cleanup_net RIP: 0010:devl_port_unregister+0x2f6/0x390 net/core/devlink.c:10001 Code: e8 3f 37 0b fa 85 ed 0f 85 7a fd ff ff e8 62 3a 0b fa 0f 0b e9 6e fd ff ff e8 56 3a 0b fa 0f 0b e9 53 ff ff ff e8 4a 3a 0b fa <0f> 0b e9 94 fd ff ff e8 ae ac 57 fa e9 78 ff ff ff e8 74 ac 57 fa RSP: 0018:ffffc90000b27a08 EFLAGS: 00010293 RAX: 0000000000000000 RBX: ffff88806ee3f810 RCX: 0000000000000000 RDX: ffff8880175e1d40 RSI: ffffffff877177d6 RDI: 0000000000000005 RBP: 0000000000000002 R08: 0000000000000005 R09: 0000000000000000 R10: 0000000000000002 R11: 0000000000000000 R12: ffff88806ee3f810 R13: ffff88806ee3f808 R14: ffff88806ee3e800 R15: ffff88806ee3f800 FS: 0000000000000000(0000) GS:ffff8880b9a00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 000000c00023dee0 CR3: 0000000074faf000 CR4: 00000000003506f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: <TASK> __nsim_dev_port_del+0x1bb/0x240 drivers/net/netdevsim/dev.c:1433 nsim_dev_port_del_all drivers/net/netdevsim/dev.c:1443 [inline] nsim_dev_reload_destroy+0x171/0x510 drivers/net/netdevsim/dev.c:1660 nsim_dev_reload_down+0x6b/0xd0 drivers/net/netdevsim/dev.c:968 devlink_reload+0x1c4/0x6e0 net/core/devlink.c:4501 devlink_pernet_pre_exit+0x104/0x1c0 net/core/devlink.c:12615 ops_pre_exit_list net/core/net_namespace.c:159 [inline] cleanup_net+0x451/0xb10 net/core/net_namespace.c:594 process_one_work+0x9bf/0x1710 kernel/workqueue.c:2289 worker_thread+0x665/0x1080 kernel/workqueue.c:2436 kthread+0x2e4/0x3a0 kernel/kthread.c:376 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306 </TASK>
On Thu, Nov 10, 2022 at 09:21:23AM -0800, Eric Dumazet wrote: > On Wed, Nov 9, 2022 at 11:54 PM Jiri Pirko <jiri@resnulli.us> wrote: > > > > Wed, Nov 09, 2022 at 10:45:36PM CET, kuba@kernel.org wrote: > > >On Wed, 9 Nov 2022 08:26:10 -0800 Eric Dumazet wrote: > > >> > On Tue, Nov 08, 2022 at 02:22:08PM +0100, Jiri Pirko wrote: > > >> > > From: Jiri Pirko <jiri@nvidia.com> > > >> > > > > >> > > As the return value is not 0 only in case there is no such notifier > > >> > > block registered, add a WARN_ON() to yell about it. > > >> > > > > >> > > Suggested-by: Ido Schimmel <idosch@idosch.org> > > >> > > Signed-off-by: Jiri Pirko <jiri@nvidia.com> > > >> > > > >> > Reviewed-by: Ido Schimmel <idosch@nvidia.com> > > >> > > >> Please consider WARN_ON_ONCE(), or DEBUG_NET_WARN_ON_ONCE() > > > > > >Do you have any general guidance on when to pick WARN() vs WARN_ONCE()? > > >Or should we always prefer _ONCE() going forward? > > > > Good question. If so, it should be documented or spotted by checkpatch. > > > > > > > >Let me take the first 2 in, to lower the syzbot volume. > > Well, I am not sure what you call 'lower syzbot volume' > > netdevsim netdevsim0 netdevsim3 (unregistering): unset [1, 0] type 2 > family 0 port 6081 - 0 > ------------[ cut here ]------------ > WARNING: CPU: 1 PID: 41 at net/core/devlink.c:10001 > devl_port_unregister+0x2f6/0x390 net/core/devlink.c:10001 Hi Eric, That's a different bug than the one fixed by this patchset. Should be fixed by this patch: https://patchwork.kernel.org/project/netdevbpf/patch/20221110085150.520800-1-idosch@nvidia.com/ > Modules linked in: > CPU: 0 PID: 41 Comm: kworker/u4:2 Not tainted > 6.1.0-rc3-syzkaller-00887-g0c9ef08a4d0f #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, > BIOS Google 10/26/2022 > Workqueue: netns cleanup_net > RIP: 0010:devl_port_unregister+0x2f6/0x390 net/core/devlink.c:10001 > Code: e8 3f 37 0b fa 85 ed 0f 85 7a fd ff ff e8 62 3a 0b fa 0f 0b e9 > 6e fd ff ff e8 56 3a 0b fa 0f 0b e9 53 ff ff ff e8 4a 3a 0b fa <0f> 0b > e9 94 fd ff ff e8 ae ac 57 fa e9 78 ff ff ff e8 74 ac 57 fa > RSP: 0018:ffffc90000b27a08 EFLAGS: 00010293 > RAX: 0000000000000000 RBX: ffff88806ee3f810 RCX: 0000000000000000 > RDX: ffff8880175e1d40 RSI: ffffffff877177d6 RDI: 0000000000000005 > RBP: 0000000000000002 R08: 0000000000000005 R09: 0000000000000000 > R10: 0000000000000002 R11: 0000000000000000 R12: ffff88806ee3f810 > R13: ffff88806ee3f808 R14: ffff88806ee3e800 R15: ffff88806ee3f800 > FS: 0000000000000000(0000) GS:ffff8880b9a00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 000000c00023dee0 CR3: 0000000074faf000 CR4: 00000000003506f0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Call Trace: > <TASK> > __nsim_dev_port_del+0x1bb/0x240 drivers/net/netdevsim/dev.c:1433 > nsim_dev_port_del_all drivers/net/netdevsim/dev.c:1443 [inline] > nsim_dev_reload_destroy+0x171/0x510 drivers/net/netdevsim/dev.c:1660 > nsim_dev_reload_down+0x6b/0xd0 drivers/net/netdevsim/dev.c:968 > devlink_reload+0x1c4/0x6e0 net/core/devlink.c:4501 > devlink_pernet_pre_exit+0x104/0x1c0 net/core/devlink.c:12615 > ops_pre_exit_list net/core/net_namespace.c:159 [inline] > cleanup_net+0x451/0xb10 net/core/net_namespace.c:594 > process_one_work+0x9bf/0x1710 kernel/workqueue.c:2289 > worker_thread+0x665/0x1080 kernel/workqueue.c:2436 > kthread+0x2e4/0x3a0 kernel/kthread.c:376 > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306 > </TASK>
On Thu, Nov 10, 2022 at 10:04 AM Ido Schimmel <idosch@idosch.org> wrote: > > On Thu, Nov 10, 2022 at 09:21:23AM -0800, Eric Dumazet wrote: > > On Wed, Nov 9, 2022 at 11:54 PM Jiri Pirko <jiri@resnulli.us> wrote: > > > > > > Wed, Nov 09, 2022 at 10:45:36PM CET, kuba@kernel.org wrote: > > > >On Wed, 9 Nov 2022 08:26:10 -0800 Eric Dumazet wrote: > > > >> > On Tue, Nov 08, 2022 at 02:22:08PM +0100, Jiri Pirko wrote: > > > >> > > From: Jiri Pirko <jiri@nvidia.com> > > > >> > > > > > >> > > As the return value is not 0 only in case there is no such notifier > > > >> > > block registered, add a WARN_ON() to yell about it. > > > >> > > > > > >> > > Suggested-by: Ido Schimmel <idosch@idosch.org> > > > >> > > Signed-off-by: Jiri Pirko <jiri@nvidia.com> > > > >> > > > > >> > Reviewed-by: Ido Schimmel <idosch@nvidia.com> > > > >> > > > >> Please consider WARN_ON_ONCE(), or DEBUG_NET_WARN_ON_ONCE() > > > > > > > >Do you have any general guidance on when to pick WARN() vs WARN_ONCE()? > > > >Or should we always prefer _ONCE() going forward? > > > > > > Good question. If so, it should be documented or spotted by checkpatch. > > > > > > > > > > >Let me take the first 2 in, to lower the syzbot volume. > > > > Well, I am not sure what you call 'lower syzbot volume' > > > > netdevsim netdevsim0 netdevsim3 (unregistering): unset [1, 0] type 2 > > family 0 port 6081 - 0 > > ------------[ cut here ]------------ > > WARNING: CPU: 1 PID: 41 at net/core/devlink.c:10001 > > devl_port_unregister+0x2f6/0x390 net/core/devlink.c:10001 > > Hi Eric, > > That's a different bug than the one fixed by this patchset. Should be > fixed by this patch: > > https://patchwork.kernel.org/project/netdevbpf/patch/20221110085150.520800-1-idosch@nvidia.com/ OK, I will dup the new syzbot report, thanks.
diff --git a/net/core/devlink.c b/net/core/devlink.c index ea0b319385fc..6096baf74b00 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -9843,8 +9843,8 @@ void devlink_free(struct devlink *devlink) xa_destroy(&devlink->snapshot_ids); - unregister_netdevice_notifier_net(devlink_net(devlink), - &devlink->netdevice_nb); + WARN_ON(unregister_netdevice_notifier_net(devlink_net(devlink), + &devlink->netdevice_nb)); xa_erase(&devlinks, devlink->index);