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

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

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
>