Re: [PATCH v2 20/20] KVM: x86: Use gfn_to_pfn_cache for record_steal_time

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Calling sleepable operations while holding an rwlock (gpc->lock) in record_steal_time() causes 'scheduling while atomic' bugs.
- [High] Calling read_unlock() on an RT rwlock from sched_out() triggers wake_up_process() while rq->lock is held, causing an ABBA deadlock.
--

commit cb799f9970f13f02eaea4c9d6c87fa7537434bcf
Author: Carsten Stollmaier <stollmc@amazon.com>
KVM: x86: Use gfn_to_pfn_cache for record_steal_time

This commit reverts to using the gfn_to_pfn_cache for guest steal time and
preempted status updates to avoid mmap_lock contention with userfaultfd. It
replaces unsafe_put_user accesses with plain C reads and writes. It also
uses a read_trylock in the scheduler path to avoid sleeping under PREEMPT_RT.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3746,42 +3744,20 @@ static void record_steal_time(struct kvm_vcpu *vcpu)
[ ... ]
> +	CLASS(gpc_map_local, st_map)(gpc, sizeof(*st));
> +	if (IS_ERR(st_map))
> +		return;
>  
> -	st = (struct kvm_steal_time __user *)ghc->hva;
>  	/*
>  	 * Doing a TLB flush here, on the guest's behalf, can avoid
>  	 * expensive IPIs.
>  	 */
>  	if (guest_pv_has(vcpu, KVM_FEATURE_PV_TLB_FLUSH)) {
[ ... ]
> +		u8 st_preempted = xchg(&st->preempted, 0);
>  
>  		vcpu->arch.st.preempted = 0;
>  
> @@ -3789,39 +3765,30 @@ static void record_steal_time(struct kvm_vcpu *vcpu)
>  				       st_preempted & KVM_VCPU_FLUSH_TLB);
>  		if (st_preempted & KVM_VCPU_FLUSH_TLB)
>  			kvm_vcpu_flush_tlb_guest(vcpu);

[Severity: High]
Does this introduce a scheduling while atomic bug on non-PREEMPT_RT kernels?

The CLASS(gpc_map_local, st_map) macro acquires a read_lock on gpc->lock,
which disables preemption. While this lock is held, if the guest supports
PV TLB flush, the code calls kvm_vcpu_flush_tlb_guest().

If TDP is disabled (shadow paging), kvm_vcpu_flush_tlb_guest() calls
kvm_mmu_sync_roots() and eventually mmu_sync_children(). This path can yield
via cond_resched_rwlock_write(). Yielding while preemption is disabled by
the gpc read lock will trigger a BUG.

The old code safely dropped the user access guard before executing the
TLB flush, avoiding this issue.

[ ... ]
> @@ -5260,20 +5227,32 @@ static void kvm_steal_time_set_preempted(struct kvm_vcpu *vcpu)
[ ... ]
> -	st = (struct kvm_steal_time __user *)ghc->hva;
> -	BUILD_BUG_ON(sizeof(st->preempted) != sizeof(preempted));
> -
> -	if (!copy_to_user_nofault(&st->preempted, &preempted, sizeof(preempted)))
> -		vcpu->arch.st.preempted = KVM_VCPU_PREEMPTED;
> +	/*
> +	 * Use a trylock as this is called from the scheduler path (via
> +	 * kvm_sched_out), where rwlock_t is not safe on PREEMPT_RT (it
> +	 * becomes sleepable).  Setting preempted is best-effort anyway;
> +	 * the old HVA-based code used copy_to_user_nofault() which could
> +	 * also silently fail.
> +	 *
> +	 * Since we only trylock and bail on failure, there is no risk of
> +	 * deadlock with an interrupt handler, so no need to disable
> +	 * interrupts.
> +	 */
> +	CLASS(gpc_try_map_local, st_map)(gpc, sizeof(st->preempted));
> +	if (IS_ERR(st_map))
>  		return;
>  
> -	mark_page_dirty_in_slot(vcpu->kvm, ghc->memslot, gpa_to_gfn(ghc->gpa));
> +	st = *st_map;
> +	WRITE_ONCE(st->preempted, KVM_VCPU_PREEMPTED);
> +	vcpu->arch.st.preempted = KVM_VCPU_PREEMPTED;
> +}

[Severity: High]
Can this cause an ABBA deadlock in the scheduler on PREEMPT_RT?

kvm_steal_time_set_preempted() is called during a vCPU context switch
via kvm_sched_out(), which is a preempt notifier hook. This runs with
the local CPUs runqueue lock (rq->lock) held.

While the code correctly uses a trylock to avoid sleeping when acquiring
the lock, the CLASS macro destructor will implicitly call read_unlock()
at the end of the function. On PREEMPT_RT kernels, unlocking an rt_rwlock
can wake up a waiting writer, such as a concurrent kvm_gpc_refresh().

If a writer is woken, rt_mutex_wake_up_q() calls wake_up_process(), which
attempts to acquire the target tasks rq->lock via try_to_wake_up().
Attempting to acquire a runqueue lock while the local CPUs rq->lock is
already held violates scheduler lock ordering and leads to a deadlock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529165114.748639-1-seanjc@google.com?part=20