Re: KVM: MMU: update sp->gfns on pte update path

[Marcelo Tosatti <mtosatti@redhat.com>]

On Tue, Jan 25, 2011 at 04:32:29PM +0200, Avi Kivity wrote:
> On 01/25/2011 03:07 PM, Marcelo Tosatti wrote:
> >If an emulated pte write modifies the gpa of a present spte, sp->gfns is
> >not updated, retaining a stale value which later leads to:
> >
> >rmap_remove: ffff8807d245fff8 0->BUG
> >------------[ cut here ]------------
> >kernel BUG at arch/x86/kvm/mmu.c:695!
> >
> >Fix by updating sp->gfns even if spte was present.
> >
> >Resolves: https://bugzilla.kernel.org/show_bug.cgi?id=27052
> >Reported-and-tested-by: Nicolas Prochazka<prochazka.nicolas@gmail.com>
> >KVM-Stable-Tag.
> >
> >diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> >index cc1bada..37d0886 100644
> >--- a/arch/x86/kvm/mmu.c
> >+++ b/arch/x86/kvm/mmu.c
> >@@ -2054,6 +2054,12 @@ static void mmu_set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
> >  		rmap_count = rmap_add(vcpu, sptep, gfn);
> >  		if (rmap_count>  RMAP_RECYCLE_THRESHOLD)
> >  			rmap_recycle(vcpu, sptep, gfn);
> >+	} else {
> >+		struct kvm_mmu_page *sp = page_header(__pa(sptep));
> >+		int index = sptep - sp->spt;
> >+
> >+		if (!sp->role.direct&&  sp->gfns[index] != gfn)
> >+			sp->gfns[index] = gfn;
> >  	}
> >  	kvm_release_pfn_clean(pfn);
> >  	if (speculative) {
> >
> 
> Should be done by a call to kvm_mmu_page_set_gfn().  But I don't
> understand how it could become inconsistent in the first place.
> 
>     if (is_rmap_spte(*sptep)) {
>         /*
>          * If we overwrite a PTE page pointer with a 2MB PMD, unlink
>          * the parent of the now unreachable PTE.
>          */
>         if (level > PT_PAGE_TABLE_LEVEL &&
>             !is_large_pte(*sptep)) {
>             struct kvm_mmu_page *child;
>             u64 pte = *sptep;
> 
>             child = page_header(pte & PT64_BASE_ADDR_MASK);
>             mmu_page_remove_parent_pte(child, sptep);
>             __set_spte(sptep, shadow_trap_nonpresent_pte);
>             kvm_flush_remote_tlbs(vcpu->kvm);
>         } else if (pfn != spte_to_pfn(*sptep)) {
>             pgprintk("hfn old %llx new %llx\n",
>                  spte_to_pfn(*sptep), pfn);
>             drop_spte(vcpu->kvm, sptep, shadow_trap_nonpresent_pte);
>             kvm_flush_remote_tlbs(vcpu->kvm);
>         } else
>             was_rmapped = 1;
>     }
> 
> If we set was_rmapped, that means rmap_add() was previously called
> for this spte/gfn/pfn pair, and all that changes is permissions, no?

What if pfn is the same but gfn differs?