> On Apr 8, 2022, at 3:21 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
>
> 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.
Sounds good to me.
Acked-by: Song Liu <songliubraving@xxxxxx>
