[patch] kvm: make cr3 loading more robust

[patch] kvm: make cr3 loading more robust

[Ingo Molnar]

From: Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org>
Subject: [patch] kvm: make cr3 loading more robust

rmap_write_protect() has a BUG_ON() if a physical address is not found 
the the memslot. But this is a possible scenario if a buggy guest OS 
loads an invalid or corrupted cr3 value. So exit more gracefully.

[ Avi: this patch needs kvm_mmu_get_page() NULL return to not be ignored 
  by mmu_alloc_roots()/nonpaging_init_context()/paging_new_cr3()/etc. 
  and passed further down - i assume you are working on those
  issues already, or should i fix those too? ]

Signed-off-by: Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org>

Index: linux/drivers/kvm/mmu.c
===================================================================
--- linux.orig/drivers/kvm/mmu.c
+++ linux/drivers/kvm/mmu.c
@@ -378,7 +378,7 @@ static void rmap_remove(struct kvm_vcpu 
 	}
 }
 
-static void rmap_write_protect(struct kvm_vcpu *vcpu, u64 gfn)
+static int rmap_write_protect(struct kvm_vcpu *vcpu, u64 gfn)
 {
 	struct kvm *kvm = vcpu->kvm;
 	struct page *page;
@@ -387,7 +387,8 @@ static void rmap_write_protect(struct kv
 	u64 *spte;
 
 	slot = gfn_to_memslot(kvm, gfn);
-	BUG_ON(!slot);
+	if (!slot)
+		return -1;
 	page = gfn_to_page(slot, gfn);
 
 	while (page->private) {
@@ -407,6 +408,7 @@ static void rmap_write_protect(struct kv
 		kvm_arch_ops->tlb_flush(vcpu);
 		*spte &= ~(u64)PT_WRITABLE_MASK;
 	}
+	return 0;
 }
 
 static int is_empty_shadow_page(hpa_t page_hpa)
@@ -592,12 +594,17 @@ static struct kvm_mmu_page *kvm_mmu_get_
 	page = kvm_mmu_alloc_page(vcpu, parent_pte);
 	if (!page)
 		return page;
+	if (!metaphysical) {
+		if (rmap_write_protect(vcpu, gfn) < 0) {
+			kvm_mmu_free_page(vcpu, page->page_hpa);
+			return NULL;
+		}
+	}
 	pgprintk("%s: adding gfn %lx role %x\n", __FUNCTION__, gfn, role.word);
 	page->gfn = gfn;
 	page->role = role;
 	hlist_add_head(&page->hash_link, bucket);
-	if (!metaphysical)
-		rmap_write_protect(vcpu, gfn);
+
 	return page;
 }
 

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Avi Kivity]

Ingo Molnar wrote:
> From: Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org>
> Subject: [patch] kvm: make cr3 loading more robust
>
> rmap_write_protect() has a BUG_ON() if a physical address is not found 
> the the memslot. But this is a possible scenario if a buggy guest OS 
> loads an invalid or corrupted cr3 value. So exit more gracefully.
>
>   

I think a better solution is to detect the invalid cr3 in new_cr3(). 
That way the entire chain of error returns is avoided.

> [ Avi: this patch needs kvm_mmu_get_page() NULL return to not be ignored 
>   by mmu_alloc_roots()/nonpaging_init_context()/paging_new_cr3()/etc. 
>   and passed further down - i assume you are working on those
>   issues already, or should i fix those too? ]
>   

kvm_mmu_get_page() cannot return NULL (before this patch), as we ensure 
there are free pages before entry to page_fault.

