Linux XFS filesystem development
 help / color / mirror / Atom feed
* [PATCH 0/1] Fix random.c UB on int overflow
@ 2026-10-06 13:51 Lukas Herbolt
  2026-10-06 13:51 ` [PATCH 1/1] lib/random.c fix undefined behavior on signed integer overflow Lukas Herbolt
  0 siblings, 1 reply; 5+ messages in thread
From: Lukas Herbolt @ 2026-10-06 13:51 UTC (permalink / raw)
  To: zlang; +Cc: fstests, linux-xfs, linux-ext4, Lukas Herbolt

While building xfstests with default rpm CFLAGS/LDFLAGS, I noticed
the generic/007 fails with:

    --- /opt/xfstests-dev/tests/generic/007.out	2026-10-06 14:08:02.556220624 +0200
    +++ /opt/xfstests-dev/results//generic/007.out.bad	2026-10-06 14:40:01.667593670 +0200
    @@ -14,9 +14,9 @@
     .........................................................................
     .........................................................................
     ....................................................
    -creates:  18736 OK,  18802 EEXIST  ( 37538 total, 50% EEXIST)
    -removes:  18675 OK,  19927 ENOENT  ( 38602 total, 51% ENOENT)
    -lookups:  12000 OK,  11860 ENOENT  ( 23860 total, 49% ENOENT)
    -total  :  49411 OK,  50589 w/error (100000 total, 50% w/error)
    +creates:  18745 OK,  19086 EEXIST  ( 37831 total, 50% EEXIST)
    +removes:  18685 OK,  19864 ENOENT  ( 38549 total, 51% ENOENT)
    +lookups:  12065 OK,  11555 ENOENT  ( 23620 total, 48% ENOENT)
    +total  :  49495 OK,  50505 w/error (100000 total, 50% w/error)
     
    -cleanup:     61 removes
    +cleanup:     60 removes

This happens because because the lib/random.c has optimized out the negative
branch check as overflow on signed int is undefined behavior.

    if (it <= 0)	
        it = (it + it) ^ MASK;
    else
        it = it + it;

Changing the if-else to single if based on the original `it` value prevents the
compiler from optimizing out the code.

    int apply_mask = (it <= 0);
    it = (unsigned)it << 1;
    if (apply_mask)
        it ^= MASK;

Steps to reproduce: 
    dnf install redhat-rpm-config -y
    ./configure CFLAGS="$(rpm --eval '%{optflags}')" \
        LDFLAGS="$(rpm --eval '%{__global_ldflags}')"
    ./check generic/007

The random.c is used in:

    ext4/052
    generic/007
    generic/010
    generic/011
    generic/014
    generic/094
    generic/225
    generic/311
    generic/676
    xfs/008
    xfs/188

and I did not noticed any regression with this change and the only one
failing with the original implementation is the generic/007. 

Lukas Herbolt (1):
  lib/random.c fix undefined behavior on signed integer overflow.

 lib/random.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

-- 
2.55.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/1] lib/random.c fix undefined behavior on signed integer overflow.
  2026-10-06 13:51 [PATCH 0/1] Fix random.c UB on int overflow Lukas Herbolt
@ 2026-10-06 13:51 ` Lukas Herbolt
  2026-10-07 17:00   ` Darrick J. Wong
  0 siblings, 1 reply; 5+ messages in thread
From: Lukas Herbolt @ 2026-10-06 13:51 UTC (permalink / raw)
  To: zlang; +Cc: fstests, linux-xfs, linux-ext4, Lukas Herbolt

The random.c now expects that signed integer to overflow and
triggers the  ^MASK branch. But signed int overflow is undefined
behavior and GCC can optimize this branch out with certain
CFLAGS/LDFLAGS.

Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
---
 lib/random.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/lib/random.c b/lib/random.c
index d5c81be817c8..f767ed1d1337 100644
--- a/lib/random.c
+++ b/lib/random.c
@@ -186,14 +186,15 @@ _random (int32_t is [2])
 int32_t 
 _irandm (int32_t is [2])
 {
-	int32_t it, leh, nit;
-
+	int32_t it, leh, nit, apply_mask;
 	it = is [0];
 	leh = is [1];
-	if (it <= 0)	
-		it = (it + it) ^ MASK;
-	else
-		it = it + it;
+
+/* 	evaluate on original value — no UB here */
+	apply_mask = (it <= 0);
+/*	double via unsigned shift — well-defined */
+	it = (unsigned)it << 1;
+	if (apply_mask) it ^= MASK;
 	nit = it - 1;
 /*	to ensure all-ones pattern omitted    */
 	leh = leh * mt[nit & 127] + nit;
-- 
2.55.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] lib/random.c fix undefined behavior on signed integer overflow.
  2026-10-06 13:51 ` [PATCH 1/1] lib/random.c fix undefined behavior on signed integer overflow Lukas Herbolt
