Skip to content

Commit

Permalink
nfsd: fix incorrect high limit in clamp() on over-allocation
Browse files Browse the repository at this point in the history
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.

While at it, apply below checkpatch fix:

  WARNING: min() should probably be min_t(unsigned long, NFSD_MAX_MEM_PER_SESSION, total_avail)
  torvalds#100: FILE: fs/nfsd/nfs4state.c:1954:
  +		avail = min((unsigned long)NFSD_MAX_MEM_PER_SESSION, total_avail);

Fixes: 7f49fd5 ("nfsd: handle drc over-allocation gracefully.")
Signed-off-by: Vincent Mailhol <[email protected]>
  • Loading branch information
vincent-mailhol authored and intel-lab-lkp committed Dec 9, 2024
1 parent fac04ef commit 83984cc
Showing 1 changed file with 25 additions and 21 deletions.
46 changes: 25 additions & 21 deletions fs/nfsd/nfs4state.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
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);

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;
}
nfsd_drc_mem_used += num * slotsize;
spin_unlock(&nfsd_drc_lock);

Expand Down

0 comments on commit 83984cc

Please sign in to comment.