[patch 0/3] vdso: Unbreak VDSO with PV and HyperV clocksources

[patch 0/3] vdso: Unbreak VDSO with PV and HyperV clocksources

[Thomas Gleixner]

Miklos reported [1] that the recent VDSO changes broke paravirt clocksource
based VDSO in the case that the clocksource is invalidated by the
hypervisor which happens after a suspend/resume cycle of the host.

The result is a stale clocksource which is about 2200 seconds ahead of the
actual time and jumps forward by 2200 seconds once 2200 seconds have
elapsed.

The reason for this is the core code change which optimized the VDSO
clocksource validation by checking for the clocksource mode instead of
using the rather subtle check for the clocksource read return value whether
it has bit 63 set.

For some reason my brain blanked when doing that change, even if I should
have known better.

The following series restores the previous behaviour but preserves the
initially intended optimization for architectures which don't need that PV
handling.

Thanks,

	tglx

[1] https://lore.kernel.org/r/CAJfpegstNYeseo_C4KOF9Y74qRxr78x2tK-9rTgmYM4CK30nRQ@mail.gmail.com

8<-----------------
 arch/x86/include/asm/vdso/gettimeofday.h |   18 ++++++++++++++++++
 kernel/time/clocksource.c                |    2 --
 lib/vdso/gettimeofday.c                  |   19 +++++++++++++++++++
 3 files changed, 37 insertions(+), 2 deletions(-)

[patch 1/3] clocksource: Remove obsolete ifdef

[Thomas Gleixner]

CONFIG_GENERIC_VDSO_CLOCK_MODE was a transitional config switch which got
removed after all architectures got converted to the new storage model.

But the removal forgot to remove the #ifdef which guards the
vdso_clock_mode sanity check, which effectively disables the sanity check.

Remove it now.

Fixes: f86fd32db706 ("lib/vdso: Cleanup clock mode storage leftovers")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
 kernel/time/clocksource.c |    2 --
 1 file changed, 2 deletions(-)

