| Message ID | 20211008083220.3713926-1-gavinli@thegavinli.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: trigger: fix iio_trigger reference leak | expand |
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; >
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;
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(-)