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

Dave Jiang
2021-03-24T20:32:07Z

On 3/24/2021 12:52 PM, Dan Carpenter wrote:
> On Wed, Mar 24, 2021 at 01:52:46PM -0300, Jason Gunthorpe wrote:
>> On Wed, Mar 24, 2021 at 09:13:35AM -0700, Dan Williams wrote:
>>
>>> Which is just:
>>>
>>> device_initialize()
>>> dev_set_name()
>>>
>>> ...then the name is set as early as the device is ready to filled in
>>> with other details. Just checking for dev_set_name() failures does not
>>> move the api forward in my opinion.
>> This doesn't work either as the release function must be set after
>> initialize but before dev_set_name(), otherwise we both can't and must
>> call put_device() after something like this fails.
>>
>> I can't see an option other than bite the bullet and fix things.
>>
>> A static tool to look for these special lifetime rules around the
>> driver core would be nice.
> If y'all are specific enough about what you want, then I can write the
> check for you. What I really want is some buggy sample code and the
> warning you want me to print. I kind of vaguely know that devm_ life
> time rules are tricky but I don't know the details.
>
> https://lore.kernel.org/dmaengine/CAPcyv4g2Odzusx621vatPbA041NXMmc1JK_3oSNM-EOPwDaxqA@mail.gmail.com/T/#m18d39df4097b12a9a5bdf51bb86b31cd0c6c0136
>
> The error handling in idxd_setup_interrupts() and
> idxd_allocate_wqs/engines/groups() is faulty.
>
> + for (i = 0; i < idxd->max_engines; i++) {
> + engine = kzalloc_node(sizeof(*engine), GFP_KERNEL, dev_to_node(dev));
> + if (!engine) {
> + rc = -ENOMEM;
> + goto err;
>
> Imagine that kzalloc_node() fails on the first iteration.
>
> + }
> +
> + idxd->engines[i] = engine;
> + }
> +
> + return 0;
> +
> + err:
> + while (--i)
> + kfree(idxd->engines[i]);
>
> The while loop should be while (i--) or while (--i >= 0). Otherwise,
> --i is -1 so this will loop downwards until it crashes.

Thanks Dan. I'll fix that in the next rev.

> regards,
> dan carpenter
>