Re: [PATCH 1/2] KVM: arm64: Disable interrupts while walking userspace PTs

From: Sean Christopherson <seanjc@google.com>
To: David Matlack <dmatlack@google.com>
Cc: Marc Zyngier <maz@kernel.org>,
	kvmarm@lists.linux.dev,  linux-arm-kernel@lists.infradead.org,
	kvm@vger.kernel.org,  James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	 Oliver Upton <oliver.upton@linux.dev>,
	Zenghui Yu <yuzenghui@huawei.com>,
	 Ard Biesheuvel <ardb@kernel.org>, Will Deacon <will@kernel.org>,
	Quentin Perret <qperret@google.com>,
	 stable@vger.kernel.org
Subject: Re: [PATCH 1/2] KVM: arm64: Disable interrupts while walking userspace PTs
Date: Mon, 13 Mar 2023 10:21:09 -0700	[thread overview]
Message-ID: <ZA9bhVQBtgES7Qqi@google.com> (raw)
In-Reply-To: <CALzav=c3aKNT-7CzkrFC8YCCwj-E16JYWwsmOV-HaLbODG67hQ@mail.gmail.com>

On Mon, Mar 13, 2023, David Matlack wrote:
> On Mon, Mar 13, 2023 at 8:53 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > +David
> >
> > On Mon, Mar 13, 2023, Marc Zyngier wrote:
> > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > > index 7113587222ff..d7b8b25942df 100644
> > > --- a/arch/arm64/kvm/mmu.c
> > > +++ b/arch/arm64/kvm/mmu.c
> > > @@ -666,14 +666,23 @@ static int get_user_mapping_size(struct kvm *kvm, u64 addr)
> > >                                  CONFIG_PGTABLE_LEVELS),
> > >               .mm_ops         = &kvm_user_mm_ops,
> > >       };
> > > +     unsigned long flags;
> > >       kvm_pte_t pte = 0;      /* Keep GCC quiet... */
> > >       u32 level = ~0;
> > >       int ret;
> > >
> > > +     /*
> > > +      * Disable IRQs so that we hazard against a concurrent
> > > +      * teardown of the userspace page tables (which relies on
> > > +      * IPI-ing threads).
> > > +      */
> > > +     local_irq_save(flags);
> > >       ret = kvm_pgtable_get_leaf(&pgt, addr, &pte, &level);
> > > -     VM_BUG_ON(ret);
> > > -     VM_BUG_ON(level >= KVM_PGTABLE_MAX_LEVELS);
> > > -     VM_BUG_ON(!(pte & PTE_VALID));
> > > +     local_irq_restore(flags);
> > > +
> > > +     /* Oops, the userspace PTs are gone... */
> > > +     if (ret || level >= KVM_PGTABLE_MAX_LEVELS || !(pte & PTE_VALID))
> > > +             return -EFAULT;
> >
> > I don't think this should return -EFAULT all the way out to userspace.  Unless
> > arm64 differs from x86 in terms of how the userspace page tables are managed, not
> > having a valid translation _right now_ doesn't mean that one can't be created in
> > the future, e.g. by way of a subsequent hva_to_pfn().
> >
> > FWIW, the approach x86 takes is to install a 4KiB (smallest granuale) translation,
> 
> If I'm reading the ARM code correctly, returning -EFAULT here will
> have that effect. get_user_mapping_size() is only called by
> transparent_hugepage_adjust() which returns PAGE_SIZE if
> get_user_mapping_size() returns anything less than PMD_SIZE.

No, this patch adds

+               int sz = get_user_mapping_size(kvm, hva);
+
+               if (sz < 0)
+                       return sz;
+
+               if (sz < PMD_SIZE)
+                       return PAGE_SIZE;
+

and 

                        vma_pagesize = transparent_hugepage_adjust(kvm, memslot,
                                                                   hva, &pfn,
                                                                   &fault_ipa);
+
+               if (vma_pagesize < 0) {
+                       ret = vma_pagesize;
+                       goto out_unlock;
+               }