Re: [PATCH bpf-next 6/9] uprobes: Don't sleep on mmap_lock with dup_mmap_sem held in dup_mmap()

[David Hildenbrand (Arm)]

On 10/22/26 03:57, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@xxxxxxxxxx>
> 
> dup_mm() takes dup_mmap_sem for reading and then dup_mmap() sleeps on
> mmap_lock of the parent. When another thread keeps that mmap_lock busy
> register_for_each_vma() waits for the reader, and every other fork() in
> the system waits for the writer. The parent doesn't have to map the file:
> 
>   A thread 1:  mmap(MAP_POPULATE) -> mmap_read_lock(A)
>   A thread 2:  fork() -> percpu_down_read(&dup_mmap_sem),
>                mmap_write_lock_killable(A) sleeps
>   tracer:      percpu_down_write(&dup_mmap_sem) sleeps
>   others:      fork() -> percpu_down_read(&dup_mmap_sem) sleeps
> 
> dup_mmap() needs both locks. Don't sleep on one of them while holding
> the other. Sleep on mmap_lock and trylock dup_mmap_sem. If that fails sleep
> on dup_mmap_sem and trylock mmap_lock, and so on. Taking mmap_lock and then
> sleeping on dup_mmap_sem is simpler, but then all threads of the parent
> wait for mmap_lock as long as the writer holds dup_mmap_sem.
> 
> Both are taken in dup_mmap() now, so release dup_mmap_sem there too.
> Commit 8ac662f5da19 ("fork: avoid inappropriate uprobe access to invalid
> mm") moved that to dup_mm() to hide the child from uprobes until mmput()
> when dup_mmap() fails. Since commit 43873af772f8 ("mm: change dup_mmap()
> recovery") dup_mmap() removes all vmas of such child before it drops
> mmap_lock.
> 
> dup_mmap_sem is released before dup_userfaultfd_complete(), so the monitor
> that doesn't read UFFD_EVENT_FORK no longer blocks every fork(). During
> that wait register_for_each_vma() can find the child, while no task points
> to its mm. The next patch deals with that.
> 
> Thread 1 of process A loops over mmap(4G, MAP_POPULATE) and munmap().
> Thread 2 calls fork() in a loop. Nobody maps the file. Process B calls
> fork() in a loop. 32 cpu VM. Median (max) of 15-25 runs, msec:
> 
>                          the longest fork() in B    attach
>                          before      after          before  after
>  uprobe_multi, 1 uprobe  206 (219)   0.10 (0.30)       220   11.7
>  uprobe_multi, 4 uprobes 208 (428)   0.08 (0.32)       219   13.5
>  perf_event, 4 uprobes   419 (424)   0.10 (0.44)       632   11.4
> 
> During detach: 265-293 -> 0.19-0.37.
> 
> Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> ---
>  include/linux/uprobes.h |  5 +++++
>  kernel/events/uprobes.c |  5 +++++
>  kernel/fork.c           |  4 ----
>  mm/mmap.c               | 19 +++++++++++++++++--
>  4 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index d34dbc0fbbfe..6d74efa80d04 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -211,6 +211,7 @@ extern void uprobe_unregister_sync(void);
>  extern int uprobe_mmap(struct vm_area_struct *vma);
>  extern void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end);
>  extern void uprobe_start_dup_mmap(void);
> +extern bool uprobe_try_start_dup_mmap(void);
>  extern void uprobe_end_dup_mmap(void);
>  extern void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm);
>  extern void uprobe_free_utask(struct task_struct *t);
> @@ -277,6 +278,10 @@ uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end
>  static inline void uprobe_start_dup_mmap(void)
>  {
>  }
> +static inline bool uprobe_try_start_dup_mmap(void)
> +{
> +	return true;
> +}
>  static inline void uprobe_end_dup_mmap(void)
>  {
>  }
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index a9274016c55d..92ccb730278f 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1927,6 +1927,11 @@ void uprobe_start_dup_mmap(void)
>  	percpu_down_read(&dup_mmap_sem);
>  }
>  
> +bool uprobe_try_start_dup_mmap(void)
> +{
> +	return percpu_down_read_trylock(&dup_mmap_sem);
> +}
> +
>  void uprobe_end_dup_mmap(void)
>  {
>  	percpu_up_read(&dup_mmap_sem);
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 10f2d05d816a..8fd6be932afe 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1539,11 +1539,9 @@ static struct mm_struct *dup_mm(struct task_struct *tsk,
>  	if (!mm_init(mm, tsk))
>  		goto fail_nomem;
>  
> -	uprobe_start_dup_mmap();
>  	err = dup_mmap(mm, oldmm);
>  	if (err)
>  		goto free_pt;
> -	uprobe_end_dup_mmap();
>  
>  	mm->hiwater_rss = get_mm_rss(mm);
>  	mm->hiwater_vm = mm->total_vm;
> @@ -1558,8 +1556,6 @@ 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();
>  
>  fail_nomem:
>  	return NULL;
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 4bf26b0f1e6e..6ce654e33e3b 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1713,8 +1713,22 @@ __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;
> +	/*
> +	 * Take mmap_lock of oldmm and dup_mmap_sem of uprobes.
> +	 * Don't sleep on one of them while holding the other.
> +	 */
> +	for (;;) {
> +		if (mmap_write_lock_killable(oldmm))
> +			return -EINTR;
> +		if (uprobe_try_start_dup_mmap())
> +			break;
> +		mmap_write_unlock(oldmm);
> +
> +		uprobe_start_dup_mmap();
> +		if (mmap_write_trylock(oldmm))
> +			break;
> +		uprobe_end_dup_mmap();
> +	}
A note that I will have revised fix from Josef conflict with the changes here.
Nothing that cannot be worked around, but the whole "take writelock in dup_mmap"
is shaky as it seems.

-- 
Cheers,

David