--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -928,14 +928,12 @@ int __clocksource_register_scale(struct
 
 	clocksource_arch_init(cs);
 
-#ifdef CONFIG_GENERIC_VDSO_CLOCK_MODE
 	if (cs->vdso_clock_mode < 0 ||
 	    cs->vdso_clock_mode >= VDSO_CLOCKMODE_MAX) {
 		pr_warn("clocksource %s registered with invalid VDSO mode %d. Disabling VDSO support.\n",
 			cs->name, cs->vdso_clock_mode);
 		cs->vdso_clock_mode = VDSO_CLOCKMODE_NONE;
 	}
-#endif
 
 	/* Initialize mult/shift and max_idle_ns */
 	__clocksource_update_freq_scale(cs, scale, freq);

[patch 2/3] lib/vdso: Provide sanity check for cycles (again)

[Thomas Gleixner]

The original x86 VDSO implementation checked for the validity of the clock
source read by testing whether the returned signed cycles value is less
than zero. This check was also used by the vdso read function to signal
that the current selected clocksource is not VDSO capable.

During the rework of the VDSO code the check was removed and replaced with
a check for the clocksource mode being != NONE.

This turned out to be a mistake because the check is necessary for paravirt
and hyperv clock sources. The reason is that these clock sources have their
own internal sequence counter to validate the clocksource at the point of
reading it. This is necessary because the hypervisor can invalidate the
clocksource asynchronously so a check during the VDSO data update is not
sufficient. Having a separate indicator for the validity is slower than
just validating the cycles value. The check for it being negative turned
out to be the fastest implementation and safe as it would require an uptime
of ~73 years with a 4GHz counter frequency to result in a false positive.

Add an optional function to validate the cycles with a default
implementation which allows the compiler to optimize it out for
architectures which do not require it.

Reported-by: Miklos Szeredi <miklos@szeredi.hu>
Fixes: 5d51bee725cc ("clocksource: Add common vdso clock mode storage")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
 lib/vdso/gettimeofday.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

--- a/lib/vdso/gettimeofday.c
+++ b/lib/vdso/gettimeofday.c
@@ -38,6 +38,13 @@ static inline bool vdso_clocksource_ok(c
 }
 #endif
 
+#ifndef vdso_cycles_ok
+static inline bool vdso_cycles_ok(u64 cycles)
+{
+	return true;
+}
+#endif
+
 #ifdef CONFIG_TIME_NS
 static int do_hres_timens(const struct vdso_data *vdns, clockid_t clk,
 			  struct __kernel_timespec *ts)
@@ -62,6 +69,8 @@ static int do_hres_timens(const struct v
 			return -1;
 
 		cycles = __arch_get_hw_counter(vd->clock_mode);
+		if (unlikely(!vdso_cycles_ok(cycles)))
+			return -1;
 		ns = vdso_ts->nsec;
 		last = vd->cycle_last;
 		ns += vdso_calc_delta(cycles, last, vd->mask, vd->mult);
@@ -130,6 +139,8 @@ static __always_inline int do_hres(const
 			return -1;
 
 		cycles = __arch_get_hw_counter(vd->clock_mode);
+		if (unlikely(!vdso_cycles_ok(cycles)))
+			return -1;
 		ns = vdso_ts->nsec;
 		last = vd->cycle_last;
 		ns += vdso_calc_delta(cycles, last, vd->mask, vd->mult);

[patch 3/3] x86/vdso: Unbreak paravirt VDSO clocks

[Thomas Gleixner]

The conversion of x86 VDSO to the generic clock mode storage broke the
paravirt and hyperv clocksource logic. These clock sources have their own
internal sequence counter to validate the clocksource at the point of
reading it. This is necessary because the hypervisor can invalidate the
clocksource asynchronously so a check during the VDSO data update is not
sufficient. If the internal check during read invalidates the clocksource
the read return U64_MAX. The original code checked this efficiently by
testing whether the result (casted to signed) is negative, i.e. bit 63 is
set. This was done that way because an extra indicator for the validity had
more overhead.

The conversion broke this check because the check was replaced by a check
for a valid VDSO clock mode.

The wreckage manifests itself when the paravirt clock is installed as a
valid VDSO clock and during runtime invalidated by the hypervisor,
e.g. after a host suspend/resume cycle. After the invalidation the read
function returns U64_MAX which is used as cycles and makes the clock jump
by ~2200 seconds, and become stale until the 2200 seconds have elapsed
where it starts to jump again. The period of this effect depends on the
shift/mult pair of the clocksource and the jumps and staleness are an
artifact of undefined but reproducible behaviour of math overflow.

Implement an x86 version of the new vdso_cycles_ok() inline which adds this
check back and a variant of vdso_clocksource_ok() which lets the compiler
optimize it out to avoid the extra conditional. That's suboptimal when the
system does not have a VDSO capable clocksource, but that's not the case
which is optimized for.

Reported-by: Miklos Szeredi <miklos@szeredi.hu>
Fixes: 5d51bee725cc ("clocksource: Add common vdso clock mode storage")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
 arch/x86/include/asm/vdso/gettimeofday.h |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

--- a/arch/x86/include/asm/vdso/gettimeofday.h
+++ b/arch/x86/include/asm/vdso/gettimeofday.h
@@ -271,6 +271,24 @@ static __always_inline const struct vdso
 	return __vdso_data;
 }
 
+static inline bool arch_vdso_clocksource_ok(const struct vdso_data *vd)
+{
+	return true;
+}
+#define vdso_clocksource_ok arch_vdso_clocksource_ok
+
+/*
+ * Clocksource read value validation to handle PV and HyperV clocksources
+ * which can be invalidated asynchronously and indicate invalidation by
+ * returning U64_MAX, which can be effectively tested by checking for a
+ * negative value after casting it to s64.
+ */
+static inline bool arch_vdso_cycles_ok(u64 cycles)
+{
+	return (s64)cycles >= 0;
+}
+#define vdso_cycles_ok arch_vdso_cycles_ok
+
 /*
  * x86 specific delta calculation.
  *

Re: [patch 0/3] vdso: Unbreak VDSO with PV and HyperV clocksources

[Miklos Szeredi]

On Sun, Jun 7, 2020 at 11:36 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Miklos reported [1] that the recent VDSO changes broke paravirt clocksource
> based VDSO in the case that the clocksource is invalidated by the
> hypervisor which happens after a suspend/resume cycle of the host.
>
> The result is a stale clocksource which is about 2200 seconds ahead of the
> actual time and jumps forward by 2200 seconds once 2200 seconds have
> elapsed.
>
> The reason for this is the core code change which optimized the VDSO
> clocksource validation by checking for the clocksource mode instead of
> using the rather subtle check for the clocksource read return value whether
> it has bit 63 set.
>
> For some reason my brain blanked when doing that change, even if I should
> have known better.
>
> The following series restores the previous behaviour but preserves the
> initially intended optimization for architectures which don't need that PV
> handling.

Thanks for fixing.

Tested-by: Miklos Szeredi <mszeredi@redhat.com>

Miklos

[tip: x86/urgent] x86/vdso: Unbreak paravirt VDSO clocks

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     7778d8417b74aded842eeb372961cfc460417fa0
Gitweb:        https://git.kernel.org/tip/7778d8417b74aded842eeb372961cfc460417fa0
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Sat, 06 Jun 2020 23:51:17 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 09 Jun 2020 16:36:49 +02:00

x86/vdso: Unbreak paravirt VDSO clocks

The conversion of x86 VDSO to the generic clock mode storage broke the
paravirt and hyperv clocksource logic. These clock sources have their own
internal sequence counter to validate the clocksource at the point of
reading it. This is necessary because the hypervisor can invalidate the
clocksource asynchronously so a check during the VDSO data update is not
sufficient. If the internal check during read invalidates the clocksource
the read return U64_MAX. The original code checked this efficiently by
testing whether the result (casted to signed) is negative, i.e. bit 63 is
set. This was done that way because an extra indicator for the validity had
more overhead.

The conversion broke this check because the check was replaced by a check
for a valid VDSO clock mode.

The wreckage manifests itself when the paravirt clock is installed as a
valid VDSO clock and during runtime invalidated by the hypervisor,
e.g. after a host suspend/resume cycle. After the invalidation the read
function returns U64_MAX which is used as cycles and makes the clock jump
by ~2200 seconds, and become stale until the 2200 seconds have elapsed
where it starts to jump again. The period of this effect depends on the
shift/mult pair of the clocksource and the jumps and staleness are an
artifact of undefined but reproducible behaviour of math overflow.

Implement an x86 version of the new vdso_cycles_ok() inline which adds this
check back and a variant of vdso_clocksource_ok() which lets the compiler
optimize it out to avoid the extra conditional. That's suboptimal when the
system does not have a VDSO capable clocksource, but that's not the case
which is optimized for.

Fixes: 5d51bee725cc ("clocksource: Add common vdso clock mode storage")
Reported-by: Miklos Szeredi <miklos@szeredi.hu>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Miklos Szeredi <mszeredi@redhat.com>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/20200606221532.080560273@linutronix.de

---
 arch/x86/include/asm/vdso/gettimeofday.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/x86/include/asm/vdso/gettimeofday.h b/arch/x86/include/asm/vdso/gettimeofday.h
index 9a6dc9b..fb81fea 100644
--- a/arch/x86/include/asm/vdso/gettimeofday.h
+++ b/arch/x86/include/asm/vdso/gettimeofday.h
@@ -271,6 +271,24 @@ static __always_inline const struct vdso_data *__arch_get_vdso_data(void)
 	return __vdso_data;
 }
 
+static inline bool arch_vdso_clocksource_ok(const struct vdso_data *vd)
+{
+	return true;
+}
+#define vdso_clocksource_ok arch_vdso_clocksource_ok
+
+/*
+ * Clocksource read value validation to handle PV and HyperV clocksources
+ * which can be invalidated asynchronously and indicate invalidation by
+ * returning U64_MAX, which can be effectively tested by checking for a
+ * negative value after casting it to s64.
+ */
+static inline bool arch_vdso_cycles_ok(u64 cycles)
+{
+	return (s64)cycles >= 0;
+}
+#define vdso_cycles_ok arch_vdso_cycles_ok
+
 /*
  * x86 specific delta calculation.
  *

[tip: x86/urgent] lib/vdso: Provide sanity check for cycles (again)

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     72ce778007e57e8996b4bebdec738fc5e1145fd2
Gitweb:        https://git.kernel.org/tip/72ce778007e57e8996b4bebdec738fc5e1145fd2
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Sat, 06 Jun 2020 23:51:16 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 09 Jun 2020 16:36:48 +02:00

lib/vdso: Provide sanity check for cycles (again)

The original x86 VDSO implementation checked for the validity of the clock
source read by testing whether the returned signed cycles value is less
than zero. This check was also used by the vdso read function to signal
that the current selected clocksource is not VDSO capable.

During the rework of the VDSO code the check was removed and replaced with
a check for the clocksource mode being != NONE.

This turned out to be a mistake because the check is necessary for paravirt
and hyperv clock sources. The reason is that these clock sources have their
own internal sequence counter to validate the clocksource at the point of
reading it. This is necessary because the hypervisor can invalidate the
clocksource asynchronously so a check during the VDSO data update is not
sufficient. Having a separate indicator for the validity is slower than
just validating the cycles value. The check for it being negative turned
out to be the fastest implementation and safe as it would require an uptime
of ~73 years with a 4GHz counter frequency to result in a false positive.

Add an optional function to validate the cycles with a default
implementation which allows the compiler to optimize it out for
architectures which do not require it.

Fixes: 5d51bee725cc ("clocksource: Add common vdso clock mode storage")
Reported-by: Miklos Szeredi <miklos@szeredi.hu>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Miklos Szeredi <mszeredi@redhat.com>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/20200606221531.963970768@linutronix.de

---
 lib/vdso/gettimeofday.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/lib/vdso/gettimeofday.c b/lib/vdso/gettimeofday.c
index a2909af..3bb82a6 100644
--- a/lib/vdso/gettimeofday.c
+++ b/lib/vdso/gettimeofday.c
@@ -38,6 +38,13 @@ static inline bool vdso_clocksource_ok(const struct vdso_data *vd)
 }
 #endif
 
+#ifndef vdso_cycles_ok
+static inline bool vdso_cycles_ok(u64 cycles)
+{
+	return true;
+}
+#endif
+
 #ifdef CONFIG_TIME_NS
 static int do_hres_timens(const struct vdso_data *vdns, clockid_t clk,
 			  struct __kernel_timespec *ts)
@@ -62,6 +69,8 @@ static int do_hres_timens(const struct vdso_data *vdns, clockid_t clk,
 			return -1;
 
 		cycles = __arch_get_hw_counter(vd->clock_mode);
+		if (unlikely(!vdso_cycles_ok(cycles)))
+			return -1;
 		ns = vdso_ts->nsec;
 		last = vd->cycle_last;
 		ns += vdso_calc_delta(cycles, last, vd->mask, vd->mult);
@@ -130,6 +139,8 @@ static __always_inline int do_hres(const struct vdso_data *vd, clockid_t clk,
 			return -1;
 
 		cycles = __arch_get_hw_counter(vd->clock_mode);
+		if (unlikely(!vdso_cycles_ok(cycles)))
+			return -1;
 		ns = vdso_ts->nsec;
 		last = vd->cycle_last;
 		ns += vdso_calc_delta(cycles, last, vd->mask, vd->mult);

[tip: x86/urgent] clocksource: Remove obsolete ifdef

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     c7f3d43b629b598a2bb9ec3524e844eae7492e7e
Gitweb:        https://git.kernel.org/tip/c7f3d43b629b598a2bb9ec3524e844eae7492e7e
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Sat, 06 Jun 2020 23:51:15 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 09 Jun 2020 16:36:47 +02:00

clocksource: Remove obsolete ifdef

CONFIG_GENERIC_VDSO_CLOCK_MODE was a transitional config switch which got
removed after all architectures got converted to the new storage model.

But the removal forgot to remove the #ifdef which guards the
vdso_clock_mode sanity check, which effectively disables the sanity check.

Remove it now.

Fixes: f86fd32db706 ("lib/vdso: Cleanup clock mode storage leftovers")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Miklos Szeredi <mszeredi@redhat.com>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/20200606221531.845475036@linutronix.de

---
 kernel/time/clocksource.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index 7cb09c4..02441ea 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -928,14 +928,12 @@ int __clocksource_register_scale(struct clocksource *cs, u32 scale, u32 freq)
 
 	clocksource_arch_init(cs);
 
-#ifdef CONFIG_GENERIC_VDSO_CLOCK_MODE
 	if (cs->vdso_clock_mode < 0 ||
 	    cs->vdso_clock_mode >= VDSO_CLOCKMODE_MAX) {
 		pr_warn("clocksource %s registered with invalid VDSO mode %d. Disabling VDSO support.\n",
 			cs->name, cs->vdso_clock_mode);
 		cs->vdso_clock_mode = VDSO_CLOCKMODE_NONE;
 	}
-#endif
 
 	/* Initialize mult/shift and max_idle_ns */
 	__clocksource_update_freq_scale(cs, scale, freq);