linux-kernel - Re: Buiild error in i915/xe

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: <878qr5ras2.fsf@intel.com>
Date: Mon, 20 Jan 2025 13:21:17 +0200
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: David Laight <david.laight.linux@...il.com>
Cc: Guenter Roeck <linux@...ck-us.net>, Linus Torvalds
 <torvalds@...ux-foundation.org>, David Laight <David.Laight@...lab.com>,
 Arnd Bergmann <arnd@...nel.org>, "linux-kernel@...r.kernel.org"
 <linux-kernel@...r.kernel.org>, Jens Axboe <axboe@...nel.dk>, Matthew
 Wilcox <willy@...radead.org>, Christoph Hellwig <hch@...radead.org>, Andrew
 Morton <akpm@...ux-foundation.org>, Andy Shevchenko
 <andriy.shevchenko@...ux.intel.com>, Dan Carpenter
 <dan.carpenter@...aro.org>, "Jason A . Donenfeld" <Jason@...c4.com>,
 "pedro.falcato@...il.com" <pedro.falcato@...il.com>, Mateusz Guzik
 <mjguzik@...il.com>, "linux-mm@...ck.org" <linux-mm@...ck.org>, Lorenzo
 Stoakes <lorenzo.stoakes@...cle.com>, intel-xe@...ts.freedesktop.org,
 intel-gfx@...ts.freedesktop.org, David Airlie <airlied@...il.com>, Simona
 Vetter <simona@...ll.ch>, Rodrigo Vivi <rodrigo.vivi@...el.com>
Subject: Re: Buiild error in i915/xe

On Mon, 20 Jan 2025, David Laight <david.laight.linux@...il.com> wrote:
> On Mon, 20 Jan 2025 12:48:11 +0200
> Jani Nikula <jani.nikula@...ux.intel.com> wrote:
>
>> On Sun, 19 Jan 2025, David Laight <david.laight.linux@...il.com> wrote:
>> > On Sat, 18 Jan 2025 14:58:48 -0800
>> > Guenter Roeck <linux@...ck-us.net> wrote:
>> >  
>> >> On 1/18/25 14:11, David Laight wrote:  
>> >> > On Sat, 18 Jan 2025 13:21:39 -0800
>> >> > Linus Torvalds <torvalds@...ux-foundation.org> wrote:
>> >> >     
>> >> >> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@...ck-us.net> wrote:    
>> >> >>>
>> >> >>> No idea why the compiler would know that the values are invalid.    
>> >> >>
>> >> >> It's not that the compiler knows tat they are invalid, but I bet what
>> >> >> happens is in scale() (and possibly other places that do similar
>> >> >> checks), which does this:
>> >> >>
>> >> >>          WARN_ON(source_min > source_max);
>> >> >>          ...
>> >> >>          source_val = clamp(source_val, source_min, source_max);
>> >> >>
>> >> >> and the compiler notices that the ordering comparison in the first
>> >> >> WARN_ON() is the same as the one in clamp(), so it basically converts
>> >> >> the logic to
>> >> >>
>> >> >>          if (source_min > source_max) {
>> >> >>                  WARN(..);
>> >> >>                  /* Do the clamp() knowing that source_min > source_max */
>> >> >>                  source_val = clamp(source_val, source_min, source_max);
>> >> >>          } else {
>> >> >>                  /* Do the clamp knowing that source_min <= source_max */
>> >> >>                  source_val = clamp(source_val, source_min, source_max);
>> >> >>          }
>> >> >>
>> >> >> (obviously I dropped the other WARN_ON in the conversion, it wasn't
>> >> >> relevant for this case).
>> >> >>
>> >> >> And now that first clamp() case is done with source_min > source_max,
>> >> >> and it triggers that build error because that's invalid.
>> >> >>
>> >> >> So the condition is not statically true in the *source* code, but in
>> >> >> the "I have moved code around to combine tests" case it now *is*
>> >> >> statically true as far as the compiler is concerned.    
>> >> > 
>> >> > Well spotted :-)
>> >> > 
>> >> > One option would be to move the WARN_ON() below the clamp() and
>> >> > add an OPTIMISER_HIDE_VAR(source_max) between them.
>> >> > 
>> >> > Or do something more sensible than the WARN().
>> >> > Perhaps return target_min on any such errors?
>> >> >     
>> >> 
>> >> This helps:
>> >> 
>> >> -       WARN_ON(source_min > source_max);
>> >> -       WARN_ON(target_min > target_max);
>> >> -
>> >>          /* defensive */
>> >>          source_val = clamp(source_val, source_min, source_max);
>> >> 
>> >> +       WARN_ON(source_min > source_max);
>> >> +       WARN_ON(target_min > target_max);  
>> >
>> > That is a 'quick fix' ...
>> >
>> > Much better would be to replace the WARN() with (say):
>> > 	if (target_min >= target_max)
>> > 		return target_min;
>> > 	if (source_min >= source_max)
>> > 		return target_min + (target_max - target_min)/2;
>> > So that the return values are actually in range (in as much as one is defined).
>> > Note that the >= cpmparisons also remove a divide by zero.  
>> 
>> I want the loud and early warnings for clear bugs instead of
>> "gracefully" silencing the errors only to be found through debugging
>> user reports.
>
> A user isn't going to notice a WARN() - not until you tell them to look for it.
> In any case even if you output a message you really want to return a 'sane'
> value, who knows what effect a very out of range value is going to have.

The point is, we'll catch the WARN in CI before it goes out to users.

BR,
Jani.

>
> 	David
>
>

-- 
Jani Nikula, Intel