Hi Andy,
Thanks for taking the time to review :) Much appreciated.
On 10/3/22 11:43, Andy Shevchenko wrote:
On Mon, Oct 03, 2022 at 11:13:53AM +0300, Matti Vaittinen wrote:
The iio_triggered_buffer_setup_ext() and the
devm_iio_kfifo_buffer_setup_ext() were changed by
commit 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr")
to silently expect that all attributes given in buffer_attrs array are
device-attributes. This expectation was not forced by the API - and some
drivers did register attributes created by IIO_CONST_ATTR().
When using IIO_CONST_ATTRs the added attribute "wrapping" does not copy
the pointer to stored string constant and when the sysfs file is read the
kernel will access to invalid location.
Change the function signatures to expect an array of iio_dev_attrs to
avoid similar errors in the future.
...
Wouldn't be better to split this on per driver basis or is it impossible?
We need to change the callers and function signatures in one patch so we
don't break bisecting.
struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
struct iio_dev_attr *p;
+ const struct iio_dev_attr *id_attr;
I'm wondering if we may keep this upper, so "longer line goes first" rule would
be satisfied.
Sure.
struct attribute **attr;
int ret, i, attrn, scan_el_attrcount, buffer_attrcount;
const struct iio_chan_spec *channels;
...
+ for (i = 0, id_attr = buffer->attrs[i];
+ (id_attr = buffer->attrs[i]); i++)
Not sure why we have additional parentheses...
Because gcc warns about the assignment and suggests adding parenthesis
if we don't.
+ attr[ARRAY_SIZE(iio_buffer_attrs) + i] =
+ (struct attribute *)&id_attr->dev_attr.attr;
...and explicit casting here. Isn't attr is already of a struct attribute?
I am glad you asked :)
This is one of the "things" I was not really happy about. Here we hide
the fact that our array is full of pointers to _const_ data. If we don't
cast the compiler points this out. Old code did the same thing but it
did this by just doing a memcpy for the pointers - which I personally
consider even worse as it gets really easy to miss this. The cast at
least hints there is something slightly "fishy" going on.
My "gut feeling" about the correct fix is we should check if some
attributes in the array (stored to the struct here) actually need to be
modified later (which I doubt). If I was keen on betting I'd bet we
could switch the struct definition to also contain pointers to const
attributes. I am afraid this would mean quite a few more changes to the
function signatures (changing struct attribute * to const struct
attribute *) here and there - and possibly also require some changes to
drivers. Thus I didn't even look at that option in the scope of this
fix. It should probably be a separate refactoring series. But yes - this
cast should catch attention as it did.
Yours,
-- Matti Vaittinen
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
