Re: [PATCH] KVM: arm/arm64: Handle hva aging while destroying the vm

[Andrea Arcangeli]

Hello,

On Mon, Jul 03, 2017 at 10:48:03AM +0200, Alexander Graf wrote:
> On 07/03/2017 10:03 AM, Christoffer Dall wrote:
> > Hi Alex,
> >
> > On Fri, Jun 23, 2017 at 05:21:59PM +0200, Alexander Graf wrote:
> >> If we want to age an HVA while the VM is getting destroyed, we have a
> >> tiny race window during which we may end up dereferencing an invalid
> >> kvm->arch.pgd value.
> >>
> >>     CPU0               CPU1
> >>
> >>     kvm_age_hva()
> >>                        kvm_mmu_notifier_release()
> >>                        kvm_arch_flush_shadow_all()
> >>                        kvm_free_stage2_pgd()
> >>                        <grab mmu_lock>
> >>     stage2_get_pmd()
> >>     <wait for mmu_lock>
> >>                        set kvm->arch.pgd = 0
> >>                        <free mmu_lock>
> >>     <grab mmu_lock>
> >>     stage2_get_pud()
> >>     <access kvm->arch.pgd>
> >>     <use incorrect value>
> > I don't think this sequence, can happen, but I think kvm_age_hva() can
> > be called with the mmu_lock held and kvm->pgd already being NULL.
> >
> > Is that possible for the mmu notifiers to be calling clear(_flush)_young
> > while also calling notifier_release?
> 
> I *think* the aging happens completely orthogonally to release. But 
> let's ask Andrea - I'm sure he knows :).

I think the sequence can happen. All mmu notifier methods are flushed
out of CPUs only through synchronize_srcu() which is called as the
last step in __mmu_notifier_release/unregister. Only after _unregister
returns you're sure kvm_age_hva cannot run anymore, until that point
it can still run. Even during exit_mmap->mmu_notifier_release it can
still run if invoked through rmap walks.

So while the ->release method runs, all other mmu notifier methods
could be still invoked concurrently.

mmu notifier methods are only protected by srcu to prevent the mmu
notifier structure to be freed from under them, but there's no
additional locking to serialize them (except for the synchronize_srcu
that happens as the last step of mmu_notifier_release/unregister, well
after they may have called the ->release method).

There's also a comment about it in __mmu_notifier_release:

 * runs with mm_users == 0. Other tasks may still invoke mmu notifiers
 * in parallel despite there being no task using this mm any more,
 * through the vmas outside of the exit_mmap context, such as with
 * vmtruncate. This serializes against mmu_notifier_unregister with

And in the mmu_notifier_unregister too:

 * calling mmu_notifier_unregister. ->release or any other notifier
 * method may be invoked concurrently with mmu_notifier_unregister,
 * and only after mmu_notifier_unregister returned we're guaranteed
 * that ->release or any other method can't run anymore.

Even ->release could in theory run concurrently against itself if
mmu_notifier_unregister runs concurrently with mmu_notifier_release
but that's purely theoretical possibility.

Thanks,
Andrea