+David On Mon, Mar 13, 2023, Marc Zyngier wrote: > We walk the userspace PTs to discover what mapping size was > used there. However, this can race against the userspace tables > being freed, and we end-up in the weeds. > > Thankfully, the mm code is being generous and will IPI us when > doing so. So let's implement our part of the bargain and disable > interrupts around the walk. This ensures that nothing terrible > happens during that time. > > We still need to handle the removal of the page tables before > the walk. For that, allow get_user_mapping_size() to return an > error, and make sure this error can be propagated all the way > to the the exit handler. > > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx> > Cc: stable@xxxxxxxxxxxxxxx > --- > arch/arm64/kvm/mmu.c | 35 ++++++++++++++++++++++++++++------- > 1 file changed, 28 insertions(+), 7 deletions(-) > > 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, which is safe since there _was_ a valid translation when mmu_lock was acquired and mmu_invalidate_retry() was checked. It's the primary MMU's responsibility to ensure all secondary MMUs are purged before freeing memory, i.e. worst case should be that KVMs stage-2 translation will be immediately zapped via mmu_notifier. KVM ARM also has a bug that might be related: the mmu_seq snapshot needs to be taken _before_ mmap_read_unlock(), otherwise vma_shift may be stale by the time it's consumed. I believe David is going to submit a patch (I found and "reported" the bug when doing an internal review of "common MMU" stuff).
