From patchwork Tue Apr 16 18:08:35 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Robin Holt X-Patchwork-Id: 2450431 Return-Path: X-Original-To: patchwork-kvm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id 532C13FD8C for ; Tue, 16 Apr 2013 18:09:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753090Ab3DPSIi (ORCPT ); Tue, 16 Apr 2013 14:08:38 -0400 Received: from relay3.sgi.com ([192.48.152.1]:43378 "EHLO relay.sgi.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751493Ab3DPSIh (ORCPT ); Tue, 16 Apr 2013 14:08:37 -0400 Received: from estes.americas.sgi.com (estes.americas.sgi.com [128.162.236.10]) by relay3.corp.sgi.com (Postfix) with ESMTP id 2CA98AC001; Tue, 16 Apr 2013 11:08:36 -0700 (PDT) Received: from lnx-holt.americas.sgi.com (lnx-holt.americas.sgi.com [128.162.233.109]) by estes.americas.sgi.com (Postfix) with ESMTP id EB1A77002B63; Tue, 16 Apr 2013 13:08:35 -0500 (CDT) Received: from holt by lnx-holt.americas.sgi.com with local (Exim 4.71) (envelope-from ) id 1USAJ1-0007ly-T1; Tue, 16 Apr 2013 13:08:35 -0500 Date: Tue, 16 Apr 2013 13:08:35 -0500 From: Robin Holt To: Xiao Guangrong Cc: Robin Holt , Andrew Morton , Marcelo Tosatti , Gleb Natapov , Avi Kivity , Andrea Arcangeli , LKML , KVM , Linux Memory Management List Subject: Re: [PATCH] mm: mmu_notifier: re-fix freed page still mapped in secondary MMU Message-ID: <20130416180835.GY3658@sgi.com> References: <516CF235.4060103@linux.vnet.ibm.com> <20130416093131.GJ3658@sgi.com> <516D275C.8040406@linux.vnet.ibm.com> <20130416112553.GM3658@sgi.com> <20130416114322.GN3658@sgi.com> <516D4D08.9020602@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <516D4D08.9020602@linux.vnet.ibm.com> User-Agent: Mutt/1.5.20 (2009-06-14) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Tue, Apr 16, 2013 at 09:07:20PM +0800, Xiao Guangrong wrote: > On 04/16/2013 07:43 PM, Robin Holt wrote: > > Argh. Taking a step back helped clear my head. > > > > For the -stable releases, I agree we should just go with your > > revert-plus-hlist_del_init_rcu patch. I will give it a test > > when I am in the office. > > Okay. Wait for your test report. Thank you in advance. > > > > > For the v3.10 release, we should work on making this more > > correct and completely documented. > > Better document is always welcomed. > > Double call ->release is not bad, like i mentioned it in the changelog: > > it is really rare (e.g, can not happen on kvm since mmu-notify is unregistered > after exit_mmap()) and the later call of multiple ->release should be > fast since all the pages have already been released by the first call. > > But, of course, it's great if you have a _light_ way to avoid this. Getting my test environment set back up took longer than I would have liked. Your patch passed. I got no NULL-pointer derefs. How would you feel about adding the following to your patch? --- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h index deca874..ff2fd5f 100644 --- a/include/linux/mmu_notifier.h +++ b/include/linux/mmu_notifier.h @@ -157,6 +157,7 @@ struct mmu_notifier_ops { struct mmu_notifier { struct hlist_node hlist; const struct mmu_notifier_ops *ops; + int released; }; static inline int mm_has_notifiers(struct mm_struct *mm) diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c index 606777a..949704b 100644 --- a/mm/mmu_notifier.c +++ b/mm/mmu_notifier.c @@ -44,7 +44,8 @@ void __mmu_notifier_release(struct mm_struct *mm) * ->release returns. */ id = srcu_read_lock(&srcu); - hlist_for_each_entry_rcu(mn, &mm->mmu_notifier_mm->list, hlist) + hlist_for_each_entry_rcu(mn, &mm->mmu_notifier_mm->list, hlist) { + int released; /* * if ->release runs before mmu_notifier_unregister it * must be handled as it's the only way for the driver @@ -52,8 +53,10 @@ void __mmu_notifier_release(struct mm_struct *mm) * from establishing any more sptes before all the * pages in the mm are freed. */ - if (mn->ops->release) + released = xchg(&mn->released, 1); + if (mn->ops->release && !released) mn->ops->release(mn, mm); + } srcu_read_unlock(&srcu, id); spin_lock(&mm->mmu_notifier_mm->lock); @@ -214,6 +217,7 @@ static int do_mmu_notifier_register(struct mmu_notifier *mn, mm->mmu_notifier_mm = mmu_notifier_mm; mmu_notifier_mm = NULL; } + mn->released = 0; atomic_inc(&mm->mm_count); /* @@ -295,6 +299,7 @@ void mmu_notifier_unregister(struct mmu_notifier *mn, struct mm_struct *mm) * before freeing the pages. */ int id; + int released; id = srcu_read_lock(&srcu); /* @@ -302,7 +307,8 @@ void mmu_notifier_unregister(struct mmu_notifier *mn, struct mm_struct *mm) * guarantee ->release is called before freeing the * pages. */ - if (mn->ops->release) + released = xchg(&mn->released, 1); + if (mn->ops->release && !released) mn->ops->release(mn, mm); srcu_read_unlock(&srcu, id);