On 3/4/2021 11:03 AM, Jason Gunthorpe wrote:
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.
Uck! I fixed this in one version somewhere and dropped it somehow. :(
Thanks.
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.
Ok will fix.
And probably more. Please check your code carefully!
Sometimes you stare at your own code so much that you just stop seeing
things. :( Really do appreciate you finding the issues. I'm also looking
to deprecate this char device code and convert it over to UACCE soon.
Jason
