[PATCH bpf-next 0/7] bpf: Introduce kprobe_multi session attach

[PATCH bpf-next 0/7] bpf: Introduce kprobe_multi session attach

[Jiri Olsa]

hi,
adding support to attach kprobe program through kprobe_multi link
in a session mode, which means:
  - program is attached to both function entry and return
  - entry program can decided if the return program gets executed
  - entry program can share u64 cookie value with return program

The initial RFC for this was posted in [0] and later discussed more
and which ended up with the session idea [1]

Having entry together with return probe for given function is common
use case for tetragon, bpftrace and most likely for others.

At the moment if we want both entry and return probe to execute bpf
program we need to create two (entry and return probe) links. The link
for return probe creates extra entry probe to setup the return probe.
The extra entry probe execution could be omitted if we had a way to
use just single link for both entry and exit probe.

In addition the possibility to control the return program execution
and sharing data within entry and return probe allows for other use
cases.

Changes from last RFC version [1]:
  - changed wrapper name to session 
  - changed flag to adding new attach type for session:
      BPF_TRACE_KPROBE_MULTI_SESSION
    it's more convenient wrt filtering on kfuncs setup and seems
    to make more sense alltogether
  - renamed bpf_kprobe_multi_is_return to bpf_session_is_return
  - added bpf_session_cookie kfunc, which actually already works
    on current fprobe implementation (not just fprobe-on-fgraph)
    and it provides the shared data between entry/return probes [Andrii]

    we could actually make the cookie size configurable.. thoughts?
    (it's 8 bytes atm)

  - better attach setup conditions changes [Andrii]
  - I'm not including uprobes change atm, because it needs extra
    uprobe change so I'll post it separately

Also available at:
  https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
  bpf/session_data

thanks,
jirka


[0] https://lore.kernel.org/bpf/20240207153550.856536-1-jolsa@kernel.org/
[1] https://lore.kernel.org/bpf/20240228090242.4040210-1-jolsa@kernel.org/
---
Jiri Olsa (7):
      bpf: Add support for kprobe multi session attach
      bpf: Add support for kprobe multi session context
      bpf: Add support for kprobe multi session cookie
      libbpf: Add support for kprobe multi session attach
      libbpf: Add kprobe session attach type name to attach_type_name
      selftests/bpf: Add kprobe multi session test
      selftests/bpf: Add kprobe multi wrapper cookie test

 include/uapi/linux/bpf.h                                        |   1 +
 kernel/bpf/btf.c                                                |   3 +++
 kernel/bpf/syscall.c                                            |   7 +++++-
 kernel/bpf/verifier.c                                           |   7 ++++++
 kernel/trace/bpf_trace.c                                        | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------
 tools/include/uapi/linux/bpf.h                                  |   1 +
 tools/lib/bpf/bpf.c                                             |   1 +
 tools/lib/bpf/libbpf.c                                          |  41 ++++++++++++++++++++++++++++++++--
 tools/lib/bpf/libbpf.h                                          |   4 +++-
 tools/testing/selftests/bpf/bpf_kfuncs.h                        |   3 +++
 tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c      |  84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/kprobe_multi_session.c        | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/kprobe_multi_session_cookie.c |  56 ++++++++++++++++++++++++++++++++++++++++++++++
 13 files changed, 396 insertions(+), 18 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_session.c
 create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_session_cookie.c

[PATCH bpf-next 1/7] bpf: Add support for kprobe multi session attach

[Jiri Olsa]

Adding support to attach bpf program for entry and return probe
of the same function. This is common use case which at the moment
requires to create two kprobe multi links.

Adding new BPF_TRACE_KPROBE_MULTI_SESSION attach type that instructs
kernel to attach single link program to both entry and exit probe.

It's possible to control execution of the bpf program on return
probe simply by returning zero or non zero from the entry bpf
program execution to execute or not the bpf program on return
probe respectively.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/uapi/linux/bpf.h       |  1 +
 kernel/bpf/syscall.c           |  7 ++++++-
 kernel/trace/bpf_trace.c       | 28 ++++++++++++++++++++--------
 tools/include/uapi/linux/bpf.h |  1 +
 4 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index cee0a7915c08..fb8ecb199273 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1115,6 +1115,7 @@ enum bpf_attach_type {
 	BPF_CGROUP_UNIX_GETSOCKNAME,
 	BPF_NETKIT_PRIMARY,
 	BPF_NETKIT_PEER,
+	BPF_TRACE_KPROBE_MULTI_SESSION,
 	__MAX_BPF_ATTACH_TYPE
 };
 
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 7d392ec83655..7c0cb1800b9f 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3974,11 +3974,15 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
 		if (prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI &&
 		    attach_type != BPF_TRACE_KPROBE_MULTI)
 			return -EINVAL;
+		if (prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI_SESSION &&
+		    attach_type != BPF_TRACE_KPROBE_MULTI_SESSION)
+			return -EINVAL;
 		if (prog->expected_attach_type == BPF_TRACE_UPROBE_MULTI &&
 		    attach_type != BPF_TRACE_UPROBE_MULTI)
 			return -EINVAL;
 		if (attach_type != BPF_PERF_EVENT &&
 		    attach_type != BPF_TRACE_KPROBE_MULTI &&
+		    attach_type != BPF_TRACE_KPROBE_MULTI_SESSION &&
 		    attach_type != BPF_TRACE_UPROBE_MULTI)
 			return -EINVAL;
 		return 0;
@@ -5239,7 +5243,8 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr)
 	case BPF_PROG_TYPE_KPROBE:
 		if (attr->link_create.attach_type == BPF_PERF_EVENT)
 			ret = bpf_perf_link_attach(attr, prog);
-		else if (attr->link_create.attach_type == BPF_TRACE_KPROBE_MULTI)
+		else if (attr->link_create.attach_type == BPF_TRACE_KPROBE_MULTI ||
+			 attr->link_create.attach_type == BPF_TRACE_KPROBE_MULTI_SESSION)
 			ret = bpf_kprobe_multi_link_attach(attr, prog);
 		else if (attr->link_create.attach_type == BPF_TRACE_UPROBE_MULTI)
 			ret = bpf_uprobe_multi_link_attach(attr, prog);
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index afb232b1d7c2..3b15a40f425f 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1631,6 +1631,17 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 	}
 }
 
+static bool is_kprobe_multi(const struct bpf_prog *prog)
+{
+	return prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI ||
+	       prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI_SESSION;
+}
+
+static inline bool is_kprobe_multi_session(const struct bpf_prog *prog)
+{
+	return prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI_SESSION;
+}
+
 static const struct bpf_func_proto *
 kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
@@ -1646,13 +1657,13 @@ kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_override_return_proto;
 #endif
 	case BPF_FUNC_get_func_ip:
-		if (prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI)
+		if (is_kprobe_multi(prog))
 			return &bpf_get_func_ip_proto_kprobe_multi;
 		if (prog->expected_attach_type == BPF_TRACE_UPROBE_MULTI)
 			return &bpf_get_func_ip_proto_uprobe_multi;
 		return &bpf_get_func_ip_proto_kprobe;
 	case BPF_FUNC_get_attach_cookie:
-		if (prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI)
+		if (is_kprobe_multi(prog))
 			return &bpf_get_attach_cookie_proto_kmulti;
 		if (prog->expected_attach_type == BPF_TRACE_UPROBE_MULTI)
 			return &bpf_get_attach_cookie_proto_umulti;
@@ -2834,10 +2845,11 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long fentry_ip,
 			  void *data)
 {
 	struct bpf_kprobe_multi_link *link;
+	int err;
 
 	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
-	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
-	return 0;
+	err = kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
+	return is_kprobe_multi_session(link->link.prog) ? err : 0;
 }
 
 static void
@@ -2981,7 +2993,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 	if (sizeof(u64) != sizeof(void *))
 		return -EOPNOTSUPP;
 
-	if (prog->expected_attach_type != BPF_TRACE_KPROBE_MULTI)
+	if (!is_kprobe_multi(prog))
 		return -EINVAL;
 
 	flags = attr->link_create.kprobe_multi.flags;
@@ -3062,10 +3074,10 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 	if (err)
 		goto error;
 
-	if (flags & BPF_F_KPROBE_MULTI_RETURN)
-		link->fp.exit_handler = kprobe_multi_link_exit_handler;
-	else
+	if (!(flags & BPF_F_KPROBE_MULTI_RETURN))
 		link->fp.entry_handler = kprobe_multi_link_handler;
+	if ((flags & BPF_F_KPROBE_MULTI_RETURN) || is_kprobe_multi_session(prog))
+		link->fp.exit_handler = kprobe_multi_link_exit_handler;
 
 	link->addrs = addrs;
 	link->cookies = cookies;
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index cee0a7915c08..fb8ecb199273 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1115,6 +1115,7 @@ enum bpf_attach_type {
 	BPF_CGROUP_UNIX_GETSOCKNAME,
 	BPF_NETKIT_PRIMARY,
 	BPF_NETKIT_PEER,
+	BPF_TRACE_KPROBE_MULTI_SESSION,
 	__MAX_BPF_ATTACH_TYPE
 };
 
-- 
2.44.0

Re: [PATCH bpf-next 1/7] bpf: Add support for kprobe multi session attach

[Andrii Nakryiko]

On Mon, Apr 22, 2024 at 5:12 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support to attach bpf program for entry and return probe
> of the same function. This is common use case which at the moment
> requires to create two kprobe multi links.
>
> Adding new BPF_TRACE_KPROBE_MULTI_SESSION attach type that instructs
> kernel to attach single link program to both entry and exit probe.
>
> It's possible to control execution of the bpf program on return
> probe simply by returning zero or non zero from the entry bpf
> program execution to execute or not the bpf program on return
> probe respectively.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/uapi/linux/bpf.h       |  1 +
>  kernel/bpf/syscall.c           |  7 ++++++-
>  kernel/trace/bpf_trace.c       | 28 ++++++++++++++++++++--------
>  tools/include/uapi/linux/bpf.h |  1 +
>  4 files changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index cee0a7915c08..fb8ecb199273 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1115,6 +1115,7 @@ enum bpf_attach_type {
>         BPF_CGROUP_UNIX_GETSOCKNAME,
>         BPF_NETKIT_PRIMARY,
>         BPF_NETKIT_PEER,
> +       BPF_TRACE_KPROBE_MULTI_SESSION,

let's use a shorter BPF_TRACE_KPROBE_SESSION? we'll just know that
it's multi-variant (there is no point in adding non-multi kprobes
going forward anyways, it's a new default)

>         __MAX_BPF_ATTACH_TYPE
>  };
>

[...]

> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index afb232b1d7c2..3b15a40f425f 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1631,6 +1631,17 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>         }
>  }
>
> +static bool is_kprobe_multi(const struct bpf_prog *prog)
> +{
> +       return prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI ||
> +              prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI_SESSION;
> +}
> +
> +static inline bool is_kprobe_multi_session(const struct bpf_prog *prog)

ditto, this multi is just a distraction at this point, IMO

> +{
> +       return prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI_SESSION;
> +}
> +
>  static const struct bpf_func_proto *
>  kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  {

[...]

Re: [PATCH bpf-next 1/7] bpf: Add support for kprobe multi session attach

[Jiri Olsa]

On Tue, Apr 23, 2024 at 05:26:39PM -0700, Andrii Nakryiko wrote:
> On Mon, Apr 22, 2024 at 5:12 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding support to attach bpf program for entry and return probe
> > of the same function. This is common use case which at the moment
> > requires to create two kprobe multi links.
> >
> > Adding new BPF_TRACE_KPROBE_MULTI_SESSION attach type that instructs
> > kernel to attach single link program to both entry and exit probe.
> >
> > It's possible to control execution of the bpf program on return
> > probe simply by returning zero or non zero from the entry bpf
> > program execution to execute or not the bpf program on return
> > probe respectively.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/uapi/linux/bpf.h       |  1 +
> >  kernel/bpf/syscall.c           |  7 ++++++-
> >  kernel/trace/bpf_trace.c       | 28 ++++++++++++++++++++--------
> >  tools/include/uapi/linux/bpf.h |  1 +
> >  4 files changed, 28 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index cee0a7915c08..fb8ecb199273 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -1115,6 +1115,7 @@ enum bpf_attach_type {
> >         BPF_CGROUP_UNIX_GETSOCKNAME,
> >         BPF_NETKIT_PRIMARY,
> >         BPF_NETKIT_PEER,
> > +       BPF_TRACE_KPROBE_MULTI_SESSION,
> 
> let's use a shorter BPF_TRACE_KPROBE_SESSION? we'll just know that
> it's multi-variant (there is no point in adding non-multi kprobes
> going forward anyways, it's a new default)
> 
> >         __MAX_BPF_ATTACH_TYPE
> >  };
> >
> 
> [...]
> 
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index afb232b1d7c2..3b15a40f425f 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -1631,6 +1631,17 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >         }
> >  }
> >
> > +static bool is_kprobe_multi(const struct bpf_prog *prog)
> > +{
> > +       return prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI ||
> > +              prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI_SESSION;
> > +}
> > +
> > +static inline bool is_kprobe_multi_session(const struct bpf_prog *prog)
> 
> ditto, this multi is just a distraction at this point, IMO

ok, sounds good, will drop multi for session stuff

jirka

