Re: [PATCH] arm64/kvm: Fix zapping stage2 page table wrongly

[Marc Zyngier <maz@kernel.org>]

Hi Gavin,

Adding the usual suspects to the Cc list.

On Sat, 22 Aug 2020 03:44:44 +0100,
Gavin Shan <gshan@redhat.com> wrote:
> 
> Depending on the kernel configuration, PUD_SIZE could be equal to
> PMD_SIZE. For example, both of them are 512MB with the following
> kernel configuration. In this case, both PUD and PMD are folded
> to PGD.
> 
>    CONFIG_ARM64_64K_PAGES   y
>    CONFIG_ARM64_VA_BITS     42
>    CONFIG_PGTABLE_LEVELS    2
> 
> With the above configuration, the stage2 PUD is used to backup the
> 512MB huge page when the stage2 mapping is built. During the mapping,
> the PUD and its subordinate levels of page table entries are unmapped
> if the PUD is present and not huge page sensitive in stage2_set_pud_huge().
> Unfornately, the @addr isn't aligned to S2_PUD_SIZE and wrong page table
> entries are zapped. It eventually leads to PUD's present bit can't be
> cleared successfully and infinite loop in stage2_set_pud_huge().
> 
> This fixes the issue by checking with S2_{PUD, PMD}_SIZE instead of
> {PUD, PMD}_SIZE to determine if stage2 PUD or PMD is used to back the
> huge page. For this particular case, the stage2 PMD entry should be
> used to backup the 512MB huge page with stage2_set_pmd_huge().

It isn't obvious to me how S2_PMD_SIZE can be different from PMD_SIZE,
and the current code certainly expects both to be equal (just look at
how often S2_*_SIZE is used in the current code to convince yourself).

My guess is that some lesser tested configurations (such as 64k pages)
break that assumption, and result in the wrong thing happening. Could
you please shed some light on it?

> Fixes: 3c3736cd32bf ("KVM: arm/arm64: Fix handling of stage2 huge mappings")

This commit doesn't seem to match the code your changing (it doesn't
even come near user_mem_abort()). Are there any intermediate commits
that would better explain the problem?

> Cc: stable@vger.kernel.org # v5.1+
> Reported-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> Tested-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> ---
>  arch/arm64/kvm/mmu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 0121ef2c7c8d..deb1819ba9e2 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1964,7 +1964,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  		(fault_status == FSC_PERM &&
>  		 stage2_is_exec(mmu, fault_ipa, vma_pagesize));
>  
> -	if (vma_pagesize == PUD_SIZE) {
> +	if (vma_pagesize == S2_PUD_SIZE) {
>  		pud_t new_pud = kvm_pfn_pud(pfn, mem_type);
>  
>  		new_pud = kvm_pud_mkhuge(new_pud);
> @@ -1975,7 +1975,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  			new_pud = kvm_s2pud_mkexec(new_pud);
>  
>  		ret = stage2_set_pud_huge(mmu, memcache, fault_ipa, &new_pud);
> -	} else if (vma_pagesize == PMD_SIZE) {
> +	} else if (vma_pagesize == S2_PMD_SIZE) {
>  		pmd_t new_pmd = kvm_pfn_pmd(pfn, mem_type);
>  
>  		new_pmd = kvm_pmd_mkhuge(new_pmd);
> -- 
> 2.23.0
> 
> 

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm