[PATCH] kvm: reset arch memslot info on memslot creation

[PATCH] kvm: reset arch memslot info on memslot creation

[Michael S. Tsirkin]

On x86, kvm_arch_create_memslot assumes that rmap/lpage_info for the
slot are zeroed out: if they weren't, error handling code after out_free
label will free memory which wasn't allocated here.
This always happens to be the case because on KVM_MR_DELETE we clear the
whole arch structure.  So there's no bug, but it's cleaner not to rely
on this here.

Make the code more robust by clearing the rmap/lpage_info explicitly.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 arch/x86/kvm/x86.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e8ba99c..96e6eb4 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6922,6 +6922,10 @@ int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages)
 {
 	int i;
 
+	/* Reset in case slot had some rmap/lpage_info. */
+	memset(&slot->arch.rmap, 0, sizeof slot->arch.rmap);
+	memset(&slot->arch.lpage_info, 0, sizeof slot->arch.lpage_info);
+
 	for (i = 0; i < KVM_NR_PAGE_SIZES; ++i) {
 		unsigned long ugfn;
 		int lpages;
-- 
MST

Re: [PATCH] kvm: reset arch memslot info on memslot creation

[Takuya Yoshikawa]

On Wed, 10 Jul 2013 11:24:39 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On x86, kvm_arch_create_memslot assumes that rmap/lpage_info for the
> slot are zeroed out: if they weren't, error handling code after out_free
> label will free memory which wasn't allocated here.
> This always happens to be the case because on KVM_MR_DELETE we clear the
> whole arch structure.  So there's no bug, but it's cleaner not to rely
> on this here.

Yes, the assumption is that the function is called only with zero-sized slots.
Since changing the size is not allowed, DELETE-CREATE is the only case we
care about.

But isn't it possible to make it explicit that zero-sized slots have always
zero-cleared contents instead?  Otherwise, there would be many troubles.

	Takuya

> 
> Make the code more robust by clearing the rmap/lpage_info explicitly.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e8ba99c..96e6eb4 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6922,6 +6922,10 @@ int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages)
>  {
>  	int i;
>  
> +	/* Reset in case slot had some rmap/lpage_info. */
> +	memset(&slot->arch.rmap, 0, sizeof slot->arch.rmap);
> +	memset(&slot->arch.lpage_info, 0, sizeof slot->arch.lpage_info);
> +
>  	for (i = 0; i < KVM_NR_PAGE_SIZES; ++i) {
>  		unsigned long ugfn;
>  		int lpages;
> -- 
> MST
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Takuya Yoshikawa <takuya.yoshikawa@gmail.com>

Re: [PATCH] kvm: reset arch memslot info on memslot creation

[Gleb Natapov]

On Wed, Jul 10, 2013 at 10:49:56PM +0900, Takuya Yoshikawa wrote:
> On Wed, 10 Jul 2013 11:24:39 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On x86, kvm_arch_create_memslot assumes that rmap/lpage_info for the
> > slot are zeroed out: if they weren't, error handling code after out_free
> > label will free memory which wasn't allocated here.
> > This always happens to be the case because on KVM_MR_DELETE we clear the
> > whole arch structure.  So there's no bug, but it's cleaner not to rely
> > on this here.
> 
> Yes, the assumption is that the function is called only with zero-sized slots.
> Since changing the size is not allowed, DELETE-CREATE is the only case we
> care about.
> 
> But isn't it possible to make it explicit that zero-sized slots have always
> zero-cleared contents instead?  Otherwise, there would be many troubles.
> 
Do you have something in mind?

> 	Takuya
> 
> > 
> > Make the code more robust by clearing the rmap/lpage_info explicitly.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  arch/x86/kvm/x86.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index e8ba99c..96e6eb4 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -6922,6 +6922,10 @@ int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages)
> >  {
> >  	int i;
> >  
> > +	/* Reset in case slot had some rmap/lpage_info. */
> > +	memset(&slot->arch.rmap, 0, sizeof slot->arch.rmap);
> > +	memset(&slot->arch.lpage_info, 0, sizeof slot->arch.lpage_info);
> > +
> >  	for (i = 0; i < KVM_NR_PAGE_SIZES; ++i) {
> >  		unsigned long ugfn;
> >  		int lpages;
> > -- 
> > MST
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> -- 
> Takuya Yoshikawa <takuya.yoshikawa@gmail.com>

--
			Gleb.

Re: [PATCH] kvm: reset arch memslot info on memslot creation

[Takuya Yoshikawa]

On Thu, 11 Jul 2013 10:41:53 +0300
Gleb Natapov <gleb@redhat.com> wrote:

> On Wed, Jul 10, 2013 at 10:49:56PM +0900, Takuya Yoshikawa wrote:
> > On Wed, 10 Jul 2013 11:24:39 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On x86, kvm_arch_create_memslot assumes that rmap/lpage_info for the
> > > slot are zeroed out: if they weren't, error handling code after out_free
> > > label will free memory which wasn't allocated here.
> > > This always happens to be the case because on KVM_MR_DELETE we clear the
> > > whole arch structure.  So there's no bug, but it's cleaner not to rely
> > > on this here.
> > 
> > Yes, the assumption is that the function is called only with zero-sized slots.
> > Since changing the size is not allowed, DELETE-CREATE is the only case we
> > care about.
> > 
> > But isn't it possible to make it explicit that zero-sized slots have always
> > zero-cleared contents instead?  Otherwise, there would be many troubles.
> > 
> Do you have something in mind?
> 

I remember that I once wrote code that assumed flags field was cleared before
creating a new slot and was pointed out that such assumptions might be dangerous:
actually, it's cleared separately but not so easy to notice.

So, I want to make it clear what differentiate DELETE'ed slots from others.

If we only assume (npages == 0), CREATE should properly set everything,
having out_free troubles in mind like this patch.  If we also assume the other
fields are zero, then DELETE is responsible for that assumption, some comment
in code may be helpful.

Actually, (rmap==NULL) was once used to check if we needed to allocate memory
for a new slot, meaning that we assumed the latter.  I felt uneasy and changed
that to (npages == 0).

Let's make it clear the underlying assumptions now.

	Takuya