linux-kernel - Re: [PATCH v7 clocksource 2/5] clocksource: Retry clock read if long delays detected

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]

Date:   Sat, 10 Apr 2021 10:41:21 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     paulmck@...nel.org, linux-kernel@...r.kernel.org
Cc:     john.stultz@...aro.org, sboyd@...nel.org, corbet@....net,
        Mark.Rutland@....com, maz@...nel.org, kernel-team@...com,
        neeraju@...eaurora.org, ak@...ux.intel.com,
        "Paul E. McKenney" <paulmck@...nel.org>
Subject: Re: [PATCH v7 clocksource 2/5] clocksource: Retry clock read if long delays detected

On Fri, Apr 02 2021 at 15:49, paulmck wrote:
> This commit therefore re-reads the watchdog clock on either side of

'This commit' is not any better than 'This patch' and this sentence
makes no sense. I might be missing something, but how exactly does "the
commit" re-read the watchdog clock?

 git grep 'This patch' Documentation/process/

> the read from the clock under test.  If the watchdog clock shows an
> +retry:
>  		local_irq_disable();
> -		csnow = cs->read(cs);
> -		clocksource_watchdog_inject_delay();
>  		wdnow = watchdog->read(watchdog);
> +		clocksource_watchdog_inject_delay();
> +		csnow = cs->read(cs);
> +		wdagain = watchdog->read(watchdog);
>  		local_irq_enable();
> +		delta = clocksource_delta(wdagain, wdnow, watchdog->mask);
> +		wdagain_nsec = clocksource_cyc2ns(delta, watchdog->mult, watchdog->shift);

That variable naming is confusing as hell. This is about the delta and
not about the second readout of the watchdog.

> +		if (wdagain_nsec < 0 || wdagain_nsec > WATCHDOG_MAX_SKEW) {

How exactly is this going negative especially with clocksources which
have a limited bitwidth? See clocksource_delta().

> +			wderr_nsec = wdagain_nsec;
> +			if (nretries++ < max_read_retries)
> +				goto retry;
> +		}
> +		if (nretries)
> +			pr_warn("timekeeping watchdog on CPU%d: %s read-back delay of %lldns, attempt %d\n",
> +				smp_processor_id(), watchdog->name, wderr_nsec, nretries);

Lacks curly braces around the pr_warn() simply because it's not a single
line. Breaks my parser :)

But if this ever happens to exceed max_read_retries, then what's the
point of continuing at all? The data is known to be crap already.

>  		/* Clocksource initialized ? */
>  		if (!(cs->flags & CLOCK_SOURCE_WATCHDOG) ||

Thanks,

        tglx