| Message ID | 20211024092700.6844-2-lars@metafoo.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/2] iio: mma8452: Fix trigger reference couting | expand |
> -----Original Message----- > From: Lars-Peter Clausen <lars@metafoo.de> > Sent: Sunday, October 24, 2021 11:27 AM > To: Jonathan Cameron <jic23@kernel.org> > Cc: Martin Fuzzey <mfuzzey@parkeon.com>; Peter Meerwald-Stadler > <pmeerw@pmeerw.net>; linux-iio@vger.kernel.org; Lars-Peter > Clausen <lars@metafoo.de> > Subject: [PATCH 2/2] iio: trigger: Fix reference counting > > [External] > > In viio_trigger_alloc() device_initialize() is used to set the initial > reference count of the trigger to 1. Then another get_device() is called > on > trigger. This sets the reference count to 2 before the trigger is > returned. > > iio_trigger_free(), which is the matching API to viio_trigger_alloc(), > calls put_device() which decreases the reference count by 1. But the > second > reference count acquired in viio_trigger_alloc() is never dropped. > > As a result the iio_trigger_release() function is never called and the > memory associated with the trigger is never freed. > > Since there is no reason for the trigger to start its lifetime with two > reference counts just remove the extra get_device() in > viio_trigger_alloc(). > > Fixes: 5f9c035cae18 ("staging:iio:triggers. Add a reference get to the > core for triggers.") > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> Acked-by: Nuno Sá <nuno.sa@analog.com> > --- > I'm a bit unsure about the fixes tag. I've looked at the IIO tree at the > point when this was introduced and I believe it was incorrect even > back > then. > > But we also had a few drivers that directly assigned the indio_dev->trig > without getting an extra reference. So these two bugs, one in the > core, one > in the drivers sort of even out. Except that iio_trigger_get() also gets a > reference to the drivers module and iio_trigger_put() releases it again. > So > with the missing iio_trigger_get() there is still the problem that, even > though the device references balance out, there is a module reference > count > imbalance. > --- > 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 b23caa2f2aa1..93990ff1dfe3 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.20.1
On Sun, 24 Oct 2021 11:27:00 +0200 Lars-Peter Clausen <lars@metafoo.de> wrote: > In viio_trigger_alloc() device_initialize() is used to set the initial > reference count of the trigger to 1. Then another get_device() is called on > trigger. This sets the reference count to 2 before the trigger is returned. > > iio_trigger_free(), which is the matching API to viio_trigger_alloc(), > calls put_device() which decreases the reference count by 1. But the second > reference count acquired in viio_trigger_alloc() is never dropped. > > As a result the iio_trigger_release() function is never called and the > memory associated with the trigger is never freed. > > Since there is no reason for the trigger to start its lifetime with two > reference counts just remove the extra get_device() in > viio_trigger_alloc(). > > Fixes: 5f9c035cae18 ("staging:iio:triggers. Add a reference get to the core for triggers.") > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> I fully agree the current code is wrong, but we really should be using device_put() in the error path after device_initialize() has been called. There are multiple places where we currently do this wrong in IIO but this particular one looks like a local fix would be safe. Worth doing that in the same patch at this one given it's all about reference counting logic being wrong? If not, we can do it as a separate follow up patch. Jonathan > --- > I'm a bit unsure about the fixes tag. I've looked at the IIO tree at the > point when this was introduced and I believe it was incorrect even back > then. > > But we also had a few drivers that directly assigned the indio_dev->trig > without getting an extra reference. So these two bugs, one in the core, one > in the drivers sort of even out. Except that iio_trigger_get() also gets a > reference to the drivers module and iio_trigger_put() releases it again. So > with the missing iio_trigger_get() there is still the problem that, even > though the device references balance out, there is a module reference count > imbalance. > --- > 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 b23caa2f2aa1..93990ff1dfe3 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; >
On 10/28/21 4:16 PM, Jonathan Cameron wrote: > On Sun, 24 Oct 2021 11:27:00 +0200 > Lars-Peter Clausen <lars@metafoo.de> wrote: > >> In viio_trigger_alloc() device_initialize() is used to set the initial >> reference count of the trigger to 1. Then another get_device() is called on >> trigger. This sets the reference count to 2 before the trigger is returned. >> >> iio_trigger_free(), which is the matching API to viio_trigger_alloc(), >> calls put_device() which decreases the reference count by 1. But the second >> reference count acquired in viio_trigger_alloc() is never dropped. >> >> As a result the iio_trigger_release() function is never called and the >> memory associated with the trigger is never freed. >> >> Since there is no reason for the trigger to start its lifetime with two >> reference counts just remove the extra get_device() in >> viio_trigger_alloc(). >> >> Fixes: 5f9c035cae18 ("staging:iio:triggers. Add a reference get to the core for triggers.") >> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > I fully agree the current code is wrong, but we really should be using > device_put() in the error path after device_initialize() has been called. > > There are multiple places where we currently do this wrong in IIO but this particular > one looks like a local fix would be safe. > Worth doing that in the same patch at this one given it's all about reference > counting logic being wrong? If not, we can do it as a separate follow up patch. I already have that patch. Just waiting for this to be applied since it has a dependency. > > Jonathan > > >> --- >> I'm a bit unsure about the fixes tag. I've looked at the IIO tree at the >> point when this was introduced and I believe it was incorrect even back >> then. >> >> But we also had a few drivers that directly assigned the indio_dev->trig >> without getting an extra reference. So these two bugs, one in the core, one >> in the drivers sort of even out. Except that iio_trigger_get() also gets a >> reference to the drivers module and iio_trigger_put() releases it again. So >> with the missing iio_trigger_get() there is still the problem that, even >> though the device references balance out, there is a module reference count >> imbalance. >> --- >> 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 b23caa2f2aa1..93990ff1dfe3 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; >>
On Thu, 28 Oct 2021 18:04:22 +0200 Lars-Peter Clausen <lars@metafoo.de> wrote: > On 10/28/21 4:16 PM, Jonathan Cameron wrote: > > On Sun, 24 Oct 2021 11:27:00 +0200 > > Lars-Peter Clausen <lars@metafoo.de> wrote: > > > >> In viio_trigger_alloc() device_initialize() is used to set the initial > >> reference count of the trigger to 1. Then another get_device() is called on > >> trigger. This sets the reference count to 2 before the trigger is returned. > >> > >> iio_trigger_free(), which is the matching API to viio_trigger_alloc(), > >> calls put_device() which decreases the reference count by 1. But the second > >> reference count acquired in viio_trigger_alloc() is never dropped. > >> > >> As a result the iio_trigger_release() function is never called and the > >> memory associated with the trigger is never freed. > >> > >> Since there is no reason for the trigger to start its lifetime with two > >> reference counts just remove the extra get_device() in > >> viio_trigger_alloc(). > >> > >> Fixes: 5f9c035cae18 ("staging:iio:triggers. Add a reference get to the core for triggers.") > >> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > > I fully agree the current code is wrong, but we really should be using > > device_put() in the error path after device_initialize() has been called. > > > > There are multiple places where we currently do this wrong in IIO but this particular > > one looks like a local fix would be safe. > > Worth doing that in the same patch at this one given it's all about reference > > counting logic being wrong? If not, we can do it as a separate follow up patch. > I already have that patch. Just waiting for this to be applied since it > has a dependency. In that case, applied for this one to the fixes-togreg branch of iio.git. Thanks, Jonathan > > > > Jonathan > > > > > >> --- > >> I'm a bit unsure about the fixes tag. I've looked at the IIO tree at the > >> point when this was introduced and I believe it was incorrect even back > >> then. > >> > >> But we also had a few drivers that directly assigned the indio_dev->trig > >> without getting an extra reference. So these two bugs, one in the core, one > >> in the drivers sort of even out. Except that iio_trigger_get() also gets a > >> reference to the drivers module and iio_trigger_put() releases it again. So > >> with the missing iio_trigger_get() there is still the problem that, even > >> though the device references balance out, there is a module reference count > >> imbalance. > >> --- > >> 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 b23caa2f2aa1..93990ff1dfe3 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 b23caa2f2aa1..93990ff1dfe3 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;
In viio_trigger_alloc() device_initialize() is used to set the initial reference count of the trigger to 1. Then another get_device() is called on trigger. This sets the reference count to 2 before the trigger is returned. iio_trigger_free(), which is the matching API to viio_trigger_alloc(), calls put_device() which decreases the reference count by 1. But the second reference count acquired in viio_trigger_alloc() is never dropped. As a result the iio_trigger_release() function is never called and the memory associated with the trigger is never freed. Since there is no reason for the trigger to start its lifetime with two reference counts just remove the extra get_device() in viio_trigger_alloc(). Fixes: 5f9c035cae18 ("staging:iio:triggers. Add a reference get to the core for triggers.") Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> --- I'm a bit unsure about the fixes tag. I've looked at the IIO tree at the point when this was introduced and I believe it was incorrect even back then. But we also had a few drivers that directly assigned the indio_dev->trig without getting an extra reference. So these two bugs, one in the core, one in the drivers sort of even out. Except that iio_trigger_get() also gets a reference to the drivers module and iio_trigger_put() releases it again. So with the missing iio_trigger_get() there is still the problem that, even though the device references balance out, there is a module reference count imbalance. --- drivers/iio/industrialio-trigger.c | 1 - 1 file changed, 1 deletion(-)