On Thu, 3 Jul 2025 at 18:26, Emil Tsalapatis <emil@xxxxxxxxxxxxxxx> wrote:
>
> On Tue, Jul 1, 2025 at 11:17 PM Kumar Kartikeya Dwivedi
> <memxor@xxxxxxxxx> wrote:
> >
> > Introduce a kernel function which is the analogue of dump_stack()
> > printing some useful information and the stack trace. This is not
> > exposed to BPF programs yet, but can be made available in the future.
> >
> > When we have a program counter for a BPF program in the stack trace,
> > also additionally output the filename and line number to make the trace
> > helpful. The rest of the trace can be passed into ./decode_stacktrace.sh
> > to obtain the line numbers for kernel symbols.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > ---
> > include/linux/bpf.h | 2 ++
> > kernel/bpf/stream.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 46 insertions(+)
> >
>
> Reviewed-by: Emil Tsalapatis <emil@xxxxxxxxxxxxxxx>
>
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 4d577352f3e6..18f8e4066e20 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -3615,8 +3615,10 @@ __printf(2, 3)
> > int bpf_stream_stage_printk(struct bpf_stream_stage *ss, const char *fmt, ...);
> > int bpf_stream_stage_commit(struct bpf_stream_stage *ss, struct bpf_prog *prog,
> > enum bpf_stream_id stream_id);
> > +int bpf_stream_stage_dump_stack(struct bpf_stream_stage *ss);
> >
> > #define bpf_stream_printk(ss, ...) bpf_stream_stage_printk(&ss, __VA_ARGS__)
> > +#define bpf_stream_dump_stack(ss) bpf_stream_stage_dump_stack(&ss)
> >
> > #define bpf_stream_stage(ss, prog, stream_id, expr) \
> > ({ \
> > diff --git a/kernel/bpf/stream.c b/kernel/bpf/stream.c
> > index c4925f8d275f..370eae669300 100644
> > --- a/kernel/bpf/stream.c
> > +++ b/kernel/bpf/stream.c
> > @@ -2,6 +2,7 @@
> > /* Copyright (c) 2025 Meta Platforms, Inc. and affiliates. */
> >
> > #include <linux/bpf.h>
> > +#include <linux/filter.h>
> > #include <linux/bpf_mem_alloc.h>
> > #include <linux/percpu.h>
> > #include <linux/refcount.h>
> > @@ -476,3 +477,46 @@ int bpf_stream_stage_commit(struct bpf_stream_stage *ss, struct bpf_prog *prog,
> > llist_add_batch(head, tail, &stream->log);
> > return 0;
> > }
> > +
> > +struct dump_stack_ctx {
> > + struct bpf_stream_stage *ss;
> > + int err;
> > +};
> > +
> > +static bool dump_stack_cb(void *cookie, u64 ip, u64 sp, u64 bp)
> > +{
> > + struct dump_stack_ctx *ctxp = cookie;
> > + const char *file = "", *line = "";
> > + struct bpf_prog *prog;
> > + int num, ret;
> > +
> > + rcu_read_lock();
> > + prog = bpf_prog_ksym_find(ip);
> > + rcu_read_unlock();
> > + if (prog) {
> > + ret = bpf_prog_get_file_line(prog, ip, &file, &line, &num);
> > + if (ret < 0)
> > + goto end;
>
> I assume that this is by design that if we cannot resolve the IP to a
> source line
> we just dump the IP and continue the stack walk.
Right, we fall back to what we do for non-bpf frames.
>
> > + ctxp->err = bpf_stream_stage_printk(ctxp->ss, "%pS\n %s @ %s:%d\n",
> > + (void *)ip, line, file, num);
> > + return !ctxp->err;
> > + }
> > +end:
> > + ctxp->err = bpf_stream_stage_printk(ctxp->ss, "%pS\n", (void *)ip);
> > + return !ctxp->err;
> > +}
> > +
> > +int bpf_stream_stage_dump_stack(struct bpf_stream_stage *ss)
> > +{
> > + struct dump_stack_ctx ctx = { .ss = ss };
> > + int ret;
> > +
> > + ret = bpf_stream_stage_printk(ss, "CPU: %d UID: %d PID: %d Comm: %s\n",
> > + raw_smp_processor_id(), __kuid_val(current_real_cred()->euid),
> > + current->pid, current->comm);
> > + ret = ret ?: bpf_stream_stage_printk(ss, "Call trace:\n");
> > + if (!ret)
>
> Nit: Can we flip this and just do
> if (ret)
> return ret;
> ? I get using ?: for brevity but it makes the code less obvious, and
> this specific check
> isn't even shorter than the more straightforward alternative.
Makes sense, will rework.
>
> > + arch_bpf_stack_walk(dump_stack_cb, &ctx);
> > + ret = ret ?: ctx.err;
> > + return ret ?: bpf_stream_stage_printk(ss, "\n");
> > +}
> > --
> > 2.47.1
> >
