Re: [PATCH v5 2/4] slab: Introduce kmalloc_obj() and family

[david laight <david.laight@runbox.com>]

On Tue, 25 Nov 2025 01:09:42 +0000
Matthew Wilcox <willy@infradead.org> wrote:

> On Mon, Nov 24, 2025 at 03:30:19PM -0800, Linus Torvalds wrote:
> > That all a very standard thing in assembly programming, which this is
> > all about. 'entry' is a signed offset from its own address.  
> 
> I used to be an assembly programmer ... 28 years ago.  I've mostly put
> that world out of my mind (and being able to write a 20,000 instruction
> ARM32 program entirely in assembly is just not that useful an
> accomplishment to put on my CV).  Anyway, this isn't the point ...
> 
> > > The warning is ... not the best phrased, but in terms of divining the
> > > programmer's intent, I genuinely don't know if this code is supposed
> > > to zero-extend or sign-extend the s32 to unsigned long.  
> > 
> > What?
> > 
> > A signed value gets sign-extended when cast to a larger type. That's
> > how all of this always works. Casting a signed value to 'unsigned
> > long' will set the high bits in the result.
> > 
> > That's pretty much the *definition* of a signed value. It gets
> > sign-extended when used, and then obviously it becomes a large
> > unsigned value, but this is how two's complement addition
> > fundamentally works.  
> 
> Yes, agreed.
> 
> > So honestly, what's the problem with this code?
> > 
> > The warning makes no sense, and is garbage. Are we not allowed to add
> > signed integers to unsigned 64-bit values now, because that addition
> > involves that cast of a signed 32-bit entry to an unsigned 64-bit one?
> > 
> > There is NO WAY that warning is valid, it's; not *ever* something we
> > should enable, and the fact that you people are discussing it as such
> > is just crazy.
> > 
> > That code would not be improved at all by adding another cast (to
> > first cast that s32 to 'long', in order to then add it to 'unsigned
> > long').
> > 
> > Imagine how many other places you add integers to 'unsigned long'.
> > EVERY SINGLE ONE of those places involves sign-extending the integer
> > and then doing arithmetic in unsigned.  
> 
> I have bad news.  Rust requires it.
> 
> fn add(base: u64, off: i32) -> u64 {
>     base + off
> }
> 
> error[E0308]: mismatched types
>  --> add.rs:2:12  
>   |
> 2 |     base + off
>   |            ^^^ expected `u64`, found `i32`
> 
> error[E0277]: cannot add `i32` to `u64`
>  --> add.rs:2:10  
>   |
> 2 |     base + off
>   |          ^ no implementation for `u64 + i32`
>   |
>   = help: the trait `Add<i32>` is not implemented for `u64`
>   = help: the following other types implement trait `Add<Rhs>`:
>             <u64 as Add>
>             <u64 as Add<&u64>>
>             <&'a u64 as Add<u64>>
>             <&u64 as Add<&u64>>
> 
> so the Rust language people have clearly decided that this is too
> complicated for your average programmer to figure out, and you need
> explicit casts to make it work.
> 

Jeepers...
As I've found looking at min_t() you can't trust kernel programmers
(never mind 'average' ones) to use the correct cast.
It wouldn't surprise be if the casts cause more bugs that the automatic
conversions that C does.

It wouldn't be as bad if there were separate 'casts' for widening and narrowing.
You also need the compiler to be doing 'value tracking' rather than just
looking at the types.
If I do:
	int len = read(.....);
	if (len < 0)
		return -1;
	if (len > sizeof (...))
		...
then -Wsign-compare complains, but a statically_true(len >= 0) is fine.

	David