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

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

[Yuho Choi]

The failed_dev_add and failed_dev_name error paths in idxd_cdev_open()
drop the file-device reference while still holding wq->wq_lock. If this
is the last reference, put_device(fdev) runs idxd_file_dev_release(),
which takes wq->wq_lock again and deadlocks.

Those error paths also fall through into the later ctx cleanup labels
after idxd_file_dev_release() has already freed ctx. This can make
idxd_xa_pasid_remove(ctx) operate on freed memory and can later free ctx
again at the failed label.

Use scoped put_device() cleanup for fdev and return from the fdev setup
failure path after unlocking wq->wq_lock. Take the WQ reference before
fdev can be released so idxd_file_dev_release() always balances a
matching idxd_wq_get().

Fixes: e6fd6d7e5f0fe ("dmaengine: idxd: add a device to represent the file opened")
Signed-off-by: Yuho Choi <dbgh9129@gmail.com>
---
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 | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
index 0366c7cf3502..18ff29118d12 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;
 	int rc = 0;
 	struct iommu_sva *sva = NULL;
 	unsigned int pasid;
@@ -289,6 +289,7 @@ static int idxd_cdev_open(struct inode *inode, struct file *filp)
 	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) {
 		dev_warn(dev, "set name failure\n");
@@ -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:
 failed_set_pasid:
 	if (device_user_pasid_enabled(idxd))
-- 
2.43.0

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

[sashiko-bot]

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

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

[Dave Jiang]

On 5/15/26 7:26 AM, Yuho Choi wrote:
> The failed_dev_add and failed_dev_name error paths in idxd_cdev_open()
> drop the file-device reference while still holding wq->wq_lock. If this
> is the last reference, put_device(fdev) runs idxd_file_dev_release(),
> which takes wq->wq_lock again and deadlocks.
> 
> Those error paths also fall through into the later ctx cleanup labels
> after idxd_file_dev_release() has already freed ctx. This can make
> idxd_xa_pasid_remove(ctx) operate on freed memory and can later free ctx
> again at the failed label.
> 
> Use scoped put_device() cleanup for fdev and return from the fdev setup
> failure path after unlocking wq->wq_lock. Take the WQ reference before
> fdev can be released so idxd_file_dev_release() always balances a
> matching idxd_wq_get().
> 
> Fixes: e6fd6d7e5f0fe ("dmaengine: idxd: add a device to represent the file opened")
> Signed-off-by: Yuho Choi <dbgh9129@gmail.com>
> ---
> 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 | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
> index 0366c7cf3502..18ff29118d12 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;

It's probably not a good idea to mix scope based cleanups with gotos. Use one or the other and not both. Otherwise the whole thing become a mess to read and maintain. In this function it looks to be pretty difficult to completely convert to scope based cleanups so I suggest avoiding it.

DJ

>  	int rc = 0;
>  	struct iommu_sva *sva = NULL;
>  	unsigned int pasid;
> @@ -289,6 +289,7 @@ static int idxd_cdev_open(struct inode *inode, struct file *filp)
>  	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) {
>  		dev_warn(dev, "set name failure\n");
> @@ -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:
>  failed_set_pasid:
>  	if (device_user_pasid_enabled(idxd))