kvm: write protect memory after slot swap

RFC PATCH In review Michael S. Tsirkin 4 participants 11 messages Oct 25, 2010 – Nov 25, 2010

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Michael S. Tsirkin

author

2010-10-25 01:28 UTC #0 lore raw

RFC PATCH 1/1 kvm: write protect memory after slot swap

I have observed the following bug trigger:

1. userspace calls GET_DIRTY_LOG
2. kvm_mmu_slot_remove_write_access is called and makes a page ro
3. page fault happens and makes the page writeable fault is logged in the bitmap appropriately
4. kvm_vm_ioctl_get_dirty_log swaps slot pointers

a lot of time passes

5. guest writes into the page
6. userspace calls GET_DIRTY_LOG

At point (5), bitmap is clean and page is writeable, thus, guest modification of memory is not logged and GET_DIRTY_LOG returns an empty bitmap.

The rule is that all pages are either dirty in the current bitmap, or write-protected, which is violated here.

It seems that just moving kvm_mmu_slot_remove_write_access down to after the slot pointer swap should fix this bug.

Warning: completely untested.
Please comment.
Note: fix will be needed for -stable etc.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 arch/x86/kvm/x86.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

arch/x86/kvm/x86.c +4 -4

index 3a09c62..4ca1d7f 100644
@@ -2912,10 +2912,6 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 		struct kvm_memslots *slots, *old_slots;
 		unsigned long *dirty_bitmap;
 
-		spin_lock(&kvm->mmu_lock);
-		kvm_mmu_slot_remove_write_access(kvm, log->slot);
-		spin_unlock(&kvm->mmu_lock);
-
 		r = -ENOMEM;
 		dirty_bitmap = vmalloc(n);
 		if (!dirty_bitmap)

@@ -2937,6 +2933,10 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 		dirty_bitmap = old_slots->memslots[log->slot].dirty_bitmap;
 		kfree(old_slots);
 
