[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()

Posted by Yuho Choi 1 week, 2 days ago

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()

Posted by Dave Jiang 1 week, 2 days ago


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))

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

Posted by 최유호 1 week ago

Thanks for the review.
Understood regarding mixing scope-based cleanup and gotos. I initially
introduced the scope-based cleanup following the first feedback in v1
but it became complicated. Since fully converting the function with
auto cleanup would be overly complex, I will send v3 with gotos.

Best regards,
Yuho


On Fri, 15 May 2026 at 11:53, Dave Jiang <dave.jiang@intel.com> wrote:
>
>
>
> 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))
>