lkml.org 
[lkml]   [2022]   [Sep]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [RFT] potential bug with IIO_CONST_ATTR usage with triggered buffers
    Date
    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! ~~
    \
     
     \ /
      Last update: 2022-09-19 20:07    [W:4.167 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site