On Mon, 19 Sep 2022 16:32:14 +0100
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> On Mon, 19 Sep 2022 08:52:38 +0000
> "Vaittinen, Matti" <Matti.Vaittinen@xxxxxxxxxxxxxxxxx> 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!
>
> 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.
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).
I think this combination of fix then prevent future problems is what
I would prefer.
Jonathan
>
> An alternative would be to make it 'safe' by making the data layouts
> match up.
>
> struct iio_attr {
> struct device_attribute dev_attr;
> union {
> u64 address;
> const char *string;
> };
> struct list_head l;
> struct iio_chan_spec const *c;
> struct iio_buffer *buffer;
> };
>
> #define iio_dev_attr iio_attr
> #define iio_const_attr iio_attr
>
> Looking at this raises another potential problem.
> Where is the address copied over for attributes using IIO_DEVICE_ATTR()?
> Maybe I'm just missing it somewhere. Grepping suggests we've been
> lucky and there are no users of that field in buffer attributes.
>
> Detecting the problem you found is going to be inherently tricky - though maybe
> could rely on the naming of the attributes passed in (iio_const...)
> and some scripting magic.
>
> Longer term, it's this sort of thing that motivates protections / runnable
> CI self tests with, for example, the roadtest framework that I'm hoping
> will be available upstream soonish!
>
> Would you like to send patches given you identified the problem?
>
> If not I'm happy to fix these up. My grepping identified the same 3 cases
> you found.
>
> Jonathan
>
