linux-kernel - Re: [PATCH v5 02/11] timekeeping: Add function to convert realtime to base clock

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: <87le6bhc0t.ffs@tglx>
Date: Thu, 21 Mar 2024 15:53:22 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: lakshmi.sowjanya.d@...el.com, jstultz@...gle.com, giometti@...eenne.com,
 corbet@....net, linux-kernel@...r.kernel.org
Cc: x86@...nel.org, netdev@...r.kernel.org, linux-doc@...r.kernel.org,
 intel-wired-lan@...ts.osuosl.org, andriy.shevchenko@...ux.intel.com,
 eddie.dong@...el.com, christopher.s.hall@...el.com,
 jesse.brandeburg@...el.com, davem@...emloft.net,
 alexandre.torgue@...s.st.com, joabreu@...opsys.com,
 mcoquelin.stm32@...il.com, perex@...ex.cz, linux-sound@...r.kernel.org,
 anthony.l.nguyen@...el.com, peter.hilber@...nsynergy.com,
 pandith.n@...el.com, mallikarjunappa.sangannavar@...el.com,
 subramanian.mohan@...el.com, basavaraj.goudar@...el.com,
 thejesh.reddy.t.r@...el.com, lakshmi.sowjanya.d@...el.com
Subject: Re: [PATCH v5 02/11] timekeeping: Add function to convert realtime
 to base clock

On Tue, Mar 19 2024 at 18:35, lakshmi.sowjanya.d@...el.com wrote:
> +bool ktime_real_to_base_clock(ktime_t treal, enum clocksource_ids base_id, u64 *cycles)
> +{
> +	struct timekeeper *tk = &tk_core.timekeeper;
> +	unsigned int seq;
> +	u64 delta;
> +
> +	do {
> +		seq = read_seqcount_begin(&tk_core.seq);
> +		delta = (u64)treal - tk->tkr_mono.base_real;
> +		if (delta > tk->tkr_mono.clock->max_idle_ns)
> +			return false;

I don't think this cutoff is valid. There is no guarantee that this is
linear unless:

       Treal[last timekeeper update] <= treal < Treal[next timekeeper update]

Look at the dance in get_device_system_crosststamp() and
adjust_historical_crosststamp() to see why.

> +		*cycles = tk->tkr_mono.cycle_last + convert_ns_to_cs(delta);
> +		if (!convert_cs_to_base(cycles, base_id))
> +			return false;
> +	} while (read_seqcount_retry(&tk_core.seq, seq));
> +
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(ktime_real_to_base_clock);

Looking at the usage site:

> +static bool pps_generate_next_pulse(struct pps_tio *tio, ktime_t expires)
> +{
> +	u64 art;
> +
> +	if (!ktime_real_to_base_clock(expires, CSID_X86_ART, &art)) {
> +		pps_tio_disable(tio);

I'm pretty sure this can happen when there is sufficient delay between
the check for (now - expires < SAFE_TIME_NS) and the delta computation
in ktime_real_to_base_clock() if there is a timerkeeper update
interleaving which brings tkr_mono.base_real in front of expires.

Is that intentional and correct?

If so, then it's inconsistent with the behaviour of the hrtimer
callback:

> +		return false;
> +	}
> +
> +	pps_compv_write(tio, art - ART_HW_DELAY_CYCLES);
> +	return true;
> +}
> +
> +static enum hrtimer_restart hrtimer_callback(struct hrtimer *timer)
> +{
> +	struct pps_tio *tio = container_of(timer, struct pps_tio, timer);
> +	ktime_t expires, now;
> +
> +	guard(spinlock)(&tio->lock);
> +
> +	expires = hrtimer_get_expires(timer);
> +	now = ktime_get_real();
> +
> +	if (now - expires < SAFE_TIME_NS) {
> +		if (!pps_generate_next_pulse(tio, expires + SAFE_TIME_NS))
> +			return HRTIMER_NORESTART;
> +	}

This safe guard does not care about time being set. I'm not familiar
with the PPS logic, but is it expected that the pulse pattern will be
like this:

         

    ---|-----|-----|-----|----------------->
       P     P  ^        P
                |
        clock_settime(CLOCK_REALTIME, now - 2 seconds)         
        
        Obviously the pulse gap will be as big as the time is set
        backwards, which might be way more than 2 seconds.
        

    ---|-----|-----|-----|----------------->
       P     P  ^  P     P
                |
        clock_settime(CLOCK_REALTIME, now + 2 seconds)         

I don't see anything in this code which cares about CLOCK_REALTIME being
set via clock_settime() or adjtimex().

Aside of that I have a question about how the TIO hardware treats this
case:

   ktime_real_to_base_clock(expires, &art);

-> GAP which makes @art get into the past

   pps_compv_write(tio, art - ART_HW_DELAY_CYCLES);

Will the hardware ignore that already expired value or just emit a pulse
immediately? In the latter case the pulse will be at a random point in
time, which does not sound correct.

Thanks,

        tglx