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

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

[Yuho Choi]

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@gmail.com>
---
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))
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] If `ida_alloc()` fails in `idxd_cdev_open()`, the error code is not assigned to `rc`, causing the function to return 0 (success) while leaving `filp->private_data` pointing to a freed context.
--

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

dmaengine: idxd: fix fdev setup failure cleanup in idxd_cdev_open()

This commit fixes a deadlock and use-after-free during file-device setup
failures in idxd_cdev_open(). It moves idxd_wq_get() to occur before
potential failures and adjusts the cleanup path to return directly, leaving
context cleanup to the release callback.

> diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
> --- a/drivers/dma/idxd/cdev.c
> +++ b/drivers/dma/idxd/cdev.c
[ ... ]
> @@ -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:

[Severity: Critical]
This isn't a bug introduced by this patch, but is it possible for rc to be
0 when reaching this label?

Looking at the ida_alloc() call earlier in this function:

    ctx->id = ida_alloc(&file_ida, GFP_KERNEL);
    if (ctx->id < 0) {
        dev_warn(dev, "ida alloc failure\n");
        goto failed_ida;
    }

If ida_alloc() fails, it returns a negative error code to ctx->id, but rc
is not updated and remains 0. The function then jumps to failed_ida,
eventually frees the context, and returns 0.

Since open() returns 0, the VFS layer treats the system call as successful
and returns a valid file descriptor to user space. However,
filp->private_data was already set to the freed ctx earlier in the function.

Could this lead to a use-after-free if subsequent file operations (like
write() or close()) dereference the dangling filp->private_data pointer?

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