Message-ID: <20251123230318.74b94c99@pumpkin>
Date: Sun, 23 Nov 2025 23:03:18 +0000
From: David Laight <david.laight.linux@...il.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: LKML <linux-kernel@...r.kernel.org>, bpf <bpf@...r.kernel.org>, Alexei
 Starovoitov <ast@...nel.org>, Andrii Nakryiko <andrii@...nel.org>, Daniel
 Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH 06/44] bpf: Verifier, remove some unusual uses of
 min_t() and max_t()

On Sun, 23 Nov 2025 11:20:03 -0800
Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:

> On Sun, Nov 23, 2025 at 10:07 AM David Laight
> <david.laight.linux@...il.com> wrote:
> >
> > On Sun, 23 Nov 2025 08:39:51 -0800
> > Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:
> >  
> > > On Fri, Nov 21, 2025 at 2:21 PM David Laight
> > > <david.laight.linux@...il.com> wrote:  
> > > >
> > > > On Fri, 21 Nov 2025 13:40:36 -0800
> > > > Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:
> > > >  
> > > > > On Wed, Nov 19, 2025 at 2:42 PM <david.laight.linux@...il.com> wrote:  
> > > > > >
> > > > > > From: David Laight <david.laight.linux@...il.com>
> > > > > >
> > > > > > min_t() and max_t() are normally used to change the signedness
> > > > > > of a positive value to avoid a signed-v-unsigned compare warning.
> > > > > >
> > > > > > However they are used here to convert an unsigned 64bit pattern
> > > > > > to a signed to a 32/64bit signed number.
> > > > > > To avoid any confusion use plain min()/max() and explicitely cast
> > > > > > the u64 expression to the correct signed value.
> > > > > >
> > > > > > Use a simple max() for the max_pkt_offset calulation and delete the
> > > > > > comment about why the cast to u32 is safe.
> > > > > >
> > > > > > Signed-off-by: David Laight <david.laight.linux@...il.com>
> > > > > > ---
> > > > > >  kernel/bpf/verifier.c | 29 +++++++++++------------------
> > > > > >  1 file changed, 11 insertions(+), 18 deletions(-)
> > > > > >
> > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > > > > index ff40e5e65c43..22fa9769fbdb 100644
> > > > > > --- a/kernel/bpf/verifier.c
> > > > > > +++ b/kernel/bpf/verifier.c
> > > > > > @@ -2319,12 +2319,12 @@ static void __update_reg32_bounds(struct bpf_reg_state *reg)
> > > > > >         struct tnum var32_off = tnum_subreg(reg->var_off);
> > > > > >
> > > > > >         /* min signed is max(sign bit) | min(other bits) */
> > > > > > -       reg->s32_min_value = max_t(s32, reg->s32_min_value,
> > > > > > -                       var32_off.value | (var32_off.mask & S32_MIN));
> > > > > > +       reg->s32_min_value = max(reg->s32_min_value,
> > > > > > +                       (s32)(var32_off.value | (var32_off.mask & S32_MIN)));
> > > > > >         /* max signed is min(sign bit) | max(other bits) */
> > > > > > -       reg->s32_max_value = min_t(s32, reg->s32_max_value,
> > > > > > -                       var32_off.value | (var32_off.mask & S32_MAX));
> > > > > > -       reg->u32_min_value = max_t(u32, reg->u32_min_value, (u32)var32_off.value);
> > > > > > +       reg->s32_max_value = min(reg->s32_max_value,
> > > > > > +                       (s32)(var32_off.value | (var32_off.mask & S32_MAX)));  
> > > > >
> > > > > Nack.
> > > > > This is plain ugly for no good reason.
> > > > > Leave the code as-is.  
> > > >
> > > > It is really horrid before.
> > > > From what i remember var32_off.value (and .mask) are both u64.
> > > > The pattern actually patches that used a few lines down the file.
> > > >
> > > > I've been trying to build allmodconfig with the size test added to min_t()
> > > > and max_t().
> > > > The number of real (or potentially real) bugs I've found is stunning.
> > > > The only fix is to nuke min_t() and max_t() to they can't be used.  
> > >
> > > No. min_t() is going to stay. It's not broken and
> > > this crusade against it is inappropriate.  
> >
> > I bet to differ...
> >  
> > > > The basic problem is the people have used the type of the target not that
> > > > of the largest parameter.
> > > > The might be ok for ulong v uint (on 64bit), but there are plenty of places
> > > > where u16 and u8 are used - a lot are pretty much buggy.
> > > >
> > > > Perhaps the worst ones I've found are with clamp_t(),
> > > > this is from 2/44:
> > > > -               (raw_inode)->xtime = cpu_to_le32(clamp_t(int32_t, (ts).tv_sec, S32_MIN, S32_MAX));      \
> > > > +               (raw_inode)->xtime = cpu_to_le32(clamp((ts).tv_sec, S32_MIN, S32_MAX)); \
> > > > If also found clamp_t(u8, xxx, 0, 255).
> > > >
> > > > There are just so many broken examples.  
> > >
> > > clamp_t(u8, xxx, 0, 255) is not wrong. It's silly, but
> > > it's doing the right thing and one can argue and explicit
> > > clamp values serve as a documentation.  
> >
> > Not when you look at some of the code that uses it.
> > The clear intention is to saturate a large value - which isn't what it does.
> >  
> > > clamp_t(int32_t, (ts).tv_sec, S32_MIN, S32_MAX)) is indeed incorrect,
> > > but it's a bug in the implementation of __clamp_once().
> > > Fix it, instead of spamming people with "_t" removal.  
> >
> > It is too late by the time you get to clamp_once().
> > The 'type' for all the xxx_t() functions is an input cast, not the type
> > for the result.
> > clamp_t(type, v, lo, hi) has always been clamp((type)v, (type)lo, type(hi)).
> > From a code correctness point of view you pretty much never want those casts.  
> 
> Historical behavior doesn't justify a footgun.
> You definitely can make clampt_t() to behave like clamp_val() plus
> the final cast.

