time: Fix races at clocksource switch time

arch/x86/vdso/vclock_gettime.c +1 -0

 	do {
 		seq = read_seqbegin(&gtod->lock);
+		mode = gtod->clock.vclock_mode;
 		ts->tv_sec = gtod->wall_time_sec;
 		ts->tv_nsec = gtod->wall_time_nsec;
 		ns = vgetns();
 	} while (unlikely(read_seqretry(&gtod->lock, seq)));

do {

Review

Quoted 4 lines · John Stultz

notrace static noinline int do_realtime(struct timespec *ts) {
unsigned long seq, ns;

(quoted diff) +1 -0

+	int mode;

Please keep a newline between declarations and code.

Quoted 9 lines · Thomas Gleixner

Quoted 5 lines · John Stultz

 notrace static noinline int do_realtime(struct timespec *ts) {
       unsigned long seq, ns;
+       int mode;

Please keep a newline between declarations and code.

Fixed below. Thanks!
(Let me know if you see whitespace damage, I switched mail clients today and am learning the quirks here.)
-john

When switching from a vsyscall capable to a non-vsyscall capable clocksource, there was a small race, where the last vsyscall gettimeofday before the switch might return a invalid time value using the new non-vsyscall enabled clocksource values after the switch is complete.

This is due to the vsyscall code checking the vclock_mode once outside of the seqcount protected section. After it reads the vclock mode, it doesn't re-check that the sampled clock data that is obtained in the seqcount critical section still matches.

The fix is to sample vclock_mode inside the protected section, and as long as it isn't VCLOCK_NONE, return the calculated value. If it has changed and is now VCLOCK_NONE, fall back to the syscall gettime calculation.

v2:
 * Cleanup checks as suggested by tglx
 * Also fix same issue present in gettimeofday path

CC Andy Lutomirski<[email protected]>

CC Thomas Gleixner<[email protected]>

Signed-off-by John Stultz<[email protected]>

---
 arch/x86/vdso/vclock_gettime.c |   68
+++++++++++++++++++++++++--------------

 1 files changed, 43 insertions(+), 25 deletions(-)

Quoted 39 lines · John Stultz

Quoted 9 lines · Thomas Gleixner

Quoted 5 lines · John Stultz

 notrace static noinline int do_realtime(struct timespec *ts) {
       unsigned long seq, ns;
+       int mode;

Please keep a newline between declarations and code.

Fixed below. Thanks!
(Let me know if you see whitespace damage, I switched mail clients today and am learning the quirks here.)
-john

When switching from a vsyscall capable to a non-vsyscall capable clocksource, there was a small race, where the last vsyscall gettimeofday before the switch might return a invalid time value using the new non-vsyscall enabled clocksource values after the switch is complete.

This is due to the vsyscall code checking the vclock_mode once outside of the seqcount protected section. After it reads the vclock mode, it doesn't re-check that the sampled clock data that is obtained in the seqcount critical section still matches.

The fix is to sample vclock_mode inside the protected section, and as long as it isn't VCLOCK_NONE, return the calculated value. If it has changed and is now VCLOCK_NONE, fall back to the syscall gettime calculation.

v2:
 * Cleanup checks as suggested by tglx
 * Also fix same issue present in gettimeofday path

Quoted 8 lines

CC Andy Lutomirski<[email protected]>

CC Thomas Gleixner<[email protected]>

Signed-off-by John Stultz<[email protected]>

---
 arch/x86/vdso/vclock_gettime.c |   68
+++++++++++++++++++++++++--------------

 1 files changed, 43 insertions(+), 25 deletions(-)

Looks reasonable to me.  I like this approach better than the earlier way -- it's likely to cause less slowdown in the VCLOCK_TSC case.

That being said, I think you might have a bug:

notrace static inline long vgetns(void)
{
       long v;
       cycles_t cycles;
       if (gtod->clock.vclock_mode == VCLOCK_TSC) cycles = vread_tsc();
       else
               cycles = vread_hpet();
       v = (cycles - gtod->clock.cycle_last) & gtod->clock.mask; return (v * gtod->clock.mult) >> gtod->clock.shift; }

In the VCLOCK_NONE, you'll access the hpet mapping.  But in hpet_enable, hpet_set_mapping isn't called and this will crash, I think.

Quoted 35 lines · John Stultz

Quoted 6 lines · Thomas Gleixner

Quoted 4 lines · John Stultz

notrace static noinline int do_realtime(struct timespec *ts)

  {
        unsigned long seq, ns;

+ int mode;

Please keep a newline between declarations and code.

Fixed below. Thanks!
(Let me know if you see whitespace damage, I switched mail clients today and am learning the quirks here.)
-john

When switching from a vsyscall capable to a non-vsyscall capable clocksource, there was a small race, where the last vsyscall gettimeofday before the switch might return a invalid time value using the new non-vsyscall enabled clocksource values after the switch is complete.

This is due to the vsyscall code checking the vclock_mode once outside of the seqcount protected section. After it reads the vclock mode, it doesn't re-check that the sampled clock data that is obtained in the seqcount critical section still matches.

The fix is to sample vclock_mode inside the protected section, and as long as it isn't VCLOCK_NONE, return the calculated value. If it has changed and is now VCLOCK_NONE, fall back to the syscall gettime calculation.

v2:
* Cleanup checks as suggested by tglx
* Also fix same issue present in gettimeofday path

Quoted 8 lines

CC Andy Lutomirski<[email protected]>

CC Thomas Gleixner<[email protected]>

Signed-off-by John Stultz<[email protected]>

---
  arch/x86/vdso/vclock_gettime.c |   68
+++++++++++++++++++++++++--------------
  1 files changed, 43 insertions(+), 25 deletions(-)

Looks reasonable to me. I like this approach better than the earlier way -- it's likely to cause less slowdown in the VCLOCK_TSC case.

That being said, I think you might have a bug:

notrace static inline long vgetns(void)

{
         long v;
         cycles_t cycles;
         if (gtod->clock.vclock_mode == VCLOCK_TSC)
                 cycles = vread_tsc();
         else
                 cycles = vread_hpet();
         v = (cycles - gtod->clock.cycle_last)&  gtod->clock.mask;
         return (v * gtod->clock.mult)>>  gtod->clock.shift;
}

In the VCLOCK_NONE, you'll access the hpet mapping. But in hpet_enable, hpet_set_mapping isn't called and this will crash, I think.

Quoted 67 lines · Andy Lutomirski

Quoted 41 lines · John Stultz

Quoted 9 lines · Thomas Gleixner

Quoted 5 lines · John Stultz

 notrace static noinline int do_realtime(struct timespec *ts) {
       unsigned long seq, ns;
+       int mode;

Please keep a newline between declarations and code.

Fixed below. Thanks!
(Let me know if you see whitespace damage, I switched mail clients today and
am learning the quirks here.)
-john

When switching from a vsyscall capable to a non-vsyscall capable clocksource, there was a small race, where the last vsyscall gettimeofday before the switch might return a invalid time value using the new non-vsyscall enabled clocksource values after the switch is complete.

This is due to the vsyscall code checking the vclock_mode once outside of the seqcount protected section. After it reads the vclock mode, it doesn't re-check that the sampled clock data that is obtained in the seqcount critical section still matches.

The fix is to sample vclock_mode inside the protected section, and as long as it isn't VCLOCK_NONE, return the calculated value. If it has changed and is now VCLOCK_NONE, fall back to the syscall gettime calculation.

v2:
 * Cleanup checks as suggested by tglx
 * Also fix same issue present in gettimeofday path

Quoted 8 lines

CC Andy Lutomirski<[email protected]>

CC Thomas Gleixner<[email protected]>

Signed-off-by John Stultz<[email protected]>

---
 arch/x86/vdso/vclock_gettime.c |   68
+++++++++++++++++++++++++--------------

 1 files changed, 43 insertions(+), 25 deletions(-)

Looks reasonable to me.  I like this approach better than the earlier way -- it's likely to cause less slowdown in the VCLOCK_TSC case.

That being said, I think you might have a bug:

notrace static inline long vgetns(void)
{
        long v;
        cycles_t cycles;
        if (gtod->clock.vclock_mode == VCLOCK_TSC) cycles = vread_tsc();
        else
                cycles = vread_hpet();
        v = (cycles - gtod->clock.cycle_last)&  gtod->clock.mask;

Quoted 6 lines

        return (v * gtod->clock.mult)>>  gtod->clock.shift; }

In the VCLOCK_NONE, you'll access the hpet mapping.  But in hpet_enable, hpet_set_mapping isn't called and this will crash, I think.

Thanks for catching this!

My solution is to add:

       else if (gtod->clock.vclock_mode == VCLOCK_HPET) cycles = vread_hpet();
       else
               return 0;

Let me know if this works for you.

thanks
-john