[PATCH RFC bpf-next 0/4] bpf: Add support to attach return prog in kprobe multi

[PATCH RFC bpf-next 0/4] bpf: Add support to attach return prog in kprobe multi

[Jiri Olsa]

hi,
adding support to attach both entry and return bpf program on single
kprobe multi link.

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 it's possible to control the execution of the return probe
with the return value of the entry bpf program. If the entry program
returns 0 the return probe is installed and executed, otherwise it's
skip.

I'm still working on the tetragon change, so I'll be sending non-RFC
version once that's ready, meanwhile any ideas and feedback on the
approach would be great.

The change in bpftrace [1] using the new interface shows speed increase
with tracing perf bench messaging:

  # perf bench sched messaging -l 100000

having system wide bpftrace:

  # bpftrace -e 'kprobe:ksys_write { }, kretprobe:ksys_write { }'

without bpftrace:

  # Running 'sched/messaging' benchmark:
  # 20 sender and receiver processes per group
  # 10 groups == 400 processes run

       Total time: 119.595 [sec]

   Performance counter stats for 'perf bench sched messaging -l 100000':

     102,419,967,282      cycles:u
   5,652,444,107,001      cycles:k
   5,782,645,019,612      cycles
      22,187,151,206      instructions:u                   #    0.22  insn per cycle
   2,979,040,498,455      instructions:k                   #    0.53  insn per cycle

       119.671169829 seconds time elapsed

        94.959198000 seconds user
      1815.371616000 seconds sys

with current bpftrace:

  # Running 'sched/messaging' benchmark:
  # 20 sender and receiver processes per group
  # 10 groups == 400 processes run

       Total time: 221.153 [sec]

   Performance counter stats for 'perf bench sched messaging -l 100000':

     125,292,164,504      cycles:u
  10,315,020,393,735      cycles:k
  10,501,379,274,042      cycles
      22,187,583,545      instructions:u                   #    0.18  insn per cycle
   4,856,893,111,303      instructions:k                   #    0.47  insn per cycle

       221.229234283 seconds time elapsed

       103.792498000 seconds user
      3432.643302000 seconds sys

with bpftrace using the new interface:

  # Running 'sched/messaging' benchmark:
  # 20 sender and receiver processes per group
  # 10 groups == 400 processes run

       Total time: 157.825 [sec]

   Performance counter stats for 'perf bench sched messaging -l 100000':

     102,423,112,279      cycles:u
   7,450,856,354,744      cycles:k
   7,584,769,726,693      cycles
      22,187,270,661      instructions:u                   #    0.22  insn per cycle
   3,985,522,383,425      instructions:k                   #    0.53  insn per cycle

       157.900787760 seconds time elapsed

        97.953898000 seconds user
      2425.314753000 seconds sys

thanks,
jirka


[1] https://github.com/bpftrace/bpftrace/pull/2984
---
Jiri Olsa (4):
      fprobe: Add entry/exit callbacks types
      bpf: Add return prog to kprobe multi
      libbpf: Add return_prog_fd to kprobe multi opts
      selftests/bpf: Add kprobe multi return prog test

 include/linux/fprobe.h                                       |  18 ++++++++++------
 include/uapi/linux/bpf.h                                     |   4 +++-
 kernel/trace/bpf_trace.c                                     |  50 ++++++++++++++++++++++++++++++++-----------
 tools/include/uapi/linux/bpf.h                               |   4 +++-
 tools/lib/bpf/bpf.c                                          |   1 +
 tools/lib/bpf/bpf.h                                          |   1 +
 tools/lib/bpf/libbpf.c                                       |   5 +++++
 tools/lib/bpf/libbpf.h                                       |   6 +++++-
 tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c   |  53 +++++++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/kprobe_multi_return_prog.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 10 files changed, 226 insertions(+), 21 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_return_prog.c

[PATCH RFC bpf-next 1/4] fprobe: Add entry/exit callbacks types

[Jiri Olsa]

We are going to store callbacks in following change,
so this will ease up the code.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/fprobe.h | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/include/linux/fprobe.h b/include/linux/fprobe.h
index 3e03758151f4..f39869588117 100644
--- a/include/linux/fprobe.h
+++ b/include/linux/fprobe.h
@@ -7,6 +7,16 @@
 #include <linux/ftrace.h>
 #include <linux/rethook.h>
 
+struct fprobe;
+
+typedef int (*fprobe_entry_cb)(struct fprobe *fp, unsigned long entry_ip,
+			       unsigned long ret_ip, struct pt_regs *regs,
+			       void *entry_data);
+
+typedef void (*fprobe_exit_cb)(struct fprobe *fp, unsigned long entry_ip,
+			       unsigned long ret_ip, struct pt_regs *regs,
+			       void *entry_data);
+
 /**
  * struct fprobe - ftrace based probe.
  * @ops: The ftrace_ops.
@@ -34,12 +44,8 @@ struct fprobe {
 	size_t			entry_data_size;
 	int			nr_maxactive;
 
-	int (*entry_handler)(struct fprobe *fp, unsigned long entry_ip,
-			     unsigned long ret_ip, struct pt_regs *regs,
-			     void *entry_data);
-	void (*exit_handler)(struct fprobe *fp, unsigned long entry_ip,
-			     unsigned long ret_ip, struct pt_regs *regs,
-			     void *entry_data);
+	fprobe_entry_cb entry_handler;
+	fprobe_exit_cb  exit_handler;
 };
 
 /* This fprobe is soft-disabled. */
-- 
2.43.0

Re: [PATCH RFC bpf-next 1/4] fprobe: Add entry/exit callbacks types

[Masami Hiramatsu]

Hi,

On Wed,  7 Feb 2024 16:35:47 +0100
Jiri Olsa <jolsa@kernel.org> wrote:

> We are going to store callbacks in following change,
> so this will ease up the code.
> 

Yeah, this looks good to me.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Hmm, can I pick this in my for-next tree?

Thank you,

> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/fprobe.h | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/fprobe.h b/include/linux/fprobe.h
> index 3e03758151f4..f39869588117 100644
> --- a/include/linux/fprobe.h
> +++ b/include/linux/fprobe.h
> @@ -7,6 +7,16 @@
>  #include <linux/ftrace.h>
>  #include <linux/rethook.h>
>  
> +struct fprobe;
> +
> +typedef int (*fprobe_entry_cb)(struct fprobe *fp, unsigned long entry_ip,
> +			       unsigned long ret_ip, struct pt_regs *regs,
> +			       void *entry_data);
> +
> +typedef void (*fprobe_exit_cb)(struct fprobe *fp, unsigned long entry_ip,
> +			       unsigned long ret_ip, struct pt_regs *regs,
> +			       void *entry_data);
> +
>  /**
>   * struct fprobe - ftrace based probe.
>   * @ops: The ftrace_ops.
> @@ -34,12 +44,8 @@ struct fprobe {
>  	size_t			entry_data_size;
>  	int			nr_maxactive;
>  
> -	int (*entry_handler)(struct fprobe *fp, unsigned long entry_ip,
> -			     unsigned long ret_ip, struct pt_regs *regs,
> -			     void *entry_data);
> -	void (*exit_handler)(struct fprobe *fp, unsigned long entry_ip,
> -			     unsigned long ret_ip, struct pt_regs *regs,
> -			     void *entry_data);
> +	fprobe_entry_cb entry_handler;
> +	fprobe_exit_cb  exit_handler;
>  };
>  
>  /* This fprobe is soft-disabled. */
> -- 
> 2.43.0
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH RFC bpf-next 1/4] fprobe: Add entry/exit callbacks types

[Jiri Olsa]

On Wed, Feb 14, 2024 at 12:35:46AM +0900, Masami Hiramatsu wrote:
> Hi,
> 
> On Wed,  7 Feb 2024 16:35:47 +0100
> Jiri Olsa <jolsa@kernel.org> wrote:
> 
> > We are going to store callbacks in following change,
> > so this will ease up the code.
> > 
> 
> Yeah, this looks good to me.
> 
> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Hmm, can I pick this in my for-next tree?

I don't mind you picking that up, I'll have to send new version of
the rest of the patchset, but I think I'll still need these types

jirka

> 
> Thank you,
> 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/linux/fprobe.h | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/linux/fprobe.h b/include/linux/fprobe.h
> > index 3e03758151f4..f39869588117 100644
> > --- a/include/linux/fprobe.h
> > +++ b/include/linux/fprobe.h
> > @@ -7,6 +7,16 @@
> >  #include <linux/ftrace.h>
> >  #include <linux/rethook.h>
> >  
> > +struct fprobe;
> > +
> > +typedef int (*fprobe_entry_cb)(struct fprobe *fp, unsigned long entry_ip,
> > +			       unsigned long ret_ip, struct pt_regs *regs,
> > +			       void *entry_data);
> > +
> > +typedef void (*fprobe_exit_cb)(struct fprobe *fp, unsigned long entry_ip,
> > +			       unsigned long ret_ip, struct pt_regs *regs,
> > +			       void *entry_data);
> > +
> >  /**
> >   * struct fprobe - ftrace based probe.
> >   * @ops: The ftrace_ops.
> > @@ -34,12 +44,8 @@ struct fprobe {
> >  	size_t			entry_data_size;
> >  	int			nr_maxactive;
> >  
> > -	int (*entry_handler)(struct fprobe *fp, unsigned long entry_ip,
> > -			     unsigned long ret_ip, struct pt_regs *regs,
> > -			     void *entry_data);
> > -	void (*exit_handler)(struct fprobe *fp, unsigned long entry_ip,
> > -			     unsigned long ret_ip, struct pt_regs *regs,
> > -			     void *entry_data);
> > +	fprobe_entry_cb entry_handler;
> > +	fprobe_exit_cb  exit_handler;
> >  };
> >  
> >  /* This fprobe is soft-disabled. */
> > -- 
> > 2.43.0
> > 
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

[PATCH RFC bpf-next 2/4] bpf: Add return prog to kprobe multi

[Jiri Olsa]

Adding support to attach both entry and return bpf program on single
kprobe multi link.

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 it's possible to control the execution of the return probe
with the return value of the entry bpf program. If the entry program
returns 0 the return probe is installed and executed, otherwise it's
skip.

Suggested-by: Viktor Malik <vmalik@redhat.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/uapi/linux/bpf.h       |  4 ++-
 kernel/trace/bpf_trace.c       | 50 ++++++++++++++++++++++++++--------
 tools/include/uapi/linux/bpf.h |  4 ++-
 3 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index d96708380e52..9ccd6e6850aa 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1243,7 +1243,8 @@ enum bpf_perf_event_type {
  * BPF_TRACE_KPROBE_MULTI attach type to create return probe.
  */
 enum {
-	BPF_F_KPROBE_MULTI_RETURN = (1U << 0)
+	BPF_F_KPROBE_MULTI_RETURN      = (1U << 0),
+	BPF_F_KPROBE_MULTI_RETURN_PROG = (1U << 1),
 };
 
 /* link_create.uprobe_multi.flags used in LINK_CREATE command for
@@ -1690,6 +1691,7 @@ union bpf_attr {
 				__aligned_u64	syms;
 				__aligned_u64	addrs;
 				__aligned_u64	cookies;
+				__u32           return_prog_fd;
 			} kprobe_multi;
 			struct {
 				/* this is overlaid with the target_btf_id above. */
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 241ddf5e3895..37e58a132ef0 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2587,6 +2587,7 @@ struct bpf_kprobe_multi_link {
 	u32 mods_cnt;
 	struct module **mods;
 	u32 flags;
+	struct bpf_prog *return_prog;
 };
 
 struct bpf_kprobe_multi_run_ctx {
@@ -2663,6 +2664,8 @@ static void bpf_kprobe_multi_link_release(struct bpf_link *link)
 	kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
 	unregister_fprobe(&kmulti_link->fp);
 	kprobe_multi_put_modules(kmulti_link->mods, kmulti_link->mods_cnt);
+	if (kmulti_link->return_prog)
+		bpf_prog_put(kmulti_link->return_prog);
 }
 
 static void bpf_kprobe_multi_link_dealloc(struct bpf_link *link)
@@ -2792,7 +2795,8 @@ 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)
+			   struct bpf_prog *prog, unsigned long entry_ip,
+			   struct pt_regs *regs)
 {
 	struct bpf_kprobe_multi_run_ctx run_ctx = {
 		.link = link,
@@ -2802,7 +2806,7 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
 	int err;
 
 	if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
-		bpf_prog_inc_misses_counter(link->link.prog);
+		bpf_prog_inc_misses_counter(prog);
 		err = 0;
 		goto out;
 	}
@@ -2810,7 +2814,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);
-	err = bpf_prog_run(link->link.prog, regs);
+	err = bpf_prog_run(prog, regs);
 	bpf_reset_run_ctx(old_run_ctx);
 	rcu_read_unlock();
 	migrate_enable();
@@ -2828,8 +2832,8 @@ kprobe_multi_link_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);
-	return 0;
+	return kprobe_multi_link_prog_run(link, link->link.prog,
+					  get_entry_ip(fentry_ip), regs);
 }
 
 static void
@@ -2838,9 +2842,11 @@ kprobe_multi_link_exit_handler(struct fprobe *fp, unsigned long fentry_ip,
 			       void *data)
 {
 	struct bpf_kprobe_multi_link *link;
+	struct bpf_prog *prog;
 
 	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
-	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
+	prog = link->return_prog ?: link->link.prog;
+	kprobe_multi_link_prog_run(link, prog, get_entry_ip(fentry_ip), regs);
 }
 
 static int symbols_cmp_r(const void *a, const void *b, const void *priv)
@@ -2960,6 +2966,7 @@ static int addrs_check_error_injection_list(unsigned long *addrs, u32 cnt)
 int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
 {
 	struct bpf_kprobe_multi_link *link = NULL;
+	struct bpf_prog *return_prog = NULL;
 	struct bpf_link_primer link_primer;
 	void __user *ucookies;
 	unsigned long *addrs;
@@ -2977,7 +2984,8 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 		return -EINVAL;
 
 	flags = attr->link_create.kprobe_multi.flags;
-	if (flags & ~BPF_F_KPROBE_MULTI_RETURN)
+	if (flags & ~(BPF_F_KPROBE_MULTI_RETURN |
+		      BPF_F_KPROBE_MULTI_RETURN_PROG))
 		return -EINVAL;
 
 	uaddrs = u64_to_user_ptr(attr->link_create.kprobe_multi.addrs);
@@ -2991,10 +2999,20 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 	if (cnt > MAX_KPROBE_MULTI_CNT)
 		return -E2BIG;
 
+	if (flags & BPF_F_KPROBE_MULTI_RETURN_PROG) {
+		if (flags & BPF_F_KPROBE_MULTI_RETURN)
+			return -EINVAL;
+		return_prog = bpf_prog_get(attr->link_create.kprobe_multi.return_prog_fd);
+		if (IS_ERR(return_prog))
+			return PTR_ERR(return_prog);
+	}
+
 	size = cnt * sizeof(*addrs);
 	addrs = kvmalloc_array(cnt, sizeof(*addrs), GFP_KERNEL);
-	if (!addrs)
-		return -ENOMEM;
+	if (!addrs) {
+		err = -ENOMEM;
+		goto error;
+	}
 
 	ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
 	if (ucookies) {
@@ -3054,15 +3072,21 @@ 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_PROG) {
 		link->fp.entry_handler = kprobe_multi_link_handler;
+		link->fp.exit_handler = kprobe_multi_link_exit_handler;
+	} else {
+		if (flags & BPF_F_KPROBE_MULTI_RETURN)
+			link->fp.exit_handler = kprobe_multi_link_exit_handler;
+		else
+			link->fp.entry_handler = kprobe_multi_link_handler;
+	}
 
 	link->addrs = addrs;
 	link->cookies = cookies;
 	link->cnt = cnt;
 	link->flags = flags;
+	link->return_prog = return_prog;
 
 	if (cookies) {
 		/*
@@ -3094,6 +3118,8 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 	return bpf_link_settle(&link_primer);
 
 error:
+	if (return_prog)
+		bpf_prog_put(return_prog);
 	kfree(link);
 	kvfree(addrs);
 	kvfree(cookies);
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index d96708380e52..9ccd6e6850aa 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1243,7 +1243,8 @@ enum bpf_perf_event_type {
  * BPF_TRACE_KPROBE_MULTI attach type to create return probe.
  */
 enum {
-	BPF_F_KPROBE_MULTI_RETURN = (1U << 0)
+	BPF_F_KPROBE_MULTI_RETURN      = (1U << 0),
+	BPF_F_KPROBE_MULTI_RETURN_PROG = (1U << 1),
 };
 
 /* link_create.uprobe_multi.flags used in LINK_CREATE command for
@@ -1690,6 +1691,7 @@ union bpf_attr {
 				__aligned_u64	syms;
 				__aligned_u64	addrs;
 				__aligned_u64	cookies;
+				__u32           return_prog_fd;
 			} kprobe_multi;
 			struct {
 				/* this is overlaid with the target_btf_id above. */
-- 
2.43.0

Re: [PATCH RFC bpf-next 2/4] bpf: Add return prog to kprobe multi

[Alexei Starovoitov]

On Wed, Feb 7, 2024 at 7:36 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
>
> In addition it's possible to control the execution of the return probe
> with the return value of the entry bpf program. If the entry program
> returns 0 the return probe is installed and executed, otherwise it's
> skip.

Let's not sneak in a big change in behavior like this.

> @@ -2828,8 +2832,8 @@ kprobe_multi_link_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);
> -       return 0;
> +       return kprobe_multi_link_prog_run(link, link->link.prog,
> +                                         get_entry_ip(fentry_ip), regs);

This is big.
What motivates this change?

Re: [PATCH RFC bpf-next 2/4] bpf: Add return prog to kprobe multi

[Jiri Olsa]

On Thu, Feb 08, 2024 at 11:05:06AM -0800, Alexei Starovoitov wrote:
> On Wed, Feb 7, 2024 at 7:36 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> >
> > In addition it's possible to control the execution of the return probe
> > with the return value of the entry bpf program. If the entry program
> > returns 0 the return probe is installed and executed, otherwise it's
> > skip.
> 
> Let's not sneak in a big change in behavior like this.
> 
> > @@ -2828,8 +2832,8 @@ kprobe_multi_link_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);
> > -       return 0;
> > +       return kprobe_multi_link_prog_run(link, link->link.prog,
> > +                                         get_entry_ip(fentry_ip), regs);
> 
> This is big.
> What motivates this change?

hm, this makes the difference only when the new interface is used, so I
thought about it as part of the new interface.. but right, if existing
programs are attached with new interface and un-aware of this, that might
cause issues

in tetragon we use logic where the entry probe does some filtering and
related return probe either gathers other data or quit based on that
filter result

with this change we can eliminate return probe being triggered completely
when the entry filter does not pass

jirka

[PATCH RFC bpf-next 3/4] libbpf: Add return_prog_fd to kprobe multi opts

[Jiri Olsa]

Adding support to specify return prog file descriptor in struct
bpf_kprobe_multi_opts.

When the return_prog bool is set true the return_prog_fd is used
as bpf program to be executed for the return probe.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/lib/bpf/bpf.c    | 1 +
 tools/lib/bpf/bpf.h    | 1 +
 tools/lib/bpf/libbpf.c | 5 +++++
 tools/lib/bpf/libbpf.h | 6 +++++-
 4 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 97ec005c3c47..9f296957bd30 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -771,6 +771,7 @@ int bpf_link_create(int prog_fd, int target_fd,
 		attr.link_create.kprobe_multi.syms = ptr_to_u64(OPTS_GET(opts, kprobe_multi.syms, 0));
 		attr.link_create.kprobe_multi.addrs = ptr_to_u64(OPTS_GET(opts, kprobe_multi.addrs, 0));
 		attr.link_create.kprobe_multi.cookies = ptr_to_u64(OPTS_GET(opts, kprobe_multi.cookies, 0));
+		attr.link_create.kprobe_multi.return_prog_fd = OPTS_GET(opts, kprobe_multi.return_prog_fd, 0);
 		if (!OPTS_ZEROED(opts, kprobe_multi))
 			return libbpf_err(-EINVAL);
 		break;
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index f866e98b2436..e04a14134a21 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -399,6 +399,7 @@ struct bpf_link_create_opts {
 			const char **syms;
 			const unsigned long *addrs;
 			const __u64 *cookies;
+			__u32 return_prog_fd;
 		} kprobe_multi;
 		struct {
 			__u32 flags;
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 01f407591a92..022a68b9fe83 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -11073,6 +11073,11 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
 	lopts.kprobe_multi.cnt = cnt;
 	lopts.kprobe_multi.flags = retprobe ? BPF_F_KPROBE_MULTI_RETURN : 0;
 
+	if (OPTS_GET(opts, return_prog, false)) {
+		lopts.kprobe_multi.return_prog_fd = OPTS_GET(opts, return_prog_fd, 0);
+		lopts.kprobe_multi.flags |= BPF_F_KPROBE_MULTI_RETURN_PROG;
+	}
+
 	link = calloc(1, sizeof(*link));
 	if (!link) {
 		err = -ENOMEM;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 5723cbbfcc41..d77b953009d6 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -539,10 +539,14 @@ struct bpf_kprobe_multi_opts {
 	size_t cnt;
 	/* create return kprobes */
 	bool retprobe;
+	/* attach return program (specified by return_prog_fd file descriptor) */
+	bool return_prog;
+	/* return program fd */
+	int return_prog_fd;
 	size_t :0;
 };
 
-#define bpf_kprobe_multi_opts__last_field retprobe
+#define bpf_kprobe_multi_opts__last_field return_prog_fd
 
 LIBBPF_API struct bpf_link *
 bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
-- 
2.43.0

[PATCH RFC bpf-next 4/4] selftests/bpf: Add kprobe multi return prog test

[Jiri Olsa]

Adding test for kprobe multi return program.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../bpf/prog_tests/kprobe_multi_test.c        |  53 +++++++++
 .../bpf/progs/kprobe_multi_return_prog.c      | 105 ++++++++++++++++++
 2 files changed, 158 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_return_prog.c

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 05000810e28e..b6991a6c0b2d 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_return_prog.skel.h"
 #include "bpf/libbpf_internal.h"
 #include "bpf/hashmap.h"
 
@@ -326,6 +327,56 @@ static void test_attach_api_fails(void)
 	kprobe_multi__destroy(skel);
 }
 
+static void test_attach_return_prog(void)
+{
+	struct kprobe_multi_return_prog *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_return_prog__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "fentry_raw_skel_load"))
+		goto cleanup;
+
+	prog_fd = bpf_program__fd(skel->progs.test_kretprobe);
+	opts.return_prog = true;
+	opts.return_prog_fd = prog_fd;
+
+	skel->bss->pid = getpid();
+	link = bpf_program__attach_kprobe_multi_opts(skel->progs.test_kprobe,
+						     "bpf_fentry_test*", &opts);
+	if (!ASSERT_OK_PTR(link, "bpf_program__attach_kprobe_multi_opts2"))
+		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_return_prog__destroy(skel);
+}
+
 static size_t symbol_hash(long key, void *ctx __maybe_unused)
 {
 	return str_hash((const char *) key);
@@ -538,4 +589,6 @@ void test_kprobe_multi_test(void)
 		test_attach_api_fails();
 	if (test__start_subtest("attach_override"))
 		test_attach_override();
+	if (test__start_subtest("return_prog"))
+		test_attach_return_prog();
 }
diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi_return_prog.c b/tools/testing/selftests/bpf/progs/kprobe_multi_return_prog.c
new file mode 100644
index 000000000000..e4e49ffa110f
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/kprobe_multi_return_prog.c
@@ -0,0 +1,105 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <stdbool.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 kprobe_multi_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.multi")
+int test_kprobe(struct pt_regs *ctx)
+{
+	return kprobe_multi_check(ctx, false);
+}
+
+SEC("kretprobe.multi")
+int test_kretprobe(struct pt_regs *ctx)
+{
+	return kprobe_multi_check(ctx, true);
+}
-- 
2.43.0

Re: [PATCH RFC bpf-next 0/4] bpf: Add support to attach return prog in kprobe multi

[Andrii Nakryiko]

