Re: [PATCH 02/46] fork: take mmap_lock before copying mm_struct

["Vlastimil Babka (SUSE)" <vbabka@kernel.org>]

On 10/27/26 00:17, David Hildenbrand (Arm) wrote:
> dup_mm() copies the parent's mm_struct before dup_mmap() takes the
> mmap_lock. A concurrent brk() can leave the child with a stale mm->brk
> and newer heap mappings, causing later brk() requests to fail.
> 
> Other operations (e.g., PR_SET_MM) on the parent's mm_struct can similarly
> cause inconsistencies.
> 
> As a simple and robust fix, grab the parent's mmap_lock earlier. This
> way we don't need to update individual mm fields in dup_mmap() and
> keep missing ones. Stop updating the 4 existing members (total_vm, data_vm,
> exec_vm, stack_vm), as that is now no longer required.
> 
> Keep releasing the parent's mmap_lock in dup_mmap(), before
> userfaultfd completion, to avoid moving the userfaultfd handling into the
> caller. This should get cleaned up later.
> 
> Move uprobe_start_dup_mmap() to preserve the lock ordering.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Reported-by: Josef Bacik <josef@toxicpanda.com>
> Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
> Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>

Reviewed-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>

> ---
>  kernel/fork.c | 20 ++++++++++++++------
>  mm/mmap.c     |  8 +-------
>  mm/nommu.c    |  2 +-
>  3 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 22eaf5fb844d0..c8948cadccf71 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1532,12 +1532,19 @@ static struct mm_struct *dup_mm(struct task_struct *tsk,
>  	if (!mm)
>  		goto fail_nomem;
>  
> +	uprobe_start_dup_mmap();
> +	if (mmap_write_lock_killable(oldmm)) {
> +		free_mm(mm);
> +		goto fail_uprobe;
> +	}
> +
>  	memcpy(mm, oldmm, sizeof(*mm));
>  
> -	if (!mm_init(mm, tsk))
> -		goto fail_nomem;
> +	if (!mm_init(mm, tsk)) {
> +		mmap_write_unlock(oldmm);
> +		goto fail_uprobe;
> +	}
>  
> -	uprobe_start_dup_mmap();
>  	err = dup_mmap(mm, oldmm);
>  	if (err)
>  		goto free_pt;
> @@ -1556,9 +1563,10 @@ static struct mm_struct *dup_mm(struct task_struct *tsk,
>  	mm->binfmt = NULL;
>  	mm_init_owner(mm, NULL);
>  	mmput(mm);
> -	if (err)
> -		uprobe_end_dup_mmap();
> -
> +	if (!err)
> +		return NULL;
> +fail_uprobe:
> +	uprobe_end_dup_mmap();
>  fail_nomem:
>  	return NULL;
>  }
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 148ba01c03739..7618e72766a52 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1713,8 +1713,7 @@ __latent_entropy int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
>  	LIST_HEAD(uf);
>  	VMA_ITERATOR(vmi, mm, 0);
>  
> -	if (mmap_write_lock_killable(oldmm))
> -		return -EINTR;
> +	mmap_assert_write_locked(oldmm);
>  	flush_cache_dup_mm(oldmm);
>  	uprobe_dup_mmap(oldmm, mm);
>  	/*
> @@ -1725,11 +1724,6 @@ __latent_entropy int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
>  	/* No ordering required: file already has been exposed. */
>  	dup_mm_exe_file(mm, oldmm);
>  
> -	mm->total_vm = oldmm->total_vm;
> -	mm->data_vm = oldmm->data_vm;
> -	mm->exec_vm = oldmm->exec_vm;
> -	mm->stack_vm = oldmm->stack_vm;
> -
>  	/* Use __mt_dup() to efficiently build an identical maple tree. */
>  	retval = __mt_dup(&oldmm->mm_mt, &mm->mm_mt, GFP_KERNEL);
>  	if (unlikely(retval))
> diff --git a/mm/nommu.c b/mm/nommu.c
> index ed44510e37707..6e0f9c7da2364 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1926,7 +1926,7 @@ subsys_initcall(init_admin_reserve);
>  
>  int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
>  {
> -	mmap_write_lock(oldmm);
> +	mmap_assert_write_locked(oldmm);
>  	dup_mm_exe_file(mm, oldmm);
>  	mmap_write_unlock(oldmm);
>  	return 0;
>