Re: [PATCH bpf-next v5 01/10] bpf: add fsession support
From: Menglong Dong <hidden>
Date: 2026-01-02 09:22:09
Also in:
bpf, lkml
On 2026/1/1 21:52 Jiri Olsa [off-list ref] write:
On Wed, Dec 24, 2025 at 09:07:26PM +0800, Menglong Dong wrote: SNIP
Hi, Jiri. Happy New Year!
quoted
+struct bpf_fsession_link { + struct bpf_tracing_link link; + struct bpf_tramp_link fexit; +}; + struct bpf_raw_tp_link { struct bpf_link link; struct bpf_raw_event_map *btp;@@ -2114,6 +2120,20 @@ static inline void bpf_struct_ops_desc_release(struct bpf_struct_ops_desc *st_op #endif +static inline int bpf_fsession_cnt(struct bpf_tramp_links *links) +{ + struct bpf_tramp_links fentries = links[BPF_TRAMP_FENTRY]; + int cnt = 0; + + for (int i = 0; i < links[BPF_TRAMP_FENTRY].nr_links; i++) { + if (fentries.links[i]->link.prog->expected_attach_type == + BPF_TRACE_FSESSION)let's keep it on the single line ?
OK
quoted
+ cnt++; + } + + return cnt; +} +
[......]
quoted
@@ -3628,7 +3629,21 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog, key = bpf_trampoline_compute_key(tgt_prog, NULL, btf_id); } - link = kzalloc(sizeof(*link), GFP_USER); + if (prog->expected_attach_type == BPF_TRACE_FSESSION) { + struct bpf_fsession_link *fslink; + + fslink = kzalloc(sizeof(*fslink), GFP_USER); + if (fslink) { + bpf_link_init(&fslink->fexit.link, BPF_LINK_TYPE_TRACING, + &bpf_tracing_link_lops, prog, attach_type);I don't think we need the extra exit struct bpf_link, we just need hlist_node hook for exit program, so this should perhaps be: struct bpf_fsession_link { struct bpf_tracing_link link; struct hlist_node tramp_hlist; };
I think we can't do it this way according to how we manager the bpf_link in trampoline, as you can see in bpf_trampoline_get_progs() and the struct of bpf_tramp_links. In bpf_trampoline_get_progs(), it will lookup all the bpf_link in the trampoline. If we simply add the bpf_fsession_link->tramp_hlist, the struct in the progs_hlist will be inconsistent.
SNIPquoted
@@ -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; + struct hlist_head *prog_list; int err = 0; int cnt = 0, i;@@ -621,24 +625,44 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, BPF_MOD_JUMP, NULL, link->link.prog->bpf_func); } + if (kind == BPF_TRAMP_FSESSION) { + prog_list = &tr->progs_hlist[BPF_TRAMP_FENTRY]; + cnt++; + } else { + prog_list = &tr->progs_hlist[kind]; + } 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) { + hlist_for_each_entry(link_exiting, prog_list, tramp_hlist) { if (link_exiting->link.prog != link->link.prog) continue; /* prog already linked */ return -EBUSY; } - 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]); + 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;this seems confusing, how about we just add abolish bpf_fsession_link
It was more confusing in V1. I adopted Andrii's suggestion in this version to make the logic here more clear. But it seems still confusing :/ Maybe we need more document here to help the understanding.
quoted hunk ↗ jump to hunk
and add extra hlist_node hook to struct bpf_tramp_link .. we will waste 16 bytes for other cases, but the code seems less confusing to me untested, so I might overlooked something.. jirka ---diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 4e7d72dfbcd4..7479664844ea 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h@@ -1309,6 +1309,7 @@ enum bpf_tramp_prog_type { BPF_TRAMP_MODIFY_RETURN, BPF_TRAMP_MAX, BPF_TRAMP_REPLACE, /* more than MAX */ + BPF_TRAMP_FSESSION, }; struct bpf_tramp_image {@@ -1861,6 +1862,7 @@ struct bpf_link_ops { struct bpf_tramp_link { struct bpf_link link; struct hlist_node tramp_hlist; + struct hlist_node extra_hlist; u64 cookie; };
In this way, it indeed can make the update of the hlist more clear. However, I think that you missed the reading of the hlist as I mentioned above. You can't add both the "tramp_hlist" and "extra_hlist" to the same hlist. If so, how do we iterate the hlist? Do I miss something? Thanks! Menglong Dong
[......]
void test_tracing_failure(void)