lkml.org 
[lkml]   [2008]   [Nov]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] always assign userspace_addr
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@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.
>>
>
> 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


\
 
 \ /
  Last update: 2008-11-19 19:53    [W:0.368 / U:0.296 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site