netdev - Re: [PATCH 44/44] net/mptcp: Change some dubious min

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251218201517.2f2d91d4@pumpkin>
Date: Thu, 18 Dec 2025 20:15:17 +0000
From: David Laight <david.laight.linux@...il.com>
To: Matthieu Baerts <matttbe@...nel.org>
Cc: linux-kernel@...r.kernel.org, mptcp@...ts.linux.dev,
 netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Mat
 Martineau <martineau@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH 44/44] net/mptcp: Change some dubious min_t(int, ...) to
 min()

On Thu, 18 Dec 2025 18:33:26 +0100
Matthieu Baerts <matttbe@...nel.org> wrote:

> Hi David,
> 
> On 19/11/2025 23:41, david.laight.linux@...il.com wrote:
> > From: David Laight <david.laight.linux@...il.com>
> > 
> > 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().  
> 
> Thank you for the patch!
> 
> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@...nel.org>
> 
> 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

> 
> Cheers,
> Matt