[PATCH v2 2/2] dmaengine: idxd: fix duplicate memory frees on initialization error path.

[PATCH v2 2/2] dmaengine: idxd: fix duplicate memory frees on initialization error path.

[Steve Wahl]

Error paths within idxd_pci_probe_alloc and related functions end up
attempting to free memory already freed from idxd_conf_device_release
via put_device.

This was encountered running in a kexec'd kdump kernel with reduced
resources, causing the "Device is HALTED!" branch in
idxd_device_init_reset to be taken.

In idxd_free and idxd_alloc, do not attempt to free allocations that
will already have been freed.

Signed-off-by: Steve Wahl <steve.wahl@hpe.com>
---
v2: split into two patches as requested by Vinicius Costa

 drivers/dma/idxd/init.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index f1cfc7790d95..227e323cc5a0 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -607,9 +607,6 @@ static void idxd_free(struct idxd_device *idxd)
 		return;
 
 	put_device(idxd_confdev(idxd));
-	bitmap_free(idxd->opcap_bmap);
-	ida_free(&idxd_ida, idxd->id);
-	kfree(idxd);
 }
 
 static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_data *data)
@@ -649,8 +646,13 @@ static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_d
 	return idxd;
 
 err_name:
+	/*
+	 * once device_initialize(conf_dev) is called,
+	 * put_device(conf_dev) will end up calling
+	 * idxd_conf_device_release() which will free the rest.
+	 */
 	put_device(conf_dev);
-	bitmap_free(idxd->opcap_bmap);
+	return NULL;
 err_opcap:
 	ida_free(&idxd_ida, idxd->id);
 err_ida:
-- 
2.51.0

Re: [PATCH v2 2/2] dmaengine: idxd: fix duplicate memory frees on initialization error path.

[sashiko-bot]

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

Pre-existing issues:
- [High] Double frees of child objects (workqueues, engines, groups) remain unaddressed in their respective cleanup functions and setup error paths.
- [High] Dangling pointers left by `idxd_cleanup_internals()` result in a second wave of double frees when `idxd_conf_device_release()` is triggered.
--

commit 4ee711bb61961f88e9ddb1a67af212cce348ee26
Author: Steve Wahl <steve.wahl@hpe.com>

dmaengine: idxd: fix duplicate memory frees on initialization error path.

This commit removes duplicate memory frees in idxd_alloc() and idxd_free()
by relying on the device release callback triggered by put_device() to
handle the cleanup of the idxd structure and its internal allocations.

> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index f1cfc7790d950..227e323cc5a0b 100644
> --- a/drivers/dma/idxd/init.c
> +++ b/drivers/dma/idxd/init.c
> @@ -607,9 +607,6 @@ static void idxd_free(struct idxd_device *idxd)
>  		return;
>  
>  	put_device(idxd_confdev(idxd));

[Severity: High]
This is a pre-existing issue, but does idxd_cleanup_internals() leave
dangling pointers that could cause a second wave of double frees here?

During device initialization in idxd_probe(), if an error occurs after
idxd_setup_internals() succeeds (e.g., idxd_device_load_config() fails),
idxd_cleanup_internals() is called. This function explicitly frees
sub-allocations (idxd->wqs, idxd->engines, idxd->groups) and calls
destroy_workqueue(idxd->wq), but it doesn't set these pointers to NULL.

When the error propagates back to idxd_pci_probe_alloc(), it calls
idxd_free(), which reaches this put_device() call. This triggers the
idxd_conf_device_release() callback, which blindly attempts to free the
exact same dangling pointers again and calls destroy_workqueue(idxd->wq) a
second time.

> -	bitmap_free(idxd->opcap_bmap);
> -	ida_free(&idxd_ida, idxd->id);
> -	kfree(idxd);
>  }

[ ... ]

> @@ -649,8 +646,13 @@ static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_d
>  	return idxd;
>  
>  err_name:
> +	/*
> +	 * once device_initialize(conf_dev) is called,
> +	 * put_device(conf_dev) will end up calling
> +	 * idxd_conf_device_release() which will free the rest.
> +	 */
>  	put_device(conf_dev);

[Severity: High]
This isn't a bug introduced by this patch, but are there similar duplicate
frees remaining for child objects like workqueues, engines, and groups?

While this patch correctly removes the duplicate frees for the main idxd
object, the exact same pattern appears to exist for child objects in functions
like idxd_setup_wqs(), idxd_setup_engines(), and idxd_setup_groups(), as well
as their cleanup counterparts.

In those functions, put_device(conf_dev) is called to drop the reference
count, which synchronously invokes the device's release callback (e.g.,
idxd_conf_wq_release()). The callback explicitly frees the inner members and
the object itself (kfree(wq)).

