On Fri, Jun 12, 2026 at 12:57 PM Linfeng Sun <[email protected]> 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 <[email protected]> > --- > 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 <[email protected]> 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 > >
