diff mbox series

[net-next,1/2] devlink: Break parameter notification sequence to be before/after unload/load driver

Message ID 6d59d527ccbec04615ef0b4a237ea4e27f10cd8d.1627545799.git.leonro@nvidia.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Clean devlink net namespace operations | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 4 of 4 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 1 this patch: 1
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 69 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 1 this patch: 1
netdev/header_inline success Link

Commit Message

Leon Romanovsky July 29, 2021, 8:15 a.m. UTC 
From: Leon Romanovsky <leonro@nvidia.com>

The change of namespaces during devlink reload calls to driver unload
before it accesses devlink parameters. The commands below causes to
use-after-free bug when trying to get flow steering mode.

 * ip netns add n1
 * devlink dev reload pci/0000:00:09.0 netns n1

 ==================================================================
 BUG: KASAN: use-after-free in mlx5_devlink_fs_mode_get+0x96/0xa0 [mlx5_core]
 Read of size 4 at addr ffff888009d04308 by task devlink/275

 CPU: 6 PID: 275 Comm: devlink Not tainted 5.12.0-rc2+ #2853
 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
 Call Trace:
  dump_stack+0x93/0xc2
  print_address_description.constprop.0+0x18/0x140
  ? mlx5_devlink_fs_mode_get+0x96/0xa0 [mlx5_core]
  ? mlx5_devlink_fs_mode_get+0x96/0xa0 [mlx5_core]
  kasan_report.cold+0x7c/0xd8
  ? mlx5_devlink_fs_mode_get+0x96/0xa0 [mlx5_core]
  mlx5_devlink_fs_mode_get+0x96/0xa0 [mlx5_core]
  devlink_nl_param_fill+0x1c8/0xe80
  ? __free_pages_ok+0x37a/0x8a0
  ? devlink_flash_update_timeout_notify+0xd0/0xd0
  ? lock_acquire+0x1a9/0x6d0
  ? fs_reclaim_acquire+0xb7/0x160
  ? lock_is_held_type+0x98/0x110
  ? 0xffffffff81000000
  ? lock_release+0x1f9/0x6c0
  ? fs_reclaim_release+0xa1/0xf0
  ? lock_downgrade+0x6d0/0x6d0
  ? lock_is_held_type+0x98/0x110
  ? lock_is_held_type+0x98/0x110
  ? memset+0x20/0x40
  ? __build_skb_around+0x1f8/0x2b0
  devlink_param_notify+0x6d/0x180
  devlink_reload+0x1c3/0x520
  ? devlink_remote_reload_actions_performed+0x30/0x30
  ? mutex_trylock+0x24b/0x2d0
  ? devlink_nl_cmd_reload+0x62b/0x1070
  devlink_nl_cmd_reload+0x66d/0x1070
  ? devlink_reload+0x520/0x520
  ? devlink_get_from_attrs+0x1bc/0x260
  ? devlink_nl_pre_doit+0x64/0x4d0
  genl_family_rcv_msg_doit+0x1e9/0x2f0
  ? mutex_lock_io_nested+0x1130/0x1130
  ? genl_family_rcv_msg_attrs_parse.constprop.0+0x240/0x240
  ? security_capable+0x51/0x90
  genl_rcv_msg+0x27f/0x4a0
  ? genl_get_cmd+0x3c0/0x3c0
  ? lock_acquire+0x1a9/0x6d0
  ? devlink_reload+0x520/0x520
  ? lock_release+0x6c0/0x6c0
  netlink_rcv_skb+0x11d/0x340
  ? genl_get_cmd+0x3c0/0x3c0
  ? netlink_ack+0x9f0/0x9f0
  ? lock_release+0x1f9/0x6c0
  genl_rcv+0x24/0x40
  netlink_unicast+0x433/0x700
  ? netlink_attachskb+0x730/0x730
  ? _copy_from_iter_full+0x178/0x650
  ? __alloc_skb+0x113/0x2b0
  netlink_sendmsg+0x6f1/0xbd0
  ? netlink_unicast+0x700/0x700
  ? lock_is_held_type+0x98/0x110
  ? netlink_unicast+0x700/0x700
  sock_sendmsg+0xb0/0xe0
  __sys_sendto+0x193/0x240
  ? __x64_sys_getpeername+0xb0/0xb0
  ? do_sys_openat2+0x10b/0x370
  ? __up_read+0x1a1/0x7b0
  ? do_user_addr_fault+0x219/0xdc0
  ? __x64_sys_openat+0x120/0x1d0
  ? __x64_sys_open+0x1a0/0x1a0
  __x64_sys_sendto+0xdd/0x1b0
  ? syscall_enter_from_user_mode+0x1d/0x50
  do_syscall_64+0x2d/0x40
  entry_SYSCALL_64_after_hwframe+0x44/0xae
 RIP: 0033:0x7fc69d0af14a
 Code: d8 64 89 02 48 c7 c0 ff ff ff ff eb b8 0f 1f 00 f3 0f 1e fa 41 89 ca 64 8b 04 25 18 00 00 00 85 c0 75 15 b8 2c 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 76 c3 0f 1f 44 00 00 55 48 83 ec 30 44 89 4c
 RSP: 002b:00007ffc1d8292f8 EFLAGS: 00000246 ORIG_RAX: 000000000000002c
 RAX: ffffffffffffffda RBX: 0000000000000005 RCX: 00007fc69d0af14a
 RDX: 0000000000000038 RSI: 0000555f57c56440 RDI: 0000000000000003
 RBP: 0000555f57c56410 R08: 00007fc69d17b200 R09: 000000000000000c
 R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
 R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000

 Allocated by task 146:
  kasan_save_stack+0x1b/0x40
  __kasan_kmalloc+0x99/0xc0
  mlx5_init_fs+0xf0/0x1c50 [mlx5_core]
  mlx5_load+0xd2/0x180 [mlx5_core]
  mlx5_init_one+0x2f6/0x450 [mlx5_core]
  probe_one+0x47d/0x6e0 [mlx5_core]
  pci_device_probe+0x2a0/0x4a0
  really_probe+0x20a/0xc90
  driver_probe_device+0xd8/0x380
  device_driver_attach+0x1df/0x250
  __driver_attach+0xff/0x240
  bus_for_each_dev+0x11e/0x1a0
  bus_add_driver+0x309/0x570
  driver_register+0x1ee/0x380
  0xffffffffa06b8062
  do_one_initcall+0xd5/0x410
  do_init_module+0x1c8/0x760
  load_module+0x6d8b/0x9650
  __do_sys_finit_module+0x118/0x1b0
  do_syscall_64+0x2d/0x40
  entry_SYSCALL_64_after_hwframe+0x44/0xae

 Freed by task 275:
  kasan_save_stack+0x1b/0x40
  kasan_set_track+0x1c/0x30
  kasan_set_free_info+0x20/0x30
  __kasan_slab_free+0x102/0x140
  slab_free_freelist_hook+0x74/0x1b0
  kfree+0xd7/0x2a0
  mlx5_unload+0x16/0xb0 [mlx5_core]
  mlx5_unload_one+0xae/0x120 [mlx5_core]
  mlx5_devlink_reload_down+0x1bc/0x380 [mlx5_core]
  devlink_reload+0x141/0x520
  devlink_nl_cmd_reload+0x66d/0x1070
  genl_family_rcv_msg_doit+0x1e9/0x2f0
  genl_rcv_msg+0x27f/0x4a0
  netlink_rcv_skb+0x11d/0x340
  genl_rcv+0x24/0x40
  netlink_unicast+0x433/0x700
  netlink_sendmsg+0x6f1/0xbd0
  sock_sendmsg+0xb0/0xe0
  __sys_sendto+0x193/0x240
  __x64_sys_sendto+0xdd/0x1b0
  do_syscall_64+0x2d/0x40
  entry_SYSCALL_64_after_hwframe+0x44/0xae

 The buggy address belongs to the object at ffff888009d04300
  which belongs to the cache kmalloc-128 of size 128
 The buggy address is located 8 bytes inside of
  128-byte region [ffff888009d04300, ffff888009d04380)
 The buggy address belongs to the page:
 page:0000000086a64ecc refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff888009d04000 pfn:0x9d04
 head:0000000086a64ecc order:1 compound_mapcount:0
 flags: 0x4000000000010200(slab|head)
 raw: 4000000000010200 ffffea0000203980 0000000200000002 ffff8880050428c0
 raw: ffff888009d04000 000000008020001d 00000001ffffffff 0000000000000000
 page dumped because: kasan: bad access detected

 Memory state around the buggy address:
  ffff888009d04200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
  ffff888009d04280: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 >ffff888009d04300: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                       ^
  ffff888009d04380: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
  ffff888009d04400: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ==================================================================

