* [PATCH 0/3] mm: Fix misuse of parent anon_vma in dup_mmap path
@ 2020-02-08 8:35 Li Xinhai
2020-02-08 8:35 ` [PATCH 1/3] mm: don't prepare anon_vma if vma has VM_WIPEONFORK Li Xinhai
` (4 more replies)
0 siblings, 5 replies; 16+ messages in thread
From: Li Xinhai @ 2020-02-08 8:35 UTC (permalink / raw)
To: linux-mm; +Cc: akpm
This patchset fix the misuse of parenet anon_vma, which mainly caused by
child vma's vm_next and vm_prev are left same as its parent after
duplicate vma. Finally, code reached parent vma's neighbor by referring
pointer of child vma and executed wrong logic.
The first two patches fix relevant issues, and the third patch sets vm_next
and vm_prev to NULL when duplicate vma to prevent potential misuse in future.
Li Xinhai (3):
mm: don't prepare anon_vma if vma has VM_WIPEONFORK
Revert "mm/rmap.c: reuse mergeable anon_vma as parent when fork"
mm: set vm_next and vm_prev to NULL in vm_area_dup()
kernel/fork.c | 10 ++++++----
mm/rmap.c | 13 -------------
2 files changed, 6 insertions(+), 17 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/3] mm: don't prepare anon_vma if vma has VM_WIPEONFORK 2020-02-08 8:35 [PATCH 0/3] mm: Fix misuse of parent anon_vma in dup_mmap path Li Xinhai @ 2020-02-08 8:35 ` Li Xinhai 2020-02-08 8:53 ` Li Xinhai 2020-04-02 13:45 ` Kirill A. Shutemov 2020-02-08 8:35 ` [PATCH 2/3] Revert "mm/rmap.c: reuse mergeable anon_vma as parent when fork" Li Xinhai ` (3 subsequent siblings) 4 siblings, 2 replies; 16+ messages in thread From: Li Xinhai @ 2020-02-08 8:35 UTC (permalink / raw) To: linux-mm; +Cc: akpm, Rik van Riel, Kirill A. Shutemov, Matthew Wilcox In dup_mmap(), anon_vma_prepare() is called for vma has VM_WIPEONFORK, and parameter 'tmp' (i.e., the new vma of child) has same ->vm_next and ->vm_prev as its parent vma. That allows anon_vma used by parent been mistakenly shared by child (find_mergeable_anon_vma() will do this reuse work). Besides this issue, call anon_vma_prepare() should be avoided because we don't copy page for this vma. Preparing anon_vma will be handled during fault. Fixes: d2cd9ede6e19 ("mm,fork: introduce MADV_WIPEONFORK") Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com> Cc: Rik van Riel <riel@redhat.com> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> Cc: Matthew Wilcox <willy@infradead.org> --- kernel/fork.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/kernel/fork.c b/kernel/fork.c index 0808095..1bbd49a 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -552,10 +552,12 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm, if (retval) goto fail_nomem_anon_vma_fork; if (tmp->vm_flags & VM_WIPEONFORK) { - /* VM_WIPEONFORK gets a clean slate in the child. */ + /* + * VM_WIPEONFORK gets a clean slate in the child. + * Don't prepare anon_vma until fault since we don't + * copy page for current vma. + */ tmp->anon_vma = NULL; - if (anon_vma_prepare(tmp)) - goto fail_nomem_anon_vma_fork; } else if (anon_vma_fork(tmp, mpnt)) goto fail_nomem_anon_vma_fork; tmp->vm_flags &= ~(VM_LOCKED | VM_LOCKONFAULT); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] mm: don't prepare anon_vma if vma has VM_WIPEONFORK 2020-02-08 8:35 ` [PATCH 1/3] mm: don't prepare anon_vma if vma has VM_WIPEONFORK Li Xinhai @ 2020-02-08 8:53 ` Li Xinhai 2020-04-02 13:45 ` Kirill A. Shutemov 1 sibling, 0 replies; 16+ messages in thread From: Li Xinhai @ 2020-02-08 8:53 UTC (permalink / raw) To: linux-mm; +Cc: akpm, riel, kirill.shutemov, willy sorry, should send to Rik van Riel<riel@surriel.com> On 2020-02-08 at 16:35 Li Xinhai wrote: >In dup_mmap(), anon_vma_prepare() is called for vma has VM_WIPEONFORK, >and parameter 'tmp' (i.e., the new vma of child) has same ->vm_next and >->vm_prev as its parent vma. That allows anon_vma used by parent been >mistakenly shared by child (find_mergeable_anon_vma() will do this reuse >work). > >Besides this issue, call anon_vma_prepare() should be avoided because we >don't copy page for this vma. Preparing anon_vma will be handled during >fault. > >Fixes: d2cd9ede6e19 ("mm,fork: introduce MADV_WIPEONFORK") >Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com> >Cc: Rik van Riel <riel@redhat.com> >Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> >Cc: Matthew Wilcox <willy@infradead.org> >--- > kernel/fork.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > >diff --git a/kernel/fork.c b/kernel/fork.c >index 0808095..1bbd49a 100644 >--- a/kernel/fork.c >+++ b/kernel/fork.c >@@ -552,10 +552,12 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm, > if (retval) > goto fail_nomem_anon_vma_fork; > if (tmp->vm_flags & VM_WIPEONFORK) { >- /* VM_WIPEONFORK gets a clean slate in the child. */ >+ /* >+ * VM_WIPEONFORK gets a clean slate in the child. >+ * Don't prepare anon_vma until fault since we don't >+ * copy page for current vma. >+ */ > tmp->anon_vma = NULL; >- if (anon_vma_prepare(tmp)) >- goto fail_nomem_anon_vma_fork; > } else if (anon_vma_fork(tmp, mpnt)) > goto fail_nomem_anon_vma_fork; > tmp->vm_flags &= ~(VM_LOCKED | VM_LOCKONFAULT); >-- >1.8.3.1 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] mm: don't prepare anon_vma if vma has VM_WIPEONFORK 2020-02-08 8:35 ` [PATCH 1/3] mm: don't prepare anon_vma if vma has VM_WIPEONFORK Li Xinhai 2020-02-08 8:53 ` Li Xinhai @ 2020-04-02 13:45 ` Kirill A. Shutemov 1 sibling, 0 replies; 16+ messages in thread From: Kirill A. Shutemov @ 2020-04-02 13:45 UTC (permalink / raw) To: Li Xinhai Cc: linux-mm, akpm, Rik van Riel, Kirill A. Shutemov, Matthew Wilcox On Sat, Feb 08, 2020 at 08:35:26AM +0000, Li Xinhai wrote: > In dup_mmap(), anon_vma_prepare() is called for vma has VM_WIPEONFORK, > and parameter 'tmp' (i.e., the new vma of child) has same ->vm_next and > ->vm_prev as its parent vma. That allows anon_vma used by parent been > mistakenly shared by child (find_mergeable_anon_vma() will do this reuse > work). > > Besides this issue, call anon_vma_prepare() should be avoided because we > don't copy page for this vma. Preparing anon_vma will be handled during > fault. > > Fixes: d2cd9ede6e19 ("mm,fork: introduce MADV_WIPEONFORK") > Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com> > Cc: Rik van Riel <riel@redhat.com> > Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > Cc: Matthew Wilcox <willy@infradead.org> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> -- Kirill A. Shutemov ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] Revert "mm/rmap.c: reuse mergeable anon_vma as parent when fork" 2020-02-08 8:35 [PATCH 0/3] mm: Fix misuse of parent anon_vma in dup_mmap path Li Xinhai 2020-02-08 8:35 ` [PATCH 1/3] mm: don't prepare anon_vma if vma has VM_WIPEONFORK Li Xinhai @ 2020-02-08 8:35 ` Li Xinhai 2020-04-02 13:59 ` Kirill A. Shutemov 2020-02-08 8:35 ` [PATCH 3/3] mm: set vm_next and vm_prev to NULL in vm_area_dup() Li Xinhai ` (2 subsequent siblings) 4 siblings, 1 reply; 16+ messages in thread From: Li Xinhai @ 2020-02-08 8:35 UTC (permalink / raw) To: linux-mm; +Cc: akpm, Kirill A. Shutemov, Matthew Wilcox, Johannes Weiner 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(-) 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; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] Revert "mm/rmap.c: reuse mergeable anon_vma as parent when fork" 2020-02-08 8:35 ` [PATCH 2/3] Revert "mm/rmap.c: reuse mergeable anon_vma as parent when fork" Li Xinhai @ 2020-04-02 13:59 ` Kirill A. Shutemov 2020-04-03 3:08 ` Li Xinhai 0 siblings, 1 reply; 16+ messages in thread From: Kirill A. Shutemov @ 2020-04-02 13:59 UTC (permalink / raw) To: Li Xinhai Cc: linux-mm, akpm, Kirill A. Shutemov, Matthew Wilcox, Johannes Weiner 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? -- Kirill A. Shutemov ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] Revert "mm/rmap.c: reuse mergeable anon_vma as parent when fork" 2020-04-02 13:59 ` Kirill A. Shutemov @ 2020-04-03 3:08 ` Li Xinhai 2020-04-05 1:26 ` Li Xinhai 0 siblings, 1 reply; 16+ messages in thread From: Li Xinhai @ 2020-04-03 3:08 UTC (permalink / raw) To: Kirill A. Shutemov; +Cc: linux-mm, akpm, kirill.shutemov, willy, hannes 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] Revert "mm/rmap.c: reuse mergeable anon_vma as parent when fork" 2020-04-03 3:08 ` Li Xinhai @ 2020-04-05 1:26 ` Li Xinhai 0 siblings, 0 replies; 16+ messages in thread From: Li Xinhai @ 2020-04-05 1:26 UTC (permalink / raw) To: akpm, Kirill A. Shutemov; +Cc: linux-mm, kirill.shutemov, willy, hannes On 2020-04-03 at 11:08 Li Xinhai wrote: >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. > If ->vm_next and ->vm_prev have been set as NULL in vm_area_dup(), the existing code will crash immediately by dereference NULL point (i.e., prev is NULL, and use prev->anon_vma), instead of getting involved in parent anon_vma. I don't choose to fix this NULL pointer dereference, but revert the commit because of the reason as described in the last paragraph of commit message. Andrew, please consider merge this patch as the existing code has no value to remain. >> >>-- >> Kirill A. Shutemov ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] mm: set vm_next and vm_prev to NULL in vm_area_dup() 2020-02-08 8:35 [PATCH 0/3] mm: Fix misuse of parent anon_vma in dup_mmap path Li Xinhai 2020-02-08 8:35 ` [PATCH 1/3] mm: don't prepare anon_vma if vma has VM_WIPEONFORK Li Xinhai 2020-02-08 8:35 ` [PATCH 2/3] Revert "mm/rmap.c: reuse mergeable anon_vma as parent when fork" Li Xinhai @ 2020-02-08 8:35 ` Li Xinhai 2020-04-02 14:07 ` Kirill A. Shutemov 2020-02-10 0:52 ` [PATCH 0/3] mm: Fix misuse of parent anon_vma in dup_mmap path Andrew Morton 2020-04-02 1:25 ` Andrew Morton 4 siblings, 1 reply; 16+ messages in thread From: Li Xinhai @ 2020-02-08 8:35 UTC (permalink / raw) To: linux-mm; +Cc: akpm, Kirill A. Shutemov, Matthew Wilcox, Johannes Weiner Set ->vm_next and ->vm_prev to NULL to prevent potential misuse from the new duplicated vma. Currently, only in fork path there are misuse for handling anon_vma. No other bugs been revealed with this patch applied. 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> --- kernel/fork.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/fork.c b/kernel/fork.c index 1bbd49a..eb3a148 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -361,6 +361,7 @@ struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig) if (new) { *new = *orig; INIT_LIST_HEAD(&new->anon_vma_chain); + new->vm_next = new->vm_prev = NULL; } return new; } @@ -561,7 +562,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); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] mm: set vm_next and vm_prev to NULL in vm_area_dup() 2020-02-08 8:35 ` [PATCH 3/3] mm: set vm_next and vm_prev to NULL in vm_area_dup() Li Xinhai @ 2020-04-02 14:07 ` Kirill A. Shutemov 0 siblings, 0 replies; 16+ messages in thread From: Kirill A. Shutemov @ 2020-04-02 14:07 UTC (permalink / raw) To: Li Xinhai Cc: linux-mm, akpm, Kirill A. Shutemov, Matthew Wilcox, Johannes Weiner On Sat, Feb 08, 2020 at 08:35:28AM +0000, Li Xinhai wrote: > Set ->vm_next and ->vm_prev to NULL to prevent potential misuse from > the new duplicated vma. > > Currently, only in fork path there are misuse for handling anon_vma. No > other bugs been revealed with this patch applied. > > 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> Looks okay to me. Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> -- Kirill A. Shutemov ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] mm: Fix misuse of parent anon_vma in dup_mmap path 2020-02-08 8:35 [PATCH 0/3] mm: Fix misuse of parent anon_vma in dup_mmap path Li Xinhai ` (2 preceding siblings ...) 2020-02-08 8:35 ` [PATCH 3/3] mm: set vm_next and vm_prev to NULL in vm_area_dup() Li Xinhai @ 2020-02-10 0:52 ` Andrew Morton 2020-02-10 2:15 ` Li Xinhai 2020-04-02 1:25 ` Andrew Morton 4 siblings, 1 reply; 16+ messages in thread From: Andrew Morton @ 2020-02-10 0:52 UTC (permalink / raw) To: Li Xinhai; +Cc: linux-mm On Sat, 8 Feb 2020 08:35:25 +0000 Li Xinhai <lixinhai.lxh@gmail.com> wrote: > This patchset fix the misuse of parenet anon_vma, which mainly caused by > child vma's vm_next and vm_prev are left same as its parent after > duplicate vma. Finally, code reached parent vma's neighbor by referring > pointer of child vma and executed wrong logic. > > The first two patches fix relevant issues, and the third patch sets vm_next > and vm_prev to NULL when duplicate vma to prevent potential misuse in future. What are the runtime effects of this bug? How is the bug triggered? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] mm: Fix misuse of parent anon_vma in dup_mmap path 2020-02-10 0:52 ` [PATCH 0/3] mm: Fix misuse of parent anon_vma in dup_mmap path Andrew Morton @ 2020-02-10 2:15 ` Li Xinhai 2020-04-20 12:06 ` Michal Hocko 0 siblings, 1 reply; 16+ messages in thread From: Li Xinhai @ 2020-02-10 2:15 UTC (permalink / raw) To: akpm; +Cc: linux-mm On 2020-02-10 at 08:52 Andrew Morton wrote: >On Sat, 8 Feb 2020 08:35:25 +0000 Li Xinhai <lixinhai.lxh@gmail.com> wrote: > >> This patchset fix the misuse of parenet anon_vma, which mainly caused by >> child vma's vm_next and vm_prev are left same as its parent after >> duplicate vma. Finally, code reached parent vma's neighbor by referring >> pointer of child vma and executed wrong logic. >> >> The first two patches fix relevant issues, and the third patch sets vm_next >> and vm_prev to NULL when duplicate vma to prevent potential misuse in future. > >What are the runtime effects of this bug? How is the bug triggered? Effects of the first one is that causes ramp code to check both parent and child's page table, although a page couldn't be mapped by both parent and child, because child vma has WIPEONFORK so all pages mapped by child are 'new' and not relevant to parent. Effects of the second one is that the relationship of anon_vma of parent and child are totally convoluted. It would cause 'son', 'grandson', ..., etc, to share 'parent' anon_vma, which disobey the design rule of reusing anon_vma (the rule to be followed is that reusing should among vma of same process, and vma should not gone through fork). So, both issues should cause unnecessary rmap walking and have unexpected complexity. These two issues would not be directly visible, I used debugging code to check the anon_vma pointers of parent and child when inspecting the suspicious implementation of issue #2, then find the problem. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] mm: Fix misuse of parent anon_vma in dup_mmap path 2020-02-10 2:15 ` Li Xinhai @ 2020-04-20 12:06 ` Michal Hocko 2020-04-21 3:53 ` Li Xinhai 0 siblings, 1 reply; 16+ messages in thread From: Michal Hocko @ 2020-04-20 12:06 UTC (permalink / raw) To: Li Xinhai; +Cc: akpm, linux-mm On Mon 10-02-20 10:15:53, Li Xinhai wrote: [...] > >What are the runtime effects of this bug? How is the bug triggered? > > Effects of the first one is that causes ramp code to check both parent and > child's page table, although a page couldn't be mapped by both parent > and child, because child vma has WIPEONFORK so all pages mapped by child > are 'new' and not relevant to parent. > > Effects of the second one is that the relationship of anon_vma of parent and > child are totally convoluted. It would cause 'son', 'grandson', ..., etc, to share > 'parent' anon_vma, which disobey the design rule of reusing anon_vma (the > rule to be followed is that reusing should among vma of same process, and > vma should not gone through fork). > > So, both issues should cause unnecessary rmap walking and have unexpected > complexity. > > These two issues would not be directly visible, I used debugging code to check > the anon_vma pointers of parent and child when inspecting the suspicious > implementation of issue #2, then find the problem. I am still not completely clear on the user effect. Does this allow an adversary to generate too long anon_vma chains that wouldn't normally happen or some other nasties that would make backporting to older kernels really necessary? Because from my current understanding it would only make WIPEONFORK vmas rmap walks less efficient. Or is there any other functional issue? -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] mm: Fix misuse of parent anon_vma in dup_mmap path 2020-04-20 12:06 ` Michal Hocko @ 2020-04-21 3:53 ` Li Xinhai 2020-04-21 7:26 ` Michal Hocko 0 siblings, 1 reply; 16+ messages in thread From: Li Xinhai @ 2020-04-21 3:53 UTC (permalink / raw) To: Michal Hocko; +Cc: akpm, linux-mm On 2020-04-20 at 20:06 Michal Hocko wrote: >On Mon 10-02-20 10:15:53, Li Xinhai wrote: >[...] >> >What are the runtime effects of this bug? How is the bug triggered? >> >> Effects of the first one is that causes ramp code to check both parent and >> child's page table, although a page couldn't be mapped by both parent >> and child, because child vma has WIPEONFORK so all pages mapped by child >> are 'new' and not relevant to parent. >> >> Effects of the second one is that the relationship of anon_vma of parent and >> child are totally convoluted. It would cause 'son', 'grandson', ..., etc, to share >> 'parent' anon_vma, which disobey the design rule of reusing anon_vma (the >> rule to be followed is that reusing should among vma of same process, and >> vma should not gone through fork). >> >> So, both issues should cause unnecessary rmap walking and have unexpected >> complexity. >> >> These two issues would not be directly visible, I used debugging code to check >> the anon_vma pointers of parent and child when inspecting the suspicious >> implementation of issue #2, then find the problem. > >I am still not completely clear on the user effect. Does this allow an >adversary to generate too long anon_vma chains that wouldn't normally >happen or some other nasties that would make backporting to older >kernels really necessary? It allows for generating long anon_vma chain by keep forking child process, although the memory consumed for anon_vma structure wouldn't increase accordingly. I don't see other impact of those two issues. > Because from my current understanding it would >only make WIPEONFORK vmas rmap walks less efficient. Or is there any >other functional issue? #1 issue is for case WIPEONFORK, #2 issue for all other cases of anonymous VMA dup (i.e., when parent vmas is currently sharing anon_vma among themself, child vma will share its parent vma's anon_vma). Both cause inefficient rmap walking, no other functional issues. In my understanding, this patchset can be backported to stable kernel. E.g., If someone mmap a big vm area, and split that vma to several adjacent vmas(e.g., by mprotect), then sharing anon_vma at parent process level is achivied. After that, begin fork child, maybe choose to keep forking child from the top parent process, or let child process keep forking its own child. At this moment, sharing anon_vma among children and parent is achieved and has long chain. >-- >Michal Hocko >SUSE Labs ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] mm: Fix misuse of parent anon_vma in dup_mmap path 2020-04-21 3:53 ` Li Xinhai @ 2020-04-21 7:26 ` Michal Hocko 0 siblings, 0 replies; 16+ messages in thread From: Michal Hocko @ 2020-04-21 7:26 UTC (permalink / raw) To: Li Xinhai; +Cc: akpm, linux-mm On Tue 21-04-20 11:53:02, Li Xinhai wrote: > On 2020-04-20 at 20:06 Michal Hocko wrote: [...] > > Because from my current understanding it would > >only make WIPEONFORK vmas rmap walks less efficient. Or is there any > >other functional issue? > #1 issue is for case WIPEONFORK, #2 issue for all other cases of anonymous VMA dup > (i.e., when parent vmas is currently sharing anon_vma among themself, child vma will > share its parent vma's anon_vma). OK, it was not really clear to me that also !WIPEONFORK vmas are affected. Thanks for the clarification. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] mm: Fix misuse of parent anon_vma in dup_mmap path 2020-02-08 8:35 [PATCH 0/3] mm: Fix misuse of parent anon_vma in dup_mmap path Li Xinhai ` (3 preceding siblings ...) 2020-02-10 0:52 ` [PATCH 0/3] mm: Fix misuse of parent anon_vma in dup_mmap path Andrew Morton @ 2020-04-02 1:25 ` Andrew Morton 4 siblings, 0 replies; 16+ messages in thread From: Andrew Morton @ 2020-04-02 1:25 UTC (permalink / raw) To: Li Xinhai; +Cc: linux-mm, Rik van Riel, Kirill A. Shutemov, Matthew Wilcox On Sat, 8 Feb 2020 08:35:25 +0000 Li Xinhai <lixinhai.lxh@gmail.com> wrote: > This patchset fix the misuse of parenet anon_vma, which mainly caused by > child vma's vm_next and vm_prev are left same as its parent after > duplicate vma. Finally, code reached parent vma's neighbor by referring > pointer of child vma and executed wrong logic. > > The first two patches fix relevant issues, and the third patch sets vm_next > and vm_prev to NULL when duplicate vma to prevent potential misuse in future. > > Li Xinhai (3): > mm: don't prepare anon_vma if vma has VM_WIPEONFORK > Revert "mm/rmap.c: reuse mergeable anon_vma as parent when fork" > mm: set vm_next and vm_prev to NULL in vm_area_dup() > > kernel/fork.c | 10 ++++++---- > mm/rmap.c | 13 ------------- > 2 files changed, 6 insertions(+), 17 deletions(-) Can we please have some review input on this series? ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-04-21 7:27 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-08 8:35 [PATCH 0/3] mm: Fix misuse of parent anon_vma in dup_mmap path Li Xinhai 2020-02-08 8:35 ` [PATCH 1/3] mm: don't prepare anon_vma if vma has VM_WIPEONFORK Li Xinhai 2020-02-08 8:53 ` Li Xinhai 2020-04-02 13:45 ` Kirill A. Shutemov 2020-02-08 8:35 ` [PATCH 2/3] Revert "mm/rmap.c: reuse mergeable anon_vma as parent when fork" Li Xinhai 2020-04-02 13:59 ` Kirill A. Shutemov 2020-04-03 3:08 ` Li Xinhai 2020-04-05 1:26 ` Li Xinhai 2020-02-08 8:35 ` [PATCH 3/3] mm: set vm_next and vm_prev to NULL in vm_area_dup() Li Xinhai 2020-04-02 14:07 ` Kirill A. Shutemov 2020-02-10 0:52 ` [PATCH 0/3] mm: Fix misuse of parent anon_vma in dup_mmap path Andrew Morton 2020-02-10 2:15 ` Li Xinhai 2020-04-20 12:06 ` Michal Hocko 2020-04-21 3:53 ` Li Xinhai 2020-04-21 7:26 ` Michal Hocko 2020-04-02 1:25 ` Andrew Morton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).