[PATCH v2 0/2] Sanity-check available_scan_masks array

[PATCH v2 0/2] Sanity-check available_scan_masks array

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2312 bytes --]

Sanity-check available_scan_masks array

The available_scan_masks is an array of bitmasks representing the
channels which can be simultaneously(*) scanned by a driver from the
device. Following special characteristics apply:

- When IIO is scanning through the array it will use the first mask
  which can be used to scan all enabled channels. This means drivers
  should order the array in the order of the preference. This does also
  mean that a mask which is a subset of a mask located earler in array
  will never be used because the earlier one will be picked by the core.
- Masks wider than size of long are supported only to some extent. The
  code scanning through the array will interpret the first mask with
  first long zeroed as end-of-array terminator. Changing this behaviour
  would make mask-arrays for multi-long masks to be terminated by more
  than one zero long. Failure to do so would result kernel to read
  beyond the array generating a potentially hazardous bug.

Add a sanity-check to IIO-device registration emitting a warning if
available_scan_mask array is misordered or if mask width is larger than
a long while available_scan_mask-array is populated. Currently there
should be no in-tree drivers with available_scan_mask populated and mask
wider than a long.

Revision history:
v1 => v2:
	- Add patch 2/2 documenting why iio_scan_mask_match() checks only
	  a long worth of bits while searching for the end of the
	  available_scan_mask-array.
	- Styling of patch 1/2 as per comments from Jonathan
	v1 and related discussion here:
	https://lore.kernel.org/lkml/ZRvjuZaQWdZw1U1I@dc78bmyyyyyyyyyyyyydt-3.rev.dnainternet.fi/

Matti Vaittinen (2):
  iio: sanity check available_scan_masks array
  iio: buffer: document known issue

 drivers/iio/industrialio-buffer.c | 16 ++++++++
 drivers/iio/industrialio-core.c   | 63 +++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+)

-- 
2.41.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 1/2] iio: sanity check available_scan_masks array

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 4353 bytes --]

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>

---
Revision History:
v1 => v2:
 - warn if masklength of available_scan_masks is wider than a long
 - drop unnecessary comment and extra blank line

NOTE: the v2 was compile-tested only.

The change was suggested by Jonathan here:
https://lore.kernel.org/lkml/20230924170726.41443502@jic23-huawei/
---
 drivers/iio/industrialio-core.c | 63 +++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index c77745b594bd..34e1f8d0071c 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1896,6 +1896,66 @@ 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);
+
+	/*
+	 * The code determining how many available_scan_masks is in the array
+	 * will be assuming the end of masks when first long with all bits
+	 * zeroed is encountered. This is incorrect for masks where mask
+	 * consists of more than one long, and where some of the available masks
+	 * has long worth of bits zeroed (but has subsequent bit(s) set). This
+	 * is a safety measure against bug where array of masks is terminated by
+	 * a single zero while mask width is greater than width of a long.
+	 */
+	if (longs_per_mask > 1)
+		dev_warn(indio_dev->dev.parent,
+			 "multi long available scan masks not fully supported\n");
+
+	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 +1994,9 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
 		goto error_unreg_debugfs;
 	}
 
+	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,
-- 
2.41.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 2/2] iio: buffer: document known issue

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1947 bytes --]

