| Message ID | 49E06754.8050906@web.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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.
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
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.
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
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.
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
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.
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)))
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(-)