[PATCH bpf-next 0/9] uprobes: Don't stall fork() when mmap_lock is busy

[PATCH bpf-next 0/9] uprobes: Don't stall fork() when mmap_lock is busy

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

uprobe_register() and uprobe_unregister() stall every fork() in the system
when some process keeps its mmap_lock busy:
- register_for_each_vma() sleeps on that mmap_lock with dup_mmap_sem held
  for writing.
- fork() of that process sleeps on that mmap_lock with dup_mmap_sem held
  for reading. The next register_for_each_vma() waits for it, and other
  forks wait for the writer. The process doesn't have to map the file.

We found it the hard way by using uprobes in production. uprobes were
attached to libraries and binaries that many processes map, on a large
number of hosts. On some of them every fork() was stuck for minutes while
uprobe was being registered.

Patch 5 fixes the first one. Patch 6 fixes the second one. Patch 1 is
a helper for them and patch 9 is the selftest.

The rest fix old bugs that patches 5 and 6 make easier to hit:
- Patch 2: exit_mmap() runs under dup_mmap_sem.
- Patches 3, 4: x86. The removal of optimized uprobe that failed half-way
  is never finished, and SIGKILL triggers WARN_ON_ONCE().
- Patch 7: the child loses the breakpoint of an inherited perf event.
- Patch 8: the tracer that is killed leaves its breakpoints behind.

A process loops over mmap(4G, MAP_POPULATE) and munmap(). Another process
calls fork() in a loop. The longest fork() while uprobes are being attached
and the time to attach, in msec. 32 cpu VM, HZ=1000, no debug options,
median of 15-45 runs:

                                       fork()          attach
                                    before  after   before  after
the busy process maps the file
  uprobe_multi, 1 uprobe               201   0.25      217    232
  uprobe_multi, 4 uprobes              422   0.34     1483   1558
  perf_event, 4 uprobes                421   0.47     3150   3322
it forks and doesn't map the file
  uprobe_multi, 1 uprobe               198   0.07      212    9.6
  uprobe_multi, 4 uprobes              203   0.08      213   13.4
  perf_event, 4 uprobes                419   0.08      630   12.3
nobody keeps mmap_lock busy
  uprobe_multi, 1000 processes        1.93   1.83     4.24   4.49
  uprobe_multi, 5000 processes        8.88   8.60     8.33   9.23

I don't care which tree it goes through. It targets bpf-next only because
BPF CI will run the patches, including the new selftest.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>

Alexei Starovoitov (9):
  mm: Bring back mmap_write_trylock()
  uprobes: Don't run exit_mmap() under dup_mmap_sem
  uprobes/x86: Finish an unoptimization which failed half-way
  uprobes/x86: Don't WARN if an optimized probe can't be written
  uprobes: Don't sleep on mmap_lock under dup_mmap_sem
  uprobes: Don't sleep on mmap_lock with dup_mmap_sem held in dup_mmap()
  fork: Let the new task point to its mm before uprobes can find it
  uprobes: Don't leak the breakpoints if the unregistering task is
    killed
  selftests/bpf: Test uprobes and fork() with busy mmap_lock

 arch/x86/kernel/uprobes.c                     |  23 +-
 include/linux/mmap_lock.h                     |  12 +
 include/linux/uprobes.h                       |   5 +
 kernel/events/uprobes.c                       | 260 ++++++++--
 kernel/fork.c                                 |  13 +-
 mm/internal.h                                 |   3 +-
 mm/mmap.c                                     |  33 +-
 mm/nommu.c                                    |   3 +-
 .../selftests/bpf/prog_tests/uprobe_busy_mm.c | 447 ++++++++++++++++++
 .../selftests/bpf/progs/uprobe_busy_mm.c      |  22 +
 10 files changed, 766 insertions(+), 55 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe_busy_mm.c
 create mode 100644 tools/testing/selftests/bpf/progs/uprobe_busy_mm.c

-- 
2.55.0

[PATCH bpf-next 1/9] mm: Bring back mmap_write_trylock()

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

Commit cf95e337cb63 ("mm: delete mmap_write_trylock() and
vma_try_start_write()") removed it when khugepaged stopped using it.
uprobes and dup_mmap() are going to use it.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/mmap_lock.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
index b8a13b8d36a4..e5420b0ce61b 100644
--- a/include/linux/mmap_lock.h
+++ b/include/linux/mmap_lock.h
@@ -566,6 +566,18 @@ static inline int __must_check mmap_write_lock_killable(struct mm_struct *mm)
 	return ret;
 }
 
+static inline bool __must_check mmap_write_trylock(struct mm_struct *mm)
+{
+	bool ret;
+
+	__mmap_lock_trace_start_locking(mm, true);
+	ret = down_write_trylock(&mm->mmap_lock) != 0;
+	if (ret)
+		mm_lock_seqcount_begin(mm);
+	__mmap_lock_trace_acquire_returned(mm, true, ret);
+	return ret;
+}
+
 /*
  * Drop all currently-held per-VMA locks.
  * This is called from the mmap_lock implementation directly before releasing
-- 
2.55.0

[PATCH bpf-next 2/9] uprobes: Don't run exit_mmap() under dup_mmap_sem

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

build_map_info() takes mm_users of every mm that maps the file and
register_for_each_vma() drops it after the visit. When the process has
exited meanwhile it's the last mmput(), and exit_mmap() of a large mm runs
with dup_mmap_sem held for writing. Every fork() in the system waits for
it. Use mmput_async().

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/events/uprobes.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 7709ea882477..6792004be6fa 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1250,7 +1250,7 @@ build_map_info(struct address_space *mapping, loff_t offset, bool is_register)
 
 	prev = curr;
 	while (curr) {
-		mmput(curr->mm);
+		mmput_async(curr->mm);
 		curr = curr->next;
 	}
 
@@ -1324,7 +1324,8 @@ register_for_each_vma(struct uprobe *uprobe, struct uprobe_consumer *new)
  unlock:
 		mmap_write_unlock(mm);
  free:
-		mmput(mm);
+		/* exit_mmap() of the last user must not run under dup_mmap_sem */
+		mmput_async(mm);
 		info = free_map_info(info);
 	}
  out:
-- 
2.55.0

[PATCH bpf-next 3/9] uprobes/x86: Finish an unoptimization which failed half-way

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

int3_update_unoptimize() restores optimized nop10 in three writes:

  48 8d 64 24 80 e8 d0 d1 d2 d3    lea -0x80(%rsp),%rsp; call tramp
  cc 8d 64 24 80 e8 d0 d1 d2 d3    1. trap new entries
  cc 2e 0f 1f 84 e8 d0 d1 d2 d3    2. restore bytes 1..4
  66 2e 0f 1f 84 e8 d0 d1 d2 d3    3. restore byte 0

uprobe_write() can fail with -ENOMEM, or with -EINTR when the task is being
killed. When the 2nd write fails the result of the 1st one stays. Byte 0 is
a breakpoint and it works. But the next set_orig_insn() doesn't find lea in
byte 0, takes it for a plain breakpoint and restores one byte:

  66 8d 64 24 80 e8 d0 d1 d2 d3    lea -0x80(%rsp),%sp; call tramp

