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

[Alexei Starovoitov]

On Fri, Nov 21, 2025 at 2:21 PM David Laight
<david.laight.linux@xxxxxxxxx> wrote:
>
> On Fri, 21 Nov 2025 13:40:36 -0800
> Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote:
>
> > On Wed, Nov 19, 2025 at 2:42 PM <david.laight.linux@xxxxxxxxx> wrote:
> > >
> > > From: David Laight <david.laight.linux@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> > > ---
> > >  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.

> 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.
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.