linux-kernel - [RFC PATCH] x86/kvm: do not touch watchdogs in pvclock

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 16 Jul 2021 14:34:05 +0900
From:   Sergey Senozhatsky <senozhatsky@...omium.org>
To:     Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>
Cc:     Suleiman Souhlal <suleiman@...gle.com>,
        "Paul E . McKenney" <paulmck@...nel.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Sergey Senozhatsky <senozhatsky@...omium.org>
Subject: [RFC PATCH] x86/kvm: do not touch watchdogs in pvclock

When host suspends a VM it may signal the guest that its
being suspended via KVM_KVMCLOCK_CTRL ioctl, so that once
resumed guest VCPUs can discover PVCLOCK_GUEST_STOPPED bit
and touch watchdogs to update stale timeouts.

The way it's implemented is that every kvm_clock_read() calls
pvclock_clocksource_read(), which tests PVCLOCK_GUEST_STOPPED
and invokes pvclock_touch_watchdogs() when needed. This scheme
appears not to be always working as intended. For instance,
when lockdep is enabled we have the following:

<IRQ>
apic_timer_interrupt()
 smp_apic_timer_interrupt()
  hrtimer_interrupt()
   _raw_spin_lock_irqsave()
    lock_acquire()
     __lock_acquire()
      sched_clock_cpu()
       sched_clock()
        kvm_sched_clock_read()
         kvm_clock_read()
          pvclock_clocksource_read()
           pvclock_touch_watchdogs()

Since this is VM and VCPU resume path, jiffies still maybe
be outdated here, which is often the case on my device.
pvclock_clocksource_read() clears PVCLOCK_GUEST_STOPPED,
touches watchdogs, but it uses stale jiffies: 4294740764
(for example).

Now comes in the sched tick IRQ, which invokes RCU watchdog:

<IRQ>
apic_timer_interrupt()
 smp_apic_timer_interrupt()
  hrtimer_interrupt()
   __hrtimer_run_queues()
    tick_sched_timer()
     tick_sched_handle()
      update_process_times()
       rcu_sched_clock_irq()

At this point, however, jiffies are already updated and include
the VM suspension time (approx 80 seconds in this case): 4294819216,
but PVCLOCK_GUEST_STOPPED is already cleared and we used outdated
jiffies, so RCU watchdog concludes it's a stall.

There are probably more scenarios under which resuming VCPUs can
invoke kvm_clock_read() too early.

Both lockup and RCU watchdogs call kvm_check_and_clear_guest_paused()
from hard IRQ contexts, so we probably can remove one from
pvclock_clocksource_read() and avoid preliminary PVCLOCK_GUEST_STOPPED
handling from some random paths.

That is, since kvm_check_and_clear_guest_paused() is for watchdogs
then only watchdogs should use it.

Signed-off-by: Sergey Senozhatsky <senozhatsky@...omium.org>
---
 arch/x86/kernel/kvmclock.c | 2 +-
 arch/x86/kernel/pvclock.c  | 5 -----
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index ad273e5861c1..af90b889e923 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -150,8 +150,8 @@ bool kvm_check_and_clear_guest_paused(void)
 		return ret;

 	if ((src->pvti.flags & PVCLOCK_GUEST_STOPPED) != 0) {
-		src->pvti.flags &= ~PVCLOCK_GUEST_STOPPED;
 		pvclock_touch_watchdogs();
+		src->pvti.flags &= ~PVCLOCK_GUEST_STOPPED;
 		ret = true;
 	}
 	return ret;
diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index eda37df016f0..b176e083e543 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -77,11 +77,6 @@ u64 pvclock_clocksource_read(struct pvclock_vcpu_time_info *src)
 		flags = src->flags;
 	} while (pvclock_read_retry(src, version));

-	if (unlikely((flags & PVCLOCK_GUEST_STOPPED) != 0)) {
-		src->flags &= ~PVCLOCK_GUEST_STOPPED;
-		pvclock_touch_watchdogs();
-	}
-
 	if ((valid_flags & PVCLOCK_TSC_STABLE_BIT) &&
 		(flags & PVCLOCK_TSC_STABLE_BIT))
 		return ret;
-- 
2.32.0.402.g57bb445576-goog