Nothing recognizes these bytes anymore. The process keeps the call into
uprobe trampoline, which can be unmapped already.

Teach is_optimized() to recognize the half-way state, and let the 1st write
of int3_update_unoptimize() skip what is already done.

The failure wasn't reproduced, but the state can be made by hand. Attach to
nop10, call it to get it optimized, write 0xcc over byte 0, detach.

"uprobes: Don't sleep on mmap_lock under dup_mmap_sem" needs this. There
fork() removes the breakpoint from its mm. When fork() is killed in
the middle register_for_each_vma() removes it for the second time.

Fixes: 554ba38456da ("uprobes/x86: Move optimized uprobe from nop5 to nop10")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 arch/x86/kernel/uprobes.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 65a2de82ecd2..62bbe41c6466 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -896,6 +896,9 @@ static int verify_insn(struct page *page, unsigned long vaddr, uprobe_opcode_t *
 	case EXPECT_OPTIMIZED:
 		if (is_opt_insns(&old_opcode[0]))
 			return 1;
+		/* An earlier attempt got this far and failed, nothing to write. */
+		if (is_swbp_opt_insns(&old_opcode[0]))
+			return 0;
 		break;
 	case EXPECT_SWBP_OPTIMIZED:
 		if (is_swbp_opt_insns(&old_opcode[0]))
@@ -1109,6 +1112,17 @@ static bool __is_optimized(struct mm_struct *mm, uprobe_opcode_t *insn, unsigned
 		s32 raddr;
 	} *call = (struct __arch_relative_insn *)(insn + LEA_INSN_SIZE);
 
+	/*
+	 * INT3 followed by the rest of the optimized instructions is what is
+	 * left if int3_update_optimize() or int3_update_unoptimize() failed
+	 * half-way. It works as a breakpoint, but to restore the original
+	 * instruction it has to be unoptimized like a completely optimized one.
+	 * The same has to be done if a debugger has put its INT3 on top of an
+	 * optimized uprobe. The trampoline can be gone already, so do not look
+	 * at the target of the call.
+	 */
+	if (is_swbp_opt_insns(insn))
+		return true;
 	if (!is_opt_insns(insn))
 		return false;
 	return __in_uprobe_trampoline(mm, vaddr + OPT_INSN_SIZE + call->raddr);
-- 
2.55.0

[PATCH bpf-next 4/9] uprobes/x86: Don't WARN if an optimized probe can't be written

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

__arch_uprobe_optimize() and set_orig_insn() do WARN_ON_ONCE() when
uprobe_write() fails, and user space can make it fail:
- -EINTR from get_user_pages_remote() when current is being killed.
- -ENOMEM when COW can't be broken, since memcg is at its limit.
- -EINVAL from __update_ref_ctr() when the process cleared the semaphore
  of SDT probe.

Kill the processes that call probed nop10. It warns in 8 of 8 runs:

  WARNING: arch/x86/kernel/uprobes.c:1186 at arch_uprobe_optimize+0x291/0x3a0
   uprobe_notify_resume+0x1c3/0x4c0
   exit_to_user_mode_loop+0x63/0x4e0
   exc_int3+0x1f3/0x250

The next patch calls set_orig_insn() from fork(). Kill the processes that
fork and keep mmap_lock busy, while a trace event on nop10 is enabled and
disabled. The kernel with the next two patches and without this one warns
in 3 of 3 runs. The whole series in 0 of 3:

  WARNING: arch/x86/kernel/uprobes.c:1161 at set_orig_insn+0x161/0x170
   register_one_mm+0x154/0x180
   uprobe_dup_mmap+0x61/0xb0
   dup_mmap+0x149/0x9f0

The callers handle the error. arch_uprobe_optimize() marks uprobe as not
optimizable and byte 0 stays a breakpoint. set_orig_insn() returns
the error. Drop both WARNs.

This depends on the previous patch. Without it WARN was the only sign of
the bytes that set_orig_insn() cannot restore. Hence no Fixes tag.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 arch/x86/kernel/uprobes.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 62bbe41c6466..33c35bf07801 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -1170,11 +1170,8 @@ int set_orig_insn(struct arch_uprobe *auprobe, struct vm_area_struct *vma,
 		int ret = is_optimized(vma->vm_mm, vaddr);
 		if (ret < 0)
 			return ret;
-		if (ret) {
-			ret = swbp_unoptimize(auprobe, vma, vaddr);
-			WARN_ON_ONCE(ret);
-			return ret;
-		}
+		if (ret)
+			return swbp_unoptimize(auprobe, vma, vaddr);
 	}
 	return uprobe_write_opcode(auprobe, vma, vaddr, *(uprobe_opcode_t *)&auprobe->insn,
 				   false /* is_register */);
@@ -1197,7 +1194,7 @@ static int __arch_uprobe_optimize(struct arch_uprobe *auprobe, struct mm_struct
 	if (IS_ERR(tramp))
 		return PTR_ERR(tramp);
 	ret = swbp_optimize(auprobe, vma, vaddr, tramp->vm_start);
-	if (WARN_ON_ONCE(ret) && new_mapping)
+	if (ret && new_mapping)
 		WARN_ON_ONCE(do_munmap(mm, tramp->vm_start, PAGE_SIZE, NULL));
 	return ret;
 }
-- 
2.55.0

[PATCH bpf-next 5/9] uprobes: Don't sleep on mmap_lock under dup_mmap_sem

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

register_for_each_vma() takes dup_mmap_sem for writing, collects mms that
map the file and takes mmap_lock of every mm. fork() takes dup_mmap_sem for
reading. When mmap_lock of one mm is busy every fork() in the system waits
for it:

  A:       mmap(MAP_POPULATE) -> mmap_read_lock(A)
  tracer:  percpu_down_write(&dup_mmap_sem), mmap_write_lock(A) sleeps
  others:  fork() -> percpu_down_read(&dup_mmap_sem) sleeps

Only trylock mmap_lock under dup_mmap_sem. Leave mms with busy mmap_lock in
the list, drop dup_mmap_sem and then wait for them.

dup_mmap_sem makes sure that mm doesn't fork between build_map_info() and
install_breakpoint() or remove_breakpoint(). Otherwise the child misses
the breakpoint, or inherits the breakpoint of uprobe that is gone and gets
SIGTRAP. Now mm in the list can fork. dup_mmap() holds mmap_lock of
the parent, so uprobe_dup_mmap() does what the walk is going to do to this
mm before vmas are copied. The walk sees info->done and skips it.

The error in uprobe_dup_mmap() is the error of the walk, unless fork() is
being killed. Such fork() fails in copy_process() and there is no child, so
leave mm to the walk. With ref_ctr_offset the walk can update the counter
twice in this case.

dup_mmap_sem protects the list of walks. mm in the list is kept by
mm_count. The walk takes mm_users only for the visit, so the process that
exits meanwhile frees its memory.

Busy mms are visited last. The tracer that is killed while it waits for one
of them has installed the breakpoint in all other mms and cannot remove it.
"uprobes: Don't leak the breakpoints if the unregistering task is killed"
fixes that.

Process A loops over mmap(4G, MAP_POPULATE) and munmap(). A and 10 idle
processes map 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  197 (209)   0.33 (0.70)       210    233
 uprobe_multi, 4 uprobes 421 (429)   0.38 (0.60)      1467   1532
 perf_event, 4 uprobes   422 (439)   0.43 (0.59)      3135   3289

During detach: 264-423 -> 0.35-0.47. Attach and detach still wait for
mmap_lock of A. B doesn't.

The tracer pays when traced processes fork all the time. mm that forks
holds its mmap_lock, so the walk sleeps on many mms in turn. Fork storms of
1200, 2000 and 5000 processes, 6 cpu VM, msec, without and with the series:

 enable uprobe event    105 -> 205   201 -> 481   207 -> 784
 disable uprobe event   225 -> 294   354 -> 579   407 -> 893

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/events/uprobes.c | 178 ++++++++++++++++++++++++++++++++--------
 1 file changed, 143 insertions(+), 35 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 6792004be6fa..a9274016c55d 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1192,8 +1192,29 @@ struct map_info {
 	struct map_info *next;
 	struct mm_struct *mm;
 	unsigned long vaddr;
+	bool done;
 };
 
+/*
+ * Every fork() waits for dup_mmap_sem, so register_for_each_vma() must not
+ * sleep on mmap_lock while it holds dup_mmap_sem. It leaves mms with busy
+ * mmap_lock in ->busy, drops dup_mmap_sem, and only then waits for them.
+ *
+ * Such mm can fork before its turn comes. dup_mmap() holds its mmap_lock, so
+ * uprobe_dup_mmap() does what the walk is going to do to this mm, and
+ * the child has the same breakpoints as the child of mm that wasn't busy.
+ */
+struct uprobe_walk {
+	struct list_head list;
+	struct uprobe *uprobe;
+	struct uprobe_consumer *new;
+	struct map_info *busy;
+	int err;
+};
+
+/* dup_mmap_sem protects the list and ->busy of every walk in it */
+static LIST_HEAD(uprobe_walks);
+
 static inline struct map_info *free_map_info(struct map_info *info)
 {
 	struct map_info *next = info->next;
@@ -1242,6 +1263,7 @@ build_map_info(struct address_space *mapping, loff_t offset, bool is_register)
 
 		info->mm = vma->vm_mm;
 		info->vaddr = offset_to_vaddr(vma, offset);
+		info->done = false;
 	}
 	i_mmap_unlock_read(mapping);
 
@@ -1271,26 +1293,79 @@ build_map_info(struct address_space *mapping, loff_t offset, bool is_register)
 	return curr;
 }
 
