Re: [PATCH net V3 4/4] net/mlx5e: SD, Fix race condition in secondary device probe/remove

Shay Drori
2026-04-26T13:26:57Z

On 23/04/2026 15:31, Tariq Toukan wrote:
> From: Shay Drory <shayd@nvidia.com>
>
> When utilizing Socket-Direct single netdev functionality the driver
> resolves the actual auxiliary device using mlx5_sd_get_adev(). However,
> the current implementation returns the primary ETH auxiliary device
> without holding the device lock, leading to a potential race condition
> where the ETH device could be unbound or removed concurrently during
> probe, suspend, resume, or remove operations.[1]
>
> Fix this by introducing mlx5_sd_put_adev() and updating
> mlx5_sd_get_adev() so that secondaries devices would acquire the device
> lock of the returned auxiliary device. After the lock is acquired, a
> second devcom check is needed[2].
> In addition, update The callers to pair the get operation with the new
> put operation, ensuring the lock is held while the auxiliary device is
> being operated on and released afterwards.
>
> The "primary" designation is determined once in sd_register(). It's set
> before devcom is marked ready, and it never changes after that.
> In Addition, The primary path never locks a secondary: When the primary
> device invoke mlx5_sd_get_adev(), it sees dev == primary and returns.
> no additional lock is taken.
> Therefore lock ordering is always: secondary_lock -> primary_lock. The
> reverse never happens, so ABBA deadlock is impossible.
>
> [1]
> for example:
> BUG: kernel NULL pointer dereference, address: 0000000000000370
> PGD 0 P4D 0
> Oops: Oops: 0000 [#1] SMP
> CPU: 4 UID: 0 PID: 3945 Comm: bash Not tainted 6.19.0-rc3+ #1 NONE
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
> RIP: 0010:mlx5e_dcbnl_dscp_app+0x23/0x100 [mlx5_core]
> Call Trace:
> <TASK>
> mlx5e_remove+0x82/0x12a [mlx5_core]
> device_release_driver_internal+0x194/0x1f0
> bus_remove_device+0xc6/0x140
> device_del+0x159/0x3c0
> ? devl_param_driverinit_value_get+0x29/0x80
> mlx5_rescan_drivers_locked+0x92/0x160 [mlx5_core]
> mlx5_unregister_device+0x34/0x50 [mlx5_core]
> mlx5_uninit_one+0x43/0xb0 [mlx5_core]
> remove_one+0x4e/0xc0 [mlx5_core]
> pci_device_remove+0x39/0xa0
> device_release_driver_internal+0x194/0x1f0
> unbind_store+0x99/0xa0
> kernfs_fop_write_iter+0x12e/0x1e0
> vfs_write+0x215/0x3d0
> ksys_write+0x5f/0xd0
> do_syscall_64+0x55/0xe90
> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>
> [2]
> CPU0 (primary) CPU1 (secondary)
> ==========================================================================
> mlx5e_remove() (device_lock held)
> mlx5e_remove() (2nd device_lock held)
> mlx5_sd_get_adev()
> mlx5_devcom_comp_is_ready() => true
> device_lock(primary)
> mlx5_sd_get_adev() ==> ret adev
> _mlx5e_remove()
> mlx5_sd_cleanup()
> // mlx5e_remove finished
> // releasing device_lock
> //need another check here...
> mlx5_devcom_comp_is_ready() => false
>
> 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 | 10 ++++++++++
> .../net/ethernet/mellanox/mlx5/core/lib/sd.c | 17 +++++++++++++++++
> .../net/ethernet/mellanox/mlx5/core/lib/sd.h | 2 ++
> 3 files changed, 29 insertions(+)

I went over all Sashiko comments on this patch and they are false
positive

>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index 9c340ad2fe09..c4cb5369f0a0 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -6778,11 +6778,14 @@ static int mlx5e_resume(struct auxiliary_device *adev)
> err = _mlx5e_resume(actual_adev);
> if (err)
> goto sd_cleanup;
> + mlx5_sd_put_adev(actual_adev, adev);
> }
> return 0;
>
> sd_cleanup:
> mlx5_sd_cleanup(mdev);
> + if (actual_adev)
> + mlx5_sd_put_adev(actual_adev, adev);
> return err;
> }
>
> @@ -6822,6 +6825,8 @@ static int mlx5e_suspend(struct auxiliary_device *adev, pm_message_t state)
> err = _mlx5e_suspend(actual_adev, false);
>
> mlx5_sd_cleanup(mdev);
> + if (actual_adev)
> + mlx5_sd_put_adev(actual_adev, adev);
> return err;
> }
>
> @@ -6923,11 +6928,14 @@ static int mlx5e_probe(struct auxiliary_device *adev,
> err = _mlx5e_probe(actual_adev);
> if (err)
> goto sd_cleanup;
> + mlx5_sd_put_adev(actual_adev, adev);
> }
> return 0;
>
> sd_cleanup:
> mlx5_sd_cleanup(mdev);
> + if (actual_adev)
> + mlx5_sd_put_adev(actual_adev, adev);
> return err;
> }
>
> @@ -6980,6 +6988,8 @@ static void mlx5e_remove(struct auxiliary_device *adev)
> _mlx5e_remove(actual_adev);
>
> mlx5_sd_cleanup(mdev);
> + if (actual_adev)
> + mlx5_sd_put_adev(actual_adev, adev);
> }
>
> static const struct auxiliary_device_id mlx5e_id_table[] = {
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
> index 897b0d81b27d..f7b226823ab4 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
> @@ -538,6 +538,10 @@ void mlx5_sd_cleanup(struct mlx5_core_dev *dev)
> sd_cleanup(dev);
> }
>
> +/* Cannot take devcom lock as a gate for device lock. ABBA deadlock:
> + * primary: actual_adev_lock -> SD devcom comp lock
> + * secondary: SD devcom comp lock -> actual_adev_lock
> + */
> struct auxiliary_device *mlx5_sd_get_adev(struct mlx5_core_dev *dev,
> struct auxiliary_device *adev,
> int idx)
> @@ -555,5 +559,18 @@ struct auxiliary_device *mlx5_sd_get_adev(struct mlx5_core_dev *dev,
> if (dev == primary)
> return adev;
>
> + device_lock(&primary->priv.adev[idx]->adev.dev);
> + /* In case primary finish removing its adev */
> + if (!mlx5_devcom_comp_is_ready(sd->devcom)) {
> + device_unlock(&primary->priv.adev[idx]->adev.dev);
> + return NULL;
> + }
> return &primary->priv.adev[idx]->adev;
> }
> +
> +void mlx5_sd_put_adev(struct auxiliary_device *actual_adev,
> + struct auxiliary_device *adev)
> +{
> + if (actual_adev != adev)
> + device_unlock(&actual_adev->dev);
> +}
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.h b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.h
> index 137efaf9aabc..9bfd5b9756b5 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.h
> @@ -15,6 +15,8 @@ struct mlx5_core_dev *mlx5_sd_ch_ix_get_dev(struct mlx5_core_dev *primary, int c
> struct auxiliary_device *mlx5_sd_get_adev(struct mlx5_core_dev *dev,
> struct auxiliary_device *adev,
> int idx);
> +void mlx5_sd_put_adev(struct auxiliary_device *actual_adev,
> + struct auxiliary_device *adev);
>
> int mlx5_sd_init(struct mlx5_core_dev *dev);
> void mlx5_sd_cleanup(struct mlx5_core_dev *dev);