On Fri, Apr 8, 2022 at 1:46 PM Song Liu <song@xxxxxxxxxx> wrote:
>
> On Fri, Apr 8, 2022 at 1:34 PM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> >
> > In a lot of cases the target of kprobe/kretprobe, tracepoint, raw
> > tracepoint, etc BPF program might not be known at the compilation time
> > and will be discovered at runtime. This was always a supported case by
> > libbpf, with APIs like bpf_program__attach_{kprobe,tracepoint,etc}()
> > accepting full target definition, regardless of what was defined in
> > SEC() definition in BPF source code.
> >
> > Unfortunately, up till now libbpf still enforced users to specify at
> > least something for the fake target, e.g., SEC("kprobe/whatever"), which
> > is cumbersome and somewhat misleading.
> >
> > This patch allows target-less SEC() definitions for basic tracing BPF
> > program types:
> > - kprobe/kretprobe;
> > - multi-kprobe/multi-kretprobe;
> > - tracepoints;
> > - raw tracepoints.
> >
> > Such target-less SEC() definitions are meant to specify declaratively
> > proper BPF program type only. Attachment of them will have to be handled
> > programmatically using correct APIs. As such, skeleton's auto-attachment
> > of such BPF programs is skipped and generic bpf_program__attach() will
> > fail, if attempted, due to the lack of enough target information.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
> > tools/lib/bpf/libbpf.c | 69 +++++++++++++++++++++++++++++++-----------
> > 1 file changed, 51 insertions(+), 18 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 9deb1fc67f19..81911a1e1f3e 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -8668,22 +8668,22 @@ static const struct bpf_sec_def section_defs[] = {
> > SEC_DEF("socket", SOCKET_FILTER, 0, SEC_NONE | SEC_SLOPPY_PFX),
> > SEC_DEF("sk_reuseport/migrate", SK_REUSEPORT, BPF_SK_REUSEPORT_SELECT_OR_MIGRATE, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
> > SEC_DEF("sk_reuseport", SK_REUSEPORT, BPF_SK_REUSEPORT_SELECT, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
> > - SEC_DEF("kprobe/", KPROBE, 0, SEC_NONE, attach_kprobe),
> > + SEC_DEF("kprobe+", KPROBE, 0, SEC_NONE, attach_kprobe),
> > SEC_DEF("uprobe+", KPROBE, 0, SEC_NONE, attach_uprobe),
> > - SEC_DEF("kretprobe/", KPROBE, 0, SEC_NONE, attach_kprobe),
> > + SEC_DEF("kretprobe+", KPROBE, 0, SEC_NONE, attach_kprobe),
> > SEC_DEF("uretprobe+", KPROBE, 0, SEC_NONE, attach_uprobe),
> > - SEC_DEF("kprobe.multi/", KPROBE, BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe_multi),
> > - SEC_DEF("kretprobe.multi/", KPROBE, BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe_multi),
> > + SEC_DEF("kprobe.multi+", KPROBE, BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe_multi),
> > + SEC_DEF("kretprobe.multi+", KPROBE, BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe_multi),
> > SEC_DEF("usdt+", KPROBE, 0, SEC_NONE, attach_usdt),
> > SEC_DEF("tc", SCHED_CLS, 0, SEC_NONE),
> > SEC_DEF("classifier", SCHED_CLS, 0, SEC_NONE | SEC_SLOPPY_PFX | SEC_DEPRECATED),
> > SEC_DEF("action", SCHED_ACT, 0, SEC_NONE | SEC_SLOPPY_PFX),
> > - SEC_DEF("tracepoint/", TRACEPOINT, 0, SEC_NONE, attach_tp),
> > - SEC_DEF("tp/", TRACEPOINT, 0, SEC_NONE, attach_tp),
> > - SEC_DEF("raw_tracepoint/", RAW_TRACEPOINT, 0, SEC_NONE, attach_raw_tp),
> > - SEC_DEF("raw_tp/", RAW_TRACEPOINT, 0, SEC_NONE, attach_raw_tp),
> > - SEC_DEF("raw_tracepoint.w/", RAW_TRACEPOINT_WRITABLE, 0, SEC_NONE, attach_raw_tp),
> > - SEC_DEF("raw_tp.w/", RAW_TRACEPOINT_WRITABLE, 0, SEC_NONE, attach_raw_tp),
> > + SEC_DEF("tracepoint+", TRACEPOINT, 0, SEC_NONE, attach_tp),
> > + SEC_DEF("tp+", TRACEPOINT, 0, SEC_NONE, attach_tp),
> > + SEC_DEF("raw_tracepoint+", RAW_TRACEPOINT, 0, SEC_NONE, attach_raw_tp),
> > + SEC_DEF("raw_tp+", RAW_TRACEPOINT, 0, SEC_NONE, attach_raw_tp),
> > + SEC_DEF("raw_tracepoint.w+", RAW_TRACEPOINT_WRITABLE, 0, SEC_NONE, attach_raw_tp),
> > + SEC_DEF("raw_tp.w+", RAW_TRACEPOINT_WRITABLE, 0, SEC_NONE, attach_raw_tp),
> > SEC_DEF("tp_btf/", TRACING, BPF_TRACE_RAW_TP, SEC_ATTACH_BTF, attach_trace),
> > SEC_DEF("fentry/", TRACING, BPF_TRACE_FENTRY, SEC_ATTACH_BTF, attach_trace),
> > SEC_DEF("fmod_ret/", TRACING, BPF_MODIFY_RETURN, SEC_ATTACH_BTF, attach_trace),
> > @@ -10411,6 +10411,12 @@ static int attach_kprobe(const struct bpf_program *prog, long cookie, struct bpf
> > char *func;
> > int n;
> >
> > + *link = NULL;
> > +
> > + /* no auto-attach for SEC("kprobe") and SEC("kretprobe") */
> > + if (strcmp(prog->sec_name, "kprobe") == 0 || strcmp(prog->sec_name, "kretprobe") == 0)
> > + return 0;
> > +
> > opts.retprobe = str_has_pfx(prog->sec_name, "kretprobe/");
> > if (opts.retprobe)
> > func_name = prog->sec_name + sizeof("kretprobe/") - 1;
> > @@ -10441,6 +10447,13 @@ static int attach_kprobe_multi(const struct bpf_program *prog, long cookie, stru
> > char *pattern;
> > int n;
> >
> > + *link = NULL;
> > +
> > + /* no auto-attach for SEC("kprobe.multi") and SEC("kretprobe.multi") */
> > + if (strcmp(prog->sec_name, "kprobe.multi") == 0 ||
> > + strcmp(prog->sec_name, "kretprobe.multi") == 0)
> > + return 0;
> > +
> > opts.retprobe = str_has_pfx(prog->sec_name, "kretprobe.multi/");
> > if (opts.retprobe)
> > spec = prog->sec_name + sizeof("kretprobe.multi/") - 1;
> > @@ -11145,6 +11158,12 @@ static int attach_tp(const struct bpf_program *prog, long cookie, struct bpf_lin
> > if (!sec_name)
> > return -ENOMEM;
> >
> > + *link = NULL;
> > +
> > + /* no auto-attach for SEC("tp") or SEC("tracepoint") */
> > + if (strcmp(prog->sec_name, "tp") == 0 || strcmp(prog->sec_name, "tracepoint") == 0)
> > + return 0;
> > +
> > /* extract "tp/<category>/<name>" or "tracepoint/<category>/<name>" */
> > if (str_has_pfx(prog->sec_name, "tp/"))
> > tp_cat = sec_name + sizeof("tp/") - 1;
> > @@ -11196,20 +11215,34 @@ struct bpf_link *bpf_program__attach_raw_tracepoint(const struct bpf_program *pr
> > static int attach_raw_tp(const struct bpf_program *prog, long cookie, struct bpf_link **link)
> > {
> > static const char *const prefixes[] = {
> > - "raw_tp/",
> > - "raw_tracepoint/",
> > - "raw_tp.w/",
> > - "raw_tracepoint.w/",
> > + "raw_tp",
> > + "raw_tracepoint",
> > + "raw_tp.w",
> > + "raw_tracepoint.w",
> > };
> > size_t i;
> > const char *tp_name = NULL;
> >
> > + *link = NULL;
> > +
> > for (i = 0; i < ARRAY_SIZE(prefixes); i++) {
> > - if (str_has_pfx(prog->sec_name, prefixes[i])) {
> > - tp_name = prog->sec_name + strlen(prefixes[i]);
> > - break;
> > - }
> > + size_t pfx_len;
> > +
> > + if (!str_has_pfx(prog->sec_name, prefixes[i]))
> > + continue;
> > +
> > + pfx_len = strlen(prefixes[i]);
> > + /* no auto-attach case of, e.g., SEC("raw_tp") */
> > + if (prog->sec_name[pfx_len] == '\0')
> > + return 0;
> > +
> > + if (prog->sec_name[pfx_len] != '/')
> > + continue;
>
> Maybe introduce a sec_has_pfx() function with tri-state return value:
> 1 for match with tp_name, 0, for match without tp_name, -1 for no match.
>
Hm.. tri-state might be quite confusing, but there might be some clean
ups to be done around this prefix handling for SEC_DEF()s. I'm
planning to do some more work on SEC() handling, I'll do this clean up
as a follow up, if you don't mind. Need to see how to best consolidate
this across all the places where we do this prefix matching.
> > +
> > + tp_name = prog->sec_name + pfx_len + 1;
> > + break;
> > }
> > +
> > if (!tp_name) {
> > pr_warn("prog '%s': invalid section name '%s'\n",
> > prog->name, prog->sec_name);
> > --
> > 2.30.2
> >
