Re: [PATCH 2/2] KVM: x86: map/unmap private slots in __x86_set_memory_region
2015-10-13 18:28+0200, Paolo Bonzini:
> On 13/10/2015 17:39, Radim Krčmář wrote:
>> 2015-10-12 14:09+0200, Paolo Bonzini:
>>> Otherwise, two copies (one of them never used and thus bogus) are
>>> allocated for the regular and SMM address spaces. This breaks
>>> SMM with EPT but without unrestricted guest support, because the
>>> SMM copy of the identity page map is all zeros.
>>
>> (Have you found out why EPT+unrestricted didn't use the alternative SMM
>> mapping as well?)
>
> Yes, that I already knew; EPT+unrestricted uses CR0.PG=0 directly so
> it doesn't use the identity page at all. (CR0.PG=0 w/o unrestricted
> instead runs with CR0.PG=1. CR3 load and store exits are enabled,
> and the guest CR3 always points to the identity page map while the
> guest runs).
Thank you.
>>> + } else {
>>> + if (!slot->npages)
>>> + return 0;
>>> +
>>> + hva = 0;
>>> + }
>>> +
>>> + old = *slot;
>>
>> (Assignment could be in the 'else' == !size branch, GCC would have fun.)
>
> It would have fun _and_ warn, which is why it's not in the else branch. :)
I wondered if its "used uninitialized" analyzer got any better :)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] KVM: x86: map/unmap private slots in __x86_set_memory_region
On 13/10/2015 17:39, Radim Krčmář wrote:
> 2015-10-12 14:09+0200, Paolo Bonzini:
>> Otherwise, two copies (one of them never used and thus bogus) are
>> allocated for the regular and SMM address spaces. This breaks
>> SMM with EPT but without unrestricted guest support, because the
>> SMM copy of the identity page map is all zeros.
>
> (Have you found out why EPT+unrestricted didn't use the alternative SMM
> mapping as well?)
Yes, that I already knew; EPT+unrestricted uses CR0.PG=0 directly so
it doesn't use the identity page at all. (CR0.PG=0 w/o unrestricted
instead runs with CR0.PG=1. CR3 load and store exits are enabled,
and the guest CR3 always points to the identity page map while the
guest runs).
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> @@ -7717,23 +7717,53 @@ void kvm_arch_sync_events(struct kvm *kvm)
>> int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
>> {
>> int i, r;
>> +u64 hva;
>> +struct kvm_memslots *slots = kvm_memslots(kvm);
>> +struct kvm_memory_slot *slot, old;
> | [...]
>> +slot = &slots->memslots[slots->id_to_index[id]];
>
> This seems better written as
>
> slot = id_to_memslot(slots, id);
Gah, I could not recall the right API! I'll fix it.
> (Made me remember that I want to refactor the memslot API ...)
>
> | [...]
>> +} else {
>> +if (!slot->npages)
>> +return 0;
>> +
>> +hva = 0;
>> +}
>> +
>> +old = *slot;
>
> (Assignment could be in the 'else' == !size branch, GCC would have fun.)
It would have fun _and_ warn, which is why it's not in the else branch. :)
Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] KVM: x86: map/unmap private slots in __x86_set_memory_region
2015-10-12 14:09+0200, Paolo Bonzini:
> Otherwise, two copies (one of them never used and thus bogus) are
> allocated for the regular and SMM address spaces. This breaks
> SMM with EPT but without unrestricted guest support, because the
> SMM copy of the identity page map is all zeros.
(Have you found out why EPT+unrestricted didn't use the alternative SMM
mapping as well?)
> By moving the allocation to the caller we also remove the last
> vestiges of kernel-allocated memory regions (not accessible anymore
> in userspace since commit b74a07beed0e, "KVM: Remove kernel-allocated
> memory regions", 2010-06-21); that is a nice bonus.
>
> Reported-by: Alexandre DERUMIER
> Cc: sta...@vger.kernel.org
> Fixes: 9da0e4d5ac969909f6b435ce28ea28135a9cbd69
> Signed-off-by: Paolo Bonzini
> ---
vm_mmap() leaks if __kvm_set_memory_region() fails. It's nothing new
and following process termination should take care of it,
Reviewed-by: Radim Krčmář
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> @@ -7717,23 +7717,53 @@ void kvm_arch_sync_events(struct kvm *kvm)
> int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
> {
> int i, r;
> + u64 hva;
> + struct kvm_memslots *slots = kvm_memslots(kvm);
> + struct kvm_memory_slot *slot, old;
| [...]
> + slot = &slots->memslots[slots->id_to_index[id]];
This seems better written as
slot = id_to_memslot(slots, id);
(Made me remember that I want to refactor the memslot API ...)
| [...]
> + } else {
> + if (!slot->npages)
> + return 0;
> +
> + hva = 0;
> + }
> +
> + old = *slot;
(Assignment could be in the 'else' == !size branch, GCC would have fun.)
| [...]
> + if (!size) {
> + r = vm_munmap(old.userspace_addr, old.npages * PAGE_SIZE);
> + WARN_ON(r < 0);
> + }
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] KVM: x86: map/unmap private slots in __x86_set_memory_region
Otherwise, two copies (one of them never used and thus bogus) are
allocated for the regular and SMM address spaces. This breaks
SMM with EPT but without unrestricted guest support, because the
SMM copy of the identity page map is all zeros.
By moving the allocation to the caller we also remove the last
vestiges of kernel-allocated memory regions (not accessible anymore
in userspace since commit b74a07beed0e, "KVM: Remove kernel-allocated
memory regions", 2010-06-21); that is a nice bonus.
Reported-by: Alexandre DERUMIER
Cc: sta...@vger.kernel.org
Fixes: 9da0e4d5ac969909f6b435ce28ea28135a9cbd69
Signed-off-by: Paolo Bonzini
---
arch/x86/kvm/x86.c | 62 ++
1 file changed, 30 insertions(+), 32 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a3a4cf900e0c..ab59eccb9e78 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7717,23 +7717,53 @@ void kvm_arch_sync_events(struct kvm *kvm)
int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
{
int i, r;
+ u64 hva;
+ struct kvm_memslots *slots = kvm_memslots(kvm);
+ struct kvm_memory_slot *slot, old;
/* Called with kvm->slots_lock held. */
if (WARN_ON(id >= KVM_MEM_SLOTS_NUM))
return -EINVAL;
+ slot = &slots->memslots[slots->id_to_index[id]];
+ if (size) {
+ if (WARN_ON(slot->npages))
+ return -EEXIST;
+
+ /*
+* MAP_SHARED to prevent internal slot pages from being moved
+* by fork()/COW.
+*/
+ hva = vm_mmap(NULL, 0, size, PROT_READ | PROT_WRITE,
+ MAP_SHARED | MAP_ANONYMOUS, 0);
+ if (IS_ERR((void *)hva))
+ return PTR_ERR((void *)hva);
+ } else {
+ if (!slot->npages)
+ return 0;
+
+ hva = 0;
+ }
+
+ old = *slot;
for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
struct kvm_userspace_memory_region m;
m.slot = id | (i << 16);
m.flags = 0;
m.guest_phys_addr = gpa;
+ m.userspace_addr = hva;
m.memory_size = size;
r = __kvm_set_memory_region(kvm, &m);
if (r < 0)
return r;
}
+ if (!size) {
+ r = vm_munmap(old.userspace_addr, old.npages * PAGE_SIZE);
+ WARN_ON(r < 0);
+ }
+
return 0;
}
EXPORT_SYMBOL_GPL(__x86_set_memory_region);
@@ -7863,27 +7893,6 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
const struct kvm_userspace_memory_region *mem,
enum kvm_mr_change change)
{
- /*
-* Only private memory slots need to be mapped here since
-* KVM_SET_MEMORY_REGION ioctl is no longer supported.
-*/
- if ((memslot->id >= KVM_USER_MEM_SLOTS) && (change == KVM_MR_CREATE)) {
- unsigned long userspace_addr;
-
- /*
-* MAP_SHARED to prevent internal slot pages from being moved
-* by fork()/COW.
-*/
- userspace_addr = vm_mmap(NULL, 0, memslot->npages * PAGE_SIZE,
-PROT_READ | PROT_WRITE,
-MAP_SHARED | MAP_ANONYMOUS, 0);
-
- if (IS_ERR((void *)userspace_addr))
- return PTR_ERR((void *)userspace_addr);
-
- memslot->userspace_addr = userspace_addr;
- }
-
return 0;
}
@@ -7945,17 +7954,6 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
{
int nr_mmu_pages = 0;
- if (change == KVM_MR_DELETE && old->id >= KVM_USER_MEM_SLOTS) {
- int ret;
-
- ret = vm_munmap(old->userspace_addr,
- old->npages * PAGE_SIZE);
- if (ret < 0)
- printk(KERN_WARNING
- "kvm_vm_ioctl_set_memory_region: "
- "failed to munmap memory\n");
- }
-
if (!kvm->arch.n_requested_mmu_pages)
nr_mmu_pages = kvm_mmu_calculate_mmu_pages(kvm);
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
