Change a lot of min_t() that might mask high bits
Subsystems (66)
cover Change a lot of min_t() that might mask high bits
From: David Laight <[email protected]>
It in not uncommon for code to use min_t(uint, a, b) when one of a or b is 64bit and can have a value that is larger than 2^32;
This is particularly prevelant with:
uint_var = min_t(uint, uint_var, uint64_expression);
Casts to u8 and u16 are very likely to discard significant bits.
These can be detected at compile time by changing min_t(), for example:
#define CHECK_SIZE(fn, type, val) \ BUILD_BUG_ON_MSG(sizeof (val) > sizeof (type) && \ !statically_true(((val) >> 8 * (sizeof (type) - 1)) < 256), \ fn "() significant bits of '" #val "' may be discarded")
#define min_t(type, x, y) ({ \
CHECK_SIZE("min_t", type, x); \
CHECK_SIZE("min_t", type, y); \
__cmp_once(min, type, x, y); })(and similar changes to max_t() and clamp_t().)
This shows up some real bugs, some unlikely bugs and some false positives. In most cases both arguments are unsigned type (just different ones) and min_t() can just be replaced by min().
The patches are all independant and are most of the ones needed to get the x86-64 kernel I build to compile.
I've not tried building an allyesconfig or allmodconfig kernel. I've also not included the patch to minmax.h itself.
I've tried to put the patches that actually fix things first. The last one is 0009.
I gave up on fixing sched/fair.c - it is too broken for a single patch! The patch for net/ipv4/tcp.c is also absent because do_tcp_getsockopt() needs multiple/larger changes to make it 'sane'.
I've had to trim the 124 maintainers/lists that get_maintainer.pl finds from 124 to under 100 to be able to send the cover letter.
The individual patches only go to the addresses found for the associated files. That reduces the number of emails to a less unsane number.
David Laight (44):
x86/asm/bitops: Change the return type of variable__ffs() to unsigned
int
ext4: Fix saturation of 64bit inode times for old filesystems
perf: Fix branch stack callchain limit
io_uring/net: Change some dubious min_t()
ipc/msg: Fix saturation of percpu counts in msgctl_info()
bpf: Verifier, remove some unusual uses of min_t() and max_t() net/core/flow_dissector: Fix cap of __skb_flow_dissect() return value.
net: ethtool: Use min3() instead of nested min_t(u16,...)
ipv6: __ip6_append_data() don't abuse max_t() casts x86/crypto: ctr_crypt() use min() instead of min_t() arch/x96/kvm: use min() instead of min_t()
block: use min() instead of min_t()
drivers/acpi: use min() instead of min_t() drivers/char/hw_random: use min3() instead of nested min_t() drivers/char/tpm: use min() instead of min_t() drivers/crypto/ccp: use min() instead of min_t() drivers/cxl: use min() instead of min_t()
drivers/gpio: use min() instead of min_t() drivers/gpu/drm/amd: use min() instead of min_t() drivers/i2c/busses: use min() instead of min_t() drivers/net/ethernet/realtek: use min() instead of min_t() drivers/nvme: use min() instead of min_t()
arch/x86/mm: use min() instead of min_t()
drivers/nvmem: use min() instead of min_t()
drivers/pci: use min() instead of min_t()
drivers/scsi: use min() instead of min_t()
drivers/tty/vt: use umin() instead of min_t(u16, ...) for row/col
limits
drivers/usb/storage: use min() instead of min_t() drivers/xen: use min() instead of min_t()
fs: use min() or umin() instead of min_t()
block: bvec.h: use min() instead of min_t()
nodemask: use min() instead of min_t()
ipc: use min() instead of min_t()
bpf: use min() instead of min_t()
bpf_trace: use min() instead of min_t()
lib/bucket_locks: use min() instead of min_t() lib/crypto/mpi: use min() instead of min_t() lib/dynamic_queue_limits: use max() instead of max_t()
mm: use min() instead of min_t()
net: Don't pass bitfields to max_t()
net/core: Change loop conditions so min() can be used
net: use min() instead of min_t()
net/netlink: Use umin() to avoid min_t(int, ...) discarding high bits net/mptcp: Change some dubious min_t(int, ...) to min()
arch/x86/crypto/aesni-intel_glue.c | 3 +- arch/x86/include/asm/bitops.h | 18 +++++------- arch/x86/kvm/emulate.c | 3 +- arch/x86/kvm/lapic.c | 2 +- arch/x86/kvm/mmu/mmu.c | 2 +- arch/x86/mm/pat/set_memory.c | 12 ++++---- block/blk-iocost.c | 6 ++-- block/blk-settings.c | 2 +- block/partitions/efi.c | 3 +- drivers/acpi/property.c | 2 +- drivers/char/hw_random/core.c | 2 +- drivers/char/tpm/tpm1-cmd.c | 2 +- drivers/char/tpm/tpm_tis_core.c | 4 +-- drivers/crypto/ccp/ccp-dev.c | 2 +- drivers/cxl/core/mbox.c | 2 +- drivers/gpio/gpiolib-acpi-core.c | 2 +- .../gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c | 4 +-- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- drivers/i2c/busses/i2c-designware-master.c | 2 +- drivers/net/ethernet/realtek/r8169_main.c | 3 +- drivers/nvme/host/pci.c | 3 +- drivers/nvme/host/zns.c | 3 +- drivers/nvmem/core.c | 2 +- drivers/pci/probe.c | 3 +- drivers/scsi/hosts.c | 2 +- drivers/tty/vt/selection.c | 9 +++--- drivers/usb/storage/protocol.c | 3 +- drivers/xen/grant-table.c | 2 +- fs/buffer.c | 2 +- fs/exec.c | 2 +- fs/ext4/ext4.h | 2 +- fs/ext4/mballoc.c | 3 +- fs/ext4/resize.c | 2 +- fs/ext4/super.c | 2 +- fs/fat/dir.c | 4 +-- fs/fat/file.c | 3 +- fs/fuse/dev.c | 2 +- fs/fuse/file.c | 8 ++--- fs/splice.c | 2 +- include/linux/bvec.h | 3 +- include/linux/nodemask.h | 9 +++--- include/linux/perf_event.h | 2 +- include/net/tcp_ecn.h | 5 ++-- io_uring/net.c | 6 ++-- ipc/mqueue.c | 4 +-- ipc/msg.c | 6 ++-- kernel/bpf/core.c | 4 +-- kernel/bpf/log.c | 2 +- kernel/bpf/verifier.c | 29 +++++++------------ kernel/trace/bpf_trace.c | 2 +- lib/bucket_locks.c | 2 +- lib/crypto/mpi/mpicoder.c | 2 +- lib/dynamic_queue_limits.c | 2 +- mm/gup.c | 4 +-- mm/memblock.c | 2 +- mm/memory.c | 2 +- mm/percpu.c | 2 +- mm/truncate.c | 3 +- mm/vmscan.c | 2 +- net/core/datagram.c | 6 ++-- net/core/flow_dissector.c | 7 ++--- net/core/net-sysfs.c | 3 +- net/core/skmsg.c | 4 +-- net/ethtool/cmis_cdb.c | 7 ++--- net/ipv4/fib_trie.c | 2 +- net/ipv4/tcp_input.c | 4 +-- net/ipv4/tcp_output.c | 5 ++-- net/ipv4/tcp_timer.c | 4 +-- net/ipv6/addrconf.c | 8 ++--- net/ipv6/ip6_output.c | 7 +++-- net/ipv6/ndisc.c | 5 ++-- net/mptcp/protocol.c | 8 ++--- net/netlink/genetlink.c | 9 +++--- net/packet/af_packet.c | 2 +- net/unix/af_unix.c | 4 +-- 76 files changed, 141 insertions(+), 176 deletions(-)
Signature · 1 line
2.39.5
PATCH 1/44 x86/asm/bitops: Change the return type of variable__ffs() to unsigned int
From: David Laight <[email protected]>
The return type of variable__ffs() is currently 'unsigned long'. This makes the x86 __ffs() be 'unsigned long' whereas the generic version is 'unsigned int'.
Similarly change variable_ffz() and ffz().
This may save some REX prefix on 64bit.
Detected by some extra checks added to min_t() to detect possible truncation of large values.
--- arch/x86/include/asm/bitops.h | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)
arch/x86/include/asm/bitops.h +7 -11
@@ -240,7 +240,7 @@ arch_test_bit_acquire(unsigned long nr, const volatile unsigned long *addr)variable_test_bit (nr, addr); } -static __always_inline __attribute_const__ unsigned long variable__ffs(unsigned long word) +static __always_inline __attribute_const__ unsigned int variable__ffs(unsigned long word) { asm("tzcnt %1,%0" : "=r" (word)
@@ -254,12 +254,10 @@ static __always_inline __attribute_const__ unsigned long variable__ffs(unsigned* * Undefined if no bit exists, so code should check against 0 first. */ -#define __ffs (word) \ - (__builtin_constant_p(word) ? \ - (unsigned long)__builtin_ctzl(word) : \ - variable__ffs(word)) +#define __ffs (word) \ + (__builtin_constant_p(word) ? __builtin_ctzl(word) : variable__ffs(word)) -static __always_inline __attribute_const__ unsigned long variable_ffz(unsigned long word) +static __always_inline __attribute_const__ unsigned int variable_ffz(unsigned long word) {return variable__ffs (~word); }
@@ -270,10 +268,8 @@ static __always_inline __attribute_const__ unsigned long variable_ffz(unsigned l* * Undefined if no zero exists, so code should check against ~0UL first. */ -#define ffz (word) \ - (__builtin_constant_p(word) ? \ - (unsigned long)__builtin_ctzl(~word) : \ - variable_ffz(word)) +#define ffz (word) \ + (__builtin_constant_p(word) ? __builtin_ctzl(~word) : variable_ffz(word))/* * __fls: find last set bit in word
@@ -281,7 +277,7 @@ static __always_inline __attribute_const__ unsigned long variable_ffz(unsigned l* * Undefined if no set bit exists, so code should check against 0 first. */ -static __always_inline __attribute_const__ unsigned long __fls(unsigned long word) +static __always_inline __attribute_const__ unsigned int __fls(unsigned long word) {if (__builtin_constant_p (word))return BITS_PER_LONG -1 -__builtin_clzl (word);
Signature · 1 line
2.39.5
Yury Norov
maintainerRe: [PATCH 01/44] x86/asm/bitops: Change the return type of variable__ffs() to unsigned int
Quoted 23 lines · On Wed, Nov 19, 2025 at 10:40:57PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
The return type of
variable__ffs()is currently 'unsigned long'. This makes the x86__ffs()be 'unsigned long' whereas the generic version is 'unsigned int'.Similarly change
variable_ffz()andffz().This may save some REX prefix on 64bit.
Detected by some extra checks added to
min_t()to detect possible truncation of large values.Signed-off-by David Laight <[email protected]>--- arch/x86/include/asm/bitops.h | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)arch/x86/include/asm/bitops.h +0
-0
arch/x86/include/asm/bitops.h arch_test_bit_acquire(unsigned long nr, const volatile unsigned long *addr) · line 243
Quoted 5 lines
arch/x86/include/asm/bitops.h +1
-1@@ -240,7 +240,7 @@ arch_test_bit_acquire(unsigned long nr, const volatile unsigned long *addr)variable_test_bit (nr, addr); } -static __always_inline __attribute_const__ unsigned long variable__ffs(unsigned long word) +static __always_inline __attribute_const__ unsigned int variable__ffs(unsigned long word)
There's a mismatch with the generic ffs() in asm-generic/bitops/ffs.h.
The generic_ffs() returns int. There is another variable__ffs() defined in arch/risk, also returning int.
So I believe, the correct fix would be to switch x86 to int as well. And anyways, I believe this deserves a separate series.
Thanks,
Yury
Re: [PATCH 01/44] x86/asm/bitops: Change the return type of variable__ffs() to unsigned int
Quoted 32 lines · On Thu, 20 Nov 2025 10:54:01 -0500 Yury Norov <[email protected]> wrote:
Quoted 23 lines · On Wed, Nov 19, 2025 at 10:40:57PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
The return type of
variable__ffs()is currently 'unsigned long'. This makes the x86__ffs()be 'unsigned long' whereas the generic version is 'unsigned int'.Similarly change
variable_ffz()andffz().This may save some REX prefix on 64bit.
Detected by some extra checks added to
min_t()to detect possible truncation of large values.Signed-off-by David Laight <[email protected]>--- arch/x86/include/asm/bitops.h | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)arch/x86/include/asm/bitops.h +0
-0arch/x86/include/asm/bitops.h arch_test_bit_acquire(unsigned long nr, const volatile unsigned long *addr) · line 243
Quoted 5 lines
arch/x86/include/asm/bitops.h +1
-1@@ -240,7 +240,7 @@ arch_test_bit_acquire(unsigned long nr, const volatile unsigned long *addr)variable_test_bit (nr, addr); } -static __always_inline __attribute_const__ unsigned long variable__ffs(unsigned long word) +static __always_inline __attribute_const__ unsigned int variable__ffs(unsigned long word)There's a mismatch with the generic
ffs()inasm-generic/bitops/ffs.h.
Quoted 5 lines
The
generic_ffs()returns int. There is anothervariable__ffs()defined inarch/risk, also returning int.So I believe, the correct fix would be to switch x86 to int as well. And anyways, I believe this deserves a separate series.
It is a single patch, do you want me to resend the patch on its own? (With a different commit message)
David
Quoted 4 lines
Thanks,
Yury
Yury Norov
maintainerRe: [PATCH 01/44] x86/asm/bitops: Change the return type of variable__ffs() to unsigned int
Quoted 35 lines · David Laight
Quoted 32 lines · On Thu, 20 Nov 2025 10:54:01 -0500 Yury Norov <[email protected]> wrote:
Quoted 23 lines · On Wed, Nov 19, 2025 at 10:40:57PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
The return type of
variable__ffs()is currently 'unsigned long'. This makes the x86__ffs()be 'unsigned long' whereas the generic version is 'unsigned int'.Similarly change
variable_ffz()andffz().This may save some REX prefix on 64bit.
Detected by some extra checks added to
min_t()to detect possible truncation of large values.Signed-off-by David Laight <[email protected]>--- arch/x86/include/asm/bitops.h | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)arch/x86/include/asm/bitops.h +0
-0arch/x86/include/asm/bitops.h arch_test_bit_acquire(unsigned long nr, const volatile unsigned long *addr) · line 243
Quoted 5 lines
arch/x86/include/asm/bitops.h +1
-1@@ -240,7 +240,7 @@ arch_test_bit_acquire(unsigned long nr, const volatile unsigned long *addr)variable_test_bit (nr, addr); } -static __always_inline __attribute_const__ unsigned long variable__ffs(unsigned long word) +static __always_inline __attribute_const__ unsigned int variable__ffs(unsigned long word)There's a mismatch with the generic
ffs()inasm-generic/bitops/ffs.h.
Quoted 8 lines
Quoted 5 lines
The
generic_ffs()returns int. There is anothervariable__ffs()defined inarch/risk, also returning int.So I believe, the correct fix would be to switch x86 to int as well. And anyways, I believe this deserves a separate series.
It is a single patch, do you want me to resend the patch on its own? (With a different commit message)
The patch looks wrong to me. All the flavors of ffs(), ffz() and others should have the same signature.
Re: [PATCH 01/44] x86/asm/bitops: Change the return type of variable__ffs() to unsigned int
Quoted 39 lines · On Thu, 20 Nov 2025 13:33:11 -0500 Yury Norov <[email protected]> wrote:
Quoted 38 lines · David Laight
Quoted 35 lines · On Thu, 20 Nov 2025 10:54:01 -0500 Yury Norov <[email protected]> wrote:
Quoted 23 lines · On Wed, Nov 19, 2025 at 10:40:57PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
The return type of
variable__ffs()is currently 'unsigned long'. This makes the x86__ffs()be 'unsigned long' whereas the generic version is 'unsigned int'.Similarly change
variable_ffz()andffz().This may save some REX prefix on 64bit.
Detected by some extra checks added to
min_t()to detect possible truncation of large values.Signed-off-by David Laight <[email protected]>--- arch/x86/include/asm/bitops.h | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)arch/x86/include/asm/bitops.h +0
-0arch/x86/include/asm/bitops.h arch_test_bit_acquire(unsigned long nr, const volatile unsigned long *addr) · line 243
Quoted 5 lines
arch/x86/include/asm/bitops.h +1
-1@@ -240,7 +240,7 @@ arch_test_bit_acquire(unsigned long nr, const volatile unsigned long *addr)variable_test_bit (nr, addr); } -static __always_inline __attribute_const__ unsigned long variable__ffs(unsigned long word) +static __always_inline __attribute_const__ unsigned int variable__ffs(unsigned long word)There's a mismatch with the generic
ffs()inasm-generic/bitops/ffs.h.The
generic_ffs()returns int. There is anothervariable__ffs()defined inarch/risk, also returning int.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
So I believe, the correct fix would be to switch x86 to int as well. And anyways, I believe this deserves a separate series.
It is a single patch, do you want me to resend the patch on its own? (With a different commit message)
The patch looks wrong to me. All the flavors of
ffs(),ffz()and others should have the same signature.
I only changed 'long' to 'int', all the generic (and builtin) ones are 'int'. So I'm not sure which difference you are talking about.
IIRC the __attribute_const__ just lets the compiler do CSE.
David
Yury Norov
maintainerRe: [PATCH 01/44] x86/asm/bitops: Change the return type of variable__ffs() to unsigned int
Quoted 48 lines · David Laight
Quoted 45 lines · On Thu, 20 Nov 2025 13:33:11 -0500 Yury Norov <[email protected]> wrote:
Quoted 43 lines · David Laight
Quoted 32 lines · On Thu, 20 Nov 2025 10:54:01 -0500 Yury Norov <[email protected]> wrote:
Quoted 23 lines · On Wed, Nov 19, 2025 at 10:40:57PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
The return type of
variable__ffs()is currently 'unsigned long'. This makes the x86__ffs()be 'unsigned long' whereas the generic version is 'unsigned int'.Similarly change
variable_ffz()andffz().This may save some REX prefix on 64bit.
Detected by some extra checks added to
min_t()to detect possible truncation of large values.Signed-off-by David Laight <[email protected]>--- arch/x86/include/asm/bitops.h | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)arch/x86/include/asm/bitops.h +0
-0arch/x86/include/asm/bitops.h arch_test_bit_acquire(unsigned long nr, const volatile unsigned long *addr) · line 243
Quoted 5 lines
arch/x86/include/asm/bitops.h +1
-1@@ -240,7 +240,7 @@ arch_test_bit_acquire(unsigned long nr, const volatile unsigned long *addr)variable_test_bit (nr, addr); } -static __always_inline __attribute_const__ unsigned long variable__ffs(unsigned long word) +static __always_inline __attribute_const__ unsigned int variable__ffs(unsigned long word)There's a mismatch with the generic
ffs()inasm-generic/bitops/ffs.h.Quoted 5 lines
The
generic_ffs()returns int. There is anothervariable__ffs()defined inarch/risk, also returning int.So I believe, the correct fix would be to switch x86 to int as well. And anyways, I believe this deserves a separate series.
It is a single patch, do you want me to resend the patch on its own? (With a different commit message)
Quoted 6 lines
Quoted 2 lines
The patch looks wrong to me. All the flavors of
ffs(),ffz()and others should have the same signature.I only changed 'long' to 'int', all the generic (and builtin) ones are 'int'. So I'm not sure which difference you are talking about.
IIRC the__attribute_const__just lets the compiler do CSE.
All the versions of ffs(), ffz() and others should in the better world have the same signatures. This is unfortunately not what we have now.
for __ffs() we've got signatures:
RISCV:
static __always_inline __attribute_const__ unsigned long variable__ffs(unsigned long word)
x86:
static __always_inline __attribute_const__ unsigned long variable__ffs(unsigned long word)
Generic:
static __always_inline __attribute_const__ unsigned int generic___ffs(unsigned long word)
You're changing x86 implementation to match the generic one, but don't change RISCV, and this is wrong.
Can you make them all matching, and check the same for ffz() please? This would require a small separate series, I guess.
Moving forward, it would be nice to make all that helpers looking more unified across different arches and flavors. Although, out of scope of your series.
Thanks,
Yury
Re: [PATCH 01/44] x86/asm/bitops: Change the return type of variable__ffs() to unsigned int
Quoted 64 lines · On Mon, 24 Nov 2025 09:58:51 -0500 Yury Norov <[email protected]> wrote:
Quoted 48 lines · David Laight
Quoted 45 lines · On Thu, 20 Nov 2025 13:33:11 -0500 Yury Norov <[email protected]> wrote:
Quoted 43 lines · David Laight
Quoted 32 lines · On Thu, 20 Nov 2025 10:54:01 -0500 Yury Norov <[email protected]> wrote:
Quoted 23 lines · On Wed, Nov 19, 2025 at 10:40:57PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
The return type of
variable__ffs()is currently 'unsigned long'. This makes the x86__ffs()be 'unsigned long' whereas the generic version is 'unsigned int'.Similarly change
variable_ffz()andffz().This may save some REX prefix on 64bit.
Detected by some extra checks added to
min_t()to detect possible truncation of large values.Signed-off-by David Laight <[email protected]>--- arch/x86/include/asm/bitops.h | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)arch/x86/include/asm/bitops.h +0
-0arch/x86/include/asm/bitops.h arch_test_bit_acquire(unsigned long nr, const volatile unsigned long *addr) · line 243
Quoted 5 lines
arch/x86/include/asm/bitops.h +1
-1@@ -240,7 +240,7 @@ arch_test_bit_acquire(unsigned long nr, const volatile unsigned long *addr)variable_test_bit (nr, addr); } -static __always_inline __attribute_const__ unsigned long variable__ffs(unsigned long word) +static __always_inline __attribute_const__ unsigned int variable__ffs(unsigned long word)There's a mismatch with the generic
ffs()inasm-generic/bitops/ffs.h.Quoted 5 lines
The
generic_ffs()returns int. There is anothervariable__ffs()defined inarch/risk, also returning int.So I believe, the correct fix would be to switch x86 to int as well. And anyways, I believe this deserves a separate series.
It is a single patch, do you want me to resend the patch on its own? (With a different commit message)
Quoted 6 lines
Quoted 2 lines
The patch looks wrong to me. All the flavors of
ffs(),ffz()and others should have the same signature.I only changed 'long' to 'int', all the generic (and builtin) ones are 'int'. So I'm not sure which difference you are talking about.
IIRC the__attribute_const__just lets the compiler do CSE.All the versions of
ffs(),ffz()and others should in the better world have the same signatures. This is unfortunately not what we have now.for
__ffs()we've got signatures:RISCV:
static__always_inline__attribute_const__unsigned longvariable__ffs(unsigned long word)
Quoted 8 lines
x86:
static__always_inline__attribute_const__unsigned longvariable__ffs(unsigned long word)Generic:
static__always_inline__attribute_const__unsigned intgeneric___ffs(unsigned long word)You're changing x86 implementation to match the generic one, but don't change RISCV, and this is wrong.
More like 'incomplete'.
It looks like chunks of the RISCV file are a straight C&P of the x86 one.
Quoted 2 lines
Can you make them all matching, and check the same for
ffz()please? This would require a small separate series, I guess.
I can do a patch for RISCV - but I'm not setup to compile it. But it should be just changing the function return types.
Quoted 4 lines
Moving forward, it would be nice to make all that helpers looking more unified across different arches and flavors. Although, out of scope of your series.
I tripped up this one as part of my min_t() checks. Basically if you do any maths with the result of the x86 'ffs' functions the other operand (which is likely to be 32bits due to the domain of the value) has to be promoted to 64bits.
This is likely to happen more often than the opposite.
David
Quoted 3 lines
Thanks,
Yury
PATCH 2/44 ext4: Fix saturation of 64bit inode times for old filesystems
In reply to [email protected]From: David Laight <[email protected]>
If an inode only has space for 32bit seconds values the code tries to saturate the times at the limit of the range (1901..2038). However the 64bit values is cast to 32bits before the comparisons.
Fix by using clamp() instead of clamp_t(int32_t, ...).
Note that this is unlikely to cause any issues until 2038.
--- fs/ext4/ext4.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/ext4/ext4.h +1 -1
@@ -909,7 +909,7 @@ do { \ (raw_inode)->xtime =cpu_to_le32 ((ts).tv_sec); \ (raw_inode)->xtime ## _extra = ext4_encode_extra_time(ts); \ }else \ - (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 )); \ }while (0 ) #define EXT4_INODE_SET_ATIME(inode, raw_inode) \
Signature · 1 line
2.39.5
PATCH 3/44 perf: Fix branch stack callchain limit
In reply to [email protected]From: David Laight <[email protected]>
The code that bounds the brs->nr to event->attr.sample_max_stack incorrectly masks brs->nr with 65535 before the limit check. Replace the min_t(u16, ...) with a plain min(...).
I guess there may be another limit on brs->nr (which is u64).
--- include/linux/perf_event.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/perf_event.h +1 -1
@@ -1430,7 +1430,7 @@ static inline void perf_sample_save_brstack(struct perf_sample_data *data,if (branch_sample_hw_index (event)) size +=sizeof (u64); - brs->nr =min_t (u16, event->attr.sample_max_stack, brs->nr); + brs->nr =min (event->attr.sample_max_stack, brs->nr); size += brs->nr *sizeof (struct perf_branch_entry );
Signature · 1 line
2.39.5
Re: [PATCH 03/44] perf: Fix branch stack callchain limit
Quoted 19 lines · On 11/20/2025 6:40 AM, [email protected] wrote:
From: David Laight <[email protected]>
The code that bounds the
brs->nrtoevent->attr.sample_max_stackincorrectly masksbrs->nrwith 65535 before the limit check. Replace themin_t(u16, ...)with a plain min(...).I guess there may be another limit on
brs->nr(which is u64).Fixes c53e14f1ea4a8 ("perf: Extend per event callchain limit to branch stack")Signed-off-by David Laight <[email protected]>--- include/linux/perf_event.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)include/linux/perf_event.h +0
-0
include/linux/perf_event.h static inline void perf_sample_save_brstack(struct perf_sample_data *data, · line 1436
Quoted 8 lines
include/linux/perf_event.h +1
-1@@ -1430,7 +1430,7 @@ static inline void perf_sample_save_brstack(struct perf_sample_data *data,if (branch_sample_hw_index (event)) size +=sizeof (u64); - brs->nr =min_t (u16, event->attr.sample_max_stack, brs->nr); + brs->nr =min (event->attr.sample_max_stack, brs->nr); size += brs->nr *sizeof (struct perf_branch_entry );
LGTM. Thanks.
PATCH 4/44 io_uring/net: Change some dubious min_t()
In reply to [email protected]From: David Laight <[email protected]>
Since iov_len is 'unsigned long' it is possible that the cast to 'int' will change the value of min_t(int, iov[nbufs].iov_len, ret). Use a plain min() and change the loop bottom to while (ret > 0) so that the compiler knows 'ret' is always positive.
Also change min_t(int, sel->val, sr->mshot_total_len) to a simple min() since sel->val is also long and subject to possible trunctation.
It might be that other checks stop these being problems, but they are picked up by some compile-time tests for min_t() truncating values.
--- io_uring/net.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
io_uring/net.c +3 -3
@@ -483,11 +483,11 @@ static int io_bundle_nbufs(struct io_async_msghdr *kmsg, int ret)/* short transfer, count segments */ nbufs =0 ;do { -int this_len =min_t (int, iov[nbufs].iov_len, ret); +int this_len =min (iov[nbufs].iov_len, ret); nbufs++; ret -= this_len; - }while (ret); + }while (ret >0 );return nbufs; }
@@ -853,7 +853,7 @@ static inline bool io_recv_finish(struct io_kiocb *req,* mshot as finished, and flag MSHOT_DONE as well to prevent * a potential bundle from being retried. */ - sr->mshot_total_len -=min_t (int, sel->val, sr->mshot_total_len); + sr->mshot_total_len -=min (sel->val, sr->mshot_total_len);if (!sr->mshot_total_len) { sr->flags |=IORING_RECV_MSHOT_DONE ; mshot_finished = true;
Signature · 1 line
2.39.5
Jens Axboe
maintainerRe: [PATCH 04/44] io_uring/net: Change some dubious min_t()
Quoted 7 lines · On 11/19/25 3:41 PM, [email protected] wrote:
From: David Laight <[email protected]>
Since
iov_lenis 'unsigned long' it is possible that the cast to 'int' will change the value ofmin_t(int, iov[nbufs].iov_len, ret). Use a plainmin()and change the loop bottom to while (ret > 0) so that the compiler knows 'ret' is always positive.
Quoted 5 lines
Also change
min_t(int, sel->val, sr->mshot_total_len)to a simplemin()sincesel->valis also long and subject to possible trunctation.It might be that other checks stop these being problems, but they are picked up by some compile-time tests for
min_t()truncating values.
Fails with clang-21:
io_uring/net.c:855:26: error: call to '__compiletime_assert_2006' declared with 'error' attribute: min(sel->val, sr->mshot_total_len) signedness error
855 | sr->mshot_total_len -= min(sel->val, sr->mshot_total_len);
| ^./include/linux/minmax.h:105:19: note: expanded from macro 'min'
105 | #define min(x, y) __careful_cmp(min, x, y)
| ^
./include/linux/minmax.h:98:2: note: expanded from macro '__careful_cmp'
98 | __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
| ^
./include/linux/minmax.h:93:2: note: expanded from macro '__careful_cmp_once'
93 | BUILD_BUG_ON_MSG(!__types_ok(ux, uy), \
| ^
note: (skipping 2 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
././include/linux/compiler_types.h:590:2: note: expanded from macro '_compiletime_assert'
590 | __compiletime_assert(condition, msg, prefix, suffix)
| ^
././include/linux/compiler_types.h:583:4: note: expanded from macro '__compiletime_assert'
583 | prefix ## suffix(); \
| ^
<scratch space>:319:1: note: expanded from here
319 | __compiletime_assert_2006
| ^Signature · 1 line
Jens Axboe
Re: [PATCH 04/44] io_uring/net: Change some dubious min_t()
Quoted 11 lines · On Thu, 20 Nov 2025 07:48:58 -0700 Jens Axboe <[email protected]> wrote:
Quoted 10 lines · On 11/19/25 3:41 PM, [email protected] wrote:
From: David Laight <[email protected]>
Since
iov_lenis 'unsigned long' it is possible that the cast to 'int' will change the value ofmin_t(int, iov[nbufs].iov_len, ret). Use a plainmin()and change the loop bottom to while (ret > 0) so that the compiler knows 'ret' is always positive.Also change
min_t(int, sel->val, sr->mshot_total_len)to a simplemin()sincesel->valis also long and subject to possible trunctation.
Quoted 7 lines
Quoted 2 lines
It might be that other checks stop these being problems, but they are picked up by some compile-time tests for
min_t()truncating values.Fails with clang-21:
io_uring/net.c:855:26: error: call to '__compiletime_assert_2006' declared with 'error' attribute:min(sel->val, sr->mshot_total_len)signedness error
855 |sr->mshot_total_len-=min(sel->val, sr->mshot_total_len);
I'll take a look, I normally use gcc but there must be something subtle going on. Actually which architecture?
I only tested x86-64.
David
Quoted 21 lines
| ^
./include/linux/minmax.h:105:19: note: expanded from macro 'min'105 | #define min(x, y) __careful_cmp(min, x, y) | ^ ./include/linux/minmax.h:98:2: note: expanded from macro '__careful_cmp' 98 | __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_)) | ^ ./include/linux/minmax.h:93:2: note: expanded from macro '__careful_cmp_once' 93 | BUILD_BUG_ON_MSG(!__types_ok(ux, uy), \ | ^ note: (skipping 2 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all) ././include/linux/compiler_types.h:590:2: note: expanded from macro '_compiletime_assert' 590 | __compiletime_assert(condition, msg, prefix, suffix) | ^ ././include/linux/compiler_types.h:583:4: note: expanded from macro '__compiletime_assert' 583 | prefix ## suffix(); \ | ^ <scratch space>:319:1: note: expanded from here 319 | __compiletime_assert_2006 | ^
Jens Axboe
maintainerRe: [PATCH 04/44] io_uring/net: Change some dubious min_t()
Quoted 17 lines · David Laight
Quoted 14 lines · On Thu, 20 Nov 2025 07:48:58 -0700 Jens Axboe <[email protected]> wrote:
Quoted 12 lines · On 11/19/25 3:41 PM, [email protected] wrote:
From: David Laight <[email protected]>
Since
iov_lenis 'unsigned long' it is possible that the cast to 'int' will change the value ofmin_t(int, iov[nbufs].iov_len, ret). Use a plainmin()and change the loop bottom to while (ret > 0) so that the compiler knows 'ret' is always positive.Also change
min_t(int, sel->val, sr->mshot_total_len)to a simplemin()sincesel->valis also long and subject to possible trunctation.It might be that other checks stop these being problems, but they are picked up by some compile-time tests for
min_t()truncating values.
Quoted 7 lines
Quoted 4 lines
Fails with clang-21:
io_uring/net.c:855:26: error: call to '__compiletime_assert_2006' declared with 'error' attribute:min(sel->val, sr->mshot_total_len)signedness error
855 |sr->mshot_total_len-=min(sel->val, sr->mshot_total_len);I'll take a look, I normally use gcc but there must be something subtle going on. Actually which architecture? I only tested x86-64.
This is x86-64.
Signature · 1 line
Jens Axboe
Re: [PATCH 04/44] io_uring/net: Change some dubious min_t()
Quoted 20 lines · On Thu, 20 Nov 2025 08:53:56 -0700 Jens Axboe <[email protected]> wrote:
Quoted 19 lines · David Laight
Quoted 16 lines · On Thu, 20 Nov 2025 07:48:58 -0700 Jens Axboe <[email protected]> wrote:
Quoted 7 lines · On 11/19/25 3:41 PM, [email protected] wrote:
From: David Laight <[email protected]>
Since
iov_lenis 'unsigned long' it is possible that the cast to 'int' will change the value ofmin_t(int, iov[nbufs].iov_len, ret). Use a plainmin()and change the loop bottom to while (ret > 0) so that the compiler knows 'ret' is always positive.Quoted 5 lines
Also change
min_t(int, sel->val, sr->mshot_total_len)to a simplemin()sincesel->valis also long and subject to possible trunctation.It might be that other checks stop these being problems, but they are picked up by some compile-time tests for
min_t()truncating values.Fails with clang-21:
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
io_uring/net.c:855:26: error: call to '__compiletime_assert_2006' declared with 'error' attribute:min(sel->val, sr->mshot_total_len)signedness error
855 |sr->mshot_total_len-=min(sel->val, sr->mshot_total_len);I'll take a look, I normally use gcc but there must be something subtle going on. Actually which architecture? I only tested x86-64.
This is x86-64.
It is related to the test a few lines higher:
if (sel->val > 0 && sr->flags & IORING_RECV_MSHOT_LIM) {'sel->val' is ssize_t, gcc is tracking that test so statically_true(sel->val >= 0) is 'true' and the signed variable can be compared against the 'unsigned' 'sr->mshot_total_len'.
It seems clang isn't as clever.
Perhaps it reloads sel->val from memory - so loses the result of the comparison. Even swapping the order of the two tests might fix it.
As might caching sel->val in a local (even a signed one).
The comment in the header file for io_br_sel.val doesn't seem to include the case where 'val' is a length!
The simple fix is to use umin() since all the values are clearly non-negative.
David
PATCH 5/44 ipc/msg: Fix saturation of percpu counts in msgctl_info()
In reply to [email protected]From: David Laight <[email protected]>
While the percpu_counter_sum() values are unlikely to be large the code tries to saturate them using min_t(int, percpu_counter_sum(), INT_MAX) This just doesn't work since the high bits are all masked and then the (possibly negative) value assigned.
Replace the min_t() with a plain min().
--- ipc/msg.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
ipc/msg.c +2 -4
@@ -501,11 +501,9 @@ static int msgctl_info(struct ipc_namespace *ns, int msqid, max_idx =ipc_get_maxidx (&msg_ids (ns));up_read (&msg_ids (ns).rwsem);if (cmd ==MSG_INFO ) { - msginfo->msgmap =min_t (int, -percpu_counter_sum (&ns->percpu_msg_hdrs), + msginfo->msgmap =min (percpu_counter_sum (&ns->percpu_msg_hdrs),INT_MAX ); - msginfo->msgtql =min_t (int, -percpu_counter_sum (&ns->percpu_msg_bytes), + msginfo->msgtql =min (percpu_counter_sum (&ns->percpu_msg_bytes),INT_MAX ); }else { msginfo->msgmap =MSGMAP ;
Signature · 1 line
2.39.5
Andrew Morton
maintainerRe: [PATCH 05/44] ipc/msg: Fix saturation of percpu counts in msgctl_info()
Quoted 1 line · On Wed, 19 Nov 2025 22:41:01 +0000 [email protected] wrote:
It seems but yesterday.
Quoted 21 lines
While the
percpu_counter_sum()values are unlikely to be large the code tries to saturate them usingmin_t(int, percpu_counter_sum(), INT_MAX)This just doesn't work since the high bits are all masked and then the (possibly negative) value assigned.Replace the
min_t()with a plainmin().Fixes 72d1e611082ed ("ipc/msg: mitigate the lock contention with percpu counter")Signed-off-by David Laight <[email protected]>--- ipc/msg.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)ipc/msg.c +0
-0@@ -501,11 +501,9 @@ static int msgctl_info(struct ipc_namespace *ns, int msqid, max_idx =ipc_get_maxidx (&msg_ids (ns));up_read (&msg_ids (ns).rwsem);if (cmd ==MSG_INFO ) {
ipc/msg.c static int msgctl_info(struct ipc_namespace *ns, int msqid, · line 509
Quoted 10 lines
ipc/msg.c +2
-4- msginfo->msgmap =min_t (int, -percpu_counter_sum (&ns->percpu_msg_hdrs), + msginfo->msgmap =min (percpu_counter_sum (&ns->percpu_msg_hdrs),INT_MAX ); - msginfo->msgtql =min_t (int, -percpu_counter_sum (&ns->percpu_msg_bytes), + msginfo->msgtql =min (percpu_counter_sum (&ns->percpu_msg_bytes),INT_MAX ); }else { msginfo->msgmap =MSGMAP ;
Added, thanks.
As always, bug fixes should include a clear description of the worst-case userspace-visible runtime effects. LLM said:
: When aggregate System V message queue usage exceeds INT_MAX, MSG_INFO : returns wrapped statistics instead of saturating them. At 2 GiB of : queued data the byte count becomes negative, and at 4 GiB it becomes : zero. Consequently, ipcs -q -u can display an enormous used-space : value or report no queued data despite substantial usage. Monitoring : programs using this interface can therefore substantially underreport : or overreport resource consumption. The bug affects statistics; it : does not corrupt queued messages or alter queue-limit enforcement.
So I'll add a cc:stable to this.
Andrew Morton
maintainerRe: [PATCH 05/44] ipc/msg: Fix saturation of percpu counts in msgctl_info()
Quoted 12 lines · On Wed, 28 Oct 2026 18:15:02 -0700 Andrew Morton <[email protected]>…
Quoted 12 lines
ipc/msg.c +1
-2@@ -501,11 +501,9 @@ static int msgctl_info(struct ipc_namespace *ns, int msqid, max_idx =ipc_get_maxidx (&msg_ids (ns));up_read (&msg_ids (ns).rwsem);if (cmd ==MSG_INFO ) { - msginfo->msgmap =min_t (int, -percpu_counter_sum (&ns->percpu_msg_hdrs), + msginfo->msgmap =min (percpu_counter_sum (&ns->percpu_msg_hdrs),INT_MAX );
Quoted 9 lines
ipc/msg.c static int msgctl_info(struct ipc_namespace *ns, int msqid, · line 509
Quoted 6 lines
ipc/msg.c +1
-2- msginfo->msgtql =min_t (int, -percpu_counter_sum (&ns->percpu_msg_bytes), + msginfo->msgtql =min (percpu_counter_sum (&ns->percpu_msg_bytes),INT_MAX ); }else { msginfo->msgmap =MSGMAP ;Added, thanks.
LLM found another bug in there. Please review:
From: Andrew Morton <[email protected]>
Subject: ipc/msg: clamp negative MSG_INFO usage counts
Date: Thu, 29 Oct 2026 01:43:24 +0000
MSG_INFO can report negative message and byte counts during ordinary queue traffic. For example, ipcs -q -u prints a negative header count and can convert a negative byte count into an enormous unsigned used-space value.
percpu_counter_sum() does not take an atomic snapshot of all CPUs. While a third CPU sums the counters, it can read the sending CPU's local count before an enqueue and the receiving CPU's local count after the matching dequeue. The sampled total is then negative, even if the queue was initially empty and is empty again. The sum's lock serializes folding into the global count, but not ordinary local counter updates. Queue locking does not serialize these updates with the statistics reader.
Use percpu_counter_sum_positive() for both usage counters before applying the upper clamp. Keep the comparison in s64 so the result is not narrowed before saturation.
This bounds the reported usage without making the statistics an atomic snapshot. Message delivery and queue-limit enforcement are unchanged.
This is a follow-up to David Laight's "ipc/msg: Fix saturation of percpu counts in msgctl_info()".
Validated with a source-extraction regression harness, including a forced three-CPU sampling trace and failing old-code controls. No running-kernel reproduction or kernel build was performed.
Backporting: kernels such as v6.1 and v6.6 require matching min() operand types. Use min_t(s64, ...) instead of min() on those kernels. For trees still using min_t(int, ...), also retain the s64 upper clamp; the separate standalone variant includes both adjustments.
---
ipc/msg.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
ipc/msg.c renamed from ipc/msg.c~ipc-msg-clamp-negative-statistics +6 -4
@@ -501,10 +501,12 @@ static int msgctl_info(struct ipc_namesp max_idx =ipc_get_maxidx (&msg_ids (ns));up_read (&msg_ids (ns).rwsem);if (cmd ==MSG_INFO ) { - msginfo->msgmap =min (percpu_counter_sum (&ns->percpu_msg_hdrs), -INT_MAX ); - msginfo->msgtql =min (percpu_counter_sum (&ns->percpu_msg_bytes), -INT_MAX ); + msginfo->msgmap =min ( +percpu_counter_sum_positive (&ns->percpu_msg_hdrs), +INT_MAX ); + msginfo->msgtql =min ( +percpu_counter_sum_positive (&ns->percpu_msg_bytes), +INT_MAX ); }else { msginfo->msgmap =MSGMAP ; msginfo->msgpool =MSGPOOL ;
_
Re: [PATCH 05/44] ipc/msg: Fix saturation of percpu counts in msgctl_info()
Quoted 24 lines · On Wed, 28 Oct 2026 21:04:54 -0700 Andrew Morton <[email protected]>…
Quoted 12 lines · On Wed, 28 Oct 2026 18:15:02 -0700 Andrew Morton <[email protected]>…
Quoted 12 lines
ipc/msg.c +1
-2@@ -501,11 +501,9 @@ static int msgctl_info(struct ipc_namespace *ns, int msqid, max_idx =ipc_get_maxidx (&msg_ids (ns));up_read (&msg_ids (ns).rwsem);if (cmd ==MSG_INFO ) { - msginfo->msgmap =min_t (int, -percpu_counter_sum (&ns->percpu_msg_hdrs), + msginfo->msgmap =min (percpu_counter_sum (&ns->percpu_msg_hdrs),INT_MAX );Quoted 9 lines
ipc/msg.c static int msgctl_info(struct ipc_namespace *ns, int msqid, · line 509
Quoted 6 lines
ipc/msg.c +1
-2- msginfo->msgtql =min_t (int, -percpu_counter_sum (&ns->percpu_msg_bytes), + msginfo->msgtql =min (percpu_counter_sum (&ns->percpu_msg_bytes),INT_MAX ); }else { msginfo->msgmap =MSGMAP ;Added, thanks.
Quoted 1 line
LLM found another bug in there. Please review:
Seems reasonable, if unlikely to ever happen.
Description is overlong ...
Might be worth doing a scan of all uses of percpu_counter_sum(), I can imagine it is a common problem.
Oh and the code could use clamp(per_cpu_counter_sum(...), 0, INT_MAX); which is exactly the same but maybe clearer?
Quoted 53 lines
From: Andrew Morton <[email protected]>
Subject: ipc/msg: clamp negativeMSG_INFOusage counts
Date: Thu, 29 Oct 2026 01:43:24 +0000
MSG_INFOcan report negative message and byte counts during ordinary queue traffic. For example, ipcs -q -u prints a negative header count and can convert a negative byte count into an enormous unsigned used-space value.
percpu_counter_sum()does not take an atomic snapshot of all CPUs. While a third CPU sums the counters, it can read the sending CPU's local count before an enqueue and the receiving CPU's local count after the matching dequeue. The sampled total is then negative, even if the queue was initially empty and is empty again. The sum's lock serializes folding into the global count, but not ordinary local counter updates. Queue locking does not serialize these updates with the statistics reader.Use
percpu_counter_sum_positive()for both usage counters before applying the upper clamp. Keep the comparison in s64 so the result is not narrowed before saturation.This bounds the reported usage without making the statistics an atomic snapshot. Message delivery and queue-limit enforcement are unchanged.
This is a follow-up to David Laight's "ipc/msg: Fix saturation of percpu counts in
msgctl_info()".Validated with a source-extraction regression harness, including a forced three-CPU sampling trace and failing old-code controls. No running-kernel reproduction or kernel build was performed.
Backporting: kernels such as v6.1 and v6.6 require matching
min()operand types. Usemin_t(s64, ...)instead ofmin()on those kernels. For trees still usingmin_t(int, ...), also retain the s64 upper clamp; the separate standalone variant includes both adjustments.Fixes 72d1e611082e ("ipc/msg: mitigate the lock contention with percpu counter")Signed-off-by Andrew Morton <[email protected]>Cc David Laight <[email protected]>Cc Davidlohr Bueso <[email protected]>Cc Manfred Spraul <[email protected]>Cc <[email protected]>---
ipc/msg.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)ipc/msg.c renamed from ipc/msg.c~ipc-msg-clamp-negative-statistics +0
-0@@ -501,10 +501,12 @@ static int msgctl_info(struct ipc_namesp max_idx =ipc_get_maxidx (&msg_ids (ns));up_read (&msg_ids (ns).rwsem);if (cmd ==MSG_INFO ) {
ipc/msg.c static int msgctl_info(struct ipc_namesp · line 509
Quoted 10 lines
ipc/msg.c renamed from ipc/msg.c~ipc-msg-clamp-negative-statistics +6
-4- msginfo->msgmap =min (percpu_counter_sum (&ns->percpu_msg_hdrs), -INT_MAX ); - msginfo->msgtql =min (percpu_counter_sum (&ns->percpu_msg_bytes), -INT_MAX ); + msginfo->msgmap =min ( +percpu_counter_sum_positive (&ns->percpu_msg_hdrs), +INT_MAX ); + msginfo->msgtql =min ( +percpu_counter_sum_positive (&ns->percpu_msg_bytes), +INT_MAX );
checkpatch won't like those line breaks!
David
Quoted 5 lines
ipc/msg.c renamed from ipc/msg.c~ipc-msg-clamp-negative-statistics +0
-0}else { msginfo->msgmap =MSGMAP ; msginfo->msgpool =MSGPOOL ;_
Andrew Morton
maintainerRe: [PATCH 05/44] ipc/msg: Fix saturation of percpu counts in msgctl_info()
Quoted 27 lines · On Thu, 29 Oct 2026 09:45:43 +0000 David Laight <[email protected]>…
Quoted 24 lines · On Wed, 28 Oct 2026 21:04:54 -0700 Andrew Morton <[email protected]>…
Quoted 21 lines · On Wed, 28 Oct 2026 18:15:02 -0700 Andrew Morton <[email protected]>…
Quoted 8 lines
ipc/msg.c +0
-0@@ -501,11 +501,9 @@ static int msgctl_info(struct ipc_namespace *ns, int msqid, max_idx =ipc_get_maxidx (&msg_ids (ns));up_read (&msg_ids (ns).rwsem);if (cmd ==MSG_INFO ) {ipc/msg.c static int msgctl_info(struct ipc_namespace *ns, int msqid, · line 509
Quoted 10 lines
ipc/msg.c +2
-4- msginfo->msgmap =min_t (int, -percpu_counter_sum (&ns->percpu_msg_hdrs), + msginfo->msgmap =min (percpu_counter_sum (&ns->percpu_msg_hdrs),INT_MAX ); - msginfo->msgtql =min_t (int, -percpu_counter_sum (&ns->percpu_msg_bytes), + msginfo->msgtql =min (percpu_counter_sum (&ns->percpu_msg_bytes),INT_MAX ); }else { msginfo->msgmap =MSGMAP ;Added, thanks.
Quoted 7 lines
Quoted 1 line
LLM found another bug in there. Please review:
Seems reasonable, if unlikely to ever happen.
Description is overlong ...Might be worth doing a scan of all uses of
percpu_counter_sum(), I can imagine it is a common problem.
I had LLM do a scan:
: The scan found four similar reporting problems:
:
: - Ceph: metrics_file_show() in fs/ceph/debugfs.c prints the raw
: opened-inode count as signed. ceph_mdsc_send_metrics() in : fs/ceph/metric.c converts it to unsigned for the MDS. An open/close : pair on different CPUs can yield -1, appearing as a negative count : in debugfs or U64_MAX in the metric. : : - ext4: ext4_attr_show() in fs/ext4/sysfs.c uses the raw dirty-cluster : count for delayed_allocation_blocks. Reserving and then consuming : or releasing a reservation on different CPUs can produce a : negative sample. With one block per cluster, -1 is reported as : U64_MAX blocks. : : - BPF: htab_map_mem_usage() in kernel/bpf/hashtab.c converts the raw : entry count to unsigned before calculating memory usage. This : affects nonpreallocated hash maps using per-CPU counters. An : insert/delete pair can yield -1 entries and an incorrect memory : estimate, including less than the empty-map overhead. : : - tmpfs: shmem_statfs() in mm/shmem.c subtracts the raw used-block : count from the block limit. An allocation/free pair can yield -1 : used blocks, reporting more free blocks than total blocks. Tools : calculating used space as total minus free can consequently : report negative usage.
:
: These are source-level findings, not runtime reproductions. No : corruption or resource-limit bypass was established.
:
: Other callers already floor negative samples, count cumulative events, : check accounting after teardown, or use deliberately signed values. : Drain checks and some XFS statistics still need further investigation. : This should not be a blanket conversion, and flooring negative samples : does not make the statistics an atomic snapshot.
Quoted 2 lines
Oh and the code could use
clamp(per_cpu_counter_sum(...), 0, INT_MAX); which is exactly the same but maybe clearer?
Sounds good. How does the below look?
From: Andrew Morton <[email protected]>
Subject: ipc/msg: clamp negative MSG_INFO usage counts
Date: Thu, 29 Oct 2026 01:43:24 +0000
MSG_INFO can report negative usage during ordinary queue traffic. For example, ipcs -q -u prints negative header counts and converts negative byte counts into enormous unsigned used-space values.
percpu_counter_sum() can sample the sending CPU before an enqueue and the receiving CPU after the matching dequeue, yielding a negative total. Clamp both counts to [0, INT_MAX] before converting to int. The statistics remain an approximate, non-atomic snapshot.
---
ipc/msg.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
ipc/msg.c renamed from ipc/msg.c~ipc-msg-clamp-negative-msg_info-usage-counts +4 -4
@@ -501,10 +501,10 @@ static int msgctl_info(struct ipc_namesp max_idx =ipc_get_maxidx (&msg_ids (ns));up_read (&msg_ids (ns).rwsem);if (cmd ==MSG_INFO ) { - msginfo->msgmap =min (percpu_counter_sum (&ns->percpu_msg_hdrs), -INT_MAX ); - msginfo->msgtql =min (percpu_counter_sum (&ns->percpu_msg_bytes), -INT_MAX ); + msginfo->msgmap =clamp (percpu_counter_sum (&ns->percpu_msg_hdrs), +0 ,INT_MAX ); + msginfo->msgtql =clamp (percpu_counter_sum (&ns->percpu_msg_bytes), +0 ,INT_MAX ); }else { msginfo->msgmap =MSGMAP ; msginfo->msgpool =MSGPOOL ;
_
PATCH 6/44 bpf: Verifier, remove some unusual uses of min_t() and max_t()
In reply to [email protected]From: David Laight <[email protected]>
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.
--- kernel/bpf/verifier.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-)
kernel/bpf/verifier.c +11 -18
@@ -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 ))); + reg->u32_min_value =max (reg->u32_min_value, (u32 )var32_off.value); reg->u32_max_value =min (reg->u32_max_value, (u32)(var32_off.value | var32_off.mask)); }
@@ -2332,11 +2332,11 @@ static void __update_reg32_bounds(struct bpf_reg_state *reg)static void __update_reg64_bounds (struct bpf_reg_state *reg) {/* min signed is max(sign bit) | min(other bits) */ - reg->smin_value =max_t (s64, reg->smin_value, - reg->var_off.value | (reg->var_off.mask &S64_MIN )); + reg->smin_value =max (reg->smin_value, + (s64)(reg->var_off.value | (reg->var_off.mask &S64_MIN )));/* max signed is min(sign bit) | max(other bits) */ - reg->smax_value =min_t (s64, reg->smax_value, - reg->var_off.value | (reg->var_off.mask &S64_MAX )); + reg->smax_value =min (reg->smax_value, + (s64)(reg->var_off.value | (reg->var_off.mask &S64_MAX ))); reg->umin_value =max (reg->umin_value, reg->var_off.value); reg->umax_value =min (reg->umax_value, reg->var_off.value | reg->var_off.mask);
@@ -6128,15 +6128,8 @@ static int check_packet_access(struct bpf_verifier_env *env, u32 regno, int off,return err; } -/* __check_mem_access has made sure "off + size - 1" is within u16. -* reg->umax_value can't be bigger than MAX_PACKET_OFF which is 0xffff, -* otherwise find_good_pkt_pointers would have refused to set range info -* that __check_mem_access would have rejected this pkt access. -* Therefore, "off + reg->umax_value + size - 1" won't overflow u32. -*/ - env->prog->aux->max_pkt_offset = -max_t (u32, env->prog->aux->max_pkt_offset, - off + reg->umax_value + size -1 ); + env->prog->aux->max_pkt_offset =max (env->prog->aux->max_pkt_offset, + off + reg->umax_value + size -1 );return err; }
Signature · 1 line
2.39.5
Re: [PATCH 06/44] bpf: Verifier, remove some unusual uses of min_t() and max_t()
Quoted 27 lines · On Wed, Nov 19, 2025 at 2:42 PM <[email protected]> wrote:
From: David Laight <[email protected]>
min_t()andmax_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 themax_pkt_offsetcalulation and delete the comment about why the cast to u32 is safe.Signed-off-by David Laight <[email protected]>--- kernel/bpf/verifier.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-)kernel/bpf/verifier.c +0
-0@@ -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) */
kernel/bpf/verifier.c static void __update_reg32_bounds(struct bpf_reg_state *reg) · line 2326
Quoted 10 lines
kernel/bpf/verifier.c +4
-5- 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.
pw-bot: cr
Re: [PATCH 06/44] bpf: Verifier, remove some unusual uses of min_t() and max_t()
Quoted 27 lines · On Fri, 21 Nov 2025 13:40:36 -0800 Alexei Starovoitov…
Quoted 26 lines · On Wed, Nov 19, 2025 at 2:42 PM <[email protected]> wrote:
From: David Laight <[email protected]>
min_t()andmax_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 themax_pkt_offsetcalulation and delete the comment about why the cast to u32 is safe.Signed-off-by David Laight <[email protected]>--- kernel/bpf/verifier.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-)kernel/bpf/verifier.c +0
-0@@ -2319,12 +2319,12 @@ static void __update_reg32_bounds(struct bpf_reg_state *reg)struct tnum var32_off =tnum_subreg (reg->var_off);
Quoted 15 lines
kernel/bpf/verifier.c static void __update_reg32_bounds(struct bpf_reg_state *reg) · line 2326
Quoted 11 lines
kernel/bpf/verifier.c +4
-5/* 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
Quoted 2 lines
pw-bot: cr
Re: [PATCH 06/44] bpf: Verifier, remove some unusual uses of min_t() and max_t()
Quoted 47 lines · David Laight
Quoted 27 lines · On Fri, 21 Nov 2025 13:40:36 -0800 Alexei Starovoitov…
Quoted 26 lines · On Wed, Nov 19, 2025 at 2:42 PM <[email protected]> wrote:
From: David Laight <[email protected]>
min_t()andmax_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 themax_pkt_offsetcalulation and delete the comment about why the cast to u32 is safe.Signed-off-by David Laight <[email protected]>--- kernel/bpf/verifier.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-)kernel/bpf/verifier.c +0
-0@@ -2319,12 +2319,12 @@ static void __update_reg32_bounds(struct bpf_reg_state *reg)struct tnum var32_off =tnum_subreg (reg->var_off);Quoted 15 lines
kernel/bpf/verifier.c static void __update_reg32_bounds(struct bpf_reg_state *reg) · line 2326
Quoted 11 lines
kernel/bpf/verifier.c +4
-5/* 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.
Quoted 8 lines
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()andmax_t().
The number of real (or potentially real) bugs I've found is stunning. The only fix is to nukemin_t()andmax_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.
Quoted 11 lines
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:(quoted diff) +1
-1- (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).
Quoted 1 line
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.
Re: [PATCH 06/44] bpf: Verifier, remove some unusual uses of min_t() and max_t()
Quoted 53 lines · On Sun, 23 Nov 2025 08:39:51 -0800 Alexei Starovoitov…
Quoted 51 lines · David Laight
Quoted 27 lines · On Fri, 21 Nov 2025 13:40:36 -0800 Alexei Starovoitov…
Quoted 26 lines · On Wed, Nov 19, 2025 at 2:42 PM <[email protected]> wrote:
From: David Laight <[email protected]>
min_t()andmax_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 themax_pkt_offsetcalulation and delete the comment about why the cast to u32 is safe.Signed-off-by David Laight <[email protected]>--- kernel/bpf/verifier.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-)kernel/bpf/verifier.c +0
-0@@ -2319,12 +2319,12 @@ static void __update_reg32_bounds(struct bpf_reg_state *reg)struct tnum var32_off =tnum_subreg (reg->var_off);Quoted 15 lines
kernel/bpf/verifier.c static void __update_reg32_bounds(struct bpf_reg_state *reg) · line 2326
Quoted 11 lines
kernel/bpf/verifier.c +4
-5/* 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.
Quoted 7 lines
Quoted 4 lines
I've been trying to build allmodconfig with the size test added to
min_t()andmax_t().
The number of real (or potentially real) bugs I've found is stunning. The only fix is to nukemin_t()andmax_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...
Quoted 11 lines
Quoted 11 lines
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:(quoted diff) +1
-1- (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).
Quoted 5 lines
Quoted 1 line
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.
Quoted 3 lines
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
Re: [PATCH 06/44] bpf: Verifier, remove some unusual uses of min_t() and max_t()
Quoted 91 lines · David Laight
Quoted 53 lines · On Sun, 23 Nov 2025 08:39:51 -0800 Alexei Starovoitov…
Quoted 51 lines · David Laight
Quoted 27 lines · On Fri, 21 Nov 2025 13:40:36 -0800 Alexei Starovoitov…
Quoted 26 lines · On Wed, Nov 19, 2025 at 2:42 PM <[email protected]> wrote:
From: David Laight <[email protected]>
min_t()andmax_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 themax_pkt_offsetcalulation and delete the comment about why the cast to u32 is safe.Signed-off-by David Laight <[email protected]>--- kernel/bpf/verifier.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-)kernel/bpf/verifier.c +0
-0@@ -2319,12 +2319,12 @@ static void __update_reg32_bounds(struct bpf_reg_state *reg)struct tnum var32_off =tnum_subreg (reg->var_off);Quoted 15 lines
kernel/bpf/verifier.c static void __update_reg32_bounds(struct bpf_reg_state *reg) · line 2326
Quoted 11 lines
kernel/bpf/verifier.c +4
-5/* 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.Quoted 7 lines
Quoted 4 lines
I've been trying to build allmodconfig with the size test added to
min_t()andmax_t().
The number of real (or potentially real) bugs I've found is stunning. The only fix is to nukemin_t()andmax_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...
Quoted 11 lines
Quoted 11 lines
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:(quoted diff) +1
-1- (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).Quoted 5 lines
Quoted 1 line
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.Quoted 3 lines
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.
Quoted 5 lines
It is too late by the time you get to
clamp_once().
The 'type' for all thexxx_t()functions is an input cast, not the type for the result.clamp_t(type, v, lo, hi)has always beenclamp((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.
Quoted 2 lines
I've already fixed
clamp()so it doesn't complain about comparing s64 against s32. The next stage is to change pretty much all thexxx_t()to plainxxx().
Nack to that. Fix the problem. Not the symptom.
Re: [PATCH 06/44] bpf: Verifier, remove some unusual uses of min_t() and max_t()
Quoted 99 lines · On Sun, 23 Nov 2025 11:20:03 -0800 Alexei Starovoitov…
Quoted 91 lines · David Laight
Quoted 53 lines · On Sun, 23 Nov 2025 08:39:51 -0800 Alexei Starovoitov…
Quoted 51 lines · David Laight
Quoted 27 lines · On Fri, 21 Nov 2025 13:40:36 -0800 Alexei Starovoitov…
Quoted 26 lines · On Wed, Nov 19, 2025 at 2:42 PM <[email protected]> wrote:
From: David Laight <[email protected]>
min_t()andmax_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 themax_pkt_offsetcalulation and delete the comment about why the cast to u32 is safe.Signed-off-by David Laight <[email protected]>--- kernel/bpf/verifier.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-)kernel/bpf/verifier.c +0
-0@@ -2319,12 +2319,12 @@ static void __update_reg32_bounds(struct bpf_reg_state *reg)struct tnum var32_off =tnum_subreg (reg->var_off);Quoted 15 lines
kernel/bpf/verifier.c static void __update_reg32_bounds(struct bpf_reg_state *reg) · line 2326
Quoted 11 lines
kernel/bpf/verifier.c +4
-5/* 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.Quoted 7 lines
Quoted 4 lines
I've been trying to build allmodconfig with the size test added to
min_t()andmax_t().
The number of real (or potentially real) bugs I've found is stunning. The only fix is to nukemin_t()andmax_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...
Quoted 11 lines
Quoted 11 lines
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:(quoted diff) +1
-1- (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).Quoted 5 lines
Quoted 1 line
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.Quoted 3 lines
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.Quoted 5 lines
It is too late by the time you get to
clamp_once().
The 'type' for all thexxx_t()functions is an input cast, not the type for the result.clamp_t(type, v, lo, hi)has always beenclamp((type)v, (type)lo, type(hi)). From a code correctness point of view you pretty much never want those casts.
Quoted 3 lines
Historical behavior doesn't justify a footgun.
You definitely can makeclampt_t()to behave likeclamp_val()plus the final cast.
clamp_val() is actually the worst of the lot.
Quoted 7 lines
Also note:
git grep -w clamp_val|wc -l
818
git grep -w clamp_t|wc -l
494a safer macro is already used more often.
clamp_val() is worse than clamp_t() ...
Nope...
The problem is that clamp() requires all three parameters have the same type. Coders are lazy and want to write clamp(variable, 1, 10).
This was fine if 'variable' had type 'int', but if it was 'unsigned int' you had to write clamp(variable, 1u, 10u), worse if it is 'u8' you to either cast both constants clamp(variable, (u8)1, (u8)10) or the variable clamp((int)variable, 1, 10).
It the types/values aren't immediately obvious then any of those casts can discard high bits.
A lot of the clamp_val() are actually for u8 structure members. One thing to remember about C is it doesn't have any maths operators for u8, the values are always promoted to 'int' before anything happens. So if you write (foo->u8_member > 4 ? foo->u8_member : 4) the comparison is done as an integer one.
Add some casts ((u8)f->m > (u8)x ? (u8)f->m : (u8)x) then the values are all masked to 8 bits, promoted to 32 and then compared.
Even the ?: operator promotes its arguments and has a result type of 'int'.
Consider clamp_val(f->u8_m, LO, HI);
If HI is 255 it is fine, make HI 256 (perhaps it is sizeof() and something got changed) and you suddenly have clamp(f->u8_m, LO, 0).
It is all just so fragile.
Maybe you are trying to find the 'chunk size' for a transfer of some kind. If the transfer size is 'small' it might be in a 'u32', you want to limit it to the size of the hardware's PCIe window - so do:
copy_size = min(transfer_size, hardware_window_size);
But the hardware_window_size is a size_t (so 64bit).
The old min() would complain about the type mismatch, since the hardware_window_size might actually be 4GB (that is true) casting to u32 is broken - you have to use the larger type.
But it might be the other way around, transfer_size is u64 and hardware_window_size is u32, you still have to cast to u64 - but this time it is the size of the other parameter.
The trouble is people have a habit of using the type they want for the result, u32 in both the above and wrong twice.
But it was only the type check in min() that caused a problem. Without the casts the compiler generates the right code, the only problem is when a signed variable might contain a negative value that gets promoted to a large negative value.
The current implementations of min/max/clamp only generate an error if they can't prove that negative values won't be promoted to large unsigned values.
This is all fine provided the variable/expressions have the correct type for the value they contain - and they usually do.
But for this bpf code the type of 'var32_off.value | (var32_off.mask & S32_MIN)' is actually u64, it really does need an explicit cast to s32.
David
Quoted 5 lines
Quoted 3 lines
I've already fixed
clamp()so it doesn't complain about comparing s64 against s32. The next stage is to change pretty much all thexxx_t()to plainxxx().Nack to that. Fix the problem. Not the symptom.
PATCH 7/44 net/core/flow_dissector: Fix cap of __skb_flow_dissect() return value.
In reply to [email protected]From: David Laight <[email protected]>
There are some dodgy clamp_t(u16, ...) and min_t(u16, ...).
__skb_flow_dissect() tries to cap its return value with:
key_control->thoff = min_t(u16, nhoff, skb ? skb->len : hlen);
however this casts skb->len to u16 before the comparison. While both nboff and hlen are 'small', skb->len could be 0x10001 which gets converted to 1 by the cast.
This gives an invalid (small) value for thoff for valid packets.
bpf_flow_dissect() used clamp_t(u16, ...) to set both flow_keys->nhoff and flow_keys->thoff.
While I think these can't lose significant bits the casts are unnecessary plain clamp(...) works fine.
--- net/core/flow_dissector.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
net/core/flow_dissector.c +3 -4
@@ -1023,9 +1023,8 @@ u32 bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx, result =bpf_prog_run_pin_on_cpu (prog, ctx); - flow_keys->nhoff =clamp_t (u16, flow_keys->nhoff, nhoff, hlen); - flow_keys->thoff =clamp_t (u16, flow_keys->thoff, - flow_keys->nhoff, hlen); + flow_keys->nhoff =clamp (flow_keys->nhoff, nhoff, hlen); + flow_keys->thoff =clamp (flow_keys->thoff, flow_keys->nhoff, hlen);return result; }
@@ -1687,7 +1686,7 @@ bool __skb_flow_dissect(const struct net *net, ret = true; out: - key_control->thoff =min_t (u16, nhoff, skb ? skb->len : hlen); + key_control->thoff =umin (nhoff, skb ? skb->len : hlen); key_basic->n_proto = proto; key_basic->ip_proto = ip_proto;
Signature · 1 line
2.39.5
PATCH 8/44 net: ethtool: Use min3() instead of nested min_t(u16,...)
In reply to [email protected]From: David Laight <[email protected]>
In ethtool_cmis_cdb_execute_epl_cmd() change space_left and bytes_to_write from u16 to u32.
Although the values may fit in 16 bits, 32bit variables will generate better code.
Replace the nested min_t(u16, bytes_left, min_t(u16, space_left, x)) with a call to min3().
--- net/ethtool/cmis_cdb.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
net/ethtool/cmis_cdb.c +3 -4
@@ -573,12 +573,11 @@ ethtool_cmis_cdb_execute_epl_cmd(struct net_device *dev,while (offset <=CMIS_CDB_EPL_FW_BLOCK_OFFSET_END && bytes_written < epl_len) {u32 bytes_left = epl_len - bytes_written; -u16 space_left, bytes_to_write; +u32 space_left, bytes_to_write; space_left =CMIS_CDB_EPL_FW_BLOCK_OFFSET_END - offset +1 ; - bytes_to_write =min_t (u16, bytes_left, -min_t (u16, space_left, - args->read_write_len_ext)); + bytes_to_write =min3 (bytes_left, space_left, + args->read_write_len_ext); err = __ethtool_cmis_cdb_execute_cmd(dev, page_data, page, offset,
Signature · 1 line
2.39.5
PATCH 9/44 ipv6: __ip6_append_data() don't abuse max_t() casts
In reply to [email protected]From: David Laight <[email protected]>
The implicit casts done by max_t() should only really be used to convert positive values to signed or unsigned types. In the EMSGSIZE error path
pmtu = max_t(int, mtu - headersize + sizeof(struct ipv6hdr), 0);
is being used to convert a large unsigned value to a signed negative one. Rework using a signed temporary variable and max(pmtu, 0), as well as casting sizeof() to (int) - which is where the unsignedness comes from.
--- net/ipv6/ip6_output.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
net/ipv6/ip6_output.c +4 -3
@@ -1440,7 +1440,7 @@ static int __ip6_append_data(struct sock *sk,struct sk_buff *skb, *skb_prev =NULL ;struct inet_cork *cork = &cork_full->base;struct flowi6 *fl6 = &cork_full->fl.u.ip6; -unsigned int maxfraglen, fragheaderlen, mtu, orig_mtu, pmtu; +unsigned int maxfraglen, fragheaderlen, mtu, orig_mtu;struct ubuf_info *uarg =NULL ;int exthdrlen =0 ;int dst_exthdrlen =0 ;
@@ -1504,9 +1504,10 @@ static int __ip6_append_data(struct sock *sk, maxnonfragsize = mtu;if (cork->length + length > maxnonfragsize - headersize) { +int pmtu; emsgsize: - pmtu =max_t (int, mtu - headersize +sizeof (struct ipv6hdr ),0 ); -ipv6_local_error (sk,EMSGSIZE , fl6, pmtu); + pmtu = mtu - headersize + (int )sizeof (struct ipv6hdr ); +ipv6_local_error (sk,EMSGSIZE , fl6,max (pmtu,0 ));return -EMSGSIZE ; }
Signature · 1 line
2.39.5
[email protected]
AI reviewRe: [PATCH 09/44] ipv6: __ip6_append_data() don't abuse max_t() casts
Can the variable pmtu be used uninitialized here?
Quoted 10 lines
net/ipv6/ip6_output.c +1
-1@@ -1440,7 +1440,7 @@ static int __ip6_append_data(struct sock *sk,struct sk_buff *skb, *skb_prev =NULL ;struct inet_cork *cork = &cork_full->base;struct flowi6 *fl6 = &cork_full->fl.u.ip6; -unsigned int maxfraglen, fragheaderlen, mtu, orig_mtu, pmtu; +unsigned int maxfraglen, fragheaderlen, mtu, orig_mtu;
[ ... ]
net/ipv6/ip6_output.c static int __ip6_append_data(struct sock *sk, · line 1512
Quoted 12 lines
net/ipv6/ip6_output.c +3
-2@@ -1504,9 +1504,10 @@ static int __ip6_append_data(struct sock *sk, maxnonfragsize = mtu;if (cork->length + length > maxnonfragsize - headersize) { +int pmtu; emsgsize: - pmtu =max_t (int, mtu - headersize +sizeof (struct ipv6hdr ),0 ); -ipv6_local_error (sk,EMSGSIZE , fl6, pmtu); + pmtu = mtu - headersize + (int )sizeof (struct ipv6hdr ); +ipv6_local_error (sk,EMSGSIZE , fl6,max (pmtu,0 ));return -EMSGSIZE ; }
Can the variable pmtu be used uninitialized here?
When reaching the emsgsize label via goto statements from earlier in __ip6_append_data() (the checks at lines around mtu <= fragheaderlen, headersize + transhdrlen > mtu, and the dontfrag check), the int pmtu declaration is skipped. This leaves pmtu uninitialized when it's used in the calculation that's passed to ipv6_local_error().
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19519124312
Re: [PATCH 09/44] ipv6: __ip6_append_data() don't abuse max_t() casts
Quoted 16 lines · On Thu, 20 Nov 2025 00:32:34 +0000 (UTC) [email protected] wrote:
Quoted 10 lines
net/ipv6/ip6_output.c +1
-1@@ -1440,7 +1440,7 @@ static int __ip6_append_data(struct sock *sk,struct sk_buff *skb, *skb_prev =NULL ;struct inet_cork *cork = &cork_full->base;struct flowi6 *fl6 = &cork_full->fl.u.ip6; -unsigned int maxfraglen, fragheaderlen, mtu, orig_mtu, pmtu; +unsigned int maxfraglen, fragheaderlen, mtu, orig_mtu;[ ... ]
Quoted 3 lines
net/ipv6/ip6_output.c +0
-0@@ -1504,9 +1504,10 @@ static int __ip6_append_data(struct sock *sk, maxnonfragsize = mtu;
Quoted 11 lines
net/ipv6/ip6_output.c static int __ip6_append_data(struct sock *sk, · line 1512
Quoted 9 lines
net/ipv6/ip6_output.c +3
-2if (cork->length + length > maxnonfragsize - headersize) { +int pmtu; emsgsize: - pmtu =max_t (int, mtu - headersize +sizeof (struct ipv6hdr ),0 ); -ipv6_local_error (sk,EMSGSIZE , fl6, pmtu); + pmtu = mtu - headersize + (int )sizeof (struct ipv6hdr ); +ipv6_local_error (sk,EMSGSIZE , fl6,max (pmtu,0 ));return -EMSGSIZE ; }Can the variable pmtu be used uninitialized here?
If your AI bot doesn't understand code don't run it.
David
Quoted 13 lines
When reaching the emsgsize label via goto statements from earlier in
__ip6_append_data()(the checks at lines around mtu <= fragheaderlen, headersize + transhdrlen > mtu, and the dontfrag check), the int pmtu declaration is skipped. This leaves pmtu uninitialized when it's used in the calculation that's passed toipv6_local_error().---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.mdCI run summary: https://github.com/kernel-patches/bpf/actions/runs/19519124312
Re: [PATCH 09/44] ipv6: __ip6_append_data() don't abuse max_t() casts
Quoted 31 lines · David Laight
Quoted 16 lines · On Thu, 20 Nov 2025 00:32:34 +0000 (UTC) [email protected] wrote:
Quoted 10 lines
net/ipv6/ip6_output.c +1
-1@@ -1440,7 +1440,7 @@ static int __ip6_append_data(struct sock *sk,struct sk_buff *skb, *skb_prev =NULL ;struct inet_cork *cork = &cork_full->base;struct flowi6 *fl6 = &cork_full->fl.u.ip6; -unsigned int maxfraglen, fragheaderlen, mtu, orig_mtu, pmtu; +unsigned int maxfraglen, fragheaderlen, mtu, orig_mtu;[ ... ]
Quoted 3 lines
net/ipv6/ip6_output.c +0
-0@@ -1504,9 +1504,10 @@ static int __ip6_append_data(struct sock *sk, maxnonfragsize = mtu;Quoted 11 lines
net/ipv6/ip6_output.c static int __ip6_append_data(struct sock *sk, · line 1512
Quoted 9 lines
net/ipv6/ip6_output.c +3
-2if (cork->length + length > maxnonfragsize - headersize) { +int pmtu; emsgsize: - pmtu =max_t (int, mtu - headersize +sizeof (struct ipv6hdr ),0 ); -ipv6_local_error (sk,EMSGSIZE , fl6, pmtu); + pmtu = mtu - headersize + (int )sizeof (struct ipv6hdr ); +ipv6_local_error (sk,EMSGSIZE , fl6,max (pmtu,0 ));return -EMSGSIZE ; }Can the variable pmtu be used uninitialized here?
Quoted 1 line
If your AI bot doesn't understand code don't run it.
I ran this locally three more times and this false positive didn't reproduce, but I'll see if the CI has enough logs to figure out where it got confused.
Regardless, I'm doing periodic checks for patterns of false positives and fine tuning the prompts.
-chris
PATCH 10/44 x86/crypto: ctr_crypt() use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- arch/x86/crypto/aesni-intel_glue.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
arch/x86/crypto/aesni-intel_glue.c +1 -2
@@ -693,8 +693,7 @@ ctr_crypt(struct skcipher_request *req,* operation into two at the point where the overflow * will occur. After the first part, add the carry bit. */ - p1_nbytes = min_t(unsigned int, nbytes, - (nblocks - ctr64) *AES_BLOCK_SIZE ); + p1_nbytes =min (nbytes, (nblocks - ctr64) *AES_BLOCK_SIZE ); (*ctr64_func)(key, walk.src.virt.addr, walk.dst.virt.addr, p1_nbytes, le_ctr); le_ctr[0 ] =0 ;
Signature · 1 line
2.39.5
PATCH 11/44 arch/x96/kvm: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
(Similarly for max_t() and clamp_t().)
Use min3() in __do_insn_fetch_bytes().
Detected by an extra check added to min_t().
--- arch/x86/kvm/emulate.c | 3 +-- arch/x86/kvm/lapic.c | 2 +- arch/x86/kvm/mmu/mmu.c | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-)
arch/x86/kvm/emulate.c +1 -2
@@ -861,8 +861,7 @@ static int __do_insn_fetch_bytes(struct x86_emulate_ctxt *ctxt, int op_size)if (unlikely (rc !=X86EMUL_CONTINUE ))return rc; - size =min_t (unsigned,15UL ^ cur_size, max_size); - size =min_t (unsigned, size,PAGE_SIZE -offset_in_page (linear)); + size =min3 (15U ^ cur_size, max_size,PAGE_SIZE -offset_in_page (linear));/* * One instruction can only straddle two pages,
arch/x86/kvm/lapic.c +1 -1
@@ -1894,7 +1894,7 @@ static inline void __wait_lapic_expire(struct kvm_vcpu *vcpu, u64 guest_cycles) }else {u64 delay_ns = guest_cycles *1000000ULL ;do_div (delay_ns, vcpu->arch.virtual_tsc_khz); -ndelay (min_t (u32, delay_ns, timer_advance_ns)); +ndelay (min (delay_ns, timer_advance_ns)); } }
arch/x86/kvm/mmu/mmu.c +1 -1
@@ -5768,7 +5768,7 @@ static void kvm_init_shadow_mmu(struct kvm_vcpu *vcpu, root_role = cpu_role.base;/* KVM uses PAE paging whenever the guest isn't using 64-bit paging. */ - root_role.level =max_t (u32, root_role.level,PT32E_ROOT_LEVEL ); + root_role.level =max (root_role.level +0 ,PT32E_ROOT_LEVEL );/* * KVM forces EFER.NX=1 when TDP is disabled, reflect it in the MMU role.
Signature · 1 line
2.39.5
PATCH 12/44 block: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
(Similarly for max_t() and clamp_t().)
Detected by an extra check added to min_t().
--- block/blk-iocost.c | 6 ++---- block/blk-settings.c | 2 +- block/partitions/efi.c | 3 +-- 3 files changed, 4 insertions(+), 7 deletions(-)
block/blk-iocost.c +2 -4
@@ -2334,10 +2334,8 @@ static void ioc_timer_fn(struct timer_list *timer)else usage_dur =max_t (u64, now.now - ioc->period_at,1 ); - usage =clamp_t (u32, -DIV64_U64_ROUND_UP (usage_us *WEIGHT_ONE , - usage_dur), -1 ,WEIGHT_ONE ); + usage =clamp (DIV64_U64_ROUND_UP (usage_us *WEIGHT_ONE , usage_dur), +1 ,WEIGHT_ONE );/* * Already donating or accumulated enough to start.
block/blk-settings.c +1 -1
@@ -472,7 +472,7 @@ int blk_validate_limits(struct queue_limits *lim) seg_size = lim->max_segment_size; else seg_size = lim->seg_boundary_mask +1 ; - lim->min_segment_size = min_t(unsigned int, seg_size, PAGE_SIZE); + lim->min_segment_size =min (seg_size,PAGE_SIZE );/* * We require drivers to at least do logical block aligned I/O, but
block/partitions/efi.c +1 -2
@@ -215,8 +215,7 @@ static int is_pmbr_valid(legacy_mbr *mbr, sector_t total_sectors) sz =le32_to_cpu (mbr->partition_record[part].size_in_lba);if (sz != (uint32_t ) total_sectors -1 && sz !=0xFFFFFFFF )pr_debug ("GPT: mbr size in lba (%u) different than whole disk (%u).\n" , - sz,min_t (uint32_t, - total_sectors -1 ,0xFFFFFFFF )); + sz, (uint32_t )min (total_sectors -1 ,0xFFFFFFFF )); } done:return ret;
Signature · 1 line
2.39.5
Jens Axboe
maintainerRe: [PATCH 12/44] block: use min() instead of min_t()
block/blk-settings.c int blk_validate_limits(struct queue_limits *lim) · line 478
Quoted 13 lines · On 11/19/25 3:41 PM, [email protected] wrote:
block/blk-settings.c +1
-1@@ -472,7 +472,7 @@ int blk_validate_limits(struct queue_limits *lim) seg_size = lim->max_segment_size; else seg_size = lim->seg_boundary_mask +1 ; - lim->min_segment_size = min_t(unsigned int, seg_size, PAGE_SIZE); + lim->min_segment_size =min (seg_size,PAGE_SIZE );/* * We require drivers to at least do logical block aligned I/O, but
This doesn't exist in the 6.19 branch, dropped.
Signature · 1 line
Jens Axboe
PATCH 13/44 drivers/acpi: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- drivers/acpi/property.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/acpi/property.c +1 -1
@@ -1280,7 +1280,7 @@ static int acpi_data_prop_read(const struct acpi_device_data *data, ret =acpi_copy_property_array_uint (items, (u64 *)val, nval);break ;case DEV_PROP_STRING : - nval =min_t (u32, nval, obj->package.count); + nval =min (nval, obj->package.count);if (nval ==0 )return -ENODATA ;
Signature · 1 line
2.39.5
Re: [PATCH 13/44] drivers/acpi: use min() instead of min_t()
Quoted 22 lines · On Wed, Nov 19, 2025 at 11:42 PM <[email protected]> wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to
min_t().Signed-off-by David Laight <[email protected]>--- drivers/acpi/property.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/acpi/property.c +0
-0
drivers/acpi/property.c static int acpi_data_prop_read(const struct acpi_device_data *data,
Quoted 9 lines
drivers/acpi/property.c +1
-2@@ -1280,7 +1280,7 @@ static int acpi_data_prop_read(const struct acpi_device_data *data, ret =acpi_copy_property_array_uint (items, (u64 *)val, nval);break ;case DEV_PROP_STRING : - nval =min_t (u32, nval, obj->package.count); + nval =min (nval, obj->package.count);if (nval ==0 )return -ENODATA ; --
Applied as 6.19 material with adjusted subject, thanks!
PATCH 14/44 drivers/char/hw_random: use min3() instead of nested min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(u16, a, b) is likely to discard significant bits. Replace:
min_t(u16, min_t(u16, default_quality, 1024), rng->quality ?: 1024); with: min3(default_quality, 1024, rng->quality ?: 1024);
--- drivers/char/hw_random/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/char/hw_random/core.c +1 -1
@@ -542,7 +542,7 @@ int hwrng_register(struct hwrng *rng)init_completion (&rng->dying);/* Adjust quality field to always have a proper value */ - rng->quality =min_t (u16,min_t (u16, default_quality,1024 ), rng->quality ?:1024 ); + rng->quality =min3 (default_quality,1024 , rng->quality ?:1024 );if (!current_rng || (!cur_rng_set_by_user && rng->quality > current_rng->quality)) {
Signature · 1 line
2.39.5
PATCH 15/44 drivers/char/tpm: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(int, a, b) casts an 'long' to 'int'.
Use min(a, b) instead as it promotes any 'int' to 'long' and so cannot discard significant bits.
In this case the 'long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- drivers/char/tpm/tpm1-cmd.c | 2 +- drivers/char/tpm/tpm_tis_core.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
drivers/char/tpm/tpm1-cmd.c +1 -1
@@ -530,7 +530,7 @@ struct tpm1_get_random_out {int tpm1_get_random (struct tpm_chip *chip,u8 *dest,size_t max) {struct tpm1_get_random_out *out; -u32 num_bytes =min_t (u32, max,TPM_MAX_RNG_DATA ); +u32 num_bytes =min (max,TPM_MAX_RNG_DATA );struct tpm_buf buf;u32 total =0 ;int retries =5 ;
drivers/char/tpm/tpm_tis_core.c +2 -2
@@ -310,7 +310,7 @@ static int get_burstcount(struct tpm_chip *chip)return -EBUSY ; } -static int recv_data (struct tpm_chip *chip,u8 *buf,size_t count) +static int recv_data (struct tpm_chip *chip,u8 *buf,u32 count) {struct tpm_tis_data *priv =dev_get_drvdata (&chip->dev);int size =0 , burstcnt, rc;
@@ -453,7 +453,7 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len) rc = burstcnt; goto out_err; } - burstcnt =min_t (int, burstcnt, len - count -1 ); + burstcnt =min (burstcnt, len - count -1 ); rc =tpm_tis_write_bytes (priv,TPM_DATA_FIFO (priv->locality), burstcnt, buf + count);if (rc <0 )
Signature · 1 line
2.39.5
Re: [PATCH 15/44] drivers/char/tpm: use min() instead of min_t()
Quoted 54 lines · On Wed, Nov 19, 2025 at 10:41:11PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
min_t(int, a, b)casts an 'long' to 'int'.
Usemin(a, b)instead as it promotes any 'int' to 'long' and so cannot discard significant bits.In this case the 'long' value is small enough that the result is ok.
Detected by an extra check added to
min_t().Signed-off-by David Laight <[email protected]>--- drivers/char/tpm/tpm1-cmd.c | 2 +- drivers/char/tpm/tpm_tis_core.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)drivers/char/tpm/tpm1-cmd.c +1
-1@@ -530,7 +530,7 @@ struct tpm1_get_random_out {int tpm1_get_random (struct tpm_chip *chip,u8 *dest,size_t max) {struct tpm1_get_random_out *out; -u32 num_bytes =min_t (u32, max,TPM_MAX_RNG_DATA ); +u32 num_bytes =min (max,TPM_MAX_RNG_DATA );struct tpm_buf buf;u32 total =0 ;int retries =5 ;drivers/char/tpm/tpm_tis_core.c +2
-2@@ -310,7 +310,7 @@ static int get_burstcount(struct tpm_chip *chip)return -EBUSY ; } -static int recv_data (struct tpm_chip *chip,u8 *buf,size_t count) +static int recv_data (struct tpm_chip *chip,u8 *buf,u32 count) {struct tpm_tis_data *priv =dev_get_drvdata (&chip->dev);int size =0 , burstcnt, rc;@@ -453,7 +453,7 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len) rc = burstcnt; goto out_err; } - burstcnt =min_t (int, burstcnt, len - count -1 ); + burstcnt =min (burstcnt, len - count -1 ); rc =tpm_tis_write_bytes (priv,TPM_DATA_FIFO (priv->locality), burstcnt, buf + count);if (rc <0 )Signature · 1 line
2.39.5
BR, Jarkko
PATCH 16/44 drivers/crypto/ccp: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- drivers/crypto/ccp/ccp-dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/crypto/ccp/ccp-dev.c +1 -1
@@ -507,7 +507,7 @@ int ccp_trng_read(struct hwrng *rng, void *data, size_t max, bool wait) { struct ccp_device *ccp = container_of(rng, struct ccp_device, hwrng);u32 trng_value; -int len =min_t (int,sizeof (trng_value), max); +int len =min (sizeof (trng_value), max);/* Locking is provided by the caller so we can update device * hwrng-related fields safely
Signature · 1 line
2.39.5
PATCH 17/44 drivers/cxl: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- drivers/cxl/core/mbox.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/cxl/core/mbox.c +1 -1
@@ -663,7 +663,7 @@ static int cxl_xfer_log(struct cxl_memdev_state *mds, uuid_t *uuid,u32 offset =0 ;while (remaining) { -u32 xfer_size =min_t (u32, remaining, cxl_mbox->payload_size); +u32 xfer_size =min (remaining, cxl_mbox->payload_size);struct cxl_mbox_cmd mbox_cmd;struct cxl_mbox_get_log log;int rc;
Signature · 1 line
2.39.5
Re: [PATCH 17/44] drivers/cxl: use min() instead of min_t()
Quoted 6 lines · On 11/19/25 3:41 PM, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
Quoted 6 lines
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to
min_t().Signed-off-by David Laight <[email protected]>
Quoted 17 lines
--- drivers/cxl/core/mbox.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/cxl/core/mbox.c +1
-1@@ -663,7 +663,7 @@ static int cxl_xfer_log(struct cxl_memdev_state *mds, uuid_t *uuid,u32 offset =0 ;while (remaining) { -u32 xfer_size =min_t (u32, remaining, cxl_mbox->payload_size); +u32 xfer_size =min (remaining, cxl_mbox->payload_size);struct cxl_mbox_cmd mbox_cmd;struct cxl_mbox_get_log log;int rc;
PATCH 18/44 drivers/gpio: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(u16, a, b) casts an 'unsigned long' to 'u16'. Use min(a, b) instead as it promotes the both values to int and so cannot discard significant bits.
In this case the values should be ok.
Detected by an extra check added to min_t().
--- drivers/gpio/gpiolib-acpi-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/gpiolib-acpi-core.c +1 -1
@@ -1099,7 +1099,7 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,return AE_BAD_PARAMETER ; } - length =min_t (u16, agpio->pin_table_length, pin_index + bits); + length =min (agpio->pin_table_length, pin_index + bits);for (i = pin_index; i < length; ++i) {unsigned int pin = agpio->pin_table[i];struct acpi_gpio_connection *conn;
Signature · 1 line
2.39.5
Andy Shevchenko
maintainerRe: [PATCH 18/44] drivers/gpio: use min() instead of min_t()
Quoted 8 lines · On Wed, Nov 19, 2025 at 10:41:14PM +0000, [email protected] wrote:
min_t(u16, a, b)casts an 'unsigned long' to 'u16'. Usemin(a, b)instead as it promotes the both values to int and so cannot discard significant bits.In this case the values should be ok.
Detected by an extra check added to
min_t().
In most of the patches you need to follow the commonly used Subject prefix. This can be done by doing
git log --oneline --no-merges -- $FILE(S)_OF_INTEREST
For example,
$ git log --no-decorate --oneline --no-merges -- drivers/gpio/gpiolib-acpi* b1055678a016 gpiolib: acpi: Use %pe when passing an error pointer to dev_err() e4a77f9c85a5 gpiolib: acpi: Make set debounce errors non fatal 19c839a98c73 gpiolib: acpi: initialize acpi_gpio_info struct 3712ce9fa501 gpiolib: acpi: Ignore touchpad wakeup on GPD G1619-05 16c07342b542 gpiolib: acpi: Program debounce when finding GPIO 23800ad1265f gpiolib: acpi: Add quirk for ASUS ProArt PX13 ...
drivers/gpio/gpiolib-acpi-core.c acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address, · line 1102
Quoted 4 lines
acpi_gpio_adr_space_handler(u32 function,
acpi_physical_addressaddress,drivers/gpio/gpiolib-acpi-core.c +1
-1- length =min_t (u16, agpio->pin_table_length, pin_index + bits); + length =min (agpio->pin_table_length, pin_index + bits);
Now, if you look closer at the code, the pin_index alone has the problem you are targeting here. On top of that the iterator and 'length' are signed, while the result of min_t(u16) is unsigned (however it has no difference in this case).
...
TL;DR: I apply this patch with subject changed, but I think more work needs to be done if you want to fix it fully.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH 18/44] drivers/gpio: use min() instead of min_t()
Quoted 6 lines · On Thu, 20 Nov 2025 10:01:29 +0200 Andy Shevchenko…
Quoted 5 lines · On Wed, Nov 19, 2025 at 10:41:14PM +0000, [email protected] wrote:
min_t(u16, a, b)casts an 'unsigned long' to 'u16'. Usemin(a, b)instead as it promotes the both values to int and so cannot discard significant bits.
Quoted 6 lines
Quoted 3 lines
In this case the values should be ok.
Detected by an extra check added to
min_t().In most of the patches you need to follow the commonly used Subject prefix. This can be done by doing
I did look up quite a few files to see what had been used previously. But it is a bit tedious with 44 patches.
Quoted 1 line
git log --oneline --no-merges -- $FILE(S)_OF_INTEREST
I wasn't aware of that spell :-)
...
Quoted 8 lines
drivers/gpio/gpiolib-acpi-core.c acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address, · line 1102
Quoted 5 lines
acpi_gpio_adr_space_handler(u32 function,
acpi_physical_addressaddress,drivers/gpio/gpiolib-acpi-core.c +1
-1- length =min_t (u16, agpio->pin_table_length, pin_index + bits); + length =min (agpio->pin_table_length, pin_index + bits);Now, if you look closer at the code, the
pin_indexalone has the problem you are targeting here.
The compiler warning happens because 'pin_index + bits' is 'int' and the compiler doesn't know the value fits in 16 bits.
It should fit, but only if the caller passes in valid data.
Quoted 2 lines
On top of that the iterator and 'length' are signed, while
the result ofmin_t(u16)is unsigned (however it has no difference in this case).
Actually the result type of min_t(u16) is 'int' (:? promotes char/short to int). So the u16 cast does '(pin_index + bits) & 0xffff', everything is then promoted to 'int' for all the comparisons (etc).
David
Quoted 6 lines
...
TL;DR: I apply this patch with subject changed, but I think more work needs to be done if you want to fix it fully.
Andy Shevchenko
maintainerRe: [PATCH 18/44] drivers/gpio: use min() instead of min_t()
Quoted 11 lines · David Laight
Quoted 9 lines · On Thu, 20 Nov 2025 10:01:29 +0200 Andy Shevchenko…
Quoted 8 lines · On Wed, Nov 19, 2025 at 10:41:14PM +0000, [email protected] wrote:
min_t(u16, a, b)casts an 'unsigned long' to 'u16'. Usemin(a, b)instead as it promotes the both values to int and so cannot discard significant bits.In this case the values should be ok.
Detected by an extra check added to
min_t().
...
Quoted 11 lines
Quoted 7 lines
drivers/gpio/gpiolib-acpi-core.c acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address, · line 1102
Quoted 4 lines
acpi_gpio_adr_space_handler(u32 function,
acpi_physical_addressaddress,drivers/gpio/gpiolib-acpi-core.c +1
-1- length =min_t (u16, agpio->pin_table_length, pin_index + bits); + length =min (agpio->pin_table_length, pin_index + bits);Now, if you look closer at the code, the
pin_indexalone has the problem you are targeting here.The compiler warning happens because '
pin_index+ bits' is 'int' and the compiler doesn't know the value fits in 16 bits.
It should fit, but only if the caller passes in valid data.
I meant that assignment to pin_index already cuts the higher bits from the input.
Quoted 6 lines
Quoted 2 lines
On top of that the iterator and 'length' are signed, while
the result ofmin_t(u16)is unsigned (however it has no difference in this case).Actually the result type of
min_t(u16)is 'int' (:? promotes char/short to int). So the u16 cast does '(pin_index+ bits) &0xffff', everything is then promoted to 'int' for all the comparisons (etc).
Sure, but the value is positive even if int is signed. That's why I put a remark in the parentheses that it has no difference in this case.
...
Quoted 2 lines
Quoted 2 lines
TL;DR: I apply this patch with subject changed, but I think more work needs to be done if you want to fix it fully.
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH 19/44 drivers/gpu/drm/amd: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
min_t(u8, a, b) is particularly likely to be problematic
In this case I think the values are small enough that the result is ok.
Detected by an extra check added to min_t().
--- drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c | 4 ++-- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c +2 -2
@@ -211,8 +211,8 @@ int amdgpu_doorbell_init(struct amdgpu_device *adev) adev->doorbell.size =pci_resource_len (adev->pdev,2 ); adev->doorbell.num_kernel_doorbells = -min_t (u32, adev->doorbell.size /sizeof (u32), - adev->doorbell_index.max_assignment +1 ); +min (adev->doorbell.size /sizeof (u32), + adev->doorbell_index.max_assignment +1 );if (adev->doorbell.num_kernel_doorbells ==0 )return -EINVAL ;
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +1 -1
@@ -2388,7 +2388,7 @@ void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t min_vm_size,if (amdgpu_vm_block_size !=-1 ) tmp >>= amdgpu_vm_block_size -9 ; tmp =DIV_ROUND_UP (fls64 (tmp) -1 ,9 ) -1 ; - adev->vm_manager.num_level = min_t(unsigned int, max_level, tmp); + adev->vm_manager.num_level =min (max_level, tmp);switch (adev->vm_manager.num_level) {case 3 : adev->vm_manager.root_level =AMDGPU_VM_PDB2 ;
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +1 -1
@@ -6257,7 +6257,7 @@ convert_color_depth_from_display_info(const struct drm_connector *connector,* or if this was called outside of atomic check, so it * can't be used directly. */ - bpc =min_t (u8, bpc, requested_bpc); + bpc =umin (bpc, requested_bpc);/* Round down to the nearest even number. */ bpc = bpc - (bpc &1 );
Signature · 1 line
2.39.5
PATCH 20/44 drivers/i2c/busses: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(u8, a, b) casts both its arguments to u8 potentially discarding signifinact bits.
Use min(a, b) instead as it cannot discard significant bits.
Detected by an extra check added to min_t().
--- drivers/i2c/busses/i2c-designware-master.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-designware-master.c +1 -1
@@ -564,7 +564,7 @@ i2c_dw_recv_len(struct dw_i2c_dev *dev, u8 len)* after receiving the first byte. */ len += (flags &I2C_CLIENT_PEC ) ?2 :1 ; - dev->tx_buf_len = len -min_t (u8, len, dev->rx_outstanding); + dev->tx_buf_len = len -min (len, dev->rx_outstanding); msgs[dev->msg_read_idx].len = len; msgs[dev->msg_read_idx].flags &= ~I2C_M_RECV_LEN ;
Signature · 1 line
2.39.5
Re: [PATCH 20/44] drivers/i2c/busses: use min() instead of min_t()
Hi David,
Quoted 9 lines · On Wed, Nov 19, 2025 at 10:41:16PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
min_t(u8, a, b)casts both its arguments to u8 potentially discarding signifinact bits.
Usemin(a, b)instead as it cannot discard significant bits.Detected by an extra check added to
min_t().Signed-off-by David Laight <[email protected]>
merged to i2c/i2c-host.
Thanks,
Andi
PATCH 21/44 drivers/net/ethernet/realtek: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- drivers/net/ethernet/realtek/r8169_main.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/net/ethernet/realtek/r8169_main.c +1 -2
@@ -4238,8 +4238,7 @@ static unsigned int rtl8125_quirk_udp_padto(struct rtl8169_private *tp, }if (trans_data_len <sizeof (struct udphdr )) - padto = max_t(unsigned int, padto, - len +sizeof (struct udphdr ) - trans_data_len); + padto =max (padto, len +sizeof (struct udphdr ) - trans_data_len); }return padto;
Signature · 1 line
2.39.5
PATCH 22/44 drivers/nvme: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- drivers/nvme/host/pci.c | 3 +-- drivers/nvme/host/zns.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-)
drivers/nvme/host/pci.c +1 -2
@@ -3412,8 +3412,7 @@ static struct nvme_dev *nvme_pci_alloc_dev(struct pci_dev *pdev,* Limit the max command size to prevent iod->sg allocations going * over a single page. */ - dev->ctrl.max_hw_sectors =min_t (u32, -NVME_MAX_BYTES >>SECTOR_SHIFT , + dev->ctrl.max_hw_sectors =min (NVME_MAX_BYTES >>SECTOR_SHIFT ,dma_opt_mapping_size (&pdev->dev) >>9 ); dev->ctrl.max_segments =NVME_MAX_SEGS ; dev->ctrl.max_integrity_segments =1 ;
drivers/nvme/host/zns.c +1 -2
@@ -126,8 +126,7 @@ static void *nvme_zns_alloc_report_buffer(struct nvme_ns *ns,const size_t min_bufsize =sizeof (struct nvme_zone_report ) +sizeof (struct nvme_zone_descriptor ); - nr_zones = min_t(unsigned int, nr_zones, -get_capacity (ns->disk) >>ilog2 (ns->head->zsze)); + nr_zones =min (nr_zones,get_capacity (ns->disk) >>ilog2 (ns->head->zsze)); bufsize =sizeof (struct nvme_zone_report ) + nr_zones *sizeof (struct nvme_zone_descriptor );
Signature · 1 line
2.39.5
PATCH 23/44 arch/x86/mm: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- arch/x86/mm/pat/set_memory.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
arch/x86/mm/pat/set_memory.c +6 -6
@@ -1459,7 +1459,7 @@ static void unmap_pmd_range(pud_t *pud, unsigned long start, unsigned long end)*/ if (start & (PMD_SIZE -1 )) {unsigned long next_page = (start +PMD_SIZE ) &PMD_MASK ; - unsigned long pre_end = min_t(unsigned long, end, next_page); +unsigned long pre_end =min (end, next_page);__unmap_pmd_range (pud, pmd, start, pre_end);
@@ -1503,7 +1503,7 @@ static void unmap_pud_range(p4d_t *p4d, unsigned long start, unsigned long end)*/ if (start & (PUD_SIZE -1 )) {unsigned long next_page = (start +PUD_SIZE ) &PUD_MASK ; - unsigned long pre_end = min_t(unsigned long, end, next_page); +unsigned long pre_end =min (end, next_page);unmap_pmd_range (pud, start, pre_end);
@@ -1591,9 +1591,9 @@ static long populate_pmd(struct cpa_data *cpa,unsigned long pre_end = start + (num_pages <<PAGE_SHIFT );unsigned long next_page = (start +PMD_SIZE ) &PMD_MASK ; - pre_end = min_t(unsigned long, pre_end, next_page); + pre_end =min (pre_end, next_page); cur_pages = (pre_end - start) >>PAGE_SHIFT ; - cur_pages = min_t(unsigned int, num_pages, cur_pages); + cur_pages =min (num_pages, cur_pages);/* * Need a PTE page?
@@ -1668,9 +1668,9 @@ static int populate_pud(struct cpa_data *cpa, unsigned long start, p4d_t *p4d,unsigned long pre_end;unsigned long next_page = (start +PUD_SIZE ) &PUD_MASK ; - pre_end = min_t(unsigned long, end, next_page); + pre_end =min (end, next_page); cur_pages = (pre_end - start) >>PAGE_SHIFT ; - cur_pages =min_t (int, (int )cpa->numpages, cur_pages); + cur_pages =min (cpa->numpages, cur_pages); pud =pud_offset (p4d, start);
Signature · 1 line
2.39.5
PATCH 24/44 drivers/nvmem: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- drivers/nvmem/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvmem/core.c +1 -1
@@ -359,7 +359,7 @@ static ssize_t nvmem_cell_attr_read(struct file *filp, struct kobject *kobj, goto destroy_cell; } - read_len = min_t(unsigned int, cell_sz - pos, count); + read_len =min (cell_sz - pos, count);memcpy (buf, content + pos, read_len);kfree (content);
Signature · 1 line
2.39.5
PATCH 25/44 drivers/pci: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case although pci_hotplug_bus_size is 'long' it is constrained to be <= 255.
Detected by an extra check added to min_t().
--- drivers/pci/probe.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/pci/probe.c +1 -2
@@ -3163,8 +3163,7 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,* bus number if there is room. */ if (bus->self && bus->self->is_hotplug_bridge) { - used_buses = max_t(unsigned int, available_buses, - pci_hotplug_bus_size -1 ); + used_buses =max (available_buses, pci_hotplug_bus_size -1 );if (max - start < used_buses) { max = start + used_buses;
Signature · 1 line
2.39.5
Bjorn Helgaas
AppliedRe: [PATCH 25/44] drivers/pci: use min() instead of min_t()
Quoted 10 lines · On Wed, Nov 19, 2025 at 10:41:21PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.In this case although
pci_hotplug_bus_sizeis 'long' it is constrained to be <= 255.Detected by an extra check added to
min_t().
I don't mind applying this, but it sure would be nice to have a hint at the max() and max_t() definitions about when and why to prefer one over the other.
Applied to pci/misc for v6.19 with the following commit log:
PCI: Use max() instead of max_t() to ease static analysis
In this code:
used_buses = max_t(unsigned int, available_buses,
pci_hotplug_bus_size - 1);
max_t() casts the 'unsigned long' pci_hotplug_bus_size (either 32 or 64 bits) to 'unsigned int' (32 bits) result type, so there's a potential of discarding significant bits.
Instead, use max(a, b), which casts 'unsigned int' to 'unsigned long' and cannot discard significant bits.
In this case, pci_hotplug_bus_size is constrained to <= 0xff by pci_setup() so this doesn't fix a bug, but it makes static analysis easier.
Quoted 22 lines
Signed-off-by David Laight <[email protected]>--- drivers/pci/probe.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/pci/probe.c +1
-2@@ -3163,8 +3163,7 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,* bus number if there is room. */ if (bus->self && bus->self->is_hotplug_bridge) { - used_buses = max_t(unsigned int, available_buses, - pci_hotplug_bus_size -1 ); + used_buses =max (available_buses, pci_hotplug_bus_size -1 );if (max - start < used_buses) { max = start + used_buses;Signature · 1 line
2.39.5
Re: [PATCH 25/44] drivers/pci: use min() instead of min_t()
Quoted 7 lines · On Mon, 24 Nov 2025 15:04:10 -0600 Bjorn Helgaas <[email protected]> wrote:
Quoted 6 lines · On Wed, Nov 19, 2025 at 10:41:21PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
Quoted 8 lines
Quoted 4 lines
In this case although
pci_hotplug_bus_sizeis 'long' it is constrained to be <= 255.Detected by an extra check added to
min_t().I don't mind applying this, but it sure would be nice to have a hint at the
max()andmax_t()definitions about when and why to prefer one over the other.
When I've sorted out local commits for all the other 400 files I've had to change to get allmodconfig to build I'm going to look at minmax.h and checkpatch.pl.
The current bit in checkpatch that suggests transforming
min(a, (type)b) => min_t(type, a, b)
isn't even a good idea.
The latter is min((type)a, (type)b) - which isn't the same. at least in with min(a, (type)b)) the truncation is obvious.
I think I'll try to get checkpatch to reject min_t(u8|s8|u16|s16, ... outright - remember the values get promoted the 'int'. Unless the code is doing something really obscure they can all be replaced with a plain min().
Then get minmax.h to reject u8 casts when one of the values is 255 (and u16 casts with 65535) - particularly for clamp_t(). That will error a small number of files - but only a handful.
I need to find the #if that is set for a W=1 build so that I can 'error' more of the 'dodgy' cases.
That might include all the u8|s8|u16|s16 ones.
But I'll have to leave all the u32 casts of u64 values (on 64bit) for later (they are typically u32 and size_t).
I've not yet looked for u32 casts of u64 values (long v long long) on 32bit. I suspect there are some real bugs there.
Maybe I'll try to get them into the W=2 build no one does :-)
I'm retired, need to find something to do ...
(apart from getting to PoGo level 80)
David
PATCH 26/44 drivers/scsi: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- drivers/scsi/hosts.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/hosts.c +1 -1
@@ -247,7 +247,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev, shost->dma_dev = dma_dev;if (dma_dev->dma_mask) { - shost->max_sectors = min_t(unsigned int, shost->max_sectors, + shost->max_sectors =min (shost->max_sectors,dma_max_mapping_size (dma_dev) >>SECTOR_SHIFT ); }
Signature · 1 line
2.39.5
Re: [PATCH 26/44] drivers/scsi: use min() instead of min_t()
Quoted 21 lines · On 11/19/25 2:41 PM, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to
min_t().Signed-off-by David Laight <[email protected]>--- drivers/scsi/hosts.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/scsi/hosts.c +0
-0
drivers/scsi/hosts.c int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev, · line 252
Quoted 7 lines
drivers/scsi/hosts.c +1
-1@@ -247,7 +247,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev, shost->dma_dev = dma_dev;if (dma_dev->dma_mask) { - shost->max_sectors = min_t(unsigned int, shost->max_sectors, + shost->max_sectors =min (shost->max_sectors,dma_max_mapping_size (dma_dev) >>SECTOR_SHIFT ); }
So instead of the type cast performed by min_t() potentially discarding bits, the assignment potentially discards bits. I'm not sure this is an improvement.
Thanks,
Bart.
Re: [PATCH 26/44] drivers/scsi: use min() instead of min_t()
Quoted 24 lines · On Wed, 19 Nov 2025 15:09:02 -0800 Bart Van Assche <[email protected]> wrote:
Quoted 23 lines · On 11/19/25 2:41 PM, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to
min_t().Signed-off-by David Laight <[email protected]>--- drivers/scsi/hosts.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/scsi/hosts.c +0
-0@@ -247,7 +247,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev, shost->dma_dev = dma_dev;
Quoted 9 lines
drivers/scsi/hosts.c int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev, · line 252
Quoted 5 lines
drivers/scsi/hosts.c +1
-1if (dma_dev->dma_mask) { - shost->max_sectors = min_t(unsigned int, shost->max_sectors, + shost->max_sectors =min (shost->max_sectors,dma_max_mapping_size (dma_dev) >>SECTOR_SHIFT ); }So instead of the type cast performed by
min_t()potentially discarding bits, the assignment potentially discards bits. I'm not sure this is an improvement.
The assignment cant discard bits because shost->max_sectors is also on the RHS. In any case you'd need a 2TB mapping size to get an error.
That probably cant happen for other reasons.
The patch is remove a false positive for a check added to min_t(T, a, b) that sizeof (b) <= sizeof (T).
David
Quoted 5 lines
Thanks,
Bart.
Re: [PATCH 26/44] drivers/scsi: use min() instead of min_t()
In reply to Bart Van AsscheQuoted 24 lines · On Wed, 19 Nov 2025 15:09:02 -0800 Bart Van Assche <[email protected]> wrote:
Quoted 23 lines · On 11/19/25 2:41 PM, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to
min_t().Signed-off-by David Laight <[email protected]>--- drivers/scsi/hosts.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/scsi/hosts.c +0
-0@@ -247,7 +247,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev, shost->dma_dev = dma_dev;
Quoted 9 lines
drivers/scsi/hosts.c int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev, · line 252
Quoted 5 lines
drivers/scsi/hosts.c +1
-1if (dma_dev->dma_mask) { - shost->max_sectors = min_t(unsigned int, shost->max_sectors, + shost->max_sectors =min (shost->max_sectors,dma_max_mapping_size (dma_dev) >>SECTOR_SHIFT ); }So instead of the type cast performed by
min_t()potentially discarding bits, the assignment potentially discards bits. I'm not sure this is an improvement.
In this case the assignment is fine - shost->max_sectors is on both sides, so the value can only go down.
The issue is that dma_max_mapping_size() returns a 64bit value. So if you have some magic high speed interface with a 2TB 'dma map' then 'dma_max_mapping_size(dma_dev) >> SECTOR_SHIFT' is '1u << 32' so is zero when cast to 'unsigned int'.
At that point things start going horribly wrong.
I don't think there are any such interfaces - so the bug this fixes can't happen.
OTOH the same test does pick up a lot (and I mean a lot [1]) of driver code that contains code like:ssize_t do_xxx(... size_t len)
{
unsigned int copied = 0, frag_len;
while (copied < len) {
frag_len = min_t(unsigned int, len, MAX_FRAG);
....
copied += frag_len;
len -= frag_len;
}
return copied;
}If you manage to request a transfer for 4G (or more) then it doesn't work. Now there might be a test earlier that stops that happening in a lot of places. But from the perspective of the function it isn't true.
(I suspect readv() with a single iov[] can generate a big buffer.) The compile-time test detects that (unsigned int)len may not equal len and it can be fixed by changing to min(len, MAX_FRAG);
In this case it might be a real bug.
Note that a read can be truncated after a few bytes - it is only the buffer size that needs to be massive.
David
[1] I've just built allmodconfig - 'only' 488 more files needed changing. A fair number of 'real bugs', a few false positives because PAGE_SIZE and sizeof() are 64bit, and a lot of dubious code.
By far the worst are all the min_t([u8|u16], ...) in many cases the code uses the type of the destination - so you get (eg):
u8_var = min(u8, value_32 / 2, 255);
Quoted 4 lines
Thanks,
Bart.
PATCH 27/44 drivers/tty/vt: use umin() instead of min_t(u16, ...) for row/col limits
In reply to [email protected]From: David Laight <[email protected]>
The row/column bounds (for a screen window box) are changed from 'offset one' to 'offset zero' and bound to the screen size using:
v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
This has the side effect of converting zero to the limit.
A check I'm adding to min_t() reports that (u16)(v->xs - 1) (etc) discards signiticant bits (because v->xs is promoted to 'int' before the addition).
If v->xs is zero (it comes from userspace) it converts -1 to 0xffff. This is then bounded to 'vc->vc_cols - 1' which will be fine.
Replace with:
v->xs = umin(v->xs - 1, vc->vc_cols - 1);
which again converts a -1 to unsigned - this time to 0xffffffff, with the same overall effect.
Whether zero is meant to mean the 'maximum size' is unknown. I can't find any documentation for the ioctl and it pre-dates git.
Detected by an extra check added to min_t().
--- drivers/tty/vt/selection.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/tty/vt/selection.c +5 -4
@@ -348,10 +348,11 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,return 0 ; } - v->xs =min_t (u16, v->xs -1 , vc->vc_cols -1 ); - v->ys =min_t (u16, v->ys -1 , vc->vc_rows -1 ); - v->xe =min_t (u16, v->xe -1 , vc->vc_cols -1 ); - v->ye =min_t (u16, v->ye -1 , vc->vc_rows -1 ); +/* Historically 0 => max value */ + v->xs =umin (v->xs -1 , vc->vc_cols -1 ); + v->ys =umin (v->ys -1 , vc->vc_rows -1 ); + v->xe =umin (v->xe -1 , vc->vc_cols -1 ); + v->ye =umin (v->ye -1 , vc->vc_rows -1 );if (mouse_reporting () && (v->sel_mode &TIOCL_SELMOUSEREPORT )) { mouse_report(tty, v->sel_mode & TIOCL_SELBUTTONMASK, v->xs,
Signature · 1 line
2.39.5
Re: [PATCH 27/44] drivers/tty/vt: use umin() instead of min_t(u16, ...) for row/col limits
Quoted 13 lines · On 19. 11. 25, 23:41, [email protected] wrote:
From: David Laight <[email protected]>
The row/column bounds (for a screen window box) are changed from 'offset one' to 'offset zero' and bound to the screen size using:
v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);This has the side effect of converting zero to the limit.
A check I'm adding to
min_t()reports that (u16)(v->xs - 1) (etc) discards signiticant bits (becausev->xsis promoted to 'int' before the addition).
Ifv->xsis zero (it comes from userspace) it converts -1 to0xffff. This is then bounded to 'vc->vc_cols- 1' which will be fine.
Quoted 4 lines
Replace with:
v->xs = umin(v->xs - 1, vc->vc_cols - 1);which again converts a -1 to unsigned - this time to
0xffffffff, with the same overall effect.
LGTM:
Quoted 2 lines
Whether zero is meant to mean the 'maximum size' is unknown. I can't find any documentation for the ioctl and it pre-dates git.
Behavior of zero is unspecified AFAICT (the impl can do whatever is easy :)).
thanks,
Signature · 2 lines
js suse labs
PATCH 28/44 drivers/usb/storage: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- drivers/usb/storage/protocol.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/usb/storage/protocol.c +1 -2
@@ -139,8 +139,7 @@ unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,return cnt;while (sg_miter_next (&miter) && cnt < buflen) { - unsigned int len = min_t(unsigned int, miter.length, - buflen - cnt); +unsigned int len =min (miter.length, buflen - cnt);if (dir ==FROM_XFER_BUF )memcpy (buffer + cnt, miter.addr, len);
Signature · 1 line
2.39.5
Re: [PATCH 28/44] drivers/usb/storage: use min() instead of min_t()
Quoted 10 lines · On Wed, Nov 19, 2025 at 10:41:24PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to
min_t().
In fact, min_t(T, a, b) cannot go wrong as long as all the types are unsigned and at least one of a, b has type T or smaller. Of course, in this situation there's no reason not to simply use min(). (And if both a and b have types larger than T, why would someone use min_t() like this in the first place?)
Regardless, the patch is fine with me.
Alan Stern
Quoted 22 lines
Signed-off-by David Laight <[email protected]>--- drivers/usb/storage/protocol.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/usb/storage/protocol.c +1
-2@@ -139,8 +139,7 @@ unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,return cnt;while (sg_miter_next (&miter) && cnt < buflen) { - unsigned int len = min_t(unsigned int, miter.length, - buflen - cnt); +unsigned int len =min (miter.length, buflen - cnt);if (dir ==FROM_XFER_BUF )memcpy (buffer + cnt, miter.addr, len);Signature · 1 line
2.39.5
Re: [PATCH 28/44] drivers/usb/storage: use min() instead of min_t()
Quoted 7 lines · On Wed, 19 Nov 2025 21:59:42 -0500 Alan Stern <[email protected]> wrote:
Quoted 6 lines · On Wed, Nov 19, 2025 at 10:41:24PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
Quoted 7 lines
Quoted 4 lines
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to
min_t().In fact,
min_t(T, a, b)cannot go wrong as long as all the types are unsigned and at least one of a, b has type T or smaller.
That is backwards, both a and b have to have types at least as large as T (or rather values that will fit in T).
- which is exactly what people keep getting wrong. Consider:
u32 a = 4; u64 b = 0x100000001ull; then: min_t(u32, a, b)
has value 1 not 4.
David
Quoted 33 lines
Of course, in
this situation there's no reason not to simply usemin(). (And if both a and b have types larger than T, why would someone usemin_t()like this in the first place?)Regardless, the patch is fine with me.
Acked-by Alan Stern <[email protected]>Alan Stern
Quoted 22 lines
Signed-off-by David Laight <[email protected]>--- drivers/usb/storage/protocol.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/usb/storage/protocol.c +1
-2@@ -139,8 +139,7 @@ unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,return cnt;while (sg_miter_next (&miter) && cnt < buflen) { - unsigned int len = min_t(unsigned int, miter.length, - buflen - cnt); +unsigned int len =min (miter.length, buflen - cnt);if (dir ==FROM_XFER_BUF )memcpy (buffer + cnt, miter.addr, len);Signature · 1 line
2.39.5
Alan Stern
maintainerRe: [usb-storage] Re: [PATCH 28/44] drivers/usb/storage: use min() instead of min_t()
Quoted 22 lines · David Laight
Quoted 7 lines · On Wed, 19 Nov 2025 21:59:42 -0500 Alan Stern <[email protected]> wrote:
Quoted 6 lines · On Wed, Nov 19, 2025 at 10:41:24PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.Quoted 7 lines
Quoted 4 lines
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to
min_t().In fact,
min_t(T, a, b)cannot go wrong as long as all the types are unsigned and at least one of a, b has type T or smaller.That is backwards, both a and b have to have types at least as large as T (or rather values that will fit in T).
- which is exactly what people keep getting wrong. Consider:
Quoted 5 lines
u32 a = 4; u64 b = 0x100000001ull; then: min_t(u32, a, b)has value 1 not 4.
You are right. For some reason I was thinking that the comparison took place before the casts, which doesn't make any sense.
Alan Stern
PATCH 29/44 drivers/xen: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- drivers/xen/grant-table.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/xen/grant-table.c +1 -1
@@ -1204,7 +1204,7 @@ void gnttab_foreach_grant_in_range(struct page *page,unsigned int glen;unsigned long xen_pfn; - len = min_t(unsigned int, PAGE_SIZE - offset, len); + len =min (PAGE_SIZE - offset, len); goffset =xen_offset_in_page (offset); xen_pfn =page_to_xen_pfn (page) +XEN_PFN_DOWN (offset);
Signature · 1 line
2.39.5
Re: [PATCH 29/44] drivers/xen: use min() instead of min_t()
Quoted 6 lines · On 19.11.25 23:41, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
Quoted 6 lines
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to
min_t().Signed-off-by David Laight <[email protected]>
I'll take this patch through the Xen tree.
Juergen
PATCH 30/44 fs: use min() or umin() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
A couple of places need umin() because of loops like:
nfolios = DIV_ROUND_UP(ret + start, PAGE_SIZE);
for (i = 0; i < nfolios; i++) {
struct folio *folio = page_folio(pages[i]);
...
unsigned int len = umin(ret, PAGE_SIZE - start);
...
ret -= len;
...
}where the compiler doesn't track things well enough to know that 'ret' is never negative.
The alternate loop:
for (i = 0; ret > 0; i++) {
struct folio *folio = page_folio(pages[i]);
...
unsigned int len = min(ret, PAGE_SIZE - start);
...
ret -= len;
...
}would be equivalent and doesn't need 'nfolios'.
Most of the 'unsigned long' actually come from PAGE_SIZE.
Detected by an extra check added to min_t().
--- fs/buffer.c | 2 +- fs/exec.c | 2 +- fs/ext4/mballoc.c | 3 +-- fs/ext4/resize.c | 2 +- fs/ext4/super.c | 2 +- fs/fat/dir.c | 4 ++-- fs/fat/file.c | 3 +-- fs/fuse/dev.c | 2 +- fs/fuse/file.c | 8 +++----- fs/splice.c | 2 +- 10 files changed, 13 insertions(+), 17 deletions(-)
fs/buffer.c +1 -1
@@ -2354,7 +2354,7 @@ bool block_is_partially_uptodate(struct folio *folio, size_t from, size_t count)if (!head)return false; blocksize = head->b_size; - to =min_t (unsigned,folio_size (folio) - from, count); + to =min (folio_size (folio) - from, count); to = from + to;if (from < blocksize && to >folio_size (folio) - blocksize)return false;
fs/exec.c +1 -1
@@ -555,7 +555,7 @@ int copy_string_kernel(const char *arg, struct linux_binprm *bprm)return -E2BIG ;while (len >0 ) { - unsigned int bytes_to_copy = min_t(unsigned int, len, +unsigned int bytes_to_copy =min (len,min_not_zero (offset_in_page (pos),PAGE_SIZE ));struct page *page;
fs/ext4/mballoc.c +1 -2
@@ -4254,8 +4254,7 @@ void ext4_mb_mark_bb(struct super_block *sb, ext4_fsblk_t block,* get the corresponding group metadata to work with. * For this we have goto again loop. */ - thisgrp_len = min_t(unsigned int, (unsigned int)len, -EXT4_BLOCKS_PER_GROUP (sb) -EXT4_C2B (sbi, blkoff)); + thisgrp_len =min (len,EXT4_BLOCKS_PER_GROUP (sb) -EXT4_C2B (sbi, blkoff)); clen =EXT4_NUM_B2C (sbi, thisgrp_len);if (!ext4_sb_block_valid (sb,NULL , block, thisgrp_len)) {
fs/ext4/resize.c +1 -1
@@ -1479,7 +1479,7 @@ static void ext4_update_super(struct super_block *sb,/* Update the global fs size fields */ sbi->s_groups_count += flex_gd->count; - sbi->s_blockfile_groups =min_t (ext4_group_t, sbi->s_groups_count, + sbi->s_blockfile_groups =min (sbi->s_groups_count, (EXT4_MAX_BLOCK_FILE_PHYS /EXT4_BLOCKS_PER_GROUP (sb)));/* Update the reserved block counts only once the new group is
fs/ext4/super.c +1 -1
@@ -4830,7 +4830,7 @@ static int ext4_check_geometry(struct super_block *sb,return -EINVAL ; } sbi->s_groups_count = blocks_count; - sbi->s_blockfile_groups =min_t (ext4_group_t, sbi->s_groups_count, + sbi->s_blockfile_groups =min (sbi->s_groups_count, (EXT4_MAX_BLOCK_FILE_PHYS /EXT4_BLOCKS_PER_GROUP (sb)));if (((u64 )sbi->s_groups_count * sbi->s_inodes_per_group) !=le32_to_cpu (es->s_inodes_count)) {
fs/fat/dir.c +2 -2
@@ -1353,7 +1353,7 @@ int fat_add_entries(struct inode *dir, void *slots, int nr_slots,/* Fill the long name slots. */ for (i =0 ; i < long_bhs; i++) { -int copy =min_t (int, sb->s_blocksize - offset, size); +int copy =umin (sb->s_blocksize - offset, size);memcpy (bhs[i]->b_data + offset, slots, copy);mark_buffer_dirty_inode (bhs[i], dir); offset =0 ;
@@ -1364,7 +1364,7 @@ int fat_add_entries(struct inode *dir, void *slots, int nr_slots, err =fat_sync_bhs (bhs, long_bhs);if (!err && i < nr_bhs) {/* Fill the short name slot. */ -int copy =min_t (int, sb->s_blocksize - offset, size); +int copy =umin (sb->s_blocksize - offset, size);memcpy (bhs[i]->b_data + offset, slots, copy);mark_buffer_dirty_inode (bhs[i], dir);if (IS_DIRSYNC (dir))
fs/fat/file.c +1 -2
@@ -140,8 +140,7 @@ static int fat_ioctl_fitrim(struct inode *inode, unsigned long arg)if (copy_from_user (&range, user_range,sizeof (range)))return -EFAULT ; - range.minlen = max_t(unsigned int, range.minlen, -bdev_discard_granularity (sb->s_bdev)); + range.minlen =max (range.minlen,bdev_discard_granularity (sb->s_bdev)); err =fat_trim_fs (inode, &range);if (err <0 )
fs/fuse/dev.c +1 -1
@@ -1813,7 +1813,7 @@ static int fuse_notify_store(struct fuse_conn *fc, unsigned int size, goto out_iput; folio_offset = ((index - folio->index) <<PAGE_SHIFT ) + offset; - nr_bytes =min_t (unsigned, num,folio_size (folio) - folio_offset); + nr_bytes =min (num,folio_size (folio) - folio_offset); nr_pages = (offset + nr_bytes +PAGE_SIZE -1 ) >>PAGE_SHIFT ; err =fuse_copy_folio (cs, &folio, folio_offset, nr_bytes,0 );
fs/fuse/file.c +3 -5
@@ -1252,10 +1252,8 @@ static ssize_t fuse_fill_write_pages(struct fuse_io_args *ia,static inline unsigned int fuse_wr_pages (loff_t pos,size_t len,unsigned int max_pages) { - return min_t(unsigned int, - ((pos + len -1 ) >>PAGE_SHIFT ) - - (pos >>PAGE_SHIFT ) +1 , - max_pages); +return min (((pos + len -1 ) >>PAGE_SHIFT ) - (pos >>PAGE_SHIFT ) +1 , + max_pages); }static ssize_t fuse_perform_write (struct kiocb *iocb,struct iov_iter *ii)
@@ -1550,7 +1548,7 @@ static int fuse_get_user_pages(struct fuse_args_pages *ap, struct iov_iter *ii,struct folio *folio =page_folio (pages[i]);unsigned int offset = start + (folio_page_idx (folio, pages[i]) <<PAGE_SHIFT ); - unsigned int len = min_t(unsigned int, ret, PAGE_SIZE - start); +unsigned int len =umin (ret,PAGE_SIZE - start); ap->descs[ap->num_folios].offset = offset; ap->descs[ap->num_folios].length = len;
fs/splice.c +1 -1
@@ -1467,7 +1467,7 @@ static ssize_t iter_to_pipe(struct iov_iter *from, n =DIV_ROUND_UP (left + start,PAGE_SIZE );for (i =0 ; i < n; i++) { -int size =min_t (int, left,PAGE_SIZE - start); +int size =umin (left,PAGE_SIZE - start); buf.page = pages[i]; buf.offset = start;
Signature · 1 line
2.39.5
Christian Brauner
maintainerRe: [PATCH 30/44] fs: use min() or umin() instead of min_t()
Quoted 29 lines · On Wed, Nov 19, 2025 at 10:41:26PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.A couple of places need
umin()because of loops like:nfolios = DIV_ROUND_UP(ret + start, PAGE_SIZE); for (i = 0; i < nfolios; i++) { struct folio *folio = page_folio(pages[i]); ... unsigned int len = umin(ret, PAGE_SIZE - start); ... ret -= len; ... }where the compiler doesn't track things well enough to know that 'ret' is never negative.
The alternate loop:
for (i = 0; ret > 0; i++) { struct folio *folio = page_folio(pages[i]); ... unsigned int len = min(ret, PAGE_SIZE - start); ... ret -= len; ... }
Quoted 8 lines
would be equivalent and doesn't need 'nfolios'.
Most of the 'unsigned long' actually come from
PAGE_SIZE.Detected by an extra check added to
min_t().Signed-off-by David Laight <[email protected]>---
Too late for this cycle but we will pick this up next cycle!
Quoted 169 lines
fs/buffer.c | 2 +- fs/exec.c | 2 +- fs/ext4/mballoc.c | 3 +-- fs/ext4/resize.c | 2 +- fs/ext4/super.c | 2 +- fs/fat/dir.c | 4 ++-- fs/fat/file.c | 3 +-- fs/fuse/dev.c | 2 +- fs/fuse/file.c | 8 +++----- fs/splice.c | 2 +- 10 files changed, 13 insertions(+), 17 deletions(-)fs/buffer.c +1
-1@@ -2354,7 +2354,7 @@ bool block_is_partially_uptodate(struct folio *folio, size_t from, size_t count)if (!head)return false; blocksize = head->b_size; - to =min_t (unsigned,folio_size (folio) - from, count); + to =min (folio_size (folio) - from, count); to = from + to;if (from < blocksize && to >folio_size (folio) - blocksize)return false;fs/exec.c +1
-1@@ -555,7 +555,7 @@ int copy_string_kernel(const char *arg, struct linux_binprm *bprm)return -E2BIG ;while (len >0 ) { - unsigned int bytes_to_copy = min_t(unsigned int, len, +unsigned int bytes_to_copy =min (len,min_not_zero (offset_in_page (pos),PAGE_SIZE ));struct page *page;fs/ext4/mballoc.c +1
-2@@ -4254,8 +4254,7 @@ void ext4_mb_mark_bb(struct super_block *sb, ext4_fsblk_t block,* get the corresponding group metadata to work with. * For this we have goto again loop. */ - thisgrp_len = min_t(unsigned int, (unsigned int)len, -EXT4_BLOCKS_PER_GROUP (sb) -EXT4_C2B (sbi, blkoff)); + thisgrp_len =min (len,EXT4_BLOCKS_PER_GROUP (sb) -EXT4_C2B (sbi, blkoff)); clen =EXT4_NUM_B2C (sbi, thisgrp_len);if (!ext4_sb_block_valid (sb,NULL , block, thisgrp_len)) {fs/ext4/resize.c +1
-1@@ -1479,7 +1479,7 @@ static void ext4_update_super(struct super_block *sb,/* Update the global fs size fields */ sbi->s_groups_count += flex_gd->count; - sbi->s_blockfile_groups =min_t (ext4_group_t, sbi->s_groups_count, + sbi->s_blockfile_groups =min (sbi->s_groups_count, (EXT4_MAX_BLOCK_FILE_PHYS /EXT4_BLOCKS_PER_GROUP (sb)));/* Update the reserved block counts only once the new group is fs/ext4/super.c +1
-1@@ -4830,7 +4830,7 @@ static int ext4_check_geometry(struct super_block *sb,return -EINVAL ; } sbi->s_groups_count = blocks_count; - sbi->s_blockfile_groups =min_t (ext4_group_t, sbi->s_groups_count, + sbi->s_blockfile_groups =min (sbi->s_groups_count, (EXT4_MAX_BLOCK_FILE_PHYS /EXT4_BLOCKS_PER_GROUP (sb)));if (((u64 )sbi->s_groups_count * sbi->s_inodes_per_group) !=le32_to_cpu (es->s_inodes_count)) {fs/fat/dir.c +2
-2@@ -1353,7 +1353,7 @@ int fat_add_entries(struct inode *dir, void *slots, int nr_slots,/* Fill the long name slots. */ for (i =0 ; i < long_bhs; i++) { -int copy =min_t (int, sb->s_blocksize - offset, size); +int copy =umin (sb->s_blocksize - offset, size);memcpy (bhs[i]->b_data + offset, slots, copy);mark_buffer_dirty_inode (bhs[i], dir); offset =0 ;@@ -1364,7 +1364,7 @@ int fat_add_entries(struct inode *dir, void *slots, int nr_slots, err =fat_sync_bhs (bhs, long_bhs);if (!err && i < nr_bhs) {/* Fill the short name slot. */ -int copy =min_t (int, sb->s_blocksize - offset, size); +int copy =umin (sb->s_blocksize - offset, size);memcpy (bhs[i]->b_data + offset, slots, copy);mark_buffer_dirty_inode (bhs[i], dir);if (IS_DIRSYNC (dir))fs/fat/file.c +1
-2@@ -140,8 +140,7 @@ static int fat_ioctl_fitrim(struct inode *inode, unsigned long arg)if (copy_from_user (&range, user_range,sizeof (range)))return -EFAULT ; - range.minlen = max_t(unsigned int, range.minlen, -bdev_discard_granularity (sb->s_bdev)); + range.minlen =max (range.minlen,bdev_discard_granularity (sb->s_bdev)); err =fat_trim_fs (inode, &range);if (err <0 )fs/fuse/dev.c +1
-1@@ -1813,7 +1813,7 @@ static int fuse_notify_store(struct fuse_conn *fc, unsigned int size, goto out_iput; folio_offset = ((index - folio->index) <<PAGE_SHIFT ) + offset; - nr_bytes =min_t (unsigned, num,folio_size (folio) - folio_offset); + nr_bytes =min (num,folio_size (folio) - folio_offset); nr_pages = (offset + nr_bytes +PAGE_SIZE -1 ) >>PAGE_SHIFT ; err =fuse_copy_folio (cs, &folio, folio_offset, nr_bytes,0 );fs/fuse/file.c +3
-5@@ -1252,10 +1252,8 @@ static ssize_t fuse_fill_write_pages(struct fuse_io_args *ia,static inline unsigned int fuse_wr_pages (loff_t pos,size_t len,unsigned int max_pages) { - return min_t(unsigned int, - ((pos + len -1 ) >>PAGE_SHIFT ) - - (pos >>PAGE_SHIFT ) +1 , - max_pages); +return min (((pos + len -1 ) >>PAGE_SHIFT ) - (pos >>PAGE_SHIFT ) +1 , + max_pages); }static ssize_t fuse_perform_write (struct kiocb *iocb,struct iov_iter *ii)@@ -1550,7 +1548,7 @@ static int fuse_get_user_pages(struct fuse_args_pages *ap, struct iov_iter *ii,struct folio *folio =page_folio (pages[i]);unsigned int offset = start + (folio_page_idx (folio, pages[i]) <<PAGE_SHIFT ); - unsigned int len = min_t(unsigned int, ret, PAGE_SIZE - start); +unsigned int len =umin (ret,PAGE_SIZE - start); ap->descs[ap->num_folios].offset = offset; ap->descs[ap->num_folios].length = len;fs/splice.c +1
-1@@ -1467,7 +1467,7 @@ static ssize_t iter_to_pipe(struct iov_iter *from, n =DIV_ROUND_UP (left + start,PAGE_SIZE );for (i =0 ; i < n; i++) { -int size =min_t (int, left,PAGE_SIZE - start); +int size =umin (left,PAGE_SIZE - start); buf.page = pages[i]; buf.offset = start;Signature · 1 line
2.39.5
Re: [PATCH 30/44] fs: use min() or umin() instead of min_t()
In reply to [email protected]Hi David,
Quoted 29 lines · On Wed, Nov 19, 2025 at 10:41:26PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.A couple of places need
umin()because of loops like:nfolios = DIV_ROUND_UP(ret + start, PAGE_SIZE); for (i = 0; i < nfolios; i++) { struct folio *folio = page_folio(pages[i]); ... unsigned int len = umin(ret, PAGE_SIZE - start); ... ret -= len; ... }where the compiler doesn't track things well enough to know that 'ret' is never negative.
The alternate loop:
for (i = 0; ret > 0; i++) { struct folio *folio = page_folio(pages[i]); ... unsigned int len = min(ret, PAGE_SIZE - start); ... ret -= len; ... }
Quoted 7 lines
would be equivalent and doesn't need 'nfolios'.
Most of the 'unsigned long' actually come from
PAGE_SIZE.Detected by an extra check added to
min_t().Signed-off-by David Laight <[email protected]>
When doing a mips cross compile from an arm64 host
(via ARCH=mips CROSS_COMPILE=mips64-linux-gnu- make), the following build error occurs in linux-next and goes away when I revert this commit.
In file included from <command-line>:
In function ‘fuse_wr_pages’,
inlined from ‘fuse_perform_write’ at fs/fuse/file.c:1347:27:././include/linux/compiler_types.h:667:45: error: call to ‘__compiletime_assert_405’ declared with attribute error: min(((pos + len
- 1) >> 12) - (pos >> 12) + 1, max_pages) signedness error
667 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^././include/linux/compiler_types.h:648:25: note: in definition of macro ‘__compiletime_assert’
648 | prefix ## suffix(); \
| ^~~~~~
././include/linux/compiler_types.h:667:9: note: in expansion of macro ‘_compiletime_assert’
667 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
./include/linux/minmax.h:93:9: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
93 | BUILD_BUG_ON_MSG(!__types_ok(ux, uy), \
| ^~~~~~~~~~~~~~~~
./include/linux/minmax.h:98:9: note: in expansion of macro ‘__careful_cmp_once’
98 | __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
| ^~~~~~~~~~~~~~~~~~
./include/linux/minmax.h:105:25: note: in expansion of macro ‘__careful_cmp’
105 | #define min(x, y) __careful_cmp(min, x, y)
| ^~~~~~~~~~~~~fs/fuse/file.c:1326:16: note: in expansion of macro ‘min’
1326 | return min(((pos + len - 1) >> PAGE_SHIFT) - (pos >> PAGE_SHIFT) + 1,
| ^~~
This is on a cento-stream-10 host running
gcc version 14.3.1 20250617 (Red Hat 14.3.1-2) (GCC). I didn't look into this in detail, and I'm not entirely sure what the correct fix here should be.
Brian
Re: [PATCH 30/44] fs: use min() or umin() instead of min_t()
Quoted 36 lines · On Mon, 12 Jan 2026 16:51:22 -0500 Brian Masney <[email protected]> wrote:
Hi David,
Quoted 33 lines · On Wed, Nov 19, 2025 at 10:41:26PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.A couple of places need
umin()because of loops like:nfolios = DIV_ROUND_UP(ret + start, PAGE_SIZE); for (i = 0; i < nfolios; i++) { struct folio *folio = page_folio(pages[i]); ... unsigned int len = umin(ret, PAGE_SIZE - start); ... ret -= len; ... }where the compiler doesn't track things well enough to know that 'ret' is never negative.
The alternate loop:
for (i = 0; ret > 0; i++) { struct folio *folio = page_folio(pages[i]); ... unsigned int len = min(ret, PAGE_SIZE - start); ... ret -= len; ... }would be equivalent and doesn't need 'nfolios'.
Most of the 'unsigned long' actually come from
PAGE_SIZE.
Quoted 8 lines
Quoted 3 lines
Detected by an extra check added to
min_t().Signed-off-by David Laight <[email protected]>When doing a mips cross compile from an arm64 host
(via ARCH=mips CROSS_COMPILE=mips64-linux-gnu- make), the following build error occurs in linux-next and goes away when I revert this commit.
I've looked at this one before.
I think there is another patch lurking to fix it.
Quoted 5 lines
In file included from <command-line>:
In function ‘fuse_wr_pages’,
inlined from ‘fuse_perform_write’ atfs/fuse/file.c:1347:27:././include/linux/compiler_types.h:667:45: error: call to ‘__compiletime_assert_405’ declared with attribute error: min(((pos + len
- 1) >> 12) - (pos >> 12) + 1,max_pages) signedness error
...
Quoted 2 lines
fs/fuse/file.c:1326:16: note: in expansion of macro ‘min’
1326 | return min(((pos + len - 1) >>PAGE_SHIFT) - (pos >>PAGE_SHIFT) + 1,
max_pages);
'len' is 'unsigned long' and the expression is unsigned on 64bit. But 'pos' is s64 so the expression is signed on 32bit. IIRC the final version might have been (equivalent to):
len += pos & (PAGE_SIZE - 1); return min(DIV_ROUND_UP(len, PAGE_SIZE), max_pages);
which generates much better code as well (no 64bit maths).
I don't think len can overflow, read/write are limited to INT_MAX - PAGE_SIZE bytes in the syscall interface.
David
Quoted 8 lines
This is on a cento-stream-10 host running
gcc version 14.3.1 20250617 (Red Hat 14.3.1-2) (GCC). I didn't look into this in detail, and I'm not entirely sure what the correct fix here should be.Brian
Re: [PATCH 30/44] fs: use min() or umin() instead of min_t()
In reply to [email protected]Quoted 5 lines · On Wed, Nov 19, 2025 at 10:41:26PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
This breaks an arm imx_v6_v7_defconfig build:
In file included from <command-line>:
In function 'fuse_wr_pages',
inlined from 'fuse_perform_write' at /home/broonie/git/bisect/fs/fuse/file.c:1347:27:/home/broonie/git/bisect/include/linux/compiler_types.h:630:45: error: call to '__compiletime_assert_434' declared with attribute error: min(((pos + len - 1) >> 12) - (pos >> 12) + 1, max_pages) signedness error
630 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^/home/broonie/git/bisect/include/linux/compiler_types.h:611:25: note: in definition of macro '__compiletime_assert'
611 | prefix ## suffix(); \
| ^~~~~~
/home/broonie/git/bisect/include/linux/compiler_types.h:630:9: note: in expansion of macro '_compiletime_assert'
630 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
/home/broonie/git/bisect/include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
/home/broonie/git/bisect/include/linux/minmax.h:93:9: note: in expansion of macro 'BUILD_BUG_ON_MSG'
93 | BUILD_BUG_ON_MSG(!__types_ok(ux, uy), \
| ^~~~~~~~~~~~~~~~
/home/broonie/git/bisect/include/linux/minmax.h:98:9: note: in expansion of macro '__careful_cmp_once'
98 | __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
| ^~~~~~~~~~~~~~~~~~
/home/broonie/git/bisect/include/linux/minmax.h:105:25: note: in expansion of macro '__careful_cmp'
105 | #define min(x, y) __careful_cmp(min, x, y)
| ^~~~~~~~~~~~~/home/broonie/git/bisect/fs/fuse/file.c:1326:16: note: in expansion of macro 'min'
1326 | return min(((pos + len - 1) >> PAGE_SHIFT) - (pos >> PAGE_SHIFT) + 1,
| ^~~
Re: [PATCH 30/44] fs: use min() or umin() instead of min_t()
Quoted 8 lines · On Tue, 13 Jan 2026 16:56:56 +0000 Mark Brown <[email protected]> wrote:
Quoted 5 lines · On Wed, Nov 19, 2025 at 10:41:26PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.This breaks an arm
imx_v6_v7_defconfigbuild:
I hadn't tested 32bit when I sent the patch.
It was noticed ages ago and I thought there was a patch (to fuse/file.c) that changed the code to avoid the 64bit signed maths on 32bit.
David
Quoted 28 lines
In file included from <command-line>:
In function 'fuse_wr_pages',
inlined from 'fuse_perform_write' at/home/broonie/git/bisect/fs/fuse/file.c:1347:27:/home/broonie/git/bisect/include/linux/compiler_types.h:630:45: error: call to '__compiletime_assert_434' declared with attribute error:min(((pos + len - 1) >> 12) - (pos >> 12) + 1, max_pages)signedness error
630 |_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^/home/broonie/git/bisect/include/linux/compiler_types.h:611:25: note: in definition of macro '__compiletime_assert'611 | prefix ## suffix(); \ | ^~~~~~ /home/broonie/git/bisect/include/linux/compiler_types.h:630:9: note: in expansion of macro '_compiletime_assert' 630 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^~~~~~~~~~~~~~~~~~~ /home/broonie/git/bisect/include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert' 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) | ^~~~~~~~~~~~~~~~~~ /home/broonie/git/bisect/include/linux/minmax.h:93:9: note: in expansion of macro 'BUILD_BUG_ON_MSG' 93 | BUILD_BUG_ON_MSG(!__types_ok(ux, uy), \ | ^~~~~~~~~~~~~~~~ /home/broonie/git/bisect/include/linux/minmax.h:98:9: note: in expansion of macro '__careful_cmp_once' 98 | __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_)) | ^~~~~~~~~~~~~~~~~~ /home/broonie/git/bisect/include/linux/minmax.h:105:25: note: in expansion of macro '__careful_cmp' 105 | #define min(x, y) __careful_cmp(min, x, y) | ^~~~~~~~~~~~~
/home/broonie/git/bisect/fs/fuse/file.c:1326:16: note: in expansion of macro 'min'
1326 | return min(((pos + len - 1) >>PAGE_SHIFT) - (pos >>PAGE_SHIFT) + 1,
| ^~~
Re: [PATCH 30/44] fs: use min() or umin() instead of min_t()
Quoted 8 lines · David Laight
Quoted 3 lines · Mark Brown
On Wed, Nov 19, 2025 at 10:41:26PM +0000, [email protected] wrote:
This breaks an arm
imx_v6_v7_defconfigbuild:I hadn't tested 32bit when I sent the patch.
It was noticed ages ago and I thought there was a patch (tofuse/file.c) that changed the code to avoid the 64bit signed maths on 32bit.
It's possible there's a patch out there somewhere but it's not present in what's in -next.
Re: [PATCH 30/44] fs: use min() or umin() instead of min_t()
In reply to David LaightQuoted 10 lines · On Tue, 13 Jan 2026 18:33:46 +0000 David Laight <[email protected]>…
Quoted 7 lines · On Tue, 13 Jan 2026 16:56:56 +0000 Mark Brown <[email protected]> wrote:
Quoted 5 lines · On Wed, Nov 19, 2025 at 10:41:26PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
Quoted 5 lines
Quoted 1 line
This breaks an arm
imx_v6_v7_defconfigbuild:I hadn't tested 32bit when I sent the patch.
It was noticed ages ago and I thought there was a patch (tofuse/file.c) that changed the code to avoid the 64bit signed maths on 32bit.
I've just sent in a patch to fix it, compile tested for 32bit x86.
David
Quoted 5 lines
Quoted 5 lines
In file included from <command-line>:
In function 'fuse_wr_pages',
inlined from 'fuse_perform_write' at/home/broonie/git/bisect/fs/fuse/file.c:1347:27:/home/broonie/git/bisect/include/linux/compiler_types.h:630:45: error: call to '__compiletime_assert_434' declared with attribute error:min(((pos + len - 1) >> 12) - (pos >> 12) + 1, max_pages)signedness error
... ^~~~~~~~~~~~~
Quoted 4 lines
Quoted 3 lines
/home/broonie/git/bisect/fs/fuse/file.c:1326:16: note: in expansion of macro 'min'
1326 | return min(((pos + len - 1) >>PAGE_SHIFT) - (pos >>PAGE_SHIFT) + 1,
| ^~~
PATCH 31/44 block: bvec.h: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- include/linux/bvec.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
include/linux/bvec.h +1 -2
@@ -219,8 +219,7 @@ static inline void bvec_advance(const struct bio_vec *bvec, bv->bv_page = bvec->bv_page + (bvec->bv_offset >>PAGE_SHIFT ); bv->bv_offset = bvec->bv_offset & ~PAGE_MASK ; } - bv->bv_len = min_t(unsigned int, PAGE_SIZE - bv->bv_offset, - bvec->bv_len - iter_all->done); + bv->bv_len =min (PAGE_SIZE - bv->bv_offset, bvec->bv_len - iter_all->done); iter_all->done += bv->bv_len;if (iter_all->done == bvec->bv_len) {
Signature · 1 line
2.39.5
PATCH 32/44 nodemask: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- include/linux/nodemask.h | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
include/linux/nodemask.h +4 -5
@@ -245,18 +245,18 @@ static __always_inline int __nodes_weight(const nodemask_t *srcp, unsigned int n }/* FIXME: better would be to fix all architectures to never return -> MAX_NUMNODES, then the silly min_ts could be dropped. */ +> MAX_NUMNODES, then the silly min()s could be dropped. */ #define first_node (src) __first_node(&(src)) static __always_inline unsigned int __first_node(const nodemask_t *srcp) { - return min_t(unsigned int, MAX_NUMNODES, find_first_bit(srcp->bits, MAX_NUMNODES)); +return min (MAX_NUMNODES ,find_first_bit (srcp->bits,MAX_NUMNODES )); }#define next_node (n, src) __next_node((n), &(src)) static __always_inline unsigned int __next_node(int n, const nodemask_t *srcp) { - return min_t(unsigned int, MAX_NUMNODES, find_next_bit(srcp->bits, MAX_NUMNODES, n+1)); +return min (MAX_NUMNODES ,find_next_bit (srcp->bits,MAX_NUMNODES , n+1 )); }/*
@@ -293,8 +293,7 @@ static __always_inline void init_nodemask_of_node(nodemask_t *mask, int node)#define first_unset_node (mask) __first_unset_node(&(mask)) static __always_inline unsigned int __first_unset_node(const nodemask_t *maskp) { - return min_t(unsigned int, MAX_NUMNODES, -find_first_zero_bit (maskp->bits,MAX_NUMNODES )); +return min (MAX_NUMNODES ,find_first_zero_bit (maskp->bits,MAX_NUMNODES )); }#define NODE_MASK_LAST_WORD BITMAP_LAST_WORD_MASK(MAX_NUMNODES)
Signature · 1 line
2.39.5
Re: [PATCH 32/44] nodemask: use min() instead of min_t()
Quoted 6 lines · On Wed, Nov 19, 2025 at 10:41:28PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
Quoted 6 lines
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to
min_t().Signed-off-by David Laight <[email protected]>
Applied, thanks.
Quoted 43 lines
--- include/linux/nodemask.h | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)include/linux/nodemask.h +4
-5@@ -245,18 +245,18 @@ static __always_inline int __nodes_weight(const nodemask_t *srcp, unsigned int n }/* FIXME: better would be to fix all architectures to never return -> MAX_NUMNODES, then the silly min_ts could be dropped. */ +> MAX_NUMNODES, then the silly min()s could be dropped. */ #define first_node (src) __first_node(&(src)) static __always_inline unsigned int __first_node(const nodemask_t *srcp) { - return min_t(unsigned int, MAX_NUMNODES, find_first_bit(srcp->bits, MAX_NUMNODES)); +return min (MAX_NUMNODES ,find_first_bit (srcp->bits,MAX_NUMNODES )); }#define next_node (n, src) __next_node((n), &(src)) static __always_inline unsigned int __next_node(int n, const nodemask_t *srcp) { - return min_t(unsigned int, MAX_NUMNODES, find_next_bit(srcp->bits, MAX_NUMNODES, n+1)); +return min (MAX_NUMNODES ,find_next_bit (srcp->bits,MAX_NUMNODES , n+1 )); }/* @@ -293,8 +293,7 @@ static __always_inline void init_nodemask_of_node(nodemask_t *mask, int node)#define first_unset_node (mask) __first_unset_node(&(mask)) static __always_inline unsigned int __first_unset_node(const nodemask_t *maskp) { - return min_t(unsigned int, MAX_NUMNODES, -find_first_zero_bit (maskp->bits,MAX_NUMNODES )); +return min (MAX_NUMNODES ,find_first_zero_bit (maskp->bits,MAX_NUMNODES )); }#define NODE_MASK_LAST_WORD BITMAP_LAST_WORD_MASK(MAX_NUMNODES)Signature · 1 line
2.39.5
PATCH 33/44 ipc: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- ipc/mqueue.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ipc/mqueue.c +2 -2
@@ -363,7 +363,7 @@ static struct inode *mqueue_get_inode(struct super_block *sb,if (info->attr.mq_msgsize >ULONG_MAX /info->attr.mq_maxmsg) goto out_inode; mq_treesize = info->attr.mq_maxmsg *sizeof (struct msg_msg ) + - min_t(unsigned int, info->attr.mq_maxmsg, MQ_PRIO_MAX) * +min (info->attr.mq_maxmsg,MQ_PRIO_MAX ) *sizeof (struct posix_msg_tree_node ); mq_bytes = info->attr.mq_maxmsg * info->attr.mq_msgsize;if (mq_bytes + mq_treesize < mq_bytes)
@@ -540,7 +540,7 @@ static void mqueue_evict_inode(struct inode *inode)/* Total amount of bytes accounted for the mqueue */ mq_treesize = info->attr.mq_maxmsg *sizeof (struct msg_msg ) + - min_t(unsigned int, info->attr.mq_maxmsg, MQ_PRIO_MAX) * +min (info->attr.mq_maxmsg,MQ_PRIO_MAX ) *sizeof (struct posix_msg_tree_node ); mq_bytes = mq_treesize + (info->attr.mq_maxmsg *
Signature · 1 line
2.39.5
PATCH 34/44 bpf: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- kernel/bpf/core.c | 4 ++-- kernel/bpf/log.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
kernel/bpf/core.c +2 -2
@@ -1081,7 +1081,7 @@ bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,bpf_fill_ill_insns (hdr, size); hdr->size = size; - hole = min_t(unsigned int, size - (proglen + sizeof(*hdr)), + hole =min (size - (proglen +sizeof (*hdr)),PAGE_SIZE -sizeof (*hdr)); start =get_random_u32_below (hole) & ~(alignment -1 );
@@ -1142,7 +1142,7 @@ bpf_jit_binary_pack_alloc(unsigned int proglen, u8 **image_ptr,bpf_fill_ill_insns (*rw_header, size); (*rw_header)->size = size; - hole = min_t(unsigned int, size - (proglen + sizeof(*ro_header)), + hole =min (size - (proglen +sizeof (*ro_header)),BPF_PROG_CHUNK_SIZE -sizeof (*ro_header)); start =get_random_u32_below (hole) & ~(alignment -1 );
kernel/bpf/log.c +1 -1
@@ -79,7 +79,7 @@ void bpf_verifier_vlog(struct bpf_verifier_log *log, const char *fmt,/* check if we have at least something to put into user buf */ new_n =0 ;if (log->end_pos < log->len_total) { - new_n =min_t (u32, log->len_total - log->end_pos, n); + new_n =min (log->len_total - log->end_pos, n); log->kbuf[new_n -1 ] ='\0' ; }
Signature · 1 line
2.39.5
PATCH 35/44 bpf: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- kernel/trace/bpf_trace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/trace/bpf_trace.c +1 -1
@@ -1515,7 +1515,7 @@ BPF_CALL_4(bpf_read_branch_records, struct bpf_perf_event_data_kern *, ctx,if (!buf || (size % br_entry_size !=0 ))return -EINVAL ; - to_copy =min_t (u32, br_stack->nr * br_entry_size, size); + to_copy =min (br_stack->nr * br_entry_size, size);memcpy (buf, br_stack->entries, to_copy);return to_copy;
Signature · 1 line
2.39.5
PATCH 36/44 lib/bucket_locks: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- lib/bucket_locks.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
lib/bucket_locks.c +1 -1
@@ -25,7 +25,7 @@ int __alloc_bucket_spinlocks(spinlock_t **locks, unsigned int *locks_mask,if (cpu_mult) { nr_pcpus = min_t(unsigned int, nr_pcpus, 64UL); - size = min_t(unsigned int, nr_pcpus * cpu_mult, max_size); + size =min (nr_pcpus * cpu_mult, max_size); }else { size = max_size; }
Signature · 1 line
2.39.5
PATCH 37/44 lib/crypto/mpi: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- lib/crypto/mpi/mpicoder.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
lib/crypto/mpi/mpicoder.c +1 -1
@@ -398,7 +398,7 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int nbytes)while (sg_miter_next (&miter)) { buff = miter.addr; - len =min_t (unsigned, miter.length, nbytes); + len =min (miter.length, nbytes); nbytes -= len;for (x =0 ; x < len; x++) {
Signature · 1 line
2.39.5
PATCH 38/44 lib/dynamic_queue_limits: use max() instead of max_t()
In reply to [email protected]From: David Laight <[email protected]>
max_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use max(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to max_t().
--- lib/dynamic_queue_limits.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
lib/dynamic_queue_limits.c +1 -1
@@ -69,7 +69,7 @@ static void dql_check_stall(struct dql *dql, unsigned short stall_thrs) goto dqs_again; dql->stall_cnt++; - dql->stall_max = max_t(unsigned short, dql->stall_max, now - t); + dql->stall_max =max (dql->stall_max, now - t); trace_dql_stall_detected(dql->stall_thrs, now - t, dql->last_reap, dql->history_head,
Signature · 1 line
2.39.5
PATCH 39/44 mm: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' values are small enough that the result is ok.
(Similarly for clamp_t().)
Detected by an extra check added to min_t().
--- mm/gup.c | 4 ++-- mm/memblock.c | 2 +- mm/memory.c | 2 +- mm/percpu.c | 2 +- mm/truncate.c | 3 +-- mm/vmscan.c | 2 +- 6 files changed, 7 insertions(+), 8 deletions(-)
mm/gup.c +2 -2
@@ -237,8 +237,8 @@ static inline struct folio *gup_folio_range_next(struct page *start,unsigned int nr =1 ;if (folio_test_large (folio)) - nr = min_t(unsigned int, npages - i, -folio_nr_pages (folio) -folio_page_idx (folio, next)); + nr =min (npages - i, +folio_nr_pages (folio) -folio_page_idx (folio, next)); *ntails = nr;return folio;
mm/memblock.c +1 -1
@@ -2208,7 +2208,7 @@ static void __init __free_pages_memory(unsigned long start, unsigned long end)* the case. */ if (start) - order =min_t (int,MAX_PAGE_ORDER ,__ffs (start)); + order =min (MAX_PAGE_ORDER ,__ffs (start));else order =MAX_PAGE_ORDER ;
mm/memory.c +1 -1
@@ -2375,7 +2375,7 @@ static int insert_pages(struct vm_area_struct *vma, unsigned long addr,while (pages_to_write_in_pmd) {int pte_idx =0 ; -const int batch_size =min_t (int, pages_to_write_in_pmd,8 ); +const int batch_size =min (pages_to_write_in_pmd,8 ); start_pte =pte_offset_map_lock (mm, pmd, addr, &pte_lock);if (!start_pte) {
mm/percpu.c +1 -1
@@ -1228,7 +1228,7 @@ static int pcpu_alloc_area(struct pcpu_chunk *chunk, int alloc_bits,/* * Search to find a fit. */ - end =min_t (int, start + alloc_bits +PCPU_BITMAP_BLOCK_BITS , + end =umin (start + alloc_bits +PCPU_BITMAP_BLOCK_BITS ,pcpu_chunk_map_bits (chunk)); bit_off =pcpu_find_zero_area (chunk->alloc_map, end, start, alloc_bits, align_mask, &area_off, &area_bits);
mm/truncate.c +1 -2
@@ -849,8 +849,7 @@ void pagecache_isize_extended(struct inode *inode, loff_t from, loff_t to)unsigned int offset, end; offset = from -folio_pos (folio); - end = min_t(unsigned int, to - folio_pos(folio), -folio_size (folio)); + end =umin (to -folio_pos (folio),folio_size (folio));folio_zero_segment (folio, offset, end); }
mm/vmscan.c +1 -1
@@ -3489,7 +3489,7 @@ static struct folio *get_pfn_folio(unsigned long pfn, struct mem_cgroup *memcg,static bool suitable_to_scan (int total,int young) { -int n =clamp_t (int,cache_line_size () /sizeof (pte_t),2 ,8 ); +int n =clamp (cache_line_size () /sizeof (pte_t),2 ,8 );/* suitable if the average number of young PTEs per cacheline is >=1 */ return young * n >= total;
Signature · 1 line
2.39.5
David Hildenbrand (Red Hat)
maintainerRe: [PATCH 39/44] mm: use min() instead of min_t()
Quoted 5 lines · On 11/19/25 23:41, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
I thought using min() was frowned upon and we were supposed to use min_t() instead to make it clear which type we want to use.
Do I misremember or have things changed?
Wasn't there a checkpatch warning that states exactly that?
Signature · 3 lines
Cheers David
Re: [PATCH 39/44] mm: use min() instead of min_t()
Quoted 9 lines · On Thu, 20 Nov 2025 10:20:41 +0100 "David Hildenbrand (Red Hat)"…
Quoted 5 lines · On 11/19/25 23:41, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.I thought using
min()was frowned upon and we were supposed to usemin_t()instead to make it clear which type we want to use.
I'm not sure that was ever true.min_t() is just an accident waiting to happen. (and I found a few of them, the worst are in sched/fair.c)
Most of the min_t() are there because of the rather overzealous type check that used to be in min().
But even then it would really be better to explicitly cast one of the parameters to min(), so min_t(T, a, b) => min(a, (T)b). Then it becomes rather more obvious that min_t(u8, x->m_u8, expr) is going mask off the high bits of 'expr'.
Quoted 3 lines
Do I misremember or have things changed?
Wasn't there a checkpatch warning that states exactly that?
There is one that suggests min_t() - it ought to be nuked. The real fix is to backtrack the types so there isn't an error. min_t() ought to be a 'last resort' and a single cast is better.
With the relaxed checks in min() most of the min_t() can just be replaced by min(), even this is ok:
int len = fun(); if (len < 0) return; count = min(len, sizeof(T));
I did look at the history of min() and min_t().
IIRC some of the networking code had a real function min() with 'unsigned int' arguments.
This was moved to a common header, changed to a #define and had a type added - so min(T, a, b).
Pretty much immediately that was renamed min_t() and min() added that accepted any type - but checked the types of 'a' and 'b' exactly matched.
Code was then changed (over the years) to use min(), but in many cases the types didn't quite match - so min_t() was used a lot.
I keep spotting new commits that pass too small a type to min_t(). So this is the start of a '5 year' campaign to nuke min_t() (et al).
David
Eric Biggers
maintainerRe: [PATCH 39/44] mm: use min() instead of min_t()
Quoted 50 lines · David Laight
Quoted 9 lines · On Thu, 20 Nov 2025 10:20:41 +0100 "David Hildenbrand (Red Hat)"…
Quoted 5 lines · On 11/19/25 23:41, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.I thought using
min()was frowned upon and we were supposed to usemin_t()instead to make it clear which type we want to use.I'm not sure that was ever true.
min_t()is just an accident waiting to happen. (and I found a few of them, the worst are insched/fair.c)Most of the
min_t()are there because of the rather overzealous type check that used to be inmin().
But even then it would really be better to explicitly cast one of the parameters tomin(), somin_t(T, a, b)=>min(a, (T)b). Then it becomes rather more obvious thatmin_t(u8, x->m_u8, expr)is going mask off the high bits of 'expr'.Quoted 3 lines
Do I misremember or have things changed?
Wasn't there a checkpatch warning that states exactly that?
There is one that suggests
min_t()- it ought to be nuked. The real fix is to backtrack the types so there isn't an error.min_t()ought to be a 'last resort' and a single cast is better.With the relaxed checks in
min()most of themin_t()can just be replaced bymin(), even this is ok:int len = fun(); if (len < 0) return; count = min(len, sizeof(T));I did look at the history of
min()andmin_t().
IIRC some of the networking code had a real functionmin()with 'unsigned int' arguments.
This was moved to a common header, changed to a #define and had a type added - somin(T, a, b).
Pretty much immediately that was renamedmin_t()andmin()added that accepted any type - but checked the types of 'a' and 'b' exactly matched.
Code was then changed (over the years) to usemin(), but in many cases the types didn't quite match - somin_t()was used a lot.
Quoted 2 lines
I keep spotting new commits that pass too small a type to
min_t(). So this is the start of a '5 year' campaign to nukemin_t()(et al).
Yes, checkpatch suggests min_t() or max_t() if you cast an argument to min() or max(). Grep for "typecasts on min/max could be min_t/max_t" in scripts/checkpatch.pl.
And historically you could not pass different types to min() and max(), which is why people use min_t() and max_t(). It looks like you fixed that a couple years ago in
https://lore.kernel.org/all/[email protected]/,
which is great! It just takes some time for the whole community to get the message. Also, it seems that checkpatch is in need of an update.
Doing these conversions looks good to me, but unfortunately this is probably the type of thing that shouldn't be a single kernel-wide patch series. They should be sent out per-subsystem.
I suggest also putting a sentence in the commit message that mentions that min() and max() have been updated to accept arguments with different types. (Seeing as historically that wasn't true.) I suggest also being extra clear about when each change is a cleanup vs a fix.
- Eric
David Hildenbrand (Red Hat)
maintainerRe: [PATCH 39/44] mm: use min() instead of min_t()
Quoted 51 lines · Eric Biggers
Quoted 50 lines · David Laight
Quoted 9 lines · On Thu, 20 Nov 2025 10:20:41 +0100 "David Hildenbrand (Red Hat)"…
Quoted 5 lines · On 11/19/25 23:41, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.I thought using
min()was frowned upon and we were supposed to usemin_t()instead to make it clear which type we want to use.I'm not sure that was ever true.
min_t()is just an accident waiting to happen. (and I found a few of them, the worst are insched/fair.c)Most of the
min_t()are there because of the rather overzealous type check that used to be inmin().
But even then it would really be better to explicitly cast one of the parameters tomin(), somin_t(T, a, b)=>min(a, (T)b). Then it becomes rather more obvious thatmin_t(u8, x->m_u8, expr)is going mask off the high bits of 'expr'.Quoted 3 lines
Do I misremember or have things changed?
Wasn't there a checkpatch warning that states exactly that?
There is one that suggests
min_t()- it ought to be nuked. The real fix is to backtrack the types so there isn't an error.min_t()ought to be a 'last resort' and a single cast is better.With the relaxed checks in
min()most of themin_t()can just be replaced bymin(), even this is ok:int len = fun(); if (len < 0) return; count = min(len, sizeof(T));I did look at the history of
min()andmin_t().
IIRC some of the networking code had a real functionmin()with 'unsigned int' arguments.
This was moved to a common header, changed to a #define and had a type added - somin(T, a, b).
Pretty much immediately that was renamedmin_t()andmin()added that accepted any type - but checked the types of 'a' and 'b' exactly matched.
Code was then changed (over the years) to usemin(), but in many cases the types didn't quite match - somin_t()was used a lot.
Quoted 6 lines
Quoted 2 lines
I keep spotting new commits that pass too small a type to
min_t(). So this is the start of a '5 year' campaign to nukemin_t()(et al).Yes, checkpatch suggests
min_t()ormax_t()if you cast an argument tomin()ormax(). Grep for "typecasts on min/max could be min_t/max_t" inscripts/checkpatch.pl.
Right, that's the one I recalled.
Quoted 7 lines
And historically you could not pass different types to
min()andmax(), which is why people usemin_t()andmax_t(). It looks like you fixed that a couple years ago in
https://lore.kernel.org/all/[email protected]/,
which is great! It just takes some time for the whole community to get the message. Also, it seems that checkpatch is in need of an update.
Exactly.
And whenever it comes to such things, I wonder if we want to clearly spell them out somewhere (codying-style): especially, when to use min/max and when to use min_t/max_t.
coding-style currently mentions:
"There are also min() and max() macros that do strict type checking ..." is that also outdated or am I just confused at this point?
Quoted 4 lines
Doing these conversions looks good to me, but unfortunately this is probably the type of thing that shouldn't be a single kernel-wide patch series. They should be sent out per-subsystem.
Agreed!
In particular as there is no need to rush and individual subsystems can just pick it up separately.
Quoted 5 lines
I suggest also putting a sentence in the commit message that mentions that
min()andmax()have been updated to accept arguments with different types. (Seeing as historically that wasn't true.) I suggest also being extra clear about when each change is a cleanup vs a fix.
+1
Signature · 3 lines
Cheers David
Re: [PATCH 39/44] mm: use min() instead of min_t()
In reply to Eric BiggersQuoted 51 lines · On Thu, 20 Nov 2025 23:45:22 +0000 Eric Biggers <[email protected]> wrote:
Quoted 50 lines · David Laight
Quoted 9 lines · On Thu, 20 Nov 2025 10:20:41 +0100 "David Hildenbrand (Red Hat)"…
Quoted 5 lines · On 11/19/25 23:41, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.I thought using
min()was frowned upon and we were supposed to usemin_t()instead to make it clear which type we want to use.I'm not sure that was ever true.
min_t()is just an accident waiting to happen. (and I found a few of them, the worst are insched/fair.c)Most of the
min_t()are there because of the rather overzealous type check that used to be inmin().
But even then it would really be better to explicitly cast one of the parameters tomin(), somin_t(T, a, b)=>min(a, (T)b). Then it becomes rather more obvious thatmin_t(u8, x->m_u8, expr)is going mask off the high bits of 'expr'.Quoted 3 lines
Do I misremember or have things changed?
Wasn't there a checkpatch warning that states exactly that?
There is one that suggests
min_t()- it ought to be nuked. The real fix is to backtrack the types so there isn't an error.min_t()ought to be a 'last resort' and a single cast is better.With the relaxed checks in
min()most of themin_t()can just be replaced bymin(), even this is ok:int len = fun(); if (len < 0) return; count = min(len, sizeof(T));I did look at the history of
min()andmin_t().
IIRC some of the networking code had a real functionmin()with 'unsigned int' arguments.
This was moved to a common header, changed to a #define and had a type added - somin(T, a, b).
Pretty much immediately that was renamedmin_t()andmin()added that accepted any type - but checked the types of 'a' and 'b' exactly matched.
Code was then changed (over the years) to usemin(), but in many cases the types didn't quite match - somin_t()was used a lot.
Quoted 6 lines
Quoted 2 lines
I keep spotting new commits that pass too small a type to
min_t(). So this is the start of a '5 year' campaign to nukemin_t()(et al).Yes, checkpatch suggests
min_t()ormax_t()if you cast an argument tomin()ormax(). Grep for "typecasts on min/max could be min_t/max_t" inscripts/checkpatch.pl.
IMHO that is a really bad suggestion (and always has been). In reality min(a, (T)b) is less likely to be buggy than min_t(T, a, b). Someone will notice that (u16)long_var is likely to be buggy but min_t() is expected to 'do something magic'.
There are a log of examples of 'T_var = min_t(T, T_var, b)' which really needed (typeof (b))T_var rather than (T)b
and T_var = min_t(T, a, b) which just doesn't need a cast at all.
Quoted 6 lines
And historically you could not pass different types to
min()andmax(), which is why people usemin_t()andmax_t(). It looks like you fixed that a couple years ago in
https://lore.kernel.org/all/[email protected]/,
which is great!
I wrote that, and then Linus redid it to avoid some very long lines from nested expansion (with some tree-wide patches that only he could do).
Quoted 6 lines
It just takes some time for the whole community to get
the message. Also, it seems that checkpatch is in need of an update.Doing these conversions looks good to me, but unfortunately this is probably the type of thing that shouldn't be a single kernel-wide patch series. They should be sent out per-subsystem.
In effect it is a list of separate patches, one per subsystem. They just have a common 0/n wrapper.
I wanted to link them together, I guess I could have put a bit more text in the common commit message I pasted into all the commits.
I didn't post the change to minmax.h (apart from a summary in 0/44) because I hadn't even tried to build a 32bit kernel nevery mind an allmodconfig or allyesconfig one.
I spent all yesterday trying to build allyesconfig...
David
Quoted 7 lines
I suggest also putting a sentence in the commit message that mentions that
min()andmax()have been updated to accept arguments with different types. (Seeing as historically that wasn't true.) I suggest also being extra clear about when each change is a cleanup vs a fix.- Eric
Re: [PATCH 39/44] mm: use min() instead of min_t()
In reply to [email protected]I guess you decided to drop all reviewers for the series...?
I do wonder what the aversion to sending to more people is, email for review is flawed but I don't think it's problematic to ensure that people signed up to review everything for maintained files are cc'd...
Quoted 5 lines · On Wed, Nov 19, 2025 at 10:41:35PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
you're changing min_t(int, ...) too? This commit message seems incomplete as a result.
None of the changes you make here seem to have any bearing on reality, so I think the commit message should reflect that this is an entirely pedantic change for the sake of satisfying a check you feel will reveal actual bugs in the future or something?
Commit messages should include actual motivation rather than a theoretical one.
Quoted 7 lines
In this case the 'unsigned long' values are small enough that the result is ok.
(Similarly for
clamp_t().)Detected by an extra check added to
min_t().
In general I really question the value of the check when basically every use here is pointless...?
I guess idea is in future it'll catch some real cases right?
Is this check implemented in this series at all? Because presumably with the cover letter saying you couldn't fix the CFS code etc. you aren't? So it's just laying the groundwork for this?
Quoted 16 lines
Signed-off-by David Laight <[email protected]>--- mm/gup.c | 4 ++-- mm/memblock.c | 2 +- mm/memory.c | 2 +- mm/percpu.c | 2 +- mm/truncate.c | 3 +-- mm/vmscan.c | 2 +- 6 files changed, 7 insertions(+), 8 deletions(-)mm/gup.c +0
-0
mm/gup.c static inline struct folio *gup_folio_range_next(struct page *start, · line 241
Quoted 7 lines
mm/gup.c +2
-2@@ -237,8 +237,8 @@ static inline struct folio *gup_folio_range_next(struct page *start,unsigned int nr =1 ;if (folio_test_large (folio)) - nr = min_t(unsigned int, npages - i, -folio_nr_pages (folio) -folio_page_idx (folio, next)); + nr =min (npages - i, +folio_nr_pages (folio) -folio_page_idx (folio, next));
There's no cases where any of these would discard significant bits. But we ultimately cast to unisnged int anyway (nr) so not sure this achieves anything.
But at the same time I guess no harm.
mm/memblock.c static void __init __free_pages_memory(unsigned long start, unsigned long end) · line 2211
Quoted 13 lines
mm/gup.c +0
-0*ntails = nr;return folio;mm/memblock.c +1
-1@@ -2208,7 +2208,7 @@ static void __init __free_pages_memory(unsigned long start, unsigned long end)* the case. */ if (start) - order =min_t (int,MAX_PAGE_ORDER ,__ffs (start)); + order =min (MAX_PAGE_ORDER ,__ffs (start));
I guess this would already be defaulting to int anyway.
mm/memory.c static int insert_pages(struct vm_area_struct *vma, unsigned long addr, · line 2378
Quoted 13 lines
mm/memblock.c +0
-0else order =MAX_PAGE_ORDER ;mm/memory.c +1
-1@@ -2375,7 +2375,7 @@ static int insert_pages(struct vm_area_struct *vma, unsigned long addr,while (pages_to_write_in_pmd) {int pte_idx =0 ; -const int batch_size =min_t (int, pages_to_write_in_pmd,8 ); +const int batch_size =min (pages_to_write_in_pmd,8 );
Feels like there's just a mistake in pages_to_write_in_pmd being unsigned long?
Again I guess correct because we're not going to even come close to ulong64 issues with a count of pages to write.
Quoted 8 lines
mm/memory.c +0
-0start_pte =pte_offset_map_lock (mm, pmd, addr, &pte_lock);if (!start_pte) {mm/percpu.c +0
-0
mm/percpu.c static int pcpu_alloc_area(struct pcpu_chunk *chunk, int alloc_bits, · line 1232
Quoted 6 lines
mm/percpu.c +1
-1@@ -1228,7 +1228,7 @@ static int pcpu_alloc_area(struct pcpu_chunk *chunk, int alloc_bits,/* * Search to find a fit. */ - end =min_t (int, start + alloc_bits +PCPU_BITMAP_BLOCK_BITS , + end =umin (start + alloc_bits +PCPU_BITMAP_BLOCK_BITS ,pcpu_chunk_map_bits (chunk));
Is it really that useful to use umin() here? I mean in examples above all the values would be positive too. Seems strange to use umin() when everything involves an int?
mm/truncate.c void pagecache_isize_extended(struct inode *inode, loff_t from, loff_t to) · line 852
Quoted 13 lines
mm/percpu.c +0
-0bit_off =pcpu_find_zero_area (chunk->alloc_map, end, start, alloc_bits, align_mask, &area_off, &area_bits);mm/truncate.c +1
-2@@ -849,8 +849,7 @@ void pagecache_isize_extended(struct inode *inode, loff_t from, loff_t to)unsigned int offset, end; offset = from -folio_pos (folio); - end = min_t(unsigned int, to - folio_pos(folio), -folio_size (folio)); + end =umin (to -folio_pos (folio),folio_size (folio));
Again confused about why we choose to use umin() here...
min(loff_t - loff_t, size_t)
so min(long long, unsigned long)
And I guess based on fact we don't expect delta between from and folio start to be larger than a max folio size.
So probably fine.
mm/vmscan.c static struct folio *get_pfn_folio(unsigned long pfn, struct mem_cgroup *memcg, · line 3492
Quoted 13 lines
mm/truncate.c +0
-0folio_zero_segment (folio, offset, end); }mm/vmscan.c +1
-1@@ -3489,7 +3489,7 @@ static struct folio *get_pfn_folio(unsigned long pfn, struct mem_cgroup *memcg,static bool suitable_to_scan (int total,int young) { -int n =clamp_t (int,cache_line_size () /sizeof (pte_t),2 ,8 ); +int n =clamp (cache_line_size () /sizeof (pte_t),2 ,8 );
int, size_t (but a size_t way < INT_MAX), int, int
So seems fine.
Quoted 6 lines
mm/vmscan.c +0
-1/* suitable if the average number of young PTEs per cacheline is >=1 */ return young * n >= total; --2.39.5
Generally the changes look to be correct but pointless.
Lorenzo Stoakes
Reviewed-byRe: [PATCH 39/44] mm: use min() instead of min_t()
Quoted 31 lines · Lorenzo Stoakes
I guess you decided to drop all reviewers for the series...?
I do wonder what the aversion to sending to more people is, email for review is flawed but I don't think it's problematic to ensure that people signed up to review everything for maintained files are cc'd...
Quoted 5 lines · On Wed, Nov 19, 2025 at 10:41:35PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.you're changing
min_t(int, ...)too? This commit message seems incomplete as a result.None of the changes you make here seem to have any bearing on reality, so I think the commit message should reflect that this is an entirely pedantic change for the sake of satisfying a check you feel will reveal actual bugs in the future or something?
Commit messages should include actual motivation rather than a theoretical one.
Quoted 7 lines
In this case the 'unsigned long' values are small enough that the result is ok.
(Similarly for
clamp_t().)Detected by an extra check added to
min_t().
Quoted 11 lines
In general I really question the value of the check when basically every use here is pointless...?
I guess idea is in future it'll catch some real cases right?
Is this check implemented in this series at all? Because presumably with the cover letter saying you couldn't fix the CFS code etc. you aren't? So it's just laying the groundwork for this?
Quoted 2 lines
Signed-off-by David Laight <[email protected]>
I mean I don't see anything wrong here, and on the basis that this will be useful in adding this upcoming check, with the nit about commit msg above, this LGTM so:
Quoted 131 lines
Quoted 14 lines
--- mm/gup.c | 4 ++-- mm/memblock.c | 2 +- mm/memory.c | 2 +- mm/percpu.c | 2 +- mm/truncate.c | 3 +-- mm/vmscan.c | 2 +- 6 files changed, 7 insertions(+), 8 deletions(-)mm/gup.c +0
-0mm/gup.c static inline struct folio *gup_folio_range_next(struct page *start, · line 241
Quoted 7 lines
mm/gup.c +2
-2@@ -237,8 +237,8 @@ static inline struct folio *gup_folio_range_next(struct page *start,unsigned int nr =1 ;if (folio_test_large (folio)) - nr = min_t(unsigned int, npages - i, -folio_nr_pages (folio) -folio_page_idx (folio, next)); + nr =min (npages - i, +folio_nr_pages (folio) -folio_page_idx (folio, next));There's no cases where any of these would discard significant bits. But we ultimately cast to unisnged int anyway (nr) so not sure this achieves anything.
But at the same time I guess no harm.
mm/memblock.c static void __init __free_pages_memory(unsigned long start, unsigned long end) · line 2211
Quoted 13 lines
mm/gup.c +0
-0*ntails = nr;return folio;mm/memblock.c +1
-1@@ -2208,7 +2208,7 @@ static void __init __free_pages_memory(unsigned long start, unsigned long end)* the case. */ if (start) - order =min_t (int,MAX_PAGE_ORDER ,__ffs (start)); + order =min (MAX_PAGE_ORDER ,__ffs (start));I guess this would already be defaulting to int anyway.
mm/memory.c static int insert_pages(struct vm_area_struct *vma, unsigned long addr, · line 2378
Quoted 13 lines
mm/memblock.c +0
-0else order =MAX_PAGE_ORDER ;mm/memory.c +1
-1@@ -2375,7 +2375,7 @@ static int insert_pages(struct vm_area_struct *vma, unsigned long addr,while (pages_to_write_in_pmd) {int pte_idx =0 ; -const int batch_size =min_t (int, pages_to_write_in_pmd,8 ); +const int batch_size =min (pages_to_write_in_pmd,8 );Feels like there's just a mistake in
pages_to_write_in_pmdbeing unsigned long?Again I guess correct because we're not going to even come close to ulong64 issues with a count of pages to write.
Quoted 8 lines
mm/memory.c +0
-0start_pte =pte_offset_map_lock (mm, pmd, addr, &pte_lock);if (!start_pte) {mm/percpu.c +0
-0mm/percpu.c static int pcpu_alloc_area(struct pcpu_chunk *chunk, int alloc_bits, · line 1232
Quoted 6 lines
mm/percpu.c +1
-1@@ -1228,7 +1228,7 @@ static int pcpu_alloc_area(struct pcpu_chunk *chunk, int alloc_bits,/* * Search to find a fit. */ - end =min_t (int, start + alloc_bits +PCPU_BITMAP_BLOCK_BITS , + end =umin (start + alloc_bits +PCPU_BITMAP_BLOCK_BITS ,pcpu_chunk_map_bits (chunk));Is it really that useful to use
umin()here? I mean in examples above all the values would be positive too. Seems strange to useumin()when everything involves an int?mm/truncate.c void pagecache_isize_extended(struct inode *inode, loff_t from, loff_t to) · line 852
Quoted 13 lines
mm/percpu.c +0
-0bit_off =pcpu_find_zero_area (chunk->alloc_map, end, start, alloc_bits, align_mask, &area_off, &area_bits);mm/truncate.c +1
-2@@ -849,8 +849,7 @@ void pagecache_isize_extended(struct inode *inode, loff_t from, loff_t to)unsigned int offset, end; offset = from -folio_pos (folio); - end = min_t(unsigned int, to - folio_pos(folio), -folio_size (folio)); + end =umin (to -folio_pos (folio),folio_size (folio));Again confused about why we choose to use
umin()here...
min(loff_t - loff_t, size_t)so
min(long long, unsigned long)And I guess based on fact we don't expect delta between from and folio start to be larger than a max folio size.
So probably fine.
mm/vmscan.c static struct folio *get_pfn_folio(unsigned long pfn, struct mem_cgroup *memcg, · line 3492
Quoted 13 lines
mm/truncate.c +0
-0folio_zero_segment (folio, offset, end); }mm/vmscan.c +1
-1@@ -3489,7 +3489,7 @@ static struct folio *get_pfn_folio(unsigned long pfn, struct mem_cgroup *memcg,static bool suitable_to_scan (int total,int young) { -int n =clamp_t (int,cache_line_size () /sizeof (pte_t),2 ,8 ); +int n =clamp (cache_line_size () /sizeof (pte_t),2 ,8 );int,
size_t(but asize_tway <INT_MAX), int, intSo seems fine.
Quoted 6 lines
mm/vmscan.c +0
-1/* suitable if the average number of young PTEs per cacheline is >=1 */ return young * n >= total; --2.39.5
Generally the changes look to be correct but pointless.
Re: [PATCH 39/44] mm: use min() instead of min_t()
In reply to Lorenzo StoakesQuoted 5 lines · On Thu, 20 Nov 2025 10:36:16 +0000 Lorenzo Stoakes…
I guess you decided to drop all reviewers for the series...?
I do wonder what the aversion to sending to more people is, email for review is flawed but I don't think it's problematic to ensure that people signed up to review everything for maintained files are cc'd...
Even sending all 44 patches to all the mailing lists was over 5000 emails. Sending to all 124 maintainers and lists is some 50000 emails. And that is just the maintainers, not the reviewers etc.
I don't have access to a mail server that will let me send more than 500 messages/day (the gmail limit is 100).
So each patch was send to the maintainers for the files it contained, that reduced it to just under 400 emails.
Quoted 10 lines · On Wed, Nov 19, 2025 at 10:41:35PM +0000, [email protected] wrote:
Quoted 5 lines
From: David Laight <[email protected]>
min_t(unsigned int, a, b)casts an 'unsigned long' to 'unsigned int'. Usemin(a, b)instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.you're changing
min_t(int, ...)too? This commit message seems incomplete as a result.
Ok, I used the same commit message for most of the 44 patches. The large majority are 'unsigned int' ones.
Quoted 16 lines
None of the changes you make here seem to have any bearing on reality, so I think the commit message should reflect that this is an entirely pedantic change for the sake of satisfying a check you feel will reveal actual bugs in the future or something?
Commit messages should include actual motivation rather than a theoretical one.
Quoted 7 lines
In this case the 'unsigned long' values are small enough that the result is ok.
(Similarly for
clamp_t().)Detected by an extra check added to
min_t().
Quoted 8 lines
In general I really question the value of the check when basically every use here is pointless...?
I guess idea is in future it'll catch some real cases right?
Is this check implemented in this series at all? Because presumably with the cover letter saying you couldn't fix the CFS code etc. you aren't? So it's just laying the groundwork for this?
I could fix the CFS code, but not with a trivial patch.
I also wanted to put the 'fixes' in the first few patches, I didn't realise how bad that code was until I looked again.
(I've also not fixed all the drivers I don't build.)
Quoted 19 lines
Quoted 19 lines
Signed-off-by David Laight <[email protected]>--- mm/gup.c | 4 ++-- mm/memblock.c | 2 +- mm/memory.c | 2 +- mm/percpu.c | 2 +- mm/truncate.c | 3 +-- mm/vmscan.c | 2 +- 6 files changed, 7 insertions(+), 8 deletions(-)mm/gup.c +0
-0@@ -237,8 +237,8 @@ static inline struct folio *gup_folio_range_next(struct page *start,unsigned int nr =1 ;
Quoted 8 lines
mm/gup.c static inline struct folio *gup_folio_range_next(struct page *start, · line 241
Quoted 5 lines
mm/gup.c +2
-2if (folio_test_large (folio)) - nr = min_t(unsigned int, npages - i, -folio_nr_pages (folio) -folio_page_idx (folio, next)); + nr =min (npages - i, +folio_nr_pages (folio) -folio_page_idx (folio, next));There's no cases where any of these would discard significant bits. But we ultimately cast to unisnged int anyway (nr) so not sure this achieves anything.
The (implicit) cast to unsigned int is irrelevant - that happens after the min(). The issue is that 'npages' is 'unsigned long' so can (in theory) be larger than 4G. Ok that would be a 16TB buffer, but someone must have decided that npages might not fit in 32 bits otherwise they wouldn't have used 'unsigned long'.
Quoted 4 lines
But at the same time I guess no harm.
Quoted 1 line
Quoted 14 lines
mm/memblock.c static void __init __free_pages_memory(unsigned long start, unsigned long end) · line 2211
Quoted 12 lines
mm/gup.c +0
-0*ntails = nr;return folio;mm/memblock.c +1
-1@@ -2208,7 +2208,7 @@ static void __init __free_pages_memory(unsigned long start, unsigned long end)* the case. */ if (start) - order =min_t (int,MAX_PAGE_ORDER ,__ffs (start)); + order =min (MAX_PAGE_ORDER ,__ffs (start));I guess this would already be defaulting to int anyway.
Actually that one is also fixed by patch 0001 - which changes the return type of the x86-64 __ffs() to unsigned int.
Which will be why min_t() was used in the first place.
I probably did this edit first.
Quoted 10 lines
Quoted 10 lines
mm/memblock.c +0
-0else order =MAX_PAGE_ORDER ;mm/memory.c +0
-0@@ -2375,7 +2375,7 @@ static int insert_pages(struct vm_area_struct *vma, unsigned long addr,
Quoted 6 lines
mm/memory.c static int insert_pages(struct vm_area_struct *vma, unsigned long addr, · line 2378
Quoted 4 lines
mm/memory.c +1
-1while (pages_to_write_in_pmd) {int pte_idx =0 ; -const int batch_size =min_t (int, pages_to_write_in_pmd,8 ); +const int batch_size =min (pages_to_write_in_pmd,8 );Feels like there's just a mistake in
pages_to_write_in_pmdbeing unsigned long?
Changing that would be a different 'fix'.
Quoted 2 lines
Again I guess correct because we're not going to even come close to ulong64 issues with a count of pages to write.
That fact that the count of pages is small is why the existing code isn't wrong. The patch can't make things worse.
Quoted 35 lines
Quoted 9 lines
mm/memory.c +0
-0start_pte =pte_offset_map_lock (mm, pmd, addr, &pte_lock);if (!start_pte) {mm/percpu.c +0
-0mm/percpu.c static int pcpu_alloc_area(struct pcpu_chunk *chunk, int alloc_bits, · line 1232
Quoted 6 lines
mm/percpu.c +1
-1@@ -1228,7 +1228,7 @@ static int pcpu_alloc_area(struct pcpu_chunk *chunk, int alloc_bits,/* * Search to find a fit. */ - end =min_t (int, start + alloc_bits +PCPU_BITMAP_BLOCK_BITS , + end =umin (start + alloc_bits +PCPU_BITMAP_BLOCK_BITS ,pcpu_chunk_map_bits (chunk));Is it really that useful to use
umin()here? I mean in examples above all the values would be positive too. Seems strange to useumin()when everything involves an int?mm/truncate.c void pagecache_isize_extended(struct inode *inode, loff_t from, loff_t to) · line 852
Quoted 13 lines
mm/percpu.c +0
-0bit_off =pcpu_find_zero_area (chunk->alloc_map, end, start, alloc_bits, align_mask, &area_off, &area_bits);mm/truncate.c +1
-2@@ -849,8 +849,7 @@ void pagecache_isize_extended(struct inode *inode, loff_t from, loff_t to)unsigned int offset, end; offset = from -folio_pos (folio); - end = min_t(unsigned int, to - folio_pos(folio), -folio_size (folio)); + end =umin (to -folio_pos (folio),folio_size (folio));Again confused about why we choose to use
umin()here...
Quoted 3 lines
min(loff_t - loff_t, size_t)so
min(long long, unsigned long)
Which is a signedness error because both are 64bit.min(s64, u32) also reports a signedness error even though u32 is promoted to s64, allowing that would bloat min() somewhat (and it isn't common).
Quoted 3 lines
And I guess based on fact we don't expect delta between from and folio start to be larger than a max folio size.
The problem arises if 'to - folio_pos(folio)' doesn't fit in 32 bits (and its low 32bit are small).
I think that might be possible if truncating a large file. So this might be a real bug.
Quoted 12 lines
So probably fine.
Quoted 9 lines
mm/truncate.c +0
-0folio_zero_segment (folio, offset, end); }mm/vmscan.c +0
-0@@ -3489,7 +3489,7 @@ static struct folio *get_pfn_folio(unsigned long pfn, struct mem_cgroup *memcg,
Quoted 6 lines
mm/vmscan.c static struct folio *get_pfn_folio(unsigned long pfn, struct mem_cgroup *memcg, · line 3492
Quoted 4 lines
mm/vmscan.c +1
-1static bool suitable_to_scan (int total,int young) { -int n =clamp_t (int,cache_line_size () /sizeof (pte_t),2 ,8 ); +int n =clamp (cache_line_size () /sizeof (pte_t),2 ,8 );int,
size_t(but asize_tway <INT_MAX), int, int
Unfortunately even if cache_line_size() is u32, the division makes the result size_t and gcc doesn't detect the value as being 'smaller that it used to be'.
David
Quoted 11 lines
So seems fine.
Quoted 6 lines
mm/vmscan.c +0
-1/* suitable if the average number of young PTEs per cacheline is >=1 */ return young * n >= total; --2.39.5
Generally the changes look to be correct but pointless.
David Hildenbrand (Red Hat)
maintainerRe: [PATCH 39/44] mm: use min() instead of min_t()
Quoted 29 lines
Quoted 20 lines
Quoted 20 lines
Signed-off-by David Laight <[email protected]>--- mm/gup.c | 4 ++-- mm/memblock.c | 2 +- mm/memory.c | 2 +- mm/percpu.c | 2 +- mm/truncate.c | 3 +-- mm/vmscan.c | 2 +- 6 files changed, 7 insertions(+), 8 deletions(-)mm/gup.c +0
-0@@ -237,8 +237,8 @@ static inline struct folio *gup_folio_range_next(struct page *start,unsigned int nr =1 ;Quoted 8 lines
mm/gup.c static inline struct folio *gup_folio_range_next(struct page *start, · line 241
Quoted 5 lines
mm/gup.c +2
-2if (folio_test_large (folio)) - nr = min_t(unsigned int, npages - i, -folio_nr_pages (folio) -folio_page_idx (folio, next)); + nr =min (npages - i, +folio_nr_pages (folio) -folio_page_idx (folio, next));There's no cases where any of these would discard significant bits. But we ultimately cast to unisnged int anyway (nr) so not sure this achieves anything.
Quoted 4 lines
The (implicit) cast to unsigned int is irrelevant - that happens after the
min(). The issue is that 'npages' is 'unsigned long' so can (in theory) be larger than 4G. Ok that would be a 16TB buffer, but someone must have decided that npages might not fit in 32 bits otherwise they wouldn't have used 'unsigned long'.
See commit fa17bcd5f65e ("mm: make folio page count functions return unsigned") why that function used to return "long" instead of "unsigned int" and how we changed it to "unsigned long".
Until that function actually returns something that large might take a while, so no need to worry about that right now.
Signature · 3 lines
Cheers David
Re: [PATCH 39/44] mm: use min() instead of min_t()
Quoted 33 lines · On Thu, 20 Nov 2025 14:42:24 +0100 "David Hildenbrand (Red Hat)"…
Quoted 28 lines
Quoted 19 lines
Quoted 19 lines
Signed-off-by David Laight <[email protected]>--- mm/gup.c | 4 ++-- mm/memblock.c | 2 +- mm/memory.c | 2 +- mm/percpu.c | 2 +- mm/truncate.c | 3 +-- mm/vmscan.c | 2 +- 6 files changed, 7 insertions(+), 8 deletions(-)mm/gup.c +0
-0@@ -237,8 +237,8 @@ static inline struct folio *gup_folio_range_next(struct page *start,unsigned int nr =1 ;Quoted 8 lines
mm/gup.c static inline struct folio *gup_folio_range_next(struct page *start, · line 241
Quoted 5 lines
mm/gup.c +2
-2if (folio_test_large (folio)) - nr = min_t(unsigned int, npages - i, -folio_nr_pages (folio) -folio_page_idx (folio, next)); + nr =min (npages - i, +folio_nr_pages (folio) -folio_page_idx (folio, next));There's no cases where any of these would discard significant bits. But we ultimately cast to unisnged int anyway (nr) so not sure this achieves anything.
Quoted 4 lines
The (implicit) cast to unsigned int is irrelevant - that happens after the
min(). The issue is that 'npages' is 'unsigned long' so can (in theory) be larger than 4G. Ok that would be a 16TB buffer, but someone must have decided that npages might not fit in 32 bits otherwise they wouldn't have used 'unsigned long'.
Quoted 6 lines
See commit
fa17bcd5f65e("mm: make folio page count functions return unsigned") why that function used to return "long" instead of "unsigned int" and how we changed it to "unsigned long".Until that function actually returns something that large might take a while, so no need to worry about that right now.
Except that it gives a false positive on a compile-time test that finds a few real bugs.
I've been (slowly) fixing 'allmodconfig' and found 'goodies' like:
min_t(u32, MAX_UINT, expr) and min_t(u8, expr, 255)
Pretty much all the min_t(unsigned xxx) that compile when changed to min() are safe changes and might fix an obscure bug.
Probably 99% make no difference.
So I'd like to get rid of the ones that make no difference.
David
David Hildenbrand (Red Hat)
maintainerRe: [PATCH 39/44] mm: use min() instead of min_t()
Quoted 43 lines · David Laight
Quoted 33 lines · On Thu, 20 Nov 2025 14:42:24 +0100 "David Hildenbrand (Red Hat)"…
Quoted 28 lines
Quoted 19 lines
Quoted 19 lines
Signed-off-by David Laight <[email protected]>--- mm/gup.c | 4 ++-- mm/memblock.c | 2 +- mm/memory.c | 2 +- mm/percpu.c | 2 +- mm/truncate.c | 3 +-- mm/vmscan.c | 2 +- 6 files changed, 7 insertions(+), 8 deletions(-)mm/gup.c +0
-0@@ -237,8 +237,8 @@ static inline struct folio *gup_folio_range_next(struct page *start,unsigned int nr =1 ;Quoted 8 lines
mm/gup.c static inline struct folio *gup_folio_range_next(struct page *start, · line 241
Quoted 5 lines
mm/gup.c +2
-2if (folio_test_large (folio)) - nr = min_t(unsigned int, npages - i, -folio_nr_pages (folio) -folio_page_idx (folio, next)); + nr =min (npages - i, +folio_nr_pages (folio) -folio_page_idx (folio, next));There's no cases where any of these would discard significant bits. But we ultimately cast to unisnged int anyway (nr) so not sure this achieves anything.
Quoted 4 lines
The (implicit) cast to unsigned int is irrelevant - that happens after the
min(). The issue is that 'npages' is 'unsigned long' so can (in theory) be larger than 4G. Ok that would be a 16TB buffer, but someone must have decided that npages might not fit in 32 bits otherwise they wouldn't have used 'unsigned long'.Quoted 6 lines
See commit
fa17bcd5f65e("mm: make folio page count functions return unsigned") why that function used to return "long" instead of "unsigned int" and how we changed it to "unsigned long".Until that function actually returns something that large might take a while, so no need to worry about that right now.
Quoted 8 lines
Except that it gives a false positive on a compile-time test that finds a few real bugs.
I've been (slowly) fixing 'allmodconfig' and found 'goodies' like:
min_t(u32, MAX_UINT, expr) and min_t(u8, expr, 255)
:)
Quoted 5 lines
Pretty much all the
min_t(unsigned xxx)that compile when changed tomin()are safe changes and might fix an obscure bug.
Probably 99% make no difference.So I'd like to get rid of the ones that make no difference.
No objection from my side if using min() is the preferred way now and introduces no observable changes.
Signature · 3 lines
Cheers David
PATCH 40/44 net: Don't pass bitfields to max_t()
In reply to [email protected]From: David Laight <[email protected]>
It is invalid to use sizeof() or typeof() in bitfields which stops them being passed to max().
This has been fixed by using max_t().
I want to add some checks to max_t() to detect cases where the cast discards non-zero high bits - which uses sizeof().
So add 0 to the bitfield (converting it to int) then use max().
--- include/net/tcp_ecn.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
include/net/tcp_ecn.h +2 -3
@@ -196,7 +196,7 @@ static inline void tcp_accecn_opt_demand_min(struct sock *sk,struct tcp_sock *tp =tcp_sk (sk);u8 opt_demand; - opt_demand =max_t (u8, opt_demand_min, tp->accecn_opt_demand); + opt_demand =max (opt_demand_min, tp->accecn_opt_demand +0 ); tp->accecn_opt_demand = opt_demand; }
@@ -303,8 +303,7 @@ static inline void tcp_ecn_received_counters(struct sock *sk,u32 bytes_mask =GENMASK_U32 (31 ,22 ); tp->received_ecn_bytes[ecnfield -1 ] += len; - tp->accecn_minlen =max_t (u8, tp->accecn_minlen, - minlen); + tp->accecn_minlen =max (tp->accecn_minlen +0 , minlen);/* Send AccECN option at least once per 2^22-byte * increase in any ECN byte counter.
Signature · 1 line
2.39.5
PATCH 41/44 net/core: Change loop conditions so min() can be used
In reply to [email protected]From: David Laight <[email protected]>
Loops like:
int copied = ...;
...
while (copied) {
use = min_t(type, copied, PAGE_SIZE - offset);
...
copied -= 0;
}
can be converted to a plain min() if the comparison is changed to:
while (copied > 0) {This removes any chance of high bits being discded by min_t(). (In the case above PAGE_SIZE is 64bits so the 'int' cast is safe, but there are plenty of cases where the check shows up bugs.)
--- net/core/datagram.c | 6 +++--- net/core/skmsg.c | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)
net/core/datagram.c +3 -3
@@ -664,8 +664,8 @@ int zerocopy_fill_skb_from_iter(struct sk_buff *skb, head =compound_head (pages[n]); order =compound_order (head); -for (refs =0 ; copied !=0 ; start =0 ) { -int size =min_t (int, copied,PAGE_SIZE - start); +for (refs =0 ; copied >0 ; start =0 ) { +int size =min (copied,PAGE_SIZE - start);if (pages[n] - head > (1UL << order) -1 ) { head =compound_head (pages[n]);
@@ -783,7 +783,7 @@ EXPORT_SYMBOL(__zerocopy_sg_from_iter);*/ int zerocopy_sg_from_iter (struct sk_buff *skb,struct iov_iter *from) { -int copy =min_t (int,skb_headlen (skb),iov_iter_count (from)); +int copy =min (skb_headlen (skb),iov_iter_count (from));/* copy up to skb headlen */ if (skb_copy_datagram_from_iter (skb,0 , from, copy))
net/core/skmsg.c +2 -2
@@ -335,8 +335,8 @@ int sk_msg_zerocopy_from_iter(struct sock *sk, struct iov_iter *from, bytes -= copied; msg->sg.size += copied; -while (copied) { - use =min_t (int, copied,PAGE_SIZE - offset); +while (copied >0 ) { + use =min (copied,PAGE_SIZE - offset);sg_set_page (&msg->sg.data[msg->sg.end], pages[i], use, offset);sg_unmark_end (&msg->sg.data[msg->sg.end]);
Signature · 1 line
2.39.5
PATCH 42/44 net: use min() instead of min_t()
In reply to [email protected]From: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'. Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long' and so cannot discard significant bits.
In this case the 'unsigned long' value is small enough that the result is ok.
Detected by an extra check added to min_t().
--- net/core/net-sysfs.c | 3 +-- net/ipv4/fib_trie.c | 2 +- net/ipv4/tcp_input.c | 4 ++-- net/ipv4/tcp_output.c | 5 ++--- net/ipv4/tcp_timer.c | 4 ++-- net/ipv6/addrconf.c | 8 ++++---- net/ipv6/ndisc.c | 5 ++--- net/packet/af_packet.c | 2 +- net/unix/af_unix.c | 4 ++-- 9 files changed, 17 insertions(+), 20 deletions(-)
net/core/net-sysfs.c +1 -2
@@ -985,8 +985,7 @@ static int netdev_rx_queue_set_rps_mask(struct netdev_rx_queue *queue,struct rps_map *old_map, *map;int cpu, i; - map = kzalloc(max_t(unsigned int, -RPS_MAP_SIZE (cpumask_weight (mask)),L1_CACHE_BYTES ), + map =kzalloc (max (RPS_MAP_SIZE (cpumask_weight (mask)),L1_CACHE_BYTES ),GFP_KERNEL );if (!map)return -ENOMEM ;
net/ipv4/fib_trie.c +1 -1
@@ -710,7 +710,7 @@ static unsigned char update_suffix(struct key_vector *tn)* tn->pos + tn->bits, the second highest node will have a suffix * length at most of tn->pos + tn->bits - 1 */ - slen_max = min_t(unsigned char, tn->pos + tn->bits - 1, tn->slen); + slen_max =min (tn->pos + tn->bits -1 , tn->slen);/* search though the list of children looking for nodes that might * have a suffix greater than the one we currently have. This is
net/ipv4/tcp_input.c +2 -2
@@ -2870,7 +2870,7 @@ static void tcp_mtup_probe_success(struct sock *sk) val = (u64 )tcp_snd_cwnd (tp) *tcp_mss_to_mtu (sk, tp->mss_cache);do_div (val, icsk->icsk_mtup.probe_size);DEBUG_NET_WARN_ON_ONCE ((u32 )val != val); -tcp_snd_cwnd_set (tp,max_t (u32,1U , val)); +tcp_snd_cwnd_set (tp,max (1 , val)); tp->snd_cwnd_cnt =0 ; tp->snd_cwnd_stamp = tcp_jiffies32;
@@ -3323,7 +3323,7 @@ void tcp_rearm_rto(struct sock *sk)/* delta_us may not be positive if the socket is locked * when the retrans timer fires and is rescheduled. */ - rto =usecs_to_jiffies (max_t (int, delta_us,1 )); + rto =usecs_to_jiffies (max (delta_us,1 )); }tcp_reset_xmit_timer (sk,ICSK_TIME_RETRANS , rto, true); }
net/ipv4/tcp_output.c +2 -3
@@ -3076,7 +3076,7 @@ bool tcp_schedule_loss_probe(struct sock *sk, bool advancing_rto) jiffies_to_usecs(inet_csk(sk)->icsk_rto) :tcp_rto_delta_us (sk);/* How far in future is RTO? */ if (rto_delta_us >0 ) - timeout =min_t (u32, timeout,usecs_to_jiffies (rto_delta_us)); + timeout =min (timeout,usecs_to_jiffies (rto_delta_us));tcp_reset_xmit_timer (sk,ICSK_TIME_LOSS_PROBE , timeout, true);return true;
@@ -4382,8 +4382,7 @@ void tcp_send_delayed_ack(struct sock *sk)* directly. */ if (tp->srtt_us) { -int rtt =max_t (int,usecs_to_jiffies (tp->srtt_us >>3 ), -TCP_DELACK_MIN ); +int rtt =max (usecs_to_jiffies (tp->srtt_us >>3 ),TCP_DELACK_MIN );if (rtt < max_ato) max_ato = rtt;
net/ipv4/tcp_timer.c +2 -2
@@ -43,7 +43,7 @@ static u32 tcp_clamp_rto_to_user_timeout(const struct sock *sk)if (remaining <=0 )return 1 ;/* user timeout has passed; fire ASAP */ -return min_t (u32, icsk->icsk_rto,msecs_to_jiffies (remaining)); +return min (icsk->icsk_rto,msecs_to_jiffies (remaining)); }u32 tcp_clamp_probe0_to_user_timeout (const struct sock *sk,u32 when)
@@ -504,7 +504,7 @@ static bool tcp_rtx_probe0_timed_out(const struct sock *sk,*/ if (rtx_delta > user_timeout)return true; - timeout =min_t (u32, timeout,msecs_to_jiffies (user_timeout)); + timeout =umin (timeout,msecs_to_jiffies (user_timeout)); }/* Note: timer interrupt might have been delayed by at least one jiffy, * and tp->rcv_tstamp might very well have been written recently.
net/ipv6/addrconf.c +4 -4
@@ -1422,11 +1422,11 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, bool block) if_public_preferred_lft = ifp->prefered_lft;memset (&cfg,0 ,sizeof (cfg)); - cfg.valid_lft =min_t (__u32, ifp->valid_lft, -READ_ONCE (idev->cnf.temp_valid_lft) + age); + cfg.valid_lft =min (ifp->valid_lft, +READ_ONCE (idev->cnf.temp_valid_lft) + age); cfg.preferred_lft = cnf_temp_preferred_lft + age - idev->desync_factor; - cfg.preferred_lft =min_t (__u32, if_public_preferred_lft, cfg.preferred_lft); - cfg.preferred_lft =min_t (__u32, cfg.valid_lft, cfg.preferred_lft); + cfg.preferred_lft =min (if_public_preferred_lft, cfg.preferred_lft); + cfg.preferred_lft =min (cfg.valid_lft, cfg.preferred_lft); cfg.plen = ifp->prefix_len; tmp_tstamp = ifp->tstamp;
net/ipv6/ndisc.c +2 -3
@@ -1731,9 +1731,8 @@ void ndisc_send_redirect(struct sk_buff *skb, const struct in6_addr *target)neigh_release (neigh); } - rd_len = min_t(unsigned int, -IPV6_MIN_MTU -sizeof (struct ipv6hdr ) -sizeof (*msg) - optlen, - skb->len +8 ); + rd_len =min (IPV6_MIN_MTU -sizeof (struct ipv6hdr ) -sizeof (*msg) - optlen, + skb->len +8 ); rd_len &= ~0x7 ; optlen += rd_len;
net/packet/af_packet.c +1 -1
@@ -3015,7 +3015,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) hlen =LL_RESERVED_SPACE (dev); tlen = dev->needed_tailroom; linear =__virtio16_to_cpu (vio_le (), vnet_hdr.hdr_len); - linear =max (linear,min_t (int, len, dev->hard_header_len)); + linear =max (linear,min (len, dev->hard_header_len)); skb =packet_alloc_skb (sk, hlen + tlen, hlen, len, linear, msg->msg_flags &MSG_DONTWAIT , &err);if (skb ==NULL )
net/unix/af_unix.c +2 -2
@@ -2448,7 +2448,7 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,/* allow fallback to order-0 allocations */ size =min_t (int, size,SKB_MAX_HEAD (0 ) +UNIX_SKB_FRAGS_SZ ); - data_len =max_t (int,0 , size -SKB_MAX_HEAD (0 )); + data_len =max (0 , size - (int )SKB_MAX_HEAD (0 )); data_len =min_t (size_t, size,PAGE_ALIGN (data_len));
@@ -3054,7 +3054,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state, sunaddr =NULL ; } - chunk = min_t(unsigned int, unix_skb_len(skb) - skip, size); + chunk =min (unix_skb_len (skb) - skip, size); chunk = state->recv_actor (skb, skip, chunk, state);if (chunk <0 ) {if (copied ==0 )
Signature · 1 line
2.39.5
PATCH 43/44 net/netlink: Use umin() to avoid min_t(int, ...) discarding high bits
In reply to [email protected]From: David Laight <[email protected]>
The scan limit in genl_allocate_reserve_groups() is:
min_t(int, id + n_groups, mc_groups_longs * BITS_PER_LONG);
While 'id' and 'n_groups' are both 'int', 'mc_groups_longs' is 'unsigned long' (BITS_PER_LONG is 'int').
These inconsistent types (all the values are small and non-negative) means that a simple min() fails.
When checks for masking high bits are added to min_t() that also fails. Instead use umin() so safely convert all the values to unsigned.
Move the limit calculation outside the loop for efficiency and readability.
--- net/netlink/genetlink.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
net/netlink/genetlink.c +4 -5
@@ -395,10 +395,11 @@ static unsigned int genl_op_iter_idx(struct genl_op_iter *iter)return iter->cmd_idx; } -static int genl_allocate_reserve_groups (int n_groups,int *first_id) +static noinline_for_stack int genl_allocate_reserve_groups(int n_groups, int *first_id) {unsigned long *new_groups;int start =0 ; +int limit;int i;int id;bool fits;
@@ -414,10 +415,8 @@ static int genl_allocate_reserve_groups(int n_groups, int *first_id) start); fits = true; -for (i = id; - i <min_t (int, id + n_groups, - mc_groups_longs *BITS_PER_LONG ); - i++) { + limit =umin (id + n_groups, mc_groups_longs *BITS_PER_LONG ); +for (i = id; i < limit; i++) {if (test_bit (i, mc_groups)) { start = i; fits = false;
Signature · 1 line
2.39.5
PATCH 44/44 net/mptcp: Change some dubious min_t(int, ...) to min()
In reply to [email protected]From: David Laight <[email protected]>
There are two:
min_t(int, xxx, mptcp_wnd_end(msk) - msk->snd_nxt);
Both mptcp_wnd_end(msk) and msk->snd_nxt are u64, their difference (aka the window size) might be limited to 32 bits - but that isn't knowable from this code.
So checks being added to min_t() detect the potential discard of significant bits.
Provided the 'avail_size' and return of mptcp_check_allowed_size() are changed to an unsigned type (size_t matches the type the caller uses) both min_t() can be changed to min().
--- net/mptcp/protocol.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
net/mptcp/protocol.c +4 -4
@@ -1123,8 +1123,8 @@ struct mptcp_sendmsg_info {bool data_lock_held; }; -static int mptcp_check_allowed_size (const struct mptcp_sock *msk,struct sock *ssk, -u64 data_seq,int avail_size) +static size_t mptcp_check_allowed_size (const struct mptcp_sock *msk,struct sock *ssk, +u64 data_seq,size_t avail_size) {u64 window_end =mptcp_wnd_end (msk);u64 mptcp_snd_wnd;
@@ -1133,7 +1133,7 @@ static int mptcp_check_allowed_size(const struct mptcp_sock *msk, struct sock *sreturn avail_size; mptcp_snd_wnd = window_end - data_seq; - avail_size = min_t(unsigned int, mptcp_snd_wnd, avail_size); + avail_size =min (mptcp_snd_wnd, avail_size);if (unlikely (tcp_sk (ssk)->snd_wnd < mptcp_snd_wnd)) {tcp_sk (ssk)->snd_wnd =min_t (u64,U32_MAX , mptcp_snd_wnd);
@@ -1477,7 +1477,7 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)if (!ssk || !sk_stream_memory_free (ssk))return NULL ; - burst =min_t (int,MPTCP_SEND_BURST_SIZE ,mptcp_wnd_end (msk) - msk->snd_nxt); + burst =min (MPTCP_SEND_BURST_SIZE ,mptcp_wnd_end (msk) - msk->snd_nxt); wmem =READ_ONCE (ssk->sk_wmem_queued);if (!burst)return ssk;
Signature · 1 line
2.39.5
Re: [PATCH 44/44] net/mptcp: Change some dubious min_t(int, ...) to min()
Hi David,
Quoted 10 lines · On 19/11/2025 23:41, [email protected] wrote:
From: David Laight <[email protected]>
There are two:
min_t(int, xxx, mptcp_wnd_end(msk) - msk->snd_nxt);Both
mptcp_wnd_end(msk)andmsk->snd_nxtare u64, their difference (aka the window size) might be limited to 32 bits - but that isn't knowable from this code.
So checks being added tomin_t()detect the potential discard of significant bits.
Quoted 3 lines
Provided the '
avail_size' and return ofmptcp_check_allowed_size()are changed to an unsigned type (size_tmatches the type the caller uses) bothmin_t()can be changed tomin().
Thank you for the patch!
I'm not sure what the status on your side: I don't know if you still plan to send a specific series for all the modifications in the net, but just in case, I just applied your patch in the MPTCP tree. I removed the "net/" prefix from the subject. I will send this patch with others for including in the net-next tree later on if you didn't do that in between.
Cheers,
Matt
Signature · 1 line
Sponsored by the NGI0 Core fund.
Re: [PATCH 44/44] net/mptcp: Change some dubious min_t(int, ...) to min()
Quoted 19 lines · On Thu, 18 Dec 2025 18:33:26 +0100 Matthieu Baerts <[email protected]> wrote:
Hi David,
Quoted 10 lines · On 19/11/2025 23:41, [email protected] wrote:
From: David Laight <[email protected]>
There are two:
min_t(int, xxx, mptcp_wnd_end(msk) - msk->snd_nxt);Both
mptcp_wnd_end(msk)andmsk->snd_nxtare u64, their difference (aka the window size) might be limited to 32 bits - but that isn't knowable from this code.
So checks being added tomin_t()detect the potential discard of significant bits.Quoted 3 lines
Provided the '
avail_size' and return ofmptcp_check_allowed_size()are changed to an unsigned type (size_tmatches the type the caller uses) bothmin_t()can be changed tomin().Thank you for the patch!
Quoted 7 lines
Reviewed-by Matthieu Baerts (NGI0) <[email protected]>I'm not sure what the status on your side: I don't know if you still plan to send a specific series for all the modifications in the net, but just in case, I just applied your patch in the MPTCP tree. I removed the "
net/" prefix from the subject. I will send this patch with others for including in the net-next tree later on if you didn't do that in between.
I'll go through them again at some point.
I'll check against 'next' (but probably not net-next).
I actually need to look at the ones that seemed like real bugs when I did an allmodconfig build - that got to over 200 patches to get 'clean'.
It would be nice to get rid of a lot of the min_t(), but I might try to attack the dubious ones rather than the ones that appear to make no difference.
I might propose some extra checks in minmax.h that would break W=1 builds. Detecting things like min_t(u8, u32_value, 0xff) where the cast makes the comparison always succeed.
In reality any calls with casts to u8 and u16 are 'dubious'.
That and changing checkpatch.pl to not suggest min_t() at all, and to reject the more dubious uses.
After all with:
min(x, (int)y) it is clear to the reader that 'y' is being possibly truncated and converted to a signed value, but with: min_t(int, x, y) you don't know which value needed the cast (and the line isn't even shorter). But what I've found all to often is actually: a = min_t(typeof(a), x, y); and the similar: x = min_t(typeof(x), x, y);
where the type of the result is used and high bits get discarded.
I've just been trying to build with #define clamp_val clamp. That requires a few minor changes and I'm pretty sure shows up a real bug.
David
Quoted 3 lines
Cheers,
Matt
Matthieu Baerts
maintainerRe: [PATCH 44/44] net/mptcp: Change some dubious min_t(int, ...) to min()
Hi David,
Thank you for your reply!
Quoted 24 lines · David Laight
Quoted 21 lines · On Thu, 18 Dec 2025 18:33:26 +0100 Matthieu Baerts <[email protected]> wrote:
Hi David,
Quoted 10 lines · On 19/11/2025 23:41, [email protected] wrote:
From: David Laight <[email protected]>
There are two:
min_t(int, xxx, mptcp_wnd_end(msk) - msk->snd_nxt);Both
mptcp_wnd_end(msk)andmsk->snd_nxtare u64, their difference (aka the window size) might be limited to 32 bits - but that isn't knowable from this code.
So checks being added tomin_t()detect the potential discard of significant bits.Quoted 3 lines
Provided the '
avail_size' and return ofmptcp_check_allowed_size()are changed to an unsigned type (size_tmatches the type the caller uses) bothmin_t()can be changed tomin().Thank you for the patch!
Reviewed-by Matthieu Baerts (NGI0) <[email protected]>
Quoted 7 lines
Quoted 5 lines
I'm not sure what the status on your side: I don't know if you still plan to send a specific series for all the modifications in the net, but just in case, I just applied your patch in the MPTCP tree. I removed the "
net/" prefix from the subject. I will send this patch with others for including in the net-next tree later on if you didn't do that in between.I'll go through them again at some point.
Great, thank you!
Quoted 1 line
I'll check against 'next' (but probably not net-next).
net-next is in linux-next, so that should be fine.
Quoted 24 lines
I actually need to look at the ones that seemed like real bugs when I did an allmodconfig build - that got to over 200 patches to get 'clean'.
It would be nice to get rid of a lot of the
min_t(), but I might try to attack the dubious ones rather than the ones that appear to make no difference.I might propose some extra checks in
minmax.hthat would break W=1 builds. Detecting things likemin_t(u8, u32_value, 0xff)where the cast makes the comparison always succeed.
In reality any calls with casts to u8 and u16 are 'dubious'.That and changing checkpatch.pl to not suggest
min_t()at all, and to reject the more dubious uses.
After all with:min(x, (int)y) it is clear to the reader that 'y' is being possibly truncated and converted to a signed value, but with: min_t(int, x, y) you don't know which value needed the cast (and the line isn't even shorter). But what I've found all to often is actually: a = min_t(typeof(a), x, y); and the similar: x = min_t(typeof(x), x, y);
Quoted 1 line
where the type of the result is used and high bits get discarded.
Good idea to add extra checks and prevent future issues!
Quoted 3 lines
I've just been trying to build with #define
clamp_valclamp. That requires a few minor changes and I'm pretty sure shows up a real bug.
Thank you for looking at that!
Cheers,
Matt
Signature · 1 line
Sponsored by the NGI0 Core fund.
Jakub Kicinski
maintainerQuoted 4 lines · On Wed, 19 Nov 2025 22:40:56 +0000 [email protected] wrote:
I've had to trim the 124 maintainers/lists that get_maintainer.pl finds from 124 to under 100 to be able to send the cover letter.
The individual patches only go to the addresses found for the associated files. That reduces the number of emails to a less unsane number.
Please split the networking (9?) patches out to a separate series. It will help you with the CC list, and help us to get this applied..
Quoted 15 lines · On Wed, Nov 19, 2025 at 10:40:56PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
It in not uncommon for code to use
min_t(uint, a, b)when one of a or b is 64bit and can have a value that is larger than 2^32;
This is particularly prevelant with:uint_var = min_t(uint, uint_var, uint64_expression);Casts to u8 and u16 are very likely to discard significant bits.
These can be detected at compile time by changing
min_t(), for example:#define CHECK_SIZE(fn, type, val) \ BUILD_BUG_ON_MSG(sizeof (val) > sizeof (type) && \ !statically_true(((val) >> 8 * (sizeof (type) - 1)) < 256), \ fn "() significant bits of '" #val "' may be discarded")
Quoted 6 lines
#define min_t(type, x, y) ({ \ CHECK_SIZE("min_t", type, x); \ CHECK_SIZE("min_t", type, y); \ __cmp_once(min, type, x, y); })(and similar changes to
max_t()andclamp_t().)
Have we made sure that the introduction of these don't cause a combinatorial explosion like previous min()/max() changes did?
Quoted 8 lines
This shows up some real bugs, some unlikely bugs and some false positives. In most cases both arguments are unsigned type (just different ones) and
min_t()can just be replaced bymin().The patches are all independant and are most of the ones needed to get the x86-64 kernel I build to compile.
I've not tried building an allyesconfig or allmodconfig kernel.
Well I have a beefy box at my disposal so tried thiese for you :)
Both allyesconfig & allmodconfig works fine for x86-64 (I tried both for good measure)
Quoted 8 lines
I've also not included the patch to
minmax.hitself.I've tried to put the patches that actually fix things first. The last one is 0009.
I gave up on fixing
sched/fair.c- it is too broken for a single patch! The patch fornet/ipv4/tcp.cis also absent becausedo_tcp_getsockopt()needs multiple/larger changes to make it 'sane'.
I guess this isn't broken per se there just retain min_t()/max_t() right?
Quoted 135 lines
I've had to trim the 124 maintainers/lists that get_maintainer.pl finds from 124 to under 100 to be able to send the cover letter.
The individual patches only go to the addresses found for the associated files. That reduces the number of emails to a less unsane number.David Laight (44):
x86/asm/bitops: Change the return type ofvariable__ffs()to unsigned
int
ext4: Fix saturation of 64bit inode times for old filesystems
perf: Fix branch stack callchain limit
io_uring/net: Change some dubiousmin_t()
ipc/msg: Fix saturation of percpu counts inmsgctl_info()
bpf: Verifier, remove some unusual uses ofmin_t()andmax_t()net/core/flow_dissector: Fix cap of__skb_flow_dissect()return value.
net: ethtool: Usemin3()instead of nestedmin_t(u16,...)
ipv6:__ip6_append_data()don't abusemax_t()casts x86/crypto:ctr_crypt()usemin()instead ofmin_t()arch/x96/kvm: usemin()instead ofmin_t()
block: usemin()instead ofmin_t()
drivers/acpi: usemin()instead ofmin_t()drivers/char/hw_random: usemin3()instead of nestedmin_t()drivers/char/tpm: usemin()instead ofmin_t()drivers/crypto/ccp: usemin()instead ofmin_t()drivers/cxl: usemin()instead ofmin_t()
drivers/gpio: usemin()instead ofmin_t()drivers/gpu/drm/amd: usemin()instead ofmin_t()drivers/i2c/busses: usemin()instead ofmin_t()drivers/net/ethernet/realtek: usemin()instead ofmin_t()drivers/nvme: usemin()instead ofmin_t()
arch/x86/mm: usemin()instead ofmin_t()
drivers/nvmem: usemin()instead ofmin_t()
drivers/pci: usemin()instead ofmin_t()
drivers/scsi: usemin()instead ofmin_t()
drivers/tty/vt: useumin()instead ofmin_t(u16, ...)for row/col
limits
drivers/usb/storage: usemin()instead ofmin_t()drivers/xen: usemin()instead ofmin_t()
fs: usemin()orumin()instead ofmin_t()
block:bvec.h: usemin()instead ofmin_t()
nodemask: usemin()instead ofmin_t()
ipc: usemin()instead ofmin_t()
bpf: usemin()instead ofmin_t()
bpf_trace: usemin()instead ofmin_t()
lib/bucket_locks: usemin()instead ofmin_t()lib/crypto/mpi: usemin()instead ofmin_t()lib/dynamic_queue_limits: usemax()instead ofmax_t()
mm: usemin()instead ofmin_t()
net: Don't pass bitfields tomax_t()
net/core: Change loop conditions somin()can be used
net: usemin()instead ofmin_t()
net/netlink: Useumin()to avoidmin_t(int, ...)discarding high bits net/mptcp: Change some dubiousmin_t(int, ...)tomin()arch/x86/crypto/aesni-intel_glue.c | 3 +- arch/x86/include/asm/bitops.h | 18 +++++------- arch/x86/kvm/emulate.c | 3 +- arch/x86/kvm/lapic.c | 2 +- arch/x86/kvm/mmu/mmu.c | 2 +- arch/x86/mm/pat/set_memory.c | 12 ++++---- block/blk-iocost.c | 6 ++-- block/blk-settings.c | 2 +- block/partitions/efi.c | 3 +- drivers/acpi/property.c | 2 +- drivers/char/hw_random/core.c | 2 +- drivers/char/tpm/tpm1-cmd.c | 2 +- drivers/char/tpm/tpm_tis_core.c | 4 +-- drivers/crypto/ccp/ccp-dev.c | 2 +- drivers/cxl/core/mbox.c | 2 +- drivers/gpio/gpiolib-acpi-core.c | 2 +- .../gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c | 4 +-- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- drivers/i2c/busses/i2c-designware-master.c | 2 +- drivers/net/ethernet/realtek/r8169_main.c | 3 +- drivers/nvme/host/pci.c | 3 +- drivers/nvme/host/zns.c | 3 +- drivers/nvmem/core.c | 2 +- drivers/pci/probe.c | 3 +- drivers/scsi/hosts.c | 2 +- drivers/tty/vt/selection.c | 9 +++--- drivers/usb/storage/protocol.c | 3 +- drivers/xen/grant-table.c | 2 +- fs/buffer.c | 2 +- fs/exec.c | 2 +- fs/ext4/ext4.h | 2 +- fs/ext4/mballoc.c | 3 +- fs/ext4/resize.c | 2 +- fs/ext4/super.c | 2 +- fs/fat/dir.c | 4 +-- fs/fat/file.c | 3 +- fs/fuse/dev.c | 2 +- fs/fuse/file.c | 8 ++--- fs/splice.c | 2 +- include/linux/bvec.h | 3 +- include/linux/nodemask.h | 9 +++--- include/linux/perf_event.h | 2 +- include/net/tcp_ecn.h | 5 ++-- io_uring/net.c | 6 ++-- ipc/mqueue.c | 4 +-- ipc/msg.c | 6 ++-- kernel/bpf/core.c | 4 +-- kernel/bpf/log.c | 2 +- kernel/bpf/verifier.c | 29 +++++++------------ kernel/trace/bpf_trace.c | 2 +- lib/bucket_locks.c | 2 +- lib/crypto/mpi/mpicoder.c | 2 +- lib/dynamic_queue_limits.c | 2 +- mm/gup.c | 4 +-- mm/memblock.c | 2 +- mm/memory.c | 2 +- mm/percpu.c | 2 +- mm/truncate.c | 3 +- mm/vmscan.c | 2 +- net/core/datagram.c | 6 ++-- net/core/flow_dissector.c | 7 ++--- net/core/net-sysfs.c | 3 +- net/core/skmsg.c | 4 +-- net/ethtool/cmis_cdb.c | 7 ++--- net/ipv4/fib_trie.c | 2 +- net/ipv4/tcp_input.c | 4 +-- net/ipv4/tcp_output.c | 5 ++-- net/ipv4/tcp_timer.c | 4 +-- net/ipv6/addrconf.c | 8 ++--- net/ipv6/ip6_output.c | 7 +++-- net/ipv6/ndisc.c | 5 ++-- net/mptcp/protocol.c | 8 ++--- net/netlink/genetlink.c | 9 +++--- net/packet/af_packet.c | 2 +- net/unix/af_unix.c | 4 +-- 76 files changed, 141 insertions(+), 176 deletions(-)--
2.39.5
Re: (subset) [PATCH 00/44] Change a lot of min_t() that might mask high bits
In reply to [email protected]Quoted 8 lines · On Wed, 19 Nov 2025 22:40:56 +0000, [email protected] wrote:
It in not uncommon for code to use
min_t(uint, a, b)when one of a or b is 64bit and can have a value that is larger than 2^32;
This is particularly prevelant with:uint_var = min_t(uint, uint_var, uint64_expression);Casts to u8 and u16 are very likely to discard significant bits.
[...]
Applied, thanks!
[12/44] block: use min() instead of min_t()
commit: 9420e720ad192c53c8d2803c5a2313b2d586adbd
Best regards,
Signature · 1 line
Jens Axboe
Herbert Xu
maintainerQuoted 160 lines · On Wed, Nov 19, 2025 at 10:40:56PM +0000, [email protected] wrote:
From: David Laight <[email protected]>
It in not uncommon for code to use
min_t(uint, a, b)when one of a or b is 64bit and can have a value that is larger than 2^32;
This is particularly prevelant with:uint_var = min_t(uint, uint_var, uint64_expression);Casts to u8 and u16 are very likely to discard significant bits.
These can be detected at compile time by changing
min_t(), for example:#define CHECK_SIZE(fn, type, val) \ BUILD_BUG_ON_MSG(sizeof (val) > sizeof (type) && \ !statically_true(((val) >> 8 * (sizeof (type) - 1)) < 256), \ fn "() significant bits of '" #val "' may be discarded")#define min_t(type, x, y) ({ \ CHECK_SIZE("min_t", type, x); \ CHECK_SIZE("min_t", type, y); \ __cmp_once(min, type, x, y); })(and similar changes to
max_t()andclamp_t().)This shows up some real bugs, some unlikely bugs and some false positives. In most cases both arguments are unsigned type (just different ones) and
min_t()can just be replaced bymin().The patches are all independant and are most of the ones needed to get the x86-64 kernel I build to compile.
I've not tried building an allyesconfig or allmodconfig kernel. I've also not included the patch tominmax.hitself.I've tried to put the patches that actually fix things first. The last one is 0009.
I gave up on fixing
sched/fair.c- it is too broken for a single patch! The patch fornet/ipv4/tcp.cis also absent becausedo_tcp_getsockopt()needs multiple/larger changes to make it 'sane'.I've had to trim the 124 maintainers/lists that get_maintainer.pl finds from 124 to under 100 to be able to send the cover letter.
The individual patches only go to the addresses found for the associated files. That reduces the number of emails to a less unsane number.David Laight (44):
x86/asm/bitops: Change the return type ofvariable__ffs()to unsigned
int
ext4: Fix saturation of 64bit inode times for old filesystems
perf: Fix branch stack callchain limit
io_uring/net: Change some dubiousmin_t()
ipc/msg: Fix saturation of percpu counts inmsgctl_info()
bpf: Verifier, remove some unusual uses ofmin_t()andmax_t()net/core/flow_dissector: Fix cap of__skb_flow_dissect()return value.
net: ethtool: Usemin3()instead of nestedmin_t(u16,...)
ipv6:__ip6_append_data()don't abusemax_t()casts x86/crypto:ctr_crypt()usemin()instead ofmin_t()arch/x96/kvm: usemin()instead ofmin_t()
block: usemin()instead ofmin_t()
drivers/acpi: usemin()instead ofmin_t()drivers/char/hw_random: usemin3()instead of nestedmin_t()drivers/char/tpm: usemin()instead ofmin_t()drivers/crypto/ccp: usemin()instead ofmin_t()drivers/cxl: usemin()instead ofmin_t()
drivers/gpio: usemin()instead ofmin_t()drivers/gpu/drm/amd: usemin()instead ofmin_t()drivers/i2c/busses: usemin()instead ofmin_t()drivers/net/ethernet/realtek: usemin()instead ofmin_t()drivers/nvme: usemin()instead ofmin_t()
arch/x86/mm: usemin()instead ofmin_t()
drivers/nvmem: usemin()instead ofmin_t()
drivers/pci: usemin()instead ofmin_t()
drivers/scsi: usemin()instead ofmin_t()
drivers/tty/vt: useumin()instead ofmin_t(u16, ...)for row/col
limits
drivers/usb/storage: usemin()instead ofmin_t()drivers/xen: usemin()instead ofmin_t()
fs: usemin()orumin()instead ofmin_t()
block:bvec.h: usemin()instead ofmin_t()
nodemask: usemin()instead ofmin_t()
ipc: usemin()instead ofmin_t()
bpf: usemin()instead ofmin_t()
bpf_trace: usemin()instead ofmin_t()
lib/bucket_locks: usemin()instead ofmin_t()lib/crypto/mpi: usemin()instead ofmin_t()lib/dynamic_queue_limits: usemax()instead ofmax_t()
mm: usemin()instead ofmin_t()
net: Don't pass bitfields tomax_t()
net/core: Change loop conditions somin()can be used
net: usemin()instead ofmin_t()
net/netlink: Useumin()to avoidmin_t(int, ...)discarding high bits net/mptcp: Change some dubiousmin_t(int, ...)tomin()arch/x86/crypto/aesni-intel_glue.c | 3 +- arch/x86/include/asm/bitops.h | 18 +++++------- arch/x86/kvm/emulate.c | 3 +- arch/x86/kvm/lapic.c | 2 +- arch/x86/kvm/mmu/mmu.c | 2 +- arch/x86/mm/pat/set_memory.c | 12 ++++---- block/blk-iocost.c | 6 ++-- block/blk-settings.c | 2 +- block/partitions/efi.c | 3 +- drivers/acpi/property.c | 2 +- drivers/char/hw_random/core.c | 2 +- drivers/char/tpm/tpm1-cmd.c | 2 +- drivers/char/tpm/tpm_tis_core.c | 4 +-- drivers/crypto/ccp/ccp-dev.c | 2 +- drivers/cxl/core/mbox.c | 2 +- drivers/gpio/gpiolib-acpi-core.c | 2 +- .../gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c | 4 +-- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- drivers/i2c/busses/i2c-designware-master.c | 2 +- drivers/net/ethernet/realtek/r8169_main.c | 3 +- drivers/nvme/host/pci.c | 3 +- drivers/nvme/host/zns.c | 3 +- drivers/nvmem/core.c | 2 +- drivers/pci/probe.c | 3 +- drivers/scsi/hosts.c | 2 +- drivers/tty/vt/selection.c | 9 +++--- drivers/usb/storage/protocol.c | 3 +- drivers/xen/grant-table.c | 2 +- fs/buffer.c | 2 +- fs/exec.c | 2 +- fs/ext4/ext4.h | 2 +- fs/ext4/mballoc.c | 3 +- fs/ext4/resize.c | 2 +- fs/ext4/super.c | 2 +- fs/fat/dir.c | 4 +-- fs/fat/file.c | 3 +- fs/fuse/dev.c | 2 +- fs/fuse/file.c | 8 ++--- fs/splice.c | 2 +- include/linux/bvec.h | 3 +- include/linux/nodemask.h | 9 +++--- include/linux/perf_event.h | 2 +- include/net/tcp_ecn.h | 5 ++-- io_uring/net.c | 6 ++-- ipc/mqueue.c | 4 +-- ipc/msg.c | 6 ++-- kernel/bpf/core.c | 4 +-- kernel/bpf/log.c | 2 +- kernel/bpf/verifier.c | 29 +++++++------------ kernel/trace/bpf_trace.c | 2 +- lib/bucket_locks.c | 2 +- lib/crypto/mpi/mpicoder.c | 2 +- lib/dynamic_queue_limits.c | 2 +- mm/gup.c | 4 +-- mm/memblock.c | 2 +- mm/memory.c | 2 +- mm/percpu.c | 2 +- mm/truncate.c | 3 +- mm/vmscan.c | 2 +- net/core/datagram.c | 6 ++-- net/core/flow_dissector.c | 7 ++--- net/core/net-sysfs.c | 3 +- net/core/skmsg.c | 4 +-- net/ethtool/cmis_cdb.c | 7 ++--- net/ipv4/fib_trie.c | 2 +- net/ipv4/tcp_input.c | 4 +-- net/ipv4/tcp_output.c | 5 ++-- net/ipv4/tcp_timer.c | 4 +--
Quoted 8 lines
net/ipv6/addrconf.c | 8 ++--- net/ipv6/ip6_output.c | 7 +++-- net/ipv6/ndisc.c | 5 ++-- net/mptcp/protocol.c | 8 ++--- net/netlink/genetlink.c | 9 +++--- net/packet/af_packet.c | 2 +- net/unix/af_unix.c | 4 +-- 76 files changed, 141 insertions(+), 176 deletions(-)
Patches 10,14,16,37 applied. Thanks.
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt