kvm: reset arch memslot info on memslot creation
PATCH 1/1 kvm: reset arch memslot info on memslot creation
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.
--- arch/x86/kvm/x86.c | 4 ++++ 1 file changed, 4 insertions(+)
arch/x86/kvm/x86.c +4 -0
@@ -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;
Signature · 1 line
MST
Quoted 6 lines · On Wed, 10 Jul 2013 11:24:39 +0300 "Michael S. Tsirkin" <[email protected]> wrote:
On x86,
kvm_arch_create_memslotassumes that rmap/lpage_info for the slot are zeroed out: if they weren't, error handling code afterout_freelabel will free memory which wasn't allocated here.
This always happens to be the case because onKVM_MR_DELETEwe 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
Quoted 29 lines
Make the code more robust by clearing the rmap/lpage_info explicitly.
Signed-off-by Michael S. Tsirkin <[email protected]>--- arch/x86/kvm/x86.c | 4 ++++ 1 file changed, 4 insertions(+)arch/x86/kvm/x86.c +4
-0@@ -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;Signature · 5 lines
MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 1 line
Takuya Yoshikawa <[email protected]>
Quoted 10 lines · Takuya Yoshikawa
Quoted 6 lines · On Wed, 10 Jul 2013 11:24:39 +0300 "Michael S. Tsirkin" <[email protected]> wrote:
On x86,
kvm_arch_create_memslotassumes that rmap/lpage_info for the slot are zeroed out: if they weren't, error handling code afterout_freelabel will free memory which wasn't allocated here.
This always happens to be the case because onKVM_MR_DELETEwe clear the whole arch structure. So there's no bug, but it's cleaner not to rely on this here.
Quoted 7 lines
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?
Quoted 18 lines
Takuya
Quoted 16 lines
Make the code more robust by clearing the rmap/lpage_info explicitly.
Signed-off-by Michael S. Tsirkin <[email protected]>--- arch/x86/kvm/x86.c | 4 ++++ 1 file changed, 4 insertions(+)arch/x86/kvm/x86.c +0
-0@@ -6922,6 +6922,10 @@ int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages) {int i;
Quoted 17 lines
Quoted 13 lines
arch/x86/kvm/x86.c +4
-0+/* 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;Signature · 5 lines
MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.htmlSignature · 1 line
Takuya Yoshikawa <[email protected]>
--
Gleb.
Quoted 11 lines · On Thu, 11 Jul 2013 10:41:53 +0300 Gleb Natapov <[email protected]> wrote:
Quoted 10 lines · Takuya Yoshikawa
Quoted 6 lines · On Wed, 10 Jul 2013 11:24:39 +0300 "Michael S. Tsirkin" <[email protected]> wrote:
On x86,
kvm_arch_create_memslotassumes that rmap/lpage_info for the slot are zeroed out: if they weren't, error handling code afterout_freelabel will free memory which wasn't allocated here.
This always happens to be the case because onKVM_MR_DELETEwe clear the whole arch structure. So there's no bug, but it's cleaner not to rely on this here.
Quoted 9 lines
Quoted 7 lines
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