From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [RFC PATCH] KVM: Fix __set_bit() race in mark_page_dirty() during dirty logging Date: Thu, 5 Jan 2012 09:48:37 -0200 Message-ID: <20120105114837.GB1901@amt.cnet> References: <20120104150643.4395fe54.yoshikawa.takuya@oss.ntt.co.jp> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: avi@redhat.com, kvm@vger.kernel.org, takuya.yoshikawa@gmail.com To: Takuya Yoshikawa Return-path: Received: from mx1.redhat.com ([209.132.183.28]:54876 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755415Ab2AEMYH (ORCPT ); Thu, 5 Jan 2012 07:24:07 -0500 Content-Disposition: inline In-Reply-To: <20120104150643.4395fe54.yoshikawa.takuya@oss.ntt.co.jp> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Jan 04, 2012 at 03:06:43PM +0900, Takuya Yoshikawa wrote: > It is possible that the __set_bit() in mark_page_dirty() is called > simultaneously on the same region of memory, which may result in only > one bit being set, because some callers do not take mmu_lock before > mark_page_dirty(). > > This problem is hard to produce because when we reach mark_page_dirty() > beginning from, e.g., tdp_page_fault(), mmu_lock is being held during > __direct_map(): making kvm-unit-tests' dirty log api test write to two > pages concurrently was not useful for this reason. > > So we have confirmed that there can actually be race condition by > checking if some callers really reach there without holding mmu_lock > using spin_is_locked(): probably they were from kvm_write_guest_page(). > > To fix this race, this patch changes the bit operation to the atomic > version: note that nr_dirty_pages also suffers from the race but we do > not need exactly correct numbers for now. > > Signed-off-by: Takuya Yoshikawa > --- > virt/kvm/kvm_main.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 7287bf5..a91f980 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -1543,7 +1543,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm, struct kvm_memory_slot *memslot, > if (memslot && memslot->dirty_bitmap) { > unsigned long rel_gfn = gfn - memslot->base_gfn; > > - if (!__test_and_set_bit_le(rel_gfn, memslot->dirty_bitmap)) > + if (!test_and_set_bit_le(rel_gfn, memslot->dirty_bitmap)) > memslot->nr_dirty_pages++; > } > } Looks good to me.