linux-kernel - Re: [RFC PATCH] powerpc/32: Switch VDSO to C implementation.

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: <09d07ad3-47a2-db2f-2f14-e002b22d8d9e@c-s.fr>
Date:   Thu, 9 Jan 2020 15:21:12 +0000
From:   Christophe Leroy <christophe.leroy@....fr>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        vincenzo.frascino@....com, luto@...nel.org,
        linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [RFC PATCH] powerpc/32: Switch VDSO to C implementation.

Hi Thomas,

On 01/09/2020 02:05 PM, Thomas Gleixner wrote:
> Christophe!
> 
> Christophe Leroy <christophe.leroy@....fr> writes:
>> In do_hres(), I see:
>>
>> 		cycles = __arch_get_hw_counter(vd->clock_mode);
>> 		ns = vdso_ts->nsec;
>> 		last = vd->cycle_last;
>> 		if (unlikely((s64)cycles < 0))
>> 			return -1;
>>
>> __arch_get_hw_counter() returns a u64 values. On the PPC, this is read
>> from the timebase which is a 64 bits counter.
>>
>> Why returning -1 if (s64)cycles < 0 ? Does it means we have to mask out
>> the most significant bit when reading the HW counter ?
> 
> Only if you expect the HW counter to reach a value which has bit 63
> set. That'd require:
> 
> uptime		counter frequency
> 
> ~292 years      1GHz
> ~ 58 years      5GHz
> 
> assumed that the HW counter starts at 0 when the box is powered on.
> 
> The reason why this is implemented in this way is that
> __arch_get_hw_counter() needs a way to express that the clocksource of
> the moment is not suitable for VDSO so that the syscall fallback gets
> invoked.
> 
> Sure we could have used a pointer for the value and a return value
> indicating the validity, but given the required uptime the resulting
> code overhead seemed to be not worth it. At least not for me as I'm not
> planning to be around 58 years from now :)
> 

I managed to get better code and better performance by splitting out the 
validity check as follows. Would it be suitable for all arches ?


diff --git a/arch/powerpc/include/asm/vdso/gettimeofday.h 
b/arch/powerpc/include/asm/vdso/gettimeofday.h
index 689f51b0d8c9..11cdd6faa4ad 100644
--- a/arch/powerpc/include/asm/vdso/gettimeofday.h
+++ b/arch/powerpc/include/asm/vdso/gettimeofday.h
@@ -114,15 +114,17 @@ int clock_getres32_fallback(clockid_t _clkid, 
struct old_timespec32 *_ts)
  	return ret;
  }

-static __always_inline u64 __arch_get_hw_counter(s32 clock_mode)
+static __always_inline bool __arch_is_hw_counter_valid(s32 clock_mode)
  {
  	/*
  	 * clock_mode == 0 implies that vDSO are enabled otherwise
  	 * fallback on syscall.
  	 */
-	if (clock_mode)
-		return ULLONG_MAX;
+	return clock_mode ? false : true;
+}

+static __always_inline u64 __arch_get_hw_counter(s32 clock_mode)
+{
  	return get_tb();
  }

diff --git a/lib/vdso/gettimeofday.c b/lib/vdso/gettimeofday.c
index ee9da52a3e02..90bb5dfd0db0 100644
--- a/lib/vdso/gettimeofday.c
+++ b/lib/vdso/gettimeofday.c
@@ -46,11 +46,12 @@ static inline int do_hres(const struct vdso_data 
*vd, clockid_t clk,

  	do {
  		seq = vdso_read_begin(vd);
+		if (!__arch_is_hw_counter_valid(vd->clock_mode))
+			return -1;
+
  		cycles = __arch_get_hw_counter(vd->clock_mode);
  		ns = vdso_ts->nsec;
  		last = vd->cycle_last;
-		if (unlikely((s64)cycles < 0))
-			return -1;

  		ns += vdso_calc_delta(cycles, last, vd->mask, vd->mult);
  		ns >>= vd->shift;


Thanks
Christophe