On Wed, Feb 7, 2024 at 7:35 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> hi,
> adding support to attach both entry and return bpf program on single
> kprobe multi link.
>
> 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 it's possible to control the execution of the return probe
> with the return value of the entry bpf program. If the entry program
> returns 0 the return probe is installed and executed, otherwise it's
> skip.
>

In general, I think this is a very useful ability to have a combined
entry/return program.

But the way you implement it with extra flag and extra fd parameter
makes it harder to have a nice high-level support in libbpf (and
presumably other BPF loader libraries) for this.

When I was thinking about doing something like this, I was considering
adding a new program type, actually. That way it's possible to define
this "let's skip return probe" protocol without backwards
compatibility concerns. It's easier to use it declaratively in libbpf.
You just declare SEC("kprobe.wrap/...") (or whatever the name,
something to designate that it's both entry and exit probe) as one
program and in the code there would be some way to determine whether
we are in entry mode or exit mode (helper or field in the custom
context type, the latter being faster and more usable, but it's
probably not critical).

Another frequently requested feature and a very common use case is to
measure duration of the function, so if we have a custom type, we can
have a field to record entry timestamp and let user calculate
duration, if necessary. Without this users have to pay a bunch of
extra overhead to record timestamp and put it into hashmap (keyed by
thread id) or thread local storage (even if they have no other use for
thread local storage).

Also, consider that a similar concept is applicable to uprobes and we
should do that as well, in similar fashion. And the above approach
works for both kprobe/kretprobe and uprobe/uretprobe cases, because
they have the same pt_regs data structure as a context (even if for
exit probes most of the values of pt_regs are not that meaningful).

So anyways, great feature, but let's discuss end-to-end usability
story before we finalize the implementation?


> I'm still working on the tetragon change, so I'll be sending non-RFC
> version once that's ready, meanwhile any ideas and feedback on the
> approach would be great.
>
> The change in bpftrace [1] using the new interface shows speed increase
> with tracing perf bench messaging:
>
>   # perf bench sched messaging -l 100000
>
> having system wide bpftrace:
>
>   # bpftrace -e 'kprobe:ksys_write { }, kretprobe:ksys_write { }'
>
> without bpftrace:
>
>   # Running 'sched/messaging' benchmark:
>   # 20 sender and receiver processes per group
>   # 10 groups == 400 processes run
>
>        Total time: 119.595 [sec]
>
>    Performance counter stats for 'perf bench sched messaging -l 100000':
>
>      102,419,967,282      cycles:u
>    5,652,444,107,001      cycles:k
>    5,782,645,019,612      cycles
>       22,187,151,206      instructions:u                   #    0.22  insn per cycle
>    2,979,040,498,455      instructions:k                   #    0.53  insn per cycle
>
>        119.671169829 seconds time elapsed
>
>         94.959198000 seconds user
>       1815.371616000 seconds sys
>
> with current bpftrace:
>
>   # Running 'sched/messaging' benchmark:
>   # 20 sender and receiver processes per group
>   # 10 groups == 400 processes run
>
>        Total time: 221.153 [sec]
>
>    Performance counter stats for 'perf bench sched messaging -l 100000':
>
>      125,292,164,504      cycles:u

btw, why +25% in user space?... this looks weird


>   10,315,020,393,735      cycles:k
>   10,501,379,274,042      cycles
>       22,187,583,545      instructions:u                   #    0.18  insn per cycle
>    4,856,893,111,303      instructions:k                   #    0.47  insn per cycle
>
>        221.229234283 seconds time elapsed
>
>        103.792498000 seconds user
>       3432.643302000 seconds sys
>
> with bpftrace using the new interface:
>
>   # Running 'sched/messaging' benchmark:
>   # 20 sender and receiver processes per group
>   # 10 groups == 400 processes run
>
>        Total time: 157.825 [sec]
>
>    Performance counter stats for 'perf bench sched messaging -l 100000':
>
>      102,423,112,279      cycles:u
>    7,450,856,354,744      cycles:k
>    7,584,769,726,693      cycles
>       22,187,270,661      instructions:u                   #    0.22  insn per cycle
>    3,985,522,383,425      instructions:k                   #    0.53  insn per cycle
>
>        157.900787760 seconds time elapsed
>
>         97.953898000 seconds user
>       2425.314753000 seconds sys
>
> thanks,
> jirka
>
>
> [1] https://github.com/bpftrace/bpftrace/pull/2984
> ---
> Jiri Olsa (4):
>       fprobe: Add entry/exit callbacks types
>       bpf: Add return prog to kprobe multi
>       libbpf: Add return_prog_fd to kprobe multi opts
>       selftests/bpf: Add kprobe multi return prog test
>
>  include/linux/fprobe.h                                       |  18 ++++++++++------
>  include/uapi/linux/bpf.h                                     |   4 +++-
>  kernel/trace/bpf_trace.c                                     |  50 ++++++++++++++++++++++++++++++++-----------
>  tools/include/uapi/linux/bpf.h                               |   4 +++-
>  tools/lib/bpf/bpf.c                                          |   1 +
>  tools/lib/bpf/bpf.h                                          |   1 +
>  tools/lib/bpf/libbpf.c                                       |   5 +++++
>  tools/lib/bpf/libbpf.h                                       |   6 +++++-
>  tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c   |  53 +++++++++++++++++++++++++++++++++++++++++++++
>  tools/testing/selftests/bpf/progs/kprobe_multi_return_prog.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  10 files changed, 226 insertions(+), 21 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_return_prog.c

Re: [PATCH RFC bpf-next 0/4] bpf: Add support to attach return prog in kprobe multi

[Jiri Olsa]

On Thu, Feb 08, 2024 at 11:35:18AM -0800, Andrii Nakryiko wrote:
> On Wed, Feb 7, 2024 at 7:35 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > hi,
> > adding support to attach both entry and return bpf program on single
> > kprobe multi link.
> >
> > 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 it's possible to control the execution of the return probe
> > with the return value of the entry bpf program. If the entry program
> > returns 0 the return probe is installed and executed, otherwise it's
> > skip.
> >
> 
> In general, I think this is a very useful ability to have a combined
> entry/return program.

great, thanks

> 
> But the way you implement it with extra flag and extra fd parameter
> makes it harder to have a nice high-level support in libbpf (and
> presumably other BPF loader libraries) for this.
> 
> When I was thinking about doing something like this, I was considering
> adding a new program type, actually. That way it's possible to define
> this "let's skip return probe" protocol without backwards
> compatibility concerns. It's easier to use it declaratively in libbpf.

ok, that seems cleaner.. but we need to use current kprobe programs,
so not sure at the moment how would that fit in.. did you mean new
link type?

> You just declare SEC("kprobe.wrap/...") (or whatever the name,
> something to designate that it's both entry and exit probe) as one
> program and in the code there would be some way to determine whether
> we are in entry mode or exit mode (helper or field in the custom
> context type, the latter being faster and more usable, but it's
> probably not critical).

hum, so the single program would be for both entry and exit probe,
I'll need to check how bad it'd be for us, but it'd probably mean
just one extra tail call, so it's likely ok

> 
> Another frequently requested feature and a very common use case is to
> measure duration of the function, so if we have a custom type, we can
> have a field to record entry timestamp and let user calculate

you mean bpf program context right?

> duration, if necessary. Without this users have to pay a bunch of
> extra overhead to record timestamp and put it into hashmap (keyed by
> thread id) or thread local storage (even if they have no other use for
> thread local storage).

sounds good

> 
> Also, consider that a similar concept is applicable to uprobes and we
> should do that as well, in similar fashion. And the above approach
> works for both kprobe/kretprobe and uprobe/uretprobe cases, because
> they have the same pt_regs data structure as a context (even if for
> exit probes most of the values of pt_regs are not that meaningful).

ok, that seems useful for uprobes as well

btw I have another unrelated change in stash that that let you choose
the bpf_prog_active or prog->active re-entry check before program is
executed in krobe_multi link.. I also added extra flag, and it still
seems like good idea to me, thoughts? ;-)

> 
> So anyways, great feature, but let's discuss end-to-end usability
> story before we finalize the implementation?

yep, it does say RFC in the subject ;-)

> 
> 
> > I'm still working on the tetragon change, so I'll be sending non-RFC
> > version once that's ready, meanwhile any ideas and feedback on the
> > approach would be great.
> >
> > The change in bpftrace [1] using the new interface shows speed increase
> > with tracing perf bench messaging:
> >
> >   # perf bench sched messaging -l 100000
> >
> > having system wide bpftrace:
> >
> >   # bpftrace -e 'kprobe:ksys_write { }, kretprobe:ksys_write { }'
> >
> > without bpftrace:
> >
> >   # Running 'sched/messaging' benchmark:
> >   # 20 sender and receiver processes per group
> >   # 10 groups == 400 processes run
> >
> >        Total time: 119.595 [sec]
> >
> >    Performance counter stats for 'perf bench sched messaging -l 100000':
> >
> >      102,419,967,282      cycles:u
> >    5,652,444,107,001      cycles:k
> >    5,782,645,019,612      cycles
> >       22,187,151,206      instructions:u                   #    0.22  insn per cycle
> >    2,979,040,498,455      instructions:k                   #    0.53  insn per cycle
> >
> >        119.671169829 seconds time elapsed
> >
> >         94.959198000 seconds user
> >       1815.371616000 seconds sys
> >
> > with current bpftrace:
> >
> >   # Running 'sched/messaging' benchmark:
> >   # 20 sender and receiver processes per group
> >   # 10 groups == 400 processes run
> >
> >        Total time: 221.153 [sec]
> >
> >    Performance counter stats for 'perf bench sched messaging -l 100000':
> >
> >      125,292,164,504      cycles:u
> 
> btw, why +25% in user space?... this looks weird

hmm right.. maybe there's some bpftrace timer code that got triggered/executed
more times because it takes long to finish the workload? I'll check

thanks,
jirka

> 
> 
> >   10,315,020,393,735      cycles:k
> >   10,501,379,274,042      cycles
> >       22,187,583,545      instructions:u                   #    0.18  insn per cycle
> >    4,856,893,111,303      instructions:k                   #    0.47  insn per cycle
> >
> >        221.229234283 seconds time elapsed
> >
> >        103.792498000 seconds user
> >       3432.643302000 seconds sys
> >
> > with bpftrace using the new interface:
> >
> >   # Running 'sched/messaging' benchmark:
> >   # 20 sender and receiver processes per group
> >   # 10 groups == 400 processes run
> >
> >        Total time: 157.825 [sec]
> >
> >    Performance counter stats for 'perf bench sched messaging -l 100000':
> >
> >      102,423,112,279      cycles:u
> >    7,450,856,354,744      cycles:k
> >    7,584,769,726,693      cycles
> >       22,187,270,661      instructions:u                   #    0.22  insn per cycle
> >    3,985,522,383,425      instructions:k                   #    0.53  insn per cycle
> >
> >        157.900787760 seconds time elapsed
> >
> >         97.953898000 seconds user
> >       2425.314753000 seconds sys
> >
> > thanks,
> > jirka
> >
> >
> > [1] https://github.com/bpftrace/bpftrace/pull/2984
> > ---
> > Jiri Olsa (4):
> >       fprobe: Add entry/exit callbacks types
> >       bpf: Add return prog to kprobe multi
> >       libbpf: Add return_prog_fd to kprobe multi opts
> >       selftests/bpf: Add kprobe multi return prog test
> >
> >  include/linux/fprobe.h                                       |  18 ++++++++++------
> >  include/uapi/linux/bpf.h                                     |   4 +++-
> >  kernel/trace/bpf_trace.c                                     |  50 ++++++++++++++++++++++++++++++++-----------
> >  tools/include/uapi/linux/bpf.h                               |   4 +++-
> >  tools/lib/bpf/bpf.c                                          |   1 +
> >  tools/lib/bpf/bpf.h                                          |   1 +
> >  tools/lib/bpf/libbpf.c                                       |   5 +++++
> >  tools/lib/bpf/libbpf.h                                       |   6 +++++-
> >  tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c   |  53 +++++++++++++++++++++++++++++++++++++++++++++
> >  tools/testing/selftests/bpf/progs/kprobe_multi_return_prog.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  10 files changed, 226 insertions(+), 21 deletions(-)
> >  create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_return_prog.c