> 
> > +{
> > +       return prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI_SESSION;
> > +}
> > +
> >  static const struct bpf_func_proto *
> >  kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >  {
> 
> [...]

[PATCH bpf-next 2/7] bpf: Add support for kprobe multi session context

[Jiri Olsa]

Adding struct bpf_session_run_ctx object to hold session related
data, which is atm is_return bool and data pointer coming in
following changes.

Placing bpf_session_run_ctx layer in between bpf_run_ctx and
bpf_kprobe_multi_run_ctx so the session data can be retrieved
regardless of if it's kprobe_multi or uprobe_multi link, which
support is coming in future. This way both kprobe_multi and
uprobe_multi can use same kfuncs to access the session data.

Adding bpf_session_is_return kfunc that returns true if the
bpf program is executed from the exit probe of the kprobe multi
link attached in wrapper mode. It returns false otherwise.

Adding new kprobe hook for kprobe program type.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/btf.c         |  3 ++
 kernel/trace/bpf_trace.c | 67 +++++++++++++++++++++++++++++++++++-----
 2 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 6d46cee47ae3..7431230b1e3a 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -218,6 +218,7 @@ enum btf_kfunc_hook {
 	BTF_KFUNC_HOOK_SOCKET_FILTER,
 	BTF_KFUNC_HOOK_LWT,
 	BTF_KFUNC_HOOK_NETFILTER,
+	BTF_KFUNC_HOOK_KPROBE,
 	BTF_KFUNC_HOOK_MAX,
 };
 
@@ -8140,6 +8141,8 @@ static int bpf_prog_type_to_kfunc_hook(enum bpf_prog_type prog_type)
 		return BTF_KFUNC_HOOK_LWT;
 	case BPF_PROG_TYPE_NETFILTER:
 		return BTF_KFUNC_HOOK_NETFILTER;
+	case BPF_PROG_TYPE_KPROBE:
+		return BTF_KFUNC_HOOK_KPROBE;
 	default:
 		return BTF_KFUNC_HOOK_MAX;
 	}
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 3b15a40f425f..d82402316d84 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2596,6 +2596,11 @@ static int __init bpf_event_init(void)
 fs_initcall(bpf_event_init);
 #endif /* CONFIG_MODULES */
 
+struct bpf_session_run_ctx {
+	struct bpf_run_ctx run_ctx;
+	bool is_return;
+};
+
 #ifdef CONFIG_FPROBE
 struct bpf_kprobe_multi_link {
 	struct bpf_link link;
@@ -2609,7 +2614,7 @@ struct bpf_kprobe_multi_link {
 };
 
 struct bpf_kprobe_multi_run_ctx {
-	struct bpf_run_ctx run_ctx;
+	struct bpf_session_run_ctx session_ctx;
 	struct bpf_kprobe_multi_link *link;
 	unsigned long entry_ip;
 };
@@ -2788,7 +2793,8 @@ static u64 bpf_kprobe_multi_cookie(struct bpf_run_ctx *ctx)
 
 	if (WARN_ON_ONCE(!ctx))
 		return 0;
-	run_ctx = container_of(current->bpf_ctx, struct bpf_kprobe_multi_run_ctx, run_ctx);
+	run_ctx = container_of(current->bpf_ctx, struct bpf_kprobe_multi_run_ctx,
+			       session_ctx.run_ctx);
 	link = run_ctx->link;
 	if (!link->cookies)
 		return 0;
@@ -2805,15 +2811,20 @@ static u64 bpf_kprobe_multi_entry_ip(struct bpf_run_ctx *ctx)
 {
 	struct bpf_kprobe_multi_run_ctx *run_ctx;
 
-	run_ctx = container_of(current->bpf_ctx, struct bpf_kprobe_multi_run_ctx, run_ctx);
+	run_ctx = container_of(current->bpf_ctx, struct bpf_kprobe_multi_run_ctx,
+			       session_ctx.run_ctx);
 	return run_ctx->entry_ip;
 }
 
 static int
 kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
-			   unsigned long entry_ip, struct pt_regs *regs)
+			   unsigned long entry_ip, struct pt_regs *regs,
+			   bool is_return)
 {
 	struct bpf_kprobe_multi_run_ctx run_ctx = {
+		.session_ctx = {
+			.is_return = is_return,
+		},
 		.link = link,
 		.entry_ip = entry_ip,
 	};
@@ -2828,7 +2839,7 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
 
 	migrate_disable();
 	rcu_read_lock();
-	old_run_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);
+	old_run_ctx = bpf_set_run_ctx(&run_ctx.session_ctx.run_ctx);
 	err = bpf_prog_run(link->link.prog, regs);
 	bpf_reset_run_ctx(old_run_ctx);
 	rcu_read_unlock();
@@ -2848,7 +2859,7 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long fentry_ip,
 	int err;
 
 	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
-	err = kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
+	err = kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs, false);
 	return is_kprobe_multi_session(link->link.prog) ? err : 0;
 }
 
@@ -2860,7 +2871,7 @@ kprobe_multi_link_exit_handler(struct fprobe *fp, unsigned long fentry_ip,
 	struct bpf_kprobe_multi_link *link;
 
 	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
-	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
+	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs, true);
 }
 
 static int symbols_cmp_r(const void *a, const void *b, const void *priv)
@@ -3503,3 +3514,45 @@ static u64 bpf_uprobe_multi_entry_ip(struct bpf_run_ctx *ctx)
 	return 0;
 }
 #endif /* CONFIG_UPROBES */
+
+#ifdef CONFIG_FPROBE
+__bpf_kfunc_start_defs();
+
+__bpf_kfunc bool bpf_session_is_return(void)
+{
+	struct bpf_session_run_ctx *session_ctx;
+
+	session_ctx = container_of(current->bpf_ctx, struct bpf_session_run_ctx, run_ctx);
+	return session_ctx->is_return;
+}
+
+__bpf_kfunc_end_defs();
+
+BTF_KFUNCS_START(kprobe_multi_kfunc_set_ids)
+BTF_ID_FLAGS(func, bpf_session_is_return)
+BTF_KFUNCS_END(kprobe_multi_kfunc_set_ids)
+
+static int bpf_kprobe_multi_filter(const struct bpf_prog *prog, u32 kfunc_id)
+{
+	if (!btf_id_set8_contains(&kprobe_multi_kfunc_set_ids, kfunc_id))
+		return 0;
+
+	if (!is_kprobe_multi_session(prog))
+		return -EACCES;
+
+	return 0;
+}
+
+static const struct btf_kfunc_id_set bpf_kprobe_multi_kfunc_set = {
+	.owner = THIS_MODULE,
+	.set = &kprobe_multi_kfunc_set_ids,
+	.filter = bpf_kprobe_multi_filter,
+};
+
+static int __init bpf_kprobe_multi_kfuncs_init(void)
+{
+	return register_btf_kfunc_id_set(BPF_PROG_TYPE_KPROBE, &bpf_kprobe_multi_kfunc_set);
+}
+
+late_initcall(bpf_kprobe_multi_kfuncs_init);
+#endif
-- 
2.44.0

Re: [PATCH bpf-next 2/7] bpf: Add support for kprobe multi session context

[Andrii Nakryiko]

On Mon, Apr 22, 2024 at 5:13 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding struct bpf_session_run_ctx object to hold session related
> data, which is atm is_return bool and data pointer coming in
> following changes.
>
> Placing bpf_session_run_ctx layer in between bpf_run_ctx and
> bpf_kprobe_multi_run_ctx so the session data can be retrieved
> regardless of if it's kprobe_multi or uprobe_multi link, which
> support is coming in future. This way both kprobe_multi and
> uprobe_multi can use same kfuncs to access the session data.
>
> Adding bpf_session_is_return kfunc that returns true if the
> bpf program is executed from the exit probe of the kprobe multi
> link attached in wrapper mode. It returns false otherwise.
>
> Adding new kprobe hook for kprobe program type.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/bpf/btf.c         |  3 ++
>  kernel/trace/bpf_trace.c | 67 +++++++++++++++++++++++++++++++++++-----
>  2 files changed, 63 insertions(+), 7 deletions(-)
>

LGTM, but see the question below

Acked-by: Andrii Nakryiko <andrii@kernel.org>

[...]

> @@ -2848,7 +2859,7 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long fentry_ip,
>         int err;
>
>         link = container_of(fp, struct bpf_kprobe_multi_link, fp);
> -       err = kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
> +       err = kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs, false);
>         return is_kprobe_multi_session(link->link.prog) ? err : 0;
>  }
>
> @@ -2860,7 +2871,7 @@ kprobe_multi_link_exit_handler(struct fprobe *fp, unsigned long fentry_ip,
>         struct bpf_kprobe_multi_link *link;
>
>         link = container_of(fp, struct bpf_kprobe_multi_link, fp);
> -       kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
> +       kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs, true);

Is there some way to figure out whether we are an entry or return
probe from struct fprobe itself? I was hoping to have a single
callback for both entry and exit handler in fprobe to keep callback
call chain a bit simpler


>  }
>
>  static int symbols_cmp_r(const void *a, const void *b, const void *priv)

[...]

Re: [PATCH bpf-next 2/7] bpf: Add support for kprobe multi session context

[Jiri Olsa]

On Tue, Apr 23, 2024 at 05:26:45PM -0700, Andrii Nakryiko wrote:
> On Mon, Apr 22, 2024 at 5:13 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding struct bpf_session_run_ctx object to hold session related
> > data, which is atm is_return bool and data pointer coming in
> > following changes.
> >
> > Placing bpf_session_run_ctx layer in between bpf_run_ctx and
> > bpf_kprobe_multi_run_ctx so the session data can be retrieved
> > regardless of if it's kprobe_multi or uprobe_multi link, which
> > support is coming in future. This way both kprobe_multi and
> > uprobe_multi can use same kfuncs to access the session data.
> >
> > Adding bpf_session_is_return kfunc that returns true if the
> > bpf program is executed from the exit probe of the kprobe multi
> > link attached in wrapper mode. It returns false otherwise.
> >
> > Adding new kprobe hook for kprobe program type.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/bpf/btf.c         |  3 ++
> >  kernel/trace/bpf_trace.c | 67 +++++++++++++++++++++++++++++++++++-----
> >  2 files changed, 63 insertions(+), 7 deletions(-)
> >
> 
> LGTM, but see the question below
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> [...]
> 
> > @@ -2848,7 +2859,7 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long fentry_ip,
> >         int err;
> >
> >         link = container_of(fp, struct bpf_kprobe_multi_link, fp);
> > -       err = kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
> > +       err = kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs, false);
> >         return is_kprobe_multi_session(link->link.prog) ? err : 0;
> >  }
> >
> > @@ -2860,7 +2871,7 @@ kprobe_multi_link_exit_handler(struct fprobe *fp, unsigned long fentry_ip,
> >         struct bpf_kprobe_multi_link *link;
> >
> >         link = container_of(fp, struct bpf_kprobe_multi_link, fp);
> > -       kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
> > +       kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs, true);
> 
> Is there some way to figure out whether we are an entry or return
> probe from struct fprobe itself? I was hoping to have a single
> callback for both entry and exit handler in fprobe to keep callback
> call chain a bit simpler

AFAICS not at the moment.. also both callbacks have same arguments,
just the entry handler returns int and exit handler void

jirka

[PATCH bpf-next 3/7] bpf: Add support for kprobe multi session cookie

[Jiri Olsa]

Adding support for cookie within the session of kprobe multi
entry and return program.

The session cookie is u64 value and can be retrieved be new
kfunc bpf_session_cookie, which returns pointer to the cookie
value. The bpf program can use the pointer to store (on entry)
and load (on return) the value.

The cookie value is implemented via fprobe feature that allows
to share values between entry and return ftrace fprobe callbacks.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/verifier.c    |  7 +++++++
 kernel/trace/bpf_trace.c | 19 ++++++++++++++++---
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 68cfd6fc6ad4..baaca451aebc 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10987,6 +10987,7 @@ enum special_kfunc_type {
 	KF_bpf_percpu_obj_drop_impl,
 	KF_bpf_throw,
 	KF_bpf_iter_css_task_new,
+	KF_bpf_session_cookie,
 };
 
 BTF_SET_START(special_kfunc_set)
@@ -11013,6 +11014,7 @@ BTF_ID(func, bpf_throw)
 #ifdef CONFIG_CGROUPS
 BTF_ID(func, bpf_iter_css_task_new)
 #endif
+BTF_ID(func, bpf_session_cookie)
 BTF_SET_END(special_kfunc_set)
 
 BTF_ID_LIST(special_kfunc_list)
@@ -11043,6 +11045,7 @@ BTF_ID(func, bpf_iter_css_task_new)
 #else
 BTF_ID_UNUSED
 #endif
+BTF_ID(func, bpf_session_cookie)
 
 static bool is_kfunc_ret_null(struct bpf_kfunc_call_arg_meta *meta)
 {
@@ -12409,6 +12412,10 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 				 * because packet slices are not refcounted (see
 				 * dynptr_type_refcounted)
 				 */
+			} else if (meta.func_id == special_kfunc_list[KF_bpf_session_cookie]) {
+				mark_reg_known_zero(env, regs, BPF_REG_0);
+				regs[BPF_REG_0].type = PTR_TO_MEM;
+				regs[BPF_REG_0].mem_size = sizeof(u64);
 			} else {
 				verbose(env, "kernel function %s unhandled dynamic return type\n",
 					meta.func_name);
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index d82402316d84..a6863c1905ca 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2599,6 +2599,7 @@ fs_initcall(bpf_event_init);
 struct bpf_session_run_ctx {
 	struct bpf_run_ctx run_ctx;
 	bool is_return;
+	void *data;
 };
 
 #ifdef CONFIG_FPROBE
@@ -2819,11 +2820,12 @@ static u64 bpf_kprobe_multi_entry_ip(struct bpf_run_ctx *ctx)
 static int
 kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
 			   unsigned long entry_ip, struct pt_regs *regs,
-			   bool is_return)
+			   bool is_return, void *data)
 {
 	struct bpf_kprobe_multi_run_ctx run_ctx = {
 		.session_ctx = {
 			.is_return = is_return,
+			.data = data,
 		},
 		.link = link,
 		.entry_ip = entry_ip,
@@ -2859,7 +2861,7 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long fentry_ip,
 	int err;
 
 	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
-	err = kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs, false);
+	err = kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs, false, data);
 	return is_kprobe_multi_session(link->link.prog) ? err : 0;
 }
 
@@ -2871,7 +2873,7 @@ kprobe_multi_link_exit_handler(struct fprobe *fp, unsigned long fentry_ip,
 	struct bpf_kprobe_multi_link *link;
 
 	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
-	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs, true);
+	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs, true, data);
 }
 
 static int symbols_cmp_r(const void *a, const void *b, const void *priv)
@@ -3089,6 +3091,8 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 		link->fp.entry_handler = kprobe_multi_link_handler;
 	if ((flags & BPF_F_KPROBE_MULTI_RETURN) || is_kprobe_multi_session(prog))
 		link->fp.exit_handler = kprobe_multi_link_exit_handler;
+	if (is_kprobe_multi_session(prog))
+		link->fp.entry_data_size = sizeof(u64);
 
 	link->addrs = addrs;
 	link->cookies = cookies;
@@ -3526,10 +3530,19 @@ __bpf_kfunc bool bpf_session_is_return(void)
 	return session_ctx->is_return;
 }
 
