On Wed, Oct 09, 2024 at 12:23:44PM -0700, Sean Christopherson wrote:
> Add a lockdep assertion in kvm_unmap_gfn_range() to ensure that either
> mmu_invalidate_in_progress is elevated, or that the range is being zapped
> due to memslot removal (loosely detected by slots_lock being held).
> Zapping SPTEs without mmu_invalidate_{in_progress,seq} protection is unsafe
> as KVM's page fault path snapshots state before acquiring mmu_lock, and
> thus can create SPTEs with stale information if vCPUs aren't forced to
> retry faults (due to seeing an in-progress or past MMU invalidation).
>
> Memslot removal is a special case, as the memslot is retrieved outside of
> mmu_invalidate_seq, i.e. doesn't use the "standard" protections, and
> instead relies on SRCU synchronization to ensure any in-flight page faults
> are fully resolved before zapping SPTEs.
>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
> arch/x86/kvm/mmu/mmu.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 09494d01c38e..c6716fd3666f 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -1556,6 +1556,16 @@ bool kvm_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range *range)
> {
> bool flush = false;
>
> + /*
> + * To prevent races with vCPUs faulting in a gfn using stale data,
> + * zapping a gfn range must be protected by mmu_invalidate_in_progress
> + * (and mmu_invalidate_seq). The only exception is memslot deletion,
> + * in which case SRCU synchronization ensures SPTEs a zapped after all
> + * vCPUs have unlocked SRCU and are guaranteed to see the invalid slot.
> + */
> + lockdep_assert_once(kvm->mmu_invalidate_in_progress ||
> + lockdep_is_held(&kvm->slots_lock));
> +
Is the detection of slots_lock too loose?
If a caller just holds slots_lock without calling
"synchronize_srcu_expedited(&kvm->srcu)" as that in kvm_swap_active_memslots()
to ensure the old slot is retired, stale data may still be encountered.
> if (kvm_memslots_have_rmaps(kvm))
> flush = __kvm_rmap_zap_gfn_range(kvm, range->slot,
> range->start, range->end,
> --
> 2.47.0.rc1.288.g06298d1525-goog
>
