Re: [PATCH bpf-next v4 1/9] bpf: add tracing session support

[Menglong Dong <menglong.dong@linux.dev>]

On 2025/12/19 08:55 Andrii Nakryiko <andrii.nakryiko@gmail.com> write:
> On Wed, Dec 17, 2025 at 1:55 AM Menglong Dong <menglong8.dong@gmail.com> wrote:
> >
> > The tracing session is something that similar to kprobe session. It allow
> > to attach a single BPF program to both the entry and the exit of the
> > target functions.
> >
> > Introduce the struct bpf_fsession_link, which allows to add the link to
> > both the fentry and fexit progs_hlist of the trampoline.
> >
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > Co-developed-by: Leon Hwang <leon.hwang@linux.dev>
> > Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
> > ---
> > v4:
> > - instead of adding a new hlist to progs_hlist in trampoline, add the bpf
> >   program to both the fentry hlist and the fexit hlist.
> > ---
> >  include/linux/bpf.h                           | 20 +++++++++++
> >  include/uapi/linux/bpf.h                      |  1 +
> >  kernel/bpf/btf.c                              |  2 ++
> >  kernel/bpf/syscall.c                          | 18 +++++++++-
> >  kernel/bpf/trampoline.c                       | 36 +++++++++++++++----
> >  kernel/bpf/verifier.c                         | 12 +++++--
> >  net/bpf/test_run.c                            |  1 +
> >  net/core/bpf_sk_storage.c                     |  1 +
> >  tools/include/uapi/linux/bpf.h                |  1 +
> >  .../bpf/prog_tests/tracing_failure.c          |  2 +-
> >  10 files changed, 83 insertions(+), 11 deletions(-)
> >
> 
> [...]
> 
> >  int bpf_prog_ctx_arg_info_init(struct bpf_prog *prog,
> >                                const struct bpf_ctx_arg_aux *info, u32 cnt);
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 84ced3ed2d21..696a7d37db0e 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -1145,6 +1145,7 @@ enum bpf_attach_type {
> >         BPF_NETKIT_PEER,
> >         BPF_TRACE_KPROBE_SESSION,
> >         BPF_TRACE_UPROBE_SESSION,
> > +       BPF_TRACE_SESSION,
> 
> FSESSION for consistency with FENTRY and FEXIT

OK

> 
> >         __MAX_BPF_ATTACH_TYPE
> >  };
> >
> 
> [...]
> 
> >  {
> > -       enum bpf_tramp_prog_type kind;
> > -       struct bpf_tramp_link *link_exiting;
> > +       enum bpf_tramp_prog_type kind, okind;
> > +       struct bpf_tramp_link *link_existing;
> > +       struct bpf_fsession_link *fslink;
> >         int err = 0;
> >         int cnt = 0, i;
> >
> > -       kind = bpf_attach_type_to_tramp(link->link.prog);
> > +       okind = kind = bpf_attach_type_to_tramp(link->link.prog);
> >         if (tr->extension_prog)
> >                 /* cannot attach fentry/fexit if extension prog is attached.
> >                  * cannot overwrite extension prog either.
> > @@ -621,13 +624,18 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link,
> >                                           BPF_MOD_JUMP, NULL,
> >                                           link->link.prog->bpf_func);
> >         }
> > +       if (kind == BPF_TRAMP_SESSION) {
> > +               /* deal with fsession as fentry by default */
> > +               kind = BPF_TRAMP_FENTRY;
> > +               cnt++;
> > +       }
> 
> this "pretend we are BPF_TRAMP_FENTRY" looks a bit hacky and is very
> hard to follow. I think it would be cleaner to have explicit small
> special cases for BPF_TRAMP_SESSION, and then generalize
> hlist_for_each_entry case by using a local variable for storing
> &tr->progs_hlist[kind] (which for TRAMP_SESSION you'll set to
> &tr->progs_hlist[BPF_TRAMP_FENTRY]). You'll then just do extra
> hlist_add_head/hlist_del_init and count manipulation. IMO, it's better
> than keeping in head what kind and okind is...

Ah, the way now does seem a little hacky. I tried to make this series
looks less complex by modifying the code as less as possible.

I'll use a explicit way here as you advised.

> 
> 
> >         if (cnt >= BPF_MAX_TRAMP_LINKS)
> >                 return -E2BIG;
> >         if (!hlist_unhashed(&link->tramp_hlist))
> >                 /* prog already linked */
> >                 return -EBUSY;
> > -       hlist_for_each_entry(link_exiting, &tr->progs_hlist[kind], tramp_hlist) {
> > -               if (link_exiting->link.prog != link->link.prog)
> > +       hlist_for_each_entry(link_existing, &tr->progs_hlist[kind], tramp_hlist) {
> > +               if (link_existing->link.prog != link->link.prog)
> >                         continue;
> >                 /* prog already linked */
> >                 return -EBUSY;
> 
> [...]
> 
> > @@ -23298,6 +23299,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
> >                 if (prog_type == BPF_PROG_TYPE_TRACING &&
> >                     insn->imm == BPF_FUNC_get_func_ret) {
> >                         if (eatype == BPF_TRACE_FEXIT ||
> > +                           eatype == BPF_TRACE_SESSION ||
> >                             eatype == BPF_MODIFY_RETURN) {
> >                                 /* Load nr_args from ctx - 8 */
> >                                 insn_buf[0] = BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_1, -8);
> > @@ -24242,7 +24244,8 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> >                 if (tgt_prog->type == BPF_PROG_TYPE_TRACING &&
> >                     prog_extension &&
> >                     (tgt_prog->expected_attach_type == BPF_TRACE_FENTRY ||
> > -                    tgt_prog->expected_attach_type == BPF_TRACE_FEXIT)) {
> > +                    tgt_prog->expected_attach_type == BPF_TRACE_FEXIT ||
> > +                    tgt_prog->expected_attach_type == BPF_TRACE_SESSION)) {
> >                         /* Program extensions can extend all program types
> >                          * except fentry/fexit. The reason is the following.
> >                          * The fentry/fexit programs are used for performance
> > @@ -24257,7 +24260,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> >                          * beyond reasonable stack size. Hence extending fentry
> >                          * is not allowed.
> >                          */
> > -                       bpf_log(log, "Cannot extend fentry/fexit\n");
> > +                       bpf_log(log, "Cannot extend fentry/fexit/session\n");
> 
> fsession?

OK

Thanks!
Menglong Dong

> 
> >                         return -EINVAL;
> >                 }
> >         } else {
> > @@ -24341,6 +24344,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> >         case BPF_LSM_CGROUP:
> >         case BPF_TRACE_FENTRY:
> >         case BPF_TRACE_FEXIT:
> > +       case BPF_TRACE_SESSION:
> >                 if (!btf_type_is_func(t)) {
> >                         bpf_log(log, "attach_btf_id %u is not a function\n",
> >                                 btf_id);
> 
> [...]
>