Re: [PATCH RFC bpf-next 0/4] bpf: Add support to attach return prog in kprobe multi

[Andrii Nakryiko]

On Sat, Feb 10, 2024 at 7:31 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Thu, Feb 08, 2024 at 11:35:18AM -0800, Andrii Nakryiko wrote:
> > On Wed, Feb 7, 2024 at 7:35 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > hi,
> > > adding support to attach both entry and return bpf program on single
> > > kprobe multi link.
> > >
> > > 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 it's possible to control the execution of the return probe
> > > with the return value of the entry bpf program. If the entry program
> > > returns 0 the return probe is installed and executed, otherwise it's
> > > skip.
> > >
> >
> > In general, I think this is a very useful ability to have a combined
> > entry/return program.
>
> great, thanks
>
> >
> > But the way you implement it with extra flag and extra fd parameter
> > makes it harder to have a nice high-level support in libbpf (and
> > presumably other BPF loader libraries) for this.
> >
> > When I was thinking about doing something like this, I was considering
> > adding a new program type, actually. That way it's possible to define
> > this "let's skip return probe" protocol without backwards
> > compatibility concerns. It's easier to use it declaratively in libbpf.
>
> ok, that seems cleaner.. but we need to use current kprobe programs,
> so not sure at the moment how would that fit in.. did you mean new
> link type?

It's kind of a less important detail, actually. New program type would
allow us to have an entirely different context type, but I think we
can make do with the existing kprobe program type. We can have a
separate attach_type and link type, just like multi-kprobe and
multi-uprobe are still kprobe programs.

>
> > You just declare SEC("kprobe.wrap/...") (or whatever the name,
> > something to designate that it's both entry and exit probe) as one
> > program and in the code there would be some way to determine whether
> > we are in entry mode or exit mode (helper or field in the custom
> > context type, the latter being faster and more usable, but it's
> > probably not critical).
>
> hum, so the single program would be for both entry and exit probe,
> I'll need to check how bad it'd be for us, but it'd probably mean
> just one extra tail call, so it's likely ok

I guess, I don't know what you are doing there :) I'd recommend
looking at utilizing BPF global subprogs instead of tail calls, if
your kernel allows for that, as that's a saner way to scale BPF
verification.

>
> >
> > Another frequently requested feature and a very common use case is to
> > measure duration of the function, so if we have a custom type, we can
> > have a field to record entry timestamp and let user calculate
>
> you mean bpf program context right?

Yes, when I was writing that I was imagining a new context type with new fields.

But thinking about this a bit more, I like a slightly different
approach now. Instead of having a new context type, kernel capturing
timestamps, etc, what if we introduce a concept of "a session". This
paired kprobe+kretprobe (and same for uprobe+uretprobe) share a common
session. So, say, if we are tracing kernel function abc() and we have
our kwrapper (let's call it that for now, kwrapper and uwrapper?)
program attached to abc, we have something like this:

1) abc() is called
2) we intercept it at its entry, initialize "session"
3) call our kwrapper program in entry mode
4) abc() proceeds, until return
5) our kwrapper program is called in exit mode
6) at this point the session has ended

If abc() is called again or in parallel on another CPU, each such
abc() invocation (with kwrapper prog) has its own session state.

Now, it all sounds scary, but a session could be really just a 8 byte
value that has to last for the duration of a single kprobe+kretprobe
execution.

Imagine we introduce just one new kfunc that would be usable from
kwrapper and uwrapper programs:

u64 *bpf_get_session_cookie(void);

It returns a pointer to an 8-byte slot which initially is set to zero
by kernel (before kprobe/entry part). And then the program can put
anything into it. Either from entry (kprobe/uprobe) or exit
(kretprobe/uretprobe) executions. Whatever value was stored in entry
mode will still be there in exit mode as well.

This gives a lot of flexibility without requiring the kernel to do
anything expensive.

E.g., it's easy to detect if kwrapper program is an entry call or exit call:

u64 *cookie = bpf_get_session_cookie();
if (*cookie == 0) {
  /* entry mode */
  *cookie = 1;
} else {
  /* exit mode */
}

Or if we wanted to measure function duration/latency:

u64 *cookie = bpf_get_session_cookie();
if (*cookie == 0) {
  *cookie = bpf_get_ktime_ns();
} else {
  u64 duration = bpf_get_ktime_ns() - *cookie;
  /* output duration somehow */
}

Yes, I realize special-casing zero might be a bit inconvenient, but I
think simplicity trumps a potential for zero to be a valid value (and
there are always ways to work around zero as a meaningful value).

Now, in more complicated cases 8 bytes of temporary session state
isn't enough, just like BPF cookie being 8 byte (read-only) value
might not be enough. But the solution is the same as with the BPF
cookie. You just use those 8 bytes as a key into ARRAY/HASHMAP/whatnot
storage. It's simple and fast enough for pretty much any case.


Now, how do we implement this storage session? I recently looked into
uprobe/uretprobe implementation, and I know that for uretprobes there
is a dynamically allocated uretprobe_instance (or whatever the name)
allocated for each runtime invocation of uretprobe. We can use that.
We'll need to allocate this instance a bit earlier, before uprobe part
of uwrapper BPF program has to be executed, initialize session cookie
to zero, and store pointer to this memory in bpf_run_ctx (just like we
do for BPF cookie).

I bet there is something similar in the kretprobe case, where we can
carve out 8 bytes and pass it to both entry and exit parts of kwrapper
program.

So anyways, might need some internal refactoring, but seems very
doable and will give a lot of power and flexibility for tracing use
cases.

It would be cool to also have a similar fentry/fexit combo with "BPF
session cookie", but that's something we can think about separately,
probably.


>
> > duration, if necessary. Without this users have to pay a bunch of
> > extra overhead to record timestamp and put it into hashmap (keyed by
> > thread id) or thread local storage (even if they have no other use for
> > thread local storage).
>
> sounds good
>
> >
> > Also, consider that a similar concept is applicable to uprobes and we
> > should do that as well, in similar fashion. And the above approach
> > works for both kprobe/kretprobe and uprobe/uretprobe cases, because
> > they have the same pt_regs data structure as a context (even if for
> > exit probes most of the values of pt_regs are not that meaningful).
>
> ok, that seems useful for uprobes as well

yes, absolutely

>
> btw I have another unrelated change in stash that that let you choose
> the bpf_prog_active or prog->active re-entry check before program is
> executed in krobe_multi link.. I also added extra flag, and it still
> seems like good idea to me, thoughts? ;-)
>

no specific opinion on this from my side, I guess

> >
> > So anyways, great feature, but let's discuss end-to-end usability
> > story before we finalize the implementation?
>
> yep, it does say RFC in the subject ;-)
>
> >
> >

[...]

Re: [PATCH RFC bpf-next 0/4] bpf: Add support to attach return prog in kprobe multi

[Jiri Olsa]

On Mon, Feb 12, 2024 at 08:06:06PM -0800, Andrii Nakryiko wrote:

SNIP

> > > But the way you implement it with extra flag and extra fd parameter
> > > makes it harder to have a nice high-level support in libbpf (and
> > > presumably other BPF loader libraries) for this.
> > >
> > > When I was thinking about doing something like this, I was considering
> > > adding a new program type, actually. That way it's possible to define
> > > this "let's skip return probe" protocol without backwards
> > > compatibility concerns. It's easier to use it declaratively in libbpf.
> >
> > ok, that seems cleaner.. but we need to use current kprobe programs,
> > so not sure at the moment how would that fit in.. did you mean new
> > link type?
> 
> It's kind of a less important detail, actually. New program type would
> allow us to have an entirely different context type, but I think we
> can make do with the existing kprobe program type. We can have a
> separate attach_type and link type, just like multi-kprobe and
> multi-uprobe are still kprobe programs.

ok, having new attach type on top of kprobe_multi link makes sense

> 
> >
> > > You just declare SEC("kprobe.wrap/...") (or whatever the name,
> > > something to designate that it's both entry and exit probe) as one
> > > program and in the code there would be some way to determine whether
> > > we are in entry mode or exit mode (helper or field in the custom
> > > context type, the latter being faster and more usable, but it's
> > > probably not critical).
> >
> > hum, so the single program would be for both entry and exit probe,
> > I'll need to check how bad it'd be for us, but it'd probably mean
> > just one extra tail call, so it's likely ok
> 
> I guess, I don't know what you are doing there :) I'd recommend
> looking at utilizing BPF global subprogs instead of tail calls, if
> your kernel allows for that, as that's a saner way to scale BPF
> verification.

ok, we should probably do that.. given this enhancement will be
available on latest kernel anyway, we could use global subprogs
as well

the related bpftrace might be bit more challenging.. will have to
generate program calling entry or return program now, but seems
doable of course

> 
> >
> > >
> > > Another frequently requested feature and a very common use case is to
> > > measure duration of the function, so if we have a custom type, we can
> > > have a field to record entry timestamp and let user calculate
> >
> > you mean bpf program context right?
> 
> Yes, when I was writing that I was imagining a new context type with new fields.
> 
> But thinking about this a bit more, I like a slightly different
> approach now. Instead of having a new context type, kernel capturing
> timestamps, etc, what if we introduce a concept of "a session". This
> paired kprobe+kretprobe (and same for uprobe+uretprobe) share a common
> session. So, say, if we are tracing kernel function abc() and we have
> our kwrapper (let's call it that for now, kwrapper and uwrapper?)
> program attached to abc, we have something like this:
> 
> 1) abc() is called
> 2) we intercept it at its entry, initialize "session"
> 3) call our kwrapper program in entry mode
> 4) abc() proceeds, until return
> 5) our kwrapper program is called in exit mode
> 6) at this point the session has ended
> 
> If abc() is called again or in parallel on another CPU, each such
> abc() invocation (with kwrapper prog) has its own session state.
> 
> Now, it all sounds scary, but a session could be really just a 8 byte
> value that has to last for the duration of a single kprobe+kretprobe
> execution.
> 
> Imagine we introduce just one new kfunc that would be usable from
> kwrapper and uwrapper programs:
> 
> u64 *bpf_get_session_cookie(void);
> 
> It returns a pointer to an 8-byte slot which initially is set to zero
> by kernel (before kprobe/entry part). And then the program can put
> anything into it. Either from entry (kprobe/uprobe) or exit
> (kretprobe/uretprobe) executions. Whatever value was stored in entry
> mode will still be there in exit mode as well.
> 
> This gives a lot of flexibility without requiring the kernel to do
> anything expensive.
> 
> E.g., it's easy to detect if kwrapper program is an entry call or exit call:
> 
> u64 *cookie = bpf_get_session_cookie();
> if (*cookie == 0) {
>   /* entry mode */
>   *cookie = 1;
> } else {
>   /* exit mode */
> }
> 
> Or if we wanted to measure function duration/latency:
> 
> u64 *cookie = bpf_get_session_cookie();
> if (*cookie == 0) {
>   *cookie = bpf_get_ktime_ns();
> } else {
>   u64 duration = bpf_get_ktime_ns() - *cookie;
>   /* output duration somehow */
> }
> 
> Yes, I realize special-casing zero might be a bit inconvenient, but I
> think simplicity trumps a potential for zero to be a valid value (and
> there are always ways to work around zero as a meaningful value).
> 
> Now, in more complicated cases 8 bytes of temporary session state
> isn't enough, just like BPF cookie being 8 byte (read-only) value
> might not be enough. But the solution is the same as with the BPF
> cookie. You just use those 8 bytes as a key into ARRAY/HASHMAP/whatnot
> storage. It's simple and fast enough for pretty much any case.