+		spin_lock(&kvm->mmu_lock);
+		kvm_mmu_slot_remove_write_access(kvm, log->slot);
+		spin_unlock(&kvm->mmu_lock);
+
 		r = -EFAULT;
 		if (copy_to_user(log->dirty_bitmap, dirty_bitmap, n)) {
 			vfree(dirty_bitmap);

Signature · 1 line

1.7.3-rc1

Takuya Yoshikawa

2010-10-25 07:26 UTC #1 lore raw

Quoted 6 lines · On Mon, 25 Oct 2010 03:21:24 +0200 "Michael S. Tsirkin" <[email protected]> wrote:

I have observed the following bug trigger:
1. userspace calls GET_DIRTY_LOG
2. kvm_mmu_slot_remove_write_access is called and makes a page ro
3. page fault happens and makes the page writeable fault is logged in the bitmap appropriately

This may be the reason why my commit is a corruption magnifier. My patch moved the vmalloc() right after kvm_mmu_slot_remove_write_access() and made this chance bigger:
because vmalloc() takes some time.

Thanks,
Takuya

Quoted 13 lines

4. kvm_vm_ioctl_get_dirty_log swaps slot pointers
a lot of time passes
5. guest writes into the page
6. userspace calls GET_DIRTY_LOG
At point (5), bitmap is clean and page is writeable, thus, guest modification of memory is not logged and GET_DIRTY_LOG returns an empty bitmap.
The rule is that all pages are either dirty in the current bitmap, or write-protected, which is violated here.

Jan Kiszka

2010-10-25 09:07 UTC #2 lore raw

In reply to Michael S. Tsirkin

Quoted 8 lines · Michael S. Tsirkin

I have observed the following bug trigger:
1. userspace calls GET_DIRTY_LOG
2. kvm_mmu_slot_remove_write_access is called and makes a page ro
3. page fault happens and makes the page writeable fault is logged in the bitmap appropriately
4. kvm_vm_ioctl_get_dirty_log swaps slot pointers

Quoted 9 lines

a lot of time passes
5. guest writes into the page
6. userspace calls GET_DIRTY_LOG
At point (5), bitmap is clean and page is writeable, thus, guest modification of memory is not logged and GET_DIRTY_LOG returns an empty bitmap.

Cool, seems to be the key to the corruptions I've seen. Applying your patch make them disappear.

Quoted 9 lines

The rule is that all pages are either dirty in the current bitmap, or write-protected, which is violated here.
It seems that just moving kvm_mmu_slot_remove_write_access down to after the slot pointer swap should fix this bug.
Warning: completely untested.
Please comment.
Note: fix will be needed for -stable etc.

Assuming that a page cannot be write-enabled without having a dirty entry in the old bitmap and due to the fact that user space won't get hold of that old bitmap to read out the page before we reset write access again, your patch should actually be safe.

If no one else sees some remaining race, let's get this applied upstream ASAP and pushed down to the stable trees.

Thanks,
Jan

Quoted 32 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 arch/x86/kvm/x86.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

arch/x86/kvm/x86.c +4 -4

index 3a09c62..4ca1d7f 100644
@@ -2912,10 +2912,6 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 		struct kvm_memslots *slots, *old_slots;
 		unsigned long *dirty_bitmap;
 
-		spin_lock(&kvm->mmu_lock);
-		kvm_mmu_slot_remove_write_access(kvm, log->slot);
-		spin_unlock(&kvm->mmu_lock);
-
 		r = -ENOMEM;
 		dirty_bitmap = vmalloc(n);
 		if (!dirty_bitmap)

@@ -2937,6 +2933,10 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 		dirty_bitmap = old_slots->memslots[log->slot].dirty_bitmap;
 		kfree(old_slots);
 
+		spin_lock(&kvm->mmu_lock);
+		kvm_mmu_slot_remove_write_access(kvm, log->slot);
+		spin_unlock(&kvm->mmu_lock);
+
 		r = -EFAULT;
 		if (copy_to_user(log->dirty_bitmap, dirty_bitmap, n)) {
 			vfree(dirty_bitmap);

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Avi Kivity

2010-10-25 09:32 UTC #5 lore raw

In reply to Michael S. Tsirkin

Quoted 20 lines · Michael S. Tsirkin

I have observed the following bug trigger:
1. userspace calls GET_DIRTY_LOG
2. kvm_mmu_slot_remove_write_access is called and makes a page ro
3. page fault happens and makes the page writeable fault is logged in the bitmap appropriately
4. kvm_vm_ioctl_get_dirty_log swaps slot pointers
a lot of time passes
5. guest writes into the page
6. userspace calls GET_DIRTY_LOG
At point (5), bitmap is clean and page is writeable, thus, guest modification of memory is not logged and GET_DIRTY_LOG returns an empty bitmap.
The rule is that all pages are either dirty in the current bitmap, or write-protected, which is violated here.

Quoted 6 lines

It seems that just moving kvm_mmu_slot_remove_write_access down to after the slot pointer swap should fix this bug.
Warning: completely untested.
Please comment.
Note: fix will be needed for -stable etc.

Excellent catch, I stared at this code for a while and didn't see the bug. Patch applied.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

2010-10-25 11:41 UTC #6 lore raw

Quoted 24 lines · Avi Kivity

Quoted 23 lines · Michael S. Tsirkin
I have observed the following bug trigger:
1. userspace calls GET_DIRTY_LOG
2. kvm_mmu_slot_remove_write_access is called and makes a page ro
3. page fault happens and makes the page writeable fault is logged in the bitmap appropriately
4. kvm_vm_ioctl_get_dirty_log swaps slot pointers
a lot of time passes
5. guest writes into the page
6. userspace calls GET_DIRTY_LOG
At point (5), bitmap is clean and page is writeable, thus, guest modification of memory is not logged and GET_DIRTY_LOG returns an empty bitmap.
The rule is that all pages are either dirty in the current bitmap, or write-protected, which is violated here.
It seems that just moving kvm_mmu_slot_remove_write_access down to after the slot pointer swap should fix this bug.

Quoted 7 lines

Quoted 3 lines
Warning: completely untested.
Please comment.
Note: fix will be needed for -stable etc.
Excellent catch, I stared at this code for a while and didn't see the bug. Patch applied.

BTW, while this was an annoying one for graphic emulation, wasn't it potentially lethal for live migration?

The issue looks like is was introduced with the switch to SRCU, so every kernel since 2.6.34 should be affected, correct?

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Avi Kivity

2010-10-25 11:50 UTC #7 lore raw

Quoted 7 lines · Jan Kiszka

Quoted 4 lines
Excellent catch, I stared at this code for a while and didn't see the bug. Patch applied.
BTW, while this was an annoying one for graphic emulation, wasn't it potentially lethal for live migration?

Deadly. Yes autofs passed it happily.

We need a unit test that bangs on pages and the bitmap with tighter timing.

Quoted 2 lines

The issue looks like is was introduced with the switch to SRCU, so every kernel since 2.6.34 should be affected, correct?

Yes.

Signature · 1 line

error compiling committee.c: too many arguments to function

Avi Kivity

2010-10-25 11:51 UTC #8 lore raw

Quoted 10 lines · Avi Kivity

Quoted 7 lines · Jan Kiszka
Quoted 4 lines
Excellent catch, I stared at this code for a while and didn't see the bug. Patch applied.
BTW, while this was an annoying one for graphic emulation, wasn't it potentially lethal for live migration?
Deadly. Yes autofs passed it happily.

Er, autotest. I'm just having problems my 2.6.36+ autofs setup.

Signature · 1 line

error compiling committee.c: too many arguments to function

Michael S. Tsirkin

author

2010-10-25 12:12 UTC #3 lore raw

In reply to Jan Kiszka

Quoted 2 lines · Jan Kiszka

Cool, seems to be the key to the corruptions I've seen. Applying your patch make them disappear.

Yes, works for me as well.

Signature · 1 line

MST

Takuya Yoshikawa

2010-10-26 06:36 UTC #4 lore raw

Quoted 6 lines · Michael S. Tsirkin

Quoted 2 lines · Jan Kiszka
Cool, seems to be the key to the corruptions I've seen. Applying your patch make them disappear.
Yes, works for me as well.

I did some tests on my laptop:
- kvm.git + mst's patch
- qemu.git (upstream qemu)
and still got graphics curruption.

Corruption:
On usual Desktop environment, I opened two terminals on different workspaces. Then as Jan did, I did "find /" loop on both of them.

During these heavy updates, I tried to switch between these workspaces some times. Then, at some turn, some part of old workspace's images like terminals and mouse cursor remained in the new workspace.

I could refresh these by moving mouse over the problematic parts. But without doing so, the images remained still.

Refresh is not working on virtual machines as I expect?

Thanks,
Takuya

Jan Kiszka

2010-11-24 19:17 UTC #9 lore raw

In reply to Avi Kivity

Quoted 24 lines · Avi Kivity

Quoted 23 lines · Michael S. Tsirkin
I have observed the following bug trigger:
1. userspace calls GET_DIRTY_LOG
2. kvm_mmu_slot_remove_write_access is called and makes a page ro
3. page fault happens and makes the page writeable fault is logged in the bitmap appropriately
4. kvm_vm_ioctl_get_dirty_log swaps slot pointers
a lot of time passes
5. guest writes into the page
6. userspace calls GET_DIRTY_LOG
At point (5), bitmap is clean and page is writeable, thus, guest modification of memory is not logged and GET_DIRTY_LOG returns an empty bitmap.
The rule is that all pages are either dirty in the current bitmap, or write-protected, which is violated here.
It seems that just moving kvm_mmu_slot_remove_write_access down to after the slot pointer swap should fix this bug.

Quoted 7 lines

Quoted 3 lines
Warning: completely untested.
Please comment.
Note: fix will be needed for -stable etc.
Excellent catch, I stared at this code for a while and didn't see the bug. Patch applied.

This patch was marked KVM-stable on commit, but it did not make into any stable branch thus also none of the recent releases. Please fix (for 2.6.36 now).

Thanks,
Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Avi Kivity

2010-11-25 09:19 UTC #10 lore raw

Quoted 8 lines · Jan Kiszka

Quoted 4 lines
Excellent catch, I stared at this code for a while and didn't see the bug. Patch applied.
This patch was marked KVM-stable on commit, but it did not make into any stable branch thus also none of the recent releases. Please fix (for 2.6.36 now).

We need some bot that watches out for commits with the tag going into upstream and pokes the maintainers.

Signature · 1 line

error compiling committee.c: too many arguments to function