Re: [PATCH v3] dmaengine: idxd: fix fdev setup failure cleanup in idxd_cdev_open()

[Dave Jiang]

On 5/25/26 7:15 AM, Yuho Choi wrote:
> The failed_dev_add and failed_dev_name paths drop the file-device
> reference while wq->wq_lock is still held. If put_device(fdev) drops the
> last reference, idxd_file_dev_release() runs synchronously and tries to
> take wq->wq_lock again, deadlocking.
> 
> Those paths also fall through into the later ctx cleanup labels even
> though idxd_file_dev_release() owns that cleanup and frees ctx. This can
> make idxd_xa_pasid_remove(ctx) and kfree(ctx) operate on a freed context.
> 
> Move idxd_wq_get() before file-device setup can fail, since the release
> callback always calls idxd_wq_put(). Then unlock wq->wq_lock before
> put_device(fdev) and return directly from the file-device setup failure
> path, leaving ctx cleanup to the release callback.
> 
> Fixes: e6fd6d7e5f0fe ("dmaengine: idxd: add a device to represent the file opened")
> Signed-off-by: Yuho Choi <dbgh9129@xxxxxxxxx>

Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>


> ---
> Changes in v3:
> - Drop scoped __free(put_device) cleanup and use explicit cleanup, as
>   suggested by Dave Jiang.
> - Keep idxd_wq_get() before file-device setup can fail so the release
>   callback always balances a matching WQ reference.
> Changes in v2:
> - Use __free(put_device) for the file-device reference.
> - Take the WQ reference before fdev can be released so the release
>   callback's idxd_wq_put() has a matching idxd_wq_get().
> 
>  drivers/dma/idxd/cdev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
> index 0366c7cf3502..82b07cf942ef 100644
> --- a/drivers/dma/idxd/cdev.c
> +++ b/drivers/dma/idxd/cdev.c
> @@ -288,6 +288,7 @@ static int idxd_cdev_open(struct inode *inode, struct file *filp)
>  	fdev->parent = cdev_dev(idxd_cdev);
>  	fdev->bus = &dsa_bus_type;
>  	fdev->type = &idxd_cdev_file_type;
> +	idxd_wq_get(wq);
>  
>  	rc = dev_set_name(fdev, "file%d", ctx->id);
>  	if (rc < 0) {
> @@ -301,13 +302,14 @@ static int idxd_cdev_open(struct inode *inode, struct file *filp)
>  		goto failed_dev_add;
>  	}
>  
> -	idxd_wq_get(wq);
>  	mutex_unlock(&wq->wq_lock);
>  	return 0;
>  
>  failed_dev_add:
>  failed_dev_name:
> +	mutex_unlock(&wq->wq_lock);
>  	put_device(fdev);
> +	return rc;
>  failed_ida:
>  failed_set_pasid:
>  	if (device_user_pasid_enabled(idxd))