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.
Fixes: b35108a51cf7ba ("jiffies: Define secs_to_jiffies()") CC: stable@vger.kernel.org # 6.12+ CC: Andrew Morton akpm@linux-foundation.org Reported-by: kernel test robot lkp@intel.com Closes: https://lore.kernel.org/oe-kbuild-all/202501301334.NB6NszQR-lkp@intel.com/ Signed-off-by: Easwar Hariharan eahariha@linux.microsoft.com --- include/linux/jiffies.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h index ed945f42e064..0ea8c9887429 100644 --- a/include/linux/jiffies.h +++ b/include/linux/jiffies.h @@ -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)
On 1/30/2025 10:43 AM, Easwar Hariharan wrote:
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.
Fixes: b35108a51cf7ba ("jiffies: Define secs_to_jiffies()") CC: stable@vger.kernel.org # 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)
On Thu, 30 Jan 2025 18:43:17 +0000 Easwar Hariharan eahariha@linux.microsoft.com wrote:
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.
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
Fixes: b35108a51cf7ba ("jiffies: Define secs_to_jiffies()") CC: stable@vger.kernel.org # 6.12+ CC: Andrew Morton akpm@linux-foundation.org Reported-by: kernel test robot lkp@intel.com Closes: https://lore.kernel.org/oe-kbuild-all/202501301334.NB6NszQR-lkp@intel.com/ Signed-off-by: Easwar Hariharan eahariha@linux.microsoft.com
include/linux/jiffies.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h index ed945f42e064..0ea8c9887429 100644 --- a/include/linux/jiffies.h +++ b/include/linux/jiffies.h @@ -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)
On 30. 01. 25, 21:14, David Laight wrote:
On Thu, 30 Jan 2025 18:43:17 +0000 Easwar Hariharan eahariha@linux.microsoft.com wrote:
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.
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.
Hi Jiri,
CC linux-xfs
On Fri, 31 Jan 2025 at 08:05, Jiri Slaby jirislaby@kernel.org wrote:
On 30. 01. 25, 21:14, David Laight wrote:
On Thu, 30 Jan 2025 18:43:17 +0000 Easwar Hariharan eahariha@linux.microsoft.com wrote:
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.
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
On 1/31/2025 12:10 AM, Geert Uytterhoeven wrote:
Hi Jiri,
CC linux-xfs
On Fri, 31 Jan 2025 at 08:05, Jiri Slaby jirislaby@kernel.org wrote:
On 30. 01. 25, 21:14, David Laight wrote:
On Thu, 30 Jan 2025 18:43:17 +0000 Easwar Hariharan eahariha@linux.microsoft.com wrote:
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.
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 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
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?
[1] https://lore.kernel.org/all/20250128-converge-secs-to-jiffies-part-two-v1-0-...
Thanks, Easwar
On 1/31/2025 9:55 AM, Easwar Hariharan wrote:
On 1/31/2025 12:10 AM, Geert Uytterhoeven wrote:
Hi Jiri,
CC linux-xfs
On Fri, 31 Jan 2025 at 08:05, Jiri Slaby jirislaby@kernel.org wrote:
On 30. 01. 25, 21:14, David Laight wrote:
On Thu, 30 Jan 2025 18:43:17 +0000 Easwar Hariharan eahariha@linux.microsoft.com wrote:
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.
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
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)
On 1/31/2025 9:55 AM, Easwar Hariharan wrote:
On 1/31/2025 12:10 AM, Geert Uytterhoeven wrote:
<snip>
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.
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)
linux-stable-mirror@lists.linaro.org
-
David Laight -
Easwar Hariharan -
Geert Uytterhoeven -
Jiri Slaby