+__bpf_kfunc __u64 *bpf_session_cookie(void)
+{
+	struct bpf_session_run_ctx *session_ctx;
+
+	session_ctx = container_of(current->bpf_ctx, struct bpf_session_run_ctx, run_ctx);
+	return session_ctx->data;
+}
+
 __bpf_kfunc_end_defs();
 
 BTF_KFUNCS_START(kprobe_multi_kfunc_set_ids)
 BTF_ID_FLAGS(func, bpf_session_is_return)
+BTF_ID_FLAGS(func, bpf_session_cookie)
 BTF_KFUNCS_END(kprobe_multi_kfunc_set_ids)
 
 static int bpf_kprobe_multi_filter(const struct bpf_prog *prog, u32 kfunc_id)
-- 
2.44.0

Re: [PATCH bpf-next 3/7] bpf: Add support for kprobe multi session cookie

[Alexei Starovoitov]

On Mon, Apr 22, 2024 at 5:13 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support for cookie within the session of kprobe multi
> entry and return program.
>
> The session cookie is u64 value and can be retrieved be new
> kfunc bpf_session_cookie, which returns pointer to the cookie
> value. The bpf program can use the pointer to store (on entry)
> and load (on return) the value.
>
> The cookie value is implemented via fprobe feature that allows
> to share values between entry and return ftrace fprobe callbacks.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/bpf/verifier.c    |  7 +++++++
>  kernel/trace/bpf_trace.c | 19 ++++++++++++++++---
>  2 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 68cfd6fc6ad4..baaca451aebc 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -10987,6 +10987,7 @@ enum special_kfunc_type {
>         KF_bpf_percpu_obj_drop_impl,
>         KF_bpf_throw,
>         KF_bpf_iter_css_task_new,
> +       KF_bpf_session_cookie,
>  };
>
>  BTF_SET_START(special_kfunc_set)
> @@ -11013,6 +11014,7 @@ BTF_ID(func, bpf_throw)
>  #ifdef CONFIG_CGROUPS
>  BTF_ID(func, bpf_iter_css_task_new)
>  #endif
> +BTF_ID(func, bpf_session_cookie)
>  BTF_SET_END(special_kfunc_set)
>
>  BTF_ID_LIST(special_kfunc_list)
> @@ -11043,6 +11045,7 @@ BTF_ID(func, bpf_iter_css_task_new)
>  #else
>  BTF_ID_UNUSED
>  #endif
> +BTF_ID(func, bpf_session_cookie)
>
>  static bool is_kfunc_ret_null(struct bpf_kfunc_call_arg_meta *meta)
>  {
> @@ -12409,6 +12412,10 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>                                  * because packet slices are not refcounted (see
>                                  * dynptr_type_refcounted)
>                                  */
> +                       } else if (meta.func_id == special_kfunc_list[KF_bpf_session_cookie]) {
> +                               mark_reg_known_zero(env, regs, BPF_REG_0);
> +                               regs[BPF_REG_0].type = PTR_TO_MEM;
> +                               regs[BPF_REG_0].mem_size = sizeof(u64);

Are you sure you need this?

} else if (!__btf_type_is_struct(ptr_type)) {

block should have handled it automatically.

> +__bpf_kfunc __u64 *bpf_session_cookie(void)
> +{

...

Re: [PATCH bpf-next 3/7] bpf: Add support for kprobe multi session cookie

[Jiri Olsa]

On Mon, Apr 22, 2024 at 10:48:25AM -0700, Alexei Starovoitov wrote:
> On Mon, Apr 22, 2024 at 5:13 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding support for cookie within the session of kprobe multi
> > entry and return program.
> >
> > The session cookie is u64 value and can be retrieved be new
> > kfunc bpf_session_cookie, which returns pointer to the cookie
> > value. The bpf program can use the pointer to store (on entry)
> > and load (on return) the value.
> >
> > The cookie value is implemented via fprobe feature that allows
> > to share values between entry and return ftrace fprobe callbacks.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/bpf/verifier.c    |  7 +++++++
> >  kernel/trace/bpf_trace.c | 19 ++++++++++++++++---
> >  2 files changed, 23 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 68cfd6fc6ad4..baaca451aebc 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -10987,6 +10987,7 @@ enum special_kfunc_type {
> >         KF_bpf_percpu_obj_drop_impl,
> >         KF_bpf_throw,
> >         KF_bpf_iter_css_task_new,
> > +       KF_bpf_session_cookie,
> >  };
> >
> >  BTF_SET_START(special_kfunc_set)
> > @@ -11013,6 +11014,7 @@ BTF_ID(func, bpf_throw)
> >  #ifdef CONFIG_CGROUPS
> >  BTF_ID(func, bpf_iter_css_task_new)
> >  #endif
> > +BTF_ID(func, bpf_session_cookie)
> >  BTF_SET_END(special_kfunc_set)
> >
> >  BTF_ID_LIST(special_kfunc_list)
> > @@ -11043,6 +11045,7 @@ BTF_ID(func, bpf_iter_css_task_new)
> >  #else
> >  BTF_ID_UNUSED
> >  #endif
> > +BTF_ID(func, bpf_session_cookie)
> >
> >  static bool is_kfunc_ret_null(struct bpf_kfunc_call_arg_meta *meta)
> >  {
> > @@ -12409,6 +12412,10 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> >                                  * because packet slices are not refcounted (see
> >                                  * dynptr_type_refcounted)
> >                                  */
> > +                       } else if (meta.func_id == special_kfunc_list[KF_bpf_session_cookie]) {
> > +                               mark_reg_known_zero(env, regs, BPF_REG_0);
> > +                               regs[BPF_REG_0].type = PTR_TO_MEM;
> > +                               regs[BPF_REG_0].mem_size = sizeof(u64);
> 
> Are you sure you need this?
> 
> } else if (!__btf_type_is_struct(ptr_type)) {
> 
> block should have handled it automatically.

yes, but only as read-only memory and we need the bpf program to be able to write
to it

I'll double check that, but AFAICS we can't set r0_size/!r0_rdonly before we reach
that if block, because bpf_session_cookie has no arguments

jirka

> 
> > +__bpf_kfunc __u64 *bpf_session_cookie(void)
> > +{
> 
> ...

Re: [PATCH bpf-next 3/7] bpf: Add support for kprobe multi session cookie

[Andrii Nakryiko]

On Mon, Apr 22, 2024 at 5:13 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support for cookie within the session of kprobe multi
> entry and return program.
>
> The session cookie is u64 value and can be retrieved be new
> kfunc bpf_session_cookie, which returns pointer to the cookie
> value. The bpf program can use the pointer to store (on entry)
> and load (on return) the value.
>
> The cookie value is implemented via fprobe feature that allows
> to share values between entry and return ftrace fprobe callbacks.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/bpf/verifier.c    |  7 +++++++
>  kernel/trace/bpf_trace.c | 19 ++++++++++++++++---
>  2 files changed, 23 insertions(+), 3 deletions(-)
>

Had the same question as Alexei, but this read-write semantics quirk
makes sense. But it's probably a bit more reliable and cleaner to
handle it by special casing this kfunc a bit earlier (see
KF_bpf_rbtree_add_impl) and setting r0_size = 8, r0_rdonly = false.
And then let generic PTR -> INT logic kick in. You'll be futzing with
register state much less.

Other than that looks good:

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 68cfd6fc6ad4..baaca451aebc 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -10987,6 +10987,7 @@ enum special_kfunc_type {
>         KF_bpf_percpu_obj_drop_impl,
>         KF_bpf_throw,
>         KF_bpf_iter_css_task_new,
> +       KF_bpf_session_cookie,
>  };

[...]

Re: [PATCH bpf-next 3/7] bpf: Add support for kprobe multi session cookie

[Jiri Olsa]

On Tue, Apr 23, 2024 at 05:26:51PM -0700, Andrii Nakryiko wrote:
> On Mon, Apr 22, 2024 at 5:13 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding support for cookie within the session of kprobe multi
> > entry and return program.
> >
> > The session cookie is u64 value and can be retrieved be new
> > kfunc bpf_session_cookie, which returns pointer to the cookie
> > value. The bpf program can use the pointer to store (on entry)
> > and load (on return) the value.
> >
> > The cookie value is implemented via fprobe feature that allows
> > to share values between entry and return ftrace fprobe callbacks.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/bpf/verifier.c    |  7 +++++++
> >  kernel/trace/bpf_trace.c | 19 ++++++++++++++++---
> >  2 files changed, 23 insertions(+), 3 deletions(-)
> >
> 
> Had the same question as Alexei, but this read-write semantics quirk
> makes sense. But it's probably a bit more reliable and cleaner to
> handle it by special casing this kfunc a bit earlier (see
> KF_bpf_rbtree_add_impl) and setting r0_size = 8, r0_rdonly = false.
> And then let generic PTR -> INT logic kick in. You'll be futzing with
> register state much less.

ok, will try it this way

thanks,
jirka

> 
> Other than that looks good:
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 68cfd6fc6ad4..baaca451aebc 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -10987,6 +10987,7 @@ enum special_kfunc_type {
> >         KF_bpf_percpu_obj_drop_impl,
> >         KF_bpf_throw,
> >         KF_bpf_iter_css_task_new,
> > +       KF_bpf_session_cookie,
> >  };
> 
> [...]

[PATCH bpf-next 4/7] libbpf: Add support for kprobe multi session attach

[Jiri Olsa]

Adding support to attach program in kprobe multi session mode
with bpf_program__attach_kprobe_multi_opts function.

Adding session bool to bpf_kprobe_multi_opts struct that allows
to load and attach the bpf program via kprobe multi session.
the attachment to create kprobe multi session.

Also adding new program loader section that allows:
 SEC("kprobe.session/bpf_fentry_test*")

and loads/attaches kprobe program as kprobe multi session.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/lib/bpf/bpf.c    |  1 +
 tools/lib/bpf/libbpf.c | 40 ++++++++++++++++++++++++++++++++++++++--
 tools/lib/bpf/libbpf.h |  4 +++-
 3 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index c9f4e04f38fe..5f556e38910f 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -766,6 +766,7 @@ int bpf_link_create(int prog_fd, int target_fd,
 			return libbpf_err(-EINVAL);
 		break;
 	case BPF_TRACE_KPROBE_MULTI:
+	case BPF_TRACE_KPROBE_MULTI_SESSION:
 		attr.link_create.kprobe_multi.flags = OPTS_GET(opts, kprobe_multi.flags, 0);
 		attr.link_create.kprobe_multi.cnt = OPTS_GET(opts, kprobe_multi.cnt, 0);
 		attr.link_create.kprobe_multi.syms = ptr_to_u64(OPTS_GET(opts, kprobe_multi.syms, 0));
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 97eb6e5dd7c8..ca605240205f 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -9272,6 +9272,7 @@ static int attach_tp(const struct bpf_program *prog, long cookie, struct bpf_lin
 static int attach_raw_tp(const struct bpf_program *prog, long cookie, struct bpf_link **link);
 static int attach_trace(const struct bpf_program *prog, long cookie, struct bpf_link **link);
 static int attach_kprobe_multi(const struct bpf_program *prog, long cookie, struct bpf_link **link);
+static int attach_kprobe_session(const struct bpf_program *prog, long cookie, struct bpf_link **link);
 static int attach_uprobe_multi(const struct bpf_program *prog, long cookie, struct bpf_link **link);
 static int attach_lsm(const struct bpf_program *prog, long cookie, struct bpf_link **link);
 static int attach_iter(const struct bpf_program *prog, long cookie, struct bpf_link **link);
@@ -9288,6 +9289,7 @@ static const struct bpf_sec_def section_defs[] = {
 	SEC_DEF("uretprobe.s+",		KPROBE, 0, SEC_SLEEPABLE, attach_uprobe),
 	SEC_DEF("kprobe.multi+",	KPROBE,	BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe_multi),
 	SEC_DEF("kretprobe.multi+",	KPROBE,	BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe_multi),
+	SEC_DEF("kprobe.session+",	KPROBE,	BPF_TRACE_KPROBE_MULTI_SESSION, SEC_NONE, attach_kprobe_session),
 	SEC_DEF("uprobe.multi+",	KPROBE,	BPF_TRACE_UPROBE_MULTI, SEC_NONE, attach_uprobe_multi),
 	SEC_DEF("uretprobe.multi+",	KPROBE,	BPF_TRACE_UPROBE_MULTI, SEC_NONE, attach_uprobe_multi),
 	SEC_DEF("uprobe.multi.s+",	KPROBE,	BPF_TRACE_UPROBE_MULTI, SEC_SLEEPABLE, attach_uprobe_multi),
@@ -11380,13 +11382,14 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
 	struct kprobe_multi_resolve res = {
 		.pattern = pattern,
 	};
+	enum bpf_attach_type attach_type;
 	struct bpf_link *link = NULL;
 	char errmsg[STRERR_BUFSIZE];
 	const unsigned long *addrs;
 	int err, link_fd, prog_fd;
+	bool retprobe, session;
 	const __u64 *cookies;
 	const char **syms;
-	bool retprobe;
 	size_t cnt;
 
 	if (!OPTS_VALID(opts, bpf_kprobe_multi_opts))
@@ -11425,6 +11428,13 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
 	}
 
 	retprobe = OPTS_GET(opts, retprobe, false);
+	session  = OPTS_GET(opts, session, false);
+
+	if (retprobe && session)
+		return libbpf_err_ptr(-EINVAL);
+
+	attach_type = session ? BPF_TRACE_KPROBE_MULTI_SESSION :
+				BPF_TRACE_KPROBE_MULTI;
 
 	lopts.kprobe_multi.syms = syms;
 	lopts.kprobe_multi.addrs = addrs;
@@ -11439,7 +11449,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
 	}
 	link->detach = &bpf_link__detach_fd;
 
-	link_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, &lopts);
+	link_fd = bpf_link_create(prog_fd, 0, attach_type, &lopts);
 	if (link_fd < 0) {
 		err = -errno;
 		pr_warn("prog '%s': failed to attach: %s\n",
@@ -11545,6 +11555,32 @@ static int attach_kprobe_multi(const struct bpf_program *prog, long cookie, stru
 	return libbpf_get_error(*link);
 }
 
+static int attach_kprobe_session(const struct bpf_program *prog, long cookie,
+				 struct bpf_link **link)
+{
+	LIBBPF_OPTS(bpf_kprobe_multi_opts, opts, .session = true);
+	const char *spec;
+	char *pattern;
+	int n;
+
+	*link = NULL;
+
+	/* no auto-attach for SEC("kprobe.session") */
+	if (strcmp(prog->sec_name, "kprobe.session") == 0)
+		return 0;
+
+	spec = prog->sec_name + sizeof("kprobe.session/") - 1;
+	n = sscanf(spec, "%m[a-zA-Z0-9_.*?]", &pattern);
+	if (n < 1) {
+		pr_warn("kprobe session pattern is invalid: %s\n", pattern);
+		return -EINVAL;
+	}
+
+	*link = bpf_program__attach_kprobe_multi_opts(prog, pattern, &opts);
+	free(pattern);
+	return libbpf_get_error(*link);
+}
+
 static int attach_uprobe_multi(const struct bpf_program *prog, long cookie, struct bpf_link **link)
 {
 	char *probe_type = NULL, *binary_path = NULL, *func_name = NULL;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 1333ae20ebe6..c3f77d9260fe 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -539,10 +539,12 @@ struct bpf_kprobe_multi_opts {
 	size_t cnt;
 	/* create return kprobes */
 	bool retprobe;
+	/* create session kprobes */
+	bool session;
 	size_t :0;
 };
 
-#define bpf_kprobe_multi_opts__last_field retprobe
+#define bpf_kprobe_multi_opts__last_field session
 
 LIBBPF_API struct bpf_link *
 bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
-- 
2.44.0

Re: [PATCH bpf-next 4/7] libbpf: Add support for kprobe multi session attach

[Andrii Nakryiko]

On Mon, Apr 22, 2024 at 5:13 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support to attach program in kprobe multi session mode
> with bpf_program__attach_kprobe_multi_opts function.
>
> Adding session bool to bpf_kprobe_multi_opts struct that allows
> to load and attach the bpf program via kprobe multi session.
> the attachment to create kprobe multi session.
>
> Also adding new program loader section that allows:
>  SEC("kprobe.session/bpf_fentry_test*")
>
> and loads/attaches kprobe program as kprobe multi session.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/lib/bpf/bpf.c    |  1 +
>  tools/lib/bpf/libbpf.c | 40 ++++++++++++++++++++++++++++++++++++++--
>  tools/lib/bpf/libbpf.h |  4 +++-
>  3 files changed, 42 insertions(+), 3 deletions(-)
>

Minor nits below, but LGTM overall:

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index c9f4e04f38fe..5f556e38910f 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -766,6 +766,7 @@ int bpf_link_create(int prog_fd, int target_fd,
>                         return libbpf_err(-EINVAL);
>                 break;
>         case BPF_TRACE_KPROBE_MULTI:
> +       case BPF_TRACE_KPROBE_MULTI_SESSION:
>                 attr.link_create.kprobe_multi.flags = OPTS_GET(opts, kprobe_multi.flags, 0);
>                 attr.link_create.kprobe_multi.cnt = OPTS_GET(opts, kprobe_multi.cnt, 0);
>                 attr.link_create.kprobe_multi.syms = ptr_to_u64(OPTS_GET(opts, kprobe_multi.syms, 0));
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 97eb6e5dd7c8..ca605240205f 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -9272,6 +9272,7 @@ static int attach_tp(const struct bpf_program *prog, long cookie, struct bpf_lin
>  static int attach_raw_tp(const struct bpf_program *prog, long cookie, struct bpf_link **link);
>  static int attach_trace(const struct bpf_program *prog, long cookie, struct bpf_link **link);
>  static int attach_kprobe_multi(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> +static int attach_kprobe_session(const struct bpf_program *prog, long cookie, struct bpf_link **link);
>  static int attach_uprobe_multi(const struct bpf_program *prog, long cookie, struct bpf_link **link);
>  static int attach_lsm(const struct bpf_program *prog, long cookie, struct bpf_link **link);
>  static int attach_iter(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> @@ -9288,6 +9289,7 @@ static const struct bpf_sec_def section_defs[] = {
>         SEC_DEF("uretprobe.s+",         KPROBE, 0, SEC_SLEEPABLE, attach_uprobe),
>         SEC_DEF("kprobe.multi+",        KPROBE, BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe_multi),
>         SEC_DEF("kretprobe.multi+",     KPROBE, BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe_multi),
> +       SEC_DEF("kprobe.session+",      KPROBE, BPF_TRACE_KPROBE_MULTI_SESSION, SEC_NONE, attach_kprobe_session),
>         SEC_DEF("uprobe.multi+",        KPROBE, BPF_TRACE_UPROBE_MULTI, SEC_NONE, attach_uprobe_multi),
>         SEC_DEF("uretprobe.multi+",     KPROBE, BPF_TRACE_UPROBE_MULTI, SEC_NONE, attach_uprobe_multi),
>         SEC_DEF("uprobe.multi.s+",      KPROBE, BPF_TRACE_UPROBE_MULTI, SEC_SLEEPABLE, attach_uprobe_multi),
> @@ -11380,13 +11382,14 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
>         struct kprobe_multi_resolve res = {
>                 .pattern = pattern,
>         };
> +       enum bpf_attach_type attach_type;
>         struct bpf_link *link = NULL;
>         char errmsg[STRERR_BUFSIZE];
>         const unsigned long *addrs;
>         int err, link_fd, prog_fd;
> +       bool retprobe, session;
>         const __u64 *cookies;
>         const char **syms;
> -       bool retprobe;
>         size_t cnt;
>
>         if (!OPTS_VALID(opts, bpf_kprobe_multi_opts))
> @@ -11425,6 +11428,13 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
>         }
>
>         retprobe = OPTS_GET(opts, retprobe, false);
> +       session  = OPTS_GET(opts, session, false);
> +
> +       if (retprobe && session)
> +               return libbpf_err_ptr(-EINVAL);
> +
> +       attach_type = session ? BPF_TRACE_KPROBE_MULTI_SESSION :
> +                               BPF_TRACE_KPROBE_MULTI;

doesn't fit under 100?

>
>         lopts.kprobe_multi.syms = syms;
>         lopts.kprobe_multi.addrs = addrs;
> @@ -11439,7 +11449,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
>         }
>         link->detach = &bpf_link__detach_fd;
>
> -       link_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, &lopts);
> +       link_fd = bpf_link_create(prog_fd, 0, attach_type, &lopts);
>         if (link_fd < 0) {
>                 err = -errno;
>                 pr_warn("prog '%s': failed to attach: %s\n",
> @@ -11545,6 +11555,32 @@ static int attach_kprobe_multi(const struct bpf_program *prog, long cookie, stru
>         return libbpf_get_error(*link);
>  }
>
> +static int attach_kprobe_session(const struct bpf_program *prog, long cookie,
> +                                struct bpf_link **link)
> +{
> +       LIBBPF_OPTS(bpf_kprobe_multi_opts, opts, .session = true);
> +       const char *spec;
> +       char *pattern;
> +       int n;
> +
> +       *link = NULL;
> +
> +       /* no auto-attach for SEC("kprobe.session") */
> +       if (strcmp(prog->sec_name, "kprobe.session") == 0)
> +               return 0;
> +
> +       spec = prog->sec_name + sizeof("kprobe.session/") - 1;
> +       n = sscanf(spec, "%m[a-zA-Z0-9_.*?]", &pattern);
> +       if (n < 1) {
> +               pr_warn("kprobe session pattern is invalid: %s\n", pattern);
> +               return -EINVAL;
> +       }
> +
> +       *link = bpf_program__attach_kprobe_multi_opts(prog, pattern, &opts);
> +       free(pattern);
> +       return libbpf_get_error(*link);

let's try not to add new uses of libbpf_get_error? Would this work:

return *link ? 0 : -errno;

?


> +}
> +
>  static int attach_uprobe_multi(const struct bpf_program *prog, long cookie, struct bpf_link **link)
>  {
>         char *probe_type = NULL, *binary_path = NULL, *func_name = NULL;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 1333ae20ebe6..c3f77d9260fe 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -539,10 +539,12 @@ struct bpf_kprobe_multi_opts {
>         size_t cnt;
>         /* create return kprobes */
>         bool retprobe;
> +       /* create session kprobes */
> +       bool session;
>         size_t :0;
>  };
>
> -#define bpf_kprobe_multi_opts__last_field retprobe
> +#define bpf_kprobe_multi_opts__last_field session
>
>  LIBBPF_API struct bpf_link *
>  bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
> --
> 2.44.0
>

Re: [PATCH bpf-next 4/7] libbpf: Add support for kprobe multi session attach

[Jiri Olsa]

On Tue, Apr 23, 2024 at 05:26:59PM -0700, Andrii Nakryiko wrote:

SNIP

> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 97eb6e5dd7c8..ca605240205f 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -9272,6 +9272,7 @@ static int attach_tp(const struct bpf_program *prog, long cookie, struct bpf_lin
> >  static int attach_raw_tp(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> >  static int attach_trace(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> >  static int attach_kprobe_multi(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> > +static int attach_kprobe_session(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> >  static int attach_uprobe_multi(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> >  static int attach_lsm(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> >  static int attach_iter(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> > @@ -9288,6 +9289,7 @@ static const struct bpf_sec_def section_defs[] = {
> >         SEC_DEF("uretprobe.s+",         KPROBE, 0, SEC_SLEEPABLE, attach_uprobe),
> >         SEC_DEF("kprobe.multi+",        KPROBE, BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe_multi),
> >         SEC_DEF("kretprobe.multi+",     KPROBE, BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe_multi),
> > +       SEC_DEF("kprobe.session+",      KPROBE, BPF_TRACE_KPROBE_MULTI_SESSION, SEC_NONE, attach_kprobe_session),
> >         SEC_DEF("uprobe.multi+",        KPROBE, BPF_TRACE_UPROBE_MULTI, SEC_NONE, attach_uprobe_multi),
> >         SEC_DEF("uretprobe.multi+",     KPROBE, BPF_TRACE_UPROBE_MULTI, SEC_NONE, attach_uprobe_multi),
> >         SEC_DEF("uprobe.multi.s+",      KPROBE, BPF_TRACE_UPROBE_MULTI, SEC_SLEEPABLE, attach_uprobe_multi),
> > @@ -11380,13 +11382,14 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
> >         struct kprobe_multi_resolve res = {
> >                 .pattern = pattern,
> >         };
> > +       enum bpf_attach_type attach_type;
> >         struct bpf_link *link = NULL;
> >         char errmsg[STRERR_BUFSIZE];
> >         const unsigned long *addrs;
> >         int err, link_fd, prog_fd;
> > +       bool retprobe, session;
> >         const __u64 *cookies;
> >         const char **syms;
> > -       bool retprobe;
> >         size_t cnt;
> >
> >         if (!OPTS_VALID(opts, bpf_kprobe_multi_opts))
> > @@ -11425,6 +11428,13 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
> >         }
> >
> >         retprobe = OPTS_GET(opts, retprobe, false);
> > +       session  = OPTS_GET(opts, session, false);
> > +
> > +       if (retprobe && session)
> > +               return libbpf_err_ptr(-EINVAL);
> > +
> > +       attach_type = session ? BPF_TRACE_KPROBE_MULTI_SESSION :
> > +                               BPF_TRACE_KPROBE_MULTI;
> 
> doesn't fit under 100?

88 ;-) ok

> 
> >
> >         lopts.kprobe_multi.syms = syms;
> >         lopts.kprobe_multi.addrs = addrs;
> > @@ -11439,7 +11449,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
> >         }
> >         link->detach = &bpf_link__detach_fd;
> >
> > -       link_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, &lopts);
> > +       link_fd = bpf_link_create(prog_fd, 0, attach_type, &lopts);
> >         if (link_fd < 0) {
> >                 err = -errno;
> >                 pr_warn("prog '%s': failed to attach: %s\n",
> > @@ -11545,6 +11555,32 @@ static int attach_kprobe_multi(const struct bpf_program *prog, long cookie, stru
> >         return libbpf_get_error(*link);
> >  }
> >
> > +static int attach_kprobe_session(const struct bpf_program *prog, long cookie,
> > +                                struct bpf_link **link)
> > +{
> > +       LIBBPF_OPTS(bpf_kprobe_multi_opts, opts, .session = true);
> > +       const char *spec;
> > +       char *pattern;
> > +       int n;
> > +
> > +       *link = NULL;
> > +
> > +       /* no auto-attach for SEC("kprobe.session") */
> > +       if (strcmp(prog->sec_name, "kprobe.session") == 0)
> > +               return 0;
> > +
> > +       spec = prog->sec_name + sizeof("kprobe.session/") - 1;
> > +       n = sscanf(spec, "%m[a-zA-Z0-9_.*?]", &pattern);
> > +       if (n < 1) {
> > +               pr_warn("kprobe session pattern is invalid: %s\n", pattern);
> > +               return -EINVAL;
> > +       }
> > +
> > +       *link = bpf_program__attach_kprobe_multi_opts(prog, pattern, &opts);
> > +       free(pattern);
> > +       return libbpf_get_error(*link);
> 
> let's try not to add new uses of libbpf_get_error? Would this work:
> 
> return *link ? 0 : -errno;

ok

jirka

[PATCH bpf-next 5/7] libbpf: Add kprobe session attach type name to attach_type_name

[Jiri Olsa]

Adding kprobe session attach type name to attach_type_name,
so libbpf_bpf_attach_type_str returns proper string name for
BPF_TRACE_KPROBE_MULTI_SESSION attach type.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/lib/bpf/libbpf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index ca605240205f..9bf6cccb3443 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -132,6 +132,7 @@ static const char * const attach_type_name[] = {
 	[BPF_TRACE_UPROBE_MULTI]	= "trace_uprobe_multi",
 	[BPF_NETKIT_PRIMARY]		= "netkit_primary",
 	[BPF_NETKIT_PEER]		= "netkit_peer",
+	[BPF_TRACE_KPROBE_MULTI_SESSION]	= "trace_kprobe_multi_session",
 };
 
 static const char * const link_type_name[] = {
-- 
2.44.0

Re: [PATCH bpf-next 5/7] libbpf: Add kprobe session attach type name to attach_type_name

[Andrii Nakryiko]

On Mon, Apr 22, 2024 at 5:13 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding kprobe session attach type name to attach_type_name,
> so libbpf_bpf_attach_type_str returns proper string name for
> BPF_TRACE_KPROBE_MULTI_SESSION attach type.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/lib/bpf/libbpf.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index ca605240205f..9bf6cccb3443 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -132,6 +132,7 @@ static const char * const attach_type_name[] = {
>         [BPF_TRACE_UPROBE_MULTI]        = "trace_uprobe_multi",
>         [BPF_NETKIT_PRIMARY]            = "netkit_primary",
>         [BPF_NETKIT_PEER]               = "netkit_peer",
> +       [BPF_TRACE_KPROBE_MULTI_SESSION]        = "trace_kprobe_multi_session",

we got to shorten this to just BPF_TRACE_KPROBE_SESSION :)


>  };
>
>  static const char * const link_type_name[] = {
> --
> 2.44.0
>

Re: [PATCH bpf-next 5/7] libbpf: Add kprobe session attach type name to attach_type_name

[Jiri Olsa]

On Tue, Apr 23, 2024 at 05:27:06PM -0700, Andrii Nakryiko wrote:
> On Mon, Apr 22, 2024 at 5:13 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding kprobe session attach type name to attach_type_name,
> > so libbpf_bpf_attach_type_str returns proper string name for
> > BPF_TRACE_KPROBE_MULTI_SESSION attach type.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/lib/bpf/libbpf.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index ca605240205f..9bf6cccb3443 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -132,6 +132,7 @@ static const char * const attach_type_name[] = {
> >         [BPF_TRACE_UPROBE_MULTI]        = "trace_uprobe_multi",
> >         [BPF_NETKIT_PRIMARY]            = "netkit_primary",
> >         [BPF_NETKIT_PEER]               = "netkit_peer",
> > +       [BPF_TRACE_KPROBE_MULTI_SESSION]        = "trace_kprobe_multi_session",
> 
> we got to shorten this to just BPF_TRACE_KPROBE_SESSION :)

ook :)

jirka

> 
> 
> >  };
> >
> >  static const char * const link_type_name[] = {
> > --
> > 2.44.0
> >

[PATCH bpf-next 6/7] selftests/bpf: Add kprobe multi session test

[Jiri Olsa]

Adding kprobe multi session test and testing that the entry program
return value controls execution of the return probe program.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/testing/selftests/bpf/bpf_kfuncs.h      |   2 +
 .../bpf/prog_tests/kprobe_multi_test.c        |  49 +++++++++
 .../bpf/progs/kprobe_multi_session.c          | 100 ++++++++++++++++++
 3 files changed, 151 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_session.c

diff --git a/tools/testing/selftests/bpf/bpf_kfuncs.h b/tools/testing/selftests/bpf/bpf_kfuncs.h
index 14ebe7d9e1a3..180030b5d828 100644
--- a/tools/testing/selftests/bpf/bpf_kfuncs.h
+++ b/tools/testing/selftests/bpf/bpf_kfuncs.h
@@ -75,4 +75,6 @@ extern void bpf_key_put(struct bpf_key *key) __ksym;
 extern int bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr,
 				      struct bpf_dynptr *sig_ptr,
 				      struct bpf_key *trusted_keyring) __ksym;
+
+extern bool bpf_session_is_return(void) __ksym;
 #endif
diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
index 51628455b6f5..d1f116665551 100644
--- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
@@ -4,6 +4,7 @@
 #include "trace_helpers.h"
 #include "kprobe_multi_empty.skel.h"
 #include "kprobe_multi_override.skel.h"
+#include "kprobe_multi_session.skel.h"
 #include "bpf/libbpf_internal.h"
 #include "bpf/hashmap.h"
 
@@ -326,6 +327,52 @@ static void test_attach_api_fails(void)
 	kprobe_multi__destroy(skel);
 }
 
+static void test_session_skel_api(void)
+{
+	struct kprobe_multi_session *skel = NULL;
+	LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
+	LIBBPF_OPTS(bpf_test_run_opts, topts);
+	struct bpf_link *link = NULL;
+	int err, prog_fd;
+
+	skel = kprobe_multi_session__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "kprobe_multi_session__open_and_load"))
+		goto cleanup;
+
+	skel->bss->pid = getpid();
+
+	err =  kprobe_multi_session__attach(skel);
+	if (!ASSERT_OK(err, " kprobe_multi_session__attach"))
+		goto cleanup;
+
+	prog_fd = bpf_program__fd(skel->progs.trigger);
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	ASSERT_OK(err, "test_run");
+	ASSERT_EQ(topts.retval, 0, "test_run");
+
+	ASSERT_EQ(skel->bss->kprobe_test1_result, 1, "kprobe_test1_result");
+	ASSERT_EQ(skel->bss->kprobe_test2_result, 1, "kprobe_test2_result");
+	ASSERT_EQ(skel->bss->kprobe_test3_result, 1, "kprobe_test3_result");
+	ASSERT_EQ(skel->bss->kprobe_test4_result, 1, "kprobe_test4_result");
+	ASSERT_EQ(skel->bss->kprobe_test5_result, 1, "kprobe_test5_result");
+	ASSERT_EQ(skel->bss->kprobe_test6_result, 1, "kprobe_test6_result");
+	ASSERT_EQ(skel->bss->kprobe_test7_result, 1, "kprobe_test7_result");
+	ASSERT_EQ(skel->bss->kprobe_test8_result, 1, "kprobe_test8_result");
+
+	ASSERT_EQ(skel->bss->kretprobe_test1_result, 0, "kretprobe_test1_result");
+	ASSERT_EQ(skel->bss->kretprobe_test2_result, 1, "kretprobe_test2_result");
+	ASSERT_EQ(skel->bss->kretprobe_test3_result, 0, "kretprobe_test3_result");
+	ASSERT_EQ(skel->bss->kretprobe_test4_result, 1, "kretprobe_test4_result");
+	ASSERT_EQ(skel->bss->kretprobe_test5_result, 0, "kretprobe_test5_result");
+	ASSERT_EQ(skel->bss->kretprobe_test6_result, 1, "kretprobe_test6_result");
+	ASSERT_EQ(skel->bss->kretprobe_test7_result, 0, "kretprobe_test7_result");
+	ASSERT_EQ(skel->bss->kretprobe_test8_result, 1, "kretprobe_test8_result");
+
+cleanup:
+	bpf_link__destroy(link);
+	kprobe_multi_session__destroy(skel);
+}
+
 static size_t symbol_hash(long key, void *ctx __maybe_unused)
 {
 	return str_hash((const char *) key);
@@ -690,4 +737,6 @@ void test_kprobe_multi_test(void)
 		test_attach_api_fails();
 	if (test__start_subtest("attach_override"))
 		test_attach_override();
+	if (test__start_subtest("session"))
+		test_session_skel_api();
 }
diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi_session.c b/tools/testing/selftests/bpf/progs/kprobe_multi_session.c
new file mode 100644
index 000000000000..9fb87f7f69da
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/kprobe_multi_session.c
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <stdbool.h>
+#include "bpf_kfuncs.h"
+
+char _license[] SEC("license") = "GPL";
+
+extern const void bpf_fentry_test1 __ksym;
+extern const void bpf_fentry_test2 __ksym;
+extern const void bpf_fentry_test3 __ksym;
+extern const void bpf_fentry_test4 __ksym;
+extern const void bpf_fentry_test5 __ksym;
+extern const void bpf_fentry_test6 __ksym;
+extern const void bpf_fentry_test7 __ksym;
+extern const void bpf_fentry_test8 __ksym;
+
+int pid = 0;
+
+__u64 kprobe_test1_result = 0;
+__u64 kprobe_test2_result = 0;
+__u64 kprobe_test3_result = 0;
+__u64 kprobe_test4_result = 0;
+__u64 kprobe_test5_result = 0;
+__u64 kprobe_test6_result = 0;
+__u64 kprobe_test7_result = 0;
+__u64 kprobe_test8_result = 0;
+
+__u64 kretprobe_test1_result = 0;
+__u64 kretprobe_test2_result = 0;
+__u64 kretprobe_test3_result = 0;
+__u64 kretprobe_test4_result = 0;
+__u64 kretprobe_test5_result = 0;
+__u64 kretprobe_test6_result = 0;
+__u64 kretprobe_test7_result = 0;
+__u64 kretprobe_test8_result = 0;
+
+static int session_check(void *ctx, bool is_return)
+{
+	if (bpf_get_current_pid_tgid() >> 32 != pid)
+		return 1;
+
+	__u64 addr = bpf_get_func_ip(ctx);
+
+#define SET(__var, __addr) ({			\
+	if ((const void *) addr == __addr)	\
+		__var = 1;			\
+})
+
+	if (is_return) {
+		SET(kretprobe_test1_result, &bpf_fentry_test1);
+		SET(kretprobe_test2_result, &bpf_fentry_test2);
+		SET(kretprobe_test3_result, &bpf_fentry_test3);
+		SET(kretprobe_test4_result, &bpf_fentry_test4);
+		SET(kretprobe_test5_result, &bpf_fentry_test5);
+		SET(kretprobe_test6_result, &bpf_fentry_test6);
+		SET(kretprobe_test7_result, &bpf_fentry_test7);
+		SET(kretprobe_test8_result, &bpf_fentry_test8);
+	} else {
+		SET(kprobe_test1_result, &bpf_fentry_test1);
+		SET(kprobe_test2_result, &bpf_fentry_test2);
+		SET(kprobe_test3_result, &bpf_fentry_test3);
+		SET(kprobe_test4_result, &bpf_fentry_test4);
+		SET(kprobe_test5_result, &bpf_fentry_test5);
+		SET(kprobe_test6_result, &bpf_fentry_test6);
+		SET(kprobe_test7_result, &bpf_fentry_test7);
+		SET(kprobe_test8_result, &bpf_fentry_test8);
+	}
+
+#undef SET
+
+	/*
+	 * Force probes for function bpf_fentry_test[1357] not to
+	 * install and execute the return probe
+	 */
+	if (((const void *) addr == &bpf_fentry_test1) ||
+	    ((const void *) addr == &bpf_fentry_test3) ||
+	    ((const void *) addr == &bpf_fentry_test5) ||
+	    ((const void *) addr == &bpf_fentry_test7))
+		return 1;
+
+	return 0;
+}
+
+/*
+ * No tests in here, just to trigger 'bpf_fentry_test*'
+ * through tracing test_run
+ */
+SEC("fentry/bpf_modify_return_test")
+int BPF_PROG(trigger)
+{
+	return 0;
+}
+
+SEC("kprobe.session/bpf_fentry_test*")
+int test_kprobe(struct pt_regs *ctx)
+{
+	return session_check(ctx, bpf_session_is_return());
+}
-- 
2.44.0

Re: [PATCH bpf-next 6/7] selftests/bpf: Add kprobe multi session test

[Andrii Nakryiko]

On Mon, Apr 22, 2024 at 5:13 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding kprobe multi session test and testing that the entry program
> return value controls execution of the return probe program.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/testing/selftests/bpf/bpf_kfuncs.h      |   2 +
>  .../bpf/prog_tests/kprobe_multi_test.c        |  49 +++++++++
>  .../bpf/progs/kprobe_multi_session.c          | 100 ++++++++++++++++++
>  3 files changed, 151 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_session.c
>
> diff --git a/tools/testing/selftests/bpf/bpf_kfuncs.h b/tools/testing/selftests/bpf/bpf_kfuncs.h
> index 14ebe7d9e1a3..180030b5d828 100644
> --- a/tools/testing/selftests/bpf/bpf_kfuncs.h
> +++ b/tools/testing/selftests/bpf/bpf_kfuncs.h
> @@ -75,4 +75,6 @@ extern void bpf_key_put(struct bpf_key *key) __ksym;
>  extern int bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr,
>                                       struct bpf_dynptr *sig_ptr,
>                                       struct bpf_key *trusted_keyring) __ksym;
> +
> +extern bool bpf_session_is_return(void) __ksym;
>  #endif
> diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> index 51628455b6f5..d1f116665551 100644
> --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> @@ -4,6 +4,7 @@
>  #include "trace_helpers.h"
>  #include "kprobe_multi_empty.skel.h"
>  #include "kprobe_multi_override.skel.h"
> +#include "kprobe_multi_session.skel.h"
>  #include "bpf/libbpf_internal.h"
>  #include "bpf/hashmap.h"
>
> @@ -326,6 +327,52 @@ static void test_attach_api_fails(void)
>         kprobe_multi__destroy(skel);
>  }
>
> +static void test_session_skel_api(void)
> +{
> +       struct kprobe_multi_session *skel = NULL;
> +       LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
> +       LIBBPF_OPTS(bpf_test_run_opts, topts);
> +       struct bpf_link *link = NULL;
> +       int err, prog_fd;
> +
> +       skel = kprobe_multi_session__open_and_load();
> +       if (!ASSERT_OK_PTR(skel, "kprobe_multi_session__open_and_load"))
> +               goto cleanup;

return?

> +
> +       skel->bss->pid = getpid();
> +
> +       err =  kprobe_multi_session__attach(skel);

nit: extra space

> +       if (!ASSERT_OK(err, " kprobe_multi_session__attach"))
> +               goto cleanup;
> +
> +       prog_fd = bpf_program__fd(skel->progs.trigger);
> +       err = bpf_prog_test_run_opts(prog_fd, &topts);
> +       ASSERT_OK(err, "test_run");
> +       ASSERT_EQ(topts.retval, 0, "test_run");
> +
> +       ASSERT_EQ(skel->bss->kprobe_test1_result, 1, "kprobe_test1_result");
> +       ASSERT_EQ(skel->bss->kprobe_test2_result, 1, "kprobe_test2_result");
> +       ASSERT_EQ(skel->bss->kprobe_test3_result, 1, "kprobe_test3_result");
> +       ASSERT_EQ(skel->bss->kprobe_test4_result, 1, "kprobe_test4_result");
> +       ASSERT_EQ(skel->bss->kprobe_test5_result, 1, "kprobe_test5_result");
> +       ASSERT_EQ(skel->bss->kprobe_test6_result, 1, "kprobe_test6_result");
> +       ASSERT_EQ(skel->bss->kprobe_test7_result, 1, "kprobe_test7_result");
> +       ASSERT_EQ(skel->bss->kprobe_test8_result, 1, "kprobe_test8_result");
> +
> +       ASSERT_EQ(skel->bss->kretprobe_test1_result, 0, "kretprobe_test1_result");
> +       ASSERT_EQ(skel->bss->kretprobe_test2_result, 1, "kretprobe_test2_result");
> +       ASSERT_EQ(skel->bss->kretprobe_test3_result, 0, "kretprobe_test3_result");
> +       ASSERT_EQ(skel->bss->kretprobe_test4_result, 1, "kretprobe_test4_result");
> +       ASSERT_EQ(skel->bss->kretprobe_test5_result, 0, "kretprobe_test5_result");
> +       ASSERT_EQ(skel->bss->kretprobe_test6_result, 1, "kretprobe_test6_result");
> +       ASSERT_EQ(skel->bss->kretprobe_test7_result, 0, "kretprobe_test7_result");
> +       ASSERT_EQ(skel->bss->kretprobe_test8_result, 1, "kretprobe_test8_result");

see below, even if array of ksym ptrs idea doesn't work out, at least
results can be an array (which is cleaner to work with both on BPF and
user space sides)

> +
> +cleanup:
> +       bpf_link__destroy(link);
> +       kprobe_multi_session__destroy(skel);
> +}
> +
>  static size_t symbol_hash(long key, void *ctx __maybe_unused)
>  {
>         return str_hash((const char *) key);
> @@ -690,4 +737,6 @@ void test_kprobe_multi_test(void)
>                 test_attach_api_fails();
>         if (test__start_subtest("attach_override"))
>                 test_attach_override();
> +       if (test__start_subtest("session"))
> +               test_session_skel_api();
>  }
> diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi_session.c b/tools/testing/selftests/bpf/progs/kprobe_multi_session.c
> new file mode 100644
> index 000000000000..9fb87f7f69da
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/kprobe_multi_session.c
> @@ -0,0 +1,100 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +#include <stdbool.h>
> +#include "bpf_kfuncs.h"
> +
> +char _license[] SEC("license") = "GPL";
> +
> +extern const void bpf_fentry_test1 __ksym;
> +extern const void bpf_fentry_test2 __ksym;
> +extern const void bpf_fentry_test3 __ksym;
> +extern const void bpf_fentry_test4 __ksym;
> +extern const void bpf_fentry_test5 __ksym;
> +extern const void bpf_fentry_test6 __ksym;
> +extern const void bpf_fentry_test7 __ksym;
> +extern const void bpf_fentry_test8 __ksym;
> +
> +int pid = 0;
> +
> +__u64 kprobe_test1_result = 0;
> +__u64 kprobe_test2_result = 0;
> +__u64 kprobe_test3_result = 0;
> +__u64 kprobe_test4_result = 0;
> +__u64 kprobe_test5_result = 0;
> +__u64 kprobe_test6_result = 0;
> +__u64 kprobe_test7_result = 0;
> +__u64 kprobe_test8_result = 0;
> +
> +__u64 kretprobe_test1_result = 0;
> +__u64 kretprobe_test2_result = 0;
> +__u64 kretprobe_test3_result = 0;
> +__u64 kretprobe_test4_result = 0;
> +__u64 kretprobe_test5_result = 0;
> +__u64 kretprobe_test6_result = 0;
> +__u64 kretprobe_test7_result = 0;
> +__u64 kretprobe_test8_result = 0;
> +
> +static int session_check(void *ctx, bool is_return)
> +{
> +       if (bpf_get_current_pid_tgid() >> 32 != pid)
> +               return 1;
> +
> +       __u64 addr = bpf_get_func_ip(ctx);
> +
> +#define SET(__var, __addr) ({                  \
> +       if ((const void *) addr == __addr)      \
> +               __var = 1;                      \
> +})
> +
> +       if (is_return) {
> +               SET(kretprobe_test1_result, &bpf_fentry_test1);
> +               SET(kretprobe_test2_result, &bpf_fentry_test2);
> +               SET(kretprobe_test3_result, &bpf_fentry_test3);
> +               SET(kretprobe_test4_result, &bpf_fentry_test4);
> +               SET(kretprobe_test5_result, &bpf_fentry_test5);
> +               SET(kretprobe_test6_result, &bpf_fentry_test6);
> +               SET(kretprobe_test7_result, &bpf_fentry_test7);
> +               SET(kretprobe_test8_result, &bpf_fentry_test8);
> +       } else {
> +               SET(kprobe_test1_result, &bpf_fentry_test1);
> +               SET(kprobe_test2_result, &bpf_fentry_test2);
> +               SET(kprobe_test3_result, &bpf_fentry_test3);
> +               SET(kprobe_test4_result, &bpf_fentry_test4);
> +               SET(kprobe_test5_result, &bpf_fentry_test5);
> +               SET(kprobe_test6_result, &bpf_fentry_test6);
> +               SET(kprobe_test7_result, &bpf_fentry_test7);
> +               SET(kprobe_test8_result, &bpf_fentry_test8);
> +       }
> +
> +#undef SET

curious, have you tried implementing this through a proper for loop? I
wonder if something like

void *kfuncs[] = { &bpf_fentry_test1, ..., &bpf_fentry_test8 };

and then generic loop over this array would work. Can you please try?


> +
> +       /*
> +        * Force probes for function bpf_fentry_test[1357] not to
> +        * install and execute the return probe
> +        */
> +       if (((const void *) addr == &bpf_fentry_test1) ||
> +           ((const void *) addr == &bpf_fentry_test3) ||
> +           ((const void *) addr == &bpf_fentry_test5) ||
> +           ((const void *) addr == &bpf_fentry_test7))
> +               return 1;
> +
> +       return 0;
> +}
> +
> +/*
> + * No tests in here, just to trigger 'bpf_fentry_test*'
> + * through tracing test_run
> + */
> +SEC("fentry/bpf_modify_return_test")
> +int BPF_PROG(trigger)
> +{
> +       return 0;
> +}
> +
> +SEC("kprobe.session/bpf_fentry_test*")
> +int test_kprobe(struct pt_regs *ctx)
> +{
> +       return session_check(ctx, bpf_session_is_return());
> +}
> --
> 2.44.0
>

Re: [PATCH bpf-next 6/7] selftests/bpf: Add kprobe multi session test

[Jiri Olsa]

On Tue, Apr 23, 2024 at 05:27:14PM -0700, Andrii Nakryiko wrote:
> On Mon, Apr 22, 2024 at 5:13 AM Jiri Olsa <jolsa@kernel.org> wrote:

SNIP

> > diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> > index 51628455b6f5..d1f116665551 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> > @@ -4,6 +4,7 @@
> >  #include "trace_helpers.h"
> >  #include "kprobe_multi_empty.skel.h"
> >  #include "kprobe_multi_override.skel.h"
> > +#include "kprobe_multi_session.skel.h"
> >  #include "bpf/libbpf_internal.h"
> >  #include "bpf/hashmap.h"
> >
> > @@ -326,6 +327,52 @@ static void test_attach_api_fails(void)
> >         kprobe_multi__destroy(skel);
> >  }
> >
> > +static void test_session_skel_api(void)
> > +{
> > +       struct kprobe_multi_session *skel = NULL;
> > +       LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
> > +       LIBBPF_OPTS(bpf_test_run_opts, topts);
> > +       struct bpf_link *link = NULL;
> > +       int err, prog_fd;
> > +
> > +       skel = kprobe_multi_session__open_and_load();
> > +       if (!ASSERT_OK_PTR(skel, "kprobe_multi_session__open_and_load"))
> > +               goto cleanup;
> 
> return?

ok

> 
> > +
> > +       skel->bss->pid = getpid();
> > +
> > +       err =  kprobe_multi_session__attach(skel);
> 
> nit: extra space
> 
> > +       if (!ASSERT_OK(err, " kprobe_multi_session__attach"))
> > +               goto cleanup;
> > +
> > +       prog_fd = bpf_program__fd(skel->progs.trigger);
> > +       err = bpf_prog_test_run_opts(prog_fd, &topts);
> > +       ASSERT_OK(err, "test_run");
> > +       ASSERT_EQ(topts.retval, 0, "test_run");
> > +
> > +       ASSERT_EQ(skel->bss->kprobe_test1_result, 1, "kprobe_test1_result");
> > +       ASSERT_EQ(skel->bss->kprobe_test2_result, 1, "kprobe_test2_result");
> > +       ASSERT_EQ(skel->bss->kprobe_test3_result, 1, "kprobe_test3_result");
> > +       ASSERT_EQ(skel->bss->kprobe_test4_result, 1, "kprobe_test4_result");
> > +       ASSERT_EQ(skel->bss->kprobe_test5_result, 1, "kprobe_test5_result");
> > +       ASSERT_EQ(skel->bss->kprobe_test6_result, 1, "kprobe_test6_result");
> > +       ASSERT_EQ(skel->bss->kprobe_test7_result, 1, "kprobe_test7_result");
> > +       ASSERT_EQ(skel->bss->kprobe_test8_result, 1, "kprobe_test8_result");
> > +
> > +       ASSERT_EQ(skel->bss->kretprobe_test1_result, 0, "kretprobe_test1_result");
> > +       ASSERT_EQ(skel->bss->kretprobe_test2_result, 1, "kretprobe_test2_result");
> > +       ASSERT_EQ(skel->bss->kretprobe_test3_result, 0, "kretprobe_test3_result");
> > +       ASSERT_EQ(skel->bss->kretprobe_test4_result, 1, "kretprobe_test4_result");
> > +       ASSERT_EQ(skel->bss->kretprobe_test5_result, 0, "kretprobe_test5_result");
> > +       ASSERT_EQ(skel->bss->kretprobe_test6_result, 1, "kretprobe_test6_result");
> > +       ASSERT_EQ(skel->bss->kretprobe_test7_result, 0, "kretprobe_test7_result");
> > +       ASSERT_EQ(skel->bss->kretprobe_test8_result, 1, "kretprobe_test8_result");
> 
> see below, even if array of ksym ptrs idea doesn't work out, at least
> results can be an array (which is cleaner to work with both on BPF and
> user space sides)

I recall in past we used to do that and we switched to specific values
to be more explicit I guess.. but it might make sense in here, will try it 

SNIP

> > +static int session_check(void *ctx, bool is_return)
> > +{
> > +       if (bpf_get_current_pid_tgid() >> 32 != pid)
> > +               return 1;
> > +
> > +       __u64 addr = bpf_get_func_ip(ctx);
> > +
> > +#define SET(__var, __addr) ({                  \
> > +       if ((const void *) addr == __addr)      \
> > +               __var = 1;                      \
> > +})
> > +
> > +       if (is_return) {
> > +               SET(kretprobe_test1_result, &bpf_fentry_test1);
> > +               SET(kretprobe_test2_result, &bpf_fentry_test2);
> > +               SET(kretprobe_test3_result, &bpf_fentry_test3);
> > +               SET(kretprobe_test4_result, &bpf_fentry_test4);
> > +               SET(kretprobe_test5_result, &bpf_fentry_test5);
> > +               SET(kretprobe_test6_result, &bpf_fentry_test6);
> > +               SET(kretprobe_test7_result, &bpf_fentry_test7);
> > +               SET(kretprobe_test8_result, &bpf_fentry_test8);
> > +       } else {
> > +               SET(kprobe_test1_result, &bpf_fentry_test1);
> > +               SET(kprobe_test2_result, &bpf_fentry_test2);
> > +               SET(kprobe_test3_result, &bpf_fentry_test3);
> > +               SET(kprobe_test4_result, &bpf_fentry_test4);
> > +               SET(kprobe_test5_result, &bpf_fentry_test5);
> > +               SET(kprobe_test6_result, &bpf_fentry_test6);
> > +               SET(kprobe_test7_result, &bpf_fentry_test7);
> > +               SET(kprobe_test8_result, &bpf_fentry_test8);
> > +       }
> > +
> > +#undef SET
> 
> curious, have you tried implementing this through a proper for loop? I
> wonder if something like
> 
> void *kfuncs[] = { &bpf_fentry_test1, ..., &bpf_fentry_test8 };
> 
> and then generic loop over this array would work. Can you please try?

yep, will try, let's see if it gets nicer

jirka

Re: [PATCH bpf-next 6/7] selftests/bpf: Add kprobe multi session test

[Jiri Olsa]

On Wed, Apr 24, 2024 at 01:44:50PM +0200, Jiri Olsa wrote:

SNIP

> > see below, even if array of ksym ptrs idea doesn't work out, at least
> > results can be an array (which is cleaner to work with both on BPF and
> > user space sides)
> 
> I recall in past we used to do that and we switched to specific values
> to be more explicit I guess.. but it might make sense in here, will try it 
> 
> SNIP
> 
> > > +static int session_check(void *ctx, bool is_return)
> > > +{
> > > +       if (bpf_get_current_pid_tgid() >> 32 != pid)
> > > +               return 1;
> > > +
> > > +       __u64 addr = bpf_get_func_ip(ctx);
> > > +
> > > +#define SET(__var, __addr) ({                  \
> > > +       if ((const void *) addr == __addr)      \
> > > +               __var = 1;                      \
> > > +})
> > > +
> > > +       if (is_return) {
> > > +               SET(kretprobe_test1_result, &bpf_fentry_test1);
> > > +               SET(kretprobe_test2_result, &bpf_fentry_test2);
> > > +               SET(kretprobe_test3_result, &bpf_fentry_test3);
> > > +               SET(kretprobe_test4_result, &bpf_fentry_test4);
> > > +               SET(kretprobe_test5_result, &bpf_fentry_test5);
> > > +               SET(kretprobe_test6_result, &bpf_fentry_test6);
> > > +               SET(kretprobe_test7_result, &bpf_fentry_test7);
> > > +               SET(kretprobe_test8_result, &bpf_fentry_test8);
> > > +       } else {
> > > +               SET(kprobe_test1_result, &bpf_fentry_test1);
> > > +               SET(kprobe_test2_result, &bpf_fentry_test2);
> > > +               SET(kprobe_test3_result, &bpf_fentry_test3);
> > > +               SET(kprobe_test4_result, &bpf_fentry_test4);
> > > +               SET(kprobe_test5_result, &bpf_fentry_test5);
> > > +               SET(kprobe_test6_result, &bpf_fentry_test6);
> > > +               SET(kprobe_test7_result, &bpf_fentry_test7);
> > > +               SET(kprobe_test8_result, &bpf_fentry_test8);
> > > +       }
> > > +
> > > +#undef SET
> > 
> > curious, have you tried implementing this through a proper for loop? I
> > wonder if something like
> > 
> > void *kfuncs[] = { &bpf_fentry_test1, ..., &bpf_fentry_test8 };
> > 
> > and then generic loop over this array would work. Can you please try?
> 
> yep, will try, let's see if it gets nicer

ok it looks better, I'll send new version

thanks,
jirka


---
diff --git a/tools/testing/selftests/bpf/bpf_kfuncs.h b/tools/testing/selftests/bpf/bpf_kfuncs.h
index 14ebe7d9e1a3..180030b5d828 100644
--- a/tools/testing/selftests/bpf/bpf_kfuncs.h
+++ b/tools/testing/selftests/bpf/bpf_kfuncs.h
@@ -75,4 +75,6 @@ extern void bpf_key_put(struct bpf_key *key) __ksym;
 extern int bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr,
 				      struct bpf_dynptr *sig_ptr,
 				      struct bpf_key *trusted_keyring) __ksym;
+
+extern bool bpf_session_is_return(void) __ksym;
 #endif
diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
index 51628455b6f5..f6eac16a9339 100644
--- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
@@ -4,6 +4,7 @@
 #include "trace_helpers.h"
 #include "kprobe_multi_empty.skel.h"
 #include "kprobe_multi_override.skel.h"
+#include "kprobe_multi_session.skel.h"
 #include "bpf/libbpf_internal.h"
 #include "bpf/hashmap.h"
 
@@ -326,6 +327,46 @@ static void test_attach_api_fails(void)
 	kprobe_multi__destroy(skel);
 }
 
+static void test_session_skel_api(void)
+{
+	struct kprobe_multi_session *skel = NULL;
+	LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
+	LIBBPF_OPTS(bpf_test_run_opts, topts);
+	struct bpf_link *link = NULL;
+	int err, prog_fd;
+
+	skel = kprobe_multi_session__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "kprobe_multi_session__open_and_load"))
+		return;
+
+	skel->bss->pid = getpid();
+
+	err = kprobe_multi_session__attach(skel);
+	if (!ASSERT_OK(err, " kprobe_multi_session__attach"))
+		goto cleanup;
+
+	prog_fd = bpf_program__fd(skel->progs.trigger);
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	ASSERT_OK(err, "test_run");
+	ASSERT_EQ(topts.retval, 0, "test_run");
+
+	/* bpf_fentry_test1-4 trigger return probe, result is 2 */
+	ASSERT_EQ(skel->bss->kprobe_session_result[0], 2, "kprobe_session_result[0]");
+	ASSERT_EQ(skel->bss->kprobe_session_result[1], 2, "kprobe_session_result[1]");
+	ASSERT_EQ(skel->bss->kprobe_session_result[2], 2, "kprobe_session_result[2]");
+	ASSERT_EQ(skel->bss->kprobe_session_result[3], 2, "kprobe_session_result[3]");
+
+	/* bpf_fentry_test5-8 trigger only entry probe, result is 1 */
+	ASSERT_EQ(skel->bss->kprobe_session_result[4], 1, "kprobe_session_result[4]");
+	ASSERT_EQ(skel->bss->kprobe_session_result[5], 1, "kprobe_session_result[5]");
+	ASSERT_EQ(skel->bss->kprobe_session_result[6], 1, "kprobe_session_result[6]");
+	ASSERT_EQ(skel->bss->kprobe_session_result[7], 1, "kprobe_session_result[7]");
+
+cleanup:
+	bpf_link__destroy(link);
+	kprobe_multi_session__destroy(skel);
+}
+
 static size_t symbol_hash(long key, void *ctx __maybe_unused)
 {
 	return str_hash((const char *) key);
@@ -690,4 +731,6 @@ void test_kprobe_multi_test(void)
 		test_attach_api_fails();
 	if (test__start_subtest("attach_override"))
 		test_attach_override();
+	if (test__start_subtest("session"))
+		test_session_skel_api();
 }
diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi_session.c b/tools/testing/selftests/bpf/progs/kprobe_multi_session.c
new file mode 100644
index 000000000000..3f4137100482
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/kprobe_multi_session.c
@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <stdbool.h>
+#include "bpf_kfuncs.h"
+
+char _license[] SEC("license") = "GPL";
+
+extern const void bpf_fentry_test1 __ksym;
+extern const void bpf_fentry_test2 __ksym;
+extern const void bpf_fentry_test3 __ksym;
+extern const void bpf_fentry_test4 __ksym;
+extern const void bpf_fentry_test5 __ksym;
+extern const void bpf_fentry_test6 __ksym;
+extern const void bpf_fentry_test7 __ksym;
+extern const void bpf_fentry_test8 __ksym;
+
+int pid = 0;
+
+__u64 kprobe_session_result[8];
+
+static const void *kfuncs[8] = {
+	&bpf_fentry_test1,
+	&bpf_fentry_test2,
+	&bpf_fentry_test3,
+	&bpf_fentry_test4,
+	&bpf_fentry_test5,
+	&bpf_fentry_test6,
+	&bpf_fentry_test7,
+	&bpf_fentry_test8,
+};
+
+static int session_check(void *ctx, bool is_return)
+{
+	unsigned int i;
+	__u64 addr;
+
+	if (bpf_get_current_pid_tgid() >> 32 != pid)
+		return 1;
+
+	addr = bpf_get_func_ip(ctx);
+
+	for (i = 0; i < 8; i++) {
+		if (kfuncs[i] == (void *) addr) {
+			kprobe_session_result[i]++;
+			break;
+		}
+	}
+
+	/*
+	 * Force probes for function bpf_fentry_test[5-8] not to
+	 * install and execute the return probe
+	 */
+	if (((const void *) addr == &bpf_fentry_test5) ||
+	    ((const void *) addr == &bpf_fentry_test6) ||
+	    ((const void *) addr == &bpf_fentry_test7) ||
+	    ((const void *) addr == &bpf_fentry_test8))
+		return 1;
+
+	return 0;
+}
+
+/*
+ * No tests in here, just to trigger 'bpf_fentry_test*'
+ * through tracing test_run
+ */
+SEC("fentry/bpf_modify_return_test")
+int BPF_PROG(trigger)
+{
+	return 0;
+}
+
+SEC("kprobe.session/bpf_fentry_test*")
+int test_kprobe(struct pt_regs *ctx)
+{
+	return session_check(ctx, bpf_session_is_return());
+}

[PATCH bpf-next 7/7] selftests/bpf: Add kprobe multi wrapper cookie test

[Jiri Olsa]

Adding kprobe multi session test that verifies the cookie
value get properly propagated from entry to return program.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/testing/selftests/bpf/bpf_kfuncs.h      |  1 +
 .../bpf/prog_tests/kprobe_multi_test.c        | 35 ++++++++++++
 .../bpf/progs/kprobe_multi_session_cookie.c   | 56 +++++++++++++++++++
 3 files changed, 92 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_session_cookie.c

diff --git a/tools/testing/selftests/bpf/bpf_kfuncs.h b/tools/testing/selftests/bpf/bpf_kfuncs.h
index 180030b5d828..0281921cd654 100644
--- a/tools/testing/selftests/bpf/bpf_kfuncs.h
+++ b/tools/testing/selftests/bpf/bpf_kfuncs.h
@@ -77,4 +77,5 @@ extern int bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr,
 				      struct bpf_key *trusted_keyring) __ksym;
 
 extern bool bpf_session_is_return(void) __ksym;
+extern __u64 *bpf_session_cookie(void) __ksym;
 #endif
diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
index d1f116665551..2896467ca3cd 100644
--- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
@@ -5,6 +5,7 @@
 #include "kprobe_multi_empty.skel.h"
 #include "kprobe_multi_override.skel.h"
 #include "kprobe_multi_session.skel.h"
+#include "kprobe_multi_session_cookie.skel.h"
 #include "bpf/libbpf_internal.h"
 #include "bpf/hashmap.h"
 
@@ -373,6 +374,38 @@ static void test_session_skel_api(void)
 	kprobe_multi_session__destroy(skel);
 }
 
+static void test_session_cookie_skel_api(void)
+{
+	struct kprobe_multi_session_cookie *skel = NULL;
+	LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
+	LIBBPF_OPTS(bpf_test_run_opts, topts);
+	struct bpf_link *link = NULL;
+	int err, prog_fd;
+
+	skel = kprobe_multi_session_cookie__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "fentry_raw_skel_load"))
+		goto cleanup;
+
+	skel->bss->pid = getpid();
+
+	err = kprobe_multi_session_cookie__attach(skel);
+	if (!ASSERT_OK(err, " kprobe_multi_wrapper__attach"))
+		goto cleanup;
+
+	prog_fd = bpf_program__fd(skel->progs.trigger);
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	ASSERT_OK(err, "test_run");
+	ASSERT_EQ(topts.retval, 0, "test_run");
+
+	ASSERT_EQ(skel->bss->test_kprobe_1_result, 1, "test_kprobe_1_result");
+	ASSERT_EQ(skel->bss->test_kprobe_2_result, 2, "test_kprobe_2_result");
+	ASSERT_EQ(skel->bss->test_kprobe_3_result, 3, "test_kprobe_3_result");
+
+cleanup:
+	bpf_link__destroy(link);
+	kprobe_multi_session_cookie__destroy(skel);
+}
+
 static size_t symbol_hash(long key, void *ctx __maybe_unused)
 {
 	return str_hash((const char *) key);
@@ -739,4 +772,6 @@ void test_kprobe_multi_test(void)
 		test_attach_override();
 	if (test__start_subtest("session"))
 		test_session_skel_api();
+	if (test__start_subtest("session_cookie"))
+		test_session_cookie_skel_api();
 }
diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi_session_cookie.c b/tools/testing/selftests/bpf/progs/kprobe_multi_session_cookie.c
new file mode 100644
index 000000000000..b5c04b7b180c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/kprobe_multi_session_cookie.c
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <stdbool.h>
+#include "bpf_kfuncs.h"
+
+char _license[] SEC("license") = "GPL";
+
+int pid = 0;
+
+__u64 test_kprobe_1_result = 0;
+__u64 test_kprobe_2_result = 0;
+__u64 test_kprobe_3_result = 0;
+
+/*
+ * No tests in here, just to trigger 'bpf_fentry_test*'
+ * through tracing test_run
+ */
+SEC("fentry/bpf_modify_return_test")
+int BPF_PROG(trigger)
+{
+	return 0;
+}
+
+static int check_cookie(__u64 val, __u64 *result)
+{
+	if (bpf_get_current_pid_tgid() >> 32 != pid)
+		return 1;
+
+	__u64 *cookie = bpf_session_cookie();
+
+	if (bpf_session_is_return())
+		*result = *cookie == val ? val : 0;
+	else
+		*cookie = val;
+	return 0;
+}
+
+SEC("kprobe.session/bpf_fentry_test1")
+int test_kprobe_1(struct pt_regs *ctx)
+{
+	return check_cookie(1, &test_kprobe_1_result);
+}
+
+SEC("kprobe.session/bpf_fentry_test1")
+int test_kprobe_2(struct pt_regs *ctx)
+{
+	return check_cookie(2, &test_kprobe_2_result);
+}
+
+SEC("kprobe.session/bpf_fentry_test1")
+int test_kprobe_3(struct pt_regs *ctx)
+{
+	return check_cookie(3, &test_kprobe_3_result);
+}
-- 
2.44.0

Re: [PATCH bpf-next 7/7] selftests/bpf: Add kprobe multi wrapper cookie test

[Andrii Nakryiko]

On Mon, Apr 22, 2024 at 5:14 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding kprobe multi session test that verifies the cookie
> value get properly propagated from entry to return program.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

Forgot to update subject (still using "wrapper" naming)

overall LGTM, see nits

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  tools/testing/selftests/bpf/bpf_kfuncs.h      |  1 +
>  .../bpf/prog_tests/kprobe_multi_test.c        | 35 ++++++++++++
>  .../bpf/progs/kprobe_multi_session_cookie.c   | 56 +++++++++++++++++++
>  3 files changed, 92 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_session_cookie.c
>
> diff --git a/tools/testing/selftests/bpf/bpf_kfuncs.h b/tools/testing/selftests/bpf/bpf_kfuncs.h
> index 180030b5d828..0281921cd654 100644
> --- a/tools/testing/selftests/bpf/bpf_kfuncs.h
> +++ b/tools/testing/selftests/bpf/bpf_kfuncs.h
> @@ -77,4 +77,5 @@ extern int bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr,
>                                       struct bpf_key *trusted_keyring) __ksym;
>
>  extern bool bpf_session_is_return(void) __ksym;
> +extern __u64 *bpf_session_cookie(void) __ksym;