-static int
-register_for_each_vma(struct uprobe *uprobe, struct uprobe_consumer *new)
+/*
+ * mmap_lock of info->mm is held for writing. info->done is set under it.
+ * The walk also tests it without mmap_lock, to skip mm.
+ */
+static void register_one_mm(struct uprobe_walk *walk, struct map_info *info,
+			    bool in_fork)
 {
+	struct uprobe_consumer *new = walk->new;
+	struct uprobe *uprobe = walk->uprobe;
+	struct mm_struct *mm = info->mm;
+	struct vm_area_struct *vma;
 	bool is_register = !!new;
-	struct map_info *info;
 	int err = 0;
 
+	if (info->done)
+		return;
+
+	if (is_register && READ_ONCE(walk->err))
+		goto done;
+
+	if (check_stable_address_space(mm))
+		goto done;
+
+	vma = find_vma(mm, info->vaddr);
+	if (!vma || !valid_vma(vma, is_register) ||
+	    file_inode(vma->vm_file) != uprobe->inode)
+		goto done;
+
+	if (vma->vm_start > info->vaddr ||
+	    vaddr_to_offset(vma, info->vaddr) != uprobe->offset)
+		goto done;
+
+	if (is_register) {
+		/* consult only the "caller", new consumer. */
+		if (consumer_filter(new, mm))
+			err = install_breakpoint(uprobe, vma, info->vaddr);
+	} else if (mm_flags_test(MMF_HAS_UPROBES, mm)) {
+		if (!filter_chain(uprobe, mm))
+			err = remove_breakpoint(uprobe, vma, info->vaddr);
+	}
+
+	if (err) {
+		/*
+		 * fork() was killed meanwhile. This is not the error of
+		 * the walk. See uprobe_dup_mmap().
+		 */
+		if (in_fork && fatal_signal_pending(current))
+			return;
+		WRITE_ONCE(walk->err, err);
+	}
+ done:
+	WRITE_ONCE(info->done, true);
+}
+
+static int
+register_for_each_vma(struct uprobe *uprobe, struct uprobe_consumer *new)
+{
+	struct uprobe_walk walk = { .uprobe = uprobe, .new = new };
+	struct map_info *info, *next;
+
 	percpu_down_write(&dup_mmap_sem);
-	info = build_map_info(uprobe->inode->i_mapping,
-					uprobe->offset, is_register);
+	info = build_map_info(uprobe->inode->i_mapping, uprobe->offset, !!new);
 	if (IS_ERR(info)) {
-		err = PTR_ERR(info);
-		goto out;
+		percpu_up_write(&dup_mmap_sem);
+		return PTR_ERR(info);
 	}
 
-	while (info) {
+	for (; info; info = next) {
 		struct mm_struct *mm = info->mm;
-		struct vm_area_struct *vma;
 
-		if (err && is_register)
+		next = info->next;
+		/* If install failed the caller is going to undo it */
+		if (new && walk.err)
 			goto free;
 		/*
 		 * We take mmap_lock for writing to avoid the race with
@@ -1299,38 +1374,56 @@ register_for_each_vma(struct uprobe *uprobe, struct uprobe_consumer *new)
 		 * is_trap_at_addr() true right after find_uprobe_rcu()
 		 * returns NULL in find_active_uprobe_rcu().
 		 */
-		mmap_write_lock(mm);
-		if (check_stable_address_space(mm))
-			goto unlock;
-
-		vma = find_vma(mm, info->vaddr);
-		if (!vma || !valid_vma(vma, is_register) ||
-		    file_inode(vma->vm_file) != uprobe->inode)
-			goto unlock;
-
-		if (vma->vm_start > info->vaddr ||
-		    vaddr_to_offset(vma, info->vaddr) != uprobe->offset)
-			goto unlock;
-
-		if (is_register) {
-			/* consult only the "caller", new consumer. */
-			if (consumer_filter(new, mm))
-				err = install_breakpoint(uprobe, vma, info->vaddr);
-		} else if (mm_flags_test(MMF_HAS_UPROBES, mm)) {
-			if (!filter_chain(uprobe, mm))
-				err |= remove_breakpoint(uprobe, vma, info->vaddr);
+		if (!mmap_write_trylock(mm)) {
+			/*
+			 * uprobe_dup_mmap() compares info->mm until list_del().
+			 * Keep mm_struct, but not the memory of the process.
+			 */
+			mmgrab(mm);
+			mmput_async(mm);
+			info->next = walk.busy;
+			walk.busy = info;
+			continue;
 		}
-
- unlock:
+		register_one_mm(&walk, info, false);
 		mmap_write_unlock(mm);
  free:
 		/* exit_mmap() of the last user must not run under dup_mmap_sem */
 		mmput_async(mm);
-		info = free_map_info(info);
+		kfree(info);
 	}
- out:
+
+	if (!walk.busy) {
+		percpu_up_write(&dup_mmap_sem);
+		return walk.err;
+	}
+
+	list_add(&walk.list, &uprobe_walks);
 	percpu_up_write(&dup_mmap_sem);
-	return err;
+
+	for (info = walk.busy; info; info = info->next) {
+		struct mm_struct *mm = info->mm;
+
+		/* Don't wait for mm if its fork() did it or if install failed */
+		if (READ_ONCE(info->done) || (new && READ_ONCE(walk.err)))
+			continue;
+		/* All users are gone, like in build_map_info() */
+		if (!mmget_not_zero(mm))
+			continue;
+		mmap_write_lock(mm);
+		register_one_mm(&walk, info, false);
+		mmap_write_unlock(mm);
+		mmput(mm);
+	}
+
+	percpu_down_write(&dup_mmap_sem);
+	list_del(&walk.list);
+	percpu_up_write(&dup_mmap_sem);
+
+	for (info = walk.busy; info; info = free_map_info(info))
+		mmdrop(info->mm);
+
+	return walk.err;
 }
 
 /**
@@ -1841,6 +1934,21 @@ void uprobe_end_dup_mmap(void)
 
 void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm)
 {
+	struct uprobe_walk *walk;
+	struct map_info *info;
+
+	list_for_each_entry(walk, &uprobe_walks, list) {
+		/*
+		 * fork() that is being killed fails in copy_process().
+		 * There will be no child. Leave oldmm to the walk.
+		 */
+		if (fatal_signal_pending(current))
+			break;
+		for (info = walk->busy; info; info = info->next)
+			if (info->mm == oldmm)
+				register_one_mm(walk, info, true);
+	}
+
 	if (mm_flags_test(MMF_HAS_UPROBES, oldmm)) {
 		mm_flags_set(MMF_HAS_UPROBES, newmm);
 		/* unconditionally, dup_mmap() skips VM_DONTCOPY vmas */
-- 
2.55.0

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

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

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@kernel.org>
---
 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

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

[Alexei Starovoitov]

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>
---
 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);
 	/* 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);
+	}
 	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

[PATCH bpf-next 8/9] uprobes: Don't leak the breakpoints if the unregistering task is killed

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

__get_user_pages() returns -EINTR when current has a fatal signal pending,
and uprobe_write() begins with get_user_pages_remote(). When the task is
killed in uprobe_unregister_nosync() remove_breakpoint() fails in every mm
that register_for_each_vma() hasn't visited yet:

  uprobe: <comm>:<pid> failed to unregister, leaking uprobe

uprobe stays in uprobes_tree. The breakpoints stay in these processes and
in their children, and every hit traps into the kernel for nothing.

The same happens when the task is killed in uprobe_register(). It cannot
remove the breakpoints that it installed. Since "uprobes: Don't sleep on
mmap_lock under dup_mmap_sem" busy mms come last, so the task that is
killed while it waits for one of them leaves the breakpoint in all other
mms.

Trying again after the failure isn't enough. The signal can interrupt
the removal after ref_ctr was decremented, and the second removal
decrements it again.

kworker has no signals. Remove the breakpoints from a work item and wait
for it. A kernel thread does the walk directly, and so does everybody when
nobody maps the file.

The price:
- A round trip through the workqueue for every removal, 9 usec.
- The removal runs with the priority and cpu mask of unbound kworker.
- The caller sleeps in flush_work() for the whole walk. When the walk is
  longer than hung_task_timeout_secs the caller is reported as hung.

The tracer is killed while uprobe_multi attach waits for mmap_lock of
a busy process. 100 idle processes map the file. The number of them that
keep the breakpoint, 10 rounds:

  without this series   0 or 1
  without this patch    100
  with this patch       0

uprobe_multi attach and detach in a loop, nobody is killed, 32 cpu VM,
median of 21-27 runs, msec:

                                  detach          attach
                               before  after   before  after
  4096 uprobes, 1 process        33.5   69.4     7.35   7.44
  1024 uprobes, 100 processes    95.0    102     38.1   43.4
  256 uprobes, 1000 processes     182    192      107    117

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/events/uprobes.c | 74 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 71 insertions(+), 3 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 92ccb730278f..361b52c9c3bc 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1426,6 +1426,68 @@ register_for_each_vma(struct uprobe *uprobe, struct uprobe_consumer *new)
 	return walk.err;
 }
 
+struct unregister_work {
+	struct work_struct work;
+	struct uprobe *uprobe;
+	int err;
+};
+
+static void unregister_work_fn(struct work_struct *work)
+{
+	struct unregister_work *uw = container_of(work, struct unregister_work, work);
+
+	uw->err = register_for_each_vma(uw->uprobe, NULL);
+}
+
+/*
+ * Remove the breakpoints. If current is killed, get_user_pages_remote() fails
+ * with -EINTR from then on: the breakpoints which are left stay for good, and
+ * the removal which was interrupted can be left half-way, with the reference
+ * counter decremented but the breakpoint still there. So let a kworker do the
+ * job, it has no signals. It takes the locks we would take ourselves.
+ * (A fork() of an mm with busy mmap_lock can still do one removal in a task
+ * which is being killed, see register_one_mm().)
+ *
+ * A kernel thread can do it itself. So can anybody if nobody maps the file:
+ * then there is no breakpoint, and uprobe_mmap() installs none which we would
+ * have to remove.
+ */
+static int unregister_all_vmas(struct uprobe *uprobe)
+{
+	struct address_space *mapping = uprobe->inode->i_mapping;
+	struct unregister_work uw = { .uprobe = uprobe };
+	bool mapped;
+	int err;
+
+	/*
+	 * vma_prepare() takes a vma out of the tree and vma_complete() puts it
+	 * back, under i_mmap_lock_write(). Don't mistake that for an empty tree.
+	 */
+	i_mmap_lock_read(mapping);
+	mapped = mapping_mapped(mapping);
+	i_mmap_unlock_read(mapping);
+
+	if (!mapped || (current->flags & PF_KTHREAD)) {
+		err = register_for_each_vma(uprobe, NULL);
+		/*
+		 * An error with a fatal signal pending: the file was mapped
+		 * meanwhile, and get_user_pages_remote() returned -EINTR before
+		 * it could see that there is nothing to remove. Nothing was
+		 * written, so it is safe to let the kworker repeat the walk.
+		 * (Kernel threads which unregister uprobes don't allow SIGKILL.)
+		 */
+		if (likely(!err) || !fatal_signal_pending(current))
+			return err;
+	}
+
+	INIT_WORK_ONSTACK(&uw.work, unregister_work_fn);
+	queue_work(system_dfl_long_wq, &uw.work);
+	flush_work(&uw.work);
+	destroy_work_on_stack(&uw.work);
+
+	return uw.err;
+}
+
 /**
  * uprobe_unregister_nosync - unregister an already registered probe.
  * @uprobe: uprobe to remove
@@ -1437,10 +1499,13 @@ void uprobe_unregister_nosync(struct uprobe *uprobe, struct uprobe_consumer *uc)
 
 	down_write(&uprobe->register_rwsem);
 	consumer_del(uprobe, uc);
-	err = register_for_each_vma(uprobe, NULL);
+	err = unregister_all_vmas(uprobe);
 	up_write(&uprobe->register_rwsem);
 
-	/* TODO : cant unregister? schedule a worker thread */
+	/*
+	 * Can still fail, e.g. with -ENOMEM. Then the uprobe and the breakpoints
+	 * which are left are leaked.
+	 */
 	if (unlikely(err)) {
 		uprobe_warn(current, "unregister, leaking uprobe");
 		return;
@@ -1553,7 +1618,10 @@ int uprobe_apply(struct uprobe *uprobe, struct uprobe_consumer *uc, bool add)
 	rcu_read_lock_trace();
 	list_for_each_entry_rcu(con, &uprobe->consumers, cons_node, rcu_read_lock_trace_held()) {
 		if (con == uc) {
-			ret = register_for_each_vma(uprobe, add ? uc : NULL);
+			if (add)
+				ret = register_for_each_vma(uprobe, uc);
+			else
+				ret = unregister_all_vmas(uprobe);
 			break;
 		}
 	}
-- 
2.55.0

[PATCH bpf-next 9/9] selftests/bpf: Test uprobes and fork() with busy mmap_lock

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

A process keeps its mmap_lock busy with MADV_POPULATE_READ of a large
read-only anonymous area. It maps zero page, so it costs page tables only.

- fork_multi, fork_perf: the busy process forks while uprobe is attached
  and detached. When attach is over the child has the breakpoint. When
  detach is over the child doesn't have it, and can call the function.
- fork_not_blocked: fork() of another process returns while attach is
  waiting for mmap_lock of the busy process.
- attach_not_blocked: attach to the file that the busy process doesn't map
  returns while fork() of the busy process is waiting for its mmap_lock.

The last two compare the order of events, not the time they take.

The test is serial. The busy process maps test_progs, so it would delay
uprobe attaches of other workers.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 .../selftests/bpf/prog_tests/uprobe_busy_mm.c | 447 ++++++++++++++++++
 .../selftests/bpf/progs/uprobe_busy_mm.c      |  22 +
 2 files changed, 469 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe_busy_mm.c
 create mode 100644 tools/testing/selftests/bpf/progs/uprobe_busy_mm.c

diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_busy_mm.c b/tools/testing/selftests/bpf/prog_tests/uprobe_busy_mm.c
new file mode 100644
index 000000000000..30614e8b92cd
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_busy_mm.c
@@ -0,0 +1,447 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Attach and detach uprobes while a process keeps its mmap_lock busy.
+ * Other processes should be able to fork, and children of the busy process
+ * should have the same breakpoints as everybody else.
+ */
+#include <pthread.h>
+#include <sys/mman.h>
+#include <sys/prctl.h>
+#include <sys/wait.h>
+#include <test_progs.h>
+#include "bpf/libbpf_internal.h"
+#include "uprobe_busy_mm.skel.h"
+
+#ifndef MADV_POPULATE_READ
+#define MADV_POPULATE_READ 22
+#endif
+
+#define LOAD(x)		__atomic_load_n(&(x), __ATOMIC_SEQ_CST)
+#define STORE(x, v)	__atomic_store_n(&(x), v, __ATOMIC_SEQ_CST)
+#define INC(x)		__atomic_add_fetch(&(x), 1, __ATOMIC_SEQ_CST)
+
+#define AREA_MAX	(64UL << 30)
+#define AREA_MIN	(64UL << 20)
+#define MAX_CHILDREN	8
+#define MAX_TRIES	10
+#define TIMEOUT_NS	(30 * 1000000000ULL)
+
+struct shared {
+	/* written by the test */
+	int stop;
+	int gen;		/* odd while attach or detach is in progress */
+	int attached;
+	int hold_ms;
+	bool do_fork;
+	char insn[4];
+	/* written by the busy process */
+	int ready;
+	int hold_seq;		/* number of times mmap_lock was let go */
+	__u64 hold_ns;		/* for how long it was held the last time */
+	int in_hold;
+	int in_fork;
+	int forks;		/* number of times fork() returned */
+	int errors;
+	int checked[2];		/* children that checked their breakpoint, by ->attached */
+};
+
+static struct shared *sh;
+
+static noinline void uprobe_busy_mm_func(void)
+{
+	asm volatile ("");
+}
+
+static __u64 now_ns(void)
+{
+	struct timespec ts;
+
+	clock_gettime(CLOCK_MONOTONIC, &ts);
+	return ts.tv_sec * 1000000000ULL + ts.tv_nsec;
+}
+
+/*
+ * MADV_POPULATE_READ holds mmap_lock for reading while it maps zero page
+ * all over the area. It costs page tables only.
+ */
+static void *hold_mmap_lock(void *arg)
+{
+	size_t size = AREA_MIN;
+	void *area;
+	__u64 t;
+
+	area = mmap(NULL, AREA_MAX, PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0);
+	if (area == MAP_FAILED)
+		goto err;
+	/* EINVAL without CONFIG_TRANSPARENT_HUGEPAGE */
+	madvise(area, AREA_MAX, MADV_NOHUGEPAGE);
+
+	t = now_ns();
+	if (madvise(area, size, MADV_POPULATE_READ))
+		goto err;
+	t = now_ns() - t;
+	size *= sh->hold_ms * 1000000ULL / t + 1;
+	if (size > AREA_MAX)
+		size = AREA_MAX;
+
+	while (!LOAD(sh->stop)) {
+		STORE(sh->in_hold, 1);
+		t = now_ns();
+		madvise(area, size, MADV_POPULATE_READ);
+		/* before anything that can fault and wait for fork() */
+		INC(sh->hold_seq);
+		STORE(sh->hold_ns, now_ns() - t);
+		STORE(sh->in_hold, 0);
+		madvise(area, size, MADV_DONTNEED);
+		STORE(sh->ready, 1);
+	}
+	return NULL;
+err:
+	INC(sh->errors);
+	STORE(sh->ready, 1);
+	return NULL;
+}
+
+/*
+ * fork() began while attach or detach was in progress.
+ * When it's over the child should be like other processes.
+ */
+static int child(int gen)
+{
+	bool attached, bp;
+
+	prctl(PR_SET_PDEATHSIG, SIGKILL);
+	if (!(gen & 1))
+		return 0;
+	gen++;
+
+	while (LOAD(sh->gen) < gen) {
+		if (LOAD(sh->stop))
+			return 0;
+		usleep(100);
+	}
+
+	attached = LOAD(sh->attached);
+	bp = memcmp((void *)uprobe_busy_mm_func, sh->insn, sizeof(sh->insn));
+	if (LOAD(sh->gen) != gen)
+		return 0;
+	if (bp != attached)
+		return 1;
+
+	/* SIGTRAP when there is a breakpoint without uprobe */
+	uprobe_busy_mm_func();
+	if (LOAD(sh->gen) == gen)
+		INC(sh->checked[attached]);
+	return 0;
+}
+
+static void busy_process(void)
+{
+	int status, gen, nr = 0;
+	pthread_t thread;
+	pid_t pid;
+
+	prctl(PR_SET_PDEATHSIG, SIGKILL);
+	if (pthread_create(&thread, NULL, hold_mmap_lock, NULL)) {
+		INC(sh->errors);
+		STORE(sh->ready, 1);
+		_exit(1);
+	}
+
+	while (sh->do_fork && !LOAD(sh->stop)) {
+		while (nr && waitpid(-1, &status, nr < MAX_CHILDREN ? WNOHANG : 0) > 0) {
+			if (status)
+				INC(sh->errors);
+			nr--;
+		}
+
+		gen = LOAD(sh->gen);
+		STORE(sh->in_fork, 1);
+		pid = fork();
+		if (!pid)
+			_exit(child(gen));
+		INC(sh->forks);
+		STORE(sh->in_fork, 0);
+		if (pid < 0) {
+			INC(sh->errors);
+			break;
+		}
+		nr++;
+	}
+
+	while (nr-- && wait(&status) > 0)
+		if (status)
+			INC(sh->errors);
+	pthread_join(thread, NULL);
+	_exit(0);
+}
+
+/* The busy process gave up or is gone */
+static bool busy_process_failed(pid_t pid)
+{
+	siginfo_t si = {};
+
+	return LOAD(sh->errors) ||
+	       waitid(P_PID, pid, &si, WEXITED | WNOHANG | WNOWAIT) || si.si_pid;
+}
+
+static pid_t start_busy_process(int hold_ms, bool do_fork)
+{
+	__u64 t = now_ns();
+	pid_t pid;
+
+	sh = mmap(NULL, sizeof(*sh), PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0);
+	if (!ASSERT_NEQ(sh, MAP_FAILED, "mmap"))
+		return -1;
+
+	sh->hold_ms = hold_ms;
+	sh->do_fork = do_fork;
+	memcpy(sh->insn, (void *)uprobe_busy_mm_func, sizeof(sh->insn));
+
+	pid = fork();
+	if (!ASSERT_GE(pid, 0, "fork")) {
+		munmap(sh, sizeof(*sh));
+		return -1;
+	}
+	if (!pid)
+		busy_process();
+
+	while (!LOAD(sh->ready) && !busy_process_failed(pid) && now_ns() - t < TIMEOUT_NS)
+		usleep(1000);
+	if (!ASSERT_TRUE(LOAD(sh->ready), "busy_process_ready")) {
+		kill(pid, SIGKILL);
+		waitpid(pid, NULL, 0);
+		munmap(sh, sizeof(*sh));
+		return -1;
+	}
+	return pid;
+}
+
+static void stop_busy_process(pid_t pid)
+{
+	int status = -1;
+
+	STORE(sh->stop, 1);
+	waitpid(pid, &status, 0);
+	ASSERT_EQ(status, 0, "busy_process_status");
+	ASSERT_EQ(sh->errors, 0, "errors");
+	munmap(sh, sizeof(*sh));
+}
+
+/* Returns hold_seq of the hold that has just begun, -1 if there won't be one */
+static int wait_for_hold(pid_t pid, bool in_fork)
+{
+	__u64 t = now_ns();
+	int seq;
+
+	while (!busy_process_failed(pid) && now_ns() - t < TIMEOUT_NS) {
+		if (!LOAD(sh->in_hold) || (in_fork && !LOAD(sh->in_fork))) {
+			usleep(100);
+			continue;
+		}
+		seq = LOAD(sh->hold_seq);
+		/* let them get into the kernel */
+		usleep(10000);
+		if (LOAD(sh->hold_seq) == seq && (!in_fork || LOAD(sh->in_fork)))
+			return seq;
+	}
+
+	return -1;
+}
+
+static struct bpf_link *attach(struct uprobe_busy_mm *skel, bool multi,
+			       const char *path, unsigned long offset)
+{
+	LIBBPF_OPTS(bpf_uprobe_multi_opts, opts, .offsets = &offset, .cnt = 1);
+
+	if (multi)
+		return bpf_program__attach_uprobe_multi(skel->progs.probe_multi, -1, path,
+							NULL, &opts);
+	return bpf_program__attach_uprobe(skel->progs.probe_perf, false, -1, path, offset);
+}
+
+/*
+ * Wait for a child to check its breakpoint in this state. fork() that began
+ * during attach or detach may sleep until the next hold is over.
+ */
+static void wait_for_check(int attached, int checked)
+{
+	__u64 t = now_ns(), max = 2 * LOAD(sh->hold_ns) + 100 * 1000000ULL;
+
+	while (LOAD(sh->checked[attached]) == checked && now_ns() - t < max)
+		usleep(1000);
+}
+
+static void test_fork(struct uprobe_busy_mm *skel, bool multi)
+{
+	struct bpf_link *link;
+	ssize_t offset;
+	int i, checked;
+	pid_t pid;
+	__u64 t;
+
+	offset = get_uprobe_offset(&uprobe_busy_mm_func);
+	if (!ASSERT_GE(offset, 0, "get_uprobe_offset"))
+		return;
+
+	pid = start_busy_process(20, true);
+	if (pid < 0)
+		return;
+	skel->bss->hits = 0;
+	t = now_ns();
+
+	for (i = 0; !busy_process_failed(pid) && now_ns() - t < TIMEOUT_NS; i++) {
+		if (i >= 10 && LOAD(sh->checked[0]) && LOAD(sh->checked[1]))
+			break;
+
+		checked = LOAD(sh->checked[1]);
+		INC(sh->gen);
+		link = attach(skel, multi, "/proc/self/exe", offset);
+		if (!ASSERT_OK_PTR(link, "attach"))
+			break;
+		STORE(sh->attached, 1);
+		INC(sh->gen);
+		wait_for_check(1, checked);
+
+		checked = LOAD(sh->checked[0]);
+		INC(sh->gen);
+		bpf_link__destroy(link);
+		STORE(sh->attached, 0);
+		INC(sh->gen);
+		wait_for_check(0, checked);
+	}
+
+	ASSERT_GT(LOAD(sh->checked[1]), 0, "checked_attached");
+	ASSERT_GT(LOAD(sh->checked[0]), 0, "checked_detached");
+	ASSERT_GE(skel->bss->hits, LOAD(sh->checked[1]), "hits");
+	stop_busy_process(pid);
+}
+
+struct attach_args {
+	struct uprobe_busy_mm *skel;
+	unsigned long offset;
+	struct bpf_link *link;
+	int done;
+};
+
+static void *attach_thread(void *arg)
+{
+	struct attach_args *args = arg;
+
+	args->link = attach(args->skel, true, "/proc/self/exe", args->offset);
+	STORE(args->done, 1);
+	return NULL;
+}
+
+/* fork() doesn't wait for attach that waits for mmap_lock of the busy process */
+static void test_fork_not_blocked(struct uprobe_busy_mm *skel)
+{
+	struct attach_args args = { .skel = skel };
+	pid_t pid, child_pid;
+	bool ok = false;
+	pthread_t thread;
+	ssize_t offset;
+	int i, seq;
+
+	offset = get_uprobe_offset(&uprobe_busy_mm_func);
+	if (!ASSERT_GE(offset, 0, "get_uprobe_offset"))
+		return;
+	args.offset = offset;
+
+	pid = start_busy_process(300, false);
+	if (pid < 0)
+		return;
+
+	for (i = 0; i < MAX_TRIES && !ok && !LOAD(sh->errors); i++) {
+		seq = wait_for_hold(pid, false);
+		if (!ASSERT_GE(seq, 0, "wait_for_hold"))
+			break;
+
+		args.done = 0;
+		if (!ASSERT_OK(pthread_create(&thread, NULL, attach_thread, &args),
+			       "pthread_create"))
+			break;
+		/* attach waits for RCU first, fork() isn't blocked by that */
+		usleep(LOAD(sh->hold_ns) / 2000);
+
+		child_pid = fork();
+		if (!child_pid)
+			_exit(0);
+		ok = child_pid > 0 && !LOAD(args.done) && LOAD(sh->hold_seq) == seq;
+		if (child_pid > 0)
+			waitpid(child_pid, NULL, 0);
+
+		pthread_join(thread, NULL);
+		if (!ASSERT_OK_PTR(args.link, "attach"))
+			break;
+		bpf_link__destroy(args.link);
+	}
+
+	ASSERT_TRUE(ok, "fork_not_blocked");
+	stop_busy_process(pid);
+}
+
+/* attach doesn't wait for fork() that waits for mmap_lock of the busy process */
+static void test_attach_not_blocked(struct uprobe_busy_mm *skel)
+{
+	const char *path = "./uprobe_multi", *sym = "uprobe_multi_func_00000";
+	unsigned long *offsets = NULL;
+	struct bpf_link *link;
+	bool ok = false;
+	int i, seq, forks;
+	pid_t pid;
+
+	if (!ASSERT_OK(elf_resolve_syms_offsets(path, 1, &sym, &offsets, STT_FUNC),
+		       "elf_resolve_syms_offsets"))
+		return;
+
+	pid = start_busy_process(300, true);
+	if (pid < 0)
+		goto out;
+
+	for (i = 0; i < MAX_TRIES && !ok && !LOAD(sh->errors); i++) {
+		seq = wait_for_hold(pid, true);
+		if (!ASSERT_GE(seq, 0, "wait_for_hold"))
+			break;
+
+		forks = LOAD(sh->forks);
+
+		link = attach(skel, true, path, offsets[0]);
+		/*
+		 * attach that waited for fork() returns as soon as fork() has
+		 * copied mm, maybe before the busy process bumps forks and
+		 * hold_seq. Give it a quarter of the hold to do that.
+		 */
+		usleep(LOAD(sh->hold_ns) / 4000);
+		ok = LOAD(sh->in_fork) && LOAD(sh->forks) == forks &&
+		     LOAD(sh->hold_seq) == seq;
+		if (!ASSERT_OK_PTR(link, "attach"))
+			break;
+		bpf_link__destroy(link);
+	}
+
+	ASSERT_TRUE(ok, "attach_not_blocked");
+	stop_busy_process(pid);
+out:
+	free(offsets);
+}
+
+void serial_test_uprobe_busy_mm(void)
+{
+	struct uprobe_busy_mm *skel;
+
+	skel = uprobe_busy_mm__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "uprobe_busy_mm__open_and_load"))
+		return;
+
+	if (test__start_subtest("fork_multi"))
+		test_fork(skel, true);
+	if (test__start_subtest("fork_perf"))
+		test_fork(skel, false);
+	if (test__start_subtest("fork_not_blocked"))
+		test_fork_not_blocked(skel);
+	if (test__start_subtest("attach_not_blocked"))
+		test_attach_not_blocked(skel);
+
+	uprobe_busy_mm__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/uprobe_busy_mm.c b/tools/testing/selftests/bpf/progs/uprobe_busy_mm.c
new file mode 100644
index 000000000000..c07a9961db13
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/uprobe_busy_mm.c
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+__u64 hits;
+
+SEC("uprobe.multi")
+int probe_multi(struct pt_regs *ctx)
+{
+	__sync_fetch_and_add(&hits, 1);
+	return 0;
+}
+
+SEC("uprobe")
+int probe_perf(struct pt_regs *ctx)
+{
+	__sync_fetch_and_add(&hits, 1);
+	return 0;
+}
-- 
2.55.0

Re: [PATCH bpf-next 4/9] uprobes/x86: Don't WARN if an optimized probe can't be written

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 3100 bytes --]

