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

[Shay Drori <shayd@nvidia.com>]

On 03/04/2026 3:45, Jakub Kicinski wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, 2 Apr 2026 23:03:10 +0300 Shay Drori wrote:
>> On 02/04/2026 6:08, Jakub Kicinski wrote:
>>> On Mon, 30 Mar 2026 22:34:10 +0300 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.
>>>
>>> Please explain why the "primary" designation is reliable, and therefore
>>> we can be sure there will be no ABBA deadlock here
>>
>> 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.
> 
> And the device_lock instances have separate lockdep classes?
> So lockdep will also understand this?

I tested this patches with lockdep enable and didn't get a splat,
so it seems lockdep understand.

> 
>> Does the above is the explanation you looked for?
>> If not, can you elaborate?
>> If yes, to add it to the commit message in V2?
> 
> Sounds good, please add to the msg.
> 
>>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>>> index b6c12460b54a..5761f655f488 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;
>>>
>>> Feels like I recently complained about similar code y'all were trying
>>> to add. Magically and conditionally locking something in a get helper
>>> makes for extremely confusing code.
>>
>> Do you think explicit locking API is preferred here?
>> something like:
>> new_locking_api()
>>
>> mlx5_sd_get_adev()
>>
>> new_unlocking_api()
> 
> Readability is hard, I'd just push the locking up into the callers TBH.
> Looks like there's only 4, the LoC delta isn't going to be huge.

I though about it, but AFAIU your suggestion, a race is still possible:
your suggestion is to lock the adev returned from mlx5_sd_get_adev().
but between mlx5_sd_get_adev() and the lock, the adev can be free...

Therefore, an SD helper is still needed.
do you have a preferred approach here?

thanks