Re: [PATCH] dmaengine: idxd: fix problems on initialization error path.

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete Fix / Double Free & Double Destroy
- [High] Immediate Double Free in child device cleanup functions
- [High] Use-After-Free and Double Free due to dangling pointers in registration rollback
--

commit 36d73f2b6ed131031e13332b322a76ce56830ad7
Author: Steve Wahl <steve.wahl@hpe.com>
Subject: dmaengine: idxd: fix problems on initialization error path.

This patch fixes double frees and NULL pointer dereferences in the IDXD
initialization error paths by ensuring allocations are freed only once
during the device release callback, and checking if the workqueue exists
before destroying it.

> 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

[ ... ]

> @@ -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 is a pre-existing issue, but does a similar pattern cause immediate double
frees in the child device cleanup functions?

In functions like idxd_clean_wqs(), idxd_clean_groups(), and
idxd_clean_engines() (as well as their setup error paths), cleanup is performed
by calling put_device() on the child kobjects.

This drops the reference count to 0 and synchronously invokes the sysfs release
callback (e.g., idxd_conf_wq_release(), idxd_conf_group_release()), which safely
frees the structure (kfree(wq)) and its internal allocations.

However, immediately after put_device() returns, these cleanup functions
explicitly call kfree() on the exact same device structure (and inner pointers
like wq->wqcfg). Will this result in immediate double frees when unloading the
driver or failing initialization?


[Severity: High]
This isn't a bug introduced by this patch, but are there dangling pointers in
the registration rollback that cause a use-after-free?

If idxd_register_wq_devices() (or equivalent engine/group registration
functions) fails, its error rollback drops references to all child devices via
put_device() or device_unregister(), which invokes their release callbacks and
frees them.

It does not remove the dangling pointers from the idxd->wqs (or groups/engines)
arrays.

The error then bubbles up to idxd_pci_probe_alloc(), which jumps to
err_dev_register and calls idxd_cleanup(). This eventually calls
idxd_clean_wqs(), which iterates over the full idxd->wqs array and
unconditionally calls put_device() and kfree() on the already-freed pointers.

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

[ ... ]

> diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
> index 6d251095c350f..d5ffc641c8567 100644
> --- a/drivers/dma/idxd/sysfs.c
> +++ b/drivers/dma/idxd/sysfs.c
> @@ -1836,7 +1836,8 @@ static void idxd_conf_device_release(struct device *dev)
>  {
>  	struct idxd_device *idxd = confdev_to_idxd(dev);
>  
> -	destroy_workqueue(idxd->wq);
> +	if (idxd->wq)
> +		destroy_workqueue(idxd->wq);

[Severity: High]
This isn't a bug introduced by this patch, but do the error paths from
idxd_probe() still cause a double free and double destroy?

If an error occurs in idxd_probe() (e.g., idxd_device_load_config() fails),
idxd_cleanup_internals() or idxd_setup_internals() error paths are invoked.
These functions manually free arrays (idxd->groups, idxd->wqs, idxd->engines)
and destroy the workqueue (destroy_workqueue(idxd->wq)) but leave the pointers
non-NULL.

When the error propagates up to idxd_pci_probe_alloc(), it calls idxd_free()
which calls put_device(). This triggers idxd_conf_device_release().

Because the pointers are still non-NULL, idxd_conf_device_release()
unconditionally calls kfree() on the arrays a second time and executes
destroy_workqueue(idxd->wq) again on the already-destroyed workqueue.

>  	kfree(idxd->groups);
>  	bitmap_free(idxd->wq_enable_map);
>  	kfree(idxd->wqs);

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