Messages in this thread | | | Date | Wed, 19 Nov 2008 09:55:10 -0600 | From | Anthony Liguori <> | Subject | Re: [PATCH] always assign userspace_addr |
| |
Glauber Costa wrote: > Currently, kvm only sets new.userspace_addr in slots > that were just allocated. This is not the intended behaviour, > and actually breaks when we try to use the slots to implement > aliases, for example. > > Cirrus VGA aliases maps and address to a userspace address, and > then keep mapping this same address to different locations > until the whole screen is filled. > > The solution is to assign new.userspace_addr no matter what, > so we can be sure that whenever the guest changes this field, > it sees the change being reflected in the code. > > Signed-off-by: Glauber Costa <glommer@redhat.com> >
I think this is masking a much bigger problem.
> --- > virt/kvm/kvm_main.c | 18 +++++++++--------- > 1 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index a87f45e..fc3abf0 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -762,15 +762,6 @@ int __kvm_set_memory_region(struct kvm *kvm, > memset(new.rmap, 0, npages * sizeof(*new.rmap)); > > new.user_alloc = user_alloc; > - /* > - * hva_to_rmmap() serialzies with the mmu_lock and to be > - * safe it has to ignore memslots with !user_alloc && > - * !userspace_addr. > - */ > - if (user_alloc) > - new.userspace_addr = mem->userspace_addr; > - else > - new.userspace_addr = 0; >
This is guarded in:
> if (npages && !new.rmap) {
In this case, npages > 0 but !new.rmap is already allocated. But this is a new slot? The problem is that when we delete the slot, the rmap never gets freed. This means that if we delete a slot, then create a new slot which happens to be a different size, we use the old rmap and potentially overrun that buffer.
So I think we need a fix that properly frees the rmap when the slot is destroyed.
Regards,
Anthony Liguori
| |