Message-ID: <3026367.e9J7NaK4W3@7940hx>
Date: Thu, 22 Jan 2026 10:41:04 +0800
From: Menglong Dong <menglong.dong@...ux.dev>
To: Menglong Dong <menglong8.dong@...il.com>,
 Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: 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 v10 07/12] bpf,x86: add fsession support for x86_64

On 2026/1/22 08:22 Andrii Nakryiko <andrii.nakryiko@...il.com> write:
> On Wed, Jan 21, 2026 at 4:06 PM Andrii Nakryiko
> <andrii.nakryiko@...il.com> wrote:
> >
> > On Thu, Jan 15, 2026 at 3:24 AM Menglong Dong <menglong8.dong@...il.com> wrote:
> > >
> > > Add BPF_TRACE_FSESSION supporting to x86_64, including:
> > >
> > > 1. clear the return value in the stack before fentry to make the fentry
> > >    of the fsession can only get 0 with bpf_get_func_ret().
> > >
> > > 2. clear all the session cookies' value in the stack.
> > >
> > > 2. store the index of the cookie to ctx[-1] before the calling to fsession
> > >
> > > 3. store the "is_return" flag to ctx[-1] before the calling to fexit of
> > >    the fsession.
> > >
> > > Signed-off-by: Menglong Dong <dongml2@...natelecom.cn>
> > > Co-developed-by: Leon Hwang <leon.hwang@...ux.dev>
> > > Signed-off-by: Leon Hwang <leon.hwang@...ux.dev>
> > > ---
> > > v10:
> > > - use "|" for func_meta instead of "+"
> > > - pass the "func_meta_off" to invoke_bpf() explicitly, instead of
> > >   computing it with "stack_size + 8"
> > > - pass the "cookie_off" to invoke_bpf() instead of computing the current
> > >   cookie index with "func_meta"
> > >
> > > v5:
> > > - add the variable "func_meta"
> > > - define cookie_off in a new line
> > >
> > > v4:
> > > - some adjustment to the 1st patch, such as we get the fsession prog from
> > >   fentry and fexit hlist
> > > - remove the supporting of skipping fexit with fentry return non-zero
> > >
> > > v2:
> > > - add session cookie support
> > > - add the session stuff after return value, instead of before nr_args
> > > ---
> > >  arch/x86/net/bpf_jit_comp.c | 52 ++++++++++++++++++++++++++++---------
> > >  1 file changed, 40 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > > index 2f31331955b5..16720f2be16c 100644
> > > --- a/arch/x86/net/bpf_jit_comp.c
> > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > @@ -3094,13 +3094,19 @@ static int emit_cond_near_jump(u8 **pprog, void *func, void *ip, u8 jmp_cond)
> > >
> > >  static int invoke_bpf(const struct btf_func_model *m, u8 **pprog,
> > >                       struct bpf_tramp_links *tl, int stack_size,
> > > -                     int run_ctx_off, bool save_ret,
> > > -                     void *image, void *rw_image)
> > > +                     int run_ctx_off, int func_meta_off, bool save_ret,
> > > +                     void *image, void *rw_image, u64 func_meta,
> > > +                     int cookie_off)
> > >  {
> > > -       int i;
> > > +       int i, cur_cookie = (cookie_off - stack_size) / 8;
> >
> > not sure why you went with passing cookie_off and then calculating,
> > effectively, cookie count out of that?... why not pass cookie count
> > directly then? it's minor, but just seems like a weird choice here,
> > tbh

Hi, Andrii. I think you misunderstand it here. The cur_cookie is not the
same as cookie count. The layout of the stack looks like this:

  return value  -> 8 bytes
  argN          -> 8 bytes
  ...
  arg1          -> 8 bytes
  nr_args       -> 8 bytes
  ip (optional) -> 8 bytes
  cookie2       -> 8 bytes
  cookie1       -> 8 bytes

So if the bpf_get_func_ip() not used, the cur_cookie is exactly the same
as cookie count. But if it exist, they are not the same.

The location of the cookies is independent from the context, and the
cur_cookie, which is the index of the current cookie, don't rely on cookie
count too and can be bigger than cookie count.

PS: the location of "ip" should always laid before the nr_args, as we get
it with ctx[-2]. Maybe we can optimize it later. We store the index of
the ip the func_meta too, therefore it is independent from the ctx too.
Ah, it looks not make much sense ;)

> >
> 
> consider also just calculating cookie count out from bpf_tramp_links?
> would that work? Then "func_meta" would really be just nr_args (and
> I'd call it that) and bool for whether this is entry or exit
> invokation (for IS_RETURN bit, and maybe we'll need this distinction
> somewhere else in the future), and then invoke_bpf() will construct
> func_meta from scratch.
> 
> It's relatively minor thing, but as I mentioned before, it's this
> hybrid approach of partially opaque (from invoke_bpf's POV) func_meta
> which we also adjust or fill out (for cookie index) is a bit of a sign
> that this is not a proper interface.

Yeah, the current approach is indeed not perfect. But I think it's
a little not flex if we construct the whole func_meta in invoke_bpf().
For now, we need to pass nr_args, is_return, cookie_off to it. And
we need to add more function arguments to invoke_bpf() if there
are new flags occur in the feature, which is not convenient, right?

So what do you think?

Thanks!
Menglong Dong

> 
> >
> >
> > >         u8 *prog = *pprog;
> > >
> > >         for (i = 0; i < tl->nr_links; i++) {
> > > +               if (tl->links[i]->link.prog->call_session_cookie) {
> > > +                       emit_store_stack_imm64(&prog, BPF_REG_0, -func_meta_off,
> > > +                                              func_meta | (cur_cookie << BPF_TRAMP_SHIFT_COOKIE));
> > > +                       cur_cookie--;
> > > +               }
> > >                 if (invoke_bpf_prog(m, &prog, tl->links[i], stack_size,
> > >                                     run_ctx_off, save_ret, image, rw_image))
> > >                         return -EINVAL;
> >
> > [...]
> 

Powered by blists - more mailing lists