linux-kernel - Re: [PATCH stable 6.1.y] nfsd: use __clamp in nfsd4_get_drc

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: <20251109232300.42618f06@pumpkin>
Date: Sun, 9 Nov 2025 23:23:00 +0000
From: David Laight <david.laight.linux@...il.com>
To: NeilBrown <neilb@...mail.net>
Cc: NeilBrown <neil@...wn.name>, stable@...r.kernel.org, Chuck Lever
 <chuck.lever@...cle.com>, Andrew Morton <akpm@...ux-foundation.org>, David
 Laight <David.Laight@...LAB.COM>, Linux NFS Mailing List
 <linux-nfs@...r.kernel.org>, Linux List Kernel Mailing
 <linux-kernel@...r.kernel.org>, speedcracker@...mail.com
Subject: Re: [PATCH stable 6.1.y] nfsd: use __clamp in nfsd4_get_drc_mem()

On Mon, 10 Nov 2025 08:45:35 +1100
NeilBrown <neilb@...mail.net> wrote:

> From: NeilBrown <neil@...wn.name>
> 
> A recent change to clamp_t() in 6.1.y caused fs/nfsd/nfs4state.c to fail
> to compile with gcc-9.
> 
> The code was written with the assumption that when "max < min",
>    clamp(val, min, max)
> would return max.  This assumption is not documented as an API promise
> and the change cause a compile failure if it could be statically
> determined that "max < min".
> 
> The relevant code was no longer present upstream when the clamp() change
> landed there, so there is no upstream change to backport.
> 
> As there is no clear case that the code is functioning incorrectly, the
> patch aims to restore the behaviour to exactly that before the clamp
> change, and to match what compilers other than gcc-9 produce.
> 
> clamp_t(type,v,min,max) is replaced with
>   __clamp((type)v, (type)min, (type)max)
> 
> Some of those type casts are unnecessary but they are included to make
> the code obviously correct.

I beg to differ.
If the values are all positive the casts aren't needed.
If and one the values are ever negative the code is broken.
(I think that is a bug in the old version without the initial check
that sets 'total_avail' to zero.)

> (__clamp() is the same as clamp(), but without the static API usage
> test).

And it is really an internal define that shouldn't be used outside
on minmax.h itself.

Replace the clamp() with the actual comparisons you want:
The code should always have been:
	if (avail < slotsize)
		avail = slotsize;
	else if (avail > total_avail/scale_factor)
		avail = total_avail/scale_factor;
(The compiler will CSE to two divides.)
I think that actually works best if both 'avail' and 'slotsize' are signed.
Then it doesn't matter if 'avail' is negative - and lots of tests above it
can be deleted (as well as the max() later then ensures it isn't zero).

But for bug compatibility swap the order of the tests over:
	if (avail > total_avail/scale_factor)
		avail = total_avail/scale_factor;
	else if (avail < slotsize)
		avail = slotsize;

    David

> 
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=220745#c0
> Fixes: 1519fbc8832b ("minmax.h: use BUILD_BUG_ON_MSG() for the lo < hi test in clamp()")
> Signed-off-by: NeilBrown <neil@...wn.name>
> ---
>  fs/nfsd/nfs4state.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 08bfc2b29b65..d485a140d36d 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1822,8 +1822,9 @@ static u32 nfsd4_get_drc_mem(struct nfsd4_channel_attrs *ca, struct nfsd_net *nn
>  	 */
>  	scale_factor = max_t(unsigned int, 8, nn->nfsd_serv->sv_nrthreads);
>  
> -	avail = clamp_t(unsigned long, avail, slotsize,
> -			total_avail/scale_factor);
> +	avail = __clamp((unsigned long)avail,
> +			(unsigned long)slotsize,
> +			(unsigned long)(total_avail/scale_factor));
>  	num = min_t(int, num, avail / slotsize);
>  	num = max_t(int, num, 1);
>  	nfsd_drc_mem_used += num * slotsize;