Message-ID: <aQ44IB1b7AXun_qN@smile.fi.intel.com>
Date: Fri, 7 Nov 2025 20:19:12 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Nuno Sá <noname.nuno@...il.com>
Cc: Ma Ke <make24@...as.ac.cn>, jic23@...nel.org, dlechner@...libre.com,
	nuno.sa@...log.com, andy@...nel.org, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org
Subject: Re: [PATCH v3] iio: trigger: Fix error handling in viio_trigger_alloc

On Fri, Nov 07, 2025 at 04:48:03PM +0000, Nuno Sá wrote:
> On Fri, 2025-11-07 at 12:42 +0200, Andy Shevchenko wrote:
> > On Fri, Nov 07, 2025 at 10:26:10AM +0000, Nuno Sá wrote:
> > > On Fri, 2025-11-07 at 10:02 +0800, Ma Ke wrote:
> > > > viio_trigger_alloc() initializes the device with device_initialize()
> > > > but uses kfree() directly in error paths, which bypasses the device's
> > > > release callback iio_trig_release(). This could lead to memory leaks
> > > > and inconsistent device state.

...

> > > > -free_descs:
> > > > -	irq_free_descs(trig->subirq_base, CONFIG_IIO_CONSUMERS_PER_TRIGGER);
> > > >  free_trig:
> > > > -	kfree(trig);
> > > > +	put_device(&trig->dev);
> > > 
> > > Yes, device_initialize() docs do say that we should give the reference instead of
> > > freeing the device but I'm not see how that helps in here. Maybe initializing the
> > > device should be done only after all the resources are allocated so the code is a
> > > bit
> > > more clear... But doing it like you're doing just means that we might get into
> > > the
> > > release function with things that might or might not be allocated which is a
> > > pattern
> > > I would prefer to avoid.
> > 
> > The put_device() here is the correct (and must) thing to do independently on
> > the preferences. The problem is that device_initialise() and followed calls
> > may do much more than just some initialisation.
> 
> Well, I would argue against that (at least in the context the function is now
> implemented). To me, the right thing to do would be to move the device initialization
> code to this point:
> 
> https://elixir.bootlin.com/linux/v6.17.7/source/drivers/iio/industrialio-trigger.c#L594
> 
> trig->dev.parent = parent;
> trig->dev.type = &iio_trig_type;
> trig->dev.bus = &iio_bus_type;
> device_initialize(&trig->dev);
> 
> Then we would not even need to think about put_device(). Like it is, using it, it's
> just prone to errors (I did mentioned a couple of things this patch introduced If I'm
> not overseeing it) or we do need to have lots of care in the release function to make
> sure we don't mess up. To me that's a bad sign on how the code is architectured. 
> 
> FWIW, the pattern you find for example in SPI is the natural one for me:
> 
> You have a spi_alloc_device() [1] that initialises struct device right in the end.
> Above it, kfree() as usual. Then the callers, will indeed use put_device() in their
> error paths.
> 
> So the pattern to me is to do device_initialize() after all resources of your device
> are allocated. So that after that point put_device() does not get you into some odd
> handling in the release callback.

Sure, this can be another approach. Whatever you, folks, prefer. But at least
the mutex_destroy() (separate) patch can be issued and accepted independently.

The bottom line is:
1) the current code has an issue;
2) the proposed fix has its own flaws;
3) but the idea in the current approach at least small (if implemented
correctly) and makes sure that any new allocations won't be forgotten in
the error patch, nor in the ->release() callback.

> [1]: https://elixir.bootlin.com/linux/v6.17.7/source/drivers/spi/spi.c#L568

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists