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

[Shay Drori]

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@xxxxxxxxxx>
> > > >
> > > > 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