Message-ID: <CAADnVQL95nwYGYO0jNGR2Pt3ApJa31LsD7y7sbLMtVDyvWJWZA@mail.gmail.com>
Date: Sun, 23 Nov 2025 11:20:03 -0800
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: David Laight <david.laight.linux@...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, 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.

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.

> 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