Re: [PATCH 2/3] Revert "mm/rmap.c: reuse mergeable anon_vma as parent when fork"

["Li Xinhai" <lixinhai.lxh@gmail.com>]

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