Le 26/02/2025 à 08:28, Daniel Vacek a écrit :
On Tue, 25 Feb 2025 at 22:10, Christophe JAILLET
<christophe.jaillet-39ZsbGIQGT5GWvitb5QawA@xxxxxxxxxxxxxxxx> 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
...