The right solution to devlink reload is to notify about deletion of
parameters, unload driver, change net namespaces, load driver and notify
about addition of parameters.

Fixes: 070c63f20f6c ("net: devlink: allow to change namespaces during reload")
Reviewed-by: Parav Pandit <parav@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 net/core/devlink.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

Comments

Jiri Pirko July 29, 2021, 11:22 a.m. UTC | #1 
Thu, Jul 29, 2021 at 10:15:25AM CEST, leon@kernel.org wrote:
>From: Leon Romanovsky <leonro@nvidia.com>
>
>The change of namespaces during devlink reload calls to driver unload
>before it accesses devlink parameters. The commands below causes to
>use-after-free bug when trying to get flow steering mode.
>
> * ip netns add n1
> * devlink dev reload pci/0000:00:09.0 netns n1
>
> ==================================================================
> BUG: KASAN: use-after-free in mlx5_devlink_fs_mode_get+0x96/0xa0 [mlx5_core]
> Read of size 4 at addr ffff888009d04308 by task devlink/275
>
> CPU: 6 PID: 275 Comm: devlink Not tainted 5.12.0-rc2+ #2853
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
> Call Trace:
>  dump_stack+0x93/0xc2
>  print_address_description.constprop.0+0x18/0x140
>  ? mlx5_devlink_fs_mode_get+0x96/0xa0 [mlx5_core]
>  ? mlx5_devlink_fs_mode_get+0x96/0xa0 [mlx5_core]
>  kasan_report.cold+0x7c/0xd8
>  ? mlx5_devlink_fs_mode_get+0x96/0xa0 [mlx5_core]
>  mlx5_devlink_fs_mode_get+0x96/0xa0 [mlx5_core]
>  devlink_nl_param_fill+0x1c8/0xe80
>  ? __free_pages_ok+0x37a/0x8a0
>  ? devlink_flash_update_timeout_notify+0xd0/0xd0
>  ? lock_acquire+0x1a9/0x6d0
>  ? fs_reclaim_acquire+0xb7/0x160
>  ? lock_is_held_type+0x98/0x110
>  ? 0xffffffff81000000
>  ? lock_release+0x1f9/0x6c0
>  ? fs_reclaim_release+0xa1/0xf0
>  ? lock_downgrade+0x6d0/0x6d0
>  ? lock_is_held_type+0x98/0x110
>  ? lock_is_held_type+0x98/0x110
>  ? memset+0x20/0x40
>  ? __build_skb_around+0x1f8/0x2b0
>  devlink_param_notify+0x6d/0x180
>  devlink_reload+0x1c3/0x520
>  ? devlink_remote_reload_actions_performed+0x30/0x30
>  ? mutex_trylock+0x24b/0x2d0
>  ? devlink_nl_cmd_reload+0x62b/0x1070
>  devlink_nl_cmd_reload+0x66d/0x1070
>  ? devlink_reload+0x520/0x520
>  ? devlink_get_from_attrs+0x1bc/0x260
>  ? devlink_nl_pre_doit+0x64/0x4d0
>  genl_family_rcv_msg_doit+0x1e9/0x2f0
>  ? mutex_lock_io_nested+0x1130/0x1130
>  ? genl_family_rcv_msg_attrs_parse.constprop.0+0x240/0x240
>  ? security_capable+0x51/0x90
>  genl_rcv_msg+0x27f/0x4a0
>  ? genl_get_cmd+0x3c0/0x3c0
>  ? lock_acquire+0x1a9/0x6d0
>  ? devlink_reload+0x520/0x520
>  ? lock_release+0x6c0/0x6c0
>  netlink_rcv_skb+0x11d/0x340
>  ? genl_get_cmd+0x3c0/0x3c0
>  ? netlink_ack+0x9f0/0x9f0
>  ? lock_release+0x1f9/0x6c0
>  genl_rcv+0x24/0x40
>  netlink_unicast+0x433/0x700
>  ? netlink_attachskb+0x730/0x730
>  ? _copy_from_iter_full+0x178/0x650
>  ? __alloc_skb+0x113/0x2b0
>  netlink_sendmsg+0x6f1/0xbd0
>  ? netlink_unicast+0x700/0x700
>  ? lock_is_held_type+0x98/0x110
>  ? netlink_unicast+0x700/0x700
>  sock_sendmsg+0xb0/0xe0
>  __sys_sendto+0x193/0x240
>  ? __x64_sys_getpeername+0xb0/0xb0
>  ? do_sys_openat2+0x10b/0x370
>  ? __up_read+0x1a1/0x7b0
>  ? do_user_addr_fault+0x219/0xdc0
>  ? __x64_sys_openat+0x120/0x1d0
>  ? __x64_sys_open+0x1a0/0x1a0
>  __x64_sys_sendto+0xdd/0x1b0
>  ? syscall_enter_from_user_mode+0x1d/0x50
>  do_syscall_64+0x2d/0x40
>  entry_SYSCALL_64_after_hwframe+0x44/0xae
> RIP: 0033:0x7fc69d0af14a
> Code: d8 64 89 02 48 c7 c0 ff ff ff ff eb b8 0f 1f 00 f3 0f 1e fa 41 89 ca 64 8b 04 25 18 00 00 00 85 c0 75 15 b8 2c 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 76 c3 0f 1f 44 00 00 55 48 83 ec 30 44 89 4c
> RSP: 002b:00007ffc1d8292f8 EFLAGS: 00000246 ORIG_RAX: 000000000000002c
> RAX: ffffffffffffffda RBX: 0000000000000005 RCX: 00007fc69d0af14a
> RDX: 0000000000000038 RSI: 0000555f57c56440 RDI: 0000000000000003
> RBP: 0000555f57c56410 R08: 00007fc69d17b200 R09: 000000000000000c
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
>
> Allocated by task 146:
>  kasan_save_stack+0x1b/0x40
>  __kasan_kmalloc+0x99/0xc0
>  mlx5_init_fs+0xf0/0x1c50 [mlx5_core]
>  mlx5_load+0xd2/0x180 [mlx5_core]
>  mlx5_init_one+0x2f6/0x450 [mlx5_core]
>  probe_one+0x47d/0x6e0 [mlx5_core]
>  pci_device_probe+0x2a0/0x4a0
>  really_probe+0x20a/0xc90
>  driver_probe_device+0xd8/0x380
>  device_driver_attach+0x1df/0x250
>  __driver_attach+0xff/0x240
>  bus_for_each_dev+0x11e/0x1a0
>  bus_add_driver+0x309/0x570
>  driver_register+0x1ee/0x380
>  0xffffffffa06b8062
>  do_one_initcall+0xd5/0x410
>  do_init_module+0x1c8/0x760
>  load_module+0x6d8b/0x9650
>  __do_sys_finit_module+0x118/0x1b0
>  do_syscall_64+0x2d/0x40
>  entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> Freed by task 275:
>  kasan_save_stack+0x1b/0x40
>  kasan_set_track+0x1c/0x30
>  kasan_set_free_info+0x20/0x30
>  __kasan_slab_free+0x102/0x140
>  slab_free_freelist_hook+0x74/0x1b0
>  kfree+0xd7/0x2a0
>  mlx5_unload+0x16/0xb0 [mlx5_core]
>  mlx5_unload_one+0xae/0x120 [mlx5_core]
>  mlx5_devlink_reload_down+0x1bc/0x380 [mlx5_core]
>  devlink_reload+0x141/0x520
>  devlink_nl_cmd_reload+0x66d/0x1070
>  genl_family_rcv_msg_doit+0x1e9/0x2f0
>  genl_rcv_msg+0x27f/0x4a0
>  netlink_rcv_skb+0x11d/0x340
>  genl_rcv+0x24/0x40
>  netlink_unicast+0x433/0x700
>  netlink_sendmsg+0x6f1/0xbd0
>  sock_sendmsg+0xb0/0xe0
>  __sys_sendto+0x193/0x240
>  __x64_sys_sendto+0xdd/0x1b0
>  do_syscall_64+0x2d/0x40
>  entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> The buggy address belongs to the object at ffff888009d04300
>  which belongs to the cache kmalloc-128 of size 128
> The buggy address is located 8 bytes inside of
>  128-byte region [ffff888009d04300, ffff888009d04380)
> The buggy address belongs to the page:
> page:0000000086a64ecc refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff888009d04000 pfn:0x9d04
> head:0000000086a64ecc order:1 compound_mapcount:0
> flags: 0x4000000000010200(slab|head)
> raw: 4000000000010200 ffffea0000203980 0000000200000002 ffff8880050428c0
> raw: ffff888009d04000 000000008020001d 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
>  ffff888009d04200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>  ffff888009d04280: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >ffff888009d04300: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>                       ^
>  ffff888009d04380: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>  ffff888009d04400: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ==================================================================
>
>The right solution to devlink reload is to notify about deletion of
>parameters, unload driver, change net namespaces, load driver and notify
>about addition of parameters.
>
>Fixes: 070c63f20f6c ("net: devlink: allow to change namespaces during reload")
>Reviewed-by: Parav Pandit <parav@nvidia.com>
>Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>---
> net/core/devlink.c | 32 ++++++++++++++++++++------------
> 1 file changed, 20 insertions(+), 12 deletions(-)
>
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index b596a971b473..54e2a0375539 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -3801,8 +3801,9 @@ static void devlink_param_notify(struct devlink *devlink,
> 				 struct devlink_param_item *param_item,
> 				 enum devlink_command cmd);
> 
>-static void devlink_reload_netns_change(struct devlink *devlink,
>-					struct net *dest_net)
>+static void devlink_params_notify(struct devlink *devlink, struct net *dest_net,

Please name it differently. This function notifies not only the params,
but the devlink instance itself as well.


>+				  struct net *curr_net,
>+				  enum devlink_command cmd)
> {
> 	struct devlink_param_item *param_item;
> 
>@@ -3812,17 +3813,17 @@ static void devlink_reload_netns_change(struct devlink *devlink,
> 	 * reload process so the notifications are generated separatelly.
> 	 */
> 
>-	list_for_each_entry(param_item, &devlink->param_list, list)
>-		devlink_param_notify(devlink, 0, param_item,
>-				     DEVLINK_CMD_PARAM_DEL);
>-	devlink_notify(devlink, DEVLINK_CMD_DEL);
>+	if (!dest_net || net_eq(dest_net, curr_net))
>+		return;
> 
>-	__devlink_net_set(devlink, dest_net);
>+	if (cmd == DEVLINK_CMD_PARAM_NEW)
>+		devlink_notify(devlink, DEVLINK_CMD_NEW);

This is quite odd. According to PARAMS cmd you decife devlink CMD.

Just have bool arg which would help you select both
DEVLINK_CMD_PARAM_NEW/DEL and DEVLINK_CMD_NEW/DEL.


> 
>-	devlink_notify(devlink, DEVLINK_CMD_NEW);
> 	list_for_each_entry(param_item, &devlink->param_list, list)
>-		devlink_param_notify(devlink, 0, param_item,
>-				     DEVLINK_CMD_PARAM_NEW);
>+		devlink_param_notify(devlink, 0, param_item, cmd);
>+
>+	if (cmd == DEVLINK_CMD_PARAM_DEL)
>+		devlink_notify(devlink, DEVLINK_CMD_DEL);
> }
> 
> static bool devlink_reload_supported(const struct devlink_ops *ops)
>@@ -3902,6 +3903,7 @@ static int devlink_reload(struct devlink *devlink, struct net *dest_net,
> 			  u32 *actions_performed, struct netlink_ext_ack *extack)
> {
> 	u32 remote_reload_stats[DEVLINK_RELOAD_STATS_ARRAY_SIZE];
>+	struct net *curr_net;
> 	int err;
> 
> 	if (!devlink->reload_enabled)
>@@ -3909,18 +3911,24 @@ static int devlink_reload(struct devlink *devlink, struct net *dest_net,
> 
> 	memcpy(remote_reload_stats, devlink->stats.remote_reload_stats,
> 	       sizeof(remote_reload_stats));
>+
>+	curr_net = devlink_net(devlink);
>+	devlink_params_notify(devlink, dest_net, curr_net,
>+			      DEVLINK_CMD_PARAM_DEL);
> 	err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack);
> 	if (err)
> 		return err;
> 
>-	if (dest_net && !net_eq(dest_net, devlink_net(devlink)))
>-		devlink_reload_netns_change(devlink, dest_net);
>+	if (dest_net && !net_eq(dest_net, curr_net))
>+		__devlink_net_set(devlink, dest_net);
> 
> 	err = devlink->ops->reload_up(devlink, action, limit, actions_performed, extack);
> 	devlink_reload_failed_set(devlink, !!err);
> 	if (err)
> 		return err;
> 
>+	devlink_params_notify(devlink, dest_net, curr_net,
>+			      DEVLINK_CMD_PARAM_NEW);
> 	WARN_ON(!(*actions_performed & BIT(action)));
> 	/* Catch driver on updating the remote action within devlink reload */
> 	WARN_ON(memcmp(remote_reload_stats, devlink->stats.remote_reload_stats,
>-- 
>2.31.1
>
Leon Romanovsky July 29, 2021, 11:35 a.m. UTC | #2 
On Thu, Jul 29, 2021 at 01:22:58PM +0200, Jiri Pirko wrote:
> Thu, Jul 29, 2021 at 10:15:25AM CEST, leon@kernel.org wrote:
> >From: Leon Romanovsky <leonro@nvidia.com>

<...>

> >diff --git a/net/core/devlink.c b/net/core/devlink.c
> >index b596a971b473..54e2a0375539 100644
> >--- a/net/core/devlink.c
> >+++ b/net/core/devlink.c
> >@@ -3801,8 +3801,9 @@ static void devlink_param_notify(struct devlink *devlink,
> > 				 struct devlink_param_item *param_item,
> > 				 enum devlink_command cmd);
> > 
> >-static void devlink_reload_netns_change(struct devlink *devlink,
> >-					struct net *dest_net)
> >+static void devlink_params_notify(struct devlink *devlink, struct net *dest_net,
> 
> Please name it differently. This function notifies not only the params,
> but the devlink instance itself as well.

I'm open for suggestion. What did you have in mind?

> 
> 
> >+				  struct net *curr_net,
> >+				  enum devlink_command cmd)
> > {
> > 	struct devlink_param_item *param_item;
> > 
> >@@ -3812,17 +3813,17 @@ static void devlink_reload_netns_change(struct devlink *devlink,
> > 	 * reload process so the notifications are generated separatelly.
> > 	 */
> > 
> >-	list_for_each_entry(param_item, &devlink->param_list, list)
> >-		devlink_param_notify(devlink, 0, param_item,
> >-				     DEVLINK_CMD_PARAM_DEL);
> >-	devlink_notify(devlink, DEVLINK_CMD_DEL);
> >+	if (!dest_net || net_eq(dest_net, curr_net))
> >+		return;
> > 
> >-	__devlink_net_set(devlink, dest_net);
> >+	if (cmd == DEVLINK_CMD_PARAM_NEW)
> >+		devlink_notify(devlink, DEVLINK_CMD_NEW);
> 
> This is quite odd. According to PARAMS cmd you decife devlink CMD.
> 
> Just have bool arg which would help you select both
> DEVLINK_CMD_PARAM_NEW/DEL and DEVLINK_CMD_NEW/DEL

The patch is quite misleading, but the final result looks neat:

   3847 static void devlink_params_notify(struct devlink *devlink, struct net *dest_net,
   3848                                   struct net *curr_net,
   3849                                   enum devlink_command cmd)
   3850 {
   3851         struct devlink_param_item *param_item;
   3852 
   3853         /* Userspace needs to be notified about devlink objects
   3854          * removed from original and entering new network namespace.
   3855          * The rest of the devlink objects are re-created during
   3856          * reload process so the notifications are generated separatelly.
   3857          */
   3858 
   3859         if (!dest_net || net_eq(dest_net, curr_net))
   3860                 return;
   3861 
   3862         if (cmd == DEVLINK_CMD_PARAM_NEW)
   3863                 devlink_notify(devlink, DEVLINK_CMD_NEW);
   3864 
   3865         list_for_each_entry(param_item, &devlink->param_list, list)
   3866                 devlink_param_notify(devlink, 0, param_item, cmd);
   3867 
   3868         if (cmd == DEVLINK_CMD_PARAM_DEL)
   3869                 devlink_notify(devlink, DEVLINK_CMD_DEL);
   3870 }


So as you can see in line 3866, we anyway will need to provide "cmd", so
do you suggest to add extra two bool variables to the function signature
to avoid "cmd == DEVLINK_CMD_PARAM_NEW" and "cmd == DEVLINK_CMD_PARAM_DEL" ifs?

Thanks
Jiri Pirko July 29, 2021, 11:57 a.m. UTC | #3 
Thu, Jul 29, 2021 at 01:35:55PM CEST, leon@kernel.org wrote:
>On Thu, Jul 29, 2021 at 01:22:58PM +0200, Jiri Pirko wrote:
>> Thu, Jul 29, 2021 at 10:15:25AM CEST, leon@kernel.org wrote:
>> >From: Leon Romanovsky <leonro@nvidia.com>
>
><...>
>
>> >diff --git a/net/core/devlink.c b/net/core/devlink.c
>> >index b596a971b473..54e2a0375539 100644
>> >--- a/net/core/devlink.c
>> >+++ b/net/core/devlink.c
>> >@@ -3801,8 +3801,9 @@ static void devlink_param_notify(struct devlink *devlink,
>> > 				 struct devlink_param_item *param_item,
>> > 				 enum devlink_command cmd);
>> > 
>> >-static void devlink_reload_netns_change(struct devlink *devlink,
>> >-					struct net *dest_net)
>> >+static void devlink_params_notify(struct devlink *devlink, struct net *dest_net,
>> 
>> Please name it differently. This function notifies not only the params,
>> but the devlink instance itself as well.
>
>I'm open for suggestion. What did you have in mind?

devlink_ns_change_notify?

>
>> 
>> 
>> >+				  struct net *curr_net,
>> >+				  enum devlink_command cmd)
>> > {
>> > 	struct devlink_param_item *param_item;
>> > 
>> >@@ -3812,17 +3813,17 @@ static void devlink_reload_netns_change(struct devlink *devlink,
>> > 	 * reload process so the notifications are generated separatelly.
>> > 	 */
>> > 
>> >-	list_for_each_entry(param_item, &devlink->param_list, list)
>> >-		devlink_param_notify(devlink, 0, param_item,
>> >-				     DEVLINK_CMD_PARAM_DEL);
>> >-	devlink_notify(devlink, DEVLINK_CMD_DEL);
>> >+	if (!dest_net || net_eq(dest_net, curr_net))
>> >+		return;
>> > 
>> >-	__devlink_net_set(devlink, dest_net);
>> >+	if (cmd == DEVLINK_CMD_PARAM_NEW)
>> >+		devlink_notify(devlink, DEVLINK_CMD_NEW);
>> 
>> This is quite odd. According to PARAMS cmd you decife devlink CMD.
>> 
>> Just have bool arg which would help you select both
>> DEVLINK_CMD_PARAM_NEW/DEL and DEVLINK_CMD_NEW/DEL
>
>The patch is quite misleading, but the final result looks neat:
>
>   3847 static void devlink_params_notify(struct devlink *devlink, struct net *dest_net,
>   3848                                   struct net *curr_net,
>   3849                                   enum devlink_command cmd)
>   3850 {
>   3851         struct devlink_param_item *param_item;
>   3852 
>   3853         /* Userspace needs to be notified about devlink objects
>   3854          * removed from original and entering new network namespace.
>   3855          * The rest of the devlink objects are re-created during
>   3856          * reload process so the notifications are generated separatelly.
>   3857          */
>   3858 
>   3859         if (!dest_net || net_eq(dest_net, curr_net))
>   3860                 return;
>   3861 
>   3862         if (cmd == DEVLINK_CMD_PARAM_NEW)
>   3863                 devlink_notify(devlink, DEVLINK_CMD_NEW);

Nothing misleading here. This is exactly what I'm pointing out...



>   3864 
>   3865         list_for_each_entry(param_item, &devlink->param_list, list)
>   3866                 devlink_param_notify(devlink, 0, param_item, cmd);
>   3867 
>   3868         if (cmd == DEVLINK_CMD_PARAM_DEL)
>   3869                 devlink_notify(devlink, DEVLINK_CMD_DEL);
>   3870 }
>
>
>So as you can see in line 3866, we anyway will need to provide "cmd", so
>do you suggest to add extra two bool variables to the function signature
>to avoid "cmd == DEVLINK_CMD_PARAM_NEW" and "cmd == DEVLINK_CMD_PARAM_DEL" ifs?
>
>Thanks
Leon Romanovsky July 29, 2021, 12:11 p.m. UTC | #4 
On Thu, Jul 29, 2021 at 01:57:01PM +0200, Jiri Pirko wrote:
> Thu, Jul 29, 2021 at 01:35:55PM CEST, leon@kernel.org wrote:
> >On Thu, Jul 29, 2021 at 01:22:58PM +0200, Jiri Pirko wrote:
> >> Thu, Jul 29, 2021 at 10:15:25AM CEST, leon@kernel.org wrote:
> >> >From: Leon Romanovsky <leonro@nvidia.com>
> >
> ><...>
> >
> >> >diff --git a/net/core/devlink.c b/net/core/devlink.c
> >> >index b596a971b473..54e2a0375539 100644
> >> >--- a/net/core/devlink.c
> >> >+++ b/net/core/devlink.c
> >> >@@ -3801,8 +3801,9 @@ static void devlink_param_notify(struct devlink *devlink,
> >> > 				 struct devlink_param_item *param_item,
> >> > 				 enum devlink_command cmd);
> >> > 
> >> >-static void devlink_reload_netns_change(struct devlink *devlink,
> >> >-					struct net *dest_net)
> >> >+static void devlink_params_notify(struct devlink *devlink, struct net *dest_net,
> >> 
> >> Please name it differently. This function notifies not only the params,
> >> but the devlink instance itself as well.
> >
> >I'm open for suggestion. What did you have in mind?
> 
> devlink_ns_change_notify?

NP, will change 

> 
> >
> >> 
> >> 
> >> >+				  struct net *curr_net,
> >> >+				  enum devlink_command cmd)
> >> > {
> >> > 	struct devlink_param_item *param_item;
> >> > 
> >> >@@ -3812,17 +3813,17 @@ static void devlink_reload_netns_change(struct devlink *devlink,
> >> > 	 * reload process so the notifications are generated separatelly.
> >> > 	 */
> >> > 
> >> >-	list_for_each_entry(param_item, &devlink->param_list, list)
> >> >-		devlink_param_notify(devlink, 0, param_item,
> >> >-				     DEVLINK_CMD_PARAM_DEL);
> >> >-	devlink_notify(devlink, DEVLINK_CMD_DEL);
> >> >+	if (!dest_net || net_eq(dest_net, curr_net))
> >> >+		return;
> >> > 
> >> >-	__devlink_net_set(devlink, dest_net);
> >> >+	if (cmd == DEVLINK_CMD_PARAM_NEW)
> >> >+		devlink_notify(devlink, DEVLINK_CMD_NEW);
> >> 
> >> This is quite odd. According to PARAMS cmd you decife devlink CMD.
> >> 
> >> Just have bool arg which would help you select both
> >> DEVLINK_CMD_PARAM_NEW/DEL and DEVLINK_CMD_NEW/DEL
> >
> >The patch is quite misleading, but the final result looks neat:
> >
> >   3847 static void devlink_params_notify(struct devlink *devlink, struct net *dest_net,
> >   3848                                   struct net *curr_net,
> >   3849                                   enum devlink_command cmd)
> >   3850 {
> >   3851         struct devlink_param_item *param_item;
> >   3852 
> >   3853         /* Userspace needs to be notified about devlink objects
> >   3854          * removed from original and entering new network namespace.
> >   3855          * The rest of the devlink objects are re-created during
> >   3856          * reload process so the notifications are generated separatelly.
> >   3857          */
> >   3858 
> >   3859         if (!dest_net || net_eq(dest_net, curr_net))
> >   3860                 return;
> >   3861 
> >   3862         if (cmd == DEVLINK_CMD_PARAM_NEW)
> >   3863                 devlink_notify(devlink, DEVLINK_CMD_NEW);
> 
> Nothing misleading here. This is exactly what I'm pointing out...

Do you ask for this change?

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 077310c26a8b..ee7204787b29 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3846,7 +3846,7 @@ static void devlink_param_notify(struct devlink *devlink,
 
 static void devlink_params_notify(struct devlink *devlink, struct net *dest_net,
                                  struct net *curr_net,
-                                 enum devlink_command cmd)
+                                 enum devlink_command cmd, bool new)
 {
        struct devlink_param_item *param_item;
 
@@ -3859,13 +3859,13 @@ static void devlink_params_notify(struct devlink *devlink, struct net *dest_net,
        if (!dest_net || net_eq(dest_net, curr_net))
                return;
 
-       if (cmd == DEVLINK_CMD_PARAM_NEW)
+       if (new)
                devlink_notify(devlink, DEVLINK_CMD_NEW);
 
        list_for_each_entry(param_item, &devlink->param_list, list)
                devlink_param_notify(devlink, 0, param_item, cmd);
 
-       if (cmd == DEVLINK_CMD_PARAM_DEL)
+       if (!new)
                devlink_notify(devlink, DEVLINK_CMD_DEL);
 }
 
@@ -3957,7 +3957,7 @@ static int devlink_reload(struct devlink *devlink, struct net *dest_net,
 
        curr_net = devlink_net(devlink);
        devlink_params_notify(devlink, dest_net, curr_net,
-                             DEVLINK_CMD_PARAM_DEL);
+                             DEVLINK_CMD_PARAM_DEL, false);
        err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack);
        if (err)
                return err;
@@ -3971,7 +3971,7 @@ static int devlink_reload(struct devlink *devlink, struct net *dest_net,
                return err;
 
        devlink_params_notify(devlink, dest_net, curr_net,
-                             DEVLINK_CMD_PARAM_NEW);
+                             DEVLINK_CMD_PARAM_NEW, true);
        WARN_ON(!(*actions_performed & BIT(action)));
        /* Catch driver on updating the remote action within devlink reload */
        WARN_ON(memcmp(remote_reload_stats, devlink->stats.remote_reload_stats,


> 
> 
> 
> >   3864 
> >   3865         list_for_each_entry(param_item, &devlink->param_list, list)
> >   3866                 devlink_param_notify(devlink, 0, param_item, cmd);
> >   3867 
> >   3868         if (cmd == DEVLINK_CMD_PARAM_DEL)
> >   3869                 devlink_notify(devlink, DEVLINK_CMD_DEL);
> >   3870 }
> >
> >
> >So as you can see in line 3866, we anyway will need to provide "cmd", so
> >do you suggest to add extra two bool variables to the function signature
> >to avoid "cmd == DEVLINK_CMD_PARAM_NEW" and "cmd == DEVLINK_CMD_PARAM_DEL" ifs?
> >
> >Thanks
diff mbox series

Patch

diff --git a/net/core/devlink.c b/net/core/devlink.c
index b596a971b473..54e2a0375539 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3801,8 +3801,9 @@  static void devlink_param_notify(struct devlink *devlink,
 				 struct devlink_param_item *param_item,
 				 enum devlink_command cmd);
 
-static void devlink_reload_netns_change(struct devlink *devlink,
-					struct net *dest_net)
+static void devlink_params_notify(struct devlink *devlink, struct net *dest_net,
+				  struct net *curr_net,
+				  enum devlink_command cmd)
 {
 	struct devlink_param_item *param_item;
 
@@ -3812,17 +3813,17 @@  static void devlink_reload_netns_change(struct devlink *devlink,
 	 * reload process so the notifications are generated separatelly.
 	 */
 
-	list_for_each_entry(param_item, &devlink->param_list, list)
-		devlink_param_notify(devlink, 0, param_item,
-				     DEVLINK_CMD_PARAM_DEL);
-	devlink_notify(devlink, DEVLINK_CMD_DEL);
+	if (!dest_net || net_eq(dest_net, curr_net))
+		return;
 
-	__devlink_net_set(devlink, dest_net);
+	if (cmd == DEVLINK_CMD_PARAM_NEW)
+		devlink_notify(devlink, DEVLINK_CMD_NEW);
 
-	devlink_notify(devlink, DEVLINK_CMD_NEW);
 	list_for_each_entry(param_item, &devlink->param_list, list)
-		devlink_param_notify(devlink, 0, param_item,
-				     DEVLINK_CMD_PARAM_NEW);
+		devlink_param_notify(devlink, 0, param_item, cmd);
+
+	if (cmd == DEVLINK_CMD_PARAM_DEL)
+		devlink_notify(devlink, DEVLINK_CMD_DEL);
 }
 
 static bool devlink_reload_supported(const struct devlink_ops *ops)
@@ -3902,6 +3903,7 @@  static int devlink_reload(struct devlink *devlink, struct net *dest_net,
 			  u32 *actions_performed, struct netlink_ext_ack *extack)
 {
 	u32 remote_reload_stats[DEVLINK_RELOAD_STATS_ARRAY_SIZE];
+	struct net *curr_net;
 	int err;
 
 	if (!devlink->reload_enabled)
@@ -3909,18 +3911,24 @@  static int devlink_reload(struct devlink *devlink, struct net *dest_net,
 
 	memcpy(remote_reload_stats, devlink->stats.remote_reload_stats,
 	       sizeof(remote_reload_stats));
+
+	curr_net = devlink_net(devlink);
+	devlink_params_notify(devlink, dest_net, curr_net,
+			      DEVLINK_CMD_PARAM_DEL);
 	err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack);
 	if (err)
 		return err;
 
-	if (dest_net && !net_eq(dest_net, devlink_net(devlink)))
-		devlink_reload_netns_change(devlink, dest_net);
+	if (dest_net && !net_eq(dest_net, curr_net))
+		__devlink_net_set(devlink, dest_net);
 
 	err = devlink->ops->reload_up(devlink, action, limit, actions_performed, extack);
 	devlink_reload_failed_set(devlink, !!err);
 	if (err)
 		return err;
 
+	devlink_params_notify(devlink, dest_net, curr_net,
+			      DEVLINK_CMD_PARAM_NEW);
 	WARN_ON(!(*actions_performed & BIT(action)));
 	/* Catch driver on updating the remote action within devlink reload */
 	WARN_ON(memcmp(remote_reload_stats, devlink->stats.remote_reload_stats,