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

[Shay Drori]

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.

> ---