> Signed-off-by: Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org>
>
> Index: linux/drivers/kvm/mmu.c
> ===================================================================
> --- linux.orig/drivers/kvm/mmu.c
> +++ linux/drivers/kvm/mmu.c
> @@ -378,7 +378,7 @@ static void rmap_remove(struct kvm_vcpu 
>  	}
>  }
>  
> -static void rmap_write_protect(struct kvm_vcpu *vcpu, u64 gfn)
> +static int rmap_write_protect(struct kvm_vcpu *vcpu, u64 gfn)
>  {
>  	struct kvm *kvm = vcpu->kvm;
>  	struct page *page;
> @@ -387,7 +387,8 @@ static void rmap_write_protect(struct kv
>  	u64 *spte;
>  
>  	slot = gfn_to_memslot(kvm, gfn);
> -	BUG_ON(!slot);
> +	if (!slot)
> +		return -1;
>  	page = gfn_to_page(slot, gfn);
>  
>  	while (page->private) {
> @@ -407,6 +408,7 @@ static void rmap_write_protect(struct kv
>  		kvm_arch_ops->tlb_flush(vcpu);
>  		*spte &= ~(u64)PT_WRITABLE_MASK;
>  	}
> +	return 0;
>  }
>  
>  static int is_empty_shadow_page(hpa_t page_hpa)
> @@ -592,12 +594,17 @@ static struct kvm_mmu_page *kvm_mmu_get_
>  	page = kvm_mmu_alloc_page(vcpu, parent_pte);
>  	if (!page)
>  		return page;
> +	if (!metaphysical) {
> +		if (rmap_write_protect(vcpu, gfn) < 0) {
> +			kvm_mmu_free_page(vcpu, page->page_hpa);
> +			return NULL;
> +		}
> +	}
>  	pgprintk("%s: adding gfn %lx role %x\n", __FUNCTION__, gfn, role.word);
>  	page->gfn = gfn;
>  	page->role = role;
>  	hlist_add_head(&page->hash_link, bucket);
> -	if (!metaphysical)
> -		rmap_write_protect(vcpu, gfn);
> +
>  	return page;
>  }
>  
>   


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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Ingo Molnar]

* Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote:

> >rmap_write_protect() has a BUG_ON() if a physical address is not 
> >found the the memslot. But this is a possible scenario if a buggy 
> >guest OS loads an invalid or corrupted cr3 value. So exit more 
> >gracefully.
> 
> I think a better solution is to detect the invalid cr3 in new_cr3(). 
> That way the entire chain of error returns is avoided.

i'm wondering what the right semantics would be though. Kill the VM 
context?

on a real CPU an invalid cr3 does nothing explicitly (besides being a 
sure way to get a triple fault) - physical memory is always accessible, 
non-present physical memory at most generates an #MCE, but is typically 
just returning 0xff, right? So perhaps keep a non-mapped page filled 
with 0xff and map non-existent RAM to that? But then this 'RAM' needs to 
be made non-writable. Looks a bit messy. Cleanest would be to kill the 
VM context (without injecting any fault into the guest), but that would 
then require an error return chain from set_cr3() ...

	Ingo

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Avi Kivity]

Ingo Molnar wrote:
> * Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote:
>
>   
>>> rmap_write_protect() has a BUG_ON() if a physical address is not 
>>> found the the memslot. But this is a possible scenario if a buggy 
>>> guest OS loads an invalid or corrupted cr3 value. So exit more 
>>> gracefully.
>>>       
>> I think a better solution is to detect the invalid cr3 in new_cr3(). 
>> That way the entire chain of error returns is avoided.
>>     
>
> i'm wondering what the right semantics would be though. Kill the VM 
> context?
>
> on a real CPU an invalid cr3 does nothing explicitly (besides being a 
> sure way to get a triple fault) - physical memory is always accessible, 
> non-present physical memory at most generates an #MCE, but is typically 
> just returning 0xff, right? So perhaps keep a non-mapped page filled 
> with 0xff and map non-existent RAM to that? But then this 'RAM' needs to 
> be made non-writable.

That's a good solution.  I don't see why it has to be made non-writable 
-- it has undefined content, and any old value will do.  We have (or 
maybe had) something like that somewhere.


>  Looks a bit messy. Cleanest would be to kill the 
> VM context (without injecting any fault into the guest), but that would 
> then require an error return chain from set_cr3() ...
>   

set_cr3() is fairly close to the top.  I'm mostly worried about keeping 
the innards of the mmu clean.

An alternative is to add a flag to the vcpu which would be examined on 
entry (vcpu->triple_faulted).


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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Avi Kivity]

Avi Kivity wrote:
> Ingo Molnar wrote:
>> * Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote:
>>
>>  
>>>> rmap_write_protect() has a BUG_ON() if a physical address is not 
>>>> found the the memslot. But this is a possible scenario if a buggy 
>>>> guest OS loads an invalid or corrupted cr3 value. So exit more 
>>>> gracefully.
>>>>       
>>> I think a better solution is to detect the invalid cr3 in new_cr3(). 
>>> That way the entire chain of error returns is avoided.
>>>     
>>
>> i'm wondering what the right semantics would be though. Kill the VM 
>> context?
>>
>> on a real CPU an invalid cr3 does nothing explicitly (besides being a 
>> sure way to get a triple fault) - physical memory is always 
>> accessible, non-present physical memory at most generates an #MCE, 
>> but is typically just returning 0xff, right? So perhaps keep a 
>> non-mapped page filled with 0xff and map non-existent RAM to that? 
>> But then this 'RAM' needs to be made non-writable.
>
> That's a good solution.  I don't see why it has to be made 
> non-writable -- it has undefined content, and any old value will do.  
> We have (or maybe had) something like that somewhere.

It probably solves other cases as well, and simplifies error handling 
elsewhere.

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Ingo Molnar]

* Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote:

> Ingo Molnar wrote:
> >* Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote:
> >
> >  
> >>>rmap_write_protect() has a BUG_ON() if a physical address is not 
> >>>found the the memslot. But this is a possible scenario if a buggy 
> >>>guest OS loads an invalid or corrupted cr3 value. So exit more 
> >>>gracefully.
> >>>      
> >>I think a better solution is to detect the invalid cr3 in new_cr3(). 
> >>That way the entire chain of error returns is avoided.
> >>    
> >
> >i'm wondering what the right semantics would be though. Kill the VM 
> >context?
> >
> >on a real CPU an invalid cr3 does nothing explicitly (besides being a 
> >sure way to get a triple fault) - physical memory is always accessible, 
> >non-present physical memory at most generates an #MCE, but is typically 
> >just returning 0xff, right? So perhaps keep a non-mapped page filled 
> >with 0xff and map non-existent RAM to that? But then this 'RAM' needs to 
> >be made non-writable.
> 
> That's a good solution.  I don't see why it has to be made 
> non-writable -- it has undefined content, and any old value will do.  
> We have (or maybe had) something like that somewhere.

it should always return 0xff content because that's how real hardware 
behaves. It's essentially ROM-alike, with 0xff content. Writes are 
ignored. (last i checked)

also having it writable means it's an information leak as we want to 
share this page amongst guests, etc. Then explicitly faulting the guest 
would be alot cleaner.

> > Looks a bit messy. Cleanest would be to kill the VM context (without 
> >injecting any fault into the guest), but that would then require an 
> >error return chain from set_cr3() ...
> 
> set_cr3() is fairly close to the top.  I'm mostly worried about 
> keeping the innards of the mmu clean.

ok, agreed.

> An alternative is to add a flag to the vcpu which would be examined on 
> entry (vcpu->triple_faulted).

well, the triple fault isnt really explicit behavior of the cr3 loading, 
it is "just" a side-effect of having an all-0xff piece of physical 
memory holding the CPU's page tables. Such a cr3 can be loaded fine, but 
the next instruction fetched will be 0xff 0xff, which should be an 
undefined opcode. The resulting fault will try to execute based off an 
invalid IDT so we get a double fault, which then also tries to execute 
0xff 0xff (if the IDT entry didnt generate a #GPF beforehand, due to an 
invalid segment descriptor) so it results in a triple fault. Does VMX 
report triple faults?

	Ingo

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Avi Kivity]

Ingo Molnar wrote:
>> That's a good solution.  I don't see why it has to be made 
>> non-writable -- it has undefined content, and any old value will do.  
>> We have (or maybe had) something like that somewhere.
>>     
>
> it should always return 0xff content because that's how real hardware 
> behaves. It's essentially ROM-alike, with 0xff content. Writes are 
> ignored. (last i checked)
>
>   

Still, software can't depend on it (maybe some old stuff does to get the 
end of memory).  You can't randomly poke at memory.


> also having it writable means it's an information leak as we want to 
> share this page amongst guests, etc. Then explicitly faulting the guest 
> would be alot cleaner.
>   

We can have a per-vm page.

>   
>> An alternative is to add a flag to the vcpu which would be examined on 
>> entry (vcpu->triple_faulted).
>>     
>
> well, the triple fault isnt really explicit behavior of the cr3 loading, 
> it is "just" a side-effect of having an all-0xff piece of physical 
> memory holding the CPU's page tables. Such a cr3 can be loaded fine, but 
> the next instruction fetched will be 0xff 0xff, which should be an 
> undefined opcode. The resulting fault will try to execute based off an 
> invalid IDT so we get a double fault, which then also tries to execute 
> 0xff 0xff (if the IDT entry didnt generate a #GPF beforehand, due to an 
> invalid segment descriptor) so it results in a triple fault. 

It's an optimization then :)

> Does VMX 
> report triple faults?
>   

Yes.

If we add a "read-only memory slot" abstraction we can use it for the 
unwired address space.

Note that the corner cases will never be 100% emulatable.  For example, 
you can set cr3 to point at your IDE DMA mmio space or something like 
that.  It's quite all right to kill the guest quietly at that point, as 
no real-life guest will do that.

The kvm goals do not include cycle accurate emulation.  We want to be 
reasonably close to real hardware for the real-life uses.  I'd like to 
get the other cases right too, but not at the expense of simplicity and 
correctness.  Of course, it has to be secure even in non-real-life 
situations.

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Ingo Molnar]

* Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote:

> Note that the corner cases will never be 100% emulatable.  For 
> example, you can set cr3 to point at your IDE DMA mmio space or 
> something like that.  It's quite all right to kill the guest quietly 
> at that point, as no real-life guest will do that.

yes. Or to map the lapic to the IDT ;-) (as yours truly has tried it 
years ago)

that's why my suggestion is to just kill the guest. Loading such a cr3 
is a serious bug that might be hard to debug in the guest. I had to 
debug at least one such bug in Linux before (years ago, in the lazy TLB 
switching code) and it was a royal PITA to track down. Having a 
hypervisor that points any cr3 load error out /before/ the effects of 
the error propagate further is a bonus, not an incompatibility. The CPU 
does not implement this not because the semantics is important, but i 
suspect mostly because it doesnt really know the boundaries and type of 
RAM.

> The kvm goals do not include cycle accurate emulation. [...]

yes. That's why i'm suggesting to kill the VM in such a scenario. A cr3 
value is only valid if it points to real RAM.

	Ingo

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Ingo Molnar]

* Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org> wrote:

> > The kvm goals do not include cycle accurate emulation. [...]
> 
> yes. That's why i'm suggesting to kill the VM in such a scenario. A 
> cr3 value is only valid if it points to real RAM.

and if it ever causes real incompatibility it should be easy to address 
it by returning a per-vcpu (or per-vm) special memory slot in 
gfn_to_memslot(), for invalid memory, instead of the current NULL, 
right?

	Ingo

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Avi Kivity]

Ingo Molnar wrote:
> * Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org> wrote:
>
>   
>>> The kvm goals do not include cycle accurate emulation. [...]
>>>       
>> yes. That's why i'm suggesting to kill the VM in such a scenario. A 
>> cr3 value is only valid if it points to real RAM.
>>     
>
> and if it ever causes real incompatibility it should be easy to address 
> it by returning a per-vcpu (or per-vm) special memory slot in 
> gfn_to_memslot(), for invalid memory, instead of the current NULL, 
> right?
>   

Yes.

A read-only memory slot may have other uses too (for ROMs).

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Ingo Molnar]

* Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote:

> Yes.
> 
> A read-only memory slot may have other uses too (for ROMs).

a related question: i was wondering about the purpose of the 
KVM_MEM_LOG_DIRTY_PAGES feature that is attached to physical memory 
slots. Is that used so that you can get a compact bitmap of all modified 
pages within a Qemu-emulated framebuffer, resulting in a delta update, 
instead of having to do the full framebuffer update every time?

	Ingo

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Avi Kivity]

Ingo Molnar wrote:
> * Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote:
>
>   
>> Yes.
>>
>> A read-only memory slot may have other uses too (for ROMs).
>>     
>
> a related question: i was wondering about the purpose of the 
> KVM_MEM_LOG_DIRTY_PAGES feature that is attached to physical memory 
> slots. Is that used so that you can get a compact bitmap of all modified 
> pages within a Qemu-emulated framebuffer, resulting in a delta update, 
> instead of having to do the full framebuffer update every time?
>   

Yes.  A future use is the live migration thing (although we need to 
update the mmu to mark guest page tables when it sets the dirty and 
accessed bits for that, or when it emulates an instruction).



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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Avi Kivity]

Ingo Molnar wrote:
> * Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote:
>
>   
>> Note that the corner cases will never be 100% emulatable.  For 
>> example, you can set cr3 to point at your IDE DMA mmio space or 
>> something like that.  It's quite all right to kill the guest quietly 
>> at that point, as no real-life guest will do that.
>>     
>
> yes. Or to map the lapic to the IDT ;-) (as yours truly has tried it 
> years ago)
>
> that's why my suggestion is to just kill the guest. Loading such a cr3 
> is a serious bug that might be hard to debug in the guest. I had to 
> debug at least one such bug in Linux before (years ago, in the lazy TLB 
> switching code) and it was a royal PITA to track down. Having a 
> hypervisor that points any cr3 load error out /before/ the effects of 
> the error propagate further is a bonus, not an incompatibility. The CPU 
> does not implement this not because the semantics is important, but i 
> suspect mostly because it doesnt really know the boundaries and type of 
> RAM.
>
>   
>> The kvm goals do not include cycle accurate emulation. [...]
>>     
>
> yes. That's why i'm suggesting to kill the VM in such a scenario. A cr3 
> value is only valid if it points to real RAM.
>
>   

Yes.

The value of the bad ram page is that it avoids a check in many places 
where a guest page is referenced, and so reduces the number of hard to 
test error paths.  It's hard to make the case for cr3 changes alone, but 
when you consider demand faulting, it makes more sense.


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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Ingo Molnar]

* Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote:

> >[ Avi: this patch needs kvm_mmu_get_page() NULL return to not be ignored 
> >  by mmu_alloc_roots()/nonpaging_init_context()/paging_new_cr3()/etc. 
> >  and passed further down - i assume you are working on those
> >  issues already, or should i fix those too? ]
> 
> kvm_mmu_get_page() cannot return NULL (before this patch), as we 
> ensure there are free pages before entry to page_fault.

hm, where do we ensure that? kvm_mmu_get_page() calls 
kvm_mmu_alloc_page(), which might return NULL:

 static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu,
                                                u64 *parent_pte)
 {
         struct kvm_mmu_page *page;

         if (list_empty(&vcpu->free_pages))
                 return NULL;

and i dont see where we ensure that ->free_pages is not empty. The 
relevant callchain would be kvm_vmx_return() -> handle_cr() -> set_cr3() 
-> paging_new_cr3() -> mmu_alloc_roots() -> kvm_mmu_get_page().

is there some accounting that makes a NULL pointer return impossible? 
The only one i can see is the direct quering of vcpu->free_pages or the 
querying of n_free_mmu_pages, but neither seems to be done in this 
codepath. (but i might have missed it)

	Ingo

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Avi Kivity]

Ingo Molnar wrote:
> * Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote:
>
>   
>>> [ Avi: this patch needs kvm_mmu_get_page() NULL return to not be ignored 
>>>  by mmu_alloc_roots()/nonpaging_init_context()/paging_new_cr3()/etc. 
>>>  and passed further down - i assume you are working on those
>>>  issues already, or should i fix those too? ]
>>>       
>> kvm_mmu_get_page() cannot return NULL (before this patch), as we 
>> ensure there are free pages before entry to page_fault.
>>     
>
> hm, where do we ensure that? kvm_mmu_get_page() calls 
> kvm_mmu_alloc_page(), which might return NULL:
>
>  static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu,
>                                                 u64 *parent_pte)
>  {
>          struct kvm_mmu_page *page;
>
>          if (list_empty(&vcpu->free_pages))
>                  return NULL;
>
> and i dont see where we ensure that ->free_pages is not empty. The 
> relevant callchain would be kvm_vmx_return() -> handle_cr() -> set_cr3() 
> -> paging_new_cr3() -> mmu_alloc_roots() -> kvm_mmu_get_page().
>
> is there some accounting that makes a NULL pointer return impossible? 
> The only one i can see is the direct quering of vcpu->free_pages or the 
> querying of n_free_mmu_pages, but neither seems to be done in this 
> codepath. (but i might have missed it)
>
>   

The page fault path does this:

static inline int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t gva,
                     u32 error_code)
{
    if (unlikely(vcpu->kvm->n_free_mmu_pages < KVM_MIN_FREE_MMU_PAGES))
        kvm_mmu_free_some_pages(vcpu);
    return vcpu->mmu.page_fault(vcpu, gva, error_code);
}



The context switch paths need to do the same.


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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Ingo Molnar]

* Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote:

> >hm, where do we ensure that? kvm_mmu_get_page() calls 
> >kvm_mmu_alloc_page(), which might return NULL:
> >
> > static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu,
> >                                                u64 *parent_pte)
> > {
> >         struct kvm_mmu_page *page;
> >
> >         if (list_empty(&vcpu->free_pages))
> >                 return NULL;
> >
> >and i dont see where we ensure that ->free_pages is not empty. The 
> >relevant callchain would be kvm_vmx_return() -> handle_cr() -> 
> >set_cr3() -> paging_new_cr3() -> mmu_alloc_roots() -> 
> >kvm_mmu_get_page().
> >
> >is there some accounting that makes a NULL pointer return impossible? 
> >The only one i can see is the direct quering of vcpu->free_pages or 
> >the querying of n_free_mmu_pages, but neither seems to be done in 
> >this codepath. (but i might have missed it)
> >
> >  
> 
> The page fault path does this:
> 
> static inline int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t gva,
>                     u32 error_code)
> {
>    if (unlikely(vcpu->kvm->n_free_mmu_pages < KVM_MIN_FREE_MMU_PAGES))
>        kvm_mmu_free_some_pages(vcpu);
>    return vcpu->mmu.page_fault(vcpu, gva, error_code);
> }

ah. This hid in an include file. (Please dont put real program logic 
into include files - although i guess this is a justified exception :-)

> The context switch paths need to do the same.

yeah. Although, as this example has shown it, such implicit assumptions 
carried into code tends to be volatile. Couldnt the kvm_mmu_zap_page() 
be done implicitly within kvm_mmu_alloc_page()? As long as the list is 
LRU, and the number of allocations done within a logical operation 
doesnt exceed the size of the LRU list (which it doesnt), this should be 
doable.

	Ingo

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Avi Kivity]

Ingo Molnar wrote:
>   
>> The context switch paths need to do the same.
>>     
>
> yeah. Although, as this example has shown it, such implicit assumptions 
> carried into code tends to be volatile. Couldnt the kvm_mmu_zap_page() 
> be done implicitly within kvm_mmu_alloc_page()? As long as the list is 
> LRU, and the number of allocations done within a logical operation 
> doesnt exceed the size of the LRU list (which it doesnt), this should be 
> doable.
>   

Right not the list is not LRU, so a zap can kill your parent page 
table.  I'm also worried about preserving invariants, as the page tables 
are linked through a variety of data structures.  Calling zap_page() 
while another operation is in progress could cause corruption if 
zap_page() kills one of your parents or children.

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Ingo Molnar]

* Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote:

> Right not the list is not LRU, so a zap can kill your parent page 
> table.  I'm also worried about preserving invariants, as the page 
> tables are linked through a variety of data structures.  Calling 
> zap_page() while another operation is in progress could cause 
> corruption if zap_page() kills one of your parents or children.

ok. How about the patch below then? This only addresses the OOM 
scenario, not the !memslot case.

	Ingo

Signed-off-by: Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org>

Index: linux/drivers/kvm/mmu.c
===================================================================
--- linux.orig/drivers/kvm/mmu.c
+++ linux/drivers/kvm/mmu.c
@@ -903,6 +903,8 @@ static void paging_new_cr3(struct kvm_vc
 {
 	pgprintk("%s: cr3 %lx\n", __FUNCTION__, vcpu->cr3);
 	mmu_free_roots(vcpu);
+	if (unlikely(vcpu->kvm->n_free_mmu_pages < KVM_MIN_FREE_MMU_PAGES))
+		kvm_mmu_free_some_pages(vcpu);
 	mmu_alloc_roots(vcpu);
 	kvm_mmu_flush_tlb(vcpu);
 	kvm_arch_ops->set_cr3(vcpu, vcpu->mmu.root_hpa);

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Avi Kivity]

Ingo Molnar wrote:
> ok. How about the patch below then? This only addresses the OOM 
> scenario, not the !memslot case.
>
>   

Applied, thanks.

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Ingo Molnar]

* Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org> wrote:

> ok. How about the patch below then? This only addresses the OOM 
> scenario, not the !memslot case.

the !memslot case is covered by the patch below. Injecting a #GPF is the 
easiest one to do here, although we could do a triple fault too - i just 
dont see the infrastructure for that in KVM, so i went for the easier 
solution ;-)

I have tested this with an intentionally bad cr3 value in a Linux guest, 
and the result is a relatively clean guest abort crash:

  inject_general_protection: rip 0xc012093e
  kvm_handle_exit: unexpected, valid vectoring info and exit reason is 0x9

at the right RIP:

  c012093e:       0f 22 d8                mov    %eax,%cr3

instead of a host crash. Note that i chose to put this into the generic 
cr3 loading function, so that it covers real-mode too. I think we can 
safely ignore a BIOS loading crap into cr3 and after that loading the 
right value into it. (if that ever happens we 1) want to know about it 
2) can push the test down into paging_new_cr3()) Agreed?

	Ingo

Signed-off-by: Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org>

Index: linux/drivers/kvm/kvm_main.c
===================================================================
--- linux.orig/drivers/kvm/kvm_main.c
+++ linux/drivers/kvm/kvm_main.c
@@ -466,7 +466,19 @@ void set_cr3(struct kvm_vcpu *vcpu, unsi
 
 	vcpu->cr3 = cr3;
 	spin_lock(&vcpu->kvm->lock);
-	vcpu->mmu.new_cr3(vcpu);
+	/*
+	 * Does the new cr3 value map to physical memory? (Note, we
+	 * catch an invalid cr3 even in real-mode, because it would
+	 * cause trouble later on when we turn on paging anyway.)
+	 *
+	 * A real CPU would silently accept an invalid cr3 and would
+	 * attempt to use it - with largely undefined (and often hard
+	 * to debug) behavior on the guest side.
+	 */
+	if (unlikely(!gfn_to_memslot(vcpu->kvm, cr3 >> PAGE_SHIFT)))
+		inject_gp(vcpu);
+	else
+		vcpu->mmu.new_cr3(vcpu);
 	spin_unlock(&vcpu->kvm->lock);
 }
 EXPORT_SYMBOL_GPL(set_cr3);

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Ingo Molnar]

* Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org> wrote:

> instead of a host crash. Note that i chose to put this into the 
> generic cr3 loading function, so that it covers real-mode too. I think 
> we can safely ignore a BIOS loading crap into cr3 and after that 
> loading the right value into it. (if that ever happens we 1) want to 
> know about it 2) can push the test down into paging_new_cr3()) Agreed?

another small detail is that currently KVM_SET_MEMORY_REGION appears to 
be an add-only interface - it is not possible to 'unregister' RAM from a 
VM.

That keeps things easy for now, but if it's ever implemented then the 
current cr3 of all vcpus of the VM needs to be validated against the 
reduced memory slot map. (besides migrating all existing mappings from 
the removed memory slot to other memory slots and redirecting all 
in-flight DMA transactions, etc., etc. Which all needs heavy guest-OS 
awareness as well.)

	Ingo

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Avi Kivity]

Ingo Molnar wrote:
> another small detail is that currently KVM_SET_MEMORY_REGION appears to 
> be an add-only interface - it is not possible to 'unregister' RAM from a 
> VM.
>   

Well, the _interface_ supports removing, the implementation does not :)

Everything was written in mind to allow memory hotplug.

> That keeps things easy for now, but if it's ever implemented then the 
> current cr3 of all vcpus of the VM needs to be validated against the 
> reduced memory slot map. (besides migrating all existing mappings from 
> the removed memory slot to other memory slots and redirecting all 
> in-flight DMA transactions, etc., etc. Which all needs heavy guest-OS 
> awareness as well.)
>   

Actually I think it's quite easy:

- halt all vcpus
- wait for dma to complete
- mmu_free_roots()
- zap all page tables
- actually unplug memory
- mmu_alloc_roots()

The guest needs to cooperate, but it can do so using the native memory 
hotlpug mechanisms (whatever they are).  No guest modifications are needed.

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Ingo Molnar]

* Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote:

> The guest needs to cooperate, but it can do so using the native memory 
> hotlpug mechanisms (whatever they are). [...]

as far a Linux guest goes, there's no such thing at the moment, at least 
in the mainline kernel. Most of the difficulties with RAM-unplug is on 
the guest OS side - i agree with you that doing it on the host side is 
easy. (because the host side does not really 'use' any of the guest's 
"RAM".)

	Ingo

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [patch] kvm: make cr3 loading more robust

[Avi Kivity]

Ingo Molnar wrote:
> * Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org> wrote:
>
>   
>> ok. How about the patch below then? This only addresses the OOM 
>> scenario, not the !memslot case.
>>     
>
> the !memslot case is covered by the patch below. Injecting a #GPF is the 
> easiest one to do here, although we could do a triple fault too - i just 
> dont see the infrastructure for that in KVM, so i went for the easier 
> solution ;-)
>
> I have tested this with an intentionally bad cr3 value in a Linux guest, 
> and the result is a relatively clean guest abort crash:
>
>   inject_general_protection: rip 0xc012093e
>   kvm_handle_exit: unexpected, valid vectoring info and exit reason is 0x9
>
> at the right RIP:
>
>   c012093e:       0f 22 d8                mov    %eax,%cr3
>
> instead of a host crash. Note that i chose to put this into the generic 
> cr3 loading function, so that it covers real-mode too. I think we can 
> safely ignore a BIOS loading crap into cr3 and after that loading the 
> right value into it. (if that ever happens we 1) want to know about it 
> 2) can push the test down into paging_new_cr3()) Agreed?
>
> 	Ingo
>
>   

Applied, thanks.

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV