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

[Shay Drori <shayd@nvidia.com>]

On 16/04/2026 14:07, Paolo Abeni wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 4/13/26 12:53 PM, 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  | 18 ++++++++++++++----
>>   .../net/ethernet/mellanox/mlx5/core/lib/sd.c   | 17 +++++++++++++++++
>>   .../net/ethernet/mellanox/mlx5/core/lib/sd.h   |  2 ++
>>   3 files changed, 33 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 0b8b44bbcb9e..11f80158e107 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> @@ -6657,8 +6657,11 @@ 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);
>> +             mlx5_sd_put_adev(actual_adev, adev);
>> +             return err;
>> +     }
>>        return 0;
>>   }
>>
>> @@ -6698,6 +6701,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;
>>   }
>>
>> @@ -6795,8 +6800,11 @@ 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);
>> +             mlx5_sd_put_adev(actual_adev, adev);
> 
> Sashiko says:
> 
> ---
> If _mlx5e_probe(actual_adev) fails, it frees mlx5e_dev but leaves the
> auxiliary device's drvdata pointing to it.

After probe fails, the driver core marks the device as unbound.
Hence, the stale drvdata pointer is unreachable.

> Furthermore, mlx5e_probe()
> returns the error without calling mlx5_sd_cleanup(mdev), leaving devcom
> incorrectly marked as ready.
> If the primary device is later unbound, mlx5e_remove() will see that
> devcom is ready, call _mlx5e_remove(), and blindly dereference the
> dangling mlx5e_dev pointer.
> Is there a missing cleanup step here to clear drvdata or reset the sd
> state on failure?

This is an existing bug that will be address in a new patch in V3

> ---
> 
> Please try to address AI comments (i.e. explaining why not relevant)
> proactively.

Ack.

Thanks

> 
> Thanks,
> 
> Paolo
>