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

["Nuno Sá" <noname.nuno@gmail.com>]

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;
>  }
>