Re: [PATCH v2] dmaengine: idxd: fix deadlock and double free in idxd_cdev_open()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardware state leak and potential IOMMU fault on ida_alloc() failure path.
- [Medium] Mixing goto-based error handling and scope-based __free() cleanup in the same function.
--

commit e8ca7b853ba46b3d4f21adcfca83ffcc065ffc9c
Author: Yuho Choi <dbgh9129@gmail.com>

dmaengine: idxd: fix deadlock and double free in idxd_cdev_open()

This commit resolves a deadlock and double free in idxd_cdev_open() by
using a scoped put_device() cleanup for fdev. It also adjusts the locking
and reference counting on the error paths when device_add() or dev_set_name()
fails.

> diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
> index 0366c7cf35020..18ff29118d121 100644
> --- a/drivers/dma/idxd/cdev.c
> +++ b/drivers/dma/idxd/cdev.c
> @@ -216,7 +216,7 @@ static int idxd_cdev_open(struct inode *inode, struct file *filp)
>  	struct idxd_user_context *ctx;
>  	struct idxd_device *idxd;
>  	struct idxd_wq *wq;
> -	struct device *dev, *fdev;
> +	struct device *dev, *fdev __free(put_device) = NULL;

The official cleanup subsystem guidelines recommend against mixing goto-based
error handling and scope-based __free() cleanup in the same function, as it
can create confusing ownership semantics.

Since this function still retains several goto labels for earlier failure
paths, would it be better to either fully convert the function to scope-based
cleanup or continue relying on goto for this variable?

[ ... ]

> @@ -301,13 +302,14 @@ static int idxd_cdev_open(struct inode *inode, struct file *filp)
>  		goto failed_dev_add;
>  	}
>  
> -	idxd_wq_get(wq);
> +	fdev = NULL;
>  	mutex_unlock(&wq->wq_lock);
>  	return 0;
>  
>  failed_dev_add:
>  failed_dev_name:
> -	put_device(fdev);
> +	mutex_unlock(&wq->wq_lock);
> +	return rc;
>  failed_ida:

This issue predates this commit, but I noticed a potential hardware state leak
on the ida_alloc() failure path that leads to this label.

If the workqueue is dedicated, idxd_wq_set_pasid() is called to program the
PASID into the hardware registers. Immediately after, if ida_alloc() fails,
the code jumps to failed_ida.

The error path then unbinds the SVA device via iommu_sva_unbind_device().
However, because fdev is still NULL at this point, idxd_file_dev_release()
is skipped and idxd_wq_disable_pasid() is never called.

Does this leave the hardware workqueue enabled and programmed with an unbound
PASID, potentially triggering IOMMU translation faults during subsequent
hardware activity?

>  failed_set_pasid:
>  	if (device_user_pasid_enabled(idxd))
>  		idxd_xa_pasid_remove(ctx);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515142623.793549-1-dbgh9129@gmail.com?part=1