Re: [RFC PATCH 3/4] tracing: Add disable_bpf trace option to ignore eBPF for fetchargs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The `disable_bpf` trace option is improperly exposed to instance directories and non-BPF builds, creating a dead configuration knob that silently ignores user input.

Pre-existing issues:
- [High] The `IS_ERR()` check on the return value of `bpf_prog_select_runtime()` is broken, leading to a silent failure and an uninitialized memory leak to user-space.
- [High] An early `BPF_EXIT_INSN()` generated for memory fetch failures aborts the entire probe fetch program, leaking uninitialized memory for all subsequent arguments.
--

commit 87050ba78ea1f240fee455c53d7eb225ed51e533
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

tracing: Add disable_bpf trace option to ignore eBPF for fetchargs
    
Add a trace option "disable_bpf" to disable BPF execution for fetchargs,
forcing the execution to fallback to the interpreter loop. This is useful
for evaluating BPF compilation performance impact.

> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 80fe152af1dd8..bf83680e0ba70 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -1503,6 +1503,7 @@ extern int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
>  		C(PAUSE_ON_TRACE,	"pause-on-trace"),	\
>  		C(HASH_PTR,		"hash-ptr"),	/* Print hashed pointer */ \
>  		C(BITMASK_LIST,		"bitmask-list"),	\
> +		C(DISABLE_BPF,		"disable_bpf"),		\

[Severity: Medium]
Does this option need an #ifdef CONFIG_BPF_SYSCALL wrapper? It appears this
unconditionally adds the flag, making it visible even on kernels built without
BPF support.

Additionally, since trace_probe_bpf_disabled() strictly checks
global_trace.trace_flags, toggling this option inside a local trace instance
directory won't have any effect. Should this be added to
TOP_LEVEL_TRACE_FLAGS in kernel/trace/trace.c to restrict it to the global
instance?

>  		FUNCTION_FLAGS					\
>  		FGRAPH_FLAGS					\
>  		STACK_FLAGS					\

[ ... ]


This isn't a bug introduced by this patch, but I noticed a couple of problems
in the previous patch of this series in kernel/trace/trace_probe.c.

[Severity: High]
This is a pre-existing issue, but in trace_probe_compile_bpf(),
bpf_prog_select_runtime() returns errors via its err pointer argument and
always returns a valid program pointer (or the original one), not an ERR_PTR:

    prog = bpf_prog_select_runtime(prog, &err);
    if (IS_ERR(prog))
        goto out;

Because IS_ERR(prog) is always false, won't this silently attach a dummy
program (__bpf_prog_ret0_warn) to the trace probe on JIT failure? This dummy
program immediately returns 0 without fetching arguments. Since
trace_event_buffer_reserve() doesn't zero-initialize the raw ring buffer space,
does this result in leaking uninitialized memory to user-space?


[Severity: High]
This isn't a bug introduced by this patch, but also in
trace_probe_compile_bpf(), when compiling memory fetch instructions:

    insns[i++] = BPF_JMP_IMM(BPF_JSGE, BPF_REG_0, 0, 1);
    insns[i++] = BPF_EXIT_INSN();

If copy_from_kernel_nofault() returns a negative error (e.g., from an invalid
pointer), doesn't this early exit terminate the entire BPF fetch program? Since
it stops processing any subsequent arguments, and trace_event_buffer_reserve()
doesn't zero-initialize the buffer, could this leak stale kernel ring buffer
memory to user space for all subsequent un-fetched arguments?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178291352217.1566898.14481561093843379745.stgit@devnote2?part=3