[PATCH] iio: trigger: fix iio_trigger reference leak

[PATCH] iio: trigger: fix iio_trigger reference leak

[gavinli]

From: Gavin Li <gavin@matician.com>

viio_trigger_alloc() includes a get_device() call added in commit
5f9c035cae back in 2011. While I wasn't able to ascertain why it was
added, I've noticed that it does cause a memory leak, especially when
rmmod'ing iio modules. Here is a simple module that replicates this
issue:

    #include <linux/module.h>
    #include <linux/iio/iio.h>
    #include <linux/iio/trigger.h>

    int my_init(void) {
        struct iio_trigger *trig = iio_trigger_alloc("leak-trig");
        if (!trig)
            return -ENOMEM;

        printk("iio-leak: %u uses at A\n", kref_read(&trig->dev.kobj.kref));
        iio_trigger_free(trig);
        printk("iio-leak: %u uses at B\n", kref_read(&trig->dev.kobj.kref));

        return 0;
    }

    void my_exit(void) {}

    module_init(my_init);
    module_exit(my_exit);
    MODULE_LICENSE("GPL v2");

It prints the following in the kernel log:

    iio-leak: 2 uses at A
    iio-leak: 1 uses at B

This patch removes the get_device() call. This shouldn't break
subirqs with iio_trigger_attach_poll_func() because a reference should
already exist via indio_dev->trig.
---
 drivers/iio/industrialio-trigger.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index b23caa2f2aa1f..93990ff1dfe39 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -556,7 +556,6 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent,
 		irq_modify_status(trig->subirq_base + i,
 				  IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE);
 	}
-	get_device(&trig->dev);
 
 	return trig;
 
-- 
2.33.0

Re: [PATCH] iio: trigger: fix iio_trigger reference leak

[Jonathan Cameron]

On Fri,  8 Oct 2021 01:32:20 -0700
gavinli@thegavinli.com wrote:

> From: Gavin Li <gavin@matician.com>
> 
> viio_trigger_alloc() includes a get_device() call added in commit
> 5f9c035cae back in 2011. While I wasn't able to ascertain why it was
> added, I've noticed that it does cause a memory leak, especially when
> rmmod'ing iio modules. Here is a simple module that replicates this
> issue:
> 
>     #include <linux/module.h>
>     #include <linux/iio/iio.h>
>     #include <linux/iio/trigger.h>
> 
>     int my_init(void) {
>         struct iio_trigger *trig = iio_trigger_alloc("leak-trig");
>         if (!trig)
>             return -ENOMEM;
> 
>         printk("iio-leak: %u uses at A\n", kref_read(&trig->dev.kobj.kref));
>         iio_trigger_free(trig);
>         printk("iio-leak: %u uses at B\n", kref_read(&trig->dev.kobj.kref));
> 
>         return 0;
>     }
> 
>     void my_exit(void) {}
> 
>     module_init(my_init);
>     module_exit(my_exit);
>     MODULE_LICENSE("GPL v2");
> 
> It prints the following in the kernel log:
> 
>     iio-leak: 2 uses at A
>     iio-leak: 1 uses at B
> 
> This patch removes the get_device() call. This shouldn't break
> subirqs with iio_trigger_attach_poll_func() because a reference should
> already exist via indio_dev->trig.

Agreed, it is most mysterious...  I can't apply this patch though without a
Signed-off-by: ...

as even though it's trivial we need a developer certificate of origin.
(represented by the Signed-off-by line)

Thanks,

Jonathan

> ---
>  drivers/iio/industrialio-trigger.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index b23caa2f2aa1f..93990ff1dfe39 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -556,7 +556,6 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent,
>  		irq_modify_status(trig->subirq_base + i,
>  				  IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE);
>  	}
> -	get_device(&trig->dev);
>  
>  	return trig;
>