Re: [PATCH bpf-next 7/9] fork: Let the new task point to its mm before uprobes can find it

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Thu, Oct 22, 2026 at 01:57:10AM +0000, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
>
> uprobe_perf_filter() compares event->hw.target->mm with mm. The target of
> an inherited event is the new task. copy_mm() sets tsk->mm after dup_mm()
> returned, but register_for_each_vma() can find the new mm as soon as
> dup_mmap() releases dup_mmap_sem. No task points to this mm yet, so
> the filter says that nobody wants it.
>
> The walk that removes the breakpoint, because another consumer of the same
> uprobe goes away, sees !filter_chain() and removes it from the child.
> Nobody installs it again, and the inherited event doesn't see this child.
>
> It's an old bug with a short window. The previous patch releases
> dup_mmap_sem before dup_userfaultfd_complete(), so with UFFD_EVENT_FORK
> the window is as long as the monitor likes.
>
> Set tsk->mm at the end of dup_mmap() before it releases dup_mmap_sem.
> Not earlier. The new task can be reached via its perf events and from bpf
> progs. They shouldn't find mm that is half copied. Clear tsk->mm when
> dup_mm() fails after dup_mmap(). Do both under task_lock() like exec_mmap()
> and exit_mm().
>
> The tasks with such event fork, while another consumer of the same uprobe
> comes and goes. 5 cpu VM, 90 sec and 7000 children per run. The number of
> children that the inherited event didn't see:
>
>   without this series          11 to 24, 8 runs
>   patches 1, 2, 5, 6 only      23, 12, 21
>   this series                  0, 0, 0
>
> With UFFD_EVENT_FORK monitor that reads the event after 5 sec the child
> lost the breakpoint in 5 of 5 runs without this patch, and in 0 of 5 with.
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>

This feels like a 'fix in the wrong place' thing again really.

What would a fix in uprobe code look like?

I don't love that core mm fork code is being changed to address uprobe
problems in general.

> ---
>  kernel/fork.c |  9 ++++++++-
>  mm/internal.h |  3 ++-
>  mm/mmap.c     | 14 +++++++++++++-
>  mm/nommu.c    |  3 ++-
>  4 files changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 8fd6be932afe..5a2f571b64b7 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1539,7 +1539,7 @@ static struct mm_struct *dup_mm(struct task_struct *tsk,
>  	if (!mm_init(mm, tsk))
>  		goto fail_nomem;
>
> -	err = dup_mmap(mm, oldmm);
> +	err = dup_mmap(tsk, mm, oldmm);
>  	if (err)
>  		goto free_pt;
>
> @@ -1552,6 +1552,13 @@ static struct mm_struct *dup_mm(struct task_struct *tsk,
>  	return mm;
>
>  free_pt:
> +	/*
> +	 * dup_mmap() has set it if it succeeded. get_task_mm() must not find
> +	 * it after the mmput() below.
> +	 */
> +	task_lock(tsk);
> +	tsk->mm = NULL;
> +	task_unlock(tsk);

It's not great to straddle some task stuff here, and some in
dup_mmap(). Could it all just live in copy_mm(), where other task stuff is
done?

It seems like changes you did in the previous patch dictate a move here,
but the fix that David mentions moves the mmap lock back to dup_mm() anyway
so a rework should hopefully avoid all that :)

>  	/* don't put binfmt in mmput, we haven't got module yet */
>  	mm->binfmt = NULL;
>  	mm_init_owner(mm, NULL);
> diff --git a/mm/internal.h b/mm/internal.h
> index 9e3be236f18c..70e40306d881 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -1520,7 +1520,8 @@ int walk_page_range_debug(struct mm_struct *mm, unsigned long start,
>  			  pgd_t *pgd, void *private);
>
>  void dup_mm_exe_file(struct mm_struct *mm, struct mm_struct *oldmm);
> -int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm);
> +int dup_mmap(struct task_struct *tsk, struct mm_struct *mm,
> +	     struct mm_struct *oldmm);
>
>  int remap_pfn_range_prepare(struct vm_area_desc *desc);
>  int remap_pfn_range_complete(struct vm_area_struct *vma,
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 6ce654e33e3b..3167272a77ad 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -46,6 +46,7 @@
>  #include <linux/pkeys.h>
>  #include <linux/oom.h>
>  #include <linux/sched/mm.h>
> +#include <linux/sched/task.h>
>  #include <linux/ksm.h>
>  #include <linux/memfd.h>
>
> @@ -1705,7 +1706,8 @@ bool mmap_read_lock_maybe_expand(struct mm_struct *mm,
>  	return true;
>  }
>
> -__latent_entropy int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
> +__latent_entropy int dup_mmap(struct task_struct *tsk, struct mm_struct *mm,
> +			      struct mm_struct *oldmm)
>  {
>  	struct vm_area_struct *mpnt, *tmp;
>  	int retval;
> @@ -1897,6 +1899,16 @@ __latent_entropy int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
>  	mmap_write_unlock(mm);
>  	flush_tlb_mm(oldmm);
>  	mmap_write_unlock(oldmm);
> +	if (!retval) {
> +		/*
> +		 * As soon as dup_mmap_sem is released uprobes can find mm via
> +		 * the rmap tree and ask the consumers if they want it. The
> +		 * filter of an inherited perf event looks for mm in tsk->mm.
> +		 */
> +		task_lock(tsk);
> +		tsk->mm = mm;
> +		task_unlock(tsk);
> +	}

Yeah this isn't lovely, it feels like you're having mm logic handle
uprobe's problems. I don't like the idea of changing the signature here and
having core mm deal with the task stuff like this, and now it's being
fiddled with over 3 separate functions.

You should be doing this in the more generic fork code.

Or, ideally, if you could find a solution that doesn't involve changing
core fork code but rather modified uprobe to stop making what are,
currently, seemingly broken or at least sub-optimal assumptions that'd be a
lot better!

>  	uprobe_end_dup_mmap();
>  	if (!retval)
>  		dup_userfaultfd_complete(&uf);
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 68b05565e15a..7529e03d3f2c 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1885,7 +1885,8 @@ static int __meminit init_admin_reserve(void)
>  }
>  subsys_initcall(init_admin_reserve);
>
> -int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
> +int dup_mmap(struct task_struct *tsk, struct mm_struct *mm,
> +	     struct mm_struct *oldmm)
>  {
>  	mmap_write_lock(oldmm);
>  	dup_mm_exe_file(mm, oldmm);
> --
> 2.55.0
>

--
Cheers, Lorenzo