Re: [PATCH] mm/rmap: fix reusing mergeable anon_vma as parent when fork

From: Konstantin Khlebnikov
Date: Tue Jan 07 2020 - 03:30:03 EST

On 07/01/2020 05.44, lixinhai.lxh@xxxxxxxxx wrote:
On 2020-01-07Âat 04:35ÂKonstantin KhlebnikovÂwrote:
On Mon, Jan 6, 2020 at 1:42 PM Konstantin Khlebnikov
<khlebnikov@xxxxxxxxxxxxxx> 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().

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.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@xxxxxxxxxxxxxx>
Fixes: 4e4a9eb92133 ("mm/rmap.c: reuse mergeable anon_vma as parent when fork")

Oops, I've forgot to mention that Li Xinhai <lixinhai.lxh@xxxxxxxxx>
found and reported this suspicious code. Sorry.

Reported-by: Li Xinhai <lixinhai.lxh@xxxxxxxxx>
Link: https://lore.kernel.org/linux-mm/CALYGNiNzz+dxHX0g5-gNypUQc3B=8_Scp53-NTOh=zWsdUuHAw@xxxxxxxxxxxxxx/T/#t


Can we change the interface
int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma),
to
int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma, struct vm_area_struct *pcvma),
and 'pcvma' means previous child vma.
so highlight the use of that vma, and the current code sequence for linking 'tmp' vma
in dup_mmap() is not changed(in case some code would have dependency on that
linking sequence)

There should be no dependency on linking sequence.
But we could generalize sharing: cloned vma could share prev anon-vma
(or any other actually) if anon_vma->parent == src->anon_vma.
This is more clear than sharing only between related vmas.


Another issue is for linking the avc for the reused anon_vma. anon_vma_clone()
use the iteration
list_for_each_entry_reverse(pavc, &src->anon_vma_chain, same_vma),
to link avc for child vma, and it is unable to reach the resued anon_vma because
that is from the previous vma not from parent vma. So, in anon_vma_fork(),
we need to setup theÂavc link for vma->anon.

Oh, yes. That's another example where current code miraculously stays correct.


---
 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;
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 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.

>