Re: [PATCH bpf-next v5 01/10] bpf: add fsession support

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

On 2026/1/1 21:52 Jiri Olsa <olsajiri@gmail.com> write:
> On Wed, Dec 24, 2025 at 09:07:26PM +0800, Menglong Dong wrote:
> 
> SNIP

Hi, Jiri. Happy New Year!

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

> 
> > +			cnt++;
> > +	}
> > +
> > +	return cnt;
> > +}
> > +
[......]
> > @@ -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.

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

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