Re: [PATCH] vdpa_sim: fix cleanup after worker creation failure

[Eugenio Perez Martin]

On Fri, Jun 12, 2026 at 12:57 PM Linfeng Sun <slf@xxxxxxxxxx> wrote:
>
> vdpasim_create() leaves vdpasim->worker as an ERR_PTR when
> kthread_run_worker() fails. The error path then drops the device
> reference, which releases the partially initialized simulator.
>
> vdpasim_free() unconditionally passes the worker pointer to
> kthread_destroy_worker(), so the ERR_PTR is dereferenced and can
> trigger a general protection fault.
>
> Store the worker error, clear the pointer, and make the release path
> only clean up resources that were successfully initialized before
> the failure.
>

Good catch! Yet a few things to improve,

It missees Fixes: tag

> Signed-off-by: Linfeng Sun <slf@xxxxxxxxxx>
> ---
>  drivers/vdpa/vdpa_sim/vdpa_sim.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> index 8cb1cc2ea139..6a4e28c49d2d 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> @@ -230,9 +230,12 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *dev_attr,
>
>         kthread_init_work(&vdpasim->work, vdpasim_work_fn);
>         vdpasim->worker = kthread_run_worker(0, "vDPA sim worker: %s",
> -                                               dev_attr->name);
> -       if (IS_ERR(vdpasim->worker))
> +                                            dev_attr->name);
> +       if (IS_ERR(vdpasim->worker)) {
> +               ret = PTR_ERR(vdpasim->worker);
> +               vdpasim->worker = NULL;
>                 goto err_iommu;
> +       }
>
>         mutex_init(&vdpasim->mutex);
>         spin_lock_init(&vdpasim->iommu_lock);
> @@ -742,18 +745,24 @@ static void vdpasim_free(struct vdpa_device *vdpa)
>         struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>         int i;
>
> -       kthread_cancel_work_sync(&vdpasim->work);
> -       kthread_destroy_worker(vdpasim->worker);
> +       if (vdpasim->worker) {
> +               kthread_cancel_work_sync(&vdpasim->work);
> +               kthread_destroy_worker(vdpasim->worker);
> +       }
>
> -       for (i = 0; i < vdpasim->dev_attr.nvqs; i++) {
> -               vringh_kiov_cleanup(&vdpasim->vqs[i].out_iov);
> -               vringh_kiov_cleanup(&vdpasim->vqs[i].in_iov);
> +       if (vdpasim->vqs) {
> +               for (i = 0; i < vdpasim->dev_attr.nvqs; i++) {
> +                       vringh_kiov_cleanup(&vdpasim->vqs[i].out_iov);
> +                       vringh_kiov_cleanup(&vdpasim->vqs[i].in_iov);
> +               }
>         }
>
>         vdpasim->dev_attr.free(vdpasim);
>
> -       for (i = 0; i < vdpasim->dev_attr.nas; i++)
> -               vhost_iotlb_reset(&vdpasim->iommu[i]);
> +       if (vdpasim->iommu && vdpasim->iommu_pt) {

It looks to me that this conditional and the one for `vdpasim->vqs`,
are needed if vdpasim_create returns from error paths other than the
error from kthread_run_worker, isn't it? For example, if
dma_set_mask_and_coherent fails, vhost_iotlb_reset will also be
called. In that sense, it would be great to note this in the patch
message.

Also, vdpasim->iommu[i] should be reset regardless vdpasim->iommu_pt,
so omit that in the conditional.

Finally, if you found the issue or coded the patch with AI please
indicate it with the Assisted-by tag.

With these changes applied, feel free to add:

Reviewed-by: Eugenio Pérez <eperezma@xxxxxxxxxx>

Thanks!

> +               for (i = 0; i < vdpasim->dev_attr.nas; i++)
> +                       vhost_iotlb_reset(&vdpasim->iommu[i]);
> +       }
>         kfree(vdpasim->iommu);
>         kfree(vdpasim->iommu_pt);
>         kfree(vdpasim->vqs);
> --
> 2.43.0
>
>