| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAEf4BzZ-3rs2U8x7K+Gd3dDTn5OusBh5SsZ_cE3ZeuVHnoRzKQ@mail.gmail.com>
Date: Tue, 15 Jul 2025 10:20:05 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Menglong Dong <menglong.dong@...ux.dev>
Cc: Menglong Dong <menglong8.dong@...il.com>, alexei.starovoitov@...il.com,
rostedt@...dmis.org, jolsa@...nel.org, bpf@...r.kernel.org,
Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>,
Yonghong Song <yonghong.song@...ux.dev>, John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 13/18] libbpf: support tracing_multi
On Mon, Jul 14, 2025 at 6:59 PM Menglong Dong <menglong.dong@...ux.dev> wrote:
>
>
> On 7/15/25 06:07, Andrii Nakryiko wrote:
> > On Thu, Jul 3, 2025 at 5:24 AM Menglong Dong <menglong8.dong@...il.com> wrote:
> >> Add supporting for the attach types of:
> >>
> >> BPF_TRACE_FENTRY_MULTI
> >> BPF_TRACE_FEXIT_MULTI
> >> BPF_MODIFY_RETURN_MULTI
> >>
> >> Signed-off-by: Menglong Dong <dongml2@...natelecom.cn>
> >> ---
> >> tools/bpf/bpftool/common.c | 3 +
> >> tools/lib/bpf/bpf.c | 10 +++
> >> tools/lib/bpf/bpf.h | 6 ++
> >> tools/lib/bpf/libbpf.c | 168 ++++++++++++++++++++++++++++++++++++-
> >> tools/lib/bpf/libbpf.h | 19 +++++
> >> tools/lib/bpf/libbpf.map | 1 +
> >> 6 files changed, 204 insertions(+), 3 deletions(-)
> >>
> > [...]
> >
> >> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> >> index 1342564214c8..5c97acec643d 100644
> >> --- a/tools/lib/bpf/bpf.h
> >> +++ b/tools/lib/bpf/bpf.h
> >> @@ -422,6 +422,12 @@ struct bpf_link_create_opts {
> >> struct {
> >> __u64 cookie;
> >> } tracing;
> >> + struct {
> >> + __u32 cnt;
> >> + const __u32 *btf_ids;
> >> + const __u32 *tgt_fds;
> > tgt_fds are always BTF FDs, right? Do we intend to support
> > freplace-style multi attachment at all? If not, I'd name them btf_fds,
> > and btf_ids -> btf_type_ids (because BTF ID can also refer to kernel
> > ID of BTF object, so ambiguous and somewhat confusing)
>
>
> For now, freplace is not supported. And I'm not sure if we will support
>
> it in the feature.
>
>
> I think that there should be no need to use freplace in large quantities,
>
> so we don't need to support the multi attachment for it in the feature.
>
>
> Yeah, I'll follow your advice in the next version.
>
great
>
> >
> >> + const __u64 *cookies;
> >> + } tracing_multi;
> >> struct {
> >> __u32 pf;
> >> __u32 hooknum;
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index 530c29f2f5fc..ae38b3ab84c7 100644
> >> --- a/tools/lib/bpf/libbpf.c
> >> +++ b/tools/lib/bpf/libbpf.c
> >> @@ -136,6 +136,9 @@ static const char * const attach_type_name[] = {
> >> [BPF_NETKIT_PEER] = "netkit_peer",
> >> [BPF_TRACE_KPROBE_SESSION] = "trace_kprobe_session",
> >> [BPF_TRACE_UPROBE_SESSION] = "trace_uprobe_session",
> >> + [BPF_TRACE_FENTRY_MULTI] = "trace_fentry_multi",
> >> + [BPF_TRACE_FEXIT_MULTI] = "trace_fexit_multi",
> >> + [BPF_MODIFY_RETURN_MULTI] = "modify_return_multi",
> >> };
> >>
> >> static const char * const link_type_name[] = {
> >> @@ -410,6 +413,8 @@ enum sec_def_flags {
> >> SEC_XDP_FRAGS = 16,
> >> /* Setup proper attach type for usdt probes. */
> >> SEC_USDT = 32,
> >> + /* attachment target is multi-link */
> >> + SEC_ATTACH_BTF_MULTI = 64,
> >> };
> >>
> >> struct bpf_sec_def {
> >> @@ -7419,9 +7424,9 @@ static int libbpf_prepare_prog_load(struct bpf_program *prog,
> >> opts->expected_attach_type = BPF_TRACE_UPROBE_MULTI;
> >> }
> >>
> >> - if ((def & SEC_ATTACH_BTF) && !prog->attach_btf_id) {
> >> + if ((def & (SEC_ATTACH_BTF | SEC_ATTACH_BTF_MULTI)) && !prog->attach_btf_id) {
> >> int btf_obj_fd = 0, btf_type_id = 0, err;
> >> - const char *attach_name;
> >> + const char *attach_name, *name_end;
> >>
> >> attach_name = strchr(prog->sec_name, '/');
> >> if (!attach_name) {
> >> @@ -7440,7 +7445,27 @@ static int libbpf_prepare_prog_load(struct bpf_program *prog,
> >> }
> >> attach_name++; /* skip over / */
> >>
> >> - err = libbpf_find_attach_btf_id(prog, attach_name, &btf_obj_fd, &btf_type_id);
> >> + name_end = strchr(attach_name, ',');
> >> + /* for multi-link tracing, use the first target symbol during
> >> + * loading.
> >> + */
> >> + if ((def & SEC_ATTACH_BTF_MULTI) && name_end) {
> >> + int len = name_end - attach_name + 1;
> > for multi-kprobe we decided to only support a single glob as a target
> > in declarative SEC() definition. If a user needs more control, they
> > can always fallback to the programmatic bpf_program__attach_..._opts()
> > variant. Let's do the same here, glob is good enough for declarative
> > use cases, and for complicated cases programmatic is the way to go
> > anyways. You'll avoid unnecessary complications like this one then.
>
>
> In fact, this is to make the BPF code in the selftests simple. With such
>
> control, I can test different combination of the target functions easily,
>
> just like this:
>
>
> SEC("fentry.multi/bpf_testmod_test_struct_arg_1,bpf_testmod_test_struct_arg_13")
> int BPF_PROG2(fentry_success_test1, struct bpf_testmod_struct_arg_2, a)
> {
> test_result = a.a + a.b;
> return 0;
> }
>
> SEC("fentry.multi/bpf_testmod_test_struct_arg_2,bpf_testmod_test_struct_arg_10")
> int BPF_PROG2(fentry_success_test2, int, a, struct
> bpf_testmod_struct_arg_2, b)
> {
> test_result = a + b.a + b.b;
> return 0;
> }
>
>
> And you are right, we should design it for the users, and a single glob is
>
> much better. Instead, I'll implement the combination testings in the
>
> loader with bpf_program__attach_trace_multi_opts().
>
sgtm. I'd also think if we can construct a glob that would describe
functions you need (and if necessary to rename testmod functions
slightly - so be it, it's all for testing anyways)
>
> >
> > BTW, it's not trivial to figure this out from earlier patches, but
> > does BPF verifier need to know all these BTF type IDs during program
> > verification time? If yes, why and then why do we need to specify them
> > during LINK_CREATE time. And if not, then great, and we don't need to
> > parse all this during load time.
>
>
> It doesn't need to know all the BTF type IDs, but it need to know one
>
> of them(the first one), which means that we still need to do the parse
>
> during load time.
>
>
> Of course, we can split it:
>
> step 1: parse the glob and get the first BTF type ID during load time
>
> step 2: parse the glob and get all the BTF type IDs during attachment
>
>
> But it will make the code a little more complex. Shoud I do it this way?
>
> I'd appreciate it to hear some advice here :/
I think I have a bit of disconnect here, because in my mind
multi-fentry/fexit cannot be type-aware, in general, at BPF
verification time. I.e., verifier should not assume any specific
prototype, and this gets back to my suggestion to just use
bpf_get_func_arg/cnt. While in some special cases you might want to
attach to a small number of functions that, say, have task_struct
argument and we can take a bit of advantage of this in BPF code by
verifier ensuring that all attached functions have that task_struct, I
do think this is unnecessary complication and limitation, and I'd
rather make multi-fentry/fexit not type-aware in the same way as
fentry/fexit is. With that, verifier won't need to know BTF ID, and so
multi-fentry will work very similarly to multi-kprobe, just will be
slightly cheaper at runtime.
And I'm saying all this, because even if all attached functions have
task_struct as that argument, you can achieve exactly that by just
doing `bpf_core_cast(bpf_get_func_arg(0), struct task_struct)`, and
that's all. So I'd simplify and make working with multi-fentry easier
for multi-function tracers (which is the challenging aspect with
fentry today). If you have 2-3-4-5 functions you are attaching to and
hoping to get that task_struct, you might as well just attach 2-3-4-5
times, get performance benefit, without really compromising much on
attachment time (because 5 attachments are plenty fast).
>
>
> >
> >> + char *first_tgt;
> >> +
> >> + first_tgt = malloc(len);
> >> + if (!first_tgt)
> >> + return -ENOMEM;
> >> + libbpf_strlcpy(first_tgt, attach_name, len);
> >> + first_tgt[len - 1] = '\0';
> >> + err = libbpf_find_attach_btf_id(prog, first_tgt, &btf_obj_fd,
> >> + &btf_type_id);
> >> + free(first_tgt);
> >> + } else {
> >> + err = libbpf_find_attach_btf_id(prog, attach_name, &btf_obj_fd,
> >> + &btf_type_id);
> >> + }
> >> +
> >> if (err)
> >> return err;
> >>
> >> @@ -9519,6 +9544,7 @@ static int attach_kprobe_session(const struct bpf_program *prog, long cookie, st
> >> static int attach_uprobe_multi(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> >> static int attach_lsm(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> >> static int attach_iter(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> >> +static int attach_trace_multi(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> >>
> >> static const struct bpf_sec_def section_defs[] = {
> >> SEC_DEF("socket", SOCKET_FILTER, 0, SEC_NONE),
> >> @@ -9565,6 +9591,13 @@ static const struct bpf_sec_def section_defs[] = {
> >> SEC_DEF("fentry.s+", TRACING, BPF_TRACE_FENTRY, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_trace),
> >> SEC_DEF("fmod_ret.s+", TRACING, BPF_MODIFY_RETURN, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_trace),
> >> SEC_DEF("fexit.s+", TRACING, BPF_TRACE_FEXIT, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_trace),
> >> + SEC_DEF("tp_btf+", TRACING, BPF_TRACE_RAW_TP, SEC_ATTACH_BTF, attach_trace),
> > duplicate
>
>
> Get it :/
>
>
> Thanks!
>
> Menglong Dong
>
>
> >
> >
> >> + SEC_DEF("fentry.multi+", TRACING, BPF_TRACE_FENTRY_MULTI, SEC_ATTACH_BTF_MULTI, attach_trace_multi),
> >> + SEC_DEF("fmod_ret.multi+", TRACING, BPF_MODIFY_RETURN_MULTI, SEC_ATTACH_BTF_MULTI, attach_trace_multi),
> >> + SEC_DEF("fexit.multi+", TRACING, BPF_TRACE_FEXIT_MULTI, SEC_ATTACH_BTF_MULTI, attach_trace_multi),
> >> + SEC_DEF("fentry.multi.s+", TRACING, BPF_TRACE_FENTRY_MULTI, SEC_ATTACH_BTF_MULTI | SEC_SLEEPABLE, attach_trace_multi),
> >> + SEC_DEF("fmod_ret.multi.s+", TRACING, BPF_MODIFY_RETURN_MULTI, SEC_ATTACH_BTF_MULTI | SEC_SLEEPABLE, attach_trace_multi),
> >> + SEC_DEF("fexit.multi.s+", TRACING, BPF_TRACE_FEXIT_MULTI, SEC_ATTACH_BTF_MULTI | SEC_SLEEPABLE, attach_trace_multi),
> >> SEC_DEF("freplace+", EXT, 0, SEC_ATTACH_BTF, attach_trace),
> >> SEC_DEF("lsm+", LSM, BPF_LSM_MAC, SEC_ATTACH_BTF, attach_lsm),
> >> SEC_DEF("lsm.s+", LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
> >> @@ -12799,6 +12832,135 @@ static int attach_trace(const struct bpf_program *prog, long cookie, struct bpf_
> >> return libbpf_get_error(*link);
> >> }
> >>
> > [...]
> >
Powered by blists - more mailing lists