linux-kernel - Re: [PATCH v3 3/6] iio: try searching for exact scan

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230924171007.2012a879@jic23-huawei>
Date:   Sun, 24 Sep 2023 17:10:07 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Matti Vaittinen <mazziesaccount@...il.com>,
        Lars-Peter Clausen <lars@...afoo.de>
Cc:     Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Angel Iglesias <ang.iglesiasg@...il.com>,
        Andreas Klinger <ak@...klinger.de>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Benjamin Bara <bbara93@...il.com>, linux-iio@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/6] iio: try searching for exact scan_mask

On Sun, 24 Sep 2023 17:07:26 +0100
Jonathan Cameron <jic23@...nel.org> wrote:

> On Fri, 22 Sep 2023 14:17:49 +0300
> Matti Vaittinen <mazziesaccount@...il.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. This works great for most of the
> > drivers as they can sort the list of channels in the order where
> > the 'least costy' channel selections come first.
> > 
> > It may be that in some cases the ordering of the list of available scan
> > masks is not thoroughly considered. We can't really try outsmarting the
> > drivers by selecting the smallest supported subset - as this might not
> > be the 'least costy one' - but we can at least try searching through the
> > list to see if we have an exactly matching mask. It should be sane
> > assumption that if the device can support reading only the exact
> > channels user is interested in, then this should be also the least costy
> > selection - and if it is not and optimization is important, then the
> > driver could consider omitting setting the 'available_scan_mask' and
> > doing demuxing - or just omitting the 'costy exact match' and providing
> > only the more efficient broader selection of channels.
> > 
> > Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>  
> 
> Whilst I fully agree with the reasoning behind this, I'd rather we
> did an audit of drivers to find any that have a non logical order
> (one came up today in review) and fix them up.
> 
> A quick and dirty grep didn't find it to be a common problem, at least
> partly as most users of this feature only provide one valid mask.
> The few complex corners I found appear to be fine with the expected
> shortest sequences first.
> 
> Defending against driver bugs is losing game if it makes the core
> code more complex to follow by changing stuff in non debug paths.
> One option might be to add a trivial check at iio_device_register()
> that we don't have scan modes that are subsets of modes earlier in the list.
> These lists are fairly short so should be cheap to run.
> 
> That would incorporate ensuring exact matches come earlier by default.

BTW I'd have sent these as a separate series as there is potential that
this will distract from or slow down the driver + not all the CC list
will care about this core cleanup.

Jonathan

> 
> Jonathan
> 
> 
> > ---
> >  drivers/iio/industrialio-buffer.c | 25 +++++++++++++++++++------
> >  1 file changed, 19 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index 176d31d9f9d8..e97396623373 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -411,19 +411,32 @@ static const unsigned long *iio_scan_mask_match(const unsigned long *av_masks,
> >  						const unsigned long *mask,
> >  						bool strict)
> >  {
> > +	const unsigned long *first_subset = NULL;
> > +
> >  	if (bitmap_empty(mask, masklength))
> >  		return NULL;
> > -	while (*av_masks) {
> > -		if (strict) {
> > +
> > +	if (strict) {
> > +		while (*av_masks) {
> >  			if (bitmap_equal(mask, av_masks, masklength))
> >  				return av_masks;
> > -		} else {
> > -			if (bitmap_subset(mask, av_masks, masklength))
> > -				return av_masks;
> > +
> > +			av_masks += BITS_TO_LONGS(masklength);
> >  		}
> > +
> > +		return NULL;
> > +	}
> > +	while (*av_masks) {
> > +		if (bitmap_equal(mask, av_masks, masklength))
> > +			return av_masks;
> > +
> > +		if (!first_subset && bitmap_subset(mask, av_masks, masklength))
> > +			first_subset = av_masks;
> > +
> >  		av_masks += BITS_TO_LONGS(masklength);
> >  	}
> > -	return NULL;
> > +
> > +	return first_subset;
> >  }
> >  
> >  static bool iio_validate_scan_mask(struct iio_dev *indio_dev,  
>