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] [day] [month] [year] [list]

Message-ID: <20251109135412.1b65f7da@jic23-huawei>
Date: Sun, 9 Nov 2025 13:54:12 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Nuno Sá <noname.nuno@...il.com>
Cc: Andy Shevchenko <andriy.shevchenko@...el.com>, Ma Ke
 <make24@...as.ac.cn>, 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 Sat, 08 Nov 2025 10:26:21 +0000
Nuno Sá <noname.nuno@...il.com> wrote:

> On Fri, 2025-11-07 at 20:19 +0200, Andy Shevchenko wrote:
> > 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.
> >   
> 
> Sure, agreed on that.
> 
> > 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  
> 
> FWIW and unless I'm missing something there's nothing fundamentally wrong in the
> current code (i.e any real bug). That said, I would ack a change that moved the
> device initialization code to it's natural place (at least in the way I see it).
Agreed.  I think this is simpler change. The other one that's be be happy with
would be to adjust the release to always work so we only have a put_device()
in here and no other error cleanup. Which means a flag or using whether the
irq_chip is set to split the two things currently under the single condition.

Jonathan

> 
> - Nuno Sá