[PATCH] posix-cpu-timers: use u64 multiplication in update_rlimit_cpu()

[PATCH] posix-cpu-timers: use u64 multiplication in update_rlimit_cpu()

[Zhan Xusheng]

update_rlimit_cpu() converts the RLIMIT_CPU value to nanoseconds with

        u64 nsecs = rlim_new * NSEC_PER_SEC;

On 32-bit kernels both rlim_new (unsigned long) and NSEC_PER_SEC
(1000000000L) are 32-bit, so the multiplication is performed in
unsigned long and truncated for rlim_new > 4 before being widened to
u64.

The same file already casts to u64 for the matching computation in
check_process_timers():

        u64 softns = (u64)soft * NSEC_PER_SEC;

As a result, the truncated value is installed into the CPUCLOCK_PROF
expiry cache (nextevt), causing the process CPU timer to be programmed
to fire prematurely for any RLIMIT_CPU soft limit >= 5 seconds. The
actual SIGXCPU/SIGKILL decision in check_process_timers() already casts
to u64 and is therefore correct, so limit enforcement is not broken;
only the expiry-cache programming is wrong. Apply the same cast here so
both paths convert rlim_cur identically.

64-bit kernels are unaffected.

Fixes: 858cf3a8c599 ("timers/itimer: Convert internal cputime_t units to nsec")
Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
---
 kernel/time/posix-cpu-timers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
index 74775b94d11b..5e633d8750d1 100644
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -41,7 +41,7 @@ void posix_cputimers_group_init(struct posix_cputimers *pct, u64 cpu_limit)
  */
 int update_rlimit_cpu(struct task_struct *task, unsigned long rlim_new)
 {
-	u64 nsecs = rlim_new * NSEC_PER_SEC;
+	u64 nsecs = (u64)rlim_new * NSEC_PER_SEC;
 	unsigned long irq_fl;
 
 	if (!lock_task_sighand(task, &irq_fl))
-- 
2.43.0

Re: [PATCH] posix-cpu-timers: use u64 multiplication in update_rlimit_cpu()

[Ingo Molnar]

* Zhan Xusheng <zhanxusheng1024@gmail.com> wrote:

> update_rlimit_cpu() converts the RLIMIT_CPU value to nanoseconds with
> 
>         u64 nsecs = rlim_new * NSEC_PER_SEC;
> 
> On 32-bit kernels both rlim_new (unsigned long) and NSEC_PER_SEC
> (1000000000L) are 32-bit, so the multiplication is performed in
> unsigned long and truncated for rlim_new > 4 before being widened to
> u64.
> 
> The same file already casts to u64 for the matching computation in
> check_process_timers():
> 
>         u64 softns = (u64)soft * NSEC_PER_SEC;
> 
> As a result, the truncated value is installed into the CPUCLOCK_PROF
> expiry cache (nextevt), causing the process CPU timer to be programmed
> to fire prematurely for any RLIMIT_CPU soft limit >= 5 seconds. The
> actual SIGXCPU/SIGKILL decision in check_process_timers() already casts
> to u64 and is therefore correct, so limit enforcement is not broken;
> only the expiry-cache programming is wrong. Apply the same cast here so
> both paths convert rlim_cur identically.
> 
> 64-bit kernels are unaffected.
> 
> Fixes: 858cf3a8c599 ("timers/itimer: Convert internal cputime_t units to nsec")
> Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
> ---
>  kernel/time/posix-cpu-timers.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
> index 74775b94d11b..5e633d8750d1 100644
> --- a/kernel/time/posix-cpu-timers.c
> +++ b/kernel/time/posix-cpu-timers.c
> @@ -41,7 +41,7 @@ void posix_cputimers_group_init(struct posix_cputimers *pct, u64 cpu_limit)
>   */
>  int update_rlimit_cpu(struct task_struct *task, unsigned long rlim_new)
>  {
> -	u64 nsecs = rlim_new * NSEC_PER_SEC;
> +	u64 nsecs = (u64)rlim_new * NSEC_PER_SEC;

Wouldn't it be more robust to define NSEC_PER_SEC not
as 1000000000L, but 1000000000LL? (Together with sorting
out the inevitable side effects.)

Ie. in any logic that uses NSEC_PER_SEC multiplication we
should default to 64-bit arithmetics and not silent
truncation to 32-bit width.

With that we could remove a number of '(u64)' forced
type conversions as well.

Thanks,

	Ingo

Re: [PATCH] posix-cpu-timers: use u64 multiplication in update_rlimit_cpu()

[Zhan Xusheng]

From: Zhan Xusheng <zhanxusheng1024@gmail.com>

* Ingo Molnar <mingo@kernel.org> wrote:
> Wouldn't it be more robust to define NSEC_PER_SEC not as 1000000000L,
> but 1000000000LL? (Together with sorting out the inevitable side
> effects.) Ie. in any logic that uses NSEC_PER_SEC multiplication we
> should default to 64-bit arithmetics and not silent truncation to
> 32-bit width. With that we could remove a number of '(u64)' forced
> type conversions as well.

Agreed, that would be a more robust long-term solution.

I tried it locally and did an i386 allmodconfig build to see what falls
out.

The most interesting thing it immediately exposes is a genuine 32-bit
truncation bug: drivers/scsi/lpfc uses

    60 * NSEC_PER_SEC

in an unsigned long, which should be 60000000000 but is silently
truncated to 4165425152 on 32-bit today.

Beyond that, switching NSEC_PER_SEC to LL falls into three classes:

 - ~7 -Wformat fixups where %lu/%ld now receive long long, e.g.
   fs/proc/uptime.c, drivers/ata/libata-transport.c,
   drivers/scsi/scsi_debug.c, i915 backlight, meson-ir-tx.

 - the lpfc-style constant truncation above.

 - the bulk: ~79 files with expressions like x * NSEC_PER_SEC / var.
   These currently stay in 32-bit arithmetic (and silently truncate
   when the product exceeds 32 bits); with LL they become 64-bit
   divisions and pull in __divdi3/__udivdi3, i.e. a link failure on
   32-bit until converted to div_u64()/do_div(). They span drivers/iio,
   spi, pwm, net, mtd, gpu, ... plus a few core files (kernel/time,
   kernel/events, net/core). (i386 only here; arm would add
   __aeabi_ldivmod and possibly more.)

So it's really a multi-subsystem cleanup series rather than a single
macro change. My inclination would be to fix the affected call sites
first (including the truncation cases), then flip NSEC_PER_SEC to LL
once the tree builds cleanly on 32-bit.

The same likely applies to the sibling constants (USEC_PER_SEC,
NSEC_PER_MSEC, ...), so it may be worth treating as a broader cleanup.

Thanks,
  Zhan