linux-kernel - Re: [PATCH v3] iio: trigger: Fix error handling in viio_trigger

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9e96f49f3903f704e16e8dde540507b10a978951.camel@gmail.com>
Date: Fri, 07 Nov 2025 16:48:03 +0000
From: Nuno Sá <noname.nuno@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.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, 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.

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

- Nuno Sá