> uprobes/x86: Don't WARN if an optimized probe can't be written

TL;DR: Commit message only: it says all callers handle the error, but
handler_chain() ignores a failed unoptimize on UPROBE_HANDLER_REMOVE, so with
the WARN dropped that failure goes unreported.

>     uprobes/x86: Don't WARN if an optimized probe can't be written
>
>     __arch_uprobe_optimize() and set_orig_insn() do WARN_ON_ONCE() when
>     uprobe_write() fails, and user space can make it fail:
>     - -EINTR from get_user_pages_remote() when current is being killed.
>     - -ENOMEM when COW can't be broken, since memcg is at its limit.
>     - -EINVAL from __update_ref_ctr() when the process cleared the semaphore
>       of SDT probe.
>
>     Kill the processes that call probed nop10. It warns in 8 of 8 runs:
>
>       WARNING: arch/x86/kernel/uprobes.c:1186 at arch_uprobe_optimize+0x291/0x3a0
>        uprobe_notify_resume+0x1c3/0x4c0
>        exit_to_user_mode_loop+0x63/0x4e0
>        exc_int3+0x1f3/0x250
>
>     The next patch calls set_orig_insn() from fork(). Kill the processes that
>     fork and keep mmap_lock busy, while a trace event on nop10 is enabled and
>     disabled. The kernel with the next two patches and without this one warns
>     in 3 of 3 runs. The whole series in 0 of 3:
>
>       WARNING: arch/x86/kernel/uprobes.c:1161 at set_orig_insn+0x161/0x170
>        register_one_mm+0x154/0x180
>        uprobe_dup_mmap+0x61/0xb0
>        dup_mmap+0x149/0x9f0
>
>     The callers handle the error. arch_uprobe_optimize() marks uprobe as not
>     optimizable and byte 0 stays a breakpoint. set_orig_insn() returns
>     the error. Drop both WARNs.
>
>     This depends on the previous patch. Without it WARN was the only sign of
>     the bytes that set_orig_insn() cannot restore. Hence no Fixes tag.
>
>     Signed-off-by: Alexei Starovoitov <ast@kernel.org>

