Re: [PATCH] KVM: x86/mmu: add lockdep check before lookup_address_in_mm()

[Sean Christopherson <seanjc@google.com>]

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.