On 9/16/24 22:44, Anna Schumaker 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 *unsigned long* on 32-bit
>> arches. 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 (using mul_u32_u32(), as it produces a better code on 32-bit
>> x86 arch), also checking for an overflow there and returning -ERANGE if it
>> does happen (we're also making that function *int* instead of *void* and
>> adding the result checks to its callers)...
>>
>> Found by Linux Verification Center (linuxtesting.org) with the Svace static
>> analysis tool.
>>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@xxxxxx>
>> Cc: stable@xxxxxxxxxxxxxxx
[...]
>> Index: linux-nfs/fs/nfs/nfs4renewd.c
>> ===================================================================
>> --- linux-nfs.orig/fs/nfs/nfs4renewd.c
>> +++ linux-nfs/fs/nfs/nfs4renewd.c
>> @@ -137,15 +137,22 @@ 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)
>> +int nfs4_set_lease_period(struct nfs_client *clp, u32 period)
>> {
>> + u64 result = mul_u32_u32(period, HZ);
>
> Thanks for the patch! Sorry it took a couple of weeks for me to look at.
NP, better late than never. :-)
> I do like this change for doing the calculation in a single place, rather
than relying on callers to do it first.
>
>> + unsigned long lease = result;
>> +
>> + /* First see if period * HZ actually fits into unsigned long... */
>> + if (result > ULONG_MAX)
>> + return -ERANGE;
>
> However, I'm not sold that this should be an error condition. I wonder if it would be better to change the clp->cl_lease_time field to a u64 so it has the same size on 32bit and 64bit architectures?
That would be much more invasive change. And pretty fruitless, I think, because nfs_client::cl_lease_time is only involved in the jiffies math,
and since jiffies is *unsigned long* (i.e. 32-bit on a 32-bit arch), the
results of our u64 math would get truncated to 32 bits anyway in the end...
Looking at time_{after|before}(), I even suspected they could complain
at compile time -- however I wasn't able to trigger that so far...
> Thanks,
> Anna
[...]
MBR, Sergey
