Hi Sean,
On Mon, Mar 28, 2022 at 8:16 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
> > Cc: Ben Gardon <bgardon@xxxxxxxxxx>
> > Cc: David Matlack <dmatlack@xxxxxxxxxx>
> >
> > Signed-off-by: Mingwei Zhang <mizhang@xxxxxxxxxx>
> > ---
> > 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.
Right, even holding the kvm->mmu_lock is not safe, since we may have
several concurrent invalidations ongoing and they are done zapping
entries in EPT (so that they could just release the kvm->mmu_lock) and
start working on the host page table. If we want to make it safe, we
also have to check mmu_notifier_count (and potentially mmu_seq as
well).
With that, I start to feel this is a bug. The issue is just so rare
that it has never triggered a problem.
lookup_address_in_mm() walks the host page table as if it is a
sequence of _static_ memory chunks. This is clearly dangerous. If we
look at hva_to_pfn(), which is the right way to walk host page table:
hva_to_pfn() =>
hva_to_pfn_fast() =>
get_user_page_fast_only() =>
internal_get_user_pages_fast() =>
lockless_pages_from_mm() =>
local_irq_save(flags); /* Disable interrupts here. */
gup_pgd_range(start, end, gup_flags, pages, &nr_pinned);
... ...
hva_to_pfn_slow() =>
get_user_pages_unlocked() =>
mmap_read_lock(mm); /* taking the mm lock here. */
The above code has two branches to walk the host page table: 1) the
fast one and 2) slow one; The slower one takes the mm lock, while the
faster one simply disables the interrupts.
I think we might have to mimic the same thing in
lockless_pages_from_mm(), i.e. wrapping
local_irq_{save,restore}(flags) around the lookup_address_in_mm().
Alternatively, we have to specify that the function
lookup_address_in_mm() as well as its callers:
host_pfn_mapping_level() and kvm_mmu_max_mapping_level() CANNOT be
called in generic places in KVM, but only in the fault path and AFTER
the check of "is_page_fault_stale()".
But right now, kvm_mmu_max_mapping_level() are used in other places
as well: kvm_mmu_zap_collapsible_spte(), which does not satisfy the
strict requirement of walking the host page table.
>
> I'm definitely in favor of adding a comment to document the mmu_notifier
> interactions, but I don't like adding a lockdep.
