[2/3] Revert "mm/rmap.c: reuse mergeable anon_vma as parent when fork"
diff mbox series

Message ID 1581150928-3214-3-git-send-email-lixinhai.lxh@gmail.com
State New
Headers show
Series
  • mm: Fix misuse of parent anon_vma in dup_mmap path
Related show

Commit Message

Li Xinhai Feb. 8, 2020, 8:35 a.m. UTC
This reverts commit 4e4a9eb921332b9d1edd99f76998f99f36b195f7
In dup_mmap(), anon_vma_fork() is called for attaching anon_vma and
parameter 'tmp'  (i.e., the new vma of child) has same ->vm_next and
->vm_prev as its parent vma. That causes the anon_vma used by parent been
mistakenly shared by child (In anon_vma_clone(), the code added by that
commit will do this reuse work).

Besides this issue, the design of reusing anon_vma from vma which has
gone through fork should be avoided ([1]). So, this patch reverts that
commit and maintains the consistent logic of reusing anon_vma for
fork/split/merge vma.

[1] commit d0e9fe1758f2 ("Simplify and comment on anon_vma re-use for
    anon_vma_prepare()") explains the test of "list_is_singular()".

Fixes: 4e4a9eb92133 ("mm/rmap.c: reuse mergeable anon_vma as parent when fork")
Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/rmap.c | 13 -------------
 1 file changed, 13 deletions(-)

Comments

Kirill A. Shutemov April 2, 2020, 1:59 p.m. UTC | #1
On Sat, Feb 08, 2020 at 08:35:27AM +0000, Li Xinhai wrote:
> This reverts commit 4e4a9eb921332b9d1edd99f76998f99f36b195f7
> In dup_mmap(), anon_vma_fork() is called for attaching anon_vma and
> parameter 'tmp'  (i.e., the new vma of child) has same ->vm_next and
> ->vm_prev as its parent vma. That causes the anon_vma used by parent been
> mistakenly shared by child (In anon_vma_clone(), the code added by that
> commit will do this reuse work).
> 
> Besides this issue, the design of reusing anon_vma from vma which has
> gone through fork should be avoided ([1]). So, this patch reverts that
> commit and maintains the consistent logic of reusing anon_vma for
> fork/split/merge vma.
> 
> [1] commit d0e9fe1758f2 ("Simplify and comment on anon_vma re-use for
>     anon_vma_prepare()") explains the test of "list_is_singular()".

I read the description few time, but I cannot say I understood the
problem completely. Do you have a test-case to demonstrate the issue?

IIUC, re-using anon_vma across fork is wrong, but within the process is
fine, right?

Maybe we should just check that dst->vm_mm matches src->vm_mm before
re-using anon_vma?
Li Xinhai April 3, 2020, 3:08 a.m. UTC | #2
On 2020-04-02 at 21:59 Kirill A. Shutemov wrote:
>On Sat, Feb 08, 2020 at 08:35:27AM +0000, Li Xinhai wrote:
>> This reverts commit 4e4a9eb921332b9d1edd99f76998f99f36b195f7
>> In dup_mmap(), anon_vma_fork() is called for attaching anon_vma and
>> parameter 'tmp'  (i.e., the new vma of child) has same ->vm_next and
>> ->vm_prev as its parent vma. That causes the anon_vma used by parent been
>> mistakenly shared by child (In anon_vma_clone(), the code added by that
>> commit will do this reuse work).
>>
>> Besides this issue, the design of reusing anon_vma from vma which has
>> gone through fork should be avoided ([1]). So, this patch reverts that
>> commit and maintains the consistent logic of reusing anon_vma for
>> fork/split/merge vma.
>>
>> [1] commit d0e9fe1758f2 ("Simplify and comment on anon_vma re-use for
>>     anon_vma_prepare()") explains the test of "list_is_singular()".
>
>I read the description few time, but I cannot say I understood the
>problem completely. Do you have a test-case to demonstrate the issue?
>
>IIUC, re-using anon_vma across fork is wrong, but within the process is
>fine, right?
> 

Yes, re-using anon_vma within the process is fine. But if a vma has gone through
fork(), then that vma's anon_vma should not be shared with its neighbor vma.
As explained in [1], when vma gone through fork(), the check for 
list_is_singular(vma->anon_vma_chain) will be false, and don't share anon_vma.

With current issue, one example can clarify more:
parent process do below two steps
1. p_vma_1 is created and p_anon_vma_1 is prepared;
2. p_vma_2 is created and share p_anon_vma_1; (this is allowed, becaues p_vma_1
didn't go through fork());
parent process do fork():
3. c_vma_1 is dup from p_vma_1, and has its own c_anon_vma_1 prepared; at this point,
c_vma_1->anon_vma_chain has two items, one for p_anon_vma_1 and one for
c_anon_vma_1;
4. c_vma_2 is dup from p_vma_2, it is not allowed to share c_anon_vma_1, because 
c_vma_1->anon_vma_chain has two items. 

>Maybe we should just check that dst->vm_mm matches src->vm_mm before
>re-using anon_vma? 

This don't help, iin fork() path, dst->vm_mm always don't match src->vm_mm. 

>
>--
> Kirill A. Shutemov

Patch
diff mbox series

diff --git a/mm/rmap.c b/mm/rmap.c
index b3e3819..861435b 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;