netdev - Re: [PATCH net 4/4] net/mlx5e: Keep netdev when leave switchdev for devlink set legacy only

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ec95c546-114d-402f-b7b9-b3e54b33dbf0@intel.com>
Date: Fri, 20 Dec 2024 09:48:11 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Tariq Toukan <tariqt@...dia.com>, Jianbo Liu <jianbol@...dia.com>
CC: <netdev@...r.kernel.org>, Saeed Mahameed <saeedm@...dia.com>, Gal Pressman
	<gal@...dia.com>, Leon Romanovsky <leonro@...dia.com>, Mark Bloch
	<mbloch@...dia.com>, "David S. Miller" <davem@...emloft.net>, Jakub Kicinski
	<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Eric Dumazet
	<edumazet@...gle.com>, Andrew Lunn <andrew+netdev@...n.ch>
Subject: Re: [PATCH net 4/4] net/mlx5e: Keep netdev when leave switchdev for
 devlink set legacy only

On 12/20/24 09:15, Tariq Toukan wrote:
> From: Jianbo Liu <jianbol@...dia.com>
> 
> In the cited commit, when changing from switchdev to legacy mode,
> uplink representor's netdev is kept, and its profile is replaced with
> nic profile, so netdev is detached from old profile, then attach to
> new profile.
> 
> During profile change, the hardware resources allocated by the old
> profile will be cleaned up. However, the cleanup is relying on the
> related kernel modules. And they may need to flush themselves first,
> which is triggered by netdev events, for example, NETDEV_UNREGISTER.
> However, netdev is kept, or netdev_register is called after the
> cleanup, which may cause troubles because the resources are still
> referred by kernel modules.
> 
> The same process applies to all the caes when uplink is leaving

case

> switchdev mode, including devlink eswitch mode set legacy, driver
> unload and devlink reload. For the first one, it can be blocked and
> returns failure to users, whenever possible. But it's hard for the
> others. Besides, the attachment to nic profile is unnecessary as the
> netdev will be unregistered anyway for such cases.
> 
> So in this patch, the original behavior is kept only for devlink
> eswitch set mode legacy. For the others, moves netdev unregistration
> before the profile change.
> 
> Fixes: 7a9fb35e8c3a ("net/mlx5e: Do not reload ethernet ports when changing eswitch mode")
> Signed-off-by: Jianbo Liu <jianbol@...dia.com>
> Signed-off-by: Tariq Toukan <tariqt@...dia.com>
> ---
>   .../net/ethernet/mellanox/mlx5/core/en_main.c | 19 +++++++++++++++++--
>   .../net/ethernet/mellanox/mlx5/core/en_rep.c  | 15 +++++++++++++++
>   .../mellanox/mlx5/core/eswitch_offloads.c     |  2 ++
>   include/linux/mlx5/driver.h                   |  1 +
>   4 files changed, 35 insertions(+), 2 deletions(-)
> 

sorry for nitpick-only review
I didn't spotted anything bad in the logic through the series OTOH

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index dd16d73000c3..0ec17c276bdd 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -6542,8 +6542,23 @@ static void _mlx5e_remove(struct auxiliary_device *adev)
>   
>   	mlx5_core_uplink_netdev_set(mdev, NULL);
>   	mlx5e_dcbnl_delete_app(priv);
> -	unregister_netdev(priv->netdev);
> -	_mlx5e_suspend(adev, false);
> +	/* When unload driver, the netdev is in registered state

/*
  * Netdev dropped the special comment allowance rule,
  * now you have to put one line almost blank at the front.
  */

> +	 * if it's from legacy mode. If from switchdev mode, it
> +	 * is already unregistered before changing to NIC profile.
> +	 */
> +	if (priv->netdev->reg_state == NETREG_REGISTERED) {
> +		unregister_netdev(priv->netdev);
> +		_mlx5e_suspend(adev, false);
> +	} else {
> +		struct mlx5_core_dev *pos;
> +		int i;
> +
> +		if (test_bit(MLX5E_STATE_DESTROYING, &priv->state))

you have more than one statement/expression inside the if,
so you must wrap with braces

> +			mlx5_sd_for_each_dev(i, mdev, pos)
> +				mlx5e_destroy_mdev_resources(pos);
> +		else
> +			_mlx5e_suspend(adev, true);
> +	}
>   	/* Avoid cleanup if profile rollback failed. */
>   	if (priv->profile)
>   		priv->profile->cleanup(priv);