I was recently asked for a way to have function arguments available
in the return kprobe as it is in fexit programs (which was not an
option to use, because we don't have fast multi attach for it)

using the hash map to store arguments and storing its key in the
session data might be solution for this

> 
> 
> Now, how do we implement this storage session? I recently looked into
> uprobe/uretprobe implementation, and I know that for uretprobes there
> is a dynamically allocated uretprobe_instance (or whatever the name)
> allocated for each runtime invocation of uretprobe. We can use that.
> We'll need to allocate this instance a bit earlier, before uprobe part
> of uwrapper BPF program has to be executed, initialize session cookie
> to zero, and store pointer to this memory in bpf_run_ctx (just like we
> do for BPF cookie).

I think you refer to the return_instance and it seems like we could use it,
I'll check on that

> 
> I bet there is something similar in the kretprobe case, where we can
> carve out 8 bytes and pass it to both entry and exit parts of kwrapper
> program.

for kprobes.. both kprobe and kprobe_multi/fprobe use rethook to invoke
return probes, so I guess we could use it and store that shared data
in there

btw Masami is in process of removing rethook from kprobe_multi/fprobe,
as part of migrating fprobe on top of ftrace [0]

but instead the rethook I think there'll be some sort of shadow stack/data
area accessible from both entry and return probes, that we could use

jirka


[0] https://lore.kernel.org/bpf/170723204881.502590.11906735097521170661.stgit@devnote2/

> 
> So anyways, might need some internal refactoring, but seems very
> doable and will give a lot of power and flexibility for tracing use
> cases.
> 
> It would be cool to also have a similar fentry/fexit combo with "BPF
> session cookie", but that's something we can think about separately,
> probably.
> 
> 
> >
> > > duration, if necessary. Without this users have to pay a bunch of
> > > extra overhead to record timestamp and put it into hashmap (keyed by
> > > thread id) or thread local storage (even if they have no other use for
> > > thread local storage).
> >
> > sounds good
> >
> > >
> > > Also, consider that a similar concept is applicable to uprobes and we
> > > should do that as well, in similar fashion. And the above approach
> > > works for both kprobe/kretprobe and uprobe/uretprobe cases, because
> > > they have the same pt_regs data structure as a context (even if for
> > > exit probes most of the values of pt_regs are not that meaningful).
> >
> > ok, that seems useful for uprobes as well
> 
> yes, absolutely
> 
> >
> > btw I have another unrelated change in stash that that let you choose
> > the bpf_prog_active or prog->active re-entry check before program is
> > executed in krobe_multi link.. I also added extra flag, and it still
> > seems like good idea to me, thoughts? ;-)
> >
> 
> no specific opinion on this from my side, I guess
> 
> > >
> > > So anyways, great feature, but let's discuss end-to-end usability
> > > story before we finalize the implementation?
> >
> > yep, it does say RFC in the subject ;-)
> >
> > >
> > >
> 
> [...]

Re: [PATCH RFC bpf-next 0/4] bpf: Add support to attach return prog in kprobe multi

[Andrii Nakryiko]

On Tue, Feb 13, 2024 at 4:09 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Mon, Feb 12, 2024 at 08:06:06PM -0800, Andrii Nakryiko wrote:
>
> SNIP
>
> > > > But the way you implement it with extra flag and extra fd parameter
> > > > makes it harder to have a nice high-level support in libbpf (and
> > > > presumably other BPF loader libraries) for this.
> > > >
> > > > When I was thinking about doing something like this, I was considering
> > > > adding a new program type, actually. That way it's possible to define
> > > > this "let's skip return probe" protocol without backwards
> > > > compatibility concerns. It's easier to use it declaratively in libbpf.
> > >
> > > ok, that seems cleaner.. but we need to use current kprobe programs,
> > > so not sure at the moment how would that fit in.. did you mean new
> > > link type?
> >
> > It's kind of a less important detail, actually. New program type would
> > allow us to have an entirely different context type, but I think we
> > can make do with the existing kprobe program type. We can have a
> > separate attach_type and link type, just like multi-kprobe and
> > multi-uprobe are still kprobe programs.
>
> ok, having new attach type on top of kprobe_multi link makes sense
>
> >
> > >
> > > > You just declare SEC("kprobe.wrap/...") (or whatever the name,
> > > > something to designate that it's both entry and exit probe) as one
> > > > program and in the code there would be some way to determine whether
> > > > we are in entry mode or exit mode (helper or field in the custom
> > > > context type, the latter being faster and more usable, but it's
> > > > probably not critical).
> > >
> > > hum, so the single program would be for both entry and exit probe,
> > > I'll need to check how bad it'd be for us, but it'd probably mean
> > > just one extra tail call, so it's likely ok
> >
> > I guess, I don't know what you are doing there :) I'd recommend
> > looking at utilizing BPF global subprogs instead of tail calls, if
> > your kernel allows for that, as that's a saner way to scale BPF
> > verification.
>
> ok, we should probably do that.. given this enhancement will be
> available on latest kernel anyway, we could use global subprogs
> as well
>
> the related bpftrace might be bit more challenging.. will have to
> generate program calling entry or return program now, but seems
> doable of course

So you want users to still have separate kprobe and kretprobe in
bpftrace, but combine them into this kwrapper transparently? It does
seem doable, but hopefully we'll be able to write kwrapper programs in
bpftrace directly as well.

>
> >
> > >
> > > >
> > > > Another frequently requested feature and a very common use case is to
> > > > measure duration of the function, so if we have a custom type, we can
> > > > have a field to record entry timestamp and let user calculate
> > >
> > > you mean bpf program context right?
> >
> > Yes, when I was writing that I was imagining a new context type with new fields.
> >
> > But thinking about this a bit more, I like a slightly different
> > approach now. Instead of having a new context type, kernel capturing
> > timestamps, etc, what if we introduce a concept of "a session". This
> > paired kprobe+kretprobe (and same for uprobe+uretprobe) share a common
> > session. So, say, if we are tracing kernel function abc() and we have
> > our kwrapper (let's call it that for now, kwrapper and uwrapper?)
> > program attached to abc, we have something like this:
> >
> > 1) abc() is called
> > 2) we intercept it at its entry, initialize "session"
> > 3) call our kwrapper program in entry mode
> > 4) abc() proceeds, until return
> > 5) our kwrapper program is called in exit mode
> > 6) at this point the session has ended
> >
> > If abc() is called again or in parallel on another CPU, each such
> > abc() invocation (with kwrapper prog) has its own session state.
> >
> > Now, it all sounds scary, but a session could be really just a 8 byte
> > value that has to last for the duration of a single kprobe+kretprobe
> > execution.
> >
> > Imagine we introduce just one new kfunc that would be usable from
> > kwrapper and uwrapper programs:
> >
> > u64 *bpf_get_session_cookie(void);
> >
> > It returns a pointer to an 8-byte slot which initially is set to zero
> > by kernel (before kprobe/entry part). And then the program can put
> > anything into it. Either from entry (kprobe/uprobe) or exit
> > (kretprobe/uretprobe) executions. Whatever value was stored in entry
> > mode will still be there in exit mode as well.
> >
> > This gives a lot of flexibility without requiring the kernel to do
> > anything expensive.
> >
> > E.g., it's easy to detect if kwrapper program is an entry call or exit call:
> >
> > u64 *cookie = bpf_get_session_cookie();
> > if (*cookie == 0) {
> >   /* entry mode */
> >   *cookie = 1;
> > } else {
> >   /* exit mode */
> > }
> >
> > Or if we wanted to measure function duration/latency:
> >
> > u64 *cookie = bpf_get_session_cookie();
> > if (*cookie == 0) {
> >   *cookie = bpf_get_ktime_ns();
> > } else {
> >   u64 duration = bpf_get_ktime_ns() - *cookie;
> >   /* output duration somehow */
> > }
> >
> > Yes, I realize special-casing zero might be a bit inconvenient, but I
> > think simplicity trumps a potential for zero to be a valid value (and
> > there are always ways to work around zero as a meaningful value).
> >
> > Now, in more complicated cases 8 bytes of temporary session state
> > isn't enough, just like BPF cookie being 8 byte (read-only) value
> > might not be enough. But the solution is the same as with the BPF
> > cookie. You just use those 8 bytes as a key into ARRAY/HASHMAP/whatnot
> > storage. It's simple and fast enough for pretty much any case.
>
> I was recently asked for a way to have function arguments available
> in the return kprobe as it is in fexit programs (which was not an
> option to use, because we don't have fast multi attach for it)
>
> using the hash map to store arguments and storing its key in the
> session data might be solution for this

if you are ok using hashmap keyed by tid, you can do it today without
any kernel changes. With session cookie you'll be able to utilize
faster ARRAY map (by building a simple ID allocator to get a free slot
in ARRAY map).

>
> >
> >
> > Now, how do we implement this storage session? I recently looked into
> > uprobe/uretprobe implementation, and I know that for uretprobes there
> > is a dynamically allocated uretprobe_instance (or whatever the name)
> > allocated for each runtime invocation of uretprobe. We can use that.
> > We'll need to allocate this instance a bit earlier, before uprobe part
> > of uwrapper BPF program has to be executed, initialize session cookie
> > to zero, and store pointer to this memory in bpf_run_ctx (just like we
> > do for BPF cookie).
>
> I think you refer to the return_instance and it seems like we could use it,
> I'll check on that
>

yep, probably

> >
> > I bet there is something similar in the kretprobe case, where we can
> > carve out 8 bytes and pass it to both entry and exit parts of kwrapper
> > program.
>
> for kprobes.. both kprobe and kprobe_multi/fprobe use rethook to invoke
> return probes, so I guess we could use it and store that shared data
> in there
>
> btw Masami is in process of removing rethook from kprobe_multi/fprobe,
> as part of migrating fprobe on top of ftrace [0]
>
> but instead the rethook I think there'll be some sort of shadow stack/data
> area accessible from both entry and return probes, that we could use

ok, cool. We also need to be careful to not share session cookie
between unrelated programs. E.g., if two independent kwrapper programs
are attached to the same function, they should each have their own
cookie. Otherwise it's not clear how to build anything reliable on top
of that, tbh. This might be a problem, though, right?

>
> jirka
>
>
> [0] https://lore.kernel.org/bpf/170723204881.502590.11906735097521170661.stgit@devnote2/
>
> >
> > So anyways, might need some internal refactoring, but seems very
> > doable and will give a lot of power and flexibility for tracing use
> > cases.
> >
> > It would be cool to also have a similar fentry/fexit combo with "BPF
> > session cookie", but that's something we can think about separately,
> > probably.
> >
> >
> > >
> > > > duration, if necessary. Without this users have to pay a bunch of
> > > > extra overhead to record timestamp and put it into hashmap (keyed by
> > > > thread id) or thread local storage (even if they have no other use for
> > > > thread local storage).
> > >
> > > sounds good
> > >
> > > >
> > > > Also, consider that a similar concept is applicable to uprobes and we
> > > > should do that as well, in similar fashion. And the above approach
> > > > works for both kprobe/kretprobe and uprobe/uretprobe cases, because
> > > > they have the same pt_regs data structure as a context (even if for
> > > > exit probes most of the values of pt_regs are not that meaningful).
> > >
> > > ok, that seems useful for uprobes as well
> >
> > yes, absolutely
> >
> > >
> > > btw I have another unrelated change in stash that that let you choose
> > > the bpf_prog_active or prog->active re-entry check before program is
> > > executed in krobe_multi link.. I also added extra flag, and it still
> > > seems like good idea to me, thoughts? ;-)
> > >
> >
> > no specific opinion on this from my side, I guess
> >
> > > >
> > > > So anyways, great feature, but let's discuss end-to-end usability
> > > > story before we finalize the implementation?
> > >
> > > yep, it does say RFC in the subject ;-)
> > >
> > > >
> > > >
> >
> > [...]

Re: [PATCH RFC bpf-next 0/4] bpf: Add support to attach return prog in kprobe multi

[Jiri Olsa]

