[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEf4Bza84H=FL-KxJEFAn6pFpVBQVnvrpif6_gtf_SWHH4pRJQ@mail.gmail.com>
Date: Wed, 14 Jan 2026 10:56:16 -0800
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Menglong Dong <menglong.dong@...ux.dev>
Cc: Menglong Dong <menglong8.dong@...il.com>, ast@...nel.org, andrii@...nel.org,
daniel@...earbox.net, martin.lau@...ux.dev, eddyz87@...il.com,
song@...nel.org, yonghong.song@...ux.dev, john.fastabend@...il.com,
kpsingh@...nel.org, sdf@...ichev.me, haoluo@...gle.com, jolsa@...nel.org,
davem@...emloft.net, dsahern@...nel.org, tglx@...utronix.de, mingo@...hat.com,
jiang.biao@...ux.dev, bp@...en8.de, dave.hansen@...ux.intel.com,
x86@...nel.org, hpa@...or.com, bpf@...r.kernel.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v9 01/11] bpf: add fsession support
On Tue, Jan 13, 2026 at 6:11 PM Menglong Dong <menglong.dong@...ux.dev> wrote:
>
> On 2026/1/14 09:22 Andrii Nakryiko <andrii.nakryiko@...il.com> write:
> > On Sat, Jan 10, 2026 at 6:11 AM Menglong Dong <menglong8.dong@...il.com> wrote:
> > >
> > > The fsession 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.
> > >
> [...]
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -6107,6 +6107,7 @@ static int btf_validate_prog_ctx_type(struct bpf_verifier_log *log, const struct
> > > case BPF_TRACE_FENTRY:
> > > case BPF_TRACE_FEXIT:
> > > case BPF_MODIFY_RETURN:
> > > + case BPF_TRACE_FSESSION:
> > > /* allow u64* as ctx */
> > > if (btf_is_int(t) && t->size == 8)
> > > return 0;
> > > @@ -6704,6 +6705,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> > > fallthrough;
> > > case BPF_LSM_CGROUP:
> > > case BPF_TRACE_FEXIT:
> > > + case BPF_TRACE_FSESSION:
> >
> > According to the comment below we make this exception due to LSM.
> > FSESSION won't be using FSESSION programs, no? So this is not
> > necessary?
>
> The comment describe the LSM case here, but the code
> here is not only for LSM. It is also for FEXIT, which makes
> sure that we can get the return value with "ctx[nr_args]".
> So I think we still need it here, as we need to access the
> return value with "ctx[nr_args]" too.
please update the comment then as well
>
> >
> > > /* When LSM programs are attached to void LSM hooks
> > > * they use FEXIT trampolines and when attached to
> > > * int LSM hooks, they use MODIFY_RETURN trampolines.
> >
> > [...]
> >
> > > @@ -4350,6 +4365,7 @@ attach_type_to_prog_type(enum bpf_attach_type attach_type)
> > > case BPF_TRACE_RAW_TP:
> > > case BPF_TRACE_FENTRY:
> > > case BPF_TRACE_FEXIT:
> > > + case BPF_TRACE_FSESSION:
> > > case BPF_MODIFY_RETURN:
> > > return BPF_PROG_TYPE_TRACING;
> > > case BPF_LSM_MAC:
> > > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > > index 2a125d063e62..11e043049d68 100644
> > > --- a/kernel/bpf/trampoline.c
> > > +++ b/kernel/bpf/trampoline.c
> > > @@ -111,7 +111,7 @@ bool bpf_prog_has_trampoline(const struct bpf_prog *prog)
> > >
> > > return (ptype == BPF_PROG_TYPE_TRACING &&
> > > (eatype == BPF_TRACE_FENTRY || eatype == BPF_TRACE_FEXIT ||
> > > - eatype == BPF_MODIFY_RETURN)) ||
> > > + eatype == BPF_MODIFY_RETURN || eatype == BPF_TRACE_FSESSION)) ||
> > > (ptype == BPF_PROG_TYPE_LSM && eatype == BPF_LSM_MAC);
> >
> > this is getting crazy, switch to the switch (lol) maybe?
>
> ACK
>
> >
> > > }
> > >
> > > @@ -559,6 +559,8 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog)
> > > return BPF_TRAMP_MODIFY_RETURN;
> > > case BPF_TRACE_FEXIT:
> > > return BPF_TRAMP_FEXIT;
> > > + case BPF_TRACE_FSESSION:
> > > + return BPF_TRAMP_FSESSION;
> > > case BPF_LSM_MAC:
> > > if (!prog->aux->attach_func_proto->type)
> > > /* The function returns void, we cannot modify its
> > > @@ -596,6 +598,8 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link,
> > > {
> > > enum bpf_tramp_prog_type kind;
> > > struct bpf_tramp_link *link_exiting;
> > > + struct bpf_fsession_link *fslink;
> >
> > initialize to NULL to avoid compiler (falsely, but still) complaining
> > about potentially using uninitialized value
>
> ACK
>
> >
> > > + struct hlist_head *prog_list;
> > > int err = 0;
> > > int cnt = 0, i;
> > >
> >
> > [...]
> >
> > > - hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]);
> > > - tr->progs_cnt[kind]++;
> > > + hlist_add_head(&link->tramp_hlist, prog_list);
> > > + if (kind == BPF_TRAMP_FSESSION) {
> > > + tr->progs_cnt[BPF_TRAMP_FENTRY]++;
> > > + fslink = container_of(link, struct bpf_fsession_link, link.link);
> > > + hlist_add_head(&fslink->fexit.tramp_hlist,
> > > + &tr->progs_hlist[BPF_TRAMP_FEXIT]);
> >
> > fits under 100 characters? keep on a single line then
>
> ACK
>
> >
> > > + tr->progs_cnt[BPF_TRAMP_FEXIT]++;
> > > + } else {
> > > + tr->progs_cnt[kind]++;
> > > + }
> > > err = bpf_trampoline_update(tr, true /* lock_direct_mutex */);
> > > if (err) {
> > > hlist_del_init(&link->tramp_hlist);
> > > - tr->progs_cnt[kind]--;
> > > + if (kind == BPF_TRAMP_FSESSION) {
> > > + tr->progs_cnt[BPF_TRAMP_FENTRY]--;
> > > + hlist_del_init(&fslink->fexit.tramp_hlist);
> > > + tr->progs_cnt[BPF_TRAMP_FEXIT]--;
> > > + } else {
> > > + tr->progs_cnt[kind]--;
> > > + }
> > > }
> > > return err;
> > > }
> > > @@ -659,6 +683,7 @@ static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link,
> > > struct bpf_trampoline *tr,
> > > struct bpf_prog *tgt_prog)
> > > {
> > > + struct bpf_fsession_link *fslink;
> >
> > used in only one branch, move declaration there?
>
> ACK
>
> Thanks!
> Menglong Dong
>
> >
> > > enum bpf_tramp_prog_type kind;
> > > int err;
> > >
> > > @@ -672,6 +697,11 @@ static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link,
> > > guard(mutex)(&tgt_prog->aux->ext_mutex);
> > > tgt_prog->aux->is_extended = false;
> > > return err;
> > > + } else if (kind == BPF_TRAMP_FSESSION) {
> > > + fslink = container_of(link, struct bpf_fsession_link, link.link);
> > > + hlist_del_init(&fslink->fexit.tramp_hlist);
> > > + tr->progs_cnt[BPF_TRAMP_FEXIT]--;
> > > + kind = BPF_TRAMP_FENTRY;
> > > }
> > > hlist_del_init(&link->tramp_hlist);
> > > tr->progs_cnt[kind]--;
> >
> > [...]
> >
>
>
>
>
Powered by blists - more mailing lists