Re: [PATCH net 2/3] rxrpc: Fix DATA decrypt vs splice() by copying data to buffer in recvmsg

[David Laight <david.laight.linux@gmail.com>]

On Tue, 12 May 2026 17:52:03 +0100
David Howells <dhowells@redhat.com> wrote:

> David Laight <david.laight.linux@gmail.com> wrote:
...
> > > +		size_t size = umin(round_up(sp->len, 32), 2048);  
> > 
> > Doesn't min() work?  
> 
> Actually, it should be umax() as I want the largest of the values (as Jeff
> pointed out).  I prefer using umin/umax for values that are known to be
> unsigned as you don't get casting errors (see the number of places we end up
> using min/max_t(<unsigned-type>, ...) when we should use umin/umax() instead)
> and the compiler may generate better code as we've told it that it doesn't
> have to worry about negatives.

umin() and umax() are better than min_t() and max_t() (which is why I added
them); but you lose the compile-time check in min() and max() that rejects
comparisons where one side is unsigned and the compiler doesn't know that the 
other is always non-negative.

Basically if you compare a signed 32bit value and an unsigned 64bit one
with umin() the 32bit one is zero-extended to 64 bits.
OTOH min_t(u64) will sign-extend the 32bit value and then treat it as unsigned.
In both cases the onus is on the programmer to ensure the 32bit value isn't
negative.
For valid non-negative values the result is the same.
Zero-extending is usually free, sign-extending is particularly horrid on 32bit.

But it is better to use min() or max().
The compile-time tests will reject any cases where the integer promotion
rules could convert a negative value to a large positive one.
Note that the types no longer have to match.
Code like this is (usually) ok:
	unsigned int blk_len = ...;
	int rval = fun(...);
	while (rval > 0) {
		u32 len = min(rval, blk_len);
		// process len bytes;
		rval -= len;
	}
even though the types passed to min() differ in signedness the compiler's
value tracking means it knows that rval can never become a large unsigned
value - and min() uses that to allow it all through.
	
-- David

> 
> > That doesn't look right.
> > If sp->len is bigger than 2048 the you keep allocating a new buffer
> > and the call below overruns the allocated buffer.  
> 
> Yep - see the aforementioned umax comment.
> 
> David
>