| Message ID | ZRvjuZaQWdZw1U1I@dc78bmyyyyyyyyyyyyydt-3.rev.dnainternet.fi (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: sanity check available_scan_masks array | expand |
On Tue, 3 Oct 2023 12:49:45 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > When IIO goes through the available scan masks in order to select the > best suiting one, it will just accept the first listed subset of channels > which meets the user's requirements. If driver lists a mask which is a > subset of some of the masks previously in the array of > avaliable_scan_masks, then the latter one will never be selected. > > Add a warning if driver registers masks which can't be used due to the > available_scan_masks-array ordering. > > Suggested-by: Jonathan Cameron <jic23@kernel.org> > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> Hi Matti Thanks for doing this. A few comments inline + maybe we need to think about a unit test for the matching code. I feel we aren't pushing the corners of that in any drivers so far so it might bite us later. Still that's a job for another day. Jonathan > > --- > The change was suggested by Jonathan here: > https://lore.kernel.org/lkml/20230924170726.41443502@jic23-huawei/ > --- > drivers/iio/industrialio-core.c | 57 +++++++++++++++++++++++++++++++++ > 1 file changed, 57 insertions(+) > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > index c77745b594bd..d4f37f4eeec0 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -1896,6 +1896,53 @@ static int iio_check_extended_name(const struct iio_dev *indio_dev) > > static const struct iio_buffer_setup_ops noop_ring_setup_ops; > > +static void iio_sanity_check_avail_scan_masks(struct iio_dev *indio_dev) > +{ > + unsigned int num_masks, masklength, longs_per_mask; > + const unsigned long *av_masks; > + int i; > + > + av_masks = indio_dev->available_scan_masks; > + masklength = indio_dev->masklength; > + longs_per_mask = BITS_TO_LONGS(masklength); > + > + if (bitmap_empty(av_masks, masklength)) > + dev_warn(indio_dev->dev.parent, "empty scan mask\n"); They'd definitely notice this one as you'd never be able to enable the buffer - if someone hasn't tested that, then meh. Still this function is called sanity_check so might as well check for insanity. > + > + for (num_masks = 0; *av_masks; num_masks++) I think we can't just check *av_masks - need bitmap_empty() as first long might be 0 but could be bits set in the next one. > + av_masks += longs_per_mask; hmm. Makes me wonder if the available scan mask stuff actually works for large numbers of channels (so more than one long). I don't think we have any drivers that both have large channel counts and use available_scan_masks. The code is there to support matching in this case but probably wants a selftest at somepoint to make sure it will work if such a device comes along... > + > + if (num_masks < 2) > + return; Not sure it's worth bothering with this early exit route. The loops will be trivial anyway if num_masks == 1. > + > + av_masks = indio_dev->available_scan_masks; > + > + /* > + * Go through all the masks from first to one before the last, and see > + * that no mask found later from the available_scan_masks array is a > + * subset of mask found earlier. If this happens, then the mask found > + * later will never get used because scanning the array is stopped when > + * the first suitable mask is found. Drivers should order the array of > + * available masks in the order of preference (presumably the least > + * costy to access masks first). > + */ > + for (i = 0; i < num_masks - 1; i++) { > + const unsigned long *mask1; > + int j; > + > + mask1 = av_masks + i * longs_per_mask; > + for (j = i + 1; j < num_masks; j++) { > + const unsigned long *mask2; > + > + mask2 = av_masks + j * longs_per_mask; > + if (bitmap_subset(mask2, mask1, masklength)) > + dev_warn(indio_dev->dev.parent, > + "available_scan_mask %d subset of %d. Never used\n", > + j, i); > + } > + } > +} > + > int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) > { > struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); > @@ -1934,6 +1981,16 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) > goto error_unreg_debugfs; > } > > + /* > + * In order to not wreck utter havoc we just warn for now. Might want > + * to convert this to a failure after people have had time to act upon > + * the warning. It'd be nice to check this earlier, but we need the > + * iio_buffers_alloc_sysfs_and_mask() to have the masklength set. It's not going to break anyone if they get this wrong, they will just waste time and possibly power reading too many channels! So warn is appropriate I think. I'm not sure the comment adds much in general so I'd slim it down or drop it from v2. > + */ > + if (indio_dev->available_scan_masks) > + iio_sanity_check_avail_scan_masks(indio_dev); > + One blank line is enough ;) > + > ret = iio_device_register_sysfs(indio_dev); > if (ret) { > dev_err(indio_dev->dev.parent, > > base-commit: 5e99f692d4e32e3250ab18d511894ca797407aec
On 10/5/23 18:30, Jonathan Cameron wrote: > On Tue, 3 Oct 2023 12:49:45 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> When IIO goes through the available scan masks in order to select the >> best suiting one, it will just accept the first listed subset of channels >> which meets the user's requirements. If driver lists a mask which is a >> subset of some of the masks previously in the array of >> avaliable_scan_masks, then the latter one will never be selected. >> >> Add a warning if driver registers masks which can't be used due to the >> available_scan_masks-array ordering. >> >> Suggested-by: Jonathan Cameron <jic23@kernel.org> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > Hi Matti > > Thanks for doing this. A few comments inline + maybe we need to think > about a unit test for the matching code. I feel we aren't pushing the > corners of that in any drivers so far so it might bite us later. I am extremely conservative what comes to adding unit tests. I have seen some projects where the amount of existing unit test code made code changes very much very slow - stopping people doing any improvements. Basically, no one wanted to touch the existing code unless it was absolutely must because even a minor code change caused several tests to break. OTOH, that unit test setup did not only test that end result of a function was expected - it did also check the calls done from the function to be tested - checking for example that the certain prints appeared with certain inputs and so on. That project stopped being fun very quickly... But yes. After spending a while reading IIO code, I agree that _some_ parts of it could benefit from a few carefully designed unit tests. (And sorry, I haven't checked what tests are existing already - so may be there already is relevant tests) :) Channel data demuxing and the mask handling are indeed the first to come to my mind ;) I wouldn't dare to touch that part without some testing. > Still that's a job for another day. Hey, we need to have something for tomorrow, right? :) > >> >> --- >> The change was suggested by Jonathan here: >> https://lore.kernel.org/lkml/20230924170726.41443502@jic23-huawei/ >> --- >> drivers/iio/industrialio-core.c | 57 +++++++++++++++++++++++++++++++++ >> 1 file changed, 57 insertions(+) >> >> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c >> index c77745b594bd..d4f37f4eeec0 100644 >> --- a/drivers/iio/industrialio-core.c >> +++ b/drivers/iio/industrialio-core.c >> @@ -1896,6 +1896,53 @@ static int iio_check_extended_name(const struct iio_dev *indio_dev) >> >> static const struct iio_buffer_setup_ops noop_ring_setup_ops; >> >> +static void iio_sanity_check_avail_scan_masks(struct iio_dev *indio_dev) >> +{ >> + unsigned int num_masks, masklength, longs_per_mask; >> + const unsigned long *av_masks; >> + int i; >> + >> + av_masks = indio_dev->available_scan_masks; >> + masklength = indio_dev->masklength; >> + longs_per_mask = BITS_TO_LONGS(masklength); >> + >> + if (bitmap_empty(av_masks, masklength)) >> + dev_warn(indio_dev->dev.parent, "empty scan mask\n"); > > They'd definitely notice this one as you'd never be able to enable the > buffer - if someone hasn't tested that, then meh. Still this function > is called sanity_check so might as well check for insanity. > > >> + >> + for (num_masks = 0; *av_masks; num_masks++) > > I think we can't just check *av_masks - need bitmap_empty() as first > long might be 0 but could be bits set in the next one. Ah. In case where we have bitmap consisting of many longs. Indeed. By the way, I think I stole this check from the actual matching code - we should probably fix it as well. >> + av_masks += longs_per_mask; > hmm. Makes me wonder if the available scan mask stuff actually works > for large numbers of channels (so more than one long). After you pointed out the problem in for-condition - it probably does not work for all cases. > I don't think > we have any drivers that both have large channel counts and use > available_scan_masks. The code is there to support matching in this > case but probably wants a selftest at somepoint to make sure it will work > if such a device comes along... > > >> + >> + if (num_masks < 2) >> + return; > > Not sure it's worth bothering with this early exit route. The loops > will be trivial anyway if num_masks == 1. I probably thought about the num_masks == 0 when adding this check. Decided we might just early exit while checking. >> + >> + av_masks = indio_dev->available_scan_masks; >> + >> + /* >> + * Go through all the masks from first to one before the last, and see >> + * that no mask found later from the available_scan_masks array is a >> + * subset of mask found earlier. If this happens, then the mask found >> + * later will never get used because scanning the array is stopped when >> + * the first suitable mask is found. Drivers should order the array of >> + * available masks in the order of preference (presumably the least >> + * costy to access masks first). >> + */ >> + for (i = 0; i < num_masks - 1; i++) { >> + const unsigned long *mask1; >> + int j; >> + >> + mask1 = av_masks + i * longs_per_mask; >> + for (j = i + 1; j < num_masks; j++) { >> + const unsigned long *mask2; >> + >> + mask2 = av_masks + j * longs_per_mask; >> + if (bitmap_subset(mask2, mask1, masklength)) >> + dev_warn(indio_dev->dev.parent, >> + "available_scan_mask %d subset of %d. Never used\n", >> + j, i); >> + } >> + } >> +} >> + >> int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) >> { >> struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); >> @@ -1934,6 +1981,16 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) >> goto error_unreg_debugfs; >> } >> >> + /* >> + * In order to not wreck utter havoc we just warn for now. Might want >> + * to convert this to a failure after people have had time to act upon >> + * the warning. It'd be nice to check this earlier, but we need the >> + * iio_buffers_alloc_sysfs_and_mask() to have the masklength set. > > It's not going to break anyone if they get this wrong, they will just waste time > and possibly power reading too many channels! So warn is appropriate I think. > > I'm not sure the comment adds much in general so I'd slim it down or drop it > from v2. I'm fine with dropping the comment. My mindset is easily leaning too much on developing new drivers when I think of checks like this one. It'd be nice to get a noticeable kick immediately when developing a driver - but yes, one should be kicked just by the warning alone. > >> + */ >> + if (indio_dev->available_scan_masks) >> + iio_sanity_check_avail_scan_masks(indio_dev); >> + > One blank line is enough ;) Again... Thanks! >> + >> ret = iio_device_register_sysfs(indio_dev); >> if (ret) { >> dev_err(indio_dev->dev.parent, >> >> base-commit: 5e99f692d4e32e3250ab18d511894ca797407aec Yours, -- Matti >
Hi Again Jonathan. On 10/5/23 18:30, Jonathan Cameron wrote: > On Tue, 3 Oct 2023 12:49:45 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> When IIO goes through the available scan masks in order to select the >> best suiting one, it will just accept the first listed subset of channels >> which meets the user's requirements. If driver lists a mask which is a >> subset of some of the masks previously in the array of >> avaliable_scan_masks, then the latter one will never be selected. >> >> Add a warning if driver registers masks which can't be used due to the >> available_scan_masks-array ordering. >> >> Suggested-by: Jonathan Cameron <jic23@kernel.org> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > Hi Matti > > Thanks for doing this. A few comments inline + maybe we need to think > about a unit test for the matching code. I feel we aren't pushing the > corners of that in any drivers so far so it might bite us later. > > Still that's a job for another day. > > Jonathan > >> >> --- >> The change was suggested by Jonathan here: >> https://lore.kernel.org/lkml/20230924170726.41443502@jic23-huawei/ >> --- >> drivers/iio/industrialio-core.c | 57 +++++++++++++++++++++++++++++++++ >> 1 file changed, 57 insertions(+) >> >> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c >> index c77745b594bd..d4f37f4eeec0 100644 >> --- a/drivers/iio/industrialio-core.c >> +++ b/drivers/iio/industrialio-core.c >> @@ -1896,6 +1896,53 @@ static int iio_check_extended_name(const struct iio_dev *indio_dev) ... >> + >> + for (num_masks = 0; *av_masks; num_masks++) > > I think we can't just check *av_masks - need bitmap_empty() as first > long might be 0 but could be bits set in the next one. > >> + av_masks += longs_per_mask; I did switch this to: + for (num_masks = 0; !bitmap_empty(av_masks, masklength); num_masks++) + av_masks += longs_per_mask; but this kind of freaks me out. I think in kernel we see two ways of constructing and passing arrays to frameworks. One is creating a NULL terminated array, the other being an array which size is given. The available_scan_masks is using the first approach. The array represents bitmasks, which are thought to be of arbitrary length. The type of array items is longs though. When building an arry like this, it is easy to just do: unsigned long masks[] = { mask1_hi, mask1_lo, mask2_hi, mask2_lo, ... maskN_lo, /* sentinel */ 0 } (By the way, I've always hated that 'sentinel' comment as it - in my opinion - is not worth adding. I think the meaning of 0 should be obvious, but here I just added it to alleviate the problem). Here, if I'm not mistaken, the check I implemented would go reading out of the array bounds. Knowing how easy it would be slip the above array past my reviewing eyes - I find this scary. And ugly part of this is that we can't detect this in the iio-core side, because we have no way of knowing how big the array and sentinel are. What makes this worse is that the core does: for (i = 0; i < indio_dev->num_channels; i++) ml = max(ml, channels[i].scan_index + 1); indio_dev->masklength = ml; so, masklength may not be what was set in driver. I did quick and dirty grep for "_scan_mask\[" in iio directory and didn't spot any bigger than a few channels masks. Still, this makes me worried. BTW: I did also: Author: Matti Vaittinen <mazziesaccount@gmail.com> Date: Fri Oct 6 13:53:11 2023 +0300 iio: buffer: use bitmap_empty() to find last mask When IIO buffer code is scanning the array of available masks for matching the user's enable request to channel configuration supported by driver, the code uses a 'check for long 0' as indication of last mask. This does not work right for channel masks greater than BITS_PER_LONG. Use bitmap_empty() to find the last element in available_scan_masks array. Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> --- NOTE: This is potentially hazardous change. Please, don't pick without thorough check and understanding. diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c index 176d31d9f9d8..1e59afddcf9a 100644 --- a/drivers/iio/industrialio-buffer.c +++ b/drivers/iio/industrialio-buffer.c @@ -413,7 +413,7 @@ static const unsigned long *iio_scan_mask_match(const unsigned long *av_masks, { if (bitmap_empty(mask, masklength)) return NULL; - while (*av_masks) { + while (!bitmap_empty(av_masks, masklength)) { if (strict) { if (bitmap_equal(mask, av_masks, masklength)) return av_masks; but this is just as fragile - for obvious reasons. One way around this would be to have the first bit in the long always set for a valid mask - and take this into account when going through the masks. It's probably somewhat more confusing than current code though - but it would allow using just a single long (with all - or at least first - bits zero to indicate end of masks). Other option I see is to just error out if available_scan_masks array is given with larger than one 'long' wide masks and worry things when this breaks. Anyways, I don't like using bitmap_empty() for array of bitmaps which may be longer than BITS_PER_LONG unless we can sanity check the size of the array... How do you feel about this? Yours, -- Matti
On Fri, 6 Oct 2023 14:10:16 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > Hi Again Jonathan. > > On 10/5/23 18:30, Jonathan Cameron wrote: > > On Tue, 3 Oct 2023 12:49:45 +0300 > > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > > > >> When IIO goes through the available scan masks in order to select the > >> best suiting one, it will just accept the first listed subset of channels > >> which meets the user's requirements. If driver lists a mask which is a > >> subset of some of the masks previously in the array of > >> avaliable_scan_masks, then the latter one will never be selected. > >> > >> Add a warning if driver registers masks which can't be used due to the > >> available_scan_masks-array ordering. > >> > >> Suggested-by: Jonathan Cameron <jic23@kernel.org> > >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > > Hi Matti > > > > Thanks for doing this. A few comments inline + maybe we need to think > > about a unit test for the matching code. I feel we aren't pushing the > > corners of that in any drivers so far so it might bite us later. > > > > Still that's a job for another day. > > > > Jonathan > > > >> > >> --- > >> The change was suggested by Jonathan here: > >> https://lore.kernel.org/lkml/20230924170726.41443502@jic23-huawei/ > >> --- > >> drivers/iio/industrialio-core.c | 57 +++++++++++++++++++++++++++++++++ > >> 1 file changed, 57 insertions(+) > >> > >> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > >> index c77745b594bd..d4f37f4eeec0 100644 > >> --- a/drivers/iio/industrialio-core.c > >> +++ b/drivers/iio/industrialio-core.c > >> @@ -1896,6 +1896,53 @@ static int iio_check_extended_name(const struct iio_dev *indio_dev) > > ... > > >> + > >> + for (num_masks = 0; *av_masks; num_masks++) > > > > I think we can't just check *av_masks - need bitmap_empty() as first > > long might be 0 but could be bits set in the next one. > > > >> + av_masks += longs_per_mask; > > I did switch this to: > + for (num_masks = 0; !bitmap_empty(av_masks, masklength); > num_masks++) > + av_masks += longs_per_mask; > > but this kind of freaks me out. Good because I'm fairly sure you need to reduce the masklength int hat bitmap_empty as well. It is getting a bit complex. > > I think in kernel we see two ways of constructing and passing arrays to > frameworks. One is creating a NULL terminated array, the other being an > array which size is given. The available_scan_masks is using the first > approach. > > The array represents bitmasks, which are thought to be of arbitrary > length. The type of array items is longs though. When building an arry > like this, it is easy to just do: > > unsigned long masks[] = { > mask1_hi, > mask1_lo, > mask2_hi, > mask2_lo, > ... > maskN_lo, > /* sentinel */ > 0 > } > > (By the way, I've always hated that 'sentinel' comment as it - in my > opinion - is not worth adding. I think the meaning of 0 should be > obvious, but here I just added it to alleviate the problem). > > Here, if I'm not mistaken, the check I implemented would go reading out > of the array bounds. It does indeed. > > Knowing how easy it would be slip the above array past my reviewing eyes > - I find this scary. And ugly part of this is that we can't detect this > in the iio-core side, because we have no way of knowing how big the > array and sentinel are. What makes this worse is that the core does: > > for (i = 0; i < indio_dev->num_channels; i++) > ml = max(ml, channels[i].scan_index + 1); > indio_dev->masklength = ml; > > so, masklength may not be what was set in driver. IIRC this is there to allow for sparse scan_index values. Those are very rare, but I think there are a few drivers doing that because it allowed for slightly simpler code a long time back. May not even matter today. Key is that mask_length is big enough to allow the bits at all present scan_index values to be set. So it should always match with the drivers where this is used to make sure available_scan_masks has the right number of longs per entry, but the drivers may need to be a little clever if they are both doing large numbers of channels and sparse scan_index values. AFAIK there are none doing that. Going further I don't recall any drivers that use the available_scan_masks stuff going beyond 32 channels (so needing more than one unsigned long per element). > > I did quick and dirty grep for "_scan_mask\[" in iio directory and > didn't spot any bigger than a few channels masks. Still, this makes me > worried. > > BTW: I did also: > > Author: Matti Vaittinen <mazziesaccount@gmail.com> > Date: Fri Oct 6 13:53:11 2023 +0300 > > iio: buffer: use bitmap_empty() to find last mask > > When IIO buffer code is scanning the array of available masks for > matching the user's enable request to channel configuration > supported by > driver, the code uses a 'check for long 0' as indication of last mask. > This does not work right for channel masks greater than BITS_PER_LONG. > > Use bitmap_empty() to find the last element in available_scan_masks > array. > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > --- > NOTE: This is potentially hazardous change. Please, don't pick without > thorough check and understanding. > > diff --git a/drivers/iio/industrialio-buffer.c > b/drivers/iio/industrialio-buffer.c > index 176d31d9f9d8..1e59afddcf9a 100644 > --- a/drivers/iio/industrialio-buffer.c > +++ b/drivers/iio/industrialio-buffer.c > @@ -413,7 +413,7 @@ static const unsigned long > *iio_scan_mask_match(const unsigned long *av_masks, > { > if (bitmap_empty(mask, masklength)) > return NULL; > - while (*av_masks) { > + while (!bitmap_empty(av_masks, masklength)) { > if (strict) { > if (bitmap_equal(mask, av_masks, masklength)) > return av_masks; > > but this is just as fragile - for obvious reasons. Ah. yes, that is indeed a bug. I'm not sure your fix is particularly fragile though. This comes back to us having no drivers that actually use big bitmaps yet. Key is that we need the available_scan_masks null terminator to be the same length as any other entry - so if multiple unsigned longs needed then multiple 0's should be there. We should definitely document that and ideally add a test case. We can bulk out the dummy driver to trigger these and provide an example of how available_scan_masks should be set. > > One way around this would be to have the first bit in the long always > set for a valid mask - and take this into account when going through the > masks. It's probably somewhat more confusing than current code though - > but it would allow using just a single long (with all - or at least > first - bits zero to indicate end of masks). Too complex. > > Other option I see is to just error out if available_scan_masks array is > given with larger than one 'long' wide masks and worry things when this > breaks. That would kick the problem into the long grass. > > Anyways, I don't like using bitmap_empty() for array of bitmaps which > may be longer than BITS_PER_LONG unless we can sanity check the size of > the array... > > How do you feel about this? Agreed it's problematic as that null terminator isn't clearly forced to be big enough. Hmm. Can we cheat for any drivers that actually need large masks (when they come along) and use an appropriate 2D array. unsigned long available_masks[][2] = { {mask0_ll, mask0_hl}, {mask0_ll, mask0_hl}, {} }; iio_dev->available_scan_masks = (unsigned long *)available_masks; If we put such an example into the dummy / example driver then that might act to avoid us getting bugs in future + test the fix you have above and related. Jonathan > > Yours, > -- Matti >
On 10/10/23 13:04, Jonathan Cameron wrote: > On Fri, 6 Oct 2023 14:10:16 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> Hi Again Jonathan. >> >> On 10/5/23 18:30, Jonathan Cameron wrote: >>> On Tue, 3 Oct 2023 12:49:45 +0300 >>> Matti Vaittinen <mazziesaccount@gmail.com> wrote: >>> >>>> When IIO goes through the available scan masks in order to select the >>>> best suiting one, it will just accept the first listed subset of channels >>>> which meets the user's requirements. If driver lists a mask which is a >>>> subset of some of the masks previously in the array of >>>> avaliable_scan_masks, then the latter one will never be selected. >>>> >>>> Add a warning if driver registers masks which can't be used due to the >>>> available_scan_masks-array ordering. >>>> >>>> Suggested-by: Jonathan Cameron <jic23@kernel.org> >>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> >>> Hi Matti >>> >>> Thanks for doing this. A few comments inline + maybe we need to think >>> about a unit test for the matching code. I feel we aren't pushing the >>> corners of that in any drivers so far so it might bite us later. >>> >>> Still that's a job for another day. >>> >>> Jonathan >>> >>>> >>>> --- >>>> The change was suggested by Jonathan here: >>>> https://lore.kernel.org/lkml/20230924170726.41443502@jic23-huawei/ >>>> --- >>>> drivers/iio/industrialio-core.c | 57 +++++++++++++++++++++++++++++++++ >>>> 1 file changed, 57 insertions(+) >>>> >>>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c >>>> index c77745b594bd..d4f37f4eeec0 100644 >>>> --- a/drivers/iio/industrialio-core.c >>>> +++ b/drivers/iio/industrialio-core.c >>>> @@ -1896,6 +1896,53 @@ static int iio_check_extended_name(const struct iio_dev *indio_dev) >> >> ... >> >>>> + >>>> + for (num_masks = 0; *av_masks; num_masks++) >>> >>> I think we can't just check *av_masks - need bitmap_empty() as first >>> long might be 0 but could be bits set in the next one. >>> >>>> + av_masks += longs_per_mask; >> >> I did switch this to: >> + for (num_masks = 0; !bitmap_empty(av_masks, masklength); >> num_masks++) >> + av_masks += longs_per_mask; >> >> but this kind of freaks me out. > > Good because I'm fairly sure you need to reduce the masklength int hat > bitmap_empty as well. It is getting a bit complex. Hm. As far as I can say the masklength is constant telling how many bits there is in one mask(?) I don't think we can reduce it. Idea is just to increment the av_masks pointer until we find the zero mask indicating end of an array. This is how we count the amount of masks in the array. Caveat being that if driver has used single long '0' to indicate the end of an array, and if the masklength > 32 - we'll end up reading out of bounds. (as we agreed later in the mail. OTOH, we also agreed there does not seem to be drivers with masklength > 32 utilizing the available_scan_masks - so this is just trying to avoid problems in the future). >> >> I think in kernel we see two ways of constructing and passing arrays to >> frameworks. One is creating a NULL terminated array, the other being an >> array which size is given. The available_scan_masks is using the first >> approach. >> >> The array represents bitmasks, which are thought to be of arbitrary >> length. The type of array items is longs though. When building an arry >> like this, it is easy to just do: >> >> unsigned long masks[] = { >> mask1_hi, >> mask1_lo, >> mask2_hi, >> mask2_lo, >> ... >> maskN_lo, >> /* sentinel */ >> 0 >> } >> >> (By the way, I've always hated that 'sentinel' comment as it - in my >> opinion - is not worth adding. I think the meaning of 0 should be >> obvious, but here I just added it to alleviate the problem). >> >> Here, if I'm not mistaken, the check I implemented would go reading out >> of the array bounds. > > It does indeed. > > >> >> Knowing how easy it would be slip the above array past my reviewing eyes >> - I find this scary. And ugly part of this is that we can't detect this >> in the iio-core side, because we have no way of knowing how big the >> array and sentinel are. What makes this worse is that the core does: >> >> for (i = 0; i < indio_dev->num_channels; i++) >> ml = max(ml, channels[i].scan_index + 1); >> indio_dev->masklength = ml; >> >> so, masklength may not be what was set in driver. > > IIRC this is there to allow for sparse scan_index values. Those are > very rare, but I think there are a few drivers doing that because > it allowed for slightly simpler code a long time back. May not even > matter today. Key is that mask_length is big enough to allow the > bits at all present scan_index values to be set. > > So it should always match with the drivers where this is used to make > sure available_scan_masks has the right number of longs per entry, > but the drivers may need to be a little clever if they are both > doing large numbers of channels and sparse scan_index values. > AFAIK there are none doing that. Going further I don't recall any > drivers that use the available_scan_masks stuff going beyond 32 > channels (so needing more than one unsigned long per element). I didn't find one either. >> I did quick and dirty grep for "_scan_mask\[" in iio directory and >> didn't spot any bigger than a few channels masks. Still, this makes me >> worried. >> >> BTW: I did also: >> >> Author: Matti Vaittinen <mazziesaccount@gmail.com> >> Date: Fri Oct 6 13:53:11 2023 +0300 >> >> iio: buffer: use bitmap_empty() to find last mask >> >> When IIO buffer code is scanning the array of available masks for >> matching the user's enable request to channel configuration >> supported by >> driver, the code uses a 'check for long 0' as indication of last mask. >> This does not work right for channel masks greater than BITS_PER_LONG. >> >> Use bitmap_empty() to find the last element in available_scan_masks >> array. >> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> >> --- >> NOTE: This is potentially hazardous change. Please, don't pick without >> thorough check and understanding. >> >> diff --git a/drivers/iio/industrialio-buffer.c >> b/drivers/iio/industrialio-buffer.c >> index 176d31d9f9d8..1e59afddcf9a 100644 >> --- a/drivers/iio/industrialio-buffer.c >> +++ b/drivers/iio/industrialio-buffer.c >> @@ -413,7 +413,7 @@ static const unsigned long >> *iio_scan_mask_match(const unsigned long *av_masks, >> { >> if (bitmap_empty(mask, masklength)) >> return NULL; >> - while (*av_masks) { >> + while (!bitmap_empty(av_masks, masklength)) { >> if (strict) { >> if (bitmap_equal(mask, av_masks, masklength)) >> return av_masks; >> >> but this is just as fragile - for obvious reasons. > Ah. yes, that is indeed a bug. I'm not sure your fix is particularly > fragile though. This comes back to us having no drivers that actually use > big bitmaps yet. > > Key is that we need the available_scan_masks null terminator to be the > same length as any other entry - so if multiple unsigned longs needed > then multiple 0's should be there. Exactly my thinking, and why I think this fix would be fragile. I am not convinced people would think of adding enough of zeroes. > We should definitely document that > and ideally add a test case. We can bulk out the dummy driver > to trigger these and provide an example of how available_scan_masks > should be set. > >> >> One way around this would be to have the first bit in the long always >> set for a valid mask - and take this into account when going through the >> masks. It's probably somewhat more confusing than current code though - >> but it would allow using just a single long (with all - or at least >> first - bits zero to indicate end of masks). > > Too complex. > I think I agree. At least for as long as we don't actually have any available_scan_masks users with masklength > 32 >> >> Other option I see is to just error out if available_scan_masks array is >> given with larger than one 'long' wide masks and worry things when this >> breaks. > > That would kick the problem into the long grass. Well, not 100% sure I interpret the idiom correctly ;) In any case, I'd say this would indeed postpone dealing with the problem to the future. To the point we actually seem to have a problem. The "long grass" as if hiding the problem is something we can avoid by adding something like: if (masklength > 32 && idev->available_scan_masks) { /* * Comment mowing the long grass. */ dev_err( ...); return -EINVAL; } to the device registration. >> >> Anyways, I don't like using bitmap_empty() for array of bitmaps which >> may be longer than BITS_PER_LONG unless we can sanity check the size of >> the array... >> >> How do you feel about this? > > Agreed it's problematic as that null terminator isn't clearly forced to > be big enough. Hmm. Can we cheat for any drivers that actually need large > masks (when they come along) and use an appropriate 2D array. I think we could. Or, maybe develop some mask initialization macro magic. It'd just be cool if one did not need to do things differently for multi long masks. > unsigned long available_masks[][2] = { > {mask0_ll, mask0_hl}, > {mask0_ll, mask0_hl}, > {} > }; don't know how would it work to have unsigned long available_masks[][1] = { {mask0}, {mask1}, {} }; for regular masks with 1 long / mask as well? At first look it seems horrible to me but at least it would be a standard :) Don't know if there is a sane way to make a macro for it. > iio_dev->available_scan_masks = (unsigned long *)available_masks; > > If we put such an example into the dummy / example driver then that might > act to avoid us getting bugs in future + test the fix you have above and > related. Well, at least it shouldn't hurt to have some example - although I'm still tempted to use the "long grass" - option ;) Yours, -- Matti
On Tue, 10 Oct 2023 15:56:22 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 10/10/23 13:04, Jonathan Cameron wrote: > > On Fri, 6 Oct 2023 14:10:16 +0300 > > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > > > >> Hi Again Jonathan. > >> > >> On 10/5/23 18:30, Jonathan Cameron wrote: > >>> On Tue, 3 Oct 2023 12:49:45 +0300 > >>> Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >>> > >>>> When IIO goes through the available scan masks in order to select the > >>>> best suiting one, it will just accept the first listed subset of channels > >>>> which meets the user's requirements. If driver lists a mask which is a > >>>> subset of some of the masks previously in the array of > >>>> avaliable_scan_masks, then the latter one will never be selected. > >>>> > >>>> Add a warning if driver registers masks which can't be used due to the > >>>> available_scan_masks-array ordering. > >>>> > >>>> Suggested-by: Jonathan Cameron <jic23@kernel.org> > >>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > >>> Hi Matti > >>> > >>> Thanks for doing this. A few comments inline + maybe we need to think > >>> about a unit test for the matching code. I feel we aren't pushing the > >>> corners of that in any drivers so far so it might bite us later. > >>> > >>> Still that's a job for another day. > >>> > >>> Jonathan > >>> > >>>> > >>>> --- > >>>> The change was suggested by Jonathan here: > >>>> https://lore.kernel.org/lkml/20230924170726.41443502@jic23-huawei/ > >>>> --- > >>>> drivers/iio/industrialio-core.c | 57 +++++++++++++++++++++++++++++++++ > >>>> 1 file changed, 57 insertions(+) > >>>> > >>>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > >>>> index c77745b594bd..d4f37f4eeec0 100644 > >>>> --- a/drivers/iio/industrialio-core.c > >>>> +++ b/drivers/iio/industrialio-core.c > >>>> @@ -1896,6 +1896,53 @@ static int iio_check_extended_name(const struct iio_dev *indio_dev) > >> > >> ... > >> > >>>> + > >>>> + for (num_masks = 0; *av_masks; num_masks++) > >>> > >>> I think we can't just check *av_masks - need bitmap_empty() as first > >>> long might be 0 but could be bits set in the next one. > >>> > >>>> + av_masks += longs_per_mask; > >> > >> I did switch this to: > >> + for (num_masks = 0; !bitmap_empty(av_masks, masklength); > >> num_masks++) > >> + av_masks += longs_per_mask; > >> > >> but this kind of freaks me out. > > > > Good because I'm fairly sure you need to reduce the masklength int hat > > bitmap_empty as well. It is getting a bit complex. > > Hm. As far as I can say the masklength is constant telling how many bits > there is in one mask(?) I don't think we can reduce it. Idea is just to > increment the av_masks pointer until we find the zero mask indicating > end of an array. This is how we count the amount of masks in the array. I read it wrong :( Thought we were iterating over the longs not the sets of longs in each mask. oops. I read the same code further down correctly. I blame spending whole day waiting for a plumber (who hasn't come... sigh) > > Caveat being that if driver has used single long '0' to indicate the end > of an array, and if the masklength > 32 - we'll end up reading out of > bounds. (as we agreed later in the mail. OTOH, we also agreed there does > not seem to be drivers with masklength > 32 utilizing the > available_scan_masks - so this is just trying to avoid problems in the > future). > > >> > >> I think in kernel we see two ways of constructing and passing arrays to > >> frameworks. One is creating a NULL terminated array, the other being an > >> array which size is given. The available_scan_masks is using the first > >> approach. > >> > >> The array represents bitmasks, which are thought to be of arbitrary > >> length. The type of array items is longs though. When building an arry > >> like this, it is easy to just do: > >> > >> unsigned long masks[] = { > >> mask1_hi, > >> mask1_lo, > >> mask2_hi, > >> mask2_lo, > >> ... > >> maskN_lo, > >> /* sentinel */ > >> 0 > >> } > >> > >> (By the way, I've always hated that 'sentinel' comment as it - in my > >> opinion - is not worth adding. I think the meaning of 0 should be > >> obvious, but here I just added it to alleviate the problem). > >> > >> Here, if I'm not mistaken, the check I implemented would go reading out > >> of the array bounds. > > > > It does indeed. > > > > > >> > >> Knowing how easy it would be slip the above array past my reviewing eyes > >> - I find this scary. And ugly part of this is that we can't detect this > >> in the iio-core side, because we have no way of knowing how big the > >> array and sentinel are. What makes this worse is that the core does: > >> > >> for (i = 0; i < indio_dev->num_channels; i++) > >> ml = max(ml, channels[i].scan_index + 1); > >> indio_dev->masklength = ml; > >> > >> so, masklength may not be what was set in driver. > > > > IIRC this is there to allow for sparse scan_index values. Those are > > very rare, but I think there are a few drivers doing that because > > it allowed for slightly simpler code a long time back. May not even > > matter today. Key is that mask_length is big enough to allow the > > bits at all present scan_index values to be set. > > > > So it should always match with the drivers where this is used to make > > sure available_scan_masks has the right number of longs per entry, > > but the drivers may need to be a little clever if they are both > > doing large numbers of channels and sparse scan_index values. > > AFAIK there are none doing that. Going further I don't recall any > > drivers that use the available_scan_masks stuff going beyond 32 > > channels (so needing more than one unsigned long per element). > > I didn't find one either. > > >> I did quick and dirty grep for "_scan_mask\[" in iio directory and > >> didn't spot any bigger than a few channels masks. Still, this makes me > >> worried. > >> > >> BTW: I did also: > >> > >> Author: Matti Vaittinen <mazziesaccount@gmail.com> > >> Date: Fri Oct 6 13:53:11 2023 +0300 > >> > >> iio: buffer: use bitmap_empty() to find last mask > >> > >> When IIO buffer code is scanning the array of available masks for > >> matching the user's enable request to channel configuration > >> supported by > >> driver, the code uses a 'check for long 0' as indication of last mask. > >> This does not work right for channel masks greater than BITS_PER_LONG. > >> > >> Use bitmap_empty() to find the last element in available_scan_masks > >> array. > >> > >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > >> --- > >> NOTE: This is potentially hazardous change. Please, don't pick without > >> thorough check and understanding. > >> > >> diff --git a/drivers/iio/industrialio-buffer.c > >> b/drivers/iio/industrialio-buffer.c > >> index 176d31d9f9d8..1e59afddcf9a 100644 > >> --- a/drivers/iio/industrialio-buffer.c > >> +++ b/drivers/iio/industrialio-buffer.c > >> @@ -413,7 +413,7 @@ static const unsigned long > >> *iio_scan_mask_match(const unsigned long *av_masks, > >> { > >> if (bitmap_empty(mask, masklength)) > >> return NULL; > >> - while (*av_masks) { > >> + while (!bitmap_empty(av_masks, masklength)) { > >> if (strict) { > >> if (bitmap_equal(mask, av_masks, masklength)) > >> return av_masks; > >> > >> but this is just as fragile - for obvious reasons. > > Ah. yes, that is indeed a bug. I'm not sure your fix is particularly > > fragile though. This comes back to us having no drivers that actually use > > big bitmaps yet. > > > > Key is that we need the available_scan_masks null terminator to be the > > same length as any other entry - so if multiple unsigned longs needed > > then multiple 0's should be there. > > Exactly my thinking, and why I think this fix would be fragile. I am not > convinced people would think of adding enough of zeroes. > > > We should definitely document that > > and ideally add a test case. We can bulk out the dummy driver > > to trigger these and provide an example of how available_scan_masks > > should be set. > > > >> > >> One way around this would be to have the first bit in the long always > >> set for a valid mask - and take this into account when going through the > >> masks. It's probably somewhat more confusing than current code though - > >> but it would allow using just a single long (with all - or at least > >> first - bits zero to indicate end of masks). > > > > Too complex. > > > > I think I agree. At least for as long as we don't actually have any > available_scan_masks users with masklength > 32 > > >> > >> Other option I see is to just error out if available_scan_masks array is > >> given with larger than one 'long' wide masks and worry things when this > >> breaks. > > > > That would kick the problem into the long grass. > > Well, not 100% sure I interpret the idiom correctly ;) In any case, I'd > say this would indeed postpone dealing with the problem to the future. It does indeed mean that! Sorry bad habit to use idioms in discussions like this. > To the point we actually seem to have a problem. The "long grass" as if > hiding the problem is something we can avoid by adding something like: > > if (masklength > 32 && idev->available_scan_masks) { > /* > * Comment mowing the long grass. > */ > dev_err( ...); > return -EINVAL; > } > > to the device registration. > > >> > >> Anyways, I don't like using bitmap_empty() for array of bitmaps which > >> may be longer than BITS_PER_LONG unless we can sanity check the size of > >> the array... > >> > >> How do you feel about this? > > > > Agreed it's problematic as that null terminator isn't clearly forced to > > be big enough. Hmm. Can we cheat for any drivers that actually need large > > masks (when they come along) and use an appropriate 2D array. > > I think we could. Or, maybe develop some mask initialization macro > magic. It'd just be cool if one did not need to do things differently > for multi long masks. > > > unsigned long available_masks[][2] = { > > {mask0_ll, mask0_hl}, > > {mask0_ll, mask0_hl}, > > {} > > }; > > don't know how would it work to have > unsigned long available_masks[][1] = { > {mask0}, > {mask1}, > {} > }; > > for regular masks with 1 long / mask as well? At first look it seems > horrible to me but at least it would be a standard :) Don't know if > there is a sane way to make a macro for it. Agreed - ugly for one entry so we keep those flat which means we have to keep our eyes open for the multiple long version. I've never come up with a sane way to do a macro to set bits in multiple longs or seen one unfortunately. > > > iio_dev->available_scan_masks = (unsigned long *)available_masks; > > > > If we put such an example into the dummy / example driver then that might > > act to avoid us getting bugs in future + test the fix you have above and > > related. > > Well, at least it shouldn't hurt to have some example - although I'm > still tempted to use the "long grass" - option ;) That is probably a good idea for now. Though we are carrying other infrastructure to support this eventually and it feels weird to error out on it whilst we have code to support it (assuming that terminator is long enough). > > Yours, > -- Matti >
On 10/10/23 17:47, Jonathan Cameron wrote: > On Tue, 10 Oct 2023 15:56:22 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> On 10/10/23 13:04, Jonathan Cameron wrote: >>> On Fri, 6 Oct 2023 14:10:16 +0300 >>> Matti Vaittinen <mazziesaccount@gmail.com> wrote: >>> >>>> Hi Again Jonathan. >>>> >>>> On 10/5/23 18:30, Jonathan Cameron wrote: >>>>> On Tue, 3 Oct 2023 12:49:45 +0300 >>>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote: ... >>>> Other option I see is to just error out if available_scan_masks array is >>>> given with larger than one 'long' wide masks and worry things when this >>>> breaks. >>> >>> That would kick the problem into the long grass. >> >> Well, not 100% sure I interpret the idiom correctly ;) In any case, I'd >> say this would indeed postpone dealing with the problem to the future. > > It does indeed mean that! Sorry bad habit to use idioms in discussions like > this. > >> To the point we actually seem to have a problem. The "long grass" as if >> hiding the problem is something we can avoid by adding something like: >> >> if (masklength > 32 && idev->available_scan_masks) { >> /* >> * Comment mowing the long grass. >> */ >> dev_err( ...); >> return -EINVAL; >> } >> >> to the device registration. ... >>> iio_dev->available_scan_masks = (unsigned long *)available_masks; >>> >>> If we put such an example into the dummy / example driver then that might >>> act to avoid us getting bugs in future + test the fix you have above and >>> related. >> >> Well, at least it shouldn't hurt to have some example - although I'm >> still tempted to use the "long grass" - option ;) > > That is probably a good idea for now. Though we are carrying other infrastructure > to support this eventually and it feels weird to error out on it whilst we have > code to support it (assuming that terminator is long enough). I agree. I think I won't use the bitmap_empty() - because I feel it is unsafe. I'll leave the *av_masks check as it is implemented in iio_scan_mask_match() for now. Eg: ... const unsigned long *av_masks ... while (*av_masks) { ... av_masks += BITS_TO_LONGS(masklength); } This will fail if mask is longer than unsigned long - and if we have masks with zero bits worth a leading long. Still, this won't overflow and it also works for masks which are wider than long but do not have the leading bits zeroed. Balanced act of safety and functionality. This should allow us to safely do: if (masklength > 32 && idev->available_scan_masks) { /* * Comment mowing the long grass. */ dev_warn( ...); } without returning the error. Not perfect, but should be safe and also adds a warning if someone trusts the multi-long masks to work. Yours, -- Matti
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index c77745b594bd..d4f37f4eeec0 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -1896,6 +1896,53 @@ static int iio_check_extended_name(const struct iio_dev *indio_dev) static const struct iio_buffer_setup_ops noop_ring_setup_ops; +static void iio_sanity_check_avail_scan_masks(struct iio_dev *indio_dev) +{ + unsigned int num_masks, masklength, longs_per_mask; + const unsigned long *av_masks; + int i; + + av_masks = indio_dev->available_scan_masks; + masklength = indio_dev->masklength; + longs_per_mask = BITS_TO_LONGS(masklength); + + if (bitmap_empty(av_masks, masklength)) + dev_warn(indio_dev->dev.parent, "empty scan mask\n"); + + for (num_masks = 0; *av_masks; num_masks++) + av_masks += longs_per_mask; + + if (num_masks < 2) + return; + + av_masks = indio_dev->available_scan_masks; + + /* + * Go through all the masks from first to one before the last, and see + * that no mask found later from the available_scan_masks array is a + * subset of mask found earlier. If this happens, then the mask found + * later will never get used because scanning the array is stopped when + * the first suitable mask is found. Drivers should order the array of + * available masks in the order of preference (presumably the least + * costy to access masks first). + */ + for (i = 0; i < num_masks - 1; i++) { + const unsigned long *mask1; + int j; + + mask1 = av_masks + i * longs_per_mask; + for (j = i + 1; j < num_masks; j++) { + const unsigned long *mask2; + + mask2 = av_masks + j * longs_per_mask; + if (bitmap_subset(mask2, mask1, masklength)) + dev_warn(indio_dev->dev.parent, + "available_scan_mask %d subset of %d. Never used\n", + j, i); + } + } +} + int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) { struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); @@ -1934,6 +1981,16 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) goto error_unreg_debugfs; } + /* + * In order to not wreck utter havoc we just warn for now. Might want + * to convert this to a failure after people have had time to act upon + * the warning. It'd be nice to check this earlier, but we need the + * iio_buffers_alloc_sysfs_and_mask() to have the masklength set. + */ + if (indio_dev->available_scan_masks) + iio_sanity_check_avail_scan_masks(indio_dev); + + ret = iio_device_register_sysfs(indio_dev); if (ret) { dev_err(indio_dev->dev.parent,
When IIO goes through the available scan masks in order to select the best suiting one, it will just accept the first listed subset of channels which meets the user's requirements. If driver lists a mask which is a subset of some of the masks previously in the array of avaliable_scan_masks, then the latter one will never be selected. Add a warning if driver registers masks which can't be used due to the available_scan_masks-array ordering. Suggested-by: Jonathan Cameron <jic23@kernel.org> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> --- The change was suggested by Jonathan here: https://lore.kernel.org/lkml/20230924170726.41443502@jic23-huawei/ --- drivers/iio/industrialio-core.c | 57 +++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) base-commit: 5e99f692d4e32e3250ab18d511894ca797407aec