[PATCH] arm64: KVM: fix 2-level page tables unmapping

[Christoffer Dall]

On Wed, Aug 07, 2013 at 11:34:04AM +0100, Marc Zyngier wrote:
> On 06/08/13 21:49, Christoffer Dall wrote:
> > On Tue, Aug 06, 2013 at 01:05:48PM +0100, Marc Zyngier wrote:
> >> When using 64kB pages, we only have two levels of page tables,
> >> meaning that PGD, PUD and PMD are fused. In this case, trying
> >> to refcount PUDs and PMDs independantly is a a complete disaster,
> > 
> > independently
> > 
> >> as they are the same.
> >>
> >> We manage to get it right for the allocation (stage2_set_pte uses
> >> {pmd,pud}_none), but the unmapping path clears both pud and pmd
> >> refcounts, which fails spectacularly with 2-level page tables.
> >>
> >> The fix is to avoid calling clear_pud_entry when both the pmd and
> >> pud pages are empty. For this, and instead of introducing another
> >> pud_empty function, consolidate both pte_empty and pmd_empty into
> >> page_empty (the code is actually identical) and use that to also
> >> test the validity of the pud.
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>
> >> ---
> >>  arch/arm/kvm/mmu.c | 22 ++++++++--------------
> >>  1 file changed, 8 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> >> index ca6bea4..7e1d899 100644
> >> --- a/arch/arm/kvm/mmu.c
> >> +++ b/arch/arm/kvm/mmu.c
> >> @@ -85,6 +85,12 @@ static void *mmu_memory_cache_alloc(struct kvm_mmu_memory_cache *mc)
> >>  	return p;
> >>  }
> >>  
> >> +static bool page_empty(void *ptr)
> >> +{
> >> +	struct page *ptr_page = virt_to_page(ptr);
> >> +	return page_count(ptr_page) == 1;
> >> +}
> >> +
> >>  static void clear_pud_entry(struct kvm *kvm, pud_t *pud, phys_addr_t addr)
> >>  {
> >>  	pmd_t *pmd_table = pmd_offset(pud, 0);
> >> @@ -103,12 +109,6 @@ static void clear_pmd_entry(struct kvm *kvm, pmd_t *pmd, phys_addr_t addr)
> >>  	put_page(virt_to_page(pmd));
> >>  }
> >>  
> >> -static bool pmd_empty(pmd_t *pmd)
> >> -{
> >> -	struct page *pmd_page = virt_to_page(pmd);
> >> -	return page_count(pmd_page) == 1;
> >> -}
> >> -
> >>  static void clear_pte_entry(struct kvm *kvm, pte_t *pte, phys_addr_t addr)
> >>  {
> >>  	if (pte_present(*pte)) {
> >> @@ -118,12 +118,6 @@ static void clear_pte_entry(struct kvm *kvm, pte_t *pte, phys_addr_t addr)
> >>  	}
> >>  }
> >>  
> >> -static bool pte_empty(pte_t *pte)
> >> -{
> >> -	struct page *pte_page = virt_to_page(pte);
> >> -	return page_count(pte_page) == 1;
> >> -}
> >> -
> >>  static void unmap_range(struct kvm *kvm, pgd_t *pgdp,
> >>  			unsigned long long start, u64 size)
> >>  {
> >> @@ -153,10 +147,10 @@ static void unmap_range(struct kvm *kvm, pgd_t *pgdp,
> >>  		range = PAGE_SIZE;
> >>  
> >>  		/* If we emptied the pte, walk back up the ladder */
> >> -		if (pte_empty(pte)) {
> >> +		if (page_empty(pte)) {
> >>  			clear_pmd_entry(kvm, pmd, addr);
> >>  			range = PMD_SIZE;
> >> -			if (pmd_empty(pmd)) {
> >> +			if (page_empty(pmd) && !page_empty(pud)) {
> >>  				clear_pud_entry(kvm, pud, addr);
> >>  				range = PUD_SIZE;
> >>  			}
> > 
> > looks right, an alternative would be to check in clear_pud_entry if the
> > entry actually had a value, but I don't think it's really clearer.
> > 
> > However, this got me thinking a bit.  What happens if we pass a non-pmd
> > aligned address to unmap_range, and let's assume the size of the range
> > is more than 2MB, won't we be leaking memory by incrementing with
> > PMD_SIZE?  (same argument goes for PUD_SIZE).  See the patch below:
> > 
> > 
> > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> > index ca6bea4..80a83ec 100644
> > --- a/arch/arm/kvm/mmu.c
> > +++ b/arch/arm/kvm/mmu.c
> > @@ -132,37 +132,37 @@ static void unmap_range(struct kvm *kvm, pgd_t *pgdp,
> >  	pmd_t *pmd;
> >  	pte_t *pte;
> >  	unsigned long long addr = start, end = start + size;
> > -	u64 range;
> > +	u64 next;
> >  
> >  	while (addr < end) {
> >  		pgd = pgdp + pgd_index(addr);
> >  		pud = pud_offset(pgd, addr);
> >  		if (pud_none(*pud)) {
> > -			addr += PUD_SIZE;
> > +			addr = pud_addr_end(addr, end);
> >  			continue;
> >  		}
> >  
> >  		pmd = pmd_offset(pud, addr);
> >  		if (pmd_none(*pmd)) {
> > -			addr += PMD_SIZE;
> > +			addr = pmd_addr_end(addr, end);
> >  			continue;
> >  		}
> >  
> >  		pte = pte_offset_kernel(pmd, addr);
> >  		clear_pte_entry(kvm, pte, addr);
> > -		range = PAGE_SIZE;
> > +		next = addr + PAGE_SIZE;
> >  
> >  		/* If we emptied the pte, walk back up the ladder */
> >  		if (pte_empty(pte)) {
> >  			clear_pmd_entry(kvm, pmd, addr);
> > -			range = PMD_SIZE;
> > +			next = pmd_addr_end(addr, end);
> >  			if (pmd_empty(pmd)) {
> >  				clear_pud_entry(kvm, pud, addr);
> > -				range = PUD_SIZE;
> > +				next = pud_addr_end(addr, end);
> >  			}
> >  		}
> >  
> > -		addr += range;
> > +		addr = next;
> >  	}
> >  }
> 
> That looks sensible. Would you prepare a patch on which I could rebase
> the above?
> 
I can just apply both patches as they are will the spell fix to
kvm-arm-fixes.

-Christoffer