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

[bot+bpf-ci@kernel.org]

[-- 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