btw, should we use `long *` as return type to avoid relying on having
__u64 alias be available? Long is always an 8-byte value in the BPF
world, it should be fine.

>  #endif
> diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> index d1f116665551..2896467ca3cd 100644
> --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> @@ -5,6 +5,7 @@
>  #include "kprobe_multi_empty.skel.h"
>  #include "kprobe_multi_override.skel.h"
>  #include "kprobe_multi_session.skel.h"
> +#include "kprobe_multi_session_cookie.skel.h"
>  #include "bpf/libbpf_internal.h"
>  #include "bpf/hashmap.h"
>
> @@ -373,6 +374,38 @@ static void test_session_skel_api(void)
>         kprobe_multi_session__destroy(skel);
>  }
>
> +static void test_session_cookie_skel_api(void)
> +{
> +       struct kprobe_multi_session_cookie *skel = NULL;
> +       LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
> +       LIBBPF_OPTS(bpf_test_run_opts, topts);
> +       struct bpf_link *link = NULL;
> +       int err, prog_fd;
> +
> +       skel = kprobe_multi_session_cookie__open_and_load();
> +       if (!ASSERT_OK_PTR(skel, "fentry_raw_skel_load"))
> +               goto cleanup;
> +
> +       skel->bss->pid = getpid();
> +
> +       err = kprobe_multi_session_cookie__attach(skel);
> +       if (!ASSERT_OK(err, " kprobe_multi_wrapper__attach"))
> +               goto cleanup;
> +
> +       prog_fd = bpf_program__fd(skel->progs.trigger);
> +       err = bpf_prog_test_run_opts(prog_fd, &topts);
> +       ASSERT_OK(err, "test_run");
> +       ASSERT_EQ(topts.retval, 0, "test_run");
> +
> +       ASSERT_EQ(skel->bss->test_kprobe_1_result, 1, "test_kprobe_1_result");
> +       ASSERT_EQ(skel->bss->test_kprobe_2_result, 2, "test_kprobe_2_result");
> +       ASSERT_EQ(skel->bss->test_kprobe_3_result, 3, "test_kprobe_3_result");
> +
> +cleanup:
> +       bpf_link__destroy(link);
> +       kprobe_multi_session_cookie__destroy(skel);
> +}
> +
>  static size_t symbol_hash(long key, void *ctx __maybe_unused)
>  {
>         return str_hash((const char *) key);
> @@ -739,4 +772,6 @@ void test_kprobe_multi_test(void)
>                 test_attach_override();
>         if (test__start_subtest("session"))
>                 test_session_skel_api();
> +       if (test__start_subtest("session_cookie"))
> +               test_session_cookie_skel_api();
>  }
> diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi_session_cookie.c b/tools/testing/selftests/bpf/progs/kprobe_multi_session_cookie.c
> new file mode 100644
> index 000000000000..b5c04b7b180c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/kprobe_multi_session_cookie.c
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +#include <stdbool.h>
> +#include "bpf_kfuncs.h"
> +
> +char _license[] SEC("license") = "GPL";
> +
> +int pid = 0;
> +
> +__u64 test_kprobe_1_result = 0;
> +__u64 test_kprobe_2_result = 0;
> +__u64 test_kprobe_3_result = 0;
> +
> +/*
> + * No tests in here, just to trigger 'bpf_fentry_test*'
> + * through tracing test_run
> + */
> +SEC("fentry/bpf_modify_return_test")
> +int BPF_PROG(trigger)
> +{
> +       return 0;
> +}
> +
> +static int check_cookie(__u64 val, __u64 *result)
> +{
> +       if (bpf_get_current_pid_tgid() >> 32 != pid)
> +               return 1;
> +
> +       __u64 *cookie = bpf_session_cookie();

we don't enforce this, but let's stick to C89 variable declaration
style (or rather positioning in this case)?


> +
> +       if (bpf_session_is_return())
> +               *result = *cookie == val ? val : 0;
> +       else
> +               *cookie = val;
> +       return 0;
> +}
> +
> +SEC("kprobe.session/bpf_fentry_test1")
> +int test_kprobe_1(struct pt_regs *ctx)
> +{
> +       return check_cookie(1, &test_kprobe_1_result);
> +}
> +
> +SEC("kprobe.session/bpf_fentry_test1")
> +int test_kprobe_2(struct pt_regs *ctx)
> +{
> +       return check_cookie(2, &test_kprobe_2_result);
> +}
> +
> +SEC("kprobe.session/bpf_fentry_test1")
> +int test_kprobe_3(struct pt_regs *ctx)
> +{
> +       return check_cookie(3, &test_kprobe_3_result);
> +}
> --
> 2.44.0
>

