| Message ID | 1581150928-3214-3-git-send-email-lixinhai.lxh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: Fix misuse of parent anon_vma in dup_mmap path | expand |
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?
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
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
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;
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(-)