Re: [PATCH] iio: sanity check available_scan_masks array

[Matti Vaittinen]

On 10/10/23 17:47, Jonathan Cameron wrote:
> On Tue, 10 Oct 2023 15:56:22 +0300
> Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
>
> > On 10/10/23 13:04, Jonathan Cameron wrote:
> > > On Fri, 6 Oct 2023 14:10:16 +0300
> > > Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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@xxxxxxxxx> 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

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~