Re: [PATCH bpf-next 7/7] selftests/bpf: Add kprobe multi wrapper cookie test

[Jiri Olsa]

On Tue, Apr 23, 2024 at 05:27:22PM -0700, Andrii Nakryiko wrote:
> On Mon, Apr 22, 2024 at 5:14 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding kprobe multi session test that verifies the cookie
> > value get properly propagated from entry to return program.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> 
> Forgot to update subject (still using "wrapper" naming)

ugh, thnx

> 
> overall LGTM, see nits
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> >  tools/testing/selftests/bpf/bpf_kfuncs.h      |  1 +
> >  .../bpf/prog_tests/kprobe_multi_test.c        | 35 ++++++++++++
> >  .../bpf/progs/kprobe_multi_session_cookie.c   | 56 +++++++++++++++++++
> >  3 files changed, 92 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_session_cookie.c
> >
> > diff --git a/tools/testing/selftests/bpf/bpf_kfuncs.h b/tools/testing/selftests/bpf/bpf_kfuncs.h
> > index 180030b5d828..0281921cd654 100644
> > --- a/tools/testing/selftests/bpf/bpf_kfuncs.h
> > +++ b/tools/testing/selftests/bpf/bpf_kfuncs.h
> > @@ -77,4 +77,5 @@ extern int bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr,
> >                                       struct bpf_key *trusted_keyring) __ksym;
> >
> >  extern bool bpf_session_is_return(void) __ksym;
> > +extern __u64 *bpf_session_cookie(void) __ksym;
> 
> btw, should we use `long *` as return type to avoid relying on having
> __u64 alias be available? Long is always an 8-byte value in the BPF
> world, it should be fine.

