On Fri, 2025-07-25 at 23:19 +0300, Sergey Shtylyov wrote: > On 7/2/25 7:47 PM, Sergey Shtylyov wrote: > [...] > > > > The nfs_client::cl_lease_time field (as well as the jiffies > > > variable it's > > > used with) is declared as *unsigned long*, which is 32-bit type > > > on 32-bit > > > arches and 64-bit type on 64-bit arches. When > > > nfs4_set_lease_period() that > > > sets nfs_client::cl_lease_time is called, 32-bit > > > nfs_fsinfo::lease_time > > > field is multiplied by HZ -- that might overflow before being > > > implicitly > > > cast to *unsigned long*. Actually, there's no need to multiply by > > > HZ at all > > > the call sites of nfs4_set_lease_period() -- it makes more sense > > > to do that > > > once, inside that function, calling check_mul_overflow() and > > > capping result > > > at ULONG_MAX on actual overflow... > > > > > > Found by Linux Verification Center (linuxtesting.org) with the > > > Svace static > > > analysis tool. > > > > > > Signed-off-by: Sergey Shtylyov <s.shtylyov@xxxxxx> > > > Cc: stable@xxxxxxxxxxxxxxx > > > > > > --- > > > The patch is against the master branch of Trond Myklebust's > > > linux-nfs.git repo. > > > > > > Changes in version 2: > > > - made use of check_mul_overflow() instead of mul_u32_u32(); > > > - capped the multiplication result at ULONG_MAX instead of > > > returning -ERANGE, > > > keeping nfs4_set_lease_period() *void*; > > > - rewrote the patch description accordingly. > > > > Forgot to say that I had to adjust the patch description to make > > it clear > > that the overflow happens on 64-bit arches as well... > > Gentle ping! > Anna, do you agree with this approach? > > > [...] > > MBR, Sergey > NACK. If you're going to bound check the lease time, then you can at least make it a sensible value, not a random number chosen by some peddler of silicon. 48 days (a.k.a. 2^32/HZ_1000 seconds) is not a sensible value for a NFSv4 lease, and 571 megayears (a.k.a 2^64 / HZ_1000 seconds) even less so. Just bound it at 1 hour. If some use case turns for making the value larger than that, then we can consider making the limit configurable. Even 1 hour is a long time to wait for a file lock or delegation to expire when the client that held it dies. -- Trond Myklebust Linux NFS client maintainer, Hammerspace trondmy@xxxxxxxxxx, trond.myklebust@xxxxxxxxxxxxxxx
