linux-kernel - Re: [PATCH] clocksource: Make clocksource watchdog check with WATCHDOG

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: <fa2d516e-70b5-3012-9134-5ca325282bc4@redhat.com>
Date:   Fri, 27 May 2022 22:07:59 -0400
From:   Waiman Long <longman@...hat.com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        Stephen Boyd <sboyd@...nel.org>,
        Feng Tang <feng.tang@...el.com>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        John Stultz <jstultz@...gle.com>
Cc:     linux-kernel@...r.kernel.org,
        Michael Larabel <Michael@...ronix.com>,
        Joe Mario <jmario@...hat.com>,
        Michey Mehta <mimehta@...hat.com>
Subject: Re: [PATCH] clocksource: Make clocksource watchdog check with
 WATCHDOG_INTERVAL period

On 5/27/22 21:57, Waiman Long wrote:
> Since commit c86ff8c55b8a ("clocksource: Avoid accidental unstable
> marking of clocksource"), a new WD_READ_SKIP value was introduced
> as a possible return value of cs_watchdog_read() to skip the current
> check. However, this has an undesriable side effect of extending the
> time gap between csnow and cs_last to more than one WATCHDOG_INTERVAL
> (0.5s) in case of intermittent WD_READ_SKIP's.
>
> There was an instance of reported clocksource watchdog failure with
> the time skew of 485us where the uncertainly threshold is 400us. In
> that particular case, the (now - last) gap was about 2s. Looking at
> the dmesg log, it was clear there was a successful cs_watchdog_read()
> followed by 3 skips and then another successful cs_watchdog_read().
>
> If there is an existing skew between the hpet (watchdog) and tsc
> clocksource, enlarging the period by 4x will certainly increase the
> measured skew causing it to exceed the threshold in this case. Fix
> this variable period problem by resetting the CLOCK_SOURCE_WATCHDOG bit
> after each WD_READ_SKIP to force the reloading of wd_last and cs_last
> in the next round. This ensures that we have two consecutive successful
> cs_watchdog_read()'s before checking the clock skew.
>
> Fixes: c86ff8c55b8a ("clocksource: Avoid accidental unstable marking of clocksource")
> Reported-by: Michael Larabel <Michael@...ronix.com>
> Signed-off-by: Waiman Long <longman@...hat.com>
> ---
>   kernel/time/clocksource.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> index cee5da1e54c4..173e052c12b6 100644
> --- a/kernel/time/clocksource.c
> +++ b/kernel/time/clocksource.c
> @@ -411,9 +411,18 @@ static void clocksource_watchdog(struct timer_list *unused)
>   		read_ret = cs_watchdog_read(cs, &csnow, &wdnow);
>   
>   		if (read_ret != WD_READ_SUCCESS) {
> -			if (read_ret == WD_READ_UNSTABLE)
> +			if (read_ret == WD_READ_UNSTABLE) {
>   				/* Clock readout unreliable, so give it up. */
>   				__clocksource_unstable(cs);
> +			} else { /* WD_READ_SKIP */
> +				/*
> +				 * Watchdog clock unstable at the moment,
> +				 * discard the stored wd_last and cs_last to
> +				 * make sure the gap between now and last
> +				 * is always one WATCHDOG_INTERVAL.
> +				 */
> +				cs->flags &= ~CLOCK_SOURCE_WATCHDOG;
> +			}
>   			continue;
>   		}
>   

Sorry, I accidentally use the old email address for John.

Cheers,
Longman