KVM: fix async page fault working for readonly mapping
PATCH 1/1 KVM: fix async page fault working for readonly mapping
If we map a readonly memory space from host to guest and the page is not currently mapped in the host, we will get a fault-pfn and async is not allowed, then the vm will crash
The reason is only writable vma can be allowed to be async in current code
--- virt/kvm/kvm_main.c | 19 +++++++++++++++++-- 1 files changed, 17 insertions(+), 2 deletions(-)
virt/kvm/kvm_main.c +17 -2
@@ -1052,6 +1052,21 @@ static inline int check_user_page_hwpoison(unsigned long addr)return rc == -EHWPOISON ; } +static bool vma_is_avalid (struct vm_area_struct *vma,bool write_fault) +{ +if (write_fault) { +if (unlikely (!(vma->vm_flags &VM_WRITE ))) +return false; + +return true; + } + +if (unlikely (!(vma->vm_flags & (VM_READ |VM_EXEC |VM_WRITE )))) +return false; + +return true; +} +static pfn_t hva_to_pfn (struct kvm *kvm,unsigned long addr,bool atomic,bool *async,bool write_fault,bool *writable) {
@@ -1075,7 +1090,6 @@ static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic,if (writable) *writable = write_fault; -if (async) {down_read (¤t->mm->mmap_sem); npages = get_user_page_nowait(current, current->mm,
@@ -1122,8 +1136,9 @@ static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic, vma->vm_pgoff;BUG_ON (!kvm_is_mmio_pfn (pfn)); }else { -if (async && (vma->vm_flags &VM_WRITE )) +if (async &&vma_is_avalid (vma, write_fault)) *async = true; + pfn =get_fault_pfn (); }up_read (¤t->mm->mmap_sem);
Signature · 1 line
1.7.7.6
Quoted 4 lines · Xiao Guangrong
If we map a readonly memory space from host to guest and the page is not currently mapped in the host, we will get a fault-pfn and async is not allowed, then the vm will crash
Why would we want to map a readonly memory space from host to guest? We may want to do it to support memory semantics on read and mmio on write, but do not right now unless something changed while I was not looking.
Quoted 61 lines
The reason is only writable vma can be allowed to be async in current code
Signed-off-by Xiao Guangrong <[email protected]>--- virt/kvm/kvm_main.c | 19 +++++++++++++++++-- 1 files changed, 17 insertions(+), 2 deletions(-)virt/kvm/kvm_main.c +17
-2@@ -1052,6 +1052,21 @@ static inline int check_user_page_hwpoison(unsigned long addr)return rc == -EHWPOISON ; } +static bool vma_is_avalid (struct vm_area_struct *vma,bool write_fault) +{ +if (write_fault) { +if (unlikely (!(vma->vm_flags &VM_WRITE ))) +return false; + +return true; + } + +if (unlikely (!(vma->vm_flags & (VM_READ |VM_EXEC |VM_WRITE )))) +return false; + +return true; +} +static pfn_t hva_to_pfn (struct kvm *kvm,unsigned long addr,bool atomic,bool *async,bool write_fault,bool *writable) {@@ -1075,7 +1090,6 @@ static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic,if (writable) *writable = write_fault; -if (async) {down_read (¤t->mm->mmap_sem); npages = get_user_page_nowait(current, current->mm,@@ -1122,8 +1136,9 @@ static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic, vma->vm_pgoff;BUG_ON (!kvm_is_mmio_pfn (pfn)); }else { -if (async && (vma->vm_flags &VM_WRITE )) +if (async &&vma_is_avalid (vma, write_fault)) *async = true; + pfn =get_fault_pfn (); }up_read (¤t->mm->mmap_sem);Signature · 7 lines
1.7.7.6 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
--
Gleb.
Quoted 9 lines · Gleb Natapov
Quoted 4 lines · Xiao Guangrong
If we map a readonly memory space from host to guest and the page is not currently mapped in the host, we will get a fault-pfn and async is not allowed, then the vm will crash
Why would we want to map a readonly memory space from host to guest? We may want to do it to support memory semantics on read and mmio on write, but do not right now unless something changed while I was not looking.
Some test cases in kvm-unit-tests and the benchmark i am writing for KVM need map the function on host to guest.
Quoted 7 lines · Xiao Guangrong
Quoted 5 lines · Gleb Natapov
Quoted 4 lines · Xiao Guangrong
If we map a readonly memory space from host to guest and the page is not currently mapped in the host, we will get a fault-pfn and async is not allowed, then the vm will crash
Quoted 8 lines
Quoted 4 lines
Why would we want to map a readonly memory space from host to guest? We may want to do it to support memory semantics on read and mmio on write, but do not right now unless something changed while I was not looking.
Some test cases in kvm-unit-tests and the benchmark i am writing for KVM need map the function on host to guest.
Or ROM. Or read-only mappings of IVSHMEM (which don't exist yet).
Quoted 14 lines · Marcelo Tosatti
Quoted 13 lines · Xiao Guangrong
Quoted 9 lines · Gleb Natapov
Quoted 4 lines · Xiao Guangrong
If we map a readonly memory space from host to guest and the page is not currently mapped in the host, we will get a fault-pfn and async is not allowed, then the vm will crash
Why would we want to map a readonly memory space from host to guest? We may want to do it to support memory semantics on read and mmio on write, but do not right now unless something changed while I was not looking.
Quoted 4 lines
Quoted 2 lines
Some test cases in kvm-unit-tests and the benchmark i am writing for KVM need map the function on host to guest.
Or ROM. Or read-only mappings of IVSHMEM (which don't exist yet).
True. KVM should ignore writes to such areas, not kill a guest. Is this how the code works today?
--
Gleb.
Quoted 15 lines · Gleb Natapov
Quoted 14 lines · Marcelo Tosatti
Quoted 13 lines · Xiao Guangrong
Quoted 9 lines · Gleb Natapov
Quoted 4 lines · Xiao Guangrong
If we map a readonly memory space from host to guest and the page is not currently mapped in the host, we will get a fault-pfn and async is not allowed, then the vm will crash
Why would we want to map a readonly memory space from host to guest? We may want to do it to support memory semantics on read and mmio on write, but do not right now unless something changed while I was not looking.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Some test cases in kvm-unit-tests and the benchmark i am writing for KVM need map the function on host to guest.
Or ROM. Or read-only mappings of IVSHMEM (which don't exist yet).
True. KVM should ignore writes to such areas, not kill a guest. Is this how the code works today?
Right now qemu maps ROM as RAM. There is no way to tell kvm that something is ROM (or ROMD).
There are two options for that:
- mprotect() the ROM, and teach kvm about read-only areas (this patch); but that doesn't work if we have a read-only and a writable alias of the same area
- add a flag indicating that an area is ROM or ROMD
I prefer the latter, because of the alias issue.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 27 lines · Avi Kivity
Quoted 15 lines · Gleb Natapov
Quoted 14 lines · Marcelo Tosatti
Quoted 13 lines · Xiao Guangrong
Quoted 9 lines · Gleb Natapov
Quoted 4 lines · Xiao Guangrong
If we map a readonly memory space from host to guest and the page is not currently mapped in the host, we will get a fault-pfn and async is not allowed, then the vm will crash
Why would we want to map a readonly memory space from host to guest? We may want to do it to support memory semantics on read and mmio on write, but do not right now unless something changed while I was not looking.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Some test cases in kvm-unit-tests and the benchmark i am writing for KVM need map the function on host to guest.
Or ROM. Or read-only mappings of IVSHMEM (which don't exist yet).
True. KVM should ignore writes to such areas, not kill a guest. Is this how the code works today?
Right now qemu maps ROM as RAM. There is no way to tell kvm that something is ROM (or ROMD).
Quoted 8 lines
There are two options for that:
-mprotect()the ROM, and teach kvm about read-only areas (this patch); but that doesn't work if we have a read-only and a writable alias of the same area
- add a flag indicating that an area is ROM or ROMDI prefer the latter, because of the alias issue.
I agree.
Will post a new patch to do it if other guys do not object it. :)