RE: [PATCH] nfsd: fix incorrect high limit in clamp() on over-allocation

[David Laight <David.Laight@ACULAB.COM>]

From: Vincent Mailhol
> Sent: 09 December 2024 12:26
> 
> If over allocation occurs in nfsd4_get_drc_mem(), total_avail is set
> to zero. Consequently,
> 
>   clamp_t(unsigned long, avail, slotsize, total_avail/scale_factor);
> 
> gives:
> 
>   clamp_t(unsigned long, avail, slotsize, 0);
> 
> resulting in a clamp() call where the high limit is smaller than the
> low limit, which is undefined: the result could be either slotsize or
> zero depending on the order of evaluation.
> 
> Luckily, the two instructions just below the clamp() recover the
> undefined behaviour:
> 
>   num = min_t(int, num, avail / slotsize);
>   num = max_t(int, num, 1);
> 
> If avail = slotsize, the min_t() sets it back to 1. If avail = 0, the
> max_t() sets it back to 1.
> 
> So this undefined behaviour has no visible effect.
> 
> Anyway, remove the undefined behaviour in clamp() by only calling it
> and only doing the calculation of num if memory is still available.
> Otherwise, if over-allocation occurred, directly set num to 1 as
> intended by the author.

NAK:
The code is still wrong

> While at it, apply below checkpatch fix:
> 
>   WARNING: min() should probably be min_t(unsigned long, NFSD_MAX_MEM_PER_SESSION, total_avail)
>   #100: FILE: fs/nfsd/nfs4state.c:1954:
>   +		avail = min((unsigned long)NFSD_MAX_MEM_PER_SESSION, total_avail);

That was never a bug and checkpatch should never report it!
Casting one argument to min() has always been safer than using min_t().
Indeed it should really have been the preferred solition.
Consider what happens with min_t() if 'total_avail' happens to be 64bit
(with long being 32bit) - suddenly significant bit get masked off.

With the 'new improved' min() just delete the cast - it won't complain.

> 
> Fixes: 7f49fd5d7acd ("nfsd: handle drc over-allocation gracefully.")
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
...
> Because David's patch is targetting Andrew's mm tree, I would suggest
> that my patch also goes to that tree.
> ---
>  fs/nfsd/nfs4state.c | 46 +++++++++++++++++++++++++---------------------
>  1 file changed, 25 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 741b9449f727defc794347f1b116c955d715e691..eb91460c434e30f6df70f66d937f8c0f334b8e1b 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1944,35 +1944,39 @@ static u32 nfsd4_get_drc_mem(struct nfsd4_channel_attrs *ca, struct nfsd_net
> *nn
>  {
>  	u32 slotsize = slot_bytes(ca);
>  	u32 num = ca->maxreqs;
> -	unsigned long avail, total_avail;
> -	unsigned int scale_factor;
> 
>  	spin_lock(&nfsd_drc_lock);
> -	if (nfsd_drc_max_mem > nfsd_drc_mem_used)
> +	if (nfsd_drc_max_mem > nfsd_drc_mem_used) {
> +		unsigned long avail, total_avail;
> +		unsigned int scale_factor;
> +
>  		total_avail = nfsd_drc_max_mem - nfsd_drc_mem_used;

You've only checked > the result can still be 1.

> -	else
> +		avail = min_t(unsigned long,
> +			      NFSD_MAX_MEM_PER_SESSION, total_avail);
> +		/*
> +		 * Never use more than a fraction of the remaining memory,
> +		 * unless it's the only way to give this client a slot.
> +		 * The chosen fraction is either 1/8 or 1/number of threads,
> +		 * whichever is smaller.  This ensures there are adequate
> +		 * slots to support multiple clients per thread.
> +		 * Give the client one slot even if that would require
> +		 * over-allocation--it is better than failure.
> +		 */
> +		scale_factor = max_t(unsigned int,
> +				     8, nn->nfsd_serv->sv_nrthreads);

Shouldn't need to be max_t(), max() looks to be fine.
But can we please have the constants on the right?

> +		avail = clamp_t(unsigned long, avail, slotsize,
> +				total_avail/scale_factor);
> +		num = min_t(int, num, avail / slotsize);
> +		num = max_t(int, num, 1);
> +	} else {
>  		/* We have handed out more space than we chose in
>  		 * set_max_drc() to allow.  That isn't really a
>  		 * problem as long as that doesn't make us think we
>  		 * have lots more due to integer overflow.
>  		 */
> -		total_avail = 0;
> -	avail = min((unsigned long)NFSD_MAX_MEM_PER_SESSION, total_avail);
> -	/*
> -	 * Never use more than a fraction of the remaining memory,
> -	 * unless it's the only way to give this client a slot.
> -	 * The chosen fraction is either 1/8 or 1/number of threads,
> -	 * whichever is smaller.  This ensures there are adequate
> -	 * slots to support multiple clients per thread.
> -	 * Give the client one slot even if that would require
> -	 * over-allocation--it is better than failure.
> -	 */
> -	scale_factor = max_t(unsigned int, 8, nn->nfsd_serv->sv_nrthreads);
> -
> -	avail = clamp_t(unsigned long, avail, slotsize,
> -			total_avail/scale_factor);
> -	num = min_t(int, num, avail / slotsize);
> -	num = max_t(int, num, 1);
> +		num = 1;
> +	}

I'd leave the logic alone and use explicit min() and max) instead of clamp().
(and hopefully checkpatch won't suggest clamp() again).

The clamp() is trying to increase 'avail' to 'slotsize' - that would
ensure the later max() does nothing.
So replace the clamp() with a max(), giving:
	avail = max(avail, total_avail / max(nn->nfsd_serv->sv_nrthreads, 8));
	num = min(ca->maxregs, avail / slotsize) ?: 1;

Unless I missed another assignment to 'num' that is probably equvalent.

	David

>  	nfsd_drc_mem_used += num * slotsize;
>  	spin_unlock(&nfsd_drc_lock);
> 
> 
> ---
> base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
> change-id: 20241209-nfs4state_fix-bc6f1c1fc1d1
> 
> Best regards,
> --
> Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> 

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)