diff mbox series

[net-next,1/3] devlink: move devlink reload notifications back in between _down() and _up() calls

Message ID 20230127155042.1846608-2-jiri@resnulli.us (mailing list archive)
State Accepted
Commit 7d7e9169a3ecdc14a4921b2130066ce26952c9e1
Delegated to: Netdev Maintainers
Headers show
Series devlink: fix reload notifications and remove features | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
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: 1 this patch: 1
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, 70 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jiri Pirko Jan. 27, 2023, 3:50 p.m. UTC 
From: Jiri Pirko <jiri@nvidia.com>

This effectively reverts commit 05a7f4a8dff1 ("devlink: Break parameter
notification sequence to be before/after unload/load driver").

Cited commit resolved a problem in mlx5 params implementation,
when param notification code accessed memory previously freed
during reload.

Now, when the params can be registered and unregistered when devlink
instance is registered, mlx5 code unregisters the problematic param
during devlink reload. The fix is therefore no longer needed.

Current behavior is a it problematic, as it sends DEL notifications even
in potential case when reload_down() call fails which might confuse
userspace notifications listener.

So move the reload notifications back where they were originally in
between reload_down() and reload_up() calls.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/devlink/leftover.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

Comments

Jacob Keller Jan. 28, 2023, 12:08 a.m. UTC | #1 
On 1/27/2023 7:50 AM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> This effectively reverts commit 05a7f4a8dff1 ("devlink: Break parameter
> notification sequence to be before/after unload/load driver").
> 
> Cited commit resolved a problem in mlx5 params implementation,
> when param notification code accessed memory previously freed
> during reload.
> 
> Now, when the params can be registered and unregistered when devlink
> instance is registered, mlx5 code unregisters the problematic param
> during devlink reload. The fix is therefore no longer needed.
> 
> Current behavior is a it problematic, as it sends DEL notifications even
> in potential case when reload_down() call fails which might confuse
> userspace notifications listener.
> 
> So move the reload notifications back where they were originally in
> between reload_down() and reload_up() calls.
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>  net/devlink/leftover.c | 37 ++++++++++++++++---------------------
>  1 file changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
> index bd4c5d2dd612..24e20861a28b 100644
> --- a/net/devlink/leftover.c
> +++ b/net/devlink/leftover.c
> @@ -4235,12 +4235,11 @@ static void devlink_param_notify(struct devlink *devlink,
>  				 struct devlink_param_item *param_item,
>  				 enum devlink_command cmd);
>  
> -static void devlink_ns_change_notify(struct devlink *devlink,
> -				     struct net *dest_net, struct net *curr_net,
> -				     bool new)
> +static void devlink_reload_netns_change(struct devlink *devlink,
> +					struct net *curr_net,
> +					struct net *dest_net)
>  {
>  	struct devlink_param_item *param_item;
> -	enum devlink_command cmd;
>  
>  	/* Userspace needs to be notified about devlink objects
>  	 * removed from original and entering new network namespace.
> @@ -4248,18 +4247,19 @@ static void devlink_ns_change_notify(struct devlink *devlink,
>  	 * reload process so the notifications are generated separatelly.
>  	 */
>  
> -	if (!dest_net || net_eq(dest_net, curr_net))
> -		return;
> +	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 (new)
> -		devlink_notify(devlink, DEVLINK_CMD_NEW);
> +	move_netdevice_notifier_net(curr_net, dest_net,
> +				    &devlink->netdevice_nb);
> +	write_pnet(&devlink->_net, dest_net);
>  
> -	cmd = new ? DEVLINK_CMD_PARAM_NEW : DEVLINK_CMD_PARAM_DEL;
> +	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 (!new)
> -		devlink_notify(devlink, DEVLINK_CMD_DEL);
> +		devlink_param_notify(devlink, 0, param_item,
> +				     DEVLINK_CMD_PARAM_NEW);
>  }
>  
>  static void devlink_reload_failed_set(struct devlink *devlink,
> @@ -4341,24 +4341,19 @@ 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_ns_change_notify(devlink, dest_net, curr_net, false);
>  	err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack);
>  	if (err)
>  		return err;
>  
> -	if (dest_net && !net_eq(dest_net, curr_net)) {
> -		move_netdevice_notifier_net(curr_net, dest_net,
> -					    &devlink->netdevice_nb);
> -		write_pnet(&devlink->_net, dest_net);
> -	}
> +	curr_net = devlink_net(devlink);
> +	if (dest_net && !net_eq(dest_net, curr_net))
> +		devlink_reload_netns_change(devlink, curr_net, dest_net);
>  
>  	err = devlink->ops->reload_up(devlink, action, limit, actions_performed, extack);
>  	devlink_reload_failed_set(devlink, !!err);
>  	if (err)
>  		return err;
>  
> -	devlink_ns_change_notify(devlink, dest_net, curr_net, 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,
diff mbox series

Patch

diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index bd4c5d2dd612..24e20861a28b 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -4235,12 +4235,11 @@  static void devlink_param_notify(struct devlink *devlink,
 				 struct devlink_param_item *param_item,
 				 enum devlink_command cmd);
 
-static void devlink_ns_change_notify(struct devlink *devlink,
-				     struct net *dest_net, struct net *curr_net,
-				     bool new)
+static void devlink_reload_netns_change(struct devlink *devlink,
+					struct net *curr_net,
+					struct net *dest_net)
 {
 	struct devlink_param_item *param_item;
-	enum devlink_command cmd;
 
 	/* Userspace needs to be notified about devlink objects
 	 * removed from original and entering new network namespace.
@@ -4248,18 +4247,19 @@  static void devlink_ns_change_notify(struct devlink *devlink,
 	 * reload process so the notifications are generated separatelly.
 	 */
 
-	if (!dest_net || net_eq(dest_net, curr_net))
-		return;
+	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 (new)
-		devlink_notify(devlink, DEVLINK_CMD_NEW);
+	move_netdevice_notifier_net(curr_net, dest_net,
+				    &devlink->netdevice_nb);
+	write_pnet(&devlink->_net, dest_net);
 
-	cmd = new ? DEVLINK_CMD_PARAM_NEW : DEVLINK_CMD_PARAM_DEL;
+	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 (!new)
-		devlink_notify(devlink, DEVLINK_CMD_DEL);
+		devlink_param_notify(devlink, 0, param_item,
+				     DEVLINK_CMD_PARAM_NEW);
 }
 
 static void devlink_reload_failed_set(struct devlink *devlink,
@@ -4341,24 +4341,19 @@  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_ns_change_notify(devlink, dest_net, curr_net, false);
 	err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack);
 	if (err)
 		return err;
 
-	if (dest_net && !net_eq(dest_net, curr_net)) {
-		move_netdevice_notifier_net(curr_net, dest_net,
-					    &devlink->netdevice_nb);
-		write_pnet(&devlink->_net, dest_net);
-	}
+	curr_net = devlink_net(devlink);
+	if (dest_net && !net_eq(dest_net, curr_net))
+		devlink_reload_netns_change(devlink, curr_net, dest_net);
 
 	err = devlink->ops->reload_up(devlink, action, limit, actions_performed, extack);
 	devlink_reload_failed_set(devlink, !!err);
 	if (err)
 		return err;
 
-	devlink_ns_change_notify(devlink, dest_net, curr_net, 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,