On Thu, 3 Jul 2025 at 17:02, Emil Tsalapatis <emil@xxxxxxxxxxxxxxx> wrote:
>
> On Tue, Jul 1, 2025 at 11:17 PM Kumar Kartikeya Dwivedi
> <memxor@xxxxxxxxx> wrote:
> >
> > Prepare a function for use in future patches that can extract the file
> > info, line info, and the source line number for a given BPF program
> > provided it's program counter.
> >
> > Only the basename of the file path is provided, given it can be
> > excessively long in some cases.
> >
> > This will be used in later patches to print source info to the BPF
> > stream.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > ---
> > include/linux/bpf.h | 3 +++
> > kernel/bpf/core.c | 47 +++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 50 insertions(+)
> >
>
> Nits aside:
>
> Reviewed-by: Emil Tsalapatis <emil@xxxxxxxxxxxxxxx>
>
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 85b1cbe494f5..09f06b1ea62e 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -3660,4 +3660,7 @@ static inline bool bpf_is_subprog(const struct bpf_prog *prog)
> > return prog->aux->func_idx != 0;
> > }
> >
> > +int bpf_prog_get_file_line(struct bpf_prog *prog, unsigned long ip, const char **filep,
> > + const char **linep, int *nump);
> > +
> > #endif /* _LINUX_BPF_H */
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index f0def24573ae..5c6e9fbb5508 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -3213,3 +3213,50 @@ EXPORT_SYMBOL(bpf_stats_enabled_key);
> >
> > EXPORT_TRACEPOINT_SYMBOL_GPL(xdp_exception);
> > EXPORT_TRACEPOINT_SYMBOL_GPL(xdp_bulk_tx);
> > +
> > +#ifdef CONFIG_BPF_SYSCALL
> > +
> > +int bpf_prog_get_file_line(struct bpf_prog *prog, unsigned long ip, const char **filep,
> > + const char **linep, int *nump)
> > +{
> > + int idx = -1, insn_start, insn_end, len;
> > + struct bpf_line_info *linfo;
> > + void **jited_linfo;
> > + struct btf *btf;
> > +
> > + btf = prog->aux->btf;
> > + linfo = prog->aux->linfo;
> > + jited_linfo = prog->aux->jited_linfo;
> > +
> > + if (!btf || !linfo || !prog->aux->jited_linfo)
> > + return -EINVAL;
>
> Either use jited_linfo in the condition, or remove the shorthands
> above sin ce btf and
> jited_linfo immediately get overwritten anyway.
Good catch.
>
> > + len = prog->aux->func ? prog->aux->func[prog->aux->func_idx]->len : prog->len;
> > +
> > + linfo = &prog->aux->linfo[prog->aux->linfo_idx];
> > + jited_linfo = &prog->aux->jited_linfo[prog->aux->linfo_idx];
> > +
> > + insn_start = linfo[0].insn_off;
> > + insn_end = insn_start + len;
> > +
> > + for (int i = 0; i < prog->aux->nr_linfo &&
> > + linfo[i].insn_off >= insn_start && linfo[i].insn_off < insn_end; i++) {
> > + if (jited_linfo[i] >= (void *)ip)
> > + break;
> > + idx = i;
> > + }
> > +
> > + if (idx == -1)
>
> This doesn't catch the case where we exhaust the loop without ever
> triggering the
> jited_linfo[i] >= (void *)ip branch. Is it worth using (jited_linfo[i]
> < (void *)ip as the
> error condition instead, or do we not need to account for it?
I think it will mean the ip belongs to the last idx, so it should be fine.
>
> > + return -ENOENT;
> > +
> > + /* Get base component of the file path. */
> > + *filep = btf_name_by_offset(btf, linfo[idx].file_name_off);
> > + *filep = kbasename(*filep);
> > + /* Obtain the source line, and strip whitespace in prefix. */
> > + *linep = btf_name_by_offset(btf, linfo[idx].line_off);
> > + while (isspace(**linep))
> > + *linep += 1;
> > + *nump = BPF_LINE_INFO_LINE_NUM(linfo[idx].line_col);
> > + return 0;
> > +}
> > +
> > +#endif
> > --
> > 2.47.1
> >
