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

[Ingo Molnar <mingo@kernel.org>]

* 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