lkml.org 
[lkml]   [2022]   [Sep]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFT] potential bug with IIO_CONST_ATTR usage with triggered buffers
    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
    >
    >

    \
     
     \ /
      Last update: 2022-09-24 15:50    [W:4.109 / U:0.820 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site