[PATCH v3] iio: trigger: Fix error handling in viio_trigger_alloc

[PATCH v3] iio: trigger: Fix error handling in viio_trigger_alloc

[Ma Ke]

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.

Additionally, the current error handling has the following issues:
1. Potential double-free of IRQ descriptors when kvasprintf() fails.
2. The release function may attempt to free negative subirq_base.
3. Missing mutex_destroy() in release function.

Fix these issues by:
1. Replacing kfree(trig) with put_device(&trig->dev) in error paths.
2. Removing the free_descs label and handling IRQ descriptor freeing
   directly in the kvasprintf() error path.
3. Adding missing mutex_destroy().

Found by code review.

Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
 drivers/iio/industrialio-trigger.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index 54416a384232..7576dedee68e 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -524,6 +524,7 @@ static void iio_trig_release(struct device *device)
 			       CONFIG_IIO_CONSUMERS_PER_TRIGGER);
 	}
 	kfree(trig->name);
+	mutex_destroy(&trig->pool_lock);
 	kfree(trig);
 }
 
@@ -575,8 +576,10 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent,
 		goto free_trig;
 
 	trig->name = kvasprintf(GFP_KERNEL, fmt, vargs);
-	if (trig->name == NULL)
-		goto free_descs;
+	if (trig->name == NULL) {
+		irq_free_descs(trig->subirq_base, CONFIG_IIO_CONSUMERS_PER_TRIGGER);
+		goto free_trig;
+	}
 
 	INIT_LIST_HEAD(&trig->list);
 
@@ -594,10 +597,8 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent,
 
 	return trig;
 
-free_descs:
-	irq_free_descs(trig->subirq_base, CONFIG_IIO_CONSUMERS_PER_TRIGGER);
 free_trig:
-	kfree(trig);
+	put_device(&trig->dev);
 	return NULL;
 }
 
-- 
2.17.1

Re: [PATCH v3] iio: trigger: Fix error handling in viio_trigger_alloc

[Andy Shevchenko]

On Fri, Nov 7, 2025 at 4:02 AM Ma Ke <make24@iscas.ac.cn> 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.
>
> Additionally, the current error handling has the following issues:
> 1. Potential double-free of IRQ descriptors when kvasprintf() fails.
> 2. The release function may attempt to free negative subirq_base.
> 3. Missing mutex_destroy() in release function.
>
> Fix these issues by:
> 1. Replacing kfree(trig) with put_device(&trig->dev) in error paths.
> 2. Removing the free_descs label and handling IRQ descriptor freeing
>    directly in the kvasprintf() error path.
> 3. Adding missing mutex_destroy().
>
> Found by code review.

...

>         trig->name = kvasprintf(GFP_KERNEL, fmt, vargs);
> -       if (trig->name == NULL)
> -               goto free_descs;
> +       if (trig->name == NULL) {

> +               irq_free_descs(trig->subirq_base, CONFIG_IIO_CONSUMERS_PER_TRIGGER);

Why?

> +               goto free_trig;
> +       }

Please, slow down and think a bit. Also you may split the patch to at least two:
1) add missed mutex_destroy();
2) fix the clean up in the error path.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] iio: trigger: Fix error handling in viio_trigger_alloc

[Nuno Sá]

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.
> 
> Additionally, the current error handling has the following issues:
> 1. Potential double-free of IRQ descriptors when kvasprintf() fails.

How? If I'm not missing nothing, your patch is the one bringing the above
issue.

> 2. The release function may attempt to free negative subirq_base.

Same question, how?

> 3. Missing mutex_destroy() in release function.
> 

Mostly important for debugging mutexes but not super important.

> Fix these issues by:
> 1. Replacing kfree(trig) with put_device(&trig->dev) in error paths.
> 2. Removing the free_descs label and handling IRQ descriptor freeing
>    directly in the kvasprintf() error path.
> 3. Adding missing mutex_destroy().
> 
> Found by code review.
> 
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> ---
>  drivers/iio/industrialio-trigger.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-
> trigger.c
> index 54416a384232..7576dedee68e 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -524,6 +524,7 @@ static void iio_trig_release(struct device *device)
>  			       CONFIG_IIO_CONSUMERS_PER_TRIGGER);
>  	}
>  	kfree(trig->name);

Not that this is a problem, but now you can actually get in here with trig->name =
NULL. And typically that's not a get pattern for your code.

> +	mutex_destroy(&trig->pool_lock);
>  	kfree(trig);
>  }
>  
> @@ -575,8 +576,10 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent,
>  		goto free_trig;
>  
>  	trig->name = kvasprintf(GFP_KERNEL, fmt, vargs);
> -	if (trig->name == NULL)
> -		goto free_descs;
> +	if (trig->name == NULL) {
> +		irq_free_descs(trig->subirq_base,
> CONFIG_IIO_CONSUMERS_PER_TRIGGER);
> +		goto free_trig;

I think now we do have double free of irq_free_descs(), or am I missing something?

> +	}
>  
>  	INIT_LIST_HEAD(&trig->list);
>  
> @@ -594,10 +597,8 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent,
>  
>  	return trig;
>  
> -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.

- Nuno Sá


>  	return NULL;
>  }
>  

Re: [PATCH v3] iio: trigger: Fix error handling in viio_trigger_alloc

[Andy Shevchenko]

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.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] iio: trigger: Fix error handling in viio_trigger_alloc

[Nuno Sá]

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á

Re: [PATCH v3] iio: trigger: Fix error handling in viio_trigger_alloc

[Andy Shevchenko]

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

Re: [PATCH v3] iio: trigger: Fix error handling in viio_trigger_alloc

[Nuno Sá]

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).

- Nuno Sá

Re: [PATCH v3] iio: trigger: Fix error handling in viio_trigger_alloc

[Jonathan Cameron]

On Sat, 08 Nov 2025 10:26:21 +0000
Nuno Sá <noname.nuno@gmail.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á