jiffies: Cast to unsigned long for secs_to_jiffies() conversion
PATCH 1/1 jiffies: Cast to unsigned long for secs_to_jiffies() conversion
While converting users of msecs_to_jiffies(), lkp reported that some range checks would always be true because of the mismatch between the implied int value of secs_to_jiffies() vs the unsigned long return value of the msecs_to_jiffies() calls it was replacing. Fix this by casting secs_to_jiffies() values as unsigned long.
--- include/linux/jiffies.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/jiffies.h +1 -1
@@ -537,7 +537,7 @@ static __always_inline unsigned long msecs_to_jiffies(const unsigned int m)* * Return: jiffies value */ -#define secs_to_jiffies (_secs) ((_secs) * HZ) +#define secs_to_jiffies (_secs) (unsigned long)((_secs) * HZ)extern unsigned long __usecs_to_jiffies (const unsigned int u);#if !(USEC_PER_SEC %HZ )
Signature · 1 line
2.43.0
Quoted 8 lines · Easwar Hariharan
While converting users of
msecs_to_jiffies(), lkp reported that some range checks would always be true because of the mismatch between the implied int value ofsecs_to_jiffies()vs the unsigned long return value of themsecs_to_jiffies()calls it was replacing. Fix this by castingsecs_to_jiffies()values as unsigned long.Fixes b35108a51cf7ba ("jiffies: Define secs_to_jiffies()")CC [email protected] # 6.12+
Sorry, this should be 6.13+ since secs_to_jiffies() was introduced in 6.13-rc1, not 6.12-rc1. I was mislead by git describe output. Let me send this as a v2.
Thanks,
Easwar (he/him)
Quoted 5 lines · On Thu, 30 Jan 2025 18:43:17 +0000 Easwar Hariharan…
While converting users of
msecs_to_jiffies(), lkp reported that some range checks would always be true because of the mismatch between the implied int value ofsecs_to_jiffies()vs the unsigned long return value of themsecs_to_jiffies()calls it was replacing. Fix this by castingsecs_to_jiffies()values as unsigned long.
Surely 'unsigned long' can't be the right type ? It changes between 32bit and 64bit systems.
Either it is allowed to wrap - so should be 32bit on both, or wrapping is unexpected and it needs to be 64bit on both.
As we all know (to our cost in many cases) a ms counter wraps 32bit in about 48 days.
David
Quoted 24 lines
Fixes b35108a51cf7ba ("jiffies: Define secs_to_jiffies()")CC [email protected] # 6.12+CC Andrew Morton <[email protected]>Reported-by kernel test robot <[email protected]>Signed-off-by Easwar Hariharan <[email protected]>--- include/linux/jiffies.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)include/linux/jiffies.h +1
-1@@ -537,7 +537,7 @@ static __always_inline unsigned long msecs_to_jiffies(const unsigned int m)* * Return: jiffies value */ -#define secs_to_jiffies (_secs) ((_secs) * HZ) +#define secs_to_jiffies (_secs) (unsigned long)((_secs) * HZ)extern unsigned long __usecs_to_jiffies (const unsigned int u);#if !(USEC_PER_SEC %HZ )
Quoted 9 lines · David Laight
Quoted 5 lines · On Thu, 30 Jan 2025 18:43:17 +0000 Easwar Hariharan…
While converting users of
msecs_to_jiffies(), lkp reported that some range checks would always be true because of the mismatch between the implied int value ofsecs_to_jiffies()vs the unsigned long return value of themsecs_to_jiffies()calls it was replacing. Fix this by castingsecs_to_jiffies()values as unsigned long.
Quoted 4 lines
Surely 'unsigned long' can't be the right type ? It changes between 32bit and 64bit systems.
Either it is allowed to wrap - so should be 32bit on both, or wrapping is unexpected and it needs to be 64bit on both.
But jiffies are really ulong.
Signature · 2 lines
js suse labs
Hi Jiri,
CC linux-xfs
Quoted 10 lines · Jiri Slaby
Quoted 9 lines · David Laight
Quoted 5 lines · On Thu, 30 Jan 2025 18:43:17 +0000 Easwar Hariharan…
While converting users of
msecs_to_jiffies(), lkp reported that some range checks would always be true because of the mismatch between the implied int value ofsecs_to_jiffies()vs the unsigned long return value of themsecs_to_jiffies()calls it was replacing. Fix this by castingsecs_to_jiffies()values as unsigned long.
Quoted 6 lines
Quoted 4 lines
Surely 'unsigned long' can't be the right type ? It changes between 32bit and 64bit systems.
Either it is allowed to wrap - so should be 32bit on both, or wrapping is unexpected and it needs to be 64bit on both.But jiffies are really ulong.
That's a good reason to make the change. E.g. msecs_to_jiffies() does return unsigned long.
Note that this change may cause fall-out, e.g.
int val = 5.
pr_debug("timeout = %u jiffies\n", secs_to_jiffies(val));
^^
must be changed to %lu
More importantly, I doubt this change is guaranteed to fix the reported issue. The code[*] in retry_timeout_seconds_store() does:
int val;
...
if (val < -1 || val > 86400)
return -EINVAL;
...
if (val != -1)
ASSERT(secs_to_jiffies(val) < LONG_MAX);As HZ is a known (rather small) constant, and val is range-checked before, the compiler can still devise that the condition is always true. So I think that assertion should just be removed.
[*] Before commit b524e0335da22473 ("xfs: convert timeouts to
secs_to_jiffies()"), which was applied to the MM tree only 3 days ago, the code used msecs_to_jiffies() * MSEC_PER_SEC, which is more complex than a simple multiplication, and harder for the compiler to analyze statically, thus not triggering the warning that easily...
Gr{oetje,eeting}s,
Geert
Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected] In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Quoted 55 lines · Geert Uytterhoeven
Hi Jiri,
CC linux-xfs
Quoted 10 lines · Jiri Slaby
Quoted 9 lines · David Laight
Quoted 5 lines · On Thu, 30 Jan 2025 18:43:17 +0000 Easwar Hariharan…
While converting users of
msecs_to_jiffies(), lkp reported that some range checks would always be true because of the mismatch between the implied int value ofsecs_to_jiffies()vs the unsigned long return value of themsecs_to_jiffies()calls it was replacing. Fix this by castingsecs_to_jiffies()values as unsigned long.Quoted 6 lines
Quoted 4 lines
Surely 'unsigned long' can't be the right type ? It changes between 32bit and 64bit systems.
Either it is allowed to wrap - so should be 32bit on both, or wrapping is unexpected and it needs to be 64bit on both.But jiffies are really ulong.
That's a good reason to make the change. E.g.
msecs_to_jiffies()does return unsigned long.Note that this change may cause fall-out, e.g.
int val = 5.
pr_debug("timeout = %u jiffies\n", secs_to_jiffies(val));
^^
must be changed to %luMore importantly, I doubt this change is guaranteed to fix the reported issue. The code[*] in
retry_timeout_seconds_store()does:int val; ... if (val < -1 || val > 86400) return -EINVAL; ... if (val != -1) ASSERT(secs_to_jiffies(val) < LONG_MAX);As HZ is a known (rather small) constant, and val is range-checked before, the compiler can still devise that the condition is always true. So I think that assertion should just be removed.
[*] Before commit
b524e0335da22473("xfs: convert timeouts to
secs_to_jiffies()"), which was applied to the MM tree only 3 days ago, the code usedmsecs_to_jiffies()*MSEC_PER_SEC, which is more complex than a simple multiplication, and harder for the compiler to analyze statically, thus not triggering the warning that easily...
Quoted 4 lines
Gr{oetje,eeting}s,
Geert
Thanks, Jiri and Geert. Geert, am I correct in understanding you that you're suggesting v2 of the series[1] to convert msecs_to_jiffies() calls to secs_to_jiffies() remove the ASSERT as redundant, while also keeping this patch because ulong is the right type for jiffies?
Thanks,
Easwar
Quoted 26 lines · Easwar Hariharan
Quoted 25 lines · Geert Uytterhoeven
Hi Jiri,
CC linux-xfs
Quoted 10 lines · Jiri Slaby
Quoted 9 lines · David Laight
Quoted 5 lines · On Thu, 30 Jan 2025 18:43:17 +0000 Easwar Hariharan…
While converting users of
msecs_to_jiffies(), lkp reported that some range checks would always be true because of the mismatch between the implied int value ofsecs_to_jiffies()vs the unsigned long return value of themsecs_to_jiffies()calls it was replacing. Fix this by castingsecs_to_jiffies()values as unsigned long.Quoted 6 lines
Quoted 4 lines
Surely 'unsigned long' can't be the right type ? It changes between 32bit and 64bit systems.
Either it is allowed to wrap - so should be 32bit on both, or wrapping is unexpected and it needs to be 64bit on both.But jiffies are really ulong.
That's a good reason to make the change. E.g.
msecs_to_jiffies()does return unsigned long.
Quoted 7 lines
Quoted 7 lines
Note that this change may cause fall-out, e.g.
int val = 5.
pr_debug("timeout = %u jiffies\n", secs_to_jiffies(val));
^^
must be changed to %lu
That was wrong even before the conversion to secs_to_jiffies() (or this patch) because as Jiri says, jiffies are ulong.
<snip>
- Easwar (he/him)
Quoted 1 line · Easwar Hariharan
On 1/31/2025 12:10 AM, Geert Uytterhoeven wrote:
<snip>
Quoted 12 lines
Quoted 12 lines
More importantly, I doubt this change is guaranteed to fix the reported issue. The code[*] in
retry_timeout_seconds_store()does:int val; ... if (val < -1 || val > 86400) return -EINVAL; ... if (val != -1) ASSERT(secs_to_jiffies(val) < LONG_MAX);
Quoted 4 lines
Quoted 4 lines
As HZ is a known (rather small) constant, and val is range-checked before, the compiler can still devise that the condition is always true. So I think that assertion should just be removed.
Following the lkp instructions to repro the issue, with this patch, the compiler does not continue reporting that the condition is always true. This patch is sufficient IMHO, without needing to remove the assert.
- Easwar (he/him)