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

[sashiko-bot@kernel.org]

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

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.

> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 92ccb730278f3..361b52c9c3bcf 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;
> +	}

[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