Re: [PATCH 2/2] iio: trigger: Fix reference counting

[Lars-Peter Clausen <lars@metafoo.de>]

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