@ 2026-10-07 17:00   ` Darrick J. Wong
  2026-10-08  8:23     ` Lukas Herbolt
  0 siblings, 1 reply; 5+ messages in thread
From: Darrick J. Wong @ 2026-10-07 17:00 UTC (permalink / raw)
  To: Lukas Herbolt; +Cc: zlang, fstests, linux-xfs, linux-ext4

On Tue, Oct 06, 2026 at 03:51:52PM +0200, Lukas Herbolt wrote:
> The random.c now expects that signed integer to overflow and
> triggers the  ^MASK branch. But signed int overflow is undefined
> behavior and GCC can optimize this branch out with certain
> CFLAGS/LDFLAGS.
> 
> Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
> ---
>  lib/random.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/random.c b/lib/random.c
> index d5c81be817c8..f767ed1d1337 100644
> --- a/lib/random.c
> +++ b/lib/random.c
> @@ -186,14 +186,15 @@ _random (int32_t is [2])
>  int32_t 
>  _irandm (int32_t is [2])
>  {
> -	int32_t it, leh, nit;
> -
> +	int32_t it, leh, nit, apply_mask;
>  	it = is [0];
>  	leh = is [1];
> -	if (it <= 0)	
> -		it = (it + it) ^ MASK;
> -	else
> -		it = it + it;
> +
> +/* 	evaluate on original value — no UB here */
> +	apply_mask = (it <= 0);
> +/*	double via unsigned shift — well-defined */
> +	it = (unsigned)it << 1;
> +	if (apply_mask) it ^= MASK;

Doesn't _random suffer the same flaw?

Oh.  It's dead code, maybe it should go away?

Also, what about converting all the variables to unsigned?

--D

>  	nit = it - 1;
>  /*	to ensure all-ones pattern omitted    */
>  	leh = leh * mt[nit & 127] + nit;
> -- 
> 2.55.0
> 
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] lib/random.c fix undefined behavior on signed integer overflow.
  2026-10-07 17:00   ` Darrick J. Wong
@ 2026-10-08  8:23     ` Lukas Herbolt
  2026-10-08 15:17       ` Darrick J. Wong
  0 siblings, 1 reply; 5+ messages in thread
From: Lukas Herbolt @ 2026-10-08  8:23 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: zlang, fstests, linux-xfs, linux-ext4

On 2026-10-07 19:00, Darrick J. Wong wrote:
> On Tue, Oct 06, 2026 at 03:51:52PM +0200, Lukas Herbolt wrote:
>> The random.c now expects that signed integer to overflow and
>> triggers the  ^MASK branch. But signed int overflow is undefined
>> behavior and GCC can optimize this branch out with certain
>> CFLAGS/LDFLAGS.
>> 
>> Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
>> ---
>>  lib/random.c | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>> 
>> diff --git a/lib/random.c b/lib/random.c
>> index d5c81be817c8..f767ed1d1337 100644
>> --- a/lib/random.c
>> +++ b/lib/random.c
>> @@ -186,14 +186,15 @@ _random (int32_t is [2])
>>  int32_t
>>  _irandm (int32_t is [2])
>>  {
>> -	int32_t it, leh, nit;
>> -
>> +	int32_t it, leh, nit, apply_mask;
>>  	it = is [0];
>>  	leh = is [1];
>> -	if (it <= 0)
>> -		it = (it + it) ^ MASK;
>> -	else
>> -		it = it + it;
>> +
>> +/* 	evaluate on original value — no UB here */
>> +	apply_mask = (it <= 0);
>> +/*	double via unsigned shift — well-defined */
>> +	it = (unsigned)it << 1;
>> +	if (apply_mask) it ^= MASK;
> 
> Doesn't _random suffer the same flaw?
> 
> Oh.  It's dead code, maybe it should go away?
Good point.
> 
> Also, what about converting all the variables to unsigned?

It would break two signed checks: `it <= 0` (LFSR feedback -
only fires at zero instead of when MSB set) and `leh < 0`
(output folding — never fires). The `it` register degenerates
to 33 repeating values after 64 calls. Reducing the pool of
random numbers.

-- 
-lhe

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] lib/random.c fix undefined behavior on signed integer overflow.
  2026-10-08  8:23     ` Lukas Herbolt
@ 2026-10-08 15:17       ` Darrick J. Wong
  0 siblings, 0 replies; 5+ messages in thread
From: Darrick J. Wong @ 2026-10-08 15:17 UTC (permalink / raw)
  To: Lukas Herbolt; +Cc: zlang, fstests, linux-xfs, linux-ext4

On Thu, Oct 08, 2026 at 10:23:59AM +0200, Lukas Herbolt wrote:
> On 2026-10-07 19:00, Darrick J. Wong wrote:
> > On Tue, Oct 06, 2026 at 03:51:52PM +0200, Lukas Herbolt wrote:
> > > The random.c now expects that signed integer to overflow and
> > > triggers the  ^MASK branch. But signed int overflow is undefined
> > > behavior and GCC can optimize this branch out with certain
> > > CFLAGS/LDFLAGS.
> > > 
> > > Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
> > > ---
> > >  lib/random.c | 13 +++++++------
> > >  1 file changed, 7 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/lib/random.c b/lib/random.c
> > > index d5c81be817c8..f767ed1d1337 100644
> > > --- a/lib/random.c
> > > +++ b/lib/random.c
> > > @@ -186,14 +186,15 @@ _random (int32_t is [2])
> > >  int32_t
> > >  _irandm (int32_t is [2])
> > >  {
> > > -	int32_t it, leh, nit;
> > > -
> > > +	int32_t it, leh, nit, apply_mask;
> > >  	it = is [0];
> > >  	leh = is [1];
> > > -	if (it <= 0)
> > > -		it = (it + it) ^ MASK;
> > > -	else
> > > -		it = it + it;
> > > +
> > > +/* 	evaluate on original value — no UB here */
> > > +	apply_mask = (it <= 0);
> > > +/*	double via unsigned shift — well-defined */
> > > +	it = (unsigned)it << 1;
> > > +	if (apply_mask) it ^= MASK;
> > 
> > Doesn't _random suffer the same flaw?
> > 
> > Oh.  It's dead code, maybe it should go away?
> Good point.
> > 
> > Also, what about converting all the variables to unsigned?
> 
> It would break two signed checks: `it <= 0` (LFSR feedback -
> only fires at zero instead of when MSB set) and `leh < 0`
> (output folding — never fires). The `it` register degenerates
> to 33 repeating values after 64 calls. Reducing the pool of
> random numbers.

Ah.  Well in that case I'm satisfied, so
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-10-08 15:17 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-06 13:51 [PATCH 0/1] Fix random.c UB on int overflow Lukas Herbolt
2026-10-06 13:51 ` [PATCH 1/1] lib/random.c fix undefined behavior on signed integer overflow Lukas Herbolt
2026-10-07 17:00   ` Darrick J. Wong
2026-10-08  8:23     ` Lukas Herbolt
2026-10-08 15:17       ` Darrick J. Wong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox