From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Paul E. McKenney" Subject: Re: [BUG] kvm: dereference srcu-protected pointer without srcu_read_lock() held Date: Tue, 20 Apr 2010 15:21:08 -0700 Message-ID: <20100420222108.GM2628@linux.vnet.ibm.com> References: <4BCC2543.7050104@cn.fujitsu.com> <4BCC295D.1040807@cn.fujitsu.com> <4BCC2B9D.8050008@redhat.com> <20100420014504.GB17981@amt.cnet> <4BCD49C9.8090604@cn.fujitsu.com> Reply-To: paulmck@linux.vnet.ibm.com Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Marcelo Tosatti , Avi Kivity , LKML , kvm@vger.kernel.org To: Lai Jiangshan Return-path: Content-Disposition: inline In-Reply-To: <4BCD49C9.8090604@cn.fujitsu.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On Tue, Apr 20, 2010 at 02:29:29PM +0800, Lai Jiangshan wrote: > Marcelo Tosatti wrote: > > On Mon, Apr 19, 2010 at 01:08:29PM +0300, Avi Kivity wrote: > >> On 04/19/2010 12:58 PM, Lai Jiangshan wrote: > >>> Applied the patch I just sent and let CONFIG_PROVE_RCU=y, > >>> we can got the following dmesg. And we found that it is > >>> because some codes in KVM dereferences srcu-protected pointer without > >>> srcu_read_lock() held or update-side lock held. > >>> > >>> It is not hard to fix, the problem is that: > >>> Where is the most proper place to put a srcu_read_lock()? > >>> > >>> I can not determine the answer, so I report this bug > >>> instead of fixing it. > >>> > >> I think the else branch in complete_pio() should work. Marcelo? > >> > >> Longer term I'd like to see the lock taken at the high levels > >> (ioctls, in virt/kvm) and dropped only for guest entry and when we > >> explicitly sleep (hlt emulation). > >> > >> Note: complete_pio() is gone in the current code. > > > > Yes, this was fixed by 7fb2ea1e6. > > > > > > Applied the patch I sent yesterday and let CONFIG_PROVE_RCU=y > I can get the following dmesg. > > Under very simple test, these is no complaint from PROVE_RCU > after this patch applied. > > More test or reviewing of code are need in future. > > ---------- > Subject: [PATCH] kvm: add missing srcu_read_lock() > > I got this dmesg due to srcu_read_lock() is missing in > kvm_mmu_notifier_release(). > > =================================================== > [ INFO: suspicious rcu_dereference_check() usage. ] > --------------------------------------------------- > arch/x86/kvm/x86.h:72 invoked rcu_dereference_check() without protection! > > other info that might help us debug this: > > > rcu_scheduler_active = 1, debug_locks = 0 > 2 locks held by qemu-system-x86/3100: > #0: (rcu_read_lock){.+.+..}, at: [] __mmu_notifier_release+0x38/0xdf > #1: (&(&kvm->mmu_lock)->rlock){+.+...}, at: [] kvm_mmu_zap_all+0x21/0x5e [kvm] > > stack backtrace: > Pid: 3100, comm: qemu-system-x86 Not tainted 2.6.34-rc3-22949-gbc8a97a-dirty #2 > Call Trace: > [] lockdep_rcu_dereference+0xaa/0xb3 > [] unalias_gfn+0x56/0xab [kvm] > [] gfn_to_memslot+0x16/0x25 [kvm] > [] gfn_to_rmap+0x17/0x6e [kvm] > [] rmap_remove+0xa0/0x19d [kvm] > [] kvm_mmu_zap_page+0x109/0x34d [kvm] > [] kvm_mmu_zap_all+0x35/0x5e [kvm] > [] kvm_arch_flush_shadow+0x16/0x22 [kvm] > [] kvm_mmu_notifier_release+0x15/0x17 [kvm] > [] __mmu_notifier_release+0x88/0xdf > [] ? __mmu_notifier_release+0x38/0xdf > [] ? exit_mm+0xe0/0x115 > [] exit_mmap+0x2c/0x17e > [] mmput+0x2d/0xd4 > [] exit_mm+0x108/0x115 > [...] Queued, thank you, Lai! Thanx, Paul > Signed-off-by: Lai Jiangshan > --- > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index a5dfea1..a6d639d 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -341,7 +341,11 @@ static void kvm_mmu_notifier_release(struct mmu_notifier *mn, > struct mm_struct *mm) > { > struct kvm *kvm = mmu_notifier_to_kvm(mn); > + int idx; > + > + idx = srcu_read_lock(&kvm->srcu); > kvm_arch_flush_shadow(kvm); > + srcu_read_unlock(&kvm->srcu, idx); > } > > static const struct mmu_notifier_ops kvm_mmu_notifier_ops = {