Add documentation explaining why the code which scans all available scan
masks is checking only a single long worth of bits even though the code
was intended to be supporting masks wider than single long.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/iio/industrialio-buffer.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 176d31d9f9d8..09c41e9ccf87 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -413,6 +413,22 @@ static const unsigned long *iio_scan_mask_match(const unsigned long *av_masks,
 {
 	if (bitmap_empty(mask, masklength))
 		return NULL;
+	/*
+	 * The condition here do not handle multi-long masks correctly.
+	 * It only checks the first long to be zero, and will use such mask
+	 * as a terminator even if there was bits set after the first long.
+	 *
+	 * Correct check would require using:
+	 * while (!bitmap_empty(av_masks, masklength))
+	 * instead. This is potentially hazardous because the
+	 * avaliable_scan_masks is a zero terminated array of longs - and
+	 * using the proper bitmap_empty() check for multi-long wide masks
+	 * would require the array to be terminated with multiple zero longs -
+	 * which is not such an usual pattern.
+	 *
+	 * As writing of this no multi-long wide masks were found in-tree, so
+	 * the simple while (*av_masks) check is working.
+	 */
 	while (*av_masks) {
 		if (strict) {
 			if (bitmap_equal(mask, av_masks, masklength))
-- 
2.41.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/2] Sanity-check available_scan_masks array

[Jonathan Cameron]

On Mon, 16 Oct 2023 14:04:11 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Sanity-check available_scan_masks array
> 
> The available_scan_masks is an array of bitmasks representing the
> channels which can be simultaneously(*) scanned by a driver from the
> device. Following special characteristics apply:
> 
> - When IIO is scanning through the array it will use the first mask
>   which can be used to scan all enabled channels. This means drivers
>   should order the array in the order of the preference. This does also
>   mean that a mask which is a subset of a mask located earler in array
>   will never be used because the earlier one will be picked by the core.
> - Masks wider than size of long are supported only to some extent. The
>   code scanning through the array will interpret the first mask with
>   first long zeroed as end-of-array terminator. Changing this behaviour
>   would make mask-arrays for multi-long masks to be terminated by more
>   than one zero long. Failure to do so would result kernel to read
>   beyond the array generating a potentially hazardous bug.
> 
> Add a sanity-check to IIO-device registration emitting a warning if
> available_scan_mask array is misordered or if mask width is larger than
> a long while available_scan_mask-array is populated. Currently there
> should be no in-tree drivers with available_scan_mask populated and mask
> wider than a long.
> 
> Revision history:
> v1 => v2:
> 	- Add patch 2/2 documenting why iio_scan_mask_match() checks only
> 	  a long worth of bits while searching for the end of the
> 	  available_scan_mask-array.
> 	- Styling of patch 1/2 as per comments from Jonathan
> 	v1 and related discussion here:
> 	https://lore.kernel.org/lkml/ZRvjuZaQWdZw1U1I@dc78bmyyyyyyyyyyyyydt-3.rev.dnainternet.fi/
> 
> Matti Vaittinen (2):
>   iio: sanity check available_scan_masks array
>   iio: buffer: document known issue
> 
>  drivers/iio/industrialio-buffer.c | 16 ++++++++
>  drivers/iio/industrialio-core.c   | 63 +++++++++++++++++++++++++++++++
>  2 files changed, 79 insertions(+)
> 

Hi Matti,

Just a quick note to say this looks fine to me, but I don't want to queue it up
just yet given proximity to merge window etc.  I'll aim to pick it up early
in next cycle. Give me a poke if I still haven't by rc3 or so.

Thanks,

Jonathan

Re: [PATCH v2 0/2] Sanity-check available_scan_masks array

[Matti Vaittinen]

On 10/21/23 18:55, Jonathan Cameron wrote:
> On Mon, 16 Oct 2023 14:04:11 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Sanity-check available_scan_masks array
>>
>> The available_scan_masks is an array of bitmasks representing the
>> channels which can be simultaneously(*) scanned by a driver from the
>> device. Following special characteristics apply:
>>
>> - When IIO is scanning through the array it will use the first mask
>>    which can be used to scan all enabled channels. This means drivers
>>    should order the array in the order of the preference. This does also
>>    mean that a mask which is a subset of a mask located earler in array
>>    will never be used because the earlier one will be picked by the core.
>> - Masks wider than size of long are supported only to some extent. The
>>    code scanning through the array will interpret the first mask with
>>    first long zeroed as end-of-array terminator. Changing this behaviour
>>    would make mask-arrays for multi-long masks to be terminated by more
>>    than one zero long. Failure to do so would result kernel to read
>>    beyond the array generating a potentially hazardous bug.
>>
>> Add a sanity-check to IIO-device registration emitting a warning if
>> available_scan_mask array is misordered or if mask width is larger than
>> a long while available_scan_mask-array is populated. Currently there
>> should be no in-tree drivers with available_scan_mask populated and mask
>> wider than a long.
>>
>> Revision history:
>> v1 => v2:
>> 	- Add patch 2/2 documenting why iio_scan_mask_match() checks only
>> 	  a long worth of bits while searching for the end of the
>> 	  available_scan_mask-array.
>> 	- Styling of patch 1/2 as per comments from Jonathan
>> 	v1 and related discussion here:
>> 	https://lore.kernel.org/lkml/ZRvjuZaQWdZw1U1I@dc78bmyyyyyyyyyyyyydt-3.rev.dnainternet.fi/
>>
>> Matti Vaittinen (2):
>>    iio: sanity check available_scan_masks array
>>    iio: buffer: document known issue
>>
>>   drivers/iio/industrialio-buffer.c | 16 ++++++++
>>   drivers/iio/industrialio-core.c   | 63 +++++++++++++++++++++++++++++++
>>   2 files changed, 79 insertions(+)
>>
> 
> Hi Matti,
> 
> Just a quick note to say this looks fine to me, but I don't want to queue it up
> just yet given proximity to merge window etc.

Makes perfect sense to me.

>  I'll aim to pick it up early
> in next cycle. Give me a poke if I still haven't by rc3 or so.

Ouch... My memory gets worse year by year - well, I'll try to remember :)

Yours,
	-- Matti

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

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

Re: [PATCH v2 0/2] Sanity-check available_scan_masks array

[Jonathan Cameron]

On Sat, 21 Oct 2023 19:03:15 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 10/21/23 18:55, Jonathan Cameron wrote:
> > On Mon, 16 Oct 2023 14:04:11 +0300
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> Sanity-check available_scan_masks array
> >>
> >> The available_scan_masks is an array of bitmasks representing the
> >> channels which can be simultaneously(*) scanned by a driver from the
> >> device. Following special characteristics apply:
> >>
> >> - When IIO is scanning through the array it will use the first mask
> >>    which can be used to scan all enabled channels. This means drivers
> >>    should order the array in the order of the preference. This does also
> >>    mean that a mask which is a subset of a mask located earler in array
> >>    will never be used because the earlier one will be picked by the core.
> >> - Masks wider than size of long are supported only to some extent. The
> >>    code scanning through the array will interpret the first mask with
> >>    first long zeroed as end-of-array terminator. Changing this behaviour
> >>    would make mask-arrays for multi-long masks to be terminated by more
> >>    than one zero long. Failure to do so would result kernel to read
> >>    beyond the array generating a potentially hazardous bug.
> >>
> >> Add a sanity-check to IIO-device registration emitting a warning if
> >> available_scan_mask array is misordered or if mask width is larger than
> >> a long while available_scan_mask-array is populated. Currently there
> >> should be no in-tree drivers with available_scan_mask populated and mask
> >> wider than a long.
> >>
> >> Revision history:
> >> v1 => v2:
> >> 	- Add patch 2/2 documenting why iio_scan_mask_match() checks only
> >> 	  a long worth of bits while searching for the end of the
> >> 	  available_scan_mask-array.
> >> 	- Styling of patch 1/2 as per comments from Jonathan
> >> 	v1 and related discussion here:
> >> 	https://lore.kernel.org/lkml/ZRvjuZaQWdZw1U1I@dc78bmyyyyyyyyyyyyydt-3.rev.dnainternet.fi/
> >>
> >> Matti Vaittinen (2):
> >>    iio: sanity check available_scan_masks array
> >>    iio: buffer: document known issue
> >>
> >>   drivers/iio/industrialio-buffer.c | 16 ++++++++
> >>   drivers/iio/industrialio-core.c   | 63 +++++++++++++++++++++++++++++++
> >>   2 files changed, 79 insertions(+)
> >>  
> > 
> > Hi Matti,
> > 
> > Just a quick note to say this looks fine to me, but I don't want to queue it up
> > just yet given proximity to merge window etc.  
> 
> Makes perfect sense to me.
> 
> >  I'll aim to pick it up early
> > in next cycle. Give me a poke if I still haven't by rc3 or so.  
> 
> Ouch... My memory gets worse year by year - well, I'll try to remember :)

I've started queuing stuff up for rebasing post merge window, so I've
added this as well. For now will only be exposed as the testing branch
that 0-day pokes at.

Thanks,

Jonathan

> 
> Yours,
> 	-- Matti
> 

Re: [PATCH v2 0/2] Sanity-check available_scan_masks array

[Matti Vaittinen]

On 10/28/23 19:32, Jonathan Cameron wrote:
> On Sat, 21 Oct 2023 19:03:15 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> On 10/21/23 18:55, Jonathan Cameron wrote:
>>> On Mon, 16 Oct 2023 14:04:11 +0300
>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>>    
>>>> Sanity-check available_scan_masks array
>>>>
>>>> The available_scan_masks is an array of bitmasks representing the
>>>> channels which can be simultaneously(*) scanned by a driver from the
>>>> device. Following special characteristics apply:
>>>>
>>>> - When IIO is scanning through the array it will use the first mask
>>>>     which can be used to scan all enabled channels. This means drivers
>>>>     should order the array in the order of the preference. This does also
>>>>     mean that a mask which is a subset of a mask located earler in array
>>>>     will never be used because the earlier one will be picked by the core.
>>>> - Masks wider than size of long are supported only to some extent. The
>>>>     code scanning through the array will interpret the first mask with
>>>>     first long zeroed as end-of-array terminator. Changing this behaviour
>>>>     would make mask-arrays for multi-long masks to be terminated by more
>>>>     than one zero long. Failure to do so would result kernel to read
>>>>     beyond the array generating a potentially hazardous bug.
>>>>
>>>> Add a sanity-check to IIO-device registration emitting a warning if
>>>> available_scan_mask array is misordered or if mask width is larger than
>>>> a long while available_scan_mask-array is populated. Currently there
>>>> should be no in-tree drivers with available_scan_mask populated and mask
>>>> wider than a long.
>>>>
>>>> Revision history:
>>>> v1 => v2:
>>>> 	- Add patch 2/2 documenting why iio_scan_mask_match() checks only
>>>> 	  a long worth of bits while searching for the end of the
>>>> 	  available_scan_mask-array.
>>>> 	- Styling of patch 1/2 as per comments from Jonathan
>>>> 	v1 and related discussion here:
>>>> 	https://lore.kernel.org/lkml/ZRvjuZaQWdZw1U1I@dc78bmyyyyyyyyyyyyydt-3.rev.dnainternet.fi/
>>>>
>>>> Matti Vaittinen (2):
>>>>     iio: sanity check available_scan_masks array
>>>>     iio: buffer: document known issue

...

> 
> I've started queuing stuff up for rebasing post merge window, so I've
> added this as well. For now will only be exposed as the testing branch
> that 0-day pokes at.

Thanks Jonathan!


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

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