Re: [PATCH v5] dmaengine: idxd: Do not use devm for 'struct device' object allocation

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Mar 03, 2021 at 07:56:30AM -0700, Dave Jiang wrote:
> Remove devm_* allocation of memory of 'struct device' objects.
> The devm_* lifetime is incompatible with device->release() lifetime.
> Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE. Add release
> functions for each component in order to free the allocated memory at
> the appropriate time. Each component such as wq, engine, and group now
> needs to be allocated individually in order to setup the lifetime properly.
> In the process also fix up issues from the fallout of the changes.
> 
> Reported-by: Jason Gunthorpe <jgg@nvidia.com>
> Fixes: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel data accelerators")
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> v5:
> - Rebased against 5.12-rc dmaengine/fixes
> v4:
> - fix up the life time of cdev creation/destruction (Jason)
> - Tested with KASAN and other memory allocation leak detections. (Jason)
> 
> v3:
> - Remove devm_* for irq request and cleanup related bits (Jason)
> v2:
> - Remove all devm_* alloc for idxd_device (Jason)
> - Add kref dep for dma_dev (Jason)
> 
>  drivers/dma/idxd/cdev.c   |   32 +++---
>  drivers/dma/idxd/device.c |   20 ++-
>  drivers/dma/idxd/dma.c    |   13 ++
>  drivers/dma/idxd/idxd.h   |    8 +
>  drivers/dma/idxd/init.c   |  261 +++++++++++++++++++++++++++++++++------------
>  drivers/dma/idxd/irq.c    |    6 +
>  drivers/dma/idxd/sysfs.c  |   77 +++++++++----
>  7 files changed, 290 insertions(+), 127 deletions(-)
> 
> diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
> index 0db9b82ed8cf..1b98e06fa228 100644
> +++ b/drivers/dma/idxd/cdev.c
> @@ -259,6 +259,7 @@ static int idxd_wq_cdev_dev_setup(struct idxd_wq *wq)
>  		return -ENOMEM;
>  
>  	dev = idxd_cdev->dev;
> +	device_initialize(dev);
>  	dev->parent = &idxd->pdev->dev;
>  	dev_set_name(dev, "%s/wq%u.%u", idxd_get_dev_name(idxd),
>  		     idxd->id, wq->id);

dev_set_name() can fail

> @@ -268,25 +269,17 @@ static int idxd_wq_cdev_dev_setup(struct idxd_wq *wq)
>  	minor = ida_simple_get(&cdev_ctx->minor_ida, 0, MINORMASK, GFP_KERNEL);
>  	if (minor < 0) {
>  		rc = minor;
> -		kfree(dev);
>  		goto ida_err;

This doesn't work

>  	}
>  
>  	dev->devt = MKDEV(MAJOR(cdev_ctx->devt), minor);
>  	dev->type = &idxd_cdev_device_type;

Because this hasn't been done yet and release is thus NULL, will leak memory.

Also the order here is wrong:

	rc = cdev_device_add(cdev, dev);
	 [..]
	init_waitqueue_head(&idxd_cdev->err_queue);

Userspace can race a call to poll() before err_queue is setup.

And probably more. Please check your code carefully!

Jason