However, immediately after put_device() returns, the manual cleanup code
incorrectly calls kfree() and bitmap_free() on those exact same pointers
again. Could this lead to double frees during initialization failure or device
removal?

> -	bitmap_free(idxd->opcap_bmap);
> +	return NULL;
>  err_opcap:
>  	ida_free(&idxd_ida, idxd->id);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522203414.336549-1-steve.wahl@hpe.com?part=2

Re: [PATCH v2 2/2] dmaengine: idxd: fix duplicate memory frees on initialization error path.

[Vinicius Costa Gomes]

Steve Wahl <steve.wahl@hpe.com> writes:

> Error paths within idxd_pci_probe_alloc and related functions end up
> attempting to free memory already freed from idxd_conf_device_release
> via put_device.
>
> This was encountered running in a kexec'd kdump kernel with reduced
> resources, causing the "Device is HALTED!" branch in
> idxd_device_init_reset to be taken.
>
> In idxd_free and idxd_alloc, do not attempt to free allocations that
> will already have been freed.
>
> Signed-off-by: Steve Wahl <steve.wahl@hpe.com>
> ---

Bogdan Codres series (but submitted a bit later), tries to fix this
issue, has a better splat and a reproducer, I think it makes sense to
add the splat and reproducer here, and add Bogdan as Reported-by (if
agreed, of course).

I am wondering about adding a third patch for the dangling ->wq pointer, as
reported by Sashiko would make sense.

Something like this might do the job (totally untested):

diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index f1cfc7790d95..0a74018f31a8 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -415,6 +415,7 @@ static void idxd_cleanup_internals(struct idxd_device *idxd)
 	idxd_clean_engines(idxd);
 	idxd_clean_wqs(idxd);
 	destroy_workqueue(idxd->wq);
+	idxd->wq = NULL;
 }

How does it sound?

> v2: split into two patches as requested by Vinicius Costa
>
>  drivers/dma/idxd/init.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index f1cfc7790d95..227e323cc5a0 100644
> --- a/drivers/dma/idxd/init.c
> +++ b/drivers/dma/idxd/init.c
> @@ -607,9 +607,6 @@ static void idxd_free(struct idxd_device *idxd)
>  		return;
>  
>  	put_device(idxd_confdev(idxd));
> -	bitmap_free(idxd->opcap_bmap);
> -	ida_free(&idxd_ida, idxd->id);
> -	kfree(idxd);
>  }
>  
>  static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_data *data)
> @@ -649,8 +646,13 @@ static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_d
>  	return idxd;
>  
>  err_name:
> +	/*
> +	 * once device_initialize(conf_dev) is called,
> +	 * put_device(conf_dev) will end up calling
> +	 * idxd_conf_device_release() which will free the rest.
> +	 */
>  	put_device(conf_dev);
> -	bitmap_free(idxd->opcap_bmap);
> +	return NULL;
>  err_opcap:
>  	ida_free(&idxd_ida, idxd->id);
>  err_ida:
> -- 
> 2.51.0
>

-- 
Vinicius

Re: [PATCH v2 2/2] dmaengine: idxd: fix duplicate memory frees on initialization error path

[Bogdan Codres]

Hi Vinicius, Steve,
Thanks for the heads-up. I'm totally fine with being added as

Reported-by: Bogdan Codres <bogdan.codres@windriver.com>

on Steve's patch — his fix addresses the same issue I was seeing.

You can use my splat ad the reproducer.

Regarding the third patch for the dangling ->wq pointer — the idxd->wq = NULL after destroy_workqueue()
looks reasonable to me.

Best regards, Bogdan

Re: [PATCH v2 2/2] dmaengine: idxd: fix duplicate memory frees on initialization error path.

[Steve Wahl]

On Mon, Jun 29, 2026 at 11:44:36AM +0000, Codres, Bogdan wrote:
> Hi Vinicius, Steve,
> 
> Thanks for the heads-up. I'm totally fine with being added as:
> 
> Reported-by: Bogdan Codres <bogdan.codres@windriver.com>
> 
> on Steve's patch — his fix addresses the same issue I was seeing.
> You can use my splat as the reproducer.
> 
> Regarding the third patch for the dangling ->wq pointer — the proposed change looks reasonable to me.
> 
> Best regards,
> Bogdan
> 
> Ph.D. eng. Bogdan Codres
> Member of Technical Staff in Cloud Platform Engineering, Wind River
> 

Hello, everyone,

As there's no code changes (and I even find it remarkable how similar
the two were, I think only the comments differed), I am completely
open to Vinicius just modifying the commit message to include the
parts he wants to see.  It would take less time than another round of
posting a patch.  But if you need more from me, let me know.

Thanks,

--> Steve

