Re: [PATCH net V3 3/4] net/mlx5e: SD, Fix missing cleanup on probe/resume error

Shay Drori
2026-04-26T10:46:19Z

On 23/04/2026 15:31, Tariq Toukan wrote:
> From: Shay Drory <shayd@nvidia.com>
>
> When _mlx5e_probe() or _mlx5e_resume() fails, the preceding
> successful mlx5_sd_init() is not undone, leaving the SD group in an
> UP state without a matching cleanup.
>
> Call mlx5_sd_cleanup() on the error path so the SD state is reset.
>
> Fixes: 381978d28317 ("net/mlx5e: Create single netdev per SD group")
> Signed-off-by: Shay Drory <shayd@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> ---
> .../net/ethernet/mellanox/mlx5/core/en_main.c | 22 +++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index 5a46870c4b74..9c340ad2fe09 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -6774,9 +6774,16 @@ static int mlx5e_resume(struct auxiliary_device *adev)
> return err;
>
> actual_adev = mlx5_sd_get_adev(mdev, adev, edev->idx);
> - if (actual_adev)
> - return _mlx5e_resume(actual_adev);
> + if (actual_adev) {
> + err = _mlx5e_resume(actual_adev);
> + if (err)
> + goto sd_cleanup;
> + }
> return 0;
> +
> +sd_cleanup:
> + mlx5_sd_cleanup(mdev);
> + return err;
> }

Sashiko:
"If _mlx5e_resume() fails when called from a secondary device's
mlx5e_resume(),
mlx5_sd_cleanup() is called on the secondary device. This frees the sd
struct
and marks the devcom group as not ready.
Since this is a resume failure, the secondary device remains bound to the
driver. Later, when the driver is unbound, mlx5e_remove() is invoked on the
secondary device.
Could this result in a NULL pointer dereference?
When mlx5e_remove() calls mlx5_sd_get_adev(), it returns the secondary adev
itself because the sd struct was already freed:
mlx5_sd_get_adev() {
...
if (!sd)
return adev;
...
}
This leads to _mlx5e_remove() being erroneously called on the secondary
device. Inside _mlx5e_remove(), auxiliary_get_drvdata() is called.
Because drvdata is only set on the primary device during probe, this
returns NULL. Dereferencing mlx5e_dev->netdev would then cause a kernel
panic."

That is correct. In next version mlx5_sd_cleanup() won't be called in
mlx5e_resume(), and the commit message will elaborate to explain the
different handling between probe and resume.

>
> static int _mlx5e_suspend(struct auxiliary_device *adev, bool pre_netdev_reg)
> @@ -6912,9 +6919,16 @@ static int mlx5e_probe(struct auxiliary_device *adev,
> return err;
>
> actual_adev = mlx5_sd_get_adev(mdev, adev, edev->idx);
> - if (actual_adev)
> - return _mlx5e_probe(actual_adev);
> + if (actual_adev) {
> + err = _mlx5e_probe(actual_adev);
> + if (err)
> + goto sd_cleanup;
> + }
> return 0;
> +
> +sd_cleanup:
> + mlx5_sd_cleanup(mdev);
> + return err;
> }
>
> static void _mlx5e_remove(struct auxiliary_device *adev)