Re: [PATCH] always assign userspace_addr

[Anthony Liguori]

Glauber Costa wrote:
> On Wed, Nov 19, 2008 at 09:55:10AM -0600, Anthony Liguori wrote:
>   
> > 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@xxxxxxxxxx>
> > >   
> > >       
> > 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.
> >     
>
> Oh yeah, it does get freed.
>
> The delete path ends up in a kvm_free_physmem_slot, which will effectively
> vfree() the rmap structure. In fact, my userspace use case worked totally
> properly when I deleted the slot prior to re-registering it.
>   

That's not how I read the code.  I see:

> static void kvm_free_physmem_slot(struct kvm_memory_slot *free,
>                   struct kvm_memory_slot *dont)
> {
>     if (!dont || free->rmap != dont->rmap)
>         vfree(free->rmap);

And it's called as kvm_free_physmem_slot(&old, &new);

new is assigned to old to start out with so old.rmap will equal new.rmap.

> The problem here is when there is an already existant slot, and we are
> trying to change some information about it. The problem you are concerned
> basically does not exist, because it would raise only if we are changing
> the slot size. The code says:
>   

If a memory slot exists, the current code always deletes it and creates 
a new one so this problem shouldn't exist.  See

>             /* unregister whole slot */
>             memcpy(&slot, mem, sizeof(slot));
>             mem->memory_size = 0;
>             kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, mem);

But the problem still exists even with this code.  I checked.

So if you have something working without modifying the kernel, can you 
post it?

Regards,

Anthony Liguori
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/