> ________________________________
> From: Codres, Bogdan <Bogdan.Codres@windriver.com>
> Sent: Monday, June 29, 2026 14:32
> To: Vinicius Costa Gomes <vinicius.gomes@intel.com>; Steve Wahl <steve.wahl@hpe.com>; Steve Wahl <steve.wahl@hpe.com>; Dave Jiang <dave.jiang@intel.com>; Vinod Koul <vkoul@kernel.org>; Frank Li <Frank.Li@kernel.org>; dmaengine@vger.kernel.org <dmaengine@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
> Cc: Russ Anderson <rja@hpe.com>; Dimitri Sivanich <sivanich@hpe.com>
> Subject: Re: [PATCH v2 2/2] dmaengine: idxd: fix duplicate memory frees on initialization error path.
> 
> Hi Vinicius, Steve,
> Thanks for the heads-up. I'm totally fine with being added as
> Reported-by: Bogdan Codres <bogdan.codres@windriver.com>
> on Steve's patch — his fix addresses the same issue I was seeing.
> You can use my splat ad the reproducer.
> Regarding the third patch for the dangling ->wq pointer — the idxd->wq = NULL after destroy_workqueue() looks reasonable to me
> Best regards, Bogdan
> 
> 
> Best Regards,
> 
> Ph.D. eng. Bogdan Codres
> 
> Member of Technical Staff in Cloud Platform Engineering, Wind River
> 
> ________________________________
> From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Sent: Saturday, June 27, 2026 3:57
> To: Steve Wahl <steve.wahl@hpe.com>; Steve Wahl <steve.wahl@hpe.com>; Dave Jiang <dave.jiang@intel.com>; Vinod Koul <vkoul@kernel.org>; Frank Li <Frank.Li@kernel.org>; dmaengine@vger.kernel.org <dmaengine@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
> Cc: Russ Anderson <rja@hpe.com>; Dimitri Sivanich <sivanich@hpe.com>; Codres, Bogdan <Bogdan.Codres@windriver.com>
> Subject: Re: [PATCH v2 2/2] dmaengine: idxd: fix duplicate memory frees on initialization error path.
> 
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> Steve Wahl <steve.wahl@hpe.com> writes:
> 
> > Error paths within idxd_pci_probe_alloc and related functions end up
> > attempting to free memory already freed from idxd_conf_device_release
> > via put_device.
> >
> > This was encountered running in a kexec'd kdump kernel with reduced
> > resources, causing the "Device is HALTED!" branch in
> > idxd_device_init_reset to be taken.
> >
> > In idxd_free and idxd_alloc, do not attempt to free allocations that
> > will already have been freed.
> >
> > Signed-off-by: Steve Wahl <steve.wahl@hpe.com>
> > ---
> 
> Bogdan Codres series (but submitted a bit later), tries to fix this
> issue, has a better splat and a reproducer, I think it makes sense to
> add the splat and reproducer here, and add Bogdan as Reported-by (if
> agreed, of course).
> 
> I am wondering about adding a third patch for the dangling ->wq pointer, as
> reported by Sashiko would make sense.
> 
> Something like this might do the job (totally untested):
> 
> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index f1cfc7790d95..0a74018f31a8 100644
> --- a/drivers/dma/idxd/init.c
> +++ b/drivers/dma/idxd/init.c
> @@ -415,6 +415,7 @@ static void idxd_cleanup_internals(struct idxd_device *idxd)
>         idxd_clean_engines(idxd);
>         idxd_clean_wqs(idxd);
>         destroy_workqueue(idxd->wq);
> +       idxd->wq = NULL;
>  }
> 
> How does it sound?
> 
> > v2: split into two patches as requested by Vinicius Costa
> >
> >  drivers/dma/idxd/init.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> > index f1cfc7790d95..227e323cc5a0 100644
> > --- a/drivers/dma/idxd/init.c
> > +++ b/drivers/dma/idxd/init.c
> > @@ -607,9 +607,6 @@ static void idxd_free(struct idxd_device *idxd)
> >               return;
> >
> >       put_device(idxd_confdev(idxd));
> > -     bitmap_free(idxd->opcap_bmap);
> > -     ida_free(&idxd_ida, idxd->id);
> > -     kfree(idxd);
> >  }
> >
> >  static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_data *data)
> > @@ -649,8 +646,13 @@ static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_d
> >       return idxd;
> >
> >  err_name:
> > +     /*
> > +      * once device_initialize(conf_dev) is called,
> > +      * put_device(conf_dev) will end up calling
> > +      * idxd_conf_device_release() which will free the rest.
> > +      */
> >       put_device(conf_dev);
> > -     bitmap_free(idxd->opcap_bmap);
> > +     return NULL;
> >  err_opcap:
> >       ida_free(&idxd_ida, idxd->id);
> >  err_ida:
> > --
> > 2.51.0
> >
> 
> --
> Vinicius

-- 
Steve Wahl, Hewlett Packard Enterprise