Message-ID: <2561816.jE0xQCEvom@7940hx>
Date: Wed, 16 Jul 2025 20:43:49 +0800
From: Menglong Dong <menglong.dong@...ux.dev>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
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 Wednesday, July 16, 2025 1:20 AM Andrii Nakryiko <andrii.nakryiko@...il.com> write:
> 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)

It works if I define all the functions that I need in the testmod.
However, most of the functions in the testing is reusing the
existing function, so it's a little complex to change them :/

> 
> >
> > >
> > > 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.

I see your idea now, which will free us from the function prototype
checking, and we don't need to do any consistency checking during
the attaching.

In my origin design, I tried to make the fentry-multi easy to use, and
keep the same usage with fentry.

So the only shortcoming of the method you said is that the user
can't access the function argument with ctx[x] directly, and the
bpf_core_cast() need to be used. Considering the use case, I think it's
OK in this way. After all, the common use case is we attach the bpf
prog to all the functions that has "task_struct" and store the argument
index in the cookie. And get the task_struct with
`bpf_core_cast(bpf_get_func_arg(cookie), struct task_struct)`.

I'll implement this part in this way, which can reduce 100+ line code :/

Thanks!
Menglong Dong

> 
> 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