On Tue, Feb 13, 2024 at 10:20:46AM -0800, Andrii Nakryiko wrote:
> On Tue, Feb 13, 2024 at 4:09 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Mon, Feb 12, 2024 at 08:06:06PM -0800, Andrii Nakryiko wrote:
> >
> > SNIP
> >
> > > > > But the way you implement it with extra flag and extra fd parameter
> > > > > makes it harder to have a nice high-level support in libbpf (and
> > > > > presumably other BPF loader libraries) for this.
> > > > >
> > > > > When I was thinking about doing something like this, I was considering
> > > > > adding a new program type, actually. That way it's possible to define
> > > > > this "let's skip return probe" protocol without backwards
> > > > > compatibility concerns. It's easier to use it declaratively in libbpf.
> > > >
> > > > ok, that seems cleaner.. but we need to use current kprobe programs,
> > > > so not sure at the moment how would that fit in.. did you mean new
> > > > link type?
> > >
> > > It's kind of a less important detail, actually. New program type would
> > > allow us to have an entirely different context type, but I think we
> > > can make do with the existing kprobe program type. We can have a
> > > separate attach_type and link type, just like multi-kprobe and
> > > multi-uprobe are still kprobe programs.
> >
> > ok, having new attach type on top of kprobe_multi link makes sense
> >
> > >
> > > >
> > > > > You just declare SEC("kprobe.wrap/...") (or whatever the name,
> > > > > something to designate that it's both entry and exit probe) as one
> > > > > program and in the code there would be some way to determine whether
> > > > > we are in entry mode or exit mode (helper or field in the custom
> > > > > context type, the latter being faster and more usable, but it's
> > > > > probably not critical).
> > > >
> > > > hum, so the single program would be for both entry and exit probe,
> > > > I'll need to check how bad it'd be for us, but it'd probably mean
> > > > just one extra tail call, so it's likely ok
> > >
> > > I guess, I don't know what you are doing there :) I'd recommend
> > > looking at utilizing BPF global subprogs instead of tail calls, if
> > > your kernel allows for that, as that's a saner way to scale BPF
> > > verification.
> >
> > ok, we should probably do that.. given this enhancement will be
> > available on latest kernel anyway, we could use global subprogs
> > as well
> >
> > the related bpftrace might be bit more challenging.. will have to
> > generate program calling entry or return program now, but seems
> > doable of course
> 
> So you want users to still have separate kprobe and kretprobe in
> bpftrace, but combine them into this kwrapper transparently? It does

no I meant I'd need to generate the wrapper program for the new
interface.. which is extra compared to current bpftrace changes

> seem doable, but hopefully we'll be able to write kwrapper programs in
> bpftrace directly as well.

yes, it should be fine

SNIP

> > >
> > > Yes, I realize special-casing zero might be a bit inconvenient, but I
> > > think simplicity trumps a potential for zero to be a valid value (and
> > > there are always ways to work around zero as a meaningful value).
> > >
> > > Now, in more complicated cases 8 bytes of temporary session state
> > > isn't enough, just like BPF cookie being 8 byte (read-only) value
> > > might not be enough. But the solution is the same as with the BPF
> > > cookie. You just use those 8 bytes as a key into ARRAY/HASHMAP/whatnot
> > > storage. It's simple and fast enough for pretty much any case.
> >
> > I was recently asked for a way to have function arguments available
> > in the return kprobe as it is in fexit programs (which was not an
> > option to use, because we don't have fast multi attach for it)
> >
> > using the hash map to store arguments and storing its key in the
> > session data might be solution for this
> 
> if you are ok using hashmap keyed by tid, you can do it today without
> any kernel changes. With session cookie you'll be able to utilize
> faster ARRAY map (by building a simple ID allocator to get a free slot
> in ARRAY map).

ok

SNIP

> > > I bet there is something similar in the kretprobe case, where we can
> > > carve out 8 bytes and pass it to both entry and exit parts of kwrapper
> > > program.
> >
> > for kprobes.. both kprobe and kprobe_multi/fprobe use rethook to invoke
> > return probes, so I guess we could use it and store that shared data
> > in there
> >
> > btw Masami is in process of removing rethook from kprobe_multi/fprobe,
> > as part of migrating fprobe on top of ftrace [0]
> >
> > but instead the rethook I think there'll be some sort of shadow stack/data
> > area accessible from both entry and return probes, that we could use
> 
> ok, cool. We also need to be careful to not share session cookie
> between unrelated programs. E.g., if two independent kwrapper programs
> are attached to the same function, they should each have their own
> cookie. Otherwise it's not clear how to build anything reliable on top
> of that, tbh. This might be a problem, though, right?

IIRC it's tracer specific data, the shadow stack data should be unique
for tracer and its called function, but I'll double check on that

jirka

Re: [PATCH RFC bpf-next 0/4] bpf: Add support to attach return prog in kprobe multi

[Jiri Olsa]

On Tue, Feb 13, 2024 at 10:09:09PM +0100, Jiri Olsa wrote:
> On Tue, Feb 13, 2024 at 10:20:46AM -0800, Andrii Nakryiko wrote:
> > On Tue, Feb 13, 2024 at 4:09 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > >
> > > On Mon, Feb 12, 2024 at 08:06:06PM -0800, Andrii Nakryiko wrote:
> > >
> > > SNIP
> > >
> > > > > > But the way you implement it with extra flag and extra fd parameter
> > > > > > makes it harder to have a nice high-level support in libbpf (and
> > > > > > presumably other BPF loader libraries) for this.
> > > > > >
> > > > > > When I was thinking about doing something like this, I was considering
> > > > > > adding a new program type, actually. That way it's possible to define
> > > > > > this "let's skip return probe" protocol without backwards
> > > > > > compatibility concerns. It's easier to use it declaratively in libbpf.
> > > > >
> > > > > ok, that seems cleaner.. but we need to use current kprobe programs,
> > > > > so not sure at the moment how would that fit in.. did you mean new
> > > > > link type?
> > > >
> > > > It's kind of a less important detail, actually. New program type would
> > > > allow us to have an entirely different context type, but I think we
> > > > can make do with the existing kprobe program type. We can have a
> > > > separate attach_type and link type, just like multi-kprobe and
> > > > multi-uprobe are still kprobe programs.
> > >
> > > ok, having new attach type on top of kprobe_multi link makes sense
> > >
> > > >
> > > > >
> > > > > > You just declare SEC("kprobe.wrap/...") (or whatever the name,
> > > > > > something to designate that it's both entry and exit probe) as one
> > > > > > program and in the code there would be some way to determine whether
> > > > > > we are in entry mode or exit mode (helper or field in the custom
> > > > > > context type, the latter being faster and more usable, but it's
> > > > > > probably not critical).
> > > > >
> > > > > hum, so the single program would be for both entry and exit probe,
> > > > > I'll need to check how bad it'd be for us, but it'd probably mean
> > > > > just one extra tail call, so it's likely ok
> > > >
> > > > I guess, I don't know what you are doing there :) I'd recommend
> > > > looking at utilizing BPF global subprogs instead of tail calls, if
> > > > your kernel allows for that, as that's a saner way to scale BPF
> > > > verification.
> > >
> > > ok, we should probably do that.. given this enhancement will be
> > > available on latest kernel anyway, we could use global subprogs
> > > as well
> > >
> > > the related bpftrace might be bit more challenging.. will have to
> > > generate program calling entry or return program now, but seems
> > > doable of course
> > 
> > So you want users to still have separate kprobe and kretprobe in
> > bpftrace, but combine them into this kwrapper transparently? It does
> 
> no I meant I'd need to generate the wrapper program for the new
> interface.. which is extra compared to current bpftrace changes
> 
> > seem doable, but hopefully we'll be able to write kwrapper programs in
> > bpftrace directly as well.
> 
> yes, it should be fine
> 
> SNIP
> 
> > > >
> > > > Yes, I realize special-casing zero might be a bit inconvenient, but I
> > > > think simplicity trumps a potential for zero to be a valid value (and
> > > > there are always ways to work around zero as a meaningful value).
> > > >
> > > > Now, in more complicated cases 8 bytes of temporary session state
> > > > isn't enough, just like BPF cookie being 8 byte (read-only) value
> > > > might not be enough. But the solution is the same as with the BPF
> > > > cookie. You just use those 8 bytes as a key into ARRAY/HASHMAP/whatnot
> > > > storage. It's simple and fast enough for pretty much any case.
> > >
> > > I was recently asked for a way to have function arguments available
> > > in the return kprobe as it is in fexit programs (which was not an
> > > option to use, because we don't have fast multi attach for it)
> > >
> > > using the hash map to store arguments and storing its key in the
> > > session data might be solution for this
> > 
> > if you are ok using hashmap keyed by tid, you can do it today without
> > any kernel changes. With session cookie you'll be able to utilize
> > faster ARRAY map (by building a simple ID allocator to get a free slot
> > in ARRAY map).
> 
> ok
> 
> SNIP
> 
> > > > I bet there is something similar in the kretprobe case, where we can
> > > > carve out 8 bytes and pass it to both entry and exit parts of kwrapper
> > > > program.
> > >
> > > for kprobes.. both kprobe and kprobe_multi/fprobe use rethook to invoke
> > > return probes, so I guess we could use it and store that shared data
> > > in there
> > >
> > > btw Masami is in process of removing rethook from kprobe_multi/fprobe,
> > > as part of migrating fprobe on top of ftrace [0]
> > >
> > > but instead the rethook I think there'll be some sort of shadow stack/data
> > > area accessible from both entry and return probes, that we could use
> > 
> > ok, cool. We also need to be careful to not share session cookie
> > between unrelated programs. E.g., if two independent kwrapper programs
> > are attached to the same function, they should each have their own
> > cookie. Otherwise it's not clear how to build anything reliable on top
> > of that, tbh. This might be a problem, though, right?
> 
> IIRC it's tracer specific data, the shadow stack data should be unique
> for tracer and its called function, but I'll double check on that

Masami,
we recently discussed the possibility to store data between entry/return probe,
IIUC your current patchset [0] allows that, but it seems to be shared across all
the tracers for the given function (__ftrace_return_to_handler).. is the plan to
make the shadow stack per tracer and function? /cc Steven

thanks,
jirka


[0] https://lore.kernel.org/bpf/170723204881.502590.11906735097521170661.stgit@devnote2/

Re: [PATCH RFC bpf-next 0/4] bpf: Add support to attach return prog in kprobe multi

[Steven Rostedt]

On Wed, 14 Feb 2024 21:55:03 +0100
Jiri Olsa <olsajiri@gmail.com> wrote:


> Masami,
> we recently discussed the possibility to store data between entry/return probe,
> IIUC your current patchset [0] allows that, but it seems to be shared across all
> the tracers for the given function (__ftrace_return_to_handler).. is the plan to
> make the shadow stack per tracer and function? /cc Steven

The shadow stack is per task, but it saves unique data per tracer per function.

It allows up to 16 different tracers attached to function graph tracing at
a time, as there is a limited shadow stack size, which all the attached
tracers use. Now you can create your own shadow stack per user (bpf
program?) and have a single registered function graph user that will demux
the data coming in and out of the function.

-- Steve

Re: [PATCH RFC bpf-next 0/4] bpf: Add support to attach return prog in kprobe multi

[Jiri Olsa]

On Thu, Feb 15, 2024 at 11:27:22AM -0500, Steven Rostedt wrote:
> On Wed, 14 Feb 2024 21:55:03 +0100
> Jiri Olsa <olsajiri@gmail.com> wrote:
> 
> 
> > Masami,
> > we recently discussed the possibility to store data between entry/return probe,
> > IIUC your current patchset [0] allows that, but it seems to be shared across all
> > the tracers for the given function (__ftrace_return_to_handler).. is the plan to
> > make the shadow stack per tracer and function? /cc Steven
> 
> The shadow stack is per task, but it saves unique data per tracer per function.
> 
> It allows up to 16 different tracers attached to function graph tracing at
> a time, as there is a limited shadow stack size, which all the attached
> tracers use. Now you can create your own shadow stack per user (bpf
> program?) and have a single registered function graph user that will demux
> the data coming in and out of the function.

ok, I guess that's fgraph_reserve_data/fgraph_retrieve_data api

we'd like to share data between bpf programs executed from entry
and exit handlers, that should do it

thanks,
jirka

Re: [PATCH RFC bpf-next 0/4] bpf: Add support to attach return prog in kprobe multi

[Viktor Malik]

On 2/13/24 22:09, Jiri Olsa wrote:
> On Tue, Feb 13, 2024 at 10:20:46AM -0800, Andrii Nakryiko wrote:
>> On Tue, Feb 13, 2024 at 4:09 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>>>
>>> On Mon, Feb 12, 2024 at 08:06:06PM -0800, Andrii Nakryiko wrote:
>>>
>>> SNIP
>>>
>>>>>> But the way you implement it with extra flag and extra fd parameter
>>>>>> makes it harder to have a nice high-level support in libbpf (and
>>>>>> presumably other BPF loader libraries) for this.
>>>>>>
>>>>>> When I was thinking about doing something like this, I was considering
>>>>>> adding a new program type, actually. That way it's possible to define
>>>>>> this "let's skip return probe" protocol without backwards
>>>>>> compatibility concerns. It's easier to use it declaratively in libbpf.
>>>>>
>>>>> ok, that seems cleaner.. but we need to use current kprobe programs,
>>>>> so not sure at the moment how would that fit in.. did you mean new
>>>>> link type?
>>>>
>>>> It's kind of a less important detail, actually. New program type would
>>>> allow us to have an entirely different context type, but I think we
>>>> can make do with the existing kprobe program type. We can have a
>>>> separate attach_type and link type, just like multi-kprobe and
>>>> multi-uprobe are still kprobe programs.
>>>
>>> ok, having new attach type on top of kprobe_multi link makes sense
>>>
>>>>
>>>>>
>>>>>> You just declare SEC("kprobe.wrap/...") (or whatever the name,
>>>>>> something to designate that it's both entry and exit probe) as one
>>>>>> program and in the code there would be some way to determine whether
>>>>>> we are in entry mode or exit mode (helper or field in the custom
>>>>>> context type, the latter being faster and more usable, but it's
>>>>>> probably not critical).
>>>>>
>>>>> hum, so the single program would be for both entry and exit probe,
>>>>> I'll need to check how bad it'd be for us, but it'd probably mean
>>>>> just one extra tail call, so it's likely ok
>>>>
>>>> I guess, I don't know what you are doing there :) I'd recommend
>>>> looking at utilizing BPF global subprogs instead of tail calls, if
>>>> your kernel allows for that, as that's a saner way to scale BPF
>>>> verification.
>>>
>>> ok, we should probably do that.. given this enhancement will be
>>> available on latest kernel anyway, we could use global subprogs
>>> as well
>>>
>>> the related bpftrace might be bit more challenging.. will have to
>>> generate program calling entry or return program now, but seems
>>> doable of course
>>
>> So you want users to still have separate kprobe and kretprobe in
>> bpftrace, but combine them into this kwrapper transparently? It does
> 
> no I meant I'd need to generate the wrapper program for the new
> interface.. which is extra compared to current bpftrace changes

If you end up introducing this new kwrapper program type in libbpf, I
think that it'll make sense to have something similar in bpftrace, too.
Allowing users to write separate kprobe and kretprobe programs and
transparently combining them into kwrapper doesn't seem to bring much
value to me.

Jirka, if you need help with implementing bpftrace support for this, let
me know. I'm very interested in having this capability there.

Viktor

> 
>> seem doable, but hopefully we'll be able to write kwrapper programs in
>> bpftrace directly as well.
> 
> yes, it should be fine
> 
> SNIP
> 
>>>>
>>>> Yes, I realize special-casing zero might be a bit inconvenient, but I
>>>> think simplicity trumps a potential for zero to be a valid value (and
>>>> there are always ways to work around zero as a meaningful value).
>>>>
>>>> Now, in more complicated cases 8 bytes of temporary session state
>>>> isn't enough, just like BPF cookie being 8 byte (read-only) value
>>>> might not be enough. But the solution is the same as with the BPF
>>>> cookie. You just use those 8 bytes as a key into ARRAY/HASHMAP/whatnot
>>>> storage. It's simple and fast enough for pretty much any case.
>>>
>>> I was recently asked for a way to have function arguments available
>>> in the return kprobe as it is in fexit programs (which was not an
>>> option to use, because we don't have fast multi attach for it)
>>>
>>> using the hash map to store arguments and storing its key in the
>>> session data might be solution for this
>>
>> if you are ok using hashmap keyed by tid, you can do it today without
>> any kernel changes. With session cookie you'll be able to utilize
>> faster ARRAY map (by building a simple ID allocator to get a free slot
>> in ARRAY map).
> 
> ok
> 
> SNIP
> 
>>>> I bet there is something similar in the kretprobe case, where we can
>>>> carve out 8 bytes and pass it to both entry and exit parts of kwrapper
>>>> program.
>>>
>>> for kprobes.. both kprobe and kprobe_multi/fprobe use rethook to invoke
>>> return probes, so I guess we could use it and store that shared data
>>> in there
>>>
>>> btw Masami is in process of removing rethook from kprobe_multi/fprobe,
>>> as part of migrating fprobe on top of ftrace [0]
>>>
>>> but instead the rethook I think there'll be some sort of shadow stack/data
>>> area accessible from both entry and return probes, that we could use
>>
>> ok, cool. We also need to be careful to not share session cookie
>> between unrelated programs. E.g., if two independent kwrapper programs
>> are attached to the same function, they should each have their own
>> cookie. Otherwise it's not clear how to build anything reliable on top
>> of that, tbh. This might be a problem, though, right?
> 
> IIRC it's tracer specific data, the shadow stack data should be unique
> for tracer and its called function, but I'll double check on that
> 
> jirka
> 

Re: [PATCH RFC bpf-next 0/4] bpf: Add support to attach return prog in kprobe multi

[Jiri Olsa]

On Mon, Feb 19, 2024 at 12:20:08PM +0100, Viktor Malik wrote:
> On 2/13/24 22:09, Jiri Olsa wrote:
> > On Tue, Feb 13, 2024 at 10:20:46AM -0800, Andrii Nakryiko wrote:
> >> On Tue, Feb 13, 2024 at 4:09 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >>>
> >>> On Mon, Feb 12, 2024 at 08:06:06PM -0800, Andrii Nakryiko wrote:
> >>>
> >>> SNIP
> >>>
> >>>>>> But the way you implement it with extra flag and extra fd parameter
> >>>>>> makes it harder to have a nice high-level support in libbpf (and
> >>>>>> presumably other BPF loader libraries) for this.
> >>>>>>
> >>>>>> When I was thinking about doing something like this, I was considering
> >>>>>> adding a new program type, actually. That way it's possible to define
> >>>>>> this "let's skip return probe" protocol without backwards
> >>>>>> compatibility concerns. It's easier to use it declaratively in libbpf.
> >>>>>
> >>>>> ok, that seems cleaner.. but we need to use current kprobe programs,
> >>>>> so not sure at the moment how would that fit in.. did you mean new
> >>>>> link type?
> >>>>
> >>>> It's kind of a less important detail, actually. New program type would
> >>>> allow us to have an entirely different context type, but I think we
> >>>> can make do with the existing kprobe program type. We can have a
> >>>> separate attach_type and link type, just like multi-kprobe and
> >>>> multi-uprobe are still kprobe programs.
> >>>
> >>> ok, having new attach type on top of kprobe_multi link makes sense
> >>>
> >>>>
> >>>>>
> >>>>>> You just declare SEC("kprobe.wrap/...") (or whatever the name,
> >>>>>> something to designate that it's both entry and exit probe) as one
> >>>>>> program and in the code there would be some way to determine whether
> >>>>>> we are in entry mode or exit mode (helper or field in the custom
> >>>>>> context type, the latter being faster and more usable, but it's
> >>>>>> probably not critical).
> >>>>>
> >>>>> hum, so the single program would be for both entry and exit probe,
> >>>>> I'll need to check how bad it'd be for us, but it'd probably mean
> >>>>> just one extra tail call, so it's likely ok
> >>>>
> >>>> I guess, I don't know what you are doing there :) I'd recommend
> >>>> looking at utilizing BPF global subprogs instead of tail calls, if
> >>>> your kernel allows for that, as that's a saner way to scale BPF
> >>>> verification.
> >>>
> >>> ok, we should probably do that.. given this enhancement will be
> >>> available on latest kernel anyway, we could use global subprogs
> >>> as well
> >>>
> >>> the related bpftrace might be bit more challenging.. will have to
> >>> generate program calling entry or return program now, but seems
> >>> doable of course
> >>
> >> So you want users to still have separate kprobe and kretprobe in
> >> bpftrace, but combine them into this kwrapper transparently? It does
> > 
> > no I meant I'd need to generate the wrapper program for the new
> > interface.. which is extra compared to current bpftrace changes
> 
> If you end up introducing this new kwrapper program type in libbpf, I
> think that it'll make sense to have something similar in bpftrace, too.
> Allowing users to write separate kprobe and kretprobe programs and
> transparently combining them into kwrapper doesn't seem to bring much
> value to me.

I kind of liked the idea of not introducing new probe type and silently
making things faster ;-) but having wrapper probe type makes also sense

> 
> Jirka, if you need help with implementing bpftrace support for this, let
> me know. I'm very interested in having this capability there.

that'd be great, I'll send new version of kernel changes soon

thanks,
jirka

Re: [PATCH RFC bpf-next 0/4] bpf: Add support to attach return prog in kprobe multi

[Jiri Olsa]

On Tue, Feb 13, 2024 at 01:09:54PM +0100, Jiri Olsa wrote:
> On Mon, Feb 12, 2024 at 08:06:06PM -0800, Andrii Nakryiko wrote:
> 
> SNIP
> 
> > > > But the way you implement it with extra flag and extra fd parameter
> > > > makes it harder to have a nice high-level support in libbpf (and
> > > > presumably other BPF loader libraries) for this.
> > > >
> > > > When I was thinking about doing something like this, I was considering
> > > > adding a new program type, actually. That way it's possible to define
> > > > this "let's skip return probe" protocol without backwards
> > > > compatibility concerns. It's easier to use it declaratively in libbpf.
> > >
> > > ok, that seems cleaner.. but we need to use current kprobe programs,
> > > so not sure at the moment how would that fit in.. did you mean new
> > > link type?
> > 
> > It's kind of a less important detail, actually. New program type would
> > allow us to have an entirely different context type, but I think we
> > can make do with the existing kprobe program type. We can have a
> > separate attach_type and link type, just like multi-kprobe and
> > multi-uprobe are still kprobe programs.
> 
> ok, having new attach type on top of kprobe_multi link makes sense

hi,
I have further question in here.. ;-)

so I implemented the new behaviour on top of new attach_type, but I keep
thinking that it's the 'same/similar logic' as if I added extra flag to
attr.link_create.kprobe_multi.flags, which results in much simpler change

is following logic breaking backward compatibility in any way?

  - having new flag in attr.link_create.kprobe_multi.flags
  - that force the attach of the bpf program to entry/exit function probes
  - and the kprobe entry program return value now controls invocation
    on the exit probe

so the new flag changes the semantics of the entry program return value,
which seems fine to me, because it depends on the new flag.. do I miss
any other problem with that?

thanks,
jirka

> 
> > 
> > >
> > > > You just declare SEC("kprobe.wrap/...") (or whatever the name,
> > > > something to designate that it's both entry and exit probe) as one
> > > > program and in the code there would be some way to determine whether
> > > > we are in entry mode or exit mode (helper or field in the custom
> > > > context type, the latter being faster and more usable, but it's
> > > > probably not critical).
> > >
> > > hum, so the single program would be for both entry and exit probe,
> > > I'll need to check how bad it'd be for us, but it'd probably mean
> > > just one extra tail call, so it's likely ok
> > 
> > I guess, I don't know what you are doing there :) I'd recommend
> > looking at utilizing BPF global subprogs instead of tail calls, if
> > your kernel allows for that, as that's a saner way to scale BPF
> > verification.
> 
> ok, we should probably do that.. given this enhancement will be
> available on latest kernel anyway, we could use global subprogs
> as well
> 
> the related bpftrace might be bit more challenging.. will have to
> generate program calling entry or return program now, but seems
> doable of course
> 
> > 
> > >
> > > >
> > > > Another frequently requested feature and a very common use case is to
> > > > measure duration of the function, so if we have a custom type, we can
> > > > have a field to record entry timestamp and let user calculate
> > >
> > > you mean bpf program context right?
> > 
> > Yes, when I was writing that I was imagining a new context type with new fields.
> > 
> > But thinking about this a bit more, I like a slightly different
> > approach now. Instead of having a new context type, kernel capturing
> > timestamps, etc, what if we introduce a concept of "a session". This
> > paired kprobe+kretprobe (and same for uprobe+uretprobe) share a common
> > session. So, say, if we are tracing kernel function abc() and we have
> > our kwrapper (let's call it that for now, kwrapper and uwrapper?)
> > program attached to abc, we have something like this:
> > 
> > 1) abc() is called
> > 2) we intercept it at its entry, initialize "session"
> > 3) call our kwrapper program in entry mode
> > 4) abc() proceeds, until return
> > 5) our kwrapper program is called in exit mode
> > 6) at this point the session has ended
> > 
> > If abc() is called again or in parallel on another CPU, each such
> > abc() invocation (with kwrapper prog) has its own session state.
> > 
> > Now, it all sounds scary, but a session could be really just a 8 byte
> > value that has to last for the duration of a single kprobe+kretprobe
> > execution.
> > 
> > Imagine we introduce just one new kfunc that would be usable from
> > kwrapper and uwrapper programs:
> > 
> > u64 *bpf_get_session_cookie(void);
> > 
> > It returns a pointer to an 8-byte slot which initially is set to zero
> > by kernel (before kprobe/entry part). And then the program can put
> > anything into it. Either from entry (kprobe/uprobe) or exit
> > (kretprobe/uretprobe) executions. Whatever value was stored in entry
> > mode will still be there in exit mode as well.
> > 
> > This gives a lot of flexibility without requiring the kernel to do
> > anything expensive.
> > 
> > E.g., it's easy to detect if kwrapper program is an entry call or exit call:
> > 
> > u64 *cookie = bpf_get_session_cookie();
> > if (*cookie == 0) {
> >   /* entry mode */
> >   *cookie = 1;
> > } else {
> >   /* exit mode */
> > }
> > 
> > Or if we wanted to measure function duration/latency:
> > 
> > u64 *cookie = bpf_get_session_cookie();
> > if (*cookie == 0) {
> >   *cookie = bpf_get_ktime_ns();
> > } else {
> >   u64 duration = bpf_get_ktime_ns() - *cookie;
> >   /* output duration somehow */
> > }
> > 
> > Yes, I realize special-casing zero might be a bit inconvenient, but I
> > think simplicity trumps a potential for zero to be a valid value (and
> > there are always ways to work around zero as a meaningful value).
> > 
> > Now, in more complicated cases 8 bytes of temporary session state
> > isn't enough, just like BPF cookie being 8 byte (read-only) value
> > might not be enough. But the solution is the same as with the BPF
> > cookie. You just use those 8 bytes as a key into ARRAY/HASHMAP/whatnot
> > storage. It's simple and fast enough for pretty much any case.
> 
> I was recently asked for a way to have function arguments available
> in the return kprobe as it is in fexit programs (which was not an
> option to use, because we don't have fast multi attach for it)
> 
> using the hash map to store arguments and storing its key in the
> session data might be solution for this
> 
> > 
> > 
> > Now, how do we implement this storage session? I recently looked into
> > uprobe/uretprobe implementation, and I know that for uretprobes there
> > is a dynamically allocated uretprobe_instance (or whatever the name)
> > allocated for each runtime invocation of uretprobe. We can use that.
> > We'll need to allocate this instance a bit earlier, before uprobe part
> > of uwrapper BPF program has to be executed, initialize session cookie
> > to zero, and store pointer to this memory in bpf_run_ctx (just like we
> > do for BPF cookie).
> 
> I think you refer to the return_instance and it seems like we could use it,
> I'll check on that
> 
> > 
> > I bet there is something similar in the kretprobe case, where we can
> > carve out 8 bytes and pass it to both entry and exit parts of kwrapper
> > program.
> 
> for kprobes.. both kprobe and kprobe_multi/fprobe use rethook to invoke
> return probes, so I guess we could use it and store that shared data
> in there
> 
> btw Masami is in process of removing rethook from kprobe_multi/fprobe,
> as part of migrating fprobe on top of ftrace [0]
> 
> but instead the rethook I think there'll be some sort of shadow stack/data
> area accessible from both entry and return probes, that we could use
> 
> jirka
> 
> 
> [0] https://lore.kernel.org/bpf/170723204881.502590.11906735097521170661.stgit@devnote2/
> 
> > 
> > So anyways, might need some internal refactoring, but seems very
> > doable and will give a lot of power and flexibility for tracing use
> > cases.
> > 
> > It would be cool to also have a similar fentry/fexit combo with "BPF
> > session cookie", but that's something we can think about separately,
> > probably.
> > 
> > 
> > >
> > > > duration, if necessary. Without this users have to pay a bunch of
> > > > extra overhead to record timestamp and put it into hashmap (keyed by
> > > > thread id) or thread local storage (even if they have no other use for
> > > > thread local storage).
> > >
> > > sounds good
> > >
> > > >
> > > > Also, consider that a similar concept is applicable to uprobes and we
> > > > should do that as well, in similar fashion. And the above approach
> > > > works for both kprobe/kretprobe and uprobe/uretprobe cases, because
> > > > they have the same pt_regs data structure as a context (even if for
> > > > exit probes most of the values of pt_regs are not that meaningful).
> > >
> > > ok, that seems useful for uprobes as well
> > 
> > yes, absolutely
> > 
> > >
> > > btw I have another unrelated change in stash that that let you choose
> > > the bpf_prog_active or prog->active re-entry check before program is
> > > executed in krobe_multi link.. I also added extra flag, and it still
> > > seems like good idea to me, thoughts? ;-)
> > >
> > 
> > no specific opinion on this from my side, I guess
> > 
> > > >
> > > > So anyways, great feature, but let's discuss end-to-end usability
> > > > story before we finalize the implementation?
> > >
> > > yep, it does say RFC in the subject ;-)
> > >
> > > >
> > > >
> > 
> > [...]

