From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f47.google.com (mail-wm1-f47.google.com [209.85.128.47]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 8D09E1ACEDA for ; Sun, 23 Nov 2025 18:07:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.47 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763921267; cv=none; b=h5Mmnpq+X7wUPb+eR5SMwoRIUA3pjlI1yDzUM9YlanRUPmOU0A1+4xpaApcP6aNS/IjLzE0TCBrIeO/Hq01vNLdlhFvl8RNbOCRUEJOXnayoZ/xxL5wwIIwfHJqZ1sYJmLyCMHhYJMSeMlF7LtMuBsjbybc41c5SMPcDHtyTI6s= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763921267; c=relaxed/simple; bh=PdY3TC8QxHLo+F+nDS9HZRCJ2UTQFbY+XHEF+vEH64A=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=ij9PfZ+zmFq1O6jXbaHddeMATovwNpFLo2veE+9jMXdx1mjJQI7a77+xUvRED/iy8NOnss5/JtCnvjBTLuYctYhiBQv/yClH+sDKQZTY5zxyRHiC1MzJquw6m9ZmInU+fFuRMxb/ovtHRBSV8g0rt+wPPOr1yPgWu7r7hXqDqnY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=cyeCKT6r; arc=none smtp.client-ip=209.85.128.47 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="cyeCKT6r" Received: by mail-wm1-f47.google.com with SMTP id 5b1f17b1804b1-4779adb38d3so24900155e9.2 for ; Sun, 23 Nov 2025 10:07:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1763921264; x=1764526064; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=+NhQxnxNvy8W4MIozVgyD6WAqeyUL2JQLmu0aTdNPuk=; b=cyeCKT6r0h+TFuHyEDyHltGZXbQJNcXQ2ntHOxfzV7NBcXJdhxFZWQg7kmbyOiT7Uf qfk2tR8n99vXmzeZkpHpa0M2AuEISd/+Cjb/yh0KyCOdB6+YFc4lAAAo+UyCgdjw0JS+ oMah6fSy+DBfV5ZAGxRRUvYwO6we8MTO1ZO5PLq9dfNV59uOldqLBAu8Dn844wQ6X0Na sfEEfNNib6kwxd7po8EGPJ5Y9wpeqBDp7WnWB8GkeE0OHPKsesdcQ+XO/C0y1Q8OO/Yz jTD11zWN8DIasii5qwoE5y8OdlXTpbGJygGMoTqUoP0IcOH3/tdxdj+JGJyCRKjC/Yzi Zr9g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1763921264; x=1764526064; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=+NhQxnxNvy8W4MIozVgyD6WAqeyUL2JQLmu0aTdNPuk=; b=haUONH4n6etnnWArEkr9Of5/LYHKN1jlU9CMs78zFP/3tG6P5BRu6YzHu2TbiwYmvf niUSisz+qWgTnWWP81GGVmPb8Rzv86dIO9+3DGvlRmiDz/s1Y7QyjEfKGc2dQPBNrQDI yyJLPMDgeyTgCtYbTKMFuEUwktELSqBJhexZR6BFyBR2iNe1MQR8HJcZt94ElhkfHCuS mukvzHBFqiiCjvPEvhG2Q6TPQUl+F0ShidyfcWzRIQQLErWmhdglXazedyTA5wi8EcaN J2D6de5VUq7OGIY3AiIE/IW1yBpXe9UW865neRc8GD9w8iHSCme0VWwobwfn2tPGCQGx ih8Q== X-Gm-Message-State: AOJu0YwRZvmvFPltQ//H/+YMo871rhRxJ3Wv7YpRrpAoSFwCIdHzNnov ncofm2mwqVxnrTe7E3U+Hw0Ifqt4ULlKutrELX2NHLl+ZzmH0esfZw/L X-Gm-Gg: ASbGncuRjfAXeFRjvu6JztH6Y+ULeVgNDNxjpIhrblCZoUF+QqQh8JKKOh5vMuoQksD 92Fv9jx/3aAt1uK3pfWPuBPdORiRCPDGPvKkUkYKJ9bfKR0SL+DfxsG6vEs26KVju9csKdne13I zRvbywnGBheiqsRiMR9YYXvExbqi7NKr9JIPaJ6M8yCujygGUWS+aV6mP1O9B2HtuVZJcDEOzFf 0ZUloonct075KHQB2Q8p0KEn7Nygr6mugXtb3MUkGFLB0fqmLkOi7FIvcWNB6KxwGF00oolLIC8 dyX40fCWHpzJGLYYiBcO6CdeSFffhMWT2Vt3OoGktULB6zU5H1UI0L1PfMPOyO5R74HfJpn3zun /FCyjYeSgXU7ZmrzpekhFo2c/N8IpQkJOeitAgNZZRT9IN++sv9rPMxHPvFYTfP3XHYozbUuvJl 6qn1ujszo7wlRep6zechpxOtEuMB5k77GIsnHs0yhK2BU15lYWXRj6 X-Google-Smtp-Source: AGHT+IG/cYV7vixIRxYiH4QYGFWv0RK1n9tpfEodwp2gcBftSOhsCx+PTMxXKlZgmsLIzHaPUIXBAQ== X-Received: by 2002:a05:600c:1c28:b0:46e:53cb:9e7f with SMTP id 5b1f17b1804b1-477c01c11afmr106667295e9.18.1763921263655; Sun, 23 Nov 2025 10:07:43 -0800 (PST) Received: from pumpkin (82-69-66-36.dsl.in-addr.zen.co.uk. [82.69.66.36]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-477a9dcbca9sm132822365e9.6.2025.11.23.10.07.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 23 Nov 2025 10:07:43 -0800 (PST) Date: Sun, 23 Nov 2025 18:07:41 +0000 From: David Laight To: Alexei Starovoitov Cc: LKML , bpf , Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann Subject: Re: [PATCH 06/44] bpf: Verifier, remove some unusual uses of min_t() and max_t() Message-ID: <20251123180741.65cd2dd3@pumpkin> In-Reply-To: References: <20251119224140.8616-1-david.laight.linux@gmail.com> <20251119224140.8616-7-david.laight.linux@gmail.com> <20251121222151.7056c4fa@pumpkin> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; arm-unknown-linux-gnueabihf) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Sun, 23 Nov 2025 08:39:51 -0800 Alexei Starovoitov wrote: > On Fri, Nov 21, 2025 at 2:21=E2=80=AFPM David Laight > wrote: > > > > On Fri, 21 Nov 2025 13:40:36 -0800 > > Alexei Starovoitov wrote: > > =20 > > > On Wed, Nov 19, 2025 at 2:42=E2=80=AFPM wrote: =20 > > > > > > > > From: David Laight > > > > > > > > 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 > > > > --- > > > > 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 bp= f_reg_state *reg) > > > > struct tnum var32_off =3D tnum_subreg(reg->var_off); > > > > > > > > /* min signed is max(sign bit) | min(other bits) */ > > > > - reg->s32_min_value =3D max_t(s32, reg->s32_min_value, > > > > - var32_off.value | (var32_off.mask & S32_MIN= )); > > > > + reg->s32_min_value =3D max(reg->s32_min_value, > > > > + (s32)(var32_off.value | (var32_off.mask & S= 32_MIN))); > > > > /* max signed is min(sign bit) | max(other bits) */ > > > > - reg->s32_max_value =3D min_t(s32, reg->s32_max_value, > > > > - var32_off.value | (var32_off.mask & S32_MAX= )); > > > > - reg->u32_min_value =3D max_t(u32, reg->u32_min_value, (u32)= var32_off.value); > > > > + reg->s32_max_value =3D min(reg->s32_max_value, > > > > + (s32)(var32_off.value | (var32_off.mask & S= 32_MAX))); =20 > > > > > > Nack. > > > This is plain ugly for no good reason. > > > Leave the code as-is. =20 > > > > 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. =20 >=20 > 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 th= at > > of the largest parameter. > > The might be ok for ulong v uint (on 64bit), but there are plenty of pl= aces > > 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 =3D cpu_to_le32(clamp_t(int32_t, (ts= ).tv_sec, S32_MIN, S32_MAX)); \ > > + (raw_inode)->xtime =3D cpu_to_le32(clamp((ts).tv_sec, S= 32_MIN, S32_MAX)); \ > > If also found clamp_t(u8, xxx, 0, 255). > > > > There are just so many broken examples. =20 >=20 > 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)). =46rom a code correctness point of view you pretty much never want those cast= s. I've already fixed clamp() so it doesn't complain about comparing s64 again= st 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 a= ll 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 s= plit operations - and xfer_size is size_t (so I'm pretty sure there are ways to = get 4GB in there). David