Re: [PATCH v3 08/21] KVM: arm64: Convert kvm_set_spte_hva() to generic page-table API

From: Will Deacon <will@kernel.org>
To: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: Marc Zyngier <maz@kernel.org>,
	kernel-team@android.com, kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org,
	Catalin Marinas <catalin.marinas@arm.com>
Subject: Re: [PATCH v3 08/21] KVM: arm64: Convert kvm_set_spte_hva() to generic page-table API
Date: Thu, 3 Sep 2020 17:37:11 +0100	[thread overview]
Message-ID: <20200903163711.GA7791@willie-the-truck> (raw)
In-Reply-To: <86bb295c-64d8-51a1-25f4-32a5a1bb2172@arm.com>

On Wed, Sep 02, 2020 at 04:37:18PM +0100, Alexandru Elisei wrote:
> On 8/25/20 10:39 AM, Will Deacon wrote:
> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > index 33146d3dc93a..704b471a48ce 100644
> > --- a/arch/arm64/kvm/mmu.c
> > +++ b/arch/arm64/kvm/mmu.c
> > @@ -1911,28 +1911,27 @@ int kvm_unmap_hva_range(struct kvm *kvm,
> >  
> >  static int kvm_set_spte_handler(struct kvm *kvm, gpa_t gpa, u64 size, void *data)
> >  {
> > -	pte_t *pte = (pte_t *)data;
> > +	kvm_pfn_t *pfn = (kvm_pfn_t *)data;
> >  
> >  	WARN_ON(size != PAGE_SIZE);
> > +
> >  	/*
> > -	 * We can always call stage2_set_pte with KVM_S2PTE_FLAG_LOGGING_ACTIVE
> > -	 * flag clear because MMU notifiers will have unmapped a huge PMD before
> > -	 * calling ->change_pte() (which in turn calls kvm_set_spte_hva()) and
> > -	 * therefore stage2_set_pte() never needs to clear out a huge PMD
> > -	 * through this calling path.
> > +	 * The MMU notifiers will have unmapped a huge PMD before calling
> > +	 * ->change_pte() (which in turn calls kvm_set_spte_hva()) and
> > +	 * therefore we never need to clear out a huge PMD through this
> > +	 * calling path and a memcache is not required.
> >  	 */
> > -	stage2_set_pte(&kvm->arch.mmu, NULL, gpa, pte, 0);
> > +	kvm_pgtable_stage2_map(kvm->arch.mmu.pgt, gpa, PAGE_SIZE,
> > +			       __pfn_to_phys(*pfn), KVM_PGTABLE_PROT_R, NULL);
> 
> I have to admit that I managed to confuse myself.
> 
> According to the comment, this is called after unmapping a huge PMD.
> __unmap_stage2_range() -> .. -> unmap_stage2_pmd() calls pmd_clear(), which means
> the PMD entry is now 0.
> 
> In __kvm_pgtable_visit(), kvm_pte_table() returns false, because the entry is
> invalid, and so we call stage2_map_walk_leaf(). Here, stage2_map_walker_try_leaf()
> will return false, because kvm_block_mapping_supported() returns false (PMD
> granule is larger than PAGE_SIZE), and then we end up allocating a table from the
> memcache. memcache which will NULL, and kvm_mmu_memory_cache_alloc() will
> dereference the NULL pointer.
> 
> I'm pretty sure there's something that I'm missing here, I would really appreciate
> someone pointing out where I'm making a mistake.

You're not missing anything, and this is actually a bug introduced by moving
to the generic mmu cache code. My old implementation (which you can still
see in the earlier patch) returns NULL if the cache is NULL, so I'll need to
reintroduce that check here. This then mimics the current behaviour of
ignoring map requests from the MMU if we need to allocate, and instead
handling them lazily when we take the fault.

Well spotted!

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel