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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] uprobes: double ref_ctr decrement and breakpoint leak in unregister_all_vmas()
--

commit 02db19b4afb60ef4b8ca6aaa98c8fe074f02d199
Author: Alexei Starovoitov [off-list ref]

uprobes: Don't leak the breakpoints if the unregistering task is killed

This commit offloads breakpoint removal in uprobe_unregister_nosync() and
uprobe_apply() to an unbound workqueue item when the file is mapped by user
processes, preventing a fatal signal on the unregistering task from failing
get_user_pages_remote() with -EINTR and leaking breakpoints.

[Severity: Medium]
Can the !mapped path in unregister_all_vmas() still leave ref_ctr decremented
and then decrement it a second time when the kworker repeats the walk?

When !mapped is observed under i_mmap_lock_read(mapping), unregister_all_vmas()
drops the lock and calls register_for_each_vma(uprobe, NULL) in the killable
user task instead of returning 0 directly.

While i_mmap_lock_read(mapping) is dropped, a process can map the file.
uprobe_mmap() runs without uprobe->register_rwsem and installs the breakpoint
if any consumer in filter_chain(uprobe, vma->vm_mm) matches at that moment:

kernel/events/uprobes.c:uprobe_mmap() {
    ...
	list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
		if (!fatal_signal_pending(current) &&
		    filter_chain(uprobe, vma->vm_mm)) {
			unsigned long vaddr = offset_to_vaddr(vma, uprobe->offset);
			install_breakpoint(uprobe, vma, vaddr);
		}
		put_uprobe(uprobe);
	}
}

Before register_for_each_vma() -> register_one_mm() inspects that mm,
filter_chain(uprobe, mm) can become false without taking
uprobe->register_rwsem. For example, another thread can run
uprobe_perf_close(), which updates the filter before calling uprobe_apply():

kernel/trace/trace_uprobe.c:uprobe_perf_close() {
	tu = container_of(tp, struct trace_uprobe, tp);
	if (trace_uprobe_filter_remove(tu->tp.event->filter, event))
		return 0;

	list_for_each_entry(tu, trace_probe_probe_list(tp), tp.list) {
		ret = uprobe_apply(tu->uprobe, &tu->consumer, false);
		if (ret)
			break;
	}
    ...
}

Or the target thread of a per-task perf event in a multithreaded process can
exit and clear tsk->mm in exit_mm().

Then the first register_for_each_vma(uprobe, NULL) call in unregister_all_vmas()
sees !filter_chain(uprobe, mm) and calls remove_breakpoint() -> uprobe_write().
If the anonymous page is COW-shared (for instance, the process forked after
uprobe_mmap()), the initial pass without FOLL_WRITE decrements ref_ctr via
update_ref_ctr(uprobe, mm, -1), gets -EFAULT from __uprobe_write(), and loops
to retry with FOLL_WRITE:

kernel/events/uprobes.c:uprobe_write() {
    ...
	if (do_update_ref_ctr && !ref_ctr_updated && uprobe->ref_ctr_offset) {
		ret = update_ref_ctr(uprobe, mm, is_register ? 1 : -1);
		if (ret) {
			folio_put(folio);
			goto out;
		}

		ref_ctr_updated = 1;
	}
    ...
	switch (ret) {
	case -EFAULT:
		gup_flags |= FOLL_WRITE | FOLL_SPLIT_PMD;
		fallthrough;
	case -EAGAIN:
		goto retry;
	default:
		break;
	}

out:
	/* Revert back reference counter if instruction update failed. */
	if (do_update_ref_ctr && ret < 0 && ref_ctr_updated)
		update_ref_ctr(uprobe, mm, is_register ? -1 : 1);
}

If current now receives SIGKILL, both get_user_pages_remote() at retry and the
revert call in update_ref_ctr(uprobe, mm, 1) fail with -EINTR, leaving ref_ctr
decremented and the breakpoint still installed. When unregister_all_vmas()
falls through to queue uw.work, the kworker runs register_for_each_vma() again
and decrements ref_ctr a second time (or fails with -EINVAL if ref_ctr would
go negative, leaking the uprobe and breakpoint).

If !mapped is true under i_mmap_lock_read(mapping), could unregister_all_vmas()
return 0 immediately instead of calling register_for_each_vma(uprobe, NULL)?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022015712.1297331-1-alexei.starovoitov@gmail.com?part=8