Re: [PATCH 06/44] bpf: Verifier, remove some unusual uses of min_t() and max_t()

[David Laight <david.laight.linux@gmail.com>]

On Fri, 21 Nov 2025 13:40:36 -0800
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Wed, Nov 19, 2025 at 2:42 PM <david.laight.linux@gmail.com> wrote:
> >
> > From: David Laight <david.laight.linux@gmail.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@gmail.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.

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.

	David




> 
> pw-bot: cr