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 falling back
to 1 hour on an actual overflow...

Found by Linux Verification Center (linuxtesting.org) with the Svace static
analysis tool.

Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
Cc: stable@vger.kernel.org

---
The patch is against the master branch of Trond Myklebust's linux-nfs.git repo.

Changes in version #3:
- changed the lease period overflow fallback to 1 hour, updated the patch
  description accordingly.

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.

 fs/nfs/nfs4_fs.h    |    3 +--
 fs/nfs/nfs4proc.c   |    2 +-
 fs/nfs/nfs4renewd.c |   10 +++++++---
 fs/nfs/nfs4state.c  |    2 +-
 4 files changed, 10 insertions(+), 7 deletions(-)

Index: linux-nfs/fs/nfs/nfs4_fs.h
===================================================================
--- linux-nfs.orig/fs/nfs/nfs4_fs.h
+++ linux-nfs/fs/nfs/nfs4_fs.h
@@ -464,8 +464,7 @@ struct nfs_client *nfs4_alloc_client(con
 extern void nfs4_schedule_state_renewal(struct nfs_client *);
 extern void nfs4_kill_renewd(struct nfs_client *);
 extern void nfs4_renew_state(struct work_struct *);
-extern void nfs4_set_lease_period(struct nfs_client *clp, unsigned long lease);
-
+extern void nfs4_set_lease_period(struct nfs_client *clp, u32 period);
 
 /* nfs4state.c */
 extern const nfs4_stateid current_stateid;
Index: linux-nfs/fs/nfs/nfs4proc.c
===================================================================
--- linux-nfs.orig/fs/nfs/nfs4proc.c
+++ linux-nfs/fs/nfs/nfs4proc.c
@@ -5478,7 +5478,7 @@ static int nfs4_do_fsinfo(struct nfs_ser
 		err = _nfs4_do_fsinfo(server, fhandle, fsinfo);
 		trace_nfs4_fsinfo(server, fhandle, fsinfo->fattr, err);
 		if (err == 0) {
-			nfs4_set_lease_period(server->nfs_client, fsinfo->lease_time * HZ);
+			nfs4_set_lease_period(server->nfs_client, fsinfo->lease_time);
 			break;
 		}
 		err = nfs4_handle_exception(server, err, &exception);
Index: linux-nfs/fs/nfs/nfs4renewd.c
===================================================================
--- linux-nfs.orig/fs/nfs/nfs4renewd.c
+++ linux-nfs/fs/nfs/nfs4renewd.c
@@ -137,11 +137,15 @@ nfs4_kill_renewd(struct nfs_client *clp)
  * nfs4_set_lease_period - Sets the lease period on a nfs_client
  *
  * @clp: pointer to nfs_client
- * @lease: new value for lease period
+ * @period: new value for lease period (in seconds)
  */
-void nfs4_set_lease_period(struct nfs_client *clp,
-		unsigned long lease)
+void nfs4_set_lease_period(struct nfs_client *clp, u32 period)
 {
+	unsigned long lease;
+
+	if (check_mul_overflow(period, HZ, &lease))
+		lease = 60UL * 60UL * HZ; /* one hour */
+
 	spin_lock(&clp->cl_lock);
 	clp->cl_lease_time = lease;
 	spin_unlock(&clp->cl_lock);
Index: linux-nfs/fs/nfs/nfs4state.c
===================================================================
--- linux-nfs.orig/fs/nfs/nfs4state.c
+++ linux-nfs/fs/nfs/nfs4state.c
@@ -103,7 +103,7 @@ static int nfs4_setup_state_renewal(stru
 
 	status = nfs4_proc_get_lease_time(clp, &fsinfo);
 	if (status == 0) {
-		nfs4_set_lease_period(clp, fsinfo.lease_time * HZ);
+		nfs4_set_lease_period(clp, fsinfo.lease_time);
 		nfs4_schedule_state_renewal(clp);
 	}

On Thu, 13 Nov 2025 23:37:03 +0300
Sergey Shtylyov <s.shtylyov@omp.ru> 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 falling back
> to 1 hour on an actual overflow...
> 
> Found by Linux Verification Center (linuxtesting.org) with the Svace static
> analysis tool.
> 
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> Cc: stable@vger.kernel.org
> 
> ---
> The patch is against the master branch of Trond Myklebust's linux-nfs.git repo.
> 
> Changes in version #3:
> - changed the lease period overflow fallback to 1 hour, updated the patch
>   description accordingly.
> 
> 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.
> 
>  fs/nfs/nfs4_fs.h    |    3 +--
>  fs/nfs/nfs4proc.c   |    2 +-
>  fs/nfs/nfs4renewd.c |   10 +++++++---
>  fs/nfs/nfs4state.c  |    2 +-
>  4 files changed, 10 insertions(+), 7 deletions(-)
> 
> Index: linux-nfs/fs/nfs/nfs4_fs.h
> ===================================================================
> --- linux-nfs.orig/fs/nfs/nfs4_fs.h
> +++ linux-nfs/fs/nfs/nfs4_fs.h
> @@ -464,8 +464,7 @@ struct nfs_client *nfs4_alloc_client(con
>  extern void nfs4_schedule_state_renewal(struct nfs_client *);
>  extern void nfs4_kill_renewd(struct nfs_client *);
>  extern void nfs4_renew_state(struct work_struct *);
> -extern void nfs4_set_lease_period(struct nfs_client *clp, unsigned long lease);
> -
> +extern void nfs4_set_lease_period(struct nfs_client *clp, u32 period);
>  
>  /* nfs4state.c */
>  extern const nfs4_stateid current_stateid;
> Index: linux-nfs/fs/nfs/nfs4proc.c
> ===================================================================
> --- linux-nfs.orig/fs/nfs/nfs4proc.c
> +++ linux-nfs/fs/nfs/nfs4proc.c
> @@ -5478,7 +5478,7 @@ static int nfs4_do_fsinfo(struct nfs_ser
>  		err = _nfs4_do_fsinfo(server, fhandle, fsinfo);
>  		trace_nfs4_fsinfo(server, fhandle, fsinfo->fattr, err);
>  		if (err == 0) {
> -			nfs4_set_lease_period(server->nfs_client, fsinfo->lease_time * HZ);
> +			nfs4_set_lease_period(server->nfs_client, fsinfo->lease_time);
>  			break;
>  		}
>  		err = nfs4_handle_exception(server, err, &exception);
> Index: linux-nfs/fs/nfs/nfs4renewd.c
> ===================================================================
> --- linux-nfs.orig/fs/nfs/nfs4renewd.c
> +++ linux-nfs/fs/nfs/nfs4renewd.c
> @@ -137,11 +137,15 @@ nfs4_kill_renewd(struct nfs_client *clp)
>   * nfs4_set_lease_period - Sets the lease period on a nfs_client
>   *
>   * @clp: pointer to nfs_client
> - * @lease: new value for lease period
> + * @period: new value for lease period (in seconds)
>   */
> -void nfs4_set_lease_period(struct nfs_client *clp,
> -		unsigned long lease)
> +void nfs4_set_lease_period(struct nfs_client *clp, u32 period)
>  {
> +	unsigned long lease;
> +
> +	if (check_mul_overflow(period, HZ, &lease))
> +		lease = 60UL * 60UL * HZ; /* one hour */

That isn't good enough, just a few lines higher there is:
	timeout = (2 * clp->cl_lease_time) / 3 + (long)clp->cl_last_renewal
		- (long)jiffies;

So the value has to be multipliable by 2 without overflowing.
I also suspect the modulo arithmetic also only works if the values
are 'much smaller than long'.
With HZ = 1000 and a requested period of 1000 hours the multiply (just)
fits in 32 bits - but none of the code is going to work at all.

It would be simpler and safer to just put a sanity upper limit on period.
I've no idea what normal/sane values are (lower as well as upper).
But perhaps you want:
	/* For sanity clamp between 10 mins and 100 hours */
	lease = clamp(period, 10 * 60, 100 * 60 * 60) * HZ;

> +
>  	spin_lock(&clp->cl_lock);
>  	clp->cl_lease_time = lease;
>  	spin_unlock(&clp->cl_lock);

Do I see a lock that doesn't?

	David

> Index: linux-nfs/fs/nfs/nfs4state.c
> ===================================================================
> --- linux-nfs.orig/fs/nfs/nfs4state.c
> +++ linux-nfs/fs/nfs/nfs4state.c
> @@ -103,7 +103,7 @@ static int nfs4_setup_state_renewal(stru
>  
>  	status = nfs4_proc_get_lease_time(clp, &fsinfo);
>  	if (status == 0) {
> -		nfs4_set_lease_period(clp, fsinfo.lease_time * HZ);
> +		nfs4_set_lease_period(clp, fsinfo.lease_time);
>  		nfs4_schedule_state_renewal(clp);
>  	}
>  
>

On 11/14/25 1:41 AM, David Laight 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 falling back
>> to 1 hour on an actual overflow...
>>
>> Found by Linux Verification Center (linuxtesting.org) with the Svace static
>> analysis tool.
>>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>> Cc: stable@vger.kernel.org

[...]>> Index: linux-nfs/fs/nfs/nfs4renewd.c
>> ===================================================================
>> --- linux-nfs.orig/fs/nfs/nfs4renewd.c
>> +++ linux-nfs/fs/nfs/nfs4renewd.c
>> @@ -137,11 +137,15 @@ nfs4_kill_renewd(struct nfs_client *clp)
>>   * nfs4_set_lease_period - Sets the lease period on a nfs_client
>>   *
>>   * @clp: pointer to nfs_client
>> - * @lease: new value for lease period
>> + * @period: new value for lease period (in seconds)
>>   */
>> -void nfs4_set_lease_period(struct nfs_client *clp,
>> -		unsigned long lease)
>> +void nfs4_set_lease_period(struct nfs_client *clp, u32 period)
>>  {
>> +	unsigned long lease;
>> +
>> +	if (check_mul_overflow(period, HZ, &lease))
>> +		lease = 60UL * 60UL * HZ; /* one hour */
> 
> That isn't good enough, just a few lines higher there is:
> 	timeout = (2 * clp->cl_lease_time) / 3 + (long)clp->cl_last_renewal
> 		- (long)jiffies;
   Indeed, I should have probably capped period at 3600 secs as well...

> So the value has to be multipliable by 2 without overflowing.
> I also suspect the modulo arithmetic also only works if the values
> are 'much smaller than long'.

   You mean the jiffies-relative math? I think it should work with any
values, with either 32- or 64-bit *unsigned long*...

> With HZ = 1000 and a requested period of 1000 hours the multiply (just)
> fits in 32 bits - but none of the code is going to work at all.
> 
> It would be simpler and safer to just put a sanity upper limit on period.

   Yes.

> I've no idea what normal/sane values are (lower as well as upper).

   The RFCs don't have any, it seems...

> But perhaps you want:
> 	/* For sanity clamp between 10 mins and 100 hours */
> 	lease = clamp(period, 10 * 60, 100 * 60 * 60) * HZ;

   Trond was talking about 1-hour period... And I don't think we need the
lower bound (except maybe 1 second?)...

>> +
>>  	spin_lock(&clp->cl_lock);
>>  	clp->cl_lease_time = lease;
>>  	spin_unlock(&clp->cl_lock);
> 
> Do I see a lock that doesn't?

   Doesn't do anything useful, you mean? :-)

[...]

MBR, Sergey

On Tue, 18 Nov 2025 22:51:09 +0300
Sergey Shtylyov <s.shtylyov@omp.ru> wrote:

> On 11/14/25 1:41 AM, David Laight 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 falling back
> >> to 1 hour on an actual overflow...
> >>
> >> Found by Linux Verification Center (linuxtesting.org) with the Svace static
> >> analysis tool.
> >>
> >> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> >> Cc: stable@vger.kernel.org  
> 
> [...]>> Index: linux-nfs/fs/nfs/nfs4renewd.c
> >> ===================================================================
> >> --- linux-nfs.orig/fs/nfs/nfs4renewd.c
> >> +++ linux-nfs/fs/nfs/nfs4renewd.c
> >> @@ -137,11 +137,15 @@ nfs4_kill_renewd(struct nfs_client *clp)
> >>   * nfs4_set_lease_period - Sets the lease period on a nfs_client
> >>   *
> >>   * @clp: pointer to nfs_client
> >> - * @lease: new value for lease period
> >> + * @period: new value for lease period (in seconds)
> >>   */
> >> -void nfs4_set_lease_period(struct nfs_client *clp,
> >> -		unsigned long lease)
> >> +void nfs4_set_lease_period(struct nfs_client *clp, u32 period)
> >>  {
> >> +	unsigned long lease;
> >> +
> >> +	if (check_mul_overflow(period, HZ, &lease))
> >> +		lease = 60UL * 60UL * HZ; /* one hour */  
> > 
> > That isn't good enough, just a few lines higher there is:
> > 	timeout = (2 * clp->cl_lease_time) / 3 + (long)clp->cl_last_renewal
> > 		- (long)jiffies;  
>    Indeed, I should have probably capped period at 3600 secs as well...
> 
> > So the value has to be multipliable by 2 without overflowing.
> > I also suspect the modulo arithmetic also only works if the values
> > are 'much smaller than long'.  
> 
>    You mean the jiffies-relative math? I think it should work with any
> values, with either 32- or 64-bit *unsigned long*...

There might be tests of the form (signed long)(jiffies - value) > 0.
They only work if the interval is less than half (the time) of jiffies.
Such values are insane - but you are applying a cap that isn't strong enough.

> 
> > With HZ = 1000 and a requested period of 1000 hours the multiply (just)
> > fits in 32 bits - but none of the code is going to work at all.
> > 
> > It would be simpler and safer to just put a sanity upper limit on period.  
> 
>    Yes.
> 
> > I've no idea what normal/sane values are (lower as well as upper).  
> 
>    The RFCs don't have any, it seems...
> 
> > But perhaps you want:
> > 	/* For sanity clamp between 10 mins and 100 hours */
> > 	lease = clamp(period, 10 * 60, 100 * 60 * 60) * HZ;  
> 
>    Trond was talking about 1-hour period... And I don't think we need the
> lower bound (except maybe 1 second?)...

If 1 hour might be a reasonable value, then clamp to something much bigger
that won't break the code.
In the past I've used at least twice the limit from the 'standard'.
I've also had methods of setting values outside the limits (useful
for testing timers that are supposed to be at least 10 minutes.

> 
> >> +
> >>  	spin_lock(&clp->cl_lock);
> >>  	clp->cl_lease_time = lease;
> >>  	spin_unlock(&clp->cl_lock);  
> > 
> > Do I see a lock that doesn't?  
> 
>    Doesn't do anything useful, you mean? :-)

You can think of a 'lock' as something that locks two or more
operations together - often just a compare and update.

That one is (at most) a WRITE_ONCE().

	David

> 
> [...]
> 
> MBR, Sergey

On 11/19/25 12:17 AM, David Laight 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 falling back
>>>> to 1 hour on an actual overflow...
>>>>
>>>> Found by Linux Verification Center (linuxtesting.org) with the Svace static
>>>> analysis tool.
>>>>
>>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>>> Cc: stable@vger.kernel.org  
>>
[...]
>>>> Index: linux-nfs/fs/nfs/nfs4renewd.c
>>>> ===================================================================
>>>> --- linux-nfs.orig/fs/nfs/nfs4renewd.c
>>>> +++ linux-nfs/fs/nfs/nfs4renewd.c
>>>> @@ -137,11 +137,15 @@ nfs4_kill_renewd(struct nfs_client *clp)
>>>>   * nfs4_set_lease_period - Sets the lease period on a nfs_client
>>>>   *
>>>>   * @clp: pointer to nfs_client
>>>> - * @lease: new value for lease period
>>>> + * @period: new value for lease period (in seconds)
>>>>   */
>>>> -void nfs4_set_lease_period(struct nfs_client *clp,
>>>> -		unsigned long lease)
>>>> +void nfs4_set_lease_period(struct nfs_client *clp, u32 period)
>>>>  {
>>>> +	unsigned long lease;
>>>> +
>>>> +	if (check_mul_overflow(period, HZ, &lease))
>>>> +		lease = 60UL * 60UL * HZ; /* one hour */  
>>>
>>> That isn't good enough, just a few lines higher there is:
>>> 	timeout = (2 * clp->cl_lease_time) / 3 + (long)clp->cl_last_renewal
>>> 		- (long)jiffies;  
>>    Indeed, I should have probably capped period at 3600 secs as well...

   That's one hour.

>>> So the value has to be multipliable by 2 without overflowing.
>>> I also suspect the modulo arithmetic also only works if the values
>>> are 'much smaller than long'.
>>
>>    You mean the jiffies-relative math? I think it should work with any
>> values, with either 32- or 64-bit *unsigned long*...
> 
> There might be tests of the form (signed long)(jiffies - value) > 0.
> They only work if the interval is less than half (the time) of jiffies.
> Such values are insane - but you are applying a cap that isn't strong enough.

*   3600 seconds, you mean?

>>> With HZ = 1000 and a requested period of 1000 hours the multiply (just)
>>> fits in 32 bits - but none of the code is going to work at all.
>>>
>>> It would be simpler and safer to just put a sanity upper limit on period.  
>>
>>    Yes.
>>
>>> I've no idea what normal/sane values are (lower as well as upper).  
>>
>>    The RFCs don't have any, it seems...

   Nether max nor min. It's on;y said that lease_time is a 32-bit unsigned
value...

>>> But perhaps you want:
>>> 	/* For sanity clamp between 10 mins and 100 hours */
>>> 	lease = clamp(period, 10 * 60, 100 * 60 * 60) * HZ;  
>>
>>    Trond was talking about 1-hour period... And I don't think we need the
>> lower bound (except maybe 1 second?)...
> 
> If 1 hour might be a reasonable value, then clamp to something much bigger
> that won't break the code.

   Trond said that even that seems too much for the file lock lease period...

[...]

>>>> +
>>>>  	spin_lock(&clp->cl_lock);
>>>>  	clp->cl_lease_time = lease;
>>>>  	spin_unlock(&clp->cl_lock);  
>>>
>>> Do I see a lock that doesn't?  
>>
>>    Doesn't do anything useful, you mean? :-)
> 
> You can think of a 'lock' as something that locks two or more
> operations together - often just a compare and update.
> 
> That one is (at most) a WRITE_ONCE().

   Yes, probably... :-)

[..]

MBR, Sergey