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

[Dave Jiang <dave.jiang@intel.com>]

On 2/26/2021 6:36 PM, Jason Gunthorpe wrote:
> On Wed, Feb 24, 2021 at 03:35:19PM -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.
> You've tested this now with kasn and all the other debugging turned
> on?

Only with DEBUG_KOBJECT_RELEASE. I wasn't aware of the kasn tests. I'll 
go test with those. Thanks for the thorough review. Really appreciate it.


> I poked around a bit and there are other bugs in here too:
>
> static int idxd_wq_cdev_dev_setup(struct idxd_wq *wq)
> {
>          idxd_cdev->dev = kzalloc(sizeof(*idxd_cdev->dev), GFP_KERNEL);
>          dev_set_name(dev, "%s/wq%u.%u", idxd_get_dev_name(idxd), idxd->id, wq->id);
>          ^^^ Missing error check
>
>          if (minor < 0) {
>                  rc = minor;
>                  kfree(dev);
>                  ^^^^ leaks the memory dev_set_name allocated
>
> You must call device_initialize before calling dev_set_name and once
> device_initialize is called it must do put_device to clean
> up. put_device will free memory allocated by dev_set_name
>
> Isn't this a use after free kasn should flag?
>
>          device_unregister(idxd_cdev->dev);
>          ida_simple_remove(&cdev_ctx->minor_ida, idxd_cdev->minor);
>                                                  ^^^^^^
>                                               idxd_cdev may
> 					have been freed by device_unregister
>
> Probably a good idea to check all the places working with struct
> device carefully to see that they are right
>
> Jason