From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@redhat.com>
Subject: Re: KVM: MMU: update sp->gfns on pte update path
Date: Tue, 25 Jan 2011 16:32:29 +0200
Message-ID: <4D3EDEFD.9060706@redhat.com>
References: <20110125130733.GA5645@amt.cnet>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: kvm <kvm@vger.kernel.org>,
	Nicolas Prochazka <prochazka.nicolas@gmail.com>
To: Marcelo Tosatti <mtosatti@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:10507 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753264Ab1AYOci (ORCPT <rfc822;kvm@vger.kernel.org>);
	Tue, 25 Jan 2011 09:32:38 -0500
In-Reply-To: <20110125130733.GA5645@amt.cnet>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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?

-- 
error compiling committee.c: too many arguments to function