Re: [PATCH RFC bpf-next 0/4] bpf: Add support to attach return prog in kprobe multi

[Andrii Nakryiko]

On Fri, Feb 23, 2024 at 1:32 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Tue, Feb 13, 2024 at 01:09:54PM +0100, Jiri Olsa wrote:
> > On Mon, Feb 12, 2024 at 08:06:06PM -0800, Andrii Nakryiko wrote:
> >
> > SNIP
> >
> > > > > But the way you implement it with extra flag and extra fd parameter
> > > > > makes it harder to have a nice high-level support in libbpf (and
> > > > > presumably other BPF loader libraries) for this.
> > > > >
> > > > > When I was thinking about doing something like this, I was considering
> > > > > adding a new program type, actually. That way it's possible to define
> > > > > this "let's skip return probe" protocol without backwards
> > > > > compatibility concerns. It's easier to use it declaratively in libbpf.
> > > >
> > > > ok, that seems cleaner.. but we need to use current kprobe programs,
> > > > so not sure at the moment how would that fit in.. did you mean new
> > > > link type?
> > >
> > > It's kind of a less important detail, actually. New program type would
> > > allow us to have an entirely different context type, but I think we
> > > can make do with the existing kprobe program type. We can have a
> > > separate attach_type and link type, just like multi-kprobe and
> > > multi-uprobe are still kprobe programs.
> >
> > ok, having new attach type on top of kprobe_multi link makes sense
>
> hi,
> I have further question in here.. ;-)
>
> so I implemented the new behaviour on top of new attach_type, but I keep
> thinking that it's the 'same/similar logic' as if I added extra flag to
> attr.link_create.kprobe_multi.flags, which results in much simpler change
>
> is following logic breaking backward compatibility in any way?

