From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D369FC32771 for ; Tue, 7 Jan 2020 02:32:22 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id A0C3B20715 for ; Tue, 7 Jan 2020 02:32:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A0C3B20715 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 391A58E0010; Mon, 6 Jan 2020 21:32:22 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 31B1D8E0001; Mon, 6 Jan 2020 21:32:22 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1E2738E0010; Mon, 6 Jan 2020 21:32:22 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0109.hostedemail.com [216.40.44.109]) by kanga.kvack.org (Postfix) with ESMTP id 0605D8E0001 for ; Mon, 6 Jan 2020 21:32:22 -0500 (EST) Received: from smtpin20.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with SMTP id A1A2B180AD801 for ; Tue, 7 Jan 2020 02:32:21 +0000 (UTC) X-FDA: 76349264082.20.lunch32_31a1679bb135e X-HE-Tag: lunch32_31a1679bb135e X-Filterd-Recvd-Size: 6339 Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by imf12.hostedemail.com (Postfix) with ESMTP for ; Tue, 7 Jan 2020 02:32:20 +0000 (UTC) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Jan 2020 18:32:18 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,404,1571727600"; d="scan'208";a="422327324" Received: from richard.sh.intel.com (HELO localhost) ([10.239.159.54]) by fmsmga006.fm.intel.com with ESMTP; 06 Jan 2020 18:32:17 -0800 Date: Tue, 7 Jan 2020 10:32:22 +0800 From: Wei Yang To: Konstantin Khlebnikov Cc: linux-mm@kvack.org, Andrew Morton , linux-kernel@vger.kernel.org, Wei Yang , "Kirill A. Shutemov" Subject: Re: [PATCH] mm/rmap: fix reusing mergeable anon_vma as parent when fork Message-ID: <20200107023221.GC15341@richard> Reply-To: Wei Yang References: <157830736034.8148.7070851958306750616.stgit@buzz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <157830736034.8148.7070851958306750616.stgit@buzz> User-Agent: Mutt/1.9.4 (2018-02-28) X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Mon, Jan 06, 2020 at 01:42:40PM +0300, Konstantin Khlebnikov wrote: >This fixes couple misconceptions in commit 4e4a9eb92133 ("mm/rmap.c: reuse >mergeable anon_vma as parent when fork"). > >First problem caused by initialization order in dup_mmap(): vma->vm_prev >is set after calling anon_vma_fork(). Thus in anon_vma_fork() it points to >previous VMA in parent mm. This is fixed by rearrangement in dup_mmap(). > You are right, I missed this point. >If in parent VMAs: SRC1 SRC2 .. SRCn share anon-vma ANON0, then after fork >before all patches in child process related VMAs: DST1 DST2 .. DSTn will >use different anon-vmas: ANON1 ANON2 .. ANONn. Before this patch only DST1 >will fork new ANON1 and following DST2 .. DSTn will share parent's ANON0. >With this patch DST1 will create new ANON1 and DST2 .. DSTn will share it. > >Also this patch moves sharing logic out of anon_vma_clone() into more >specific anon_vma_fork() because this supposed to work only at fork(). >Function anon_vma_clone() is more generic is also used at splitting VMAs. > >Second problem is hidden behind first one: assumption "Parent has vm_prev, >which implies we have vm_prev" is wrong if first VMA in parent mm has set >flag VM_DONTCOPY. Luckily prev->anon_vma doesn't dereference NULL pointer >because in current code 'prev' actually is same as 'pprev'. To avoid that >this patch just checks pointer and compares vm_start to verify relation >between previous VMAs in parent and child. > Correct here too. >Signed-off-by: Konstantin Khlebnikov >Fixes: 4e4a9eb92133 ("mm/rmap.c: reuse mergeable anon_vma as parent when fork") >--- > kernel/fork.c | 4 ++-- > mm/rmap.c | 25 ++++++++++++------------- > 2 files changed, 14 insertions(+), 15 deletions(-) > >diff --git a/kernel/fork.c b/kernel/fork.c >index 2508a4f238a3..04ee5e243f65 100644 >--- a/kernel/fork.c >+++ b/kernel/fork.c >@@ -548,6 +548,8 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm, > if (retval) > goto fail_nomem_policy; > tmp->vm_mm = mm; >+ tmp->vm_prev = prev; /* anon_vma_fork use this */ >+ tmp->vm_next = NULL; How about pass prev to anon_vma_fork()? So that we limit all the difference in anon_vma_fork(). > retval = dup_userfaultfd(tmp, &uf); > if (retval) > goto fail_nomem_anon_vma_fork; >@@ -559,7 +561,6 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm, > } else if (anon_vma_fork(tmp, mpnt)) > goto fail_nomem_anon_vma_fork; > tmp->vm_flags &= ~(VM_LOCKED | VM_LOCKONFAULT); >- tmp->vm_next = tmp->vm_prev = NULL; > file = tmp->vm_file; > if (file) { > struct inode *inode = file_inode(file); >@@ -592,7 +593,6 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm, > */ > *pprev = tmp; > pprev = &tmp->vm_next; >- tmp->vm_prev = prev; > prev = tmp; > > __vma_link_rb(mm, tmp, rb_link, rb_parent); >diff --git a/mm/rmap.c b/mm/rmap.c >index b3e381919835..77b3aa38d5c2 100644 >--- a/mm/rmap.c >+++ b/mm/rmap.c >@@ -269,19 +269,6 @@ int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src) > { > struct anon_vma_chain *avc, *pavc; > struct anon_vma *root = NULL; >- struct vm_area_struct *prev = dst->vm_prev, *pprev = src->vm_prev; >- >- /* >- * If parent share anon_vma with its vm_prev, keep this sharing in in >- * child. >- * >- * 1. Parent has vm_prev, which implies we have vm_prev. >- * 2. Parent and its vm_prev have the same anon_vma. >- */ >- if (!dst->anon_vma && src->anon_vma && >- pprev && pprev->anon_vma == src->anon_vma) >- dst->anon_vma = prev->anon_vma; >- > > list_for_each_entry_reverse(pavc, &src->anon_vma_chain, same_vma) { > struct anon_vma *anon_vma; >@@ -334,6 +321,7 @@ int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src) > */ > int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma) > { >+ struct vm_area_struct *prev = vma->vm_prev, *pprev = pvma->vm_prev; > struct anon_vma_chain *avc; > struct anon_vma *anon_vma; > int error; >@@ -345,6 +333,17 @@ int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma) > /* Drop inherited anon_vma, we'll reuse existing or allocate new. */ > vma->anon_vma = NULL; > >+ /* >+ * If parent shares anon_vma with its vm_prev, keep this sharing. >+ * >+ * Previous VMA could be missing or not match previuos in parent >+ * if VM_DONTCOPY is set: compare vm_start to avoid this case. >+ */ >+ if (pvma->anon_vma && pprev && prev && >+ pprev->anon_vma == pvma->anon_vma && >+ pprev->vm_start == prev->vm_start) >+ vma->anon_vma = prev->anon_vma; >+ > /* > * First, attach the new VMA to the parent VMA's anon_vmas, > * so rmap can find non-COWed pages in child processes. -- Wei Yang Help you, Help me