Re: [RFT] potential bug with IIO_CONST_ATTR usage with triggered buffers

["Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>]

On 9/19/22 20:18, Jonathan Cameron wrote:
> On Mon, 19 Sep 2022 16:32:14 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
>> On Mon, 19 Sep 2022 08:52:38 +0000
>> "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote:
>>
>>> On 9/9/22 11:12, Vaittinen, Matti wrote:
>>>> Hi dee Ho peeps!
>>>>
>>>> Disclaimer - I have no HW to test this using real in-tree drivers. If
>>>> someone has a device with a variant of bmc150 or adxl372 or  - it'd be
>>>> nice to see if reading hwfifo_watermark_max or hwfifo_watermark_min
>>>> works with the v6.0-rc4. Maybe I am misreading code and have my own
>>>> issues - in which case I apologize already now and go to the corner
>>>> while being deeply ashamed :)
>>>
>>> I would like to add at least the at91-sama5d2_adc (conditonally
>>> registers the IIO_CONST_ATTR for triggered-buffer) to the list of
>>> devices that could be potentially tested. I hope some of these devices
>>> had a user who could either make us worried and verify my assumption -
>>> or make me ashamed but rest of us relieved :) Eg - I second my request
>>> for testing this - and add potential owners of at91-sama5d2_adc to the list.
>>>    
>>>> On 2/15/21 12:40, Alexandru Ardelean wrote:
>>>>> This change wraps all buffer attributes into iio_dev_attr objects, and
>>>>> assigns a reference to the IIO buffer they belong to.
>>>>>
>>>>> With the addition of multiple IIO buffers per one IIO device, we need a way
>>>>> to know which IIO buffer is being enabled/disabled/controlled.
>>>>>
>>>>> We know that all buffer attributes are device_attributes.
>>>>
>>>> I think this assumption is slightly unsafe. I see few drivers adding
>>>> IIO_CONST_ATTRs in attribute groups. For example the bmc150 and adxl372
>>>> add the hwfifo_watermark_min and hwfifo_watermark_max.
>>>>     
>>>
>>> and at91-sama5d2_adc
>>>
>>> //snip
>>>    
>>>> I noticed that using
>>>> IIO_CONST_ATTRs for triggered buffers seem to cause access to somewhere
>>>> it shouldn't... Oops.
>>>>
>>>> Reading the code allows me to assume the problem is wrapping the
>>>> attributes to IIO_DEV_ATTRs.
>>>>
>>>> static struct attribute *iio_buffer_wrap_attr(struct iio_buffer *buffer,
>>>> +					      struct attribute *attr)
>>>> +{
>>>> +	struct device_attribute *dattr = to_dev_attr(attr);
>>>> +	struct iio_dev_attr *iio_attr;
>>>> +
>>>> +	iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL);
>>>> +	if (!iio_attr)
>>>> +		return NULL;
>>>> +
>>>> +	iio_attr->buffer = buffer;
>>>> +	memcpy(&iio_attr->dev_attr, dattr, sizeof(iio_attr->dev_attr));
>>>>
>>>> This copy does assume all attributes are device_attrs, and does not take
>>>> into account that IIO_CONST_ATTRS have the string stored in a struct
>>>> iio_const_attr which is containing the dev_attr. Eg, copying in the
>>>> iio_buffer_wrap_attr() does not copy the string - and later invoking the
>>>> 'show' callback goes reading something else than the mentioned string
>>>> because the pointer is not copied.
>>>
>>> Yours,
>>> 	-- Matti
>> Hi Matti,
>>
>> +CC Alexandru on a current email address.
>>
>> I saw this whilst travelling and completely forgot about when
>> I was back to normal - so great you sent a follow up!

I was also participating at ELCE last week so didn't do much of emails/code.

>>
>> Anyhow, your reasoning seems correct and it would be easy enough
>> to add such a case to iio/dummy/iio_simple_dummy_buffer.c and
>> provide a clear test for the problem.
>>
>> As to solutions. The quickest is probably to switch these const attrs
>> over to a non const form and add a comment to the header to say they are
>> unsuitable for use with buffers.
> 
> Thinking a little more on this - all / (most?) of the users pass a null terminated
> array of struct device_attribute * to *iio_triggered_buffer_setup_ext()
> 
> That's then assigned to buffer->attrs.
> We could add an additional pointer to the struct iio_buffer to take
> a null terminated array of struct iio_dev_attr *
> and change the signature of that function to take one of those, thus
> preventing us using iio_const_attr structures for this.

Yes. I would also rather see pointer to array of struct iio_dev_attr * 
if we continue keeping the assumption that attrs are of type iio_dev_attr.

> 
> Then we can wrap those just fine in the code you highlighted and assign the
> result into buffer->attrs.
> 
> We'd need to precede that change with fixes that just switch the
> iio_const_attr uses over to iio_dev_attr but changing this would ensure no
> accidental reintroductions of the problem in future drivers (typically
> as a result of someone forward porting a driver that is out of tree).

Again I do agree. Besides change of const_attrs is necessary in any case 
if we don't change the wrapping.

>>
>> Would you like to send patches given you identified the problem?

I am in any case about to send couple of patches to IIO. The devm-helper 
usage (v2 - I sent v1 from my other email address (mazziesaccount) - but 
I am the same person :] ) and a new accelerometer driver. So, I can look 
also at this change while I am at it if you're busy).

>> If not I'm happy to fix these up. My grepping identified the same 3 cases
>> you found.

Feel free to patch this if you wish. Just please let me know if you take 
care of this so we don't do double the work :)

Yours
	-- Matti


-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~