Re: [PATCH net] pds_core: fix auxiliary device add/del races

[Eric Joyner <eric.joyner@amd.com>]

On 7/15/2026 6:23 AM, Pavan Chebbi wrote:
> On Wed, Jul 15, 2026 at 2:38 AM Nikhil P. Rao <nikhil.rao@amd.com> wrote:
>>
>> Two paths add or delete the same slot (pf->vfs[vf_id].padev): a VF's
>> pdsc_reset_done() and the PF's devlink enable_vnet/disable_vnet handler.
>> They serialize on config_lock, but neither guards the slot under it
>> correctly.
>>
>> add() registers and stores a new auxiliary device without first checking
>> the slot, so a second add of an already-populated slot leaks the first
>> device. del() makes that check outside config_lock, so two concurrent
>> dels can both pass it; the first clears the slot, and the second
>> dereferences a NULL pointer.
>>
>> Check and update the slot under config_lock in both paths.
>>
>> Fixes: b699bdc720c0 ("pds_core: specify auxiliary_device to be created")
>> Reported-by: sashiko-bot@kernel.org # Running on a local machine
>> Signed-off-by: Nikhil P. Rao <nikhil.rao@amd.com>
>> Reviewed-by: Brett Creeley <brett.creeley@amd.com>
>> ---
>>  drivers/net/ethernet/amd/pds_core/auxbus.c | 17 ++++++++++++++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
>> index 73b3481220b1..3acafe10a6d5 100644
>> --- a/drivers/net/ethernet/amd/pds_core/auxbus.c
>> +++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
>> @@ -177,17 +177,21 @@ void pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf,
>>  {
>>         struct pds_auxiliary_dev *padev;
>>
>> -       if (!*pd_ptr)
>> -               return;
>> -
>>         mutex_lock(&pf->config_lock);
>>
>> +       /* A concurrent del may have already torn this device down and
>> +        * cleared it.
>> +        */

I don't know if this one is AI-generated, but it looked good to me; I appreciate
the comment that it is specifically supposed to guard against a scenario. Though
arguably it's a common enough pattern that it may not need it.

>>         padev = *pd_ptr;
>> +       if (!padev)
>> +               goto out_unlock;
>> +
>>         pds_client_unregister(pf, padev->client_id);
>>         auxiliary_device_delete(&padev->aux_dev);
>>         auxiliary_device_uninit(&padev->aux_dev);
>>         *pd_ptr = NULL;
>>
>> +out_unlock:
>>         mutex_unlock(&pf->config_lock);
>>  }
>>
>> @@ -210,6 +214,13 @@ int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf,
>>
>>         mutex_lock(&pf->config_lock);
>>
>> +       /* Nothing to do if the aux device is already present.  This also
>> +        * guards against a second add overwriting *pd_ptr and leaking the
>> +        * first, symmetric with the check in pdsc_auxbus_dev_del().
>> +        */
> 
> Are these AI-generated-appearing comments really necessary?
> Anyway, it is your driver.
> Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>

This is a bit flippant, but if the kernel is going to send all of our patches
through an AI, we can at least be nice to the AI the mailing list uses and
reduce its credit usage by embedding comments from our AI in it so it doesn't
have to figure things out every time. :p

But I get your point, this one in particular seems gratuitous, and the language
it uses is recognizably AI-generated which is off-putting to me, too, though
maybe I'm also just sensitive to them. I think we review these comments with the
view that unless they're wrong, they're harmless to leave in; but instead we
should view these added comments more critically.

- Eric

> 
>> +       if (*pd_ptr)
>> +               goto out_unlock;
>> +
>>         mask = BIT_ULL(PDSC_S_FW_DEAD) |
>>                BIT_ULL(PDSC_S_STOPPING_DRIVER);
>>         if (cf->state & mask) {
>> --
>> 2.43.0
>>
>>