Messages in this thread | | | Date | Mon, 3 Oct 2022 12:02:56 +0300 | Subject | Re: [RFT PATCH v3 10/10] iio: Don't silently expect attribute types | From | Matti Vaittinen <> |
| |
On 10/3/22 11:58, Matti Vaittinen wrote: > 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. >> >> ... >> >>> + 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. >
Actually, now that you pointed it out - do you think this would warrant a FIXME comment?
> Yours, > -- Matti Vaittinen >
-- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
| |