| Message ID | 20241209-nfs4state_fix-v1-1-7a66819c60f0@wanadoo.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | nfsd: fix incorrect high limit in clamp() on over-allocation | expand |
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)
On 12/9/24 7:25 AM, Vincent Mailhol via B4 Relay wrote: > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > > 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) > #100: FILE: fs/nfsd/nfs4state.c:1954: > + avail = min((unsigned long)NFSD_MAX_MEM_PER_SESSION, total_avail); > > Fixes: 7f49fd5d7acd ("nfsd: handle drc over-allocation gracefully.") > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > --- > Found by applying below patch from David: > > https://lore.kernel.org/all/34d53778977747f19cce2abb287bb3e6@AcuMS.aculab.com/ > > Doing so yield this report: > > In function ‘nfsd4_get_drc_mem’, > inlined from ‘check_forechannel_attrs’ at fs/nfsd/nfs4state.c:3791:16, > inlined from ‘nfsd4_create_session’ at fs/nfsd/nfs4state.c:3864:11: > ././include/linux/compiler_types.h:542:38: error: call to ‘__compiletime_assert_3707’ declared with attribute error: clamp() low limit (unsigned long)(slotsize) greater than high limit (unsigned long)(total_avail/scale_factor) > 542 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) > | ^ > ././include/linux/compiler_types.h:523:4: note: in definition of macro ‘__compiletime_assert’ > 523 | prefix ## suffix(); \ > | ^~~~~~ > ././include/linux/compiler_types.h:542:2: note: in expansion of macro ‘_compiletime_assert’ > 542 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) > | ^~~~~~~~~~~~~~~~~~~ > ./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’ > 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) > | ^~~~~~~~~~~~~~~~~~ > ./include/linux/minmax.h:114:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’ > 114 | BUILD_BUG_ON_MSG(statically_true(ulo > uhi), \ > | ^~~~~~~~~~~~~~~~ > ./include/linux/minmax.h:121:2: note: in expansion of macro ‘__clamp_once’ > 121 | __clamp_once(val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_)) > | ^~~~~~~~~~~~ > ./include/linux/minmax.h:275:36: note: in expansion of macro ‘__careful_clamp’ > 275 | #define clamp_t(type, val, lo, hi) __careful_clamp((type)(val), (type)(lo), (type)(hi)) > | ^~~~~~~~~~~~~~~ > fs/nfsd/nfs4state.c:1972:10: note: in expansion of macro ‘clamp_t’ > 1972 | avail = clamp_t(unsigned long, avail, slotsize, > | ^~~~~~~ > > 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; > - 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); > > > --- > base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4 > change-id: 20241209-nfs4state_fix-bc6f1c1fc1d1 > > Best regards, Hi Vincent - We're replacing this code wholesale in 6.14. See: https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/commit/?h=nfsd-testing&id=8233f78fbd970cbfcb9f78c719ac5a3aac4ea053
On Mon. 9 Dec. 2024 at 23:47, Chuck Lever <chuck.lever@oracle.com> wrote: > On 12/9/24 7:25 AM, Vincent Mailhol via B4 Relay wrote: (...) > Hi Vincent - > > We're replacing this code wholesale in 6.14. See: > > https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/commit/?h=nfsd-testing&id=8233f78fbd970cbfcb9f78c719ac5a3aac4ea053 I see. Thank you for letting me know. I will then just abandon this patch. Yours sincerely, Vincent Mailhol
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; - 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);