kvm: Fix overlapping check for memory slots

PATCH Superseded Jan Kiszka 3 participants 10 messages Apr 11, 2009 – Apr 13, 2009 KVM

lore

Share & tools

lore

Jan Kiszka

author

2009-04-11 09:48 UTC #0 lore raw

PATCH 1/1 kvm: Fix overlapping check for memory slots

This nice little buglet complicates a smarter slot management in qemu user space just "slightly". Sigh...

-------->

When checking for overlapping slots on registration of a new one, kvm currently also considers zero-length (ie. deleted) slots and rejects requests incorrectly. This finally denies user space from joining slots. Fix the check by skipping deleted slots.

Signed-off-by Jan Kiszka <[email protected]>

---

 virt/kvm/kvm_main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

virt/kvm/kvm_main.c +1 -1

index 363af32..18f06d2 100644
@@ -1117,7 +1117,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	for (i = 0; i < KVM_MEMORY_SLOTS; ++i) {
 		struct kvm_memory_slot *s = &kvm->memslots[i];
 
-		if (s == memslot)
+		if (s == memslot || !s->npages)
 			continue;
 		if (!((base_gfn + npages <= s->base_gfn) ||
 		      (base_gfn >= s->base_gfn + s->npages)))

Sheng Yang

2009-04-13 05:47 UTC #1 lore raw

Quoted 21 lines · On Saturday 11 April 2009 17:48:04 Jan Kiszka wrote:

This nice little buglet complicates a smarter slot management in qemu user space just "slightly". Sigh...
-------->
When checking for overlapping slots on registration of a new one, kvm currently also considers zero-length (ie. deleted) slots and rejects requests incorrectly. This finally denies user space from joining slots. Fix the check by skipping deleted slots.
Signed-off-by Jan Kiszka <[email protected]>
---
 virt/kvm/kvm_main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
virt/kvm/kvm_main.c +0 -0
index 363af32..18f06d2 100644

virt/kvm/kvm_main.c int __kvm_set_memory_region(struct kvm *kvm, · line 1123

Quoted 8 lines

virt/kvm/kvm_main.c +1 -1

@@ -1117,7 +1117,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	for (i = 0; i < KVM_MEMORY_SLOTS; ++i) {
 		struct kvm_memory_slot *s = &kvm->memslots[i];
 
-		if (s == memslot)
+		if (s == memslot || !s->npages)
 			continue;
 		if (!((base_gfn + npages <= s->base_gfn) ||
 		      (base_gfn >= s->base_gfn + s->npages)))

Is it necessary to preserve a valid base_gfn/flags/etc for a zeroed slot? Seems kvm_free_physmem_slot didn't clean them.

Signature · 2 lines

regards
Yang, Sheng

Jan Kiszka

author

2009-04-13 08:50 UTC #2 lore raw

Quoted 25 lines · Sheng Yang

Quoted 24 lines · On Saturday 11 April 2009 17:48:04 Jan Kiszka wrote:
This nice little buglet complicates a smarter slot management in qemu user space just "slightly". Sigh...
-------->
When checking for overlapping slots on registration of a new one, kvm currently also considers zero-length (ie. deleted) slots and rejects requests incorrectly. This finally denies user space from joining slots. Fix the check by skipping deleted slots.
Signed-off-by Jan Kiszka <[email protected]>
---
 virt/kvm/kvm_main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
virt/kvm/kvm_main.c +0 -0
index 363af32..18f06d2 100644
@@ -1117,7 +1117,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	for (i = 0; i < KVM_MEMORY_SLOTS; ++i) {
 		struct kvm_memory_slot *s = &kvm->memslots[i];
 

Quoted 8 lines

virt/kvm/kvm_main.c int __kvm_set_memory_region(struct kvm *kvm, · line 1123
Quoted 5 lines
virt/kvm/kvm_main.c +1 -1
-		if (s == memslot)
+		if (s == memslot || !s->npages)
 			continue;
 		if (!((base_gfn + npages <= s->base_gfn) ||
 		      (base_gfn >= s->base_gfn + s->npages)))
Is it necessary to preserve a valid base_gfn/flags/etc for a zeroed slot? Seems kvm_free_physmem_slot didn't clean them.

It is not necessary as long as we ignore such slots (as this patch does).

Jan

Sheng Yang

2009-04-13 08:54 UTC #3 lore raw

Quoted 35 lines · On Monday 13 April 2009 16:50:40 Jan Kiszka wrote:

Quoted 25 lines · Sheng Yang
Quoted 24 lines · On Saturday 11 April 2009 17:48:04 Jan Kiszka wrote:
This nice little buglet complicates a smarter slot management in qemu user space just "slightly". Sigh...
-------->
When checking for overlapping slots on registration of a new one, kvm currently also considers zero-length (ie. deleted) slots and rejects requests incorrectly. This finally denies user space from joining slots. Fix the check by skipping deleted slots.
Signed-off-by Jan Kiszka <[email protected]>
---
 virt/kvm/kvm_main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
virt/kvm/kvm_main.c +0 -0
index 363af32..18f06d2 100644
@@ -1117,7 +1117,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	for (i = 0; i < KVM_MEMORY_SLOTS; ++i) {
 		struct kvm_memory_slot *s = &kvm->memslots[i];
 
Quoted 8 lines
virt/kvm/kvm_main.c int __kvm_set_memory_region(struct kvm *kvm, · line 1123
Quoted 5 lines
virt/kvm/kvm_main.c +1 -1
-		if (s == memslot)
+		if (s == memslot || !s->npages)
 			continue;
 		if (!((base_gfn + npages <= s->base_gfn) ||
 		      (base_gfn >= s->base_gfn + s->npages)))
Is it necessary to preserve a valid base_gfn/flags/etc for a zeroed slot? Seems kvm_free_physmem_slot didn't clean them.

Quoted 1 line

It is not necessary as long as we ignore such slots (as this patch does).

What I think is, if they are invalid and unnecessary to keep, it's better to clean them rather than add a additional check, for it should covered by current check.

Signature · 2 lines

regards
Yang, Sheng

Jan Kiszka

author

2009-04-13 09:03 UTC #4 lore raw

Quoted 34 lines · Sheng Yang

Quoted 33 lines · On Monday 13 April 2009 16:50:40 Jan Kiszka wrote:
Quoted 32 lines · Sheng Yang
Quoted 21 lines · On Saturday 11 April 2009 17:48:04 Jan Kiszka wrote:
This nice little buglet complicates a smarter slot management in qemu user space just "slightly". Sigh...
-------->
When checking for overlapping slots on registration of a new one, kvm currently also considers zero-length (ie. deleted) slots and rejects requests incorrectly. This finally denies user space from joining slots. Fix the check by skipping deleted slots.
Signed-off-by Jan Kiszka <[email protected]>
---
 virt/kvm/kvm_main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
virt/kvm/kvm_main.c +0 -0
index 363af32..18f06d2 100644
virt/kvm/kvm_main.c int __kvm_set_memory_region(struct kvm *kvm, · line 1123
Quoted 8 lines
virt/kvm/kvm_main.c +1 -1
@@ -1117,7 +1117,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	for (i = 0; i < KVM_MEMORY_SLOTS; ++i) {
 		struct kvm_memory_slot *s = &kvm->memslots[i];
 
-		if (s == memslot)
+		if (s == memslot || !s->npages)
 			continue;
 		if (!((base_gfn + npages <= s->base_gfn) ||
 		      (base_gfn >= s->base_gfn + s->npages)))
Is it necessary to preserve a valid base_gfn/flags/etc for a zeroed slot? Seems kvm_free_physmem_slot didn't clean them.

Quoted 5 lines

Quoted 1 line
It is not necessary as long as we ignore such slots (as this patch does).
What I think is, if they are invalid and unnecessary to keep, it's better to clean them rather than add a additional check, for it should covered by current check.

I think it is cleaner to add an explicit check for "slot unused" (!npages) than re-initializing it with "mostly harmless" values. I've no problem with zeroing them, but the test here should stay.

BTW, I was hoping to find a way to initialize deleted slots with safe values from user space to work around this bug, but I found none. :(

Jan

Avi Kivity

2009-04-13 09:32 UTC #5 lore raw

In reply to Jan Kiszka

Quoted 10 lines · Jan Kiszka

This nice little buglet complicates a smarter slot management in qemu user space just "slightly". Sigh...
-------->
When checking for overlapping slots on registration of a new one, kvm currently also considers zero-length (ie. deleted) slots and rejects requests incorrectly. This finally denies user space from joining slots. Fix the check by skipping deleted slots.

Can userspace fail gracefully when the bug is present? If not, the you should add a KVM_CAP_ to advertise the fix; without the capability don't attempt the smarter slot management.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2009-04-13 09:42 UTC #6 lore raw

Quoted 12 lines · Avi Kivity

Quoted 10 lines · Jan Kiszka
This nice little buglet complicates a smarter slot management in qemu user space just "slightly". Sigh...
-------->
When checking for overlapping slots on registration of a new one, kvm currently also considers zero-length (ie. deleted) slots and rejects requests incorrectly. This finally denies user space from joining slots. Fix the check by skipping deleted slots.

Quoted 3 lines

Can userspace fail gracefully when the bug is present? If not, the you should add a KVM_CAP_ to advertise the fix; without the capability don't attempt the smarter slot management.

I already thought about adding some KVM_CAP_DESTROY_MEMORY_REGION_NOW_REALLY_WORKS and skip the workaround in [1]. Maybe a good idea, comments welcome.

Jan

[1] http://permalink.gmane.org/gmane.comp.emulators.qemu/41326

Avi Kivity

2009-04-13 09:49 UTC #7 lore raw

Quoted 15 lines · Jan Kiszka

Quoted 13 lines · Avi Kivity
Quoted 11 lines · Jan Kiszka
This nice little buglet complicates a smarter slot management in qemu user space just "slightly". Sigh...
-------->
When checking for overlapping slots on registration of a new one, kvm currently also considers zero-length (ie. deleted) slots and rejects requests incorrectly. This finally denies user space from joining slots. Fix the check by skipping deleted slots.

Quoted 10 lines

Quoted 4 lines
Can userspace fail gracefully when the bug is present? If not, the you should add a KVM_CAP_ to advertise the fix; without the capability don't attempt the smarter slot management.
I already thought about adding some KVM_CAP_DESTROY_MEMORY_REGION_NOW_REALLY_WORKS and skip the workaround in [1]. Maybe a good idea, comments welcome.

It's a good idea regardless of how qemu handles it. There can be other users.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2009-04-13 09:59 UTC #8 lore raw

PATCH v2 1/1 kvm: Fix overlapping check for memory slots

In reply to Jan Kiszka

Signed-off-by Jan Kiszka <[email protected]>

---

 include/linux/kvm.h |    2 ++
 virt/kvm/kvm_main.c |    3 ++-
 2 files changed, 4 insertions(+), 1 deletions(-)

include/linux/kvm.h +2 -0

index ee755e2..644e3a9 100644
@@ -413,6 +413,8 @@ struct kvm_trace_rec {
 #define KVM_CAP_DEVICE_MSIX 28
 #endif
 #define KVM_CAP_ASSIGN_DEV_IRQ 29
+/* Another bug in KVM_SET_USER_MEMORY_REGION fixed: */
+#define KVM_CAP_JOIN_MEMORY_REGIONS_WORKS 30
 
 #ifdef KVM_CAP_IRQ_ROUTING

virt/kvm/kvm_main.c +2 -1

index 363af32..3265566 100644
@@ -1117,7 +1117,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	for (i = 0; i < KVM_MEMORY_SLOTS; ++i) {
 		struct kvm_memory_slot *s = &kvm->memslots[i];
 
-		if (s == memslot)
+		if (s == memslot || !s->npages)
 			continue;
 		if (!((base_gfn + npages <= s->base_gfn) ||
 		      (base_gfn >= s->base_gfn + s->npages)))

@@ -2262,6 +2262,7 @@ static long kvm_dev_ioctl_check_extension_generic(long arg)
 	switch (arg) {
 	case KVM_CAP_USER_MEMORY:
 	case KVM_CAP_DESTROY_MEMORY_REGION_WORKS:
+	case KVM_CAP_JOIN_MEMORY_REGIONS_WORKS:
 		return 1;
 #ifdef CONFIG_HAVE_KVM_IRQCHIP
 	case KVM_CAP_IRQ_ROUTING:

Avi Kivity

Applied

2009-04-13 13:04 UTC #9 lore raw

Quoted 6 lines · Jan Kiszka

When checking for overlapping slots on registration of a new one, kvm currently also considers zero-length (ie. deleted) slots and rejects requests incorrectly. This finally denies user space from joining slots. Fix the check by skipping deleted slots and advertise this via a KVM_CAP_JOIN_MEMORY_REGIONS_WORKS.

Applied, thanks.

Signature · 1 line

error compiling committee.c: too many arguments to function