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 <[email protected]> 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/[email protected]?part=8
