Re: [PATCH 5/6] dmaengine: idxd: Fix error code handling in idxd_cdev_open()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] dmaengine: idxd: idxd_cdev_open() masks -ENOMEM from xa_insert()
- [High] dmaengine: stranded PASID xarray entry on shared FD close breaks fault handling
- [Medium] dmaengine: idxd: missing idxd_wq_disable_pasid() rollback on ida_alloc() failure
--

commit 63ea0ebbf579eb846dc4486575508a710df270fc
Author: Vinicius Costa Gomes <vinicius.gomes@intel.com>

dmaengine: idxd: Fix error code handling in idxd_cdev_open()

idxd_cdev_open() mishandles rc on two error paths: on ida_alloc() failure,
it jumps to failed_ida without setting rc, returning 0. And when xa_insert()
returns -EBUSY, rc is left negative and can leak out. This commit fixes
both paths.

> diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
> index 82b07cf942ef8..abbab9bdd06c3 100644
> --- a/drivers/dma/idxd/cdev.c
> +++ b/drivers/dma/idxd/cdev.c
> @@ -264,8 +264,11 @@ static int idxd_cdev_open(struct inode *inode, struct file *filp)
>  		mutex_lock(&wq->uc_lock);
>  		rc = xa_insert(&wq->upasid_xa, pasid, ctx, GFP_KERNEL);
>  		mutex_unlock(&wq->uc_lock);
> -		if (rc < 0)
> -			dev_warn(dev, "PASID entry already exist in xarray.\n");
> +		if (rc < 0) {
> +			/* An existing entry is harmless, warn and continue */
> +			dev_warn(dev, "PASID entry already exists in xarray.\n");
> +			rc = 0;
> +		}

[Severity: High]
Does this code unintentionally mask all negative return codes, including
-ENOMEM?

xa_insert() can return -ENOMEM if memory allocation fails. By unconditionally
clearing all negative return codes (rc = 0), this allows the file open to
succeed without registering the user context in the xarray.

Later, when a subsequent hardware page fault occurs for this PASID,
idxd_copy_cr() will call xa_load(), fail to find the context, and silently
drop the completion record.

[Severity: High]
If a process opens multiple file descriptors to a shared IDXD workqueue,
does treating -EBUSY as harmless leave the second FD stranded when the first
FD is closed?

When the first open succeeds, the context is inserted into the upasid_xa
xarray. The second open fails the xa_insert with -EBUSY, which this code
explicitly treats as harmless and continues, leaving the first context in
the xarray.

However, when the process closes the first file descriptor,
idxd_xa_pasid_remove() successfully matches and removes the first context
from the xarray:

drivers/dma/idxd/cdev.c:idxd_xa_pasid_remove() {
	ptr = xa_cmpxchg(&wq->upasid_xa, ctx->pasid, ctx, NULL, GFP_KERNEL);
}

The second file descriptor remains open and active, but its PASID is no
longer mapped in the xarray. If a hardware fault occurs for this PASID,
idxd_copy_cr() will fail to find the context in the xarray, warn
'No user context', and silently drop the completion record.

>  
>  		if (wq_dedicated(wq)) {
>  			rc = idxd_wq_set_pasid(wq, pasid);
> @@ -279,6 +282,7 @@ static int idxd_cdev_open(struct inode *inode, struct file *filp)
>  	idxd_cdev = wq->idxd_cdev;
>  	ctx->id = ida_alloc(&file_ida, GFP_KERNEL);
>  	if (ctx->id < 0) {
> +		rc = ctx->id;
>  		dev_warn(dev, "ida alloc failure\n");
>  		goto failed_ida;
>  	}

[Severity: Medium]
Does the failed_ida error path leak the PASID hardware configuration?

During open(), if the WQ is dedicated, idxd_wq_set_pasid() programs the
hardware WQ with the calling process's PASID. If the subsequent ida_alloc()
fails, the error path (failed_ida) unbinds the PASID from the IOMMU and
removes it from the xarray:

drivers/dma/idxd/cdev.c:idxd_cdev_open() {
    ...
failed_ida:
failed_set_pasid:
	if (device_user_pasid_enabled(idxd))
		idxd_xa_pasid_remove(ctx);
failed_get_pasid:
	if (device_user_pasid_enabled(idxd) && !IS_ERR_OR_NULL(sva))
		iommu_sva_unbind_device(sva);
    ...
}

However, it fails to call idxd_wq_disable_pasid() to clear it from the
hardware WQ. This leaves the hardware WQ in the ENABLED state programmed
with a stale, unbound PASID.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-wt-idxd-fixes-v1-0-30a35a445077@intel.com?part=5