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@xxxxxxxxxx>
> Fixes: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel data accelerators")
> Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> 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
