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

[Vaittinen, Matti]

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

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

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