Message-ID: <20251123180741.65cd2dd3@pumpkin>
Date: Sun, 23 Nov 2025 18:07:41 +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 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.

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

If you've got some spare time try issuing read calls with a 4GB buffer to all
the subsystems you can find - and see how many loop for ever.
(I think you can do that with readv() and a single buffer.)
The issue there is that a lot use min_t(u32, max_frag_size, xfer_size) to split
operations - and xfer_size is size_t (so I'm pretty sure there are ways to get
4GB in there).

	David

Powered by blists - more mailing lists