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

Dave Jiang
2021-03-04T18:27:24Z

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@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.

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