Re: [PATCH v3 06/16] rbd: convert timeouts to secs_to_jiffies()

[Easwar Hariharan <eahariha@xxxxxxxxxxxxxxxxxxx>]

On 2/25/2025 1:09 PM, Christophe JAILLET wrote:
> Le 25/02/2025 à 21:17, Easwar Hariharan a écrit :
>> Commit b35108a51cf7 ("jiffies: Define secs_to_jiffies()") introduced
>> secs_to_jiffies().  As the value here is a multiple of 1000, use
>> secs_to_jiffies() instead of msecs_to_jiffies() to avoid the multiplication
>>
>> This is converted using scripts/coccinelle/misc/secs_to_jiffies.cocci with
>> the following Coccinelle rules:
>>
>> @depends on patch@ expression E; @@
>>
>> -msecs_to_jiffies(E * 1000)
>> +secs_to_jiffies(E)
>>
>> @depends on patch@ expression E; @@
>>
>> -msecs_to_jiffies(E * MSEC_PER_SEC)
>> +secs_to_jiffies(E)
>>
>> While here, remove the no-longer necessary check for range since there's
>> no multiplication involved.
> 
> I'm not sure this is correct.
> Now you multiply by HZ and things can still overflow.
> 
> 
> Hoping I got casting right:
> 
> #define MSEC_PER_SEC    1000L
> #define HZ 100
> 
> 
> #define secs_to_jiffies(_secs) (unsigned long)((_secs) * HZ)
> 
> static inline unsigned long _msecs_to_jiffies(const unsigned int m)
> {
>     return (m + (MSEC_PER_SEC / HZ) - 1) / (MSEC_PER_SEC / HZ);
> }
> 
> int main() {
> 
>     int n = INT_MAX - 5;
> 
>     printf("res  = %ld\n", secs_to_jiffies(n));
>     printf("res  = %ld\n", _msecs_to_jiffies(1000 * n));
> 
>     return 0;
> }
> 
> 
> gives :
> 
> res  = -600
> res  = 429496130
> 
> with msec, the previous code would catch the overflow, now it overflows silently.
> 
> untested, but maybe:
>     if (result.uint_32 > INT_MAX / HZ)
>         goto out_of_range;
> 
> ?
> 
> CJ
> 

Thanks for the review! I was able to replicate your results, I'll try this range check
and get back.

Thanks,
Easwar (he/him)