ok, there are some __u64 in kfuncs already, but let's stay on safe side

SNIP

> > +/*
> > + * No tests in here, just to trigger 'bpf_fentry_test*'
> > + * through tracing test_run
> > + */
> > +SEC("fentry/bpf_modify_return_test")
> > +int BPF_PROG(trigger)
> > +{
> > +       return 0;
> > +}
> > +
> > +static int check_cookie(__u64 val, __u64 *result)
> > +{
> > +       if (bpf_get_current_pid_tgid() >> 32 != pid)
> > +               return 1;
> > +
> > +       __u64 *cookie = bpf_session_cookie();
> 
> we don't enforce this, but let's stick to C89 variable declaration
> style (or rather positioning in this case)?

ok, will change that

jirka

Re: [PATCH bpf-next 0/7] bpf: Introduce kprobe_multi session attach

[Andrii Nakryiko]

On Mon, Apr 22, 2024 at 5:12 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> hi,
> adding support to attach kprobe program through kprobe_multi link
> in a session mode, which means:
>   - program is attached to both function entry and return
>   - entry program can decided if the return program gets executed
>   - entry program can share u64 cookie value with return program
>
> The initial RFC for this was posted in [0] and later discussed more
> and which ended up with the session idea [1]
>
> Having entry together with return probe for given function is common
> use case for tetragon, bpftrace and most likely for others.
>
> At the moment if we want both entry and return probe to execute bpf
> program we need to create two (entry and return probe) links. The link
> for return probe creates extra entry probe to setup the return probe.
> The extra entry probe execution could be omitted if we had a way to
> use just single link for both entry and exit probe.
>
> In addition the possibility to control the return program execution
> and sharing data within entry and return probe allows for other use
> cases.
>
> Changes from last RFC version [1]:
>   - changed wrapper name to session
>   - changed flag to adding new attach type for session:
>       BPF_TRACE_KPROBE_MULTI_SESSION
>     it's more convenient wrt filtering on kfuncs setup and seems
>     to make more sense alltogether
>   - renamed bpf_kprobe_multi_is_return to bpf_session_is_return
>   - added bpf_session_cookie kfunc, which actually already works
>     on current fprobe implementation (not just fprobe-on-fgraph)
>     and it provides the shared data between entry/return probes [Andrii]
>
>     we could actually make the cookie size configurable.. thoughts?
>     (it's 8 bytes atm)
>

Attach cookie is fixed at 8 bytes and that works pretty well. I think
beyond 8 bytes there is no clearly "right" size. A common case would
be to capture arguments in kprobe to handle in kretprobe, and there
you might need at least 40+ bytes, which seems wasteful. So I want to
say that it's probably good to hard-code it to just 8 bytes (enough to
store timestamp and you can even fit in some flags if you reduce
timestamp precision from nanoseconds to microseconds), or use it as an
index into array or some other data structure.

let's keep it simple?



>   - better attach setup conditions changes [Andrii]
>   - I'm not including uprobes change atm, because it needs extra
>     uprobe change so I'll post it separately
>
> Also available at:
>   https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
>   bpf/session_data
>
> thanks,
> jirka
>
>
> [0] https://lore.kernel.org/bpf/20240207153550.856536-1-jolsa@kernel.org/
> [1] https://lore.kernel.org/bpf/20240228090242.4040210-1-jolsa@kernel.org/
> ---
> Jiri Olsa (7):
>       bpf: Add support for kprobe multi session attach
>       bpf: Add support for kprobe multi session context
>       bpf: Add support for kprobe multi session cookie
>       libbpf: Add support for kprobe multi session attach
>       libbpf: Add kprobe session attach type name to attach_type_name
>       selftests/bpf: Add kprobe multi session test
>       selftests/bpf: Add kprobe multi wrapper cookie test
>
>  include/uapi/linux/bpf.h                                        |   1 +
>  kernel/bpf/btf.c                                                |   3 +++
>  kernel/bpf/syscall.c                                            |   7 +++++-
>  kernel/bpf/verifier.c                                           |   7 ++++++
>  kernel/trace/bpf_trace.c                                        | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------
>  tools/include/uapi/linux/bpf.h                                  |   1 +
>  tools/lib/bpf/bpf.c                                             |   1 +
>  tools/lib/bpf/libbpf.c                                          |  41 ++++++++++++++++++++++++++++++++--
>  tools/lib/bpf/libbpf.h                                          |   4 +++-
>  tools/testing/selftests/bpf/bpf_kfuncs.h                        |   3 +++
>  tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c      |  84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tools/testing/selftests/bpf/progs/kprobe_multi_session.c        | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tools/testing/selftests/bpf/progs/kprobe_multi_session_cookie.c |  56 ++++++++++++++++++++++++++++++++++++++++++++++
>  13 files changed, 396 insertions(+), 18 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_session.c
>  create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_session_cookie.c

Re: [PATCH bpf-next 0/7] bpf: Introduce kprobe_multi session attach

[John Fastabend]

Andrii Nakryiko wrote:
> On Mon, Apr 22, 2024 at 5:12 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > hi,
> > adding support to attach kprobe program through kprobe_multi link
> > in a session mode, which means:
> >   - program is attached to both function entry and return
> >   - entry program can decided if the return program gets executed
> >   - entry program can share u64 cookie value with return program
> >
> > The initial RFC for this was posted in [0] and later discussed more
> > and which ended up with the session idea [1]
> >
> > Having entry together with return probe for given function is common
> > use case for tetragon, bpftrace and most likely for others.
> >
> > At the moment if we want both entry and return probe to execute bpf
> > program we need to create two (entry and return probe) links. The link
> > for return probe creates extra entry probe to setup the return probe.
> > The extra entry probe execution could be omitted if we had a way to
> > use just single link for both entry and exit probe.
> >
> > In addition the possibility to control the return program execution
> > and sharing data within entry and return probe allows for other use
> > cases.
> >
> > Changes from last RFC version [1]:
> >   - changed wrapper name to session
> >   - changed flag to adding new attach type for session:
> >       BPF_TRACE_KPROBE_MULTI_SESSION
> >     it's more convenient wrt filtering on kfuncs setup and seems
> >     to make more sense alltogether
> >   - renamed bpf_kprobe_multi_is_return to bpf_session_is_return
> >   - added bpf_session_cookie kfunc, which actually already works
> >     on current fprobe implementation (not just fprobe-on-fgraph)
> >     and it provides the shared data between entry/return probes [Andrii]
> >
> >     we could actually make the cookie size configurable.. thoughts?
> >     (it's 8 bytes atm)
> >
> 
> Attach cookie is fixed at 8 bytes and that works pretty well. I think
> beyond 8 bytes there is no clearly "right" size. A common case would
> be to capture arguments in kprobe to handle in kretprobe, and there
> you might need at least 40+ bytes, which seems wasteful. So I want to
> say that it's probably good to hard-code it to just 8 bytes (enough to
> store timestamp and you can even fit in some flags if you reduce
> timestamp precision from nanoseconds to microseconds), or use it as an
> index into array or some other data structure.
> 
> let's keep it simple?

+1 for keeping it simple. Use it as a key if you need more.

Re: [PATCH bpf-next 0/7] bpf: Introduce kprobe_multi session attach

[Jiri Olsa]

On Tue, Apr 23, 2024 at 10:12:07PM -0700, John Fastabend wrote:
> Andrii Nakryiko wrote:
> > On Mon, Apr 22, 2024 at 5:12 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > hi,
> > > adding support to attach kprobe program through kprobe_multi link
> > > in a session mode, which means:
> > >   - program is attached to both function entry and return
> > >   - entry program can decided if the return program gets executed
> > >   - entry program can share u64 cookie value with return program
> > >
> > > The initial RFC for this was posted in [0] and later discussed more
> > > and which ended up with the session idea [1]
> > >
> > > Having entry together with return probe for given function is common
> > > use case for tetragon, bpftrace and most likely for others.
> > >
> > > At the moment if we want both entry and return probe to execute bpf
> > > program we need to create two (entry and return probe) links. The link
> > > for return probe creates extra entry probe to setup the return probe.
> > > The extra entry probe execution could be omitted if we had a way to
> > > use just single link for both entry and exit probe.
> > >
> > > In addition the possibility to control the return program execution
> > > and sharing data within entry and return probe allows for other use
> > > cases.
> > >
> > > Changes from last RFC version [1]:
> > >   - changed wrapper name to session
> > >   - changed flag to adding new attach type for session:
> > >       BPF_TRACE_KPROBE_MULTI_SESSION
> > >     it's more convenient wrt filtering on kfuncs setup and seems
> > >     to make more sense alltogether
> > >   - renamed bpf_kprobe_multi_is_return to bpf_session_is_return
> > >   - added bpf_session_cookie kfunc, which actually already works
> > >     on current fprobe implementation (not just fprobe-on-fgraph)
> > >     and it provides the shared data between entry/return probes [Andrii]
> > >
> > >     we could actually make the cookie size configurable.. thoughts?
> > >     (it's 8 bytes atm)
> > >
> > 
> > Attach cookie is fixed at 8 bytes and that works pretty well. I think
> > beyond 8 bytes there is no clearly "right" size. A common case would
> > be to capture arguments in kprobe to handle in kretprobe, and there
> > you might need at least 40+ bytes, which seems wasteful. So I want to
> > say that it's probably good to hard-code it to just 8 bytes (enough to
> > store timestamp and you can even fit in some flags if you reduce
> > timestamp precision from nanoseconds to microseconds), or use it as an
> > index into array or some other data structure.
> > 
> > let's keep it simple?
> 
> +1 for keeping it simple. Use it as a key if you need more.

ok, it's true then it'd be tricky wrt verifier being able to check
the proper size and it does work with attach cookie so far

jirka