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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 19 Sep 2022 18:06:37 +0000
"Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote:

> 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 :)

I'm never one to turn down a volunteer, so I'll leave these for you :)

Plenty of other things on the todo list that I can be getting on with.

Jonathan

> 
> Yours
> 	-- Matti
> 
>