| Message ID | 20121206222037.24968.13698.stgit@bling.home (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 06, 2012 at 03:20:37PM -0700, Alex Williamson wrote: > The API documents that only flags and guest physical memory space can > be modified on an existing slot, but we don't enforce that the > userspace address cannot be modified. Instead we just ignore it. > This means that a user may think they've successfully moved both the > guest and user addresses, when in fact only the guest address changed. > Check and error instead. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > virt/kvm/kvm_main.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index e426704..93213e1 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -779,13 +779,19 @@ int __kvm_set_memory_region(struct kvm *kvm, > > r = -ENOMEM; > > - /* Allocate if a slot is being created */ > + /* > + * Allocate if a slot is being created. If modifying a slot, > + * the userspace_addr cannot change. > + */ > if (!old.npages) { > new.user_alloc = user_alloc; > new.userspace_addr = mem->userspace_addr; > > if (kvm_arch_create_memslot(&new, npages)) > goto out_free; > + } else if (mem->userspace_addr != old.userspace_addr) { > + r = -EINVAL; > + goto out_free; > } > > /* Allocate page dirty bitmap if needed */ > hmmm...does this mean that on a 'destroy', where npages is 0, the user has to set up userspace_addr correctly? If so, that would appear to be an unwanted change in semantics here. Thanks, -Jason -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2012-12-07 at 13:17 -0500, Jason Baron wrote: > On Thu, Dec 06, 2012 at 03:20:37PM -0700, Alex Williamson wrote: > > The API documents that only flags and guest physical memory space can > > be modified on an existing slot, but we don't enforce that the > > userspace address cannot be modified. Instead we just ignore it. > > This means that a user may think they've successfully moved both the > > guest and user addresses, when in fact only the guest address changed. > > Check and error instead. > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > --- > > virt/kvm/kvm_main.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index e426704..93213e1 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -779,13 +779,19 @@ int __kvm_set_memory_region(struct kvm *kvm, > > > > r = -ENOMEM; > > > > - /* Allocate if a slot is being created */ > > + /* > > + * Allocate if a slot is being created. If modifying a slot, > > + * the userspace_addr cannot change. > > + */ > > if (!old.npages) { > > new.user_alloc = user_alloc; > > new.userspace_addr = mem->userspace_addr; > > > > if (kvm_arch_create_memslot(&new, npages)) > > goto out_free; > > + } else if (mem->userspace_addr != old.userspace_addr) { > > + r = -EINVAL; > > + goto out_free; > > } > > > > /* Allocate page dirty bitmap if needed */ > > > > hmmm...does this mean that on a 'destroy', where npages is 0, the user > has to set up userspace_addr correctly? If so, that would appear to be > an unwanted change in semantics here. Good point, it does change that. We could make this be (npages && mem->userspace_addr != old.userspace_addr) to avoid that case. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 07, 2012 at 11:32:08AM -0700, Alex Williamson wrote: > On Fri, 2012-12-07 at 13:17 -0500, Jason Baron wrote: > > On Thu, Dec 06, 2012 at 03:20:37PM -0700, Alex Williamson wrote: > > > The API documents that only flags and guest physical memory space can > > > be modified on an existing slot, but we don't enforce that the > > > userspace address cannot be modified. Instead we just ignore it. > > > This means that a user may think they've successfully moved both the > > > guest and user addresses, when in fact only the guest address changed. > > > Check and error instead. > > > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > > --- > > > virt/kvm/kvm_main.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > > index e426704..93213e1 100644 > > > --- a/virt/kvm/kvm_main.c > > > +++ b/virt/kvm/kvm_main.c > > > @@ -779,13 +779,19 @@ int __kvm_set_memory_region(struct kvm *kvm, > > > > > > r = -ENOMEM; > > > > > > - /* Allocate if a slot is being created */ > > > + /* > > > + * Allocate if a slot is being created. If modifying a slot, > > > + * the userspace_addr cannot change. > > > + */ > > > if (!old.npages) { > > > new.user_alloc = user_alloc; > > > new.userspace_addr = mem->userspace_addr; > > > > > > if (kvm_arch_create_memslot(&new, npages)) > > > goto out_free; > > > + } else if (mem->userspace_addr != old.userspace_addr) { > > > + r = -EINVAL; > > > + goto out_free; > > > } > > > > > > /* Allocate page dirty bitmap if needed */ > > > > > > > hmmm...does this mean that on a 'destroy', where npages is 0, the user > > has to set up userspace_addr correctly? If so, that would appear to be > > an unwanted change in semantics here. > > Good point, it does change that. We could make this be (npages && > mem->userspace_addr != old.userspace_addr) to avoid that case. Thanks, > > Alex > Yup, that works for me. Thanks, -Jason -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index e426704..93213e1 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -779,13 +779,19 @@ int __kvm_set_memory_region(struct kvm *kvm, r = -ENOMEM; - /* Allocate if a slot is being created */ + /* + * Allocate if a slot is being created. If modifying a slot, + * the userspace_addr cannot change. + */ if (!old.npages) { new.user_alloc = user_alloc; new.userspace_addr = mem->userspace_addr; if (kvm_arch_create_memslot(&new, npages)) goto out_free; + } else if (mem->userspace_addr != old.userspace_addr) { + r = -EINVAL; + goto out_free; } /* Allocate page dirty bitmap if needed */
The API documents that only flags and guest physical memory space can be modified on an existing slot, but we don't enforce that the userspace address cannot be modified. Instead we just ignore it. This means that a user may think they've successfully moved both the guest and user addresses, when in fact only the guest address changed. Check and error instead. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- virt/kvm/kvm_main.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html