Fix failures caused by fork() interaction with internal slots
cover Fix failures caused by fork() interaction with internal slots
fork() has a WONTFIX bug where a page with an elevated reference count will be COWed such that the page address changes even in the process which has taken the reference. This interacts badly with internal memory slots that install pages in vmcs registers, such as the APIC access page.
This patchset disables fork() for these slots.
Avi Kivity (2):
KVM: Keep slot ID in memory slot structure
KVM: Prevent internal slots from being COWed
arch/x86/kvm/x86.c | 5 +++++ include/linux/kvm_host.h | 1 + virt/kvm/kvm_main.c | 1 + 3 files changed, 7 insertions(+), 0 deletions(-)
PATCH 1/2 KVM: Keep slot ID in memory slot structure
May be used for distinguishing between internal and user slots, or for sorting slots in size order.
--- include/linux/kvm_host.h | 1 + virt/kvm/kvm_main.c | 1 + 2 files changed, 2 insertions(+), 0 deletions(-)
include/linux/kvm_host.h +1 -0
@@ -124,6 +124,7 @@ struct kvm_memory_slot { } *lpage_info[KVM_NR_PAGE_SIZES -1 ];unsigned long userspace_addr;int user_alloc; +int id; };static inline unsigned long kvm_dirty_bitmap_bytes (struct kvm_memory_slot *memslot)
virt/kvm/kvm_main.c +1 -0
@@ -570,6 +570,7 @@ int __kvm_set_memory_region(struct kvm *kvm, new = old = *memslot; + new.id = mem->slot; new.base_gfn = base_gfn; new.npages = npages; new.flags = mem->flags;
Signature · 1 line
1.7.1
PATCH 2/2 KVM: Prevent internal slots from being COWed
In reply to Avi KivityIf a process with a memory slot is COWed, the page will change its address (despite having an elevated reference count). This breaks internal memory slots which have their physical addresses loaded into vmcs registers (see the APIC access memory slot).
--- arch/x86/kvm/x86.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
arch/x86/kvm/x86.c +5 -0
@@ -5633,6 +5633,11 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, int user_alloc) {int npages = memslot->npages; +int map_flags =MAP_PRIVATE |MAP_ANONYMOUS ; + +/* Prevent internal slot pages from being moved by fork()/COW. */ +if (memslot->id >=KVM_MEMORY_SLOTS ) + map_flags =MAP_SHARED |MAP_ANONYMOUS ;/*To keep backward compatibility with older userspace, *x86 needs to hanlde !user_alloc case.
Signature · 1 line
1.7.1
Re: [PATCH 2/2] KVM: Prevent internal slots from being COWed
Quoted 16 lines · Avi Kivity
If a process with a memory slot is COWed, the page will change its address (despite having an elevated reference count). This breaks internal memory slots which have their physical addresses loaded into vmcs registers (see the APIC access memory slot).
Signed-off-by Avi Kivity <[email protected]>--- arch/x86/kvm/x86.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)arch/x86/kvm/x86.c +0
-0@@ -5633,6 +5633,11 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, int user_alloc)
arch/x86/kvm/x86.c int kvm_arch_prepare_memory_region(struct kvm *kvm, · line 5643
Quoted 10 lines
arch/x86/kvm/x86.c +5
-0{int npages = memslot->npages; +int map_flags =MAP_PRIVATE |MAP_ANONYMOUS ; + +/* Prevent internal slot pages from being moved by fork()/COW. */ +if (memslot->id >=KVM_MEMORY_SLOTS ) + map_flags =MAP_SHARED |MAP_ANONYMOUS ;/*To keep backward compatibility with older userspace, *x86 needs to hanlde !user_alloc case.
Forgot to use map_flags below.
Re: [PATCH 2/2] KVM: Prevent internal slots from being COWed
Quoted 26 lines · Marcelo Tosatti
Quoted 24 lines · Avi Kivity
If a process with a memory slot is COWed, the page will change its address (despite having an elevated reference count). This breaks internal memory slots which have their physical addresses loaded into vmcs registers (see the APIC access memory slot).
Signed-off-by Avi Kivity<[email protected]>--- arch/x86/kvm/x86.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)arch/x86/kvm/x86.c +5
-0@@ -5633,6 +5633,11 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, int user_alloc) {int npages = memslot->npages; +int map_flags =MAP_PRIVATE |MAP_ANONYMOUS ; + +/* Prevent internal slot pages from being moved by fork()/COW. */ +if (memslot->id>=KVM_MEMORY_SLOTS ) + map_flags =MAP_SHARED |MAP_ANONYMOUS ;
Quoted 6 lines
arch/x86/kvm/x86.c int kvm_arch_prepare_memory_region(struct kvm *kvm, · line 5643
Quoted 3 lines
arch/x86/kvm/x86.c +0
-0/*To keep backward compatibility with older userspace, *x86 needs to hanlde !user_alloc case. Forgot to use
map_flagsbelow.
Ouch, corrected and applied.
Signature · 1 line
error compiling committee.c: too many arguments to function
Re: [PATCH 2/2] KVM: Prevent internal slots from being COWed
Quoted 27 lines · Avi Kivity
Quoted 26 lines · Marcelo Tosatti
Quoted 24 lines · Avi Kivity
If a process with a memory slot is COWed, the page will change its address (despite having an elevated reference count). This breaks internal memory slots which have their physical addresses loaded into vmcs registers (see the APIC access memory slot).
Signed-off-by Avi Kivity<[email protected]>--- arch/x86/kvm/x86.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)arch/x86/kvm/x86.c +5
-0@@ -5633,6 +5633,11 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, int user_alloc) {int npages = memslot->npages; +int map_flags =MAP_PRIVATE |MAP_ANONYMOUS ; + +/* Prevent internal slot pages from being moved by fork()/COW. */ +if (memslot->id>=KVM_MEMORY_SLOTS ) + map_flags =MAP_SHARED |MAP_ANONYMOUS ;
Quoted 8 lines
Quoted 6 lines
arch/x86/kvm/x86.c int kvm_arch_prepare_memory_region(struct kvm *kvm, · line 5643
Quoted 3 lines
arch/x86/kvm/x86.c +0
-0/*To keep backward compatibility with older userspace, *x86 needs to hanlde !user_alloc case. Forgot to use
map_flagsbelow.Ouch, corrected and applied.
I think I tracked down the corruption during swapping with THP enabled to this bug. The real bug is that the mmu notifier fires (it's not like fork isn't covered by the mmu notifier) but KVM ignores it and keeps writing to the old location. Shared pages can also be swapped out and if the dirty bit on the spte isn't set faster than the time it takes to write the page, the page can be relocated. Basically if do_swap_page decides to make a copy of the page (like in ksm-swapin case, erratically triggered now even for non-ksm pages in current upstream by a bug in the new anon-vma code which I fixed already in aa.git) and the dirty bit on the spte is ignored because of lumpy reclaim (which also I removed now and that makes the bug stop triggering too), eventually what happens is that the page is unmapped and during swapin it is relocated to a different page.
The bug really is in KVM that ignores the mmu_notifier_invalidate_page and keeps using the old page.
It should have rang a bell that fork was breaking anything... fork must not break anything since KVM is mmu notifier
capable. MADV_DONTFORK must only be a performance optimization now. And the above change should be unnecessary (and I doubt the above really fixes the swapping case as tmpfs can also be swapped out, at least unless the page is pinned).
The way I'd like to fix it is to allocate those magic pages by hand and not add them to lru and have page->mapping null. Then they will remain pinned in the pte, and all problems will go away.
The other way would be to have a lookup hashtable that when mmu notifier invalidate fires, we lookup the hash and we call a method to have kvm stop using the page. And then something is needed during the page fault, if the gfn in the hash is paged-in another method is called to set the magic host user address to point the new pfn.
I think pinning the pages and allocating them by hand is simpler, hopefully we can do it in a way that munmap will collect them automatically like now.
Re: [PATCH 2/2] KVM: Prevent internal slots from being COWed
Quoted 17 lines · Andrea Arcangeli
Quoted 3 lines
Ouch, corrected and applied.
I think I tracked down the corruption during swapping with THP enabled to this bug. The real bug is that the mmu notifier fires (it's not like fork isn't covered by the mmu notifier) but KVM ignores it and keeps writing to the old location. Shared pages can also be swapped out and if the dirty bit on the spte isn't set faster than the time it takes to write the page, the page can be relocated. Basically if
do_swap_pagedecides to make a copy of the page (like in ksm-swapin case, erratically triggered now even for non-ksm pages in current upstream by a bug in the new anon-vma code which I fixed already in aa.git) and the dirty bit on the spte is ignored because of lumpy reclaim (which also I removed now and that makes the bug stop triggering too), eventually what happens is that the page is unmapped and during swapin it is relocated to a different page.
Quoted 3 lines
The bug really is in KVM that ignores the
mmu_notifier_invalidate_pageand keeps using the old page.
Right. It's the same problem as O_DIRECT + fork() - kvm did a get_user_pages() long ago on this page, it got forked, and on the next write the mm duplicated the page and assigned qemu the new page, which kvm ignored.
Quoted 7 lines
It should have rang a bell that fork was breaking anything... fork must not break anything since KVM is mmu notifier
capable.MADV_DONTFORKmust only be a performance optimization now. And the above change should be unnecessary (and I doubt the above really fixes the swapping case as tmpfs can also be swapped out, at least unless the page is pinned).
That particular page is pinned.
Quoted 4 lines
The way I'd like to fix it is to allocate those magic pages by hand and not add them to lru and have
page->mappingnull. Then they will remain pinned in the pte, and all problems will go away.
Yes, that's the correct solution. It shouldn't be a user page in the first place. Problem is that this is a very intrusive change.
Quoted 10 lines
The other way would be to have a lookup hashtable that when mmu notifier invalidate fires, we lookup the hash and we call a method to have kvm stop using the page. And then something is needed during the page fault, if the gfn in the hash is paged-in another method is called to set the magic host user address to point the new pfn.
I think pinning the pages and allocating them by hand is simpler, hopefully we can do it in a way that munmap will collect them automatically like now.
I'd like to remove it completely from the memslot mechanism, unfortunately it may affect many paths.
Signature · 1 line
error compiling committee.c: too many arguments to function