[Qemu-devel] [PATCH] kvm: Fix overlapping check for memory slots

[Qemu-devel] [PATCH] kvm: Fix overlapping check for memory slots

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

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 <jan.kiszka@siemens.com>
---

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

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 363af32..18f06d2 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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)))


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH] kvm: Fix overlapping check for memory slots

[Sheng Yang]

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 <jan.kiszka@siemens.com>
> ---
>
>  virt/kvm/kvm_main.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 363af32..18f06d2 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -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.

-- 
regards
Yang, Sheng

[Qemu-devel] Re: [PATCH] kvm: Fix overlapping check for memory slots

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1377 bytes --]

Sheng Yang wrote:
> 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 <jan.kiszka@siemens.com>
>> ---
>>
>>  virt/kvm/kvm_main.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 363af32..18f06d2 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -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.

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

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH] kvm: Fix overlapping check for memory slots

[Sheng Yang]

On Monday 13 April 2009 16:50:40 Jan Kiszka wrote:
> Sheng Yang wrote:
> > 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 <jan.kiszka@siemens.com>
> >> ---
> >>
> >>  virt/kvm/kvm_main.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> >> index 363af32..18f06d2 100644
> >> --- a/virt/kvm/kvm_main.c
> >> +++ b/virt/kvm/kvm_main.c
> >> @@ -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.
>
> 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. 

-- 
regards
Yang, Sheng

[Qemu-devel] Re: [PATCH] kvm: Fix overlapping check for memory slots

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2037 bytes --]

Sheng Yang wrote:
> On Monday 13 April 2009 16:50:40 Jan Kiszka wrote:
>> Sheng Yang wrote:
>>> 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 <jan.kiszka@siemens.com>
>>>> ---
>>>>
>>>>  virt/kvm/kvm_main.c |    2 +-
>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>>>> index 363af32..18f06d2 100644
>>>> --- a/virt/kvm/kvm_main.c
>>>> +++ b/virt/kvm/kvm_main.c
>>>> @@ -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.
>> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH] kvm: Fix overlapping check for memory slots

[Avi Kivity]

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.
>   

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.


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

[Qemu-devel] Re: [PATCH] kvm: Fix overlapping check for memory slots

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

Avi Kivity wrote:
> 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.
>>   
> 
> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH] kvm: Fix overlapping check for memory slots

[Avi Kivity]

Jan Kiszka wrote:
> Avi Kivity wrote:
>   
>> 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.
>>>   
>>>       
>> 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.

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

[Qemu-devel] [PATCH v2] kvm: Fix overlapping check for memory slots

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1672 bytes --]

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.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

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

diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index ee755e2..644e3a9 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -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
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 363af32..3265566 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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:


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH v2] kvm: Fix overlapping check for memory slots

[Avi Kivity]

Jan Kiszka wrote:
> 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.

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