linux-kernel - Re: [PATCH 0/2] nohz: Deal with clock reprogram skipping issues v3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170526021051.GA27061@lerouge>
Date:   Fri, 26 May 2017 04:10:53 +0200
From:   Frederic Weisbecker <fweisbec@...il.com>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Rik van Riel <riel@...hat.com>,
        James Hartsock <hartsjc@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>, stable@...r.kernel.org,
        Tim Wright <tim@...bash.co.uk>, Pavel Machek <pavel@....cz>
Subject: Re: [PATCH 0/2] nohz: Deal with clock reprogram skipping issues v3

On Wed, May 24, 2017 at 09:16:28AM +0200, Ingo Molnar wrote:
> So the interdiff between your two patches and the 3 commits already queued up is:
> 
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index e3043873fcdc..30253ed0380b 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -150,12 +150,6 @@ static void tick_sched_handle(struct tick_sched *ts, struct pt_regs *regs)
>  		touch_softlockup_watchdog_sched();
>  		if (is_idle_task(current))
>  			ts->idle_jiffies++;
> -		/*
> -		 * In case the current tick fired too early past its expected
> -		 * expiration, make sure we don't bypass the next clock reprogramming
> -		 * to the same deadline.
> -		 */
> -		ts->next_tick = 0;
>  	}
>  #endif
>  	update_process_times(user_mode(regs));
> @@ -1103,8 +1097,15 @@ static void tick_nohz_handler(struct clock_event_device *dev)
>  	tick_sched_handle(ts, regs);
>  
>  	/* No need to reprogram if we are running tickless  */
> -	if (unlikely(ts->tick_stopped))
> +	if (unlikely(ts->tick_stopped)) {
> +		/*
> +		 * In case the current tick fired too early past its expected
> +		 * expiration, make sure we don't bypass the next clock reprogramming
> +		 * to the same deadline.
> +		 */
> +		ts->next_tick = 0;
>  		return;
> +	}
>  
>  	hrtimer_forward(&ts->sched_timer, now, tick_period);
>  	tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1);
> @@ -1202,12 +1203,17 @@ static enum hrtimer_restart tick_sched_timer(struct hrtimer *timer)
>  	 */
>  	if (regs)
>  		tick_sched_handle(ts, regs);
> -	else
> -		ts->next_tick = 0;
>  
>  	/* No need to reprogram if we are in idle or full dynticks mode */
> -	if (unlikely(ts->tick_stopped))
> +	if (unlikely(ts->tick_stopped)) {
> +		/*
> +		 * In case the current tick fired too early past its expected
> +		 * expiration, make sure we don't bypass the next clock reprogramming
> +		 * to the same deadline.
> +		 */
> +		ts->next_tick = 0;
>  		return HRTIMER_NORESTART;
> +	}
>  
>  	hrtimer_forward(timer, now, tick_period);
>  
> 
> ... so the two are not the same - I'd rather not rebase it, I'd like to keep what 
> is working, we had problems with these changes before ...
> 
> If you'd like the changes in this interdiff to be applied as well, please add a 
> changelog to it and post it as a fourth patch.
> 
> Thanks,
> 
> 	Ingo

So if you like, you can replace the top patch with the following. It's exactly
the same code, I've only added a comment and a changelog:

---
>From 72956bf08c3b2e506a5ce5ec4faac9fd6b097307 Mon Sep 17 00:00:00 2001
From: Frederic Weisbecker <fweisbec@...il.com>
Date: Mon, 15 May 2017 14:56:50 +0200
Subject: [PATCH] nohz: Reset next_tick cache even when the timer has no regs

The tick IRQ regs can be NULL if hrtimer_interrupt() is called from
non-interrupt contexts (ex: hotplug CPU down). For such very special
path we forget to clean the cached next tick deadline. If we are in
dynticks mode and the actual timer deadline is ahead of us, we might
perform a buggy bypass of the next clock reprogramming.

In fact since CPU down is the only user I'm aware of, this fix is likely
unnecessary as dying CPUs already clean their tick deadline cache. But
given how hard it is to debug such timer cache related issue, we should
never be short on paranoid measures.

Signed-off-by: Frederic Weisbecker <fweisbec@...il.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Peter Zijlstra <peterz@...radead.org>
Cc: Thomas Gleixner <tglx@...utronix.de>
Signed-off-by: Ingo Molnar <mingo@...nel.org>
---
 kernel/time/tick-sched.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 764d290..ed18ca5 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -1200,8 +1200,17 @@ static enum hrtimer_restart tick_sched_timer(struct hrtimer *timer)
 	 * Do not call, when we are not in irq context and have
 	 * no valid regs pointer
 	 */
-	if (regs)
+	if (regs) {
 		tick_sched_handle(ts, regs);
+	} else {
+		/*
+		 * IRQ regs are NULL if hrtimer_interrupt() is called from
+		 * non-interrupt contexts (ex: hotplug cpu down). Make sure to
+		 * clean the cached next tick deadline to avoid buggy bypass of
+		 * clock reprog.
+		 */
+		ts->next_tick = 0;
+	}
 
 	/* No need to reprogram if we are in idle or full dynticks mode */
 	if (unlikely(ts->tick_stopped))
-- 
2.7.4