Re: [PATCH v3] kvm: arm/arm64: Fix locking for kvm_free_stage2_pgd

[Christoffer Dall]

On Mon, Apr 03, 2017 at 03:22:11PM +0100, Mark Rutland wrote:
> Hi,
> 
> On Mon, Apr 03, 2017 at 03:12:43PM +0100, Suzuki K Poulose wrote:
> > In kvm_free_stage2_pgd() we don't hold the kvm->mmu_lock while calling
> > unmap_stage2_range() on the entire memory range for the guest. This could
> > cause problems with other callers (e.g, munmap on a memslot) trying to
> > unmap a range. And since we have to unmap the entire Guest memory range
> > holding a spinlock, make sure we yield the lock if necessary, after we
> > unmap each PUD range.
> > 
> > Fixes: commit d5d8184d35c9 ("KVM: ARM: Memory virtualization setup")
> > Cc: stable@xxxxxxxxxxxxxxx # v3.10+
> > Cc: Paolo Bonzini <pbonzin@xxxxxxxxxx>
> > Cc: Marc Zyngier <marc.zyngier@xxxxxxx>
> > Cc: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> > Cc: Mark Rutland <mark.rutland@xxxxxxx>
> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > [ Avoid vCPU starvation and lockup detector warnings ]
> > Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > 
> > ---
> > Changes since V2:
> >  - Restrict kvm->mmu_lock relaxation to bigger ranges in unmap_stage2_range(),
> >    to avoid possible issues like [0]
> > 
> >  [0] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-March/498210.html
> 
> Sorry if I'm being thick, but how does restricting this to a larger
> range help with the "sleeping function called from invalid context"
> issue?
> 
> Surely that just makes it rarer?

As far as I can tell, the unmap_stage2_range() function is only called
in the problematic path which has the extra lock taken rom
try_to_unmap_one() via the kvm_unmap_hva() function, which always
passes PAGE_SIZE as the argument, which is always smaller than
S2_PUD_SIZE.

Did I miss something?

Thanks,
-Christoffer
> 
> > 
> > Changes since V1:
> >  - Yield the kvm->mmu_lock if necessary in unmap_stage2_range to prevent
> >    vCPU starvation and lockup detector warnings.
> > ---
> >  arch/arm/kvm/mmu.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> > index 13b9c1f..db94f3a 100644
> > --- a/arch/arm/kvm/mmu.c
> > +++ b/arch/arm/kvm/mmu.c
> > @@ -292,8 +292,15 @@ static void unmap_stage2_range(struct kvm *kvm, phys_addr_t start, u64 size)
> >  	phys_addr_t addr = start, end = start + size;
> >  	phys_addr_t next;
> >  
> > +	assert_spin_locked(&kvm->mmu_lock);
> >  	pgd = kvm->arch.pgd + stage2_pgd_index(addr);
> >  	do {
> > +		/*
> > +		 * If the range is too large, release the kvm->mmu_lock
> > +		 * to prevent starvation and lockup detector warnings.
> > +		 */
> > +		if (size > S2_PUD_SIZE)
> > +			cond_resched_lock(&kvm->mmu_lock);
> >  		next = stage2_pgd_addr_end(addr, end);
> >  		if (!stage2_pgd_none(*pgd))
> >  			unmap_stage2_puds(kvm, pgd, addr, next);
> > @@ -831,7 +838,10 @@ void kvm_free_stage2_pgd(struct kvm *kvm)
> >  	if (kvm->arch.pgd == NULL)
> >  		return;
> >  
> > +	spin_lock(&kvm->mmu_lock);
> >  	unmap_stage2_range(kvm, 0, KVM_PHYS_SIZE);
> > +	spin_unlock(&kvm->mmu_lock);
> > +
> >  	/* Free the HW pgd, one page at a time */
> >  	free_pages_exact(kvm->arch.pgd, S2_PGD_SIZE);
> >  	kvm->arch.pgd = NULL;
> > -- 
> > 2.7.4
> >