Re: [patch net-next 2/2] net: devlink: move netdev notifier block to dest namespace during reload

[Ido Schimmel <idosch@idosch.org>]

On Mon, Nov 07, 2022 at 03:52:13PM +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> The notifier block tracking netdev changes in devlink is registered
> during devlink_alloc() per-net, it is then unregistered
> in devlink_free(). When devlink moves from net namespace to another one,
> the notifier block needs to move along.
> 
> Fix this by adding forgotten call to move the block.
> 
> Reported-by: Ido Schimmel <idosch@idosch.org>
> Fixes: 02a68a47eade ("net: devlink: track netdev with devlink_port assigned")
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>

Does not trigger with my reproducer. Will test the fix tonight in
regression and report tomorrow morning.

> ---
>  net/core/devlink.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 40fcdded57e6..ea0b319385fc 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -4502,8 +4502,11 @@ static int devlink_reload(struct devlink *devlink, struct net *dest_net,
>  	if (err)
>  		return err;
>  
> -	if (dest_net && !net_eq(dest_net, curr_net))
> +	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);
> +	}

I suggest adding this:

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 83fd10aeddd5..3b5aedc93335 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);

This tells about the failure right away. Instead, we saw random memory
corruptions in later tests.

>  
>  	err = devlink->ops->reload_up(devlink, action, limit, actions_performed, extack);
>  	devlink_reload_failed_set(devlink, !!err);
> -- 
> 2.37.3
>