KVM: MMU: avoid creation of unreachable pages in the shadow

KVM: MMU: avoid creation of unreachable pages in the shadow

[Marcelo Tosatti]

It is possible for a shadow page to have a parent link
pointing to a freed page. When zapping a high level table,
kvm_mmu_page_unlink_children fails to remove the parent_pte link.
For that to happen, the child must be unreachable via the shadow 
tree, which can happen in shadow_walk_entry if the guest pte was
modified in between walk() and fetch(). Remove the parent pte 
reference in such case.

Possible cause for oops in bug #2217430.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>


diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 613ec9a..84eee43 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -331,6 +331,7 @@ static int FNAME(shadow_walk_entry)(struct kvm_shadow_walk *_sw,
 		r = kvm_read_guest_atomic(vcpu->kvm, gw->pte_gpa[level - 2],
 					  &curr_pte, sizeof(curr_pte));
 		if (r || curr_pte != gw->ptes[level - 2]) {
+			kvm_mmu_put_page(shadow_page, sptep);
 			kvm_release_pfn_clean(sw->pfn);
 			sw->sptep = NULL;
 			return 1;

Re: KVM: MMU: avoid creation of unreachable pages in the shadow

[John Rousseau]

Marcelo Tosatti wrote:
> It is possible for a shadow page to have a parent link
> pointing to a freed page. When zapping a high level table,
> kvm_mmu_page_unlink_children fails to remove the parent_pte link.
> For that to happen, the child must be unreachable via the shadow 
> tree, which can happen in shadow_walk_entry if the guest pte was
> modified in between walk() and fetch(). Remove the parent pte 
> reference in such case.
> 
> Possible cause for oops in bug #2217430.

I'll apply this to the code that I'm testing, but with my change to 
2.6.27, kvm-79 and Avi's patch from bug #2217430, I haven't seen the 
problem again. I still have been testing with oos_shadow=0, which I'll 
get rid of now.

Thanks
-John

Re: KVM: MMU: avoid creation of unreachable pages in the shadow

[Marcelo Tosatti]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

On Tue, Nov 25, 2008 at 01:27:27PM -0500, John Rousseau wrote:
> Marcelo Tosatti wrote:
>> It is possible for a shadow page to have a parent link
>> pointing to a freed page. When zapping a high level table,
>> kvm_mmu_page_unlink_children fails to remove the parent_pte link.
>> For that to happen, the child must be unreachable via the shadow tree, 
>> which can happen in shadow_walk_entry if the guest pte was
>> modified in between walk() and fetch(). Remove the parent pte  
>> reference in such case.
>>
>> Possible cause for oops in bug #2217430.
>
> I'll apply this to the code that I'm testing, but with my change to  
> 2.6.27, kvm-79 and Avi's patch from bug #2217430, I haven't seen the  
> problem again. I still have been testing with oos_shadow=0, which I'll  
> get rid of now.

John, 

Please use the attached patch in addition (and drop Avi's). What
is the application set you use to reproduce these issues (that you
mentioned in the bugtrack) ?

Thanks



[-- Attachment #2: kvm-mmu-self-ref-pte.patch --]
[-- Type: text/plain, Size: 503 bytes --]

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 0813a08..8904e8a 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1046,13 +1046,13 @@ static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
 	}
 
 	rmap_write_protect(vcpu->kvm, sp->gfn);
+	kvm_unlink_unsync_page(vcpu->kvm, sp);
 	if (vcpu->arch.mmu.sync_page(vcpu, sp)) {
 		kvm_mmu_zap_page(vcpu->kvm, sp);
 		return 1;
 	}
 
 	kvm_mmu_flush_tlb(vcpu);
-	kvm_unlink_unsync_page(vcpu->kvm, sp);
 	return 0;
 }
 

Re: KVM: MMU: avoid creation of unreachable pages in the shadow

[John Rousseau]

Marcelo Tosatti wrote:
> On Tue, Nov 25, 2008 at 01:27:27PM -0500, John Rousseau wrote:
>> Marcelo Tosatti wrote:
>>> It is possible for a shadow page to have a parent link
>>> pointing to a freed page. When zapping a high level table,
>>> kvm_mmu_page_unlink_children fails to remove the parent_pte link.
>>> For that to happen, the child must be unreachable via the shadow tree, 
>>> which can happen in shadow_walk_entry if the guest pte was
>>> modified in between walk() and fetch(). Remove the parent pte  
>>> reference in such case.
>>>
>>> Possible cause for oops in bug #2217430.
>> I'll apply this to the code that I'm testing, but with my change to  
>> 2.6.27, kvm-79 and Avi's patch from bug #2217430, I haven't seen the  
>> problem again. I still have been testing with oos_shadow=0, which I'll  
>> get rid of now.
> 
> John, 
> 
> Please use the attached patch in addition (and drop Avi's). What
> is the application set you use to reproduce these issues (that you
> mentioned in the bugtrack) ?

Just Vista desktop apps (Firefox, word, SMB via Windows explorer).

Patching now.

Thanks
-John

Re: KVM: MMU: avoid creation of unreachable pages in the shadow

[John Rousseau]

Marcelo Tosatti wrote:
> On Tue, Nov 25, 2008 at 01:27:27PM -0500, John Rousseau wrote:
>> Marcelo Tosatti wrote:
>>> It is possible for a shadow page to have a parent link
>>> pointing to a freed page. When zapping a high level table,
>>> kvm_mmu_page_unlink_children fails to remove the parent_pte link.
>>> For that to happen, the child must be unreachable via the shadow tree, 
>>> which can happen in shadow_walk_entry if the guest pte was
>>> modified in between walk() and fetch(). Remove the parent pte  
>>> reference in such case.
>>>
>>> Possible cause for oops in bug #2217430.
>> I'll apply this to the code that I'm testing, but with my change to  
>> 2.6.27, kvm-79 and Avi's patch from bug #2217430, I haven't seen the  
>> problem again. I still have been testing with oos_shadow=0, which I'll  
>> get rid of now.
> 
> John, 
> 
> Please use the attached patch in addition (and drop Avi's). What
> is the application set you use to reproduce these issues (that you
> mentioned in the bugtrack) ?

I've been running the vista guest continually since Wednesday with 
variable load (mostly idle) and it's been completely stable. I'm not 
sure if it's the move to 2.6.27, kvm-79 or your patches, but your 
patches didn't seem to hurt. :-)

-John