Re: [PATCH] iio: sanity check available_scan_masks array

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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! ~~




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux