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

[Dan Carpenter <dan.carpenter@oracle.com>]

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.

regards,
dan carpenter