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

Christophe JAILLET
2025-02-26T08:11:31Z

Le 26/02/2025 à 08:28, Daniel Vacek a écrit :
> On Tue, 25 Feb 2025 at 22:10, Christophe JAILLET
> <christophe.jaillet-39ZsbGIQGT5GWvitb5QawA@public.gmane.org> 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.
>
> This does not deal with any additional multiplications. If there is an
> overflow, it was already there before to begin with, IMO.
>
>> Hoping I got casting right:
>
> Maybe not exactly? See below...
>
>> #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));
>
> I think the format should actually be %lu giving the below results:
>
> res = 18446744073709551016
> res = 429496130
>
> Which is still wrong nonetheless. But here, *both* results are wrong
> as the expected output should be 214748364200 which you'll get with
> the correct helper/macro.
>
> But note another thing, the 1000 * (INT_MAX - 5) already overflows
> even before calling _msecs_to_jiffies(). See?

Agreed and intentional in my test C code.

That is the point.

The "if (result.uint_32 > INT_MAX / 1000)" in the original code was
handling such values.

>
> Now, you'll get that mentioned correct result with:
>
> #define secs_to_jiffies(_secs) ((unsigned long)(_secs) * HZ)

Not looked in details, but I think I would second on you on this, in
this specific example. Not sure if it would handle all possible uses of
secs_to_jiffies().

But it is not how secs_to_jiffies() is defined up to now. See [1].

[1]:
https://elixir.bootlin.com/linux/v6.14-rc4/source/include/linux/jiffies.h#L540

>
> Still, why unsigned? What if you wanted to convert -5 seconds to jiffies?

See commit bb2784d9ab495 which added the cast.

>
>> return 0;
>> }
>>
>>
>> gives :
>>
>> res = -600
>> res = 429496130
>>
>> with msec, the previous code would catch the overflow, now it overflows
>> silently.
>
> What compiler options are you using? I'm not getting any warnings.

I mean, with:
if (result.uint_32 > INT_MAX / 1000)
goto out_of_range;
the overflow would be handled *at runtime*.

Without such a check, an unexpected value could be stored in
opt->lock_timeout.

I think that a test is needed and with secs_to_jiffies(), I tentatively
proposed:
if (result.uint_32 > INT_MAX / HZ)
goto out_of_range;

CJ

>
>> untested, but maybe:
>> if (result.uint_32 > INT_MAX / HZ)
>> goto out_of_range;
>>
>> ?
>>
>> CJ
>>

...