lkml.org 
[lkml]   [2022]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] KVM: x86/mmu: add lockdep check before lookup_address_in_mm()
On Sun, Mar 27, 2022, Mingwei Zhang wrote:
> Add a lockdep check before invoking lookup_address_in_mm().
> lookup_address_in_mm() walks all levels of host page table without
> accquiring any lock. This is usually unsafe unless we are walking the
> kernel addresses (check other usage cases of lookup_address_in_mm and
> lookup_address_in_pgd).
>
> Walking host page table (especially guest addresses) usually requires
> holding two types of locks: 1) mmu_lock in mm or the lock that protects
> the reverse maps of host memory in range; 2) lock for the leaf paging
> structures.
>
> One exception case is when we take the mmu_lock of the secondary mmu.
> Holding mmu_lock of KVM MMU in either read mode or write mode prevents host
> level entities from modifying the host page table concurrently. This is
> because all of them will have to invoke KVM mmu_notifier first before doing
> the actual work. Since KVM mmu_notifier invalidation operations always take
> the mmu write lock, we are safe if we hold the mmu lock here.
>
> Note: this means that KVM cannot allow concurrent multiple mmu_notifier
> invalidation callbacks by using KVM mmu read lock. Since, otherwise, any
> host level entity can cause race conditions with this one. Walking host
> page table here may get us stale information or may trigger NULL ptr
> dereference that is hard to reproduce.
>
> Having a lockdep check here will prevent or at least warn future
> development that directly walks host page table simply in a KVM ioctl
> function. In addition, it provides a record for any future development on
> KVM mmu_notifier.
>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Ben Gardon <bgardon@google.com>
> Cc: David Matlack <dmatlack@google.com>
>
> Signed-off-by: Mingwei Zhang <mizhang@google.com>
> ---
> arch/x86/kvm/mmu/mmu.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 1361eb4599b4..066bb5435156 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -2820,6 +2820,24 @@ static int host_pfn_mapping_level(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
> */
> hva = __gfn_to_hva_memslot(slot, gfn);
>
> + /*
> + * lookup_address_in_mm() walks all levels of host page table without
> + * accquiring any lock. This is not safe when KVM does not take the
> + * mmu_lock. Holding mmu_lock in either read mode or write mode prevents
> + * host level entities from modifying the host page table. This is
> + * because all of them will have to invoke KVM mmu_notifier first before
> + * doing the actual work. Since KVM mmu_notifier invalidation operations
> + * always take the mmu write lock, we are safe if we hold the mmu lock
> + * here.
> + *
> + * Note: this means that KVM cannot allow concurrent multiple
> + * mmu_notifier invalidation callbacks by using KVM mmu read lock.
> + * Otherwise, any host level entity can cause race conditions with this
> + * one. Walking host page table here may get us stale information or may
> + * trigger NULL ptr dereference that is hard to reproduce.
> + */
> + lockdep_assert_held(&kvm->mmu_lock);

Holding mmu_lock isn't strictly required. It would also be safe to use this helper
if mmu_notifier_retry_hva() were checked after grabbing the mapping level, before
consuming it. E.g. we could theoretically move this to kvm_faultin_pfn().

And simply holding the lock isn't sufficient, i.e. the lockdep gives a false sense
of security. E.g. calling this while holding mmu_lock but without first checking
mmu_notifier_count would let it run concurrently with host PTE modifications.

I'm definitely in favor of adding a comment to document the mmu_notifier
interactions, but I don't like adding a lockdep.

\
 
 \ /
  Last update: 2022-03-28 17:17    [W:0.081 / U:0.376 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site