linux-kernel - Re: [PATCH] iio: imu: adis16550: rework clock range test

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: <aGVNhIwn7CXO_lpP@smile.fi.intel.com>
Date: Wed, 2 Jul 2025 18:17:24 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: David Lechner <dlechner@...libre.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>,
	Michael Hennerich <Michael.Hennerich@...log.com>,
	Jonathan Cameron <jic23@...nel.org>,
	Nuno Sá <nuno.sa@...log.com>,
	Robert Budai <robert.budai@...log.com>,
	Andy Shevchenko <andy@...nel.org>, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: imu: adis16550: rework clock range test

On Wed, Jul 02, 2025 at 10:07:17AM -0500, David Lechner wrote:
> On 7/2/25 9:59 AM, Andy Shevchenko wrote:
> > On Wed, Jul 02, 2025 at 05:53:57PM +0300, Andy Shevchenko wrote:
> >> On Wed, Jul 02, 2025 at 09:27:45AM -0500, David Lechner wrote:
> >>> Rework the clock rate range test to test if sync_mode_data != NULL
> >>> instead of testing if the for loop index variable. This makes it easier
> >>> for static analyzers to see that we aren't using an uninitialized
> >>> sync_mode_data [1].
> >>
> >> But at the same time it makes it not to be the usual pattern.,,
> > 
> > Reading the static analyser output I think the first hunk is only what we need,
> > but this is still false positive and it's problem of that static
> > analyser. Have you filed a bug there? (My point is that modifying the code for
> > the advantage of false positives of some static analyser is wrong road to go
> > in my opinion.)
> 
> I agree that we shouldn't fix this _only_ to make the static analyzer
> happy. But I had to think quite a bit harder to see that the existing
> code was correct compared to what I have proposed here.
> 
> But if this is a common pattern that I just haven't learned to identify
> at a glance yet and everybody else can easily see that the existing code
> is correct, then perhaps it isn't worth the change.

To me checking against index variable (when it's integer, obviously) is correct
thing to do and regular pattern. OTOH, if the "index" is a pointer and rather
we call it "iterator", the angle of view is different because in some cases
it may lead to stale or invalid value which might be mistakenly dereferenced or
speculated (see more in the discussion about list entry APIs [entry is a
keyword here] and if list_entry_is_head() is a good approach.)

-- 
With Best Regards,
Andy Shevchenko