[PATCH for 3.3] KVM: Fix write protection race during dirty logging

[PATCH for 3.3] KVM: Fix write protection race during dirty logging

[Takuya Yoshikawa]

From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>

This patch fixes a race introduced by:

  commit 95d4c16ce78cb6b7549a09159c409d52ddd18dae
  KVM: Optimize dirty logging by rmap_write_protect()

During protecting pages for dirty logging, other threads may also try
to protect a page in mmu_sync_children() or kvm_mmu_get_page().

In such a case, because get_dirty_log releases mmu_lock before flushing
TLB's, the following race condition can happen:

  A (get_dirty_log)     B (another thread)

  lock(mmu_lock)
  clear pte.w
  unlock(mmu_lock)
                        lock(mmu_lock)
                        pte.w is already cleared
                        unlock(mmu_lock)
                        skip TLB flush
                        return
  ...
  TLB flush

Though thread B assumes the page has already been protected when it
returns, the remaining TLB entry will break that assumption.

This patch fixes this problem by making get_dirty_log hold the mmu_lock
until it flushes the TLB's.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/x86/kvm/x86.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2bd77a3..048fe6c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3022,6 +3022,8 @@ static void write_protect_slot(struct kvm *kvm,
 			       unsigned long *dirty_bitmap,
 			       unsigned long nr_dirty_pages)
 {
+	spin_lock(&kvm->mmu_lock);
+
 	/* Not many dirty pages compared to # of shadow pages. */
 	if (nr_dirty_pages < kvm->arch.n_used_mmu_pages) {
 		unsigned long gfn_offset;
@@ -3029,16 +3031,13 @@ static void write_protect_slot(struct kvm *kvm,
 		for_each_set_bit(gfn_offset, dirty_bitmap, memslot->npages) {
 			unsigned long gfn = memslot->base_gfn + gfn_offset;
 
-			spin_lock(&kvm->mmu_lock);
 			kvm_mmu_rmap_write_protect(kvm, gfn, memslot);
-			spin_unlock(&kvm->mmu_lock);
 		}
 		kvm_flush_remote_tlbs(kvm);
-	} else {
-		spin_lock(&kvm->mmu_lock);
+	} else
 		kvm_mmu_slot_remove_write_access(kvm, memslot->id);
-		spin_unlock(&kvm->mmu_lock);
-	}
+
+	spin_unlock(&kvm->mmu_lock);
 }
 
 /*
-- 
1.7.5.4

Re: [PATCH for 3.3] KVM: Fix write protection race during dirty logging

[Xiao Guangrong]

On 02/05/2012 07:42 PM, Takuya Yoshikawa wrote:

> From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> 
> This patch fixes a race introduced by:
> 
>   commit 95d4c16ce78cb6b7549a09159c409d52ddd18dae
>   KVM: Optimize dirty logging by rmap_write_protect()
> 
> During protecting pages for dirty logging, other threads may also try
> to protect a page in mmu_sync_children() or kvm_mmu_get_page().
> 
> In such a case, because get_dirty_log releases mmu_lock before flushing
> TLB's, the following race condition can happen:
> 
>   A (get_dirty_log)     B (another thread)
> 
>   lock(mmu_lock)
>   clear pte.w
>   unlock(mmu_lock)
>                         lock(mmu_lock)
>                         pte.w is already cleared
>                         unlock(mmu_lock)
>                         skip TLB flush
>                         return
>   ...
>   TLB flush
> 
> Though thread B assumes the page has already been protected when it
> returns, the remaining TLB entry will break that assumption.
> 
> This patch fixes this problem by making get_dirty_log hold the mmu_lock
> until it flushes the TLB's.
> 


I do not think this is a problem since the dirty page is logged when
the writeable spte is being set, and in the end of get_dirty_log, all
TLBs are always flushed.

Re: [PATCH for 3.3] KVM: Fix write protection race during dirty logging

[Takuya Yoshikawa]

(2012/02/06 12:40), Xiao Guangrong wrote:
> On 02/05/2012 07:42 PM, Takuya Yoshikawa wrote:
>
>> From: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
>>
>> This patch fixes a race introduced by:
>>
>>    commit 95d4c16ce78cb6b7549a09159c409d52ddd18dae
>>    KVM: Optimize dirty logging by rmap_write_protect()
>>
>> During protecting pages for dirty logging, other threads may also try
>> to protect a page in mmu_sync_children() or kvm_mmu_get_page().
>>
>> In such a case, because get_dirty_log releases mmu_lock before flushing
>> TLB's, the following race condition can happen:
>>
>>    A (get_dirty_log)     B (another thread)
>>
>>    lock(mmu_lock)
>>    clear pte.w
>>    unlock(mmu_lock)
>>                          lock(mmu_lock)
>>                          pte.w is already cleared
>>                          unlock(mmu_lock)
>>                          skip TLB flush
>>                          return
>>    ...
>>    TLB flush
>>
>> Though thread B assumes the page has already been protected when it
>> returns, the remaining TLB entry will break that assumption.
>>
>> This patch fixes this problem by making get_dirty_log hold the mmu_lock
>> until it flushes the TLB's.
>>
>
>
> I do not think this is a problem since the dirty page is logged when
> the writeable spte is being set, and in the end of get_dirty_log, all
> TLBs are always flushed.
>

The victim is not GET_DIRTY_LOG but thread B;  it needs to assure the page
is protected before returning.

Thanks,
	Takuya

Re: [PATCH for 3.3] KVM: Fix write protection race during dirty logging

[Xiao Guangrong]

On 02/06/2012 11:46 AM, Takuya Yoshikawa wrote:

> (2012/02/06 12:40), Xiao Guangrong wrote:
>> On 02/05/2012 07:42 PM, Takuya Yoshikawa wrote:
>>
>>> From: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
>>>
>>> This patch fixes a race introduced by:
>>>
>>>    commit 95d4c16ce78cb6b7549a09159c409d52ddd18dae
>>>    KVM: Optimize dirty logging by rmap_write_protect()
>>>
>>> During protecting pages for dirty logging, other threads may also try
>>> to protect a page in mmu_sync_children() or kvm_mmu_get_page().
>>>
>>> In such a case, because get_dirty_log releases mmu_lock before flushing
>>> TLB's, the following race condition can happen:
>>>
>>>    A (get_dirty_log)     B (another thread)
>>>
>>>    lock(mmu_lock)
>>>    clear pte.w
>>>    unlock(mmu_lock)
>>>                          lock(mmu_lock)
>>>                          pte.w is already cleared
>>>                          unlock(mmu_lock)
>>>                          skip TLB flush
>>>                          return
>>>    ...
>>>    TLB flush
>>>
>>> Though thread B assumes the page has already been protected when it
>>> returns, the remaining TLB entry will break that assumption.
>>>
>>> This patch fixes this problem by making get_dirty_log hold the mmu_lock
>>> until it flushes the TLB's.
>>>
>>
>>
>> I do not think this is a problem since the dirty page is logged when
>> the writeable spte is being set, and in the end of get_dirty_log, all
>> TLBs are always flushed.
>>
> 
> The victim is not GET_DIRTY_LOG but thread B;  it needs to assure the page
> is protected before returning.
> 


Ah, right!

Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>

Re: [PATCH for 3.3] KVM: Fix write protection race during dirty logging

[Xiao Guangrong]

On 02/06/2012 11:53 AM, Xiao Guangrong wrote:

> On 02/06/2012 11:46 AM, Takuya Yoshikawa wrote:
> 
>> (2012/02/06 12:40), Xiao Guangrong wrote:
>>> On 02/05/2012 07:42 PM, Takuya Yoshikawa wrote:
>>>
>>>> From: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
>>>>
>>>> This patch fixes a race introduced by:
>>>>
>>>>    commit 95d4c16ce78cb6b7549a09159c409d52ddd18dae
>>>>    KVM: Optimize dirty logging by rmap_write_protect()
>>>>
>>>> During protecting pages for dirty logging, other threads may also try
>>>> to protect a page in mmu_sync_children() or kvm_mmu_get_page().
>>>>
>>>> In such a case, because get_dirty_log releases mmu_lock before flushing
>>>> TLB's, the following race condition can happen:
>>>>
>>>>    A (get_dirty_log)     B (another thread)
>>>>
>>>>    lock(mmu_lock)
>>>>    clear pte.w
>>>>    unlock(mmu_lock)
>>>>                          lock(mmu_lock)
>>>>                          pte.w is already cleared
>>>>                          unlock(mmu_lock)
>>>>                          skip TLB flush
>>>>                          return
>>>>    ...
>>>>    TLB flush
>>>>
>>>> Though thread B assumes the page has already been protected when it
>>>> returns, the remaining TLB entry will break that assumption.
>>>>
>>>> This patch fixes this problem by making get_dirty_log hold the mmu_lock
>>>> until it flushes the TLB's.
>>>>
>>>
>>>
>>> I do not think this is a problem since the dirty page is logged when
>>> the writeable spte is being set, and in the end of get_dirty_log, all
>>> TLBs are always flushed.
>>>
>>
>> The victim is not GET_DIRTY_LOG but thread B;  it needs to assure the page
>> is protected before returning.
>>
> 
> 
> Ah, right!
> 
> Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
> 


I am worrying if other paths(such as like kvm_mmu_notifier_invalidate_page)
which unmap/modify writeable ptes and flush TLBs out of mmu lock have the
same problem?

Re: [PATCH for 3.3] KVM: Fix write protection race during dirty logging

[Takuya Yoshikawa]

(2012/02/06 14:02), Xiao Guangrong wrote:

> I am worrying if other paths(such as like kvm_mmu_notifier_invalidate_page)
> which unmap/modify writeable ptes and flush TLBs out of mmu lock have the
> same problem?
>

Yes, I think so ... (long standing bug?)

Let's ask Avi if he prefers to fix it separately then!

	Takuya

Re: [PATCH for 3.3] KVM: Fix write protection race during dirty logging

[Avi Kivity]

On 02/06/2012 07:12 AM, Takuya Yoshikawa wrote:
> (2012/02/06 14:02), Xiao Guangrong wrote:
>
>> I am worrying if other paths(such as like
>> kvm_mmu_notifier_invalidate_page)
>> which unmap/modify writeable ptes and flush TLBs out of mmu lock have
>> the
>> same problem?
>>
>
> Yes, I think so ... (long standing bug?)

Ugh, I looked at arch/x86/kvm for this problem but not at virt/kvm.

>
> Let's ask Avi if he prefers to fix it separately then!

Separate fixes please.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH for 3.3] KVM: Fix write protection race during dirty logging

[Marcelo Tosatti]

On Sun, Feb 05, 2012 at 08:42:41PM +0900, Takuya Yoshikawa wrote:
> From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> 
> This patch fixes a race introduced by:
> 
>   commit 95d4c16ce78cb6b7549a09159c409d52ddd18dae
>   KVM: Optimize dirty logging by rmap_write_protect()
> 
> During protecting pages for dirty logging, other threads may also try
> to protect a page in mmu_sync_children() or kvm_mmu_get_page().
> 
> In such a case, because get_dirty_log releases mmu_lock before flushing
> TLB's, the following race condition can happen:
> 
>   A (get_dirty_log)     B (another thread)
> 
>   lock(mmu_lock)
>   clear pte.w
>   unlock(mmu_lock)
>                         lock(mmu_lock)
>                         pte.w is already cleared
>                         unlock(mmu_lock)
>                         skip TLB flush
>                         return
>   ...
>   TLB flush
> 
> Though thread B assumes the page has already been protected when it
> returns, the remaining TLB entry will break that assumption.
> 
> This patch fixes this problem by making get_dirty_log hold the mmu_lock
> until it flushes the TLB's.
> 
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> ---
>  arch/x86/kvm/x86.c |   11 +++++------
>  1 files changed, 5 insertions(+), 6 deletions(-)

Applied, thanks.

Please don't forget the invalidate_page fix.

Re: [PATCH for 3.3] KVM: Fix write protection race during dirty logging

[Takuya Yoshikawa]

On Wed, 8 Feb 2012 14:38:07 -0200
Marcelo Tosatti <mtosatti@redhat.com> wrote:

> Please don't forget the invalidate_page fix.

I will send in a few days.

	Takuya