This isn't a bug, but the changelog says "The callers handle the error".
Is that accurate for every caller of set_orig_insn()?

set_orig_insn() is reached through remove_breakpoint(), and one of its
callers drops the result:

kernel/events/uprobes.c:handler_chain() {
    ...
        if (!filter_chain(uprobe, current->mm)) {
            WARN_ON(!uprobe_is_active(uprobe));
            unapply_uprobe(uprobe, current->mm);
        }
    ...
}

unapply_uprobe() ORs the remove_breakpoint() result into its return value,
but handler_chain() ignores it.  So if a consumer returns
UPROBE_HANDLER_REMOVE on an optimized nop10 and swbp_unoptimize() fails
(-EINTR, -ENOMEM or -EINVAL as listed above), nothing is propagated or
logged on this path once the WARN_ON_ONCE() is gone.

The other path, register_for_each_vma(), does propagate the error.

Could the changelog say that handler_chain() treats the removal as
best-effort and ignores the result, instead of saying all callers handle the
error?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39710454342

Re: [PATCH bpf-next 9/9] selftests/bpf: Test uprobes and fork() with busy mmap_lock

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 1242 bytes --]

> selftests/bpf: Test uprobes and fork() with busy mmap_lock

TL;DR: Nit only, in the new uprobe_busy_mm selftest: the local now_ns() helper
duplicates the existing get_time_ns() from testing_helpers.h and could be
dropped.

> diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_busy_mm.c b/tools/testing/selftests/bpf/prog_tests/uprobe_busy_mm.c
> new file mode 100644
> index 000000000000..30614e8b92cd
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_busy_mm.c

[ ... ]

> +static __u64 now_ns(void)
> +{
> +	struct timespec ts;
> +
> +	clock_gettime(CLOCK_MONOTONIC, &ts);
> +	return ts.tv_sec * 1000000000ULL + ts.tv_nsec;
> +}

This isn't a bug, but could this use the existing get_time_ns() from
testing_helpers.h instead of a local now_ns() copy?

Both read CLOCK_MONOTONIC and return tv_sec * 1000000000 + tv_nsec as a
__u64.  test_progs.h already includes testing_helpers.h, so get_time_ns() is
in scope here, and now_ns() is called about ten times in this file.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39710454342

Re: [PATCH bpf-next 5/9] uprobes: Don't sleep on mmap_lock under dup_mmap_sem

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 3333 bytes --]

> uprobes: Don't sleep on mmap_lock under dup_mmap_sem

TL;DR: Question, possibly not a bug: uprobe_dup_mmap() edits oldmm page tables
inside dup_mmap() before vma_start_write(), so it may race with per-VMA read
lockers (page fault, MADV_DONTNEED); the review suspects it is safe.

> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c

[ ... ]

>  void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm)
>  {
> +	struct uprobe_walk *walk;
> +	struct map_info *info;
> +
> +	list_for_each_entry(walk, &uprobe_walks, list) {
> +		/*
> +		 * fork() that is being killed fails in copy_process().
> +		 * There will be no child. Leave oldmm to the walk.
> +		 */
> +		if (fatal_signal_pending(current))
> +			break;
> +		for (info = walk->busy; info; info = info->next)
> +			if (info->mm == oldmm)
> +				register_one_mm(walk, info, true);
> +	}
> +

A subsystem pattern flags this call as potentially concerning, so this may
not be a bug.

uprobe_dup_mmap() is called from dup_mmap() with mmap_write_lock_killable()
held on oldmm, and before the copy loop reaches vma_start_write_killable()
on each vma:

dup_mmap()
    mmap_write_lock_killable(oldmm)
    uprobe_dup_mmap(oldmm, mm)
        register_one_mm(walk, info, true)
            install_breakpoint() / remove_breakpoint()
                set_swbp() / set_orig_insn()
                    uprobe_write()
                        get_user_pages_remote()
                        folio_walk_start() + __uprobe_write()
                        collapse_pte_mapped_thp()   (unregister path)
                            try_collapse_pte_mapped_thp()
                                pmdp_collapse_flush() + pte_free_defer()
    ...
    vma_start_write_killable(mpnt)

The guide for the VMA locking code says to report functions that hold
mmap_write_lock and access page tables or PTE pages before calling
vma_start_write(vma), because other threads of the forking process may
still hold per-VMA read locks on the same vma, for example in a page fault
or in MADV_DONTNEED with CONFIG_PT_RECLAIM.

Can uprobe_write() and collapse_pte_mapped_thp() run against oldmm here
while another thread of the forking process holds a per-VMA read lock on
the same vma?

For context, the existing register_for_each_vma() -> register_one_mm()
walk already did this with only mmap_write_lock held.  What this patch adds
is a second caller of that code inside dup_mmap(), which did no page table
access on oldmm before vma_start_write().

On the other side, uprobe_write() takes the page table lock through
folio_walk_start() and retries when fw.page does not match the expected
page, and try_collapse_pte_mapped_thp() re-checks pmd_same() under the
locks before pmdp_collapse_flush().  khugepaged's retract_page_tables()
also retracts page tables without mmap_lock at all.  So it may be that
this access is safe without the per-VMA write lock.

Could you confirm whether the vma_start_write() rule is meant to apply to
the uprobe_write() path called from uprobe_dup_mmap()?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39710454342

Re: [PATCH bpf-next 3/9] uprobes/x86: Finish an unoptimization which failed half-way

[Gregory Price (Meta)]

On 2026-10-22 01:57 +0000, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> int3_update_unoptimize() restores optimized nop10 in three writes:
> 
>   48 8d 64 24 80 e8 d0 d1 d2 d3    lea -0x80(%rsp),%rsp; call tramp
>   cc 8d 64 24 80 e8 d0 d1 d2 d3    1. trap new entries
>   cc 2e 0f 1f 84 e8 d0 d1 d2 d3    2. restore bytes 1..4
>   66 2e 0f 1f 84 e8 d0 d1 d2 d3    3. restore byte 0
> 
> uprobe_write() can fail with -ENOMEM, or with -EINTR when the task is being
> killed. When the 2nd write fails the result of the 1st one stays. Byte 0 is
> a breakpoint and it works. But the next set_orig_insn() doesn't find lea in
> byte 0, takes it for a plain breakpoint and restores one byte:
> 
>   66 8d 64 24 80 e8 d0 d1 d2 d3    lea -0x80(%rsp),%sp; call tramp
> 
> Nothing recognizes these bytes anymore. The process keeps the call into
> uprobe trampoline, which can be unmapped already.
> 
> Teach is_optimized() to recognize the half-way state, and let the 1st write
> of int3_update_unoptimize() skip what is already done.
> 
> The failure wasn't reproduced, but the state can be made by hand. Attach to
> nop10, call it to get it optimized, write 0xcc over byte 0, detach.
> 
> "uprobes: Don't sleep on mmap_lock under dup_mmap_sem" needs this. There
> fork() removes the breakpoint from its mm. When fork() is killed in
> the middle register_for_each_vma() removes it for the second time.
> 
> Fixes: 554ba38456da ("uprobes/x86: Move optimized uprobe from nop5 to nop10")
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>

This was a nice read.

Reviewed-by: Gregory Price (Meta) <gourry@gourry.net>

-- 
Gregory Price (Meta) <gourry@gourry.net>