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@xxxxxxxxx> > >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? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