Even if it doesn't break compatibility, isn't new attach_type better
to explicitly show that semantics is completely different (it's two
BPF program executions, before and after, for each specified kprobe),
isn't that different enough? We can also have a different set of
helpers for this kwrapper/uwrapper program type (probably could do it
through flags as well, but attach_type again seems a better fit
here..)?

What makes it harder if you are doing it as a new attach_type?

>
>   - having new flag in attr.link_create.kprobe_multi.flags
>   - that force the attach of the bpf program to entry/exit function probes
>   - and the kprobe entry program return value now controls invocation
>     on the exit probe
>
> so the new flag changes the semantics of the entry program return value,
> which seems fine to me, because it depends on the new flag.. do I miss
> any other problem with that?
>
> thanks,
> jirka
>
> >
> > >
> > > >
> > > > > You just declare SEC("kprobe.wrap/...") (or whatever the name,
> > > > > something to designate that it's both entry and exit probe) as one
> > > > > program and in the code there would be some way to determine whether
> > > > > we are in entry mode or exit mode (helper or field in the custom
> > > > > context type, the latter being faster and more usable, but it's
> > > > > probably not critical).
> > > >
> > > > hum, so the single program would be for both entry and exit probe,
> > > > I'll need to check how bad it'd be for us, but it'd probably mean
> > > > just one extra tail call, so it's likely ok
> > >
> > > I guess, I don't know what you are doing there :) I'd recommend
> > > looking at utilizing BPF global subprogs instead of tail calls, if
> > > your kernel allows for that, as that's a saner way to scale BPF
> > > verification.
> >

[...]

Re: [PATCH RFC bpf-next 0/4] bpf: Add support to attach return prog in kprobe multi

[Andrii Nakryiko]

On Wed, Feb 28, 2024 at 4:43 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Feb 23, 2024 at 1:32 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Tue, Feb 13, 2024 at 01:09:54PM +0100, Jiri Olsa wrote:
> > > On Mon, Feb 12, 2024 at 08:06:06PM -0800, Andrii Nakryiko wrote:
> > >
> > > SNIP
> > >
> > > > > > But the way you implement it with extra flag and extra fd parameter
> > > > > > makes it harder to have a nice high-level support in libbpf (and
> > > > > > presumably other BPF loader libraries) for this.
> > > > > >
> > > > > > When I was thinking about doing something like this, I was considering
> > > > > > adding a new program type, actually. That way it's possible to define
> > > > > > this "let's skip return probe" protocol without backwards
> > > > > > compatibility concerns. It's easier to use it declaratively in libbpf.
> > > > >
> > > > > ok, that seems cleaner.. but we need to use current kprobe programs,
> > > > > so not sure at the moment how would that fit in.. did you mean new
> > > > > link type?
> > > >
> > > > It's kind of a less important detail, actually. New program type would
> > > > allow us to have an entirely different context type, but I think we
> > > > can make do with the existing kprobe program type. We can have a
> > > > separate attach_type and link type, just like multi-kprobe and
> > > > multi-uprobe are still kprobe programs.
> > >
> > > ok, having new attach type on top of kprobe_multi link makes sense
> >
> > hi,
> > I have further question in here.. ;-)
> >
> > so I implemented the new behaviour on top of new attach_type, but I keep
> > thinking that it's the 'same/similar logic' as if I added extra flag to
> > attr.link_create.kprobe_multi.flags, which results in much simpler change
> >
> > is following logic breaking backward compatibility in any way?
>
> Even if it doesn't break compatibility, isn't new attach_type better
> to explicitly show that semantics is completely different (it's two
> BPF program executions, before and after, for each specified kprobe),
> isn't that different enough? We can also have a different set of
> helpers for this kwrapper/uwrapper program type (probably could do it
> through flags as well, but attach_type again seems a better fit
> here..)?
>
> What makes it harder if you are doing it as a new attach_type?

I just looked at the RFC patches you sent, and I guess it looks fine
to me with the flag as well, given kretprobe.multi is also specified
through flags.

>
> >
> >   - having new flag in attr.link_create.kprobe_multi.flags
> >   - that force the attach of the bpf program to entry/exit function probes
> >   - and the kprobe entry program return value now controls invocation
> >     on the exit probe
> >
> > so the new flag changes the semantics of the entry program return value,
> > which seems fine to me, because it depends on the new flag.. do I miss
> > any other problem with that?
> >
> > thanks,
> > jirka
> >
> > >
> > > >
> > > > >
> > > > > > You just declare SEC("kprobe.wrap/...") (or whatever the name,
> > > > > > something to designate that it's both entry and exit probe) as one
> > > > > > program and in the code there would be some way to determine whether
> > > > > > we are in entry mode or exit mode (helper or field in the custom
> > > > > > context type, the latter being faster and more usable, but it's
> > > > > > probably not critical).
> > > > >
> > > > > hum, so the single program would be for both entry and exit probe,
> > > > > I'll need to check how bad it'd be for us, but it'd probably mean
> > > > > just one extra tail call, so it's likely ok
> > > >
> > > > I guess, I don't know what you are doing there :) I'd recommend
> > > > looking at utilizing BPF global subprogs instead of tail calls, if
> > > > your kernel allows for that, as that's a saner way to scale BPF
> > > > verification.
> > >
>
> [...]