Message-ID: <1946544.CQOukoFCf9@7950hx>
Date: Sat, 20 Dec 2025 20:22:37 +0800
From: Menglong Dong <menglong.dong@...ux.dev>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: Menglong Dong <menglong8.dong@...il.com>, ast@...nel.org,
 andrii@...nel.org, davem@...emloft.net, dsahern@...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,
 tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
 netdev@...r.kernel.org, bpf@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v4 0/9] bpf: tracing session supporting

On 2025/12/20 17:01, Menglong Dong wrote:
> On 2025/12/20 09:12, Menglong Dong wrote:
> > On 2025/12/20 00:55, Andrii Nakryiko wrote:
> > > On Thu, Dec 18, 2025 at 5:18 PM Menglong Dong <menglong.dong@...ux.dev> wrote:
> > > >
> > > > On 2025/12/19 08:55 Andrii Nakryiko <andrii.nakryiko@...il.com> write:
> > > > > On Wed, Dec 17, 2025 at 1:54 AM Menglong Dong <menglong8.dong@...il.com> wrote:
> > > > > >
> > > > > > Hi, all.
> > > > > >
> > > > > > In this version, I combined Alexei and Andrii's advice, which makes the
> > > > > > architecture specific code much simpler.
> > > > > >
> > > > > > Sometimes, we need to hook both the entry and exit of a function with
> > > > > > TRACING. Therefore, we need define a FENTRY and a FEXIT for the target
> > > > > > function, which is not convenient.
> > > > > >
> > > > > > Therefore, we add a tracing session support for TRACING. Generally
> > > > > > speaking, it's similar to kprobe session, which can hook both the entry
> > > > > > and exit of a function with a single BPF program. Session cookie is also
> > > > > > supported with the kfunc bpf_fsession_cookie(). In order to limit the
> > > > > > stack usage, we limit the maximum number of cookies to 4.
> > > > > >
> > > > > > The kfunc bpf_fsession_is_return() and bpf_fsession_cookie() are both
> > > > > > inlined in the verifier.
> > > > >
> > > > > We have generic bpf_session_is_return() and bpf_session_cookie() (that
> > > > > currently works for ksession), can't you just implement them for the
> > > > > newly added program type instead of adding type-specific kfuncs?
> > > >
> > > > Hi, Andrii. I tried and found that it's a little hard to reuse them. The
> > > > bpf_session_is_return() and bpf_session_cookie() are defined as kfunc, which
> > > > makes we can't implement different functions for different attach type, like
> > > > what bpf helper does.
> > > 
> > > Are you sure? We certainly support kfunc implementation specialization
> > > for sleepable vs non-sleepable BPF programs. Check specialize_kfunc()
> > > in verifier.c
> > 
> > Ah, I remember it now. We do can use different kfunc version
> > for different case in specialize_kfunc().
> > 
> > > 
> > > >
> > > > The way we store "is_return" and "cookie" in fsession is different with
> > > > ksession. For ksession, it store the "is_return" in struct bpf_session_run_ctx.
> > > > Even if we move the "nr_regs" from stack to struct bpf_tramp_run_ctx,
> > > > it's still hard to reuse the bpf_session_is_return() or bpf_session_cookie(),
> > > > as the way of storing the "is_return" and "cookie" in fsession and ksession
> > > > is different, and it's a little difficult and complex to unify them.
> > > 
> > > I'm not saying we should unify the implementation, you have to
> > > implement different version of logically the same kfunc, of course.
> > 
> > I see. The problem now is that the prototype of bpf_session_cookie()
> > or bpf_session_is_return() don't satisfy our need. For bpf_session_cookie(),
> > we at least need the context to be the argument. However, both
> > of them don't have any function argument. After all, the prototype of
> > different version of logically the same kfunc should be the same.
> 
> Hi, Andrii. I see that you want to make the API consistent between
> ksession and fsession, which is more friendly for the user.
> 
> After my analysis, I think we have following approach:
> 1. change the function prototype of bpf_session_cookie and bpf_session_is_return
> to:
>     bool bpf_session_is_return(void *ctx);
>     bool bpf_session_cookie(void *ctx);
> And we do the fix up in specialize_kfunc(), which I think is the easiest
> way. The defect is that it will break existing users.
> 
> 2. We define a fixup_kfunc_call_early() and call it in add_subprog_and_kfunc.
> In the fixup_kfunc_call_early(), we will change the target kfunc(which is insn->imm)
> from bpf_session_cookie() to bpf_fsession_cookie(). For the bpf_session_cookie(),
> we make its prototype to:
>     __bpf_kfunc __u64 *bpf_session_cookie(void *ctx__ign)

Ah, it's not a good idea. The libbpf will check if the
prototype of bpf_session_cookie is compatible between local
and vmlinux. We can skip the fields whose name has "__ign"
in current libbpf in __bpf_core_types_are_compat(). But for
the old libbpf, the compatible checking will fail, which means
that it will still break the existing users :(

> Therefore, it won't break the existing users. For the ksession that uses the
> old prototype, it can pass the verifier too. Following is a demo patch of this
> approach. In this way, we can allow a extension in the prototype for a kfunc
> in the feature too.
> 
> What do you think?
> 
> Thanks!
> Menglong Dong
> 
> >patch<
> 
> +static int fixup_kfunc_call_early(struct bpf_verifier_env *env, struct bpf_insn *insn)
> +{
> +       struct bpf_prog *prog = env->prog;
> +
> +       if (prog->expected_attach_type == BPF_TRACE_FSESSION) {
> +               if (insn->imm == special_kfunc_list[KF_bpf_session_cookie])
> +                       insn->imm = special_kfunc_list[KF_bpf_fsession_cookie];
> +               else if (insn->imm == special_kfunc_list[KF_bpf_session_is_return])
> +                       insn->imm = special_kfunc_list[KF_bpf_fsession_is_return];
> +       }
> +
> +       return 0;
> +}
> 
> @@ -3489,10 +3490,12 @@ static int add_subprog_and_kfunc(struct bpf_verifier_env *env)
>                         return -EPERM;
>                 }
>  
> -               if (bpf_pseudo_func(insn) || bpf_pseudo_call(insn))
> +               if (bpf_pseudo_func(insn) || bpf_pseudo_call(insn)) {
>                         ret = add_subprog(env, i + insn->imm + 1);
> -               else
> -                       ret = add_kfunc_call(env, insn->imm, insn->off);
> +               } else {
> +                       ret = fixup_kfunc_call_early(env, insn);
> +                       ret = ret ?: add_kfunc_call(env, insn->imm, insn->off);
> +               }
> 
> @@ -3316,7 +3321,7 @@ static u64 bpf_uprobe_multi_entry_ip(struct bpf_run_ctx *ctx)
>  
>  __bpf_kfunc_start_defs();
>  
> -__bpf_kfunc bool bpf_session_is_return(void)
> +__bpf_kfunc bool bpf_session_is_return(void *ctx__ign)
>  {
>         struct bpf_session_run_ctx *session_ctx;
>  
> @@ -3324,7 +3329,7 @@ __bpf_kfunc bool bpf_session_is_return(void)
>         return session_ctx->is_return;
>  }
>  
> -__bpf_kfunc __u64 *bpf_session_cookie(void)
> +__bpf_kfunc __u64 *bpf_session_cookie(void *ctx__ign)
>  {
>         struct bpf_session_run_ctx *session_ctx;
> 
> > 
> > I think it's not a good idea to modify the prototype of existing kfunc,
> > can we?
> > 
> > > 
> > > >
> > > > What's more, we will lose the advantage of inline bpf_fsession_is_return
> > > > and bpf_fsession_cookie in verifier.
> > > >
> > > 
> > > I'd double check that either. BPF verifier and JIT do know program
> > > type, so you can pick how to inline
> > > bpf_session_is_return()/bpf_session_cookie() based on that.
> > 
> > Yeah, we can inline it depend on the program type if we can solve
> > the prototype problem.
> > 
> > Thanks!
> > Menglong Dong
> > 
> > 
> > > 
> > > > I'll check more to see if there is a more simple way to reuse them.
> > > >
> > > > Thanks!
> > > > Menglong Dong
> > > >
> > > > >
> > [...]
> > > >
> > > >
> > > >
> > > >
> > 
> > 
> > 
> > 
> > 
> 
> 
> 
> 
> 

Powered by blists - more mailing lists