Re: [PATCH net V2 1/3] net/mlx5: SD: Serialize init/cleanup

[Shay Drori <shayd@nvidia.com>]

On 16/04/2026 14:00, Paolo Abeni wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 4/13/26 12:53 PM, Tariq Toukan wrote:
>> @@ -491,23 +508,35 @@ 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 = NULL;
>>        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);
>> +     primary_sd = mlx5_get_sd(primary);
>> +
>> +     if (primary_sd->state != MLX5_SD_STATE_UP)
>> +             goto out_unlock;
>> +
>>        mlx5_sd_for_each_secondary(i, primary, pos)
>>                sd_cmd_unset_secondary(pos);
>>        sd_cmd_unset_primary(primary);
>>        debugfs_remove_recursive(sd->dfs);
>>
>>        sd_info(primary, "group id %#x, uncombined\n", sd->group_id);
>> -out:
>> +     primary_sd->state = MLX5_SD_STATE_DESTROYING;
>> +out_unlock:
>> +     mlx5_devcom_comp_unlock(sd->devcom);
>>        sd_unregister(dev);
>> +     if (primary_sd)
>> +             /* devcom isn't ready, reset the state */
>> +             primary_sd->state = MLX5_SD_STATE_DOWN;
> 
> Sashiko says:
> 
> ---
> Since primary_sd is only populated if devcom is ready, this condition will
> never trigger when devcom isn't ready, contrary to the comment. Instead,
> it triggers on the normal path after devcom is ready, immediately
> overwriting
> MLX5_SD_STATE_DESTROYING with MLX5_SD_STATE_DOWN outside the lock.
> Could this allow concurrent mlx5_sd_init() calls to see the down state and
> attempt hardware re-initialization while the group is still being torn down?
> Also, can this race and cause a use-after-free regression?
> During a concurrent Socket-Direct group teardown, the primary PF and
> secondary
> PF can execute mlx5_sd_cleanup() in parallel.
> If the primary PF completes its cleanup first, it will call
> sd_cleanup(primary)
> which calls kfree() on the sd structure, freeing the primary_sd memory.
> If the secondary PF is preempted just after releasing the devcom lock,
> it will resume, evaluate its local non-NULL primary_sd pointer, and
> locklessly
> write to primary_sd->state. Does this dereference the freed memory of the
> primary PF?
After some more thinks following the above comments, I change the logic
here so that devcom_set_ready(false) is done under the
mlx5_devcom_comp_lock(), making the is_ready() check in sd_init()
reliable gate for cleanup/init race cases.

> ---
>