From: Alexei Starovoitov <[email protected]>
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 <[email protected]>
---
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();
+ }
flush_cache_dup_mm(oldmm);
uprobe_dup_mmap(oldmm, mm);
/*
@@ -1883,6 +1897,7 @@ __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);
+ uprobe_end_dup_mmap();
if (!retval)
dup_userfaultfd_complete(&uf);
else
--
2.55.0