clamp_val() is actually the worst of the lot.

> Also note:
> git grep -w clamp_val|wc -l
> 818
> git grep -w clamp_t|wc -l
> 494
> 
> a safer macro is already used more often.

clamp_val() is worse than clamp_t() ...

Nope...
The problem is that clamp() requires all three parameters have the same type.
Coders are lazy and want to write clamp(variable, 1, 10).
This was fine if 'variable' had type 'int', but if it was 'unsigned int' you
had to write clamp(variable, 1u, 10u), worse if it is 'u8' you to either cast
both constants clamp(variable, (u8)1, (u8)10) or the variable
clamp((int)variable, 1, 10).
It the types/values aren't immediately obvious then any of those casts can
discard high bits.

A lot of the clamp_val() are actually for u8 structure members.
One thing to remember about C is it doesn't have any maths operators
for u8, the values are always promoted to 'int' before anything happens.
So if you write (foo->u8_member > 4 ? foo->u8_member : 4) the comparison
is done as an integer one.
Add some casts ((u8)f->m > (u8)x ? (u8)f->m : (u8)x) then the values are
all masked to 8 bits, promoted to 32 and then compared.
Even the ?: operator promotes its arguments and has a result type of 'int'.

Consider clamp_val(f->u8_m, LO, HI);
If HI is 255 it is fine, make HI 256 (perhaps it is sizeof() and something
got changed) and you suddenly have clamp(f->u8_m, LO, 0).
It is all just so fragile.

Maybe you are trying to find the 'chunk size' for a transfer of some kind.
If the transfer size is 'small' it might be in a 'u32', you want to limit it
to the size of the hardware's PCIe window - so do:
	copy_size = min(transfer_size, hardware_window_size);
But the hardware_window_size is a size_t (so 64bit).
The old min() would complain about the type mismatch, since the
hardware_window_size might actually be 4GB (that is true) casting to u32
is broken - you have to use the larger type.
But it might be the other way around, transfer_size is u64 and
hardware_window_size is u32, you still have to cast to u64 - but this
time it is the size of the other parameter.
The trouble is people have a habit of using the type they want for the
result, u32 in both the above and wrong twice.
But it was only the type check in min() that caused a problem.
Without the casts the compiler generates the right code, the only problem
is when a signed variable might contain a negative value that gets promoted
to a large negative value.
The current implementations of min/max/clamp only generate an error if they
can't prove that negative values won't be promoted to large unsigned values.

This is all fine provided the variable/expressions have the correct type
for the value they contain - and they usually do.
But for this bpf code the type of 'var32_off.value | (var32_off.mask & S32_MIN)'
is actually u64, it really does need an explicit cast to s32.

	David

> 
> > I've already fixed clamp() so it doesn't complain about comparing s64 against s32.
> > The next stage is to change pretty much all the xxx_t() to plain xxx().  
> 
> Nack to that. Fix the problem. Not the symptom.

Powered by blists - more mailing lists