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

[Takuya Yoshikawa]

On Thu, 11 Jul 2013 10:41:53 +0300
Gleb Natapov <gleb@xxxxxxxxxx> 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@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/