Message-ID: <CAL715WL7ejOBjzXy9vbS_M2LmvXcC-CxmNr+oQtCZW0kciozHA@mail.gmail.com>
Date:   Mon, 28 Mar 2022 10:41:23 -0700
From:   Mingwei Zhang <mizhang@...gle.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm <kvm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Ben Gardon <bgardon@...gle.com>,
        David Matlack <dmatlack@...gle.com>
Subject: Re: [PATCH] KVM: x86/mmu: add lockdep check before lookup_address_in_mm()

Hi Sean,


On Mon, Mar 28, 2022 at 8:16 AM Sean Christopherson <seanjc@...gle.com> 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@...gle.com>
> > Cc: Ben Gardon <bgardon@...gle.com>
> > Cc: David Matlack <dmatlack@...gle.com>
> >
> > Signed-off-by: Mingwei Zhang <mizhang@...gle.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.

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.

Powered by blists - more mailing lists