Re: [PATCH net V4 1/4] net/mlx5: SD: Serialize init/cleanup

[Shay Drori <shayd@nvidia.com>]

On 30/04/2026 4:42, Jakub Kicinski wrote:
> External email: Use caution opening links or attachments
> 
> 
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net/mlx5: SD: Serialize init/cleanup
> 
> This commit protects the Socket-Direct (SD) init/cleanup paths with the
> devcom component lock and tracks the SD group state on the primary device.
> It skips init if the primary is already UP, and skips cleanup unless the
> primary is UP, serializing the bring-up/tear-down sequence across PFs.
> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
>> index 762c783156b4..d42c283cbb38 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
> 
> [ ... ]
> 
>> @@ -491,22 +507,34 @@ void mlx5_sd_cleanup(struct mlx5_core_dev *dev)
>>   {
>>        struct mlx5_sd *sd = mlx5_get_sd(dev);
>>        struct mlx5_core_dev *primary, *pos;
>> +     struct mlx5_sd *primary_sd;
>>        int i;
>>
>>        if (!sd)
>>                return;
>>
>> +     mlx5_devcom_comp_lock(sd->devcom);
>>        if (!mlx5_devcom_comp_is_ready(sd->devcom))
>> -             goto out;
>> +             goto out_unlock;
>>
>>        primary = mlx5_sd_get_primary(dev);
>> +     if (!primary)
>> +             goto out_unlock;
>> +
>> +     primary_sd = mlx5_get_sd(primary);
>> +     if (primary_sd->state != MLX5_SD_STATE_UP)
>> +             goto out_unlock;
> 
> If the device is unbound during the race window where the devcom component
> is ready but the state is still MLX5_SD_STATE_DOWN (for example, just after
> sd_register() finishes but before mlx5_sd_init() sets the state to UP), does
> this early exit leave the component marked as ready?
> 
> Since mlx5_devcom_comp_set_ready(sd->devcom, false) was moved out of
> sd_unregister() and into this function, skipping it here means the device will
> unregister and free its struct mlx5_sd while the group is still ready.
> 
> Could a concurrent mlx5_sd_init() on another peer acquire the devcom lock,
> observe that it is ready, and proceed? If the unregistered device was the
> primary, mlx5_get_sd(primary) would return NULL, causing a NULL pointer
> dereference at primary_sd->state. If it was a secondary, mlx5_sd_init()
> would iterate over primary_sd->secondaries and access the freed device
> structure, resulting in a use-after-free.

This is correct :(
will fix in next version

> 
>> +
>>        mlx5_sd_for_each_secondary(i, primary, pos)
>>                sd_cmd_unset_secondary(pos);
>>        sd_cmd_unset_primary(primary);
>>        debugfs_remove_recursive(sd->dfs);
> 
> This isn't a regression introduced in this patch, but does storing sd->dfs
> locally instead of on the primary device cause debugfs entries to leak?
> 
> If a different peer is unbound first, its sd->dfs is NULL, making the call
> to debugfs_remove_recursive() a no-op. The cleanup then finishes and marks
> the group as unready.
> 
> When the original device that created the debugfs directory is later unbound,
> it sees the group is no longer ready and skips this block entirely, never
> freeing its sd->dfs. Since these files hold pointers to the mlx5_core_dev
> structs, accessing them from userspace after the devices are freed could
> trigger a use-after-free.
> 
> I note this was fixed later in the series by commit "net/mlx5: SD, Keep
> multi-pf debugfs entries on primary", but wanted to highlight it here.

like mention, this is fixed later and I don't see a reason to change the
patches order

> 
>>
>>        sd_info(primary, "group id %#x, uncombined\n", sd->group_id);
>> -out:
>> +     primary_sd->state = MLX5_SD_STATE_DOWN;
>> +     mlx5_devcom_comp_set_ready(sd->devcom, false);
>> +out_unlock:
>> +     mlx5_devcom_comp_unlock(sd->devcom);
>>        sd_unregister(dev);
>>        sd_cleanup(dev);
>>   }
> --
> pw-bot: cr