[PATCHv2 bpf-next 0/8] bpf: Add kprobe multi link

[PATCHv2 bpf-next 0/8] bpf: Add kprobe multi link

[Jiri Olsa]

hi,
this patchset adds new link type BPF_TRACE_KPROBE_MULTI that attaches
kprobe program through fprobe API [1] instroduced by Masami.

The fprobe API allows to attach probe on multiple functions at once very
fast, because it works on top of ftrace. On the other hand this limits
the probe point to the function entry or return.


With bpftrace support I see following attach speed:

  # perf stat --null -r 5 ./src/bpftrace -e 'kprobe:x* { } i:ms:1 { exit(); } '
  Attaching 2 probes...
  Attaching 3342 functions
  ...

  1.4960 +- 0.0285 seconds time elapsed  ( +-  1.91% )


v2 changes:
  - based on latest fprobe changes [1]
  - renaming the uapi interface to kprobe multi
  - adding support for sort_r to pass user pointer for swap functions
    and using that in cookie support to keep just single functions array
  - moving new link to kernel/trace/bpf_trace.c file
  - using single fprobe callback function for entry and exit
  - using kvzalloc, libbpf_ensure_mem functions
  - adding new k[ret]probe.multi sections instead of using current kprobe
  - used glob_match from test_progs.c, added '?' matching
  - move bpf_get_func_ip verifier inline change to seprate change
  - couple of other minor fixes


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

thanks,
jirka


[1] https://lore.kernel.org/bpf/164458044634.586276.3261555265565111183.stgit@devnote2/
---
Jiri Olsa (10):
      lib/sort: Add priv pointer to swap function
      bpf: Add multi kprobe link
      bpf: Add bpf_get_func_ip kprobe helper for multi kprobe link
      bpf: Add support to inline bpf_get_func_ip helper on x86
      bpf: Add cookie support to programs attached with kprobe multi link
      libbpf: Add libbpf_kallsyms_parse function
      libbpf: Add bpf_link_create support for multi kprobes
      libbpf: Add bpf_program__attach_kprobe_opts support for multi kprobes
      selftest/bpf: Add kprobe_multi attach test
      selftest/bpf: Add kprobe_multi test for bpf_cookie values

 include/linux/bpf_types.h                                   |   1 +
 include/linux/sort.h                                        |   4 +-
 include/linux/trace_events.h                                |   6 ++
 include/linux/types.h                                       |   1 +
 include/uapi/linux/bpf.h                                    |  14 ++++
 kernel/bpf/syscall.c                                        |  26 ++++++--
 kernel/bpf/verifier.c                                       |  21 +++++-
 kernel/trace/bpf_trace.c                                    | 331 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 lib/sort.c                                                  |  42 +++++++++---
 tools/include/uapi/linux/bpf.h                              |  14 ++++
 tools/lib/bpf/bpf.c                                         |   7 ++
 tools/lib/bpf/bpf.h                                         |   9 ++-
 tools/lib/bpf/libbpf.c                                      | 192 +++++++++++++++++++++++++++++++++++++++++++++--------
 tools/lib/bpf/libbpf_internal.h                             |   5 ++
 tools/testing/selftests/bpf/prog_tests/bpf_cookie.c         |  72 ++++++++++++++++++++
 tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c  | 115 ++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/kprobe_multi.c            |  58 ++++++++++++++++
 tools/testing/selftests/bpf/progs/kprobe_multi_bpf_cookie.c |  62 +++++++++++++++++
 18 files changed, 930 insertions(+), 50 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
 create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi.c
 create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_bpf_cookie.c

[PATCH 01/10] lib/sort: Add priv pointer to swap function

[Jiri Olsa]

Adding support to have priv pointer in swap callback function.

Following the initial change on cmp callback functions [1]
and adding SWAP_WRAPPER macro to identify sort call of sort_r.

Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
[1] 4333fb96ca10 ("media: lib/sort.c: implement sort() variant taking context argument")
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/sort.h  |  2 +-
 include/linux/types.h |  1 +
 lib/sort.c            | 40 ++++++++++++++++++++++++++++++----------
 3 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/include/linux/sort.h b/include/linux/sort.h
index b5898725fe9d..e163287ac6c1 100644
--- a/include/linux/sort.h
+++ b/include/linux/sort.h
@@ -6,7 +6,7 @@
 
 void sort_r(void *base, size_t num, size_t size,
 	    cmp_r_func_t cmp_func,
-	    swap_func_t swap_func,
+	    swap_r_func_t swap_func,
 	    const void *priv);
 
 void sort(void *base, size_t num, size_t size,
diff --git a/include/linux/types.h b/include/linux/types.h
index ac825ad90e44..ea8cf60a8a79 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -226,6 +226,7 @@ struct callback_head {
 typedef void (*rcu_callback_t)(struct rcu_head *head);
 typedef void (*call_rcu_func_t)(struct rcu_head *head, rcu_callback_t func);
 
+typedef void (*swap_r_func_t)(void *a, void *b, int size, const void *priv);
 typedef void (*swap_func_t)(void *a, void *b, int size);
 
 typedef int (*cmp_r_func_t)(const void *a, const void *b, const void *priv);
diff --git a/lib/sort.c b/lib/sort.c
index aa18153864d2..b399bf10d675 100644
--- a/lib/sort.c
+++ b/lib/sort.c
@@ -122,16 +122,27 @@ static void swap_bytes(void *a, void *b, size_t n)
  * a pointer, but small integers make for the smallest compare
  * instructions.
  */
-#define SWAP_WORDS_64 (swap_func_t)0
-#define SWAP_WORDS_32 (swap_func_t)1
-#define SWAP_BYTES    (swap_func_t)2
+#define SWAP_WORDS_64 (swap_r_func_t)0
+#define SWAP_WORDS_32 (swap_r_func_t)1
+#define SWAP_BYTES    (swap_r_func_t)2
+#define SWAP_WRAPPER  (swap_r_func_t)3
+
+struct wrapper {
+	cmp_func_t cmp;
+	swap_func_t swap;
+};
 
 /*
  * The function pointer is last to make tail calls most efficient if the
  * compiler decides not to inline this function.
  */
-static void do_swap(void *a, void *b, size_t size, swap_func_t swap_func)
+static void do_swap(void *a, void *b, size_t size, swap_r_func_t swap_func, const void *priv)
 {
+	if (swap_func == SWAP_WRAPPER) {
+		((const struct wrapper *)priv)->swap(a, b, (int)size);
+		return;
+	}
+
 	if (swap_func == SWAP_WORDS_64)
 		swap_words_64(a, b, size);
 	else if (swap_func == SWAP_WORDS_32)
@@ -139,7 +150,7 @@ static void do_swap(void *a, void *b, size_t size, swap_func_t swap_func)
 	else if (swap_func == SWAP_BYTES)
 		swap_bytes(a, b, size);
 	else
-		swap_func(a, b, (int)size);
+		swap_func(a, b, (int)size, priv);
 }
 
 #define _CMP_WRAPPER ((cmp_r_func_t)0L)
@@ -147,7 +158,7 @@ static void do_swap(void *a, void *b, size_t size, swap_func_t swap_func)
 static int do_cmp(const void *a, const void *b, cmp_r_func_t cmp, const void *priv)
 {
 	if (cmp == _CMP_WRAPPER)
-		return ((cmp_func_t)(priv))(a, b);
+		return ((const struct wrapper *)priv)->cmp(a, b);
 	return cmp(a, b, priv);
 }
 
@@ -198,7 +209,7 @@ static size_t parent(size_t i, unsigned int lsbit, size_t size)
  */
 void sort_r(void *base, size_t num, size_t size,
 	    cmp_r_func_t cmp_func,
-	    swap_func_t swap_func,
+	    swap_r_func_t swap_func,
 	    const void *priv)
 {
 	/* pre-scale counters for performance */
@@ -208,6 +219,10 @@ void sort_r(void *base, size_t num, size_t size,
 	if (!a)		/* num < 2 || size == 0 */
 		return;
 
+	/* called from 'sort' without swap function, let's pick the default */
+	if (swap_func == SWAP_WRAPPER && !((struct wrapper *)priv)->swap)
+		swap_func = NULL;
+
 	if (!swap_func) {
 		if (is_aligned(base, size, 8))
 			swap_func = SWAP_WORDS_64;
@@ -230,7 +245,7 @@ void sort_r(void *base, size_t num, size_t size,
 		if (a)			/* Building heap: sift down --a */
 			a -= size;
 		else if (n -= size)	/* Sorting: Extract root to --n */
-			do_swap(base, base + n, size, swap_func);
+			do_swap(base, base + n, size, swap_func, priv);
 		else			/* Sort complete */
 			break;
 
@@ -257,7 +272,7 @@ void sort_r(void *base, size_t num, size_t size,
 		c = b;			/* Where "a" belongs */
 		while (b != a) {	/* Shift it into place */
 			b = parent(b, lsbit, size);
-			do_swap(base + b, base + c, size, swap_func);
+			do_swap(base + b, base + c, size, swap_func, priv);
 		}
 	}
 }
@@ -267,6 +282,11 @@ void sort(void *base, size_t num, size_t size,
 	  cmp_func_t cmp_func,
 	  swap_func_t swap_func)
 {
-	return sort_r(base, num, size, _CMP_WRAPPER, swap_func, cmp_func);
+	struct wrapper w = {
+		.cmp  = cmp_func,
+		.swap = swap_func,
+	};
+
+	return sort_r(base, num, size, _CMP_WRAPPER, SWAP_WRAPPER, &w);
 }
 EXPORT_SYMBOL(sort);
-- 
2.35.1

Re: [PATCH 01/10] lib/sort: Add priv pointer to swap function

[Masami Hiramatsu]

On Tue, 22 Feb 2022 18:05:51 +0100
Jiri Olsa <jolsa@kernel.org> wrote:

> Adding support to have priv pointer in swap callback function.
> 
> Following the initial change on cmp callback functions [1]
> and adding SWAP_WRAPPER macro to identify sort call of sort_r.
> 

This looks good to me.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you,

> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> [1] 4333fb96ca10 ("media: lib/sort.c: implement sort() variant taking context argument")
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/sort.h  |  2 +-
>  include/linux/types.h |  1 +
>  lib/sort.c            | 40 ++++++++++++++++++++++++++++++----------
>  3 files changed, 32 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/sort.h b/include/linux/sort.h
> index b5898725fe9d..e163287ac6c1 100644
> --- a/include/linux/sort.h
> +++ b/include/linux/sort.h
> @@ -6,7 +6,7 @@
>  
>  void sort_r(void *base, size_t num, size_t size,
>  	    cmp_r_func_t cmp_func,
> -	    swap_func_t swap_func,
> +	    swap_r_func_t swap_func,
>  	    const void *priv);
>  
>  void sort(void *base, size_t num, size_t size,
> diff --git a/include/linux/types.h b/include/linux/types.h
> index ac825ad90e44..ea8cf60a8a79 100644
> --- a/include/linux/types.h
> +++ b/include/linux/types.h
> @@ -226,6 +226,7 @@ struct callback_head {
>  typedef void (*rcu_callback_t)(struct rcu_head *head);
>  typedef void (*call_rcu_func_t)(struct rcu_head *head, rcu_callback_t func);
>  
> +typedef void (*swap_r_func_t)(void *a, void *b, int size, const void *priv);
>  typedef void (*swap_func_t)(void *a, void *b, int size);
>  
>  typedef int (*cmp_r_func_t)(const void *a, const void *b, const void *priv);
> diff --git a/lib/sort.c b/lib/sort.c
> index aa18153864d2..b399bf10d675 100644
> --- a/lib/sort.c
> +++ b/lib/sort.c
> @@ -122,16 +122,27 @@ static void swap_bytes(void *a, void *b, size_t n)
>   * a pointer, but small integers make for the smallest compare
>   * instructions.
>   */
> -#define SWAP_WORDS_64 (swap_func_t)0
> -#define SWAP_WORDS_32 (swap_func_t)1
> -#define SWAP_BYTES    (swap_func_t)2
> +#define SWAP_WORDS_64 (swap_r_func_t)0
> +#define SWAP_WORDS_32 (swap_r_func_t)1
> +#define SWAP_BYTES    (swap_r_func_t)2
> +#define SWAP_WRAPPER  (swap_r_func_t)3
> +
> +struct wrapper {
> +	cmp_func_t cmp;
> +	swap_func_t swap;
> +};
>  
>  /*
>   * The function pointer is last to make tail calls most efficient if the
>   * compiler decides not to inline this function.
>   */
> -static void do_swap(void *a, void *b, size_t size, swap_func_t swap_func)
> +static void do_swap(void *a, void *b, size_t size, swap_r_func_t swap_func, const void *priv)
>  {
> +	if (swap_func == SWAP_WRAPPER) {
> +		((const struct wrapper *)priv)->swap(a, b, (int)size);
> +		return;
> +	}
> +
>  	if (swap_func == SWAP_WORDS_64)
>  		swap_words_64(a, b, size);
>  	else if (swap_func == SWAP_WORDS_32)
> @@ -139,7 +150,7 @@ static void do_swap(void *a, void *b, size_t size, swap_func_t swap_func)
>  	else if (swap_func == SWAP_BYTES)
>  		swap_bytes(a, b, size);
>  	else
> -		swap_func(a, b, (int)size);
> +		swap_func(a, b, (int)size, priv);
>  }
>  
>  #define _CMP_WRAPPER ((cmp_r_func_t)0L)
> @@ -147,7 +158,7 @@ static void do_swap(void *a, void *b, size_t size, swap_func_t swap_func)
>  static int do_cmp(const void *a, const void *b, cmp_r_func_t cmp, const void *priv)
>  {
>  	if (cmp == _CMP_WRAPPER)
> -		return ((cmp_func_t)(priv))(a, b);
> +		return ((const struct wrapper *)priv)->cmp(a, b);
>  	return cmp(a, b, priv);
>  }
>  
> @@ -198,7 +209,7 @@ static size_t parent(size_t i, unsigned int lsbit, size_t size)
>   */
>  void sort_r(void *base, size_t num, size_t size,
>  	    cmp_r_func_t cmp_func,
> -	    swap_func_t swap_func,
> +	    swap_r_func_t swap_func,
>  	    const void *priv)
>  {
>  	/* pre-scale counters for performance */
> @@ -208,6 +219,10 @@ void sort_r(void *base, size_t num, size_t size,
>  	if (!a)		/* num < 2 || size == 0 */
>  		return;
>  
> +	/* called from 'sort' without swap function, let's pick the default */
> +	if (swap_func == SWAP_WRAPPER && !((struct wrapper *)priv)->swap)
> +		swap_func = NULL;
> +
>  	if (!swap_func) {
>  		if (is_aligned(base, size, 8))
>  			swap_func = SWAP_WORDS_64;
> @@ -230,7 +245,7 @@ void sort_r(void *base, size_t num, size_t size,
>  		if (a)			/* Building heap: sift down --a */
>  			a -= size;
>  		else if (n -= size)	/* Sorting: Extract root to --n */
> -			do_swap(base, base + n, size, swap_func);
> +			do_swap(base, base + n, size, swap_func, priv);
>  		else			/* Sort complete */
>  			break;
>  
> @@ -257,7 +272,7 @@ void sort_r(void *base, size_t num, size_t size,
>  		c = b;			/* Where "a" belongs */
>  		while (b != a) {	/* Shift it into place */
>  			b = parent(b, lsbit, size);
> -			do_swap(base + b, base + c, size, swap_func);
> +			do_swap(base + b, base + c, size, swap_func, priv);
>  		}
>  	}
>  }
> @@ -267,6 +282,11 @@ void sort(void *base, size_t num, size_t size,
>  	  cmp_func_t cmp_func,
>  	  swap_func_t swap_func)
>  {
> -	return sort_r(base, num, size, _CMP_WRAPPER, swap_func, cmp_func);
> +	struct wrapper w = {
> +		.cmp  = cmp_func,
> +		.swap = swap_func,
> +	};
> +
> +	return sort_r(base, num, size, _CMP_WRAPPER, SWAP_WRAPPER, &w);
>  }
>  EXPORT_SYMBOL(sort);
> -- 
> 2.35.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[PATCH 02/10] bpf: Add multi kprobe link

[Jiri Olsa]

Adding new link type BPF_LINK_TYPE_KPROBE_MULTI that attaches kprobe
program through fprobe API.

The fprobe API allows to attach probe on multiple functions at once
very fast, because it works on top of ftrace. On the other hand this
limits the probe point to the function entry or return.

The kprobe program gets the same pt_regs input ctx as when it's attached
through the perf API.

Adding new attach type BPF_TRACE_KPROBE_MULTI that allows attachment
kprobe to multiple function with new link.

User provides array of addresses or symbols with count to attach the
kprobe program to. The new link_create uapi interface looks like:

  struct {
          __aligned_u64   syms;
          __aligned_u64   addrs;
          __u32           cnt;
          __u32           flags;
  } kprobe_multi;

The flags field allows single BPF_TRACE_KPROBE_MULTI bit to create
return multi kprobe.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/bpf_types.h      |   1 +
 include/linux/trace_events.h   |   6 +
 include/uapi/linux/bpf.h       |  13 ++
 kernel/bpf/syscall.c           |  26 +++-
 kernel/trace/bpf_trace.c       | 211 +++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h |  13 ++
 6 files changed, 265 insertions(+), 5 deletions(-)

diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
index 48a91c51c015..3e24ad0c4b3c 100644
--- a/include/linux/bpf_types.h
+++ b/include/linux/bpf_types.h
@@ -140,3 +140,4 @@ BPF_LINK_TYPE(BPF_LINK_TYPE_XDP, xdp)
 #ifdef CONFIG_PERF_EVENTS
 BPF_LINK_TYPE(BPF_LINK_TYPE_PERF_EVENT, perf)
 #endif
+BPF_LINK_TYPE(BPF_LINK_TYPE_KPROBE_MULTI, kprobe_multi)
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 70c069aef02c..29f8e66929c7 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -736,6 +736,7 @@ void bpf_put_raw_tracepoint(struct bpf_raw_event_map *btp);
 int bpf_get_perf_event_info(const struct perf_event *event, u32 *prog_id,
 			    u32 *fd_type, const char **buf,
 			    u64 *probe_offset, u64 *probe_addr);
+int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog);
 #else
 static inline unsigned int trace_call_bpf(struct trace_event_call *call, void *ctx)
 {
@@ -777,6 +778,11 @@ static inline int bpf_get_perf_event_info(const struct perf_event *event,
 {
 	return -EOPNOTSUPP;
 }
+static inline int
+bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
+{
+	return -EOPNOTSUPP;
+}
 #endif
 
 enum {
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index afe3d0d7f5f2..6c66138c1b9b 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -997,6 +997,7 @@ enum bpf_attach_type {
 	BPF_SK_REUSEPORT_SELECT,
 	BPF_SK_REUSEPORT_SELECT_OR_MIGRATE,
 	BPF_PERF_EVENT,
+	BPF_TRACE_KPROBE_MULTI,
 	__MAX_BPF_ATTACH_TYPE
 };
 
@@ -1011,6 +1012,7 @@ enum bpf_link_type {
 	BPF_LINK_TYPE_NETNS = 5,
 	BPF_LINK_TYPE_XDP = 6,
 	BPF_LINK_TYPE_PERF_EVENT = 7,
+	BPF_LINK_TYPE_KPROBE_MULTI = 8,
 
 	MAX_BPF_LINK_TYPE,
 };
@@ -1118,6 +1120,11 @@ enum bpf_link_type {
  */
 #define BPF_F_XDP_HAS_FRAGS	(1U << 5)
 
+/* link_create.kprobe_multi.flags used in LINK_CREATE command for
+ * BPF_TRACE_KPROBE_MULTI attach type to create return probe.
+ */
+#define BPF_F_KPROBE_MULTI_RETURN	(1U << 0)
+
 /* When BPF ldimm64's insn[0].src_reg != 0 then this can have
  * the following extensions:
  *
@@ -1472,6 +1479,12 @@ union bpf_attr {
 				 */
 				__u64		bpf_cookie;
 			} perf_event;
+			struct {
+				__aligned_u64	syms;
+				__aligned_u64	addrs;
+				__u32		cnt;
+				__u32		flags;
+			} kprobe_multi;
 		};
 	} link_create;
 
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 9c7a72b65eee..bf76c04597b5 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -32,6 +32,7 @@
 #include <linux/bpf-netns.h>
 #include <linux/rcupdate_trace.h>
 #include <linux/memcontrol.h>
+#include <linux/trace_events.h>
 
 #define IS_FD_ARRAY(map) ((map)->map_type == BPF_MAP_TYPE_PERF_EVENT_ARRAY || \
 			  (map)->map_type == BPF_MAP_TYPE_CGROUP_ARRAY || \
@@ -3022,6 +3023,11 @@ static int bpf_perf_link_attach(const union bpf_attr *attr, struct bpf_prog *pro
 	fput(perf_file);
 	return err;
 }
+#else
+static int bpf_perf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
+{
+	return -EOPNOTSUPP;
+}
 #endif /* CONFIG_PERF_EVENTS */
 
 #define BPF_RAW_TRACEPOINT_OPEN_LAST_FIELD raw_tracepoint.prog_fd
@@ -4255,7 +4261,7 @@ static int tracing_bpf_link_attach(const union bpf_attr *attr, bpfptr_t uattr,
 	return -EINVAL;
 }
 
-#define BPF_LINK_CREATE_LAST_FIELD link_create.iter_info_len
+#define BPF_LINK_CREATE_LAST_FIELD link_create.kprobe_multi.flags
 static int link_create(union bpf_attr *attr, bpfptr_t uattr)
 {
 	enum bpf_prog_type ptype;
@@ -4279,7 +4285,6 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr)
 		ret = tracing_bpf_link_attach(attr, uattr, prog);
 		goto out;
 	case BPF_PROG_TYPE_PERF_EVENT:
-	case BPF_PROG_TYPE_KPROBE:
 	case BPF_PROG_TYPE_TRACEPOINT:
 		if (attr->link_create.attach_type != BPF_PERF_EVENT) {
 			ret = -EINVAL;
@@ -4287,6 +4292,14 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr)
 		}
 		ptype = prog->type;
 		break;
+	case BPF_PROG_TYPE_KPROBE:
+		if (attr->link_create.attach_type != BPF_PERF_EVENT &&
+		    attr->link_create.attach_type != BPF_TRACE_KPROBE_MULTI) {
+			ret = -EINVAL;
+			goto out;
+		}
+		ptype = prog->type;
+		break;
 	default:
 		ptype = attach_type_to_prog_type(attr->link_create.attach_type);
 		if (ptype == BPF_PROG_TYPE_UNSPEC || ptype != prog->type) {
@@ -4318,13 +4331,16 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr)
 		ret = bpf_xdp_link_attach(attr, prog);
 		break;
 #endif
-#ifdef CONFIG_PERF_EVENTS
 	case BPF_PROG_TYPE_PERF_EVENT:
 	case BPF_PROG_TYPE_TRACEPOINT:
-	case BPF_PROG_TYPE_KPROBE:
 		ret = bpf_perf_link_attach(attr, prog);
 		break;
-#endif
+	case BPF_PROG_TYPE_KPROBE:
+		if (attr->link_create.attach_type == BPF_PERF_EVENT)
+			ret = bpf_perf_link_attach(attr, prog);
+		else
+			ret = bpf_kprobe_multi_link_attach(attr, prog);
+		break;
 	default:
 		ret = -EINVAL;
 	}
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index a2024ba32a20..df3771bfd6e5 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -17,6 +17,7 @@
 #include <linux/error-injection.h>
 #include <linux/btf_ids.h>
 #include <linux/bpf_lsm.h>
+#include <linux/fprobe.h>
 
 #include <net/bpf_sk_storage.h>
 
@@ -2181,3 +2182,213 @@ static int __init bpf_event_init(void)
 
 fs_initcall(bpf_event_init);
 #endif /* CONFIG_MODULES */
+
+#ifdef CONFIG_FPROBE
+struct bpf_kprobe_multi_link {
+	struct bpf_link link;
+	struct fprobe fp;
+	unsigned long *addrs;
+};
+
+static void bpf_kprobe_multi_link_release(struct bpf_link *link)
+{
+	struct bpf_kprobe_multi_link *kmulti_link;
+
+	kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
+	unregister_fprobe(&kmulti_link->fp);
+}
+
+static void bpf_kprobe_multi_link_dealloc(struct bpf_link *link)
+{
+	struct bpf_kprobe_multi_link *kmulti_link;
+
+	kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
+	kvfree(kmulti_link->addrs);
+	kfree(kmulti_link);
+}
+
+static const struct bpf_link_ops bpf_kprobe_multi_link_lops = {
+	.release = bpf_kprobe_multi_link_release,
+	.dealloc = bpf_kprobe_multi_link_dealloc,
+};
+
+static int
+kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
+			   struct pt_regs *regs)
+{
+	int err;
+
+	if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
+		err = 0;
+		goto out;
+	}
+
+	rcu_read_lock();
+	migrate_disable();
+	err = bpf_prog_run(link->link.prog, regs);
+	migrate_enable();
+	rcu_read_unlock();
+
+ out:
+	__this_cpu_dec(bpf_prog_active);
+	return err;
+}
+
+static void
+kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
+			  struct pt_regs *regs)
+{
+	unsigned long saved_ip = instruction_pointer(regs);
+	struct bpf_kprobe_multi_link *link;
+
+	/*
+	 * Because fprobe's regs->ip is set to the next instruction of
+	 * dynamic-ftrace instruction, correct entry ip must be set, so
+	 * that the bpf program can access entry address via regs as same
+	 * as kprobes.
+	 */
+	instruction_pointer_set(regs, entry_ip);
+
+	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
+	kprobe_multi_link_prog_run(link, regs);
+
+	instruction_pointer_set(regs, saved_ip);
+}
+
+static int
+kprobe_multi_resolve_syms(const void *usyms, u32 cnt,
+			  unsigned long *addrs)
+{
+	unsigned long addr, size;
+	const char **syms;
+	int err = -ENOMEM;
+	unsigned int i;
+	char *func;
+
+	size = cnt * sizeof(*syms);
+	syms = kvzalloc(size, GFP_KERNEL);
+	if (!syms)
+		return -ENOMEM;
+
+	func = kmalloc(KSYM_NAME_LEN, GFP_KERNEL);
+	if (!func)
+		goto error;
+
+	if (copy_from_user(syms, usyms, size)) {
+		err = -EFAULT;
+		goto error;
+	}
+
+	for (i = 0; i < cnt; i++) {
+		err = strncpy_from_user(func, syms[i], KSYM_NAME_LEN);
+		if (err == KSYM_NAME_LEN)
+			err = -E2BIG;
+		if (err < 0)
+			goto error;
+
+		err = -EINVAL;
+		if (func[0] == '\0')
+			goto error;
+		addr = kallsyms_lookup_name(func);
+		if (!addr)
+			goto error;
+		if (!kallsyms_lookup_size_offset(addr, &size, NULL))
+			size = MCOUNT_INSN_SIZE;
+		addr = ftrace_location_range(addr, addr + size - 1);
+		if (!addr)
+			goto error;
+		addrs[i] = addr;
+	}
+
+	err = 0;
+error:
+	kvfree(syms);
+	kfree(func);
+	return err;
+}
+
+int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
+{
+	struct bpf_kprobe_multi_link *link = NULL;
+	struct bpf_link_primer link_primer;
+	unsigned long *addrs;
+	u32 flags, cnt, size;
+	void __user *uaddrs;
+	void __user *usyms;
+	int err;
+
+	/* no support for 32bit archs yet */
+	if (sizeof(u64) != sizeof(void *))
+		return -EOPNOTSUPP;
+
+	if (prog->expected_attach_type != BPF_TRACE_KPROBE_MULTI)
+		return -EINVAL;
+
+	flags = attr->link_create.kprobe_multi.flags;
+	if (flags & ~BPF_F_KPROBE_MULTI_RETURN)
+		return -EINVAL;
+
+	uaddrs = u64_to_user_ptr(attr->link_create.kprobe_multi.addrs);
+	usyms = u64_to_user_ptr(attr->link_create.kprobe_multi.syms);
+	if (!!uaddrs == !!usyms)
+		return -EINVAL;
+
+	cnt = attr->link_create.kprobe_multi.cnt;
+	if (!cnt)
+		return -EINVAL;
+
+	size = cnt * sizeof(*addrs);
+	addrs = kvmalloc(size, GFP_KERNEL);
+	if (!addrs)
+		return -ENOMEM;
+
+	if (uaddrs) {
+		if (copy_from_user(addrs, uaddrs, size)) {
+			err = -EFAULT;
+			goto error;
+		}
+	} else {
+		err = kprobe_multi_resolve_syms(usyms, cnt, addrs);
+		if (err)
+			goto error;
+	}
+
+	link = kzalloc(sizeof(*link), GFP_KERNEL);
+	if (!link) {
+		err = -ENOMEM;
+		goto error;
+	}
+
+	bpf_link_init(&link->link, BPF_LINK_TYPE_KPROBE_MULTI,
+		      &bpf_kprobe_multi_link_lops, prog);
+
+	err = bpf_link_prime(&link->link, &link_primer);
+	if (err)
+		goto error;
+
+	if (flags & BPF_F_KPROBE_MULTI_RETURN)
+		link->fp.exit_handler = kprobe_multi_link_handler;
+	else
+		link->fp.entry_handler = kprobe_multi_link_handler;
+
+	link->addrs = addrs;
+
+	err = register_fprobe_ips(&link->fp, addrs, cnt);
+	if (err) {
+		bpf_link_cleanup(&link_primer);
+		return err;
+	}
+
+	return bpf_link_settle(&link_primer);
+
+error:
+	kfree(link);
+	kvfree(addrs);
+	return err;
+}
+#else /* !CONFIG_FPROBE */
+int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
+{
+	return -EOPNOTSUPP;
+}
+#endif
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index afe3d0d7f5f2..6c66138c1b9b 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -997,6 +997,7 @@ enum bpf_attach_type {
 	BPF_SK_REUSEPORT_SELECT,
 	BPF_SK_REUSEPORT_SELECT_OR_MIGRATE,
 	BPF_PERF_EVENT,
+	BPF_TRACE_KPROBE_MULTI,
 	__MAX_BPF_ATTACH_TYPE
 };
 
@@ -1011,6 +1012,7 @@ enum bpf_link_type {
 	BPF_LINK_TYPE_NETNS = 5,
 	BPF_LINK_TYPE_XDP = 6,
 	BPF_LINK_TYPE_PERF_EVENT = 7,
+	BPF_LINK_TYPE_KPROBE_MULTI = 8,
 
 	MAX_BPF_LINK_TYPE,
 };
@@ -1118,6 +1120,11 @@ enum bpf_link_type {
  */
 #define BPF_F_XDP_HAS_FRAGS	(1U << 5)
 
+/* link_create.kprobe_multi.flags used in LINK_CREATE command for
+ * BPF_TRACE_KPROBE_MULTI attach type to create return probe.
+ */
+#define BPF_F_KPROBE_MULTI_RETURN	(1U << 0)
+
 /* When BPF ldimm64's insn[0].src_reg != 0 then this can have
  * the following extensions:
  *
@@ -1472,6 +1479,12 @@ union bpf_attr {
 				 */
 				__u64		bpf_cookie;
 			} perf_event;
+			struct {
+				__aligned_u64	syms;
+				__aligned_u64	addrs;
+				__u32		cnt;
+				__u32		flags;
+			} kprobe_multi;
 		};
 	} link_create;
 
-- 
2.35.1

Re: [PATCH 02/10] bpf: Add multi kprobe link

[Masami Hiramatsu]

Hi Jiri,

On Tue, 22 Feb 2022 18:05:52 +0100
Jiri Olsa <jolsa@kernel.org> wrote:

[snip]
> +
> +static void
> +kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
> +			  struct pt_regs *regs)
> +{
> +	unsigned long saved_ip = instruction_pointer(regs);
> +	struct bpf_kprobe_multi_link *link;
> +
> +	/*
> +	 * Because fprobe's regs->ip is set to the next instruction of
> +	 * dynamic-ftrace instruction, correct entry ip must be set, so
> +	 * that the bpf program can access entry address via regs as same
> +	 * as kprobes.
> +	 */
> +	instruction_pointer_set(regs, entry_ip);

This is true for the entry_handler, but false for the exit_handler,
because entry_ip points the probed function address, not the
return address. Thus, when this is done in the exit_handler,
the bpf prog seems to be called from the entry of the function,
not return.

If it is what you expected, please explictly comment it to
avoid confusion. Or, make another handler function for exit
probing.

> +
> +	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
> +	kprobe_multi_link_prog_run(link, regs);
> +
> +	instruction_pointer_set(regs, saved_ip);
> +}
> +
> +static int
> +kprobe_multi_resolve_syms(const void *usyms, u32 cnt,
> +			  unsigned long *addrs)
> +{
> +	unsigned long addr, size;
> +	const char **syms;
> +	int err = -ENOMEM;
> +	unsigned int i;
> +	char *func;
> +
> +	size = cnt * sizeof(*syms);
> +	syms = kvzalloc(size, GFP_KERNEL);
> +	if (!syms)
> +		return -ENOMEM;
> +
> +	func = kmalloc(KSYM_NAME_LEN, GFP_KERNEL);
> +	if (!func)
> +		goto error;
> +
> +	if (copy_from_user(syms, usyms, size)) {
> +		err = -EFAULT;
> +		goto error;
> +	}
> +
> +	for (i = 0; i < cnt; i++) {
> +		err = strncpy_from_user(func, syms[i], KSYM_NAME_LEN);
> +		if (err == KSYM_NAME_LEN)
> +			err = -E2BIG;
> +		if (err < 0)
> +			goto error;
> +
> +		err = -EINVAL;
> +		if (func[0] == '\0')
> +			goto error;
> +		addr = kallsyms_lookup_name(func);
> +		if (!addr)
> +			goto error;
> +		if (!kallsyms_lookup_size_offset(addr, &size, NULL))
> +			size = MCOUNT_INSN_SIZE;

Note that this is good for x86, but may not be good for other arch
which use some preparation instructions before mcount call.
Maybe you can just reject it if kallsyms_lookup_size_offset() fails.

Thank you,



-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 02/10] bpf: Add multi kprobe link

[Jiri Olsa]

On Wed, Feb 23, 2022 at 02:58:40PM +0900, Masami Hiramatsu wrote:
> Hi Jiri,
> 
> On Tue, 22 Feb 2022 18:05:52 +0100
> Jiri Olsa <jolsa@kernel.org> wrote:
> 
> [snip]
> > +
> > +static void
> > +kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
> > +			  struct pt_regs *regs)
> > +{
> > +	unsigned long saved_ip = instruction_pointer(regs);
> > +	struct bpf_kprobe_multi_link *link;
> > +
> > +	/*
> > +	 * Because fprobe's regs->ip is set to the next instruction of
> > +	 * dynamic-ftrace instruction, correct entry ip must be set, so
> > +	 * that the bpf program can access entry address via regs as same
> > +	 * as kprobes.
> > +	 */
> > +	instruction_pointer_set(regs, entry_ip);
> 
> This is true for the entry_handler, but false for the exit_handler,
> because entry_ip points the probed function address, not the
> return address. Thus, when this is done in the exit_handler,
> the bpf prog seems to be called from the entry of the function,
> not return.
> 
> If it is what you expected, please explictly comment it to
> avoid confusion. Or, make another handler function for exit
> probing.

yes we want the ip of the function we are tracing, so it's correct,
I'll adjust the comment

> 
> > +
> > +	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
> > +	kprobe_multi_link_prog_run(link, regs);
> > +
> > +	instruction_pointer_set(regs, saved_ip);
> > +}
> > +
> > +static int
> > +kprobe_multi_resolve_syms(const void *usyms, u32 cnt,
> > +			  unsigned long *addrs)
> > +{
> > +	unsigned long addr, size;
> > +	const char **syms;
> > +	int err = -ENOMEM;
> > +	unsigned int i;
> > +	char *func;
> > +
> > +	size = cnt * sizeof(*syms);
> > +	syms = kvzalloc(size, GFP_KERNEL);
> > +	if (!syms)
> > +		return -ENOMEM;
> > +
> > +	func = kmalloc(KSYM_NAME_LEN, GFP_KERNEL);
> > +	if (!func)
> > +		goto error;
> > +
> > +	if (copy_from_user(syms, usyms, size)) {
> > +		err = -EFAULT;
> > +		goto error;
> > +	}
> > +
> > +	for (i = 0; i < cnt; i++) {
> > +		err = strncpy_from_user(func, syms[i], KSYM_NAME_LEN);
> > +		if (err == KSYM_NAME_LEN)
> > +			err = -E2BIG;
> > +		if (err < 0)
> > +			goto error;
> > +
> > +		err = -EINVAL;
> > +		if (func[0] == '\0')
> > +			goto error;
> > +		addr = kallsyms_lookup_name(func);
> > +		if (!addr)
> > +			goto error;
> > +		if (!kallsyms_lookup_size_offset(addr, &size, NULL))
> > +			size = MCOUNT_INSN_SIZE;
> 
> Note that this is good for x86, but may not be good for other arch
> which use some preparation instructions before mcount call.  Maybe you
> can just reject it if kallsyms_lookup_size_offset() fails.

I 'borrowed' this from fprobe's get_ftrace_locations function,
and it still seems to match.. do you plan to change that?

thanks,
jirka

Re: [PATCH 02/10] bpf: Add multi kprobe link

[Masami Hiramatsu]

On Wed, 23 Feb 2022 18:44:33 +0100
Jiri Olsa <olsajiri@gmail.com> wrote:

> On Wed, Feb 23, 2022 at 02:58:40PM +0900, Masami Hiramatsu wrote:
> > Hi Jiri,
> > 
> > On Tue, 22 Feb 2022 18:05:52 +0100
> > Jiri Olsa <jolsa@kernel.org> wrote:
> > 
> > [snip]
> > > +
> > > +static void
> > > +kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
> > > +			  struct pt_regs *regs)
> > > +{
> > > +	unsigned long saved_ip = instruction_pointer(regs);
> > > +	struct bpf_kprobe_multi_link *link;
> > > +
> > > +	/*
> > > +	 * Because fprobe's regs->ip is set to the next instruction of
> > > +	 * dynamic-ftrace instruction, correct entry ip must be set, so
> > > +	 * that the bpf program can access entry address via regs as same
> > > +	 * as kprobes.
> > > +	 */
> > > +	instruction_pointer_set(regs, entry_ip);
> > 
> > This is true for the entry_handler, but false for the exit_handler,
> > because entry_ip points the probed function address, not the
> > return address. Thus, when this is done in the exit_handler,
> > the bpf prog seems to be called from the entry of the function,
> > not return.
> > 
> > If it is what you expected, please explictly comment it to
> > avoid confusion. Or, make another handler function for exit
> > probing.
> 
> yes we want the ip of the function we are tracing, so it's correct,
> I'll adjust the comment
> 
> > 
> > > +
> > > +	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
> > > +	kprobe_multi_link_prog_run(link, regs);
> > > +
> > > +	instruction_pointer_set(regs, saved_ip);
> > > +}
> > > +
> > > +static int
> > > +kprobe_multi_resolve_syms(const void *usyms, u32 cnt,
> > > +			  unsigned long *addrs)
> > > +{
> > > +	unsigned long addr, size;
> > > +	const char **syms;
> > > +	int err = -ENOMEM;
> > > +	unsigned int i;
> > > +	char *func;
> > > +
> > > +	size = cnt * sizeof(*syms);
> > > +	syms = kvzalloc(size, GFP_KERNEL);
> > > +	if (!syms)
> > > +		return -ENOMEM;
> > > +
> > > +	func = kmalloc(KSYM_NAME_LEN, GFP_KERNEL);
> > > +	if (!func)
> > > +		goto error;
> > > +
> > > +	if (copy_from_user(syms, usyms, size)) {
> > > +		err = -EFAULT;
> > > +		goto error;
> > > +	}
> > > +
> > > +	for (i = 0; i < cnt; i++) {
> > > +		err = strncpy_from_user(func, syms[i], KSYM_NAME_LEN);
> > > +		if (err == KSYM_NAME_LEN)
> > > +			err = -E2BIG;
> > > +		if (err < 0)
> > > +			goto error;
> > > +
> > > +		err = -EINVAL;
> > > +		if (func[0] == '\0')
> > > +			goto error;
> > > +		addr = kallsyms_lookup_name(func);
> > > +		if (!addr)
> > > +			goto error;
> > > +		if (!kallsyms_lookup_size_offset(addr, &size, NULL))
> > > +			size = MCOUNT_INSN_SIZE;
> > 
> > Note that this is good for x86, but may not be good for other arch
> > which use some preparation instructions before mcount call.  Maybe you
> > can just reject it if kallsyms_lookup_size_offset() fails.
> 
> I 'borrowed' this from fprobe's get_ftrace_locations function,
> and it still seems to match.. do you plan to change that?

Oops, indeed, I need to fix my code too!
Thank you! 

> 
> thanks,
> jirka


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 02/10] bpf: Add multi kprobe link

[Andrii Nakryiko]

On Tue, Feb 22, 2022 at 9:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding new link type BPF_LINK_TYPE_KPROBE_MULTI that attaches kprobe
> program through fprobe API.
>
> The fprobe API allows to attach probe on multiple functions at once
> very fast, because it works on top of ftrace. On the other hand this
> limits the probe point to the function entry or return.
>
> The kprobe program gets the same pt_regs input ctx as when it's attached
> through the perf API.
>
> Adding new attach type BPF_TRACE_KPROBE_MULTI that allows attachment
> kprobe to multiple function with new link.
>
> User provides array of addresses or symbols with count to attach the
> kprobe program to. The new link_create uapi interface looks like:
>
>   struct {
>           __aligned_u64   syms;
>           __aligned_u64   addrs;
>           __u32           cnt;
>           __u32           flags;
>   } kprobe_multi;
>
> The flags field allows single BPF_TRACE_KPROBE_MULTI bit to create
> return multi kprobe.
>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

LGTM.

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

>  include/linux/bpf_types.h      |   1 +
>  include/linux/trace_events.h   |   6 +
>  include/uapi/linux/bpf.h       |  13 ++
>  kernel/bpf/syscall.c           |  26 +++-
>  kernel/trace/bpf_trace.c       | 211 +++++++++++++++++++++++++++++++++
>  tools/include/uapi/linux/bpf.h |  13 ++
>  6 files changed, 265 insertions(+), 5 deletions(-)
>

[...]

> +
> +static int
> +kprobe_multi_resolve_syms(const void *usyms, u32 cnt,
> +                         unsigned long *addrs)
> +{
> +       unsigned long addr, size;
> +       const char **syms;
> +       int err = -ENOMEM;
> +       unsigned int i;
> +       char *func;
> +
> +       size = cnt * sizeof(*syms);
> +       syms = kvzalloc(size, GFP_KERNEL);
> +       if (!syms)
> +               return -ENOMEM;
> +
> +       func = kmalloc(KSYM_NAME_LEN, GFP_KERNEL);
> +       if (!func)
> +               goto error;
> +
> +       if (copy_from_user(syms, usyms, size)) {
> +               err = -EFAULT;
> +               goto error;
> +       }
> +
> +       for (i = 0; i < cnt; i++) {
> +               err = strncpy_from_user(func, syms[i], KSYM_NAME_LEN);
> +               if (err == KSYM_NAME_LEN)
> +                       err = -E2BIG;
> +               if (err < 0)
> +                       goto error;
> +
> +               err = -EINVAL;
> +               if (func[0] == '\0')
> +                       goto error;

wouldn't empty string be handled by kallsyms_lookup_name?

> +               addr = kallsyms_lookup_name(func);
> +               if (!addr)
> +                       goto error;
> +               if (!kallsyms_lookup_size_offset(addr, &size, NULL))
> +                       size = MCOUNT_INSN_SIZE;
> +               addr = ftrace_location_range(addr, addr + size - 1);
> +               if (!addr)
> +                       goto error;
> +               addrs[i] = addr;
> +       }
> +
> +       err = 0;
> +error:
> +       kvfree(syms);
> +       kfree(func);
> +       return err;
> +}
> +

[...]

Re: [PATCH 02/10] bpf: Add multi kprobe link

[Jiri Olsa]

On Fri, Mar 04, 2022 at 03:11:01PM -0800, Andrii Nakryiko wrote:

SNIP

> > +static int
> > +kprobe_multi_resolve_syms(const void *usyms, u32 cnt,
> > +                         unsigned long *addrs)
> > +{
> > +       unsigned long addr, size;
> > +       const char **syms;
> > +       int err = -ENOMEM;
> > +       unsigned int i;
> > +       char *func;
> > +
> > +       size = cnt * sizeof(*syms);
> > +       syms = kvzalloc(size, GFP_KERNEL);
> > +       if (!syms)
> > +               return -ENOMEM;
> > +
> > +       func = kmalloc(KSYM_NAME_LEN, GFP_KERNEL);
> > +       if (!func)
> > +               goto error;
> > +
> > +       if (copy_from_user(syms, usyms, size)) {
> > +               err = -EFAULT;
> > +               goto error;
> > +       }
> > +
> > +       for (i = 0; i < cnt; i++) {
> > +               err = strncpy_from_user(func, syms[i], KSYM_NAME_LEN);
> > +               if (err == KSYM_NAME_LEN)
> > +                       err = -E2BIG;
> > +               if (err < 0)
> > +                       goto error;
> > +
> > +               err = -EINVAL;
> > +               if (func[0] == '\0')
> > +                       goto error;
> 
> wouldn't empty string be handled by kallsyms_lookup_name?

it would scan and compare all symbols with empty string,
so it's better to test for it

jirka

> 
> > +               addr = kallsyms_lookup_name(func);
> > +               if (!addr)
> > +                       goto error;
> > +               if (!kallsyms_lookup_size_offset(addr, &size, NULL))
> > +                       size = MCOUNT_INSN_SIZE;
> > +               addr = ftrace_location_range(addr, addr + size - 1);
> > +               if (!addr)
> > +                       goto error;
> > +               addrs[i] = addr;
> > +       }
> > +
> > +       err = 0;
> > +error:
> > +       kvfree(syms);
> > +       kfree(func);
> > +       return err;
> > +}
> > +
> 
> [...]

Re: [PATCH 02/10] bpf: Add multi kprobe link

[Andrii Nakryiko]

On Sun, Mar 6, 2022 at 9:28 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Fri, Mar 04, 2022 at 03:11:01PM -0800, Andrii Nakryiko wrote:
>
> SNIP
>
> > > +static int
> > > +kprobe_multi_resolve_syms(const void *usyms, u32 cnt,
> > > +                         unsigned long *addrs)
> > > +{
> > > +       unsigned long addr, size;
> > > +       const char **syms;
> > > +       int err = -ENOMEM;
> > > +       unsigned int i;
> > > +       char *func;
> > > +
> > > +       size = cnt * sizeof(*syms);
> > > +       syms = kvzalloc(size, GFP_KERNEL);
> > > +       if (!syms)
> > > +               return -ENOMEM;
> > > +
> > > +       func = kmalloc(KSYM_NAME_LEN, GFP_KERNEL);
> > > +       if (!func)
> > > +               goto error;
> > > +
> > > +       if (copy_from_user(syms, usyms, size)) {
> > > +               err = -EFAULT;
> > > +               goto error;
> > > +       }
> > > +
> > > +       for (i = 0; i < cnt; i++) {
> > > +               err = strncpy_from_user(func, syms[i], KSYM_NAME_LEN);
> > > +               if (err == KSYM_NAME_LEN)
> > > +                       err = -E2BIG;
> > > +               if (err < 0)
> > > +                       goto error;
> > > +
> > > +               err = -EINVAL;
> > > +               if (func[0] == '\0')
> > > +                       goto error;
> >
> > wouldn't empty string be handled by kallsyms_lookup_name?
>
> it would scan and compare all symbols with empty string,
> so it's better to test for it

I don't mind, but it seems like kallsyms_lookup_name() should be made
smarter than that instead, no?


>
> jirka
>
> >
> > > +               addr = kallsyms_lookup_name(func);
> > > +               if (!addr)
> > > +                       goto error;
> > > +               if (!kallsyms_lookup_size_offset(addr, &size, NULL))
> > > +                       size = MCOUNT_INSN_SIZE;
> > > +               addr = ftrace_location_range(addr, addr + size - 1);
> > > +               if (!addr)
> > > +                       goto error;
> > > +               addrs[i] = addr;
> > > +       }
> > > +
> > > +       err = 0;
> > > +error:
> > > +       kvfree(syms);
> > > +       kfree(func);
> > > +       return err;
> > > +}
> > > +
> >
> > [...]

Re: [PATCH 02/10] bpf: Add multi kprobe link

[Jiri Olsa]

On Mon, Mar 07, 2022 at 05:23:34PM -0800, Andrii Nakryiko wrote:
> On Sun, Mar 6, 2022 at 9:28 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Fri, Mar 04, 2022 at 03:11:01PM -0800, Andrii Nakryiko wrote:
> >
> > SNIP
> >
> > > > +static int
> > > > +kprobe_multi_resolve_syms(const void *usyms, u32 cnt,
> > > > +                         unsigned long *addrs)
> > > > +{
> > > > +       unsigned long addr, size;
> > > > +       const char **syms;
> > > > +       int err = -ENOMEM;
> > > > +       unsigned int i;
> > > > +       char *func;
> > > > +
> > > > +       size = cnt * sizeof(*syms);
> > > > +       syms = kvzalloc(size, GFP_KERNEL);
> > > > +       if (!syms)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       func = kmalloc(KSYM_NAME_LEN, GFP_KERNEL);
> > > > +       if (!func)
> > > > +               goto error;
> > > > +
> > > > +       if (copy_from_user(syms, usyms, size)) {
> > > > +               err = -EFAULT;
> > > > +               goto error;
> > > > +       }
> > > > +
> > > > +       for (i = 0; i < cnt; i++) {
> > > > +               err = strncpy_from_user(func, syms[i], KSYM_NAME_LEN);
> > > > +               if (err == KSYM_NAME_LEN)
> > > > +                       err = -E2BIG;
> > > > +               if (err < 0)
> > > > +                       goto error;
> > > > +
> > > > +               err = -EINVAL;
> > > > +               if (func[0] == '\0')
> > > > +                       goto error;
> > >
> > > wouldn't empty string be handled by kallsyms_lookup_name?
> >
> > it would scan and compare all symbols with empty string,
> > so it's better to test for it
> 
> I don't mind, but it seems like kallsyms_lookup_name() should be made
> smarter than that instead, no?

true, I'll do that

jirka

[PATCH 03/10] bpf: Add bpf_get_func_ip kprobe helper for multi kprobe link

[Jiri Olsa]

Adding support to call bpf_get_func_ip helper from kprobe
programs attached by multi kprobe link.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/trace/bpf_trace.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index df3771bfd6e5..64891b7b0885 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1037,6 +1037,18 @@ static const struct bpf_func_proto bpf_get_func_ip_proto_kprobe = {
 	.arg1_type	= ARG_PTR_TO_CTX,
 };
 
+BPF_CALL_1(bpf_get_func_ip_kprobe_multi, struct pt_regs *, regs)
+{
+	return instruction_pointer(regs);
+}
+
+static const struct bpf_func_proto bpf_get_func_ip_proto_kprobe_multi = {
+	.func		= bpf_get_func_ip_kprobe_multi,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_CTX,
+};
+
 BPF_CALL_1(bpf_get_attach_cookie_trace, void *, ctx)
 {
 	struct bpf_trace_run_ctx *run_ctx;
@@ -1280,7 +1292,9 @@ kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_override_return_proto;
 #endif
 	case BPF_FUNC_get_func_ip:
-		return &bpf_get_func_ip_proto_kprobe;
+		return prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI ?
+			&bpf_get_func_ip_proto_kprobe_multi :
+			&bpf_get_func_ip_proto_kprobe;
 	case BPF_FUNC_get_attach_cookie:
 		return &bpf_get_attach_cookie_proto_trace;
 	default:
-- 
2.35.1

Re: [PATCH 03/10] bpf: Add bpf_get_func_ip kprobe helper for multi kprobe link

[Andrii Nakryiko]

On Tue, Feb 22, 2022 at 9:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support to call bpf_get_func_ip helper from kprobe
> programs attached by multi kprobe link.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

LGTM.

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


>  kernel/trace/bpf_trace.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index df3771bfd6e5..64891b7b0885 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1037,6 +1037,18 @@ static const struct bpf_func_proto bpf_get_func_ip_proto_kprobe = {
>         .arg1_type      = ARG_PTR_TO_CTX,
>  };
>
> +BPF_CALL_1(bpf_get_func_ip_kprobe_multi, struct pt_regs *, regs)
> +{
> +       return instruction_pointer(regs);
> +}
> +
> +static const struct bpf_func_proto bpf_get_func_ip_proto_kprobe_multi = {
> +       .func           = bpf_get_func_ip_kprobe_multi,
> +       .gpl_only       = false,
> +       .ret_type       = RET_INTEGER,
> +       .arg1_type      = ARG_PTR_TO_CTX,
> +};
> +
>  BPF_CALL_1(bpf_get_attach_cookie_trace, void *, ctx)
>  {
>         struct bpf_trace_run_ctx *run_ctx;
> @@ -1280,7 +1292,9 @@ kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>                 return &bpf_override_return_proto;
>  #endif
>         case BPF_FUNC_get_func_ip:
> -               return &bpf_get_func_ip_proto_kprobe;
> +               return prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI ?
> +                       &bpf_get_func_ip_proto_kprobe_multi :
> +                       &bpf_get_func_ip_proto_kprobe;
>         case BPF_FUNC_get_attach_cookie:
>                 return &bpf_get_attach_cookie_proto_trace;
>         default:
> --
> 2.35.1
>

[PATCH 04/10] bpf: Add support to inline bpf_get_func_ip helper on x86

[Jiri Olsa]

Adding support to inline it on x86, because it's single
load instruction.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/verifier.c    | 21 ++++++++++++++++++++-
 kernel/trace/bpf_trace.c |  1 +
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d7473fee247c..f125c33a37c9 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -13635,7 +13635,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
 			continue;
 		}
 
-		/* Implement bpf_get_func_ip inline. */
+		/* Implement tracing bpf_get_func_ip inline. */
 		if (prog_type == BPF_PROG_TYPE_TRACING &&
 		    insn->imm == BPF_FUNC_get_func_ip) {
 			/* Load IP address from ctx - 16 */
@@ -13650,6 +13650,25 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
 			continue;
 		}
 
+#ifdef CONFIG_X86
+		/* Implement kprobe_multi bpf_get_func_ip inline. */
+		if (prog_type == BPF_PROG_TYPE_KPROBE &&
+		    eatype == BPF_TRACE_KPROBE_MULTI &&
+		    insn->imm == BPF_FUNC_get_func_ip) {
+			/* Load IP address from ctx (struct pt_regs) ip */
+			insn_buf[0] = BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_1,
+						  offsetof(struct pt_regs, ip));
+
+			new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, 1);
+			if (!new_prog)
+				return -ENOMEM;
+
+			env->prog = prog = new_prog;
+			insn      = new_prog->insnsi + i + delta;
+			continue;
+		}
+#endif
+
 patch_call_imm:
 		fn = env->ops->get_func_proto(insn->imm, env->prog);
 		/* all functions that have prototype and verifier allowed
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 64891b7b0885..c1998b9d5531 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1039,6 +1039,7 @@ static const struct bpf_func_proto bpf_get_func_ip_proto_kprobe = {
 
 BPF_CALL_1(bpf_get_func_ip_kprobe_multi, struct pt_regs *, regs)
 {
+	/* This helper call is inlined by verifier on x86. */
 	return instruction_pointer(regs);
 }
 
-- 
2.35.1

[PATCH 05/10] bpf: Add cookie support to programs attached with kprobe multi link

[Jiri Olsa]

Adding support to call bpf_get_attach_cookie helper from
kprobe programs attached with kprobe multi link.

The cookie is provided by array of u64 values, where each
value is paired with provided function address or symbol
with the same array index.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/sort.h           |   2 +
 include/uapi/linux/bpf.h       |   1 +
 kernel/trace/bpf_trace.c       | 103 ++++++++++++++++++++++++++++++++-
 lib/sort.c                     |   2 +-
 tools/include/uapi/linux/bpf.h |   1 +
 5 files changed, 107 insertions(+), 2 deletions(-)

diff --git a/include/linux/sort.h b/include/linux/sort.h
index e163287ac6c1..2dbf30a32143 100644
--- a/include/linux/sort.h
+++ b/include/linux/sort.h
@@ -13,4 +13,6 @@ void sort(void *base, size_t num, size_t size,
 	  cmp_func_t cmp_func,
 	  swap_func_t swap_func);
 
+void swap_words_64(void *a, void *b, size_t n);
+
 #endif
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 6c66138c1b9b..d18996502aac 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1482,6 +1482,7 @@ union bpf_attr {
 			struct {
 				__aligned_u64	syms;
 				__aligned_u64	addrs;
+				__aligned_u64	cookies;
 				__u32		cnt;
 				__u32		flags;
 			} kprobe_multi;
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index c1998b9d5531..8d85bd97cb45 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -18,6 +18,8 @@
 #include <linux/btf_ids.h>
 #include <linux/bpf_lsm.h>
 #include <linux/fprobe.h>
+#include <linux/bsearch.h>
+#include <linux/sort.h>
 
 #include <net/bpf_sk_storage.h>
 
@@ -78,6 +80,7 @@ u64 bpf_get_stack(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
 static int bpf_btf_printf_prepare(struct btf_ptr *ptr, u32 btf_ptr_size,
 				  u64 flags, const struct btf **btf,
 				  s32 *btf_id);
+static u64 bpf_kprobe_multi_cookie(struct bpf_run_ctx *ctx, u64 ip);
 
 /**
  * trace_call_bpf - invoke BPF program
@@ -1050,6 +1053,18 @@ static const struct bpf_func_proto bpf_get_func_ip_proto_kprobe_multi = {
 	.arg1_type	= ARG_PTR_TO_CTX,
 };
 
+BPF_CALL_1(bpf_get_attach_cookie_kprobe_multi, struct pt_regs *, regs)
+{
+	return bpf_kprobe_multi_cookie(current->bpf_ctx, regs->ip);
+}
+
+static const struct bpf_func_proto bpf_get_attach_cookie_proto_kmulti = {
+	.func		= bpf_get_attach_cookie_kprobe_multi,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_CTX,
+};
+
 BPF_CALL_1(bpf_get_attach_cookie_trace, void *, ctx)
 {
 	struct bpf_trace_run_ctx *run_ctx;
@@ -1297,7 +1312,9 @@ kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 			&bpf_get_func_ip_proto_kprobe_multi :
 			&bpf_get_func_ip_proto_kprobe;
 	case BPF_FUNC_get_attach_cookie:
-		return &bpf_get_attach_cookie_proto_trace;
+		return prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI ?
+			&bpf_get_attach_cookie_proto_kmulti :
+			&bpf_get_attach_cookie_proto_trace;
 	default:
 		return bpf_tracing_func_proto(func_id, prog);
 	}
@@ -2203,6 +2220,9 @@ struct bpf_kprobe_multi_link {
 	struct bpf_link link;
 	struct fprobe fp;
 	unsigned long *addrs;
+	struct bpf_run_ctx run_ctx;
+	u64 *cookies;
+	u32 cnt;
 };
 
 static void bpf_kprobe_multi_link_release(struct bpf_link *link)
@@ -2219,6 +2239,7 @@ static void bpf_kprobe_multi_link_dealloc(struct bpf_link *link)
 
 	kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
 	kvfree(kmulti_link->addrs);
+	kvfree(kmulti_link->cookies);
 	kfree(kmulti_link);
 }
 
@@ -2227,10 +2248,57 @@ static const struct bpf_link_ops bpf_kprobe_multi_link_lops = {
 	.dealloc = bpf_kprobe_multi_link_dealloc,
 };
 
+static void bpf_kprobe_multi_cookie_swap(void *a, void *b, int size, const void *priv)
+{
+	const struct bpf_kprobe_multi_link *link = priv;
+	unsigned long *addr_a = a, *addr_b = b;
+	u64 *cookie_a, *cookie_b;
+
+	cookie_a = link->cookies + (addr_a - link->addrs);
+	cookie_b = link->cookies + (addr_b - link->addrs);
+
+	swap_words_64(addr_a, addr_b, size);
+	swap_words_64(cookie_a, cookie_b, size);
+}
+
+static int __bpf_kprobe_multi_cookie_cmp(const void *a, const void *b)
+{
+	const unsigned long *addr_a = a, *addr_b = b;
+
+	if (*addr_a == *addr_b)
+		return 0;
+	return *addr_a < *addr_b ? -1 : 1;
+}
+
+static int bpf_kprobe_multi_cookie_cmp(const void *a, const void *b, const void *priv)
+{
+	return __bpf_kprobe_multi_cookie_cmp(a, b);
+}
+
+static u64 bpf_kprobe_multi_cookie(struct bpf_run_ctx *ctx, u64 ip)
+{
+	struct bpf_kprobe_multi_link *link;
+	unsigned long *addr;
+	u64 *cookie;
+
+	if (WARN_ON_ONCE(!ctx))
+		return 0;
+	link = container_of(ctx, struct bpf_kprobe_multi_link, run_ctx);
+	if (!link->cookies)
+		return 0;
+	addr = bsearch(&ip, link->addrs, link->cnt, sizeof(ip),
+		       __bpf_kprobe_multi_cookie_cmp);
+	if (!addr)
+		return 0;
+	cookie = link->cookies + (addr - link->addrs);
+	return *cookie;
+}
+
 static int
 kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
 			   struct pt_regs *regs)
 {
+	struct bpf_run_ctx *old_run_ctx;
 	int err;
 
 	if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
@@ -2238,12 +2306,16 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
 		goto out;
 	}
 
+	old_run_ctx = bpf_set_run_ctx(&link->run_ctx);
+
 	rcu_read_lock();
 	migrate_disable();
 	err = bpf_prog_run(link->link.prog, regs);
 	migrate_enable();
 	rcu_read_unlock();
 
+	bpf_reset_run_ctx(old_run_ctx);
+
  out:
 	__this_cpu_dec(bpf_prog_active);
 	return err;
@@ -2326,9 +2398,11 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 {
 	struct bpf_kprobe_multi_link *link = NULL;
 	struct bpf_link_primer link_primer;
+	void __user *ucookies;
 	unsigned long *addrs;
 	u32 flags, cnt, size;
 	void __user *uaddrs;
+	u64 *cookies = NULL;
 	void __user *usyms;
 	int err;
 
@@ -2368,6 +2442,19 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 			goto error;
 	}
 
+	ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
+	if (ucookies) {
+		cookies = kvmalloc(size, GFP_KERNEL);
+		if (!cookies) {
+			err = -ENOMEM;
+			goto error;
+		}
+		if (copy_from_user(cookies, ucookies, size)) {
+			err = -EFAULT;
+			goto error;
+		}
+	}
+
 	link = kzalloc(sizeof(*link), GFP_KERNEL);
 	if (!link) {
 		err = -ENOMEM;
@@ -2387,6 +2474,15 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 		link->fp.entry_handler = kprobe_multi_link_handler;
 
 	link->addrs = addrs;
+	link->cookies = cookies;
+	link->cnt = cnt;
+
+	if (cookies) {
+		sort_r(addrs, cnt, sizeof(*addrs),
+		       bpf_kprobe_multi_cookie_cmp,
+		       bpf_kprobe_multi_cookie_swap,
+		       link);
+	}
 
 	err = register_fprobe_ips(&link->fp, addrs, cnt);
 	if (err) {
@@ -2399,6 +2495,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 error:
 	kfree(link);
 	kvfree(addrs);
+	kvfree(cookies);
 	return err;
 }
 #else /* !CONFIG_FPROBE */
@@ -2406,4 +2503,8 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 {
 	return -EOPNOTSUPP;
 }
+static u64 bpf_kprobe_multi_cookie(struct bpf_run_ctx *ctx, u64 ip)
+{
+	return 0;
+}
 #endif
diff --git a/lib/sort.c b/lib/sort.c
index b399bf10d675..91f7ce701cf4 100644
--- a/lib/sort.c
+++ b/lib/sort.c
@@ -80,7 +80,7 @@ static void swap_words_32(void *a, void *b, size_t n)
  * but it's possible to have 64-bit loads without 64-bit pointers (e.g.
  * x32 ABI).  Are there any cases the kernel needs to worry about?
  */
-static void swap_words_64(void *a, void *b, size_t n)
+void swap_words_64(void *a, void *b, size_t n)
 {
 	do {
 #ifdef CONFIG_64BIT
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 6c66138c1b9b..d18996502aac 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1482,6 +1482,7 @@ union bpf_attr {
 			struct {
 				__aligned_u64	syms;
 				__aligned_u64	addrs;
+				__aligned_u64	cookies;
 				__u32		cnt;
 				__u32		flags;
 			} kprobe_multi;
-- 
2.35.1

Re: [PATCH 05/10] bpf: Add cookie support to programs attached with kprobe multi link

[Andrii Nakryiko]

On Tue, Feb 22, 2022 at 9:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support to call bpf_get_attach_cookie helper from
> kprobe programs attached with kprobe multi link.
>
> The cookie is provided by array of u64 values, where each
> value is paired with provided function address or symbol
> with the same array index.
>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/sort.h           |   2 +
>  include/uapi/linux/bpf.h       |   1 +
>  kernel/trace/bpf_trace.c       | 103 ++++++++++++++++++++++++++++++++-
>  lib/sort.c                     |   2 +-
>  tools/include/uapi/linux/bpf.h |   1 +
>  5 files changed, 107 insertions(+), 2 deletions(-)
>

[...]

>  BPF_CALL_1(bpf_get_attach_cookie_trace, void *, ctx)
>  {
>         struct bpf_trace_run_ctx *run_ctx;
> @@ -1297,7 +1312,9 @@ kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>                         &bpf_get_func_ip_proto_kprobe_multi :
>                         &bpf_get_func_ip_proto_kprobe;
>         case BPF_FUNC_get_attach_cookie:
> -               return &bpf_get_attach_cookie_proto_trace;
> +               return prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI ?
> +                       &bpf_get_attach_cookie_proto_kmulti :
> +                       &bpf_get_attach_cookie_proto_trace;
>         default:
>                 return bpf_tracing_func_proto(func_id, prog);
>         }
> @@ -2203,6 +2220,9 @@ struct bpf_kprobe_multi_link {
>         struct bpf_link link;
>         struct fprobe fp;
>         unsigned long *addrs;
> +       struct bpf_run_ctx run_ctx;

clever, I like it! Keep in mind, though, that this trick can only be
used here because this run_ctx is read-only (I'd leave the comment
here about this, I didn't realize immediately that this approach can't
be used for run_ctx that needs to be modified).

> +       u64 *cookies;
> +       u32 cnt;
>  };
>
>  static void bpf_kprobe_multi_link_release(struct bpf_link *link)
> @@ -2219,6 +2239,7 @@ static void bpf_kprobe_multi_link_dealloc(struct bpf_link *link)
>
>         kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
>         kvfree(kmulti_link->addrs);
> +       kvfree(kmulti_link->cookies);
>         kfree(kmulti_link);
>  }
>
> @@ -2227,10 +2248,57 @@ static const struct bpf_link_ops bpf_kprobe_multi_link_lops = {
>         .dealloc = bpf_kprobe_multi_link_dealloc,
>  };
>
> +static void bpf_kprobe_multi_cookie_swap(void *a, void *b, int size, const void *priv)
> +{
> +       const struct bpf_kprobe_multi_link *link = priv;
> +       unsigned long *addr_a = a, *addr_b = b;
> +       u64 *cookie_a, *cookie_b;
> +
> +       cookie_a = link->cookies + (addr_a - link->addrs);
> +       cookie_b = link->cookies + (addr_b - link->addrs);
> +
> +       swap_words_64(addr_a, addr_b, size);
> +       swap_words_64(cookie_a, cookie_b, size);

is it smart to call (now) non-inlined function just to swap two longs
and u64s?..

unsigned long tmp1;
u64 tmp2;

tmp1 = *addr_a; *addr_a = addr_b; *addr_b = tmp1;
tmp2 = *cookie_a; *cookie_a = cookie_b; *cookie_b = tmp2;

?

> +}
> +
> +static int __bpf_kprobe_multi_cookie_cmp(const void *a, const void *b)
> +{
> +       const unsigned long *addr_a = a, *addr_b = b;
> +
> +       if (*addr_a == *addr_b)
> +               return 0;
> +       return *addr_a < *addr_b ? -1 : 1;
> +}
> +

[...]

> @@ -2238,12 +2306,16 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
>                 goto out;
>         }
>
> +       old_run_ctx = bpf_set_run_ctx(&link->run_ctx);
> +
>         rcu_read_lock();

so looking at other code, I see that we first migrate_disable() and
then rcu_read_lock(), so let's swap? We also normally set/reset
run_ctx inside migrate+rcu_lock region. I'm not sure that's necessary,
but also shouldn't hurt to stay consistent.

>         migrate_disable();
>         err = bpf_prog_run(link->link.prog, regs);
>         migrate_enable();
>         rcu_read_unlock();
>
> +       bpf_reset_run_ctx(old_run_ctx);
> +
>   out:
>         __this_cpu_dec(bpf_prog_active);
>         return err;

[...]

> diff --git a/lib/sort.c b/lib/sort.c
> index b399bf10d675..91f7ce701cf4 100644
> --- a/lib/sort.c
> +++ b/lib/sort.c
> @@ -80,7 +80,7 @@ static void swap_words_32(void *a, void *b, size_t n)
>   * but it's possible to have 64-bit loads without 64-bit pointers (e.g.
>   * x32 ABI).  Are there any cases the kernel needs to worry about?
>   */
> -static void swap_words_64(void *a, void *b, size_t n)
> +void swap_words_64(void *a, void *b, size_t n)

I'm worried that this might change performance unintentionally in
other places (making the function global might pessimize inlining, I
think). So let's not do that, just do a straightforward swap in cookie
support code?

>  {
>         do {
>  #ifdef CONFIG_64BIT
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 6c66138c1b9b..d18996502aac 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -1482,6 +1482,7 @@ union bpf_attr {
>                         struct {
>                                 __aligned_u64   syms;
>                                 __aligned_u64   addrs;
> +                               __aligned_u64   cookies;

looks a bit weird to change layout of UAPI. That's not really a
problem, because both patches will land at the same time. But if you
move flags and cnt to the front of the struct it would a bit better.


>                                 __u32           cnt;
>                                 __u32           flags;
>                         } kprobe_multi;
> --
> 2.35.1
>

Re: [PATCH 05/10] bpf: Add cookie support to programs attached with kprobe multi link

[Jiri Olsa]

On Fri, Mar 04, 2022 at 03:11:08PM -0800, Andrii Nakryiko wrote:
> On Tue, Feb 22, 2022 at 9:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding support to call bpf_get_attach_cookie helper from
> > kprobe programs attached with kprobe multi link.
> >
> > The cookie is provided by array of u64 values, where each
> > value is paired with provided function address or symbol
> > with the same array index.
> >
> > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/linux/sort.h           |   2 +
> >  include/uapi/linux/bpf.h       |   1 +
> >  kernel/trace/bpf_trace.c       | 103 ++++++++++++++++++++++++++++++++-
> >  lib/sort.c                     |   2 +-
> >  tools/include/uapi/linux/bpf.h |   1 +
> >  5 files changed, 107 insertions(+), 2 deletions(-)
> >
> 
> [...]
> 
> >  BPF_CALL_1(bpf_get_attach_cookie_trace, void *, ctx)
> >  {
> >         struct bpf_trace_run_ctx *run_ctx;
> > @@ -1297,7 +1312,9 @@ kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >                         &bpf_get_func_ip_proto_kprobe_multi :
> >                         &bpf_get_func_ip_proto_kprobe;
> >         case BPF_FUNC_get_attach_cookie:
> > -               return &bpf_get_attach_cookie_proto_trace;
> > +               return prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI ?
> > +                       &bpf_get_attach_cookie_proto_kmulti :
> > +                       &bpf_get_attach_cookie_proto_trace;
> >         default:
> >                 return bpf_tracing_func_proto(func_id, prog);
> >         }
> > @@ -2203,6 +2220,9 @@ struct bpf_kprobe_multi_link {
> >         struct bpf_link link;
> >         struct fprobe fp;
> >         unsigned long *addrs;
> > +       struct bpf_run_ctx run_ctx;
> 
> clever, I like it! Keep in mind, though, that this trick can only be
> used here because this run_ctx is read-only (I'd leave the comment
> here about this, I didn't realize immediately that this approach can't
> be used for run_ctx that needs to be modified).

hum, I don't see it at the moment.. I'll check on that and add the
comment or come up with more questions ;-)

> 
> > +       u64 *cookies;
> > +       u32 cnt;
> >  };
> >
> >  static void bpf_kprobe_multi_link_release(struct bpf_link *link)
> > @@ -2219,6 +2239,7 @@ static void bpf_kprobe_multi_link_dealloc(struct bpf_link *link)
> >
> >         kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
> >         kvfree(kmulti_link->addrs);
> > +       kvfree(kmulti_link->cookies);
> >         kfree(kmulti_link);
> >  }
> >
> > @@ -2227,10 +2248,57 @@ static const struct bpf_link_ops bpf_kprobe_multi_link_lops = {
> >         .dealloc = bpf_kprobe_multi_link_dealloc,
> >  };
> >
> > +static void bpf_kprobe_multi_cookie_swap(void *a, void *b, int size, const void *priv)
> > +{
> > +       const struct bpf_kprobe_multi_link *link = priv;
> > +       unsigned long *addr_a = a, *addr_b = b;
> > +       u64 *cookie_a, *cookie_b;
> > +
> > +       cookie_a = link->cookies + (addr_a - link->addrs);
> > +       cookie_b = link->cookies + (addr_b - link->addrs);
> > +
> > +       swap_words_64(addr_a, addr_b, size);
> > +       swap_words_64(cookie_a, cookie_b, size);
> 
> is it smart to call (now) non-inlined function just to swap two longs
> and u64s?..
> 
> unsigned long tmp1;
> u64 tmp2;
> 
> tmp1 = *addr_a; *addr_a = addr_b; *addr_b = tmp1;
> tmp2 = *cookie_a; *cookie_a = cookie_b; *cookie_b = tmp2;

the swap_words_64 has CONFIG_64BIT ifdef with some tweaks for 32bit,
so I wanted to use that.. but I agree with your other comment below
wrt performace, so will change

> 
> ?
> 
> > +}
> > +
> > +static int __bpf_kprobe_multi_cookie_cmp(const void *a, const void *b)
> > +{
> > +       const unsigned long *addr_a = a, *addr_b = b;
> > +
> > +       if (*addr_a == *addr_b)
> > +               return 0;
> > +       return *addr_a < *addr_b ? -1 : 1;
> > +}
> > +
> 
> [...]
> 
> > @@ -2238,12 +2306,16 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
> >                 goto out;
> >         }
> >
> > +       old_run_ctx = bpf_set_run_ctx(&link->run_ctx);
> > +
> >         rcu_read_lock();
> 
> so looking at other code, I see that we first migrate_disable() and
> then rcu_read_lock(), so let's swap? We also normally set/reset
> run_ctx inside migrate+rcu_lock region. I'm not sure that's necessary,
> but also shouldn't hurt to stay consistent.

ok, will change

> 
> >         migrate_disable();
> >         err = bpf_prog_run(link->link.prog, regs);
> >         migrate_enable();
> >         rcu_read_unlock();
> >
> > +       bpf_reset_run_ctx(old_run_ctx);
> > +
> >   out:
> >         __this_cpu_dec(bpf_prog_active);
> >         return err;
> 
> [...]
> 
> > diff --git a/lib/sort.c b/lib/sort.c
> > index b399bf10d675..91f7ce701cf4 100644
> > --- a/lib/sort.c
> > +++ b/lib/sort.c
> > @@ -80,7 +80,7 @@ static void swap_words_32(void *a, void *b, size_t n)
> >   * but it's possible to have 64-bit loads without 64-bit pointers (e.g.
> >   * x32 ABI).  Are there any cases the kernel needs to worry about?
> >   */
> > -static void swap_words_64(void *a, void *b, size_t n)
> > +void swap_words_64(void *a, void *b, size_t n)
> 
> I'm worried that this might change performance unintentionally in
> other places (making the function global might pessimize inlining, I
> think). So let's not do that, just do a straightforward swap in cookie
> support code?

right, I did not realize this.. I'll add to cookie code directly

> 
> >  {
> >         do {
> >  #ifdef CONFIG_64BIT
> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index 6c66138c1b9b..d18996502aac 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -1482,6 +1482,7 @@ union bpf_attr {
> >                         struct {
> >                                 __aligned_u64   syms;
> >                                 __aligned_u64   addrs;
> > +                               __aligned_u64   cookies;
> 
> looks a bit weird to change layout of UAPI. That's not really a
> problem, because both patches will land at the same time. But if you
> move flags and cnt to the front of the struct it would a bit better.

I was following your previous comment:
  https://lore.kernel.org/bpf/CAEf4BzbPeQbURZOD93TgPudOk3JD4odsZ9uwriNkrphes9V4dg@mail.gmail.com/

I like the idea that syms/addrs/cookies stay together,
because they are all related to cnt.. but yes, it's
'breaking' KABI in between these patches

jirka

> 
> 
> >                                 __u32           cnt;
> >                                 __u32           flags;
> >                         } kprobe_multi;
> > --
> > 2.35.1
> >

Re: [PATCH 05/10] bpf: Add cookie support to programs attached with kprobe multi link

[Andrii Nakryiko]

On Sun, Mar 6, 2022 at 9:29 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Fri, Mar 04, 2022 at 03:11:08PM -0800, Andrii Nakryiko wrote:
> > On Tue, Feb 22, 2022 at 9:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Adding support to call bpf_get_attach_cookie helper from
> > > kprobe programs attached with kprobe multi link.
> > >
> > > The cookie is provided by array of u64 values, where each
> > > value is paired with provided function address or symbol
> > > with the same array index.
> > >
> > > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  include/linux/sort.h           |   2 +
> > >  include/uapi/linux/bpf.h       |   1 +
> > >  kernel/trace/bpf_trace.c       | 103 ++++++++++++++++++++++++++++++++-
> > >  lib/sort.c                     |   2 +-
> > >  tools/include/uapi/linux/bpf.h |   1 +
> > >  5 files changed, 107 insertions(+), 2 deletions(-)
> > >
> >
> > [...]
> >
> > >  BPF_CALL_1(bpf_get_attach_cookie_trace, void *, ctx)
> > >  {
> > >         struct bpf_trace_run_ctx *run_ctx;
> > > @@ -1297,7 +1312,9 @@ kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> > >                         &bpf_get_func_ip_proto_kprobe_multi :
> > >                         &bpf_get_func_ip_proto_kprobe;
> > >         case BPF_FUNC_get_attach_cookie:
> > > -               return &bpf_get_attach_cookie_proto_trace;
> > > +               return prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI ?
> > > +                       &bpf_get_attach_cookie_proto_kmulti :
> > > +                       &bpf_get_attach_cookie_proto_trace;
> > >         default:
> > >                 return bpf_tracing_func_proto(func_id, prog);
> > >         }
> > > @@ -2203,6 +2220,9 @@ struct bpf_kprobe_multi_link {
> > >         struct bpf_link link;
> > >         struct fprobe fp;
> > >         unsigned long *addrs;
> > > +       struct bpf_run_ctx run_ctx;
> >
> > clever, I like it! Keep in mind, though, that this trick can only be
> > used here because this run_ctx is read-only (I'd leave the comment
> > here about this, I didn't realize immediately that this approach can't
> > be used for run_ctx that needs to be modified).
>
> hum, I don't see it at the moment.. I'll check on that and add the
> comment or come up with more questions ;-)

if run_ctx is used to store some information, it has to be per program
execution (private to a single bpf program run, just like bpf
program's stack). So you can't just reuse bpf_link for that, because
bpf_link is shared across all CPUs and thus (potentially) across
multiple simultaneous prog runs

>
> >
> > > +       u64 *cookies;
> > > +       u32 cnt;
> > >  };
> > >

[...]

> >
> > >  {
> > >         do {
> > >  #ifdef CONFIG_64BIT
> > > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > > index 6c66138c1b9b..d18996502aac 100644
> > > --- a/tools/include/uapi/linux/bpf.h
> > > +++ b/tools/include/uapi/linux/bpf.h
> > > @@ -1482,6 +1482,7 @@ union bpf_attr {
> > >                         struct {
> > >                                 __aligned_u64   syms;
> > >                                 __aligned_u64   addrs;
> > > +                               __aligned_u64   cookies;
> >
> > looks a bit weird to change layout of UAPI. That's not really a
> > problem, because both patches will land at the same time. But if you
> > move flags and cnt to the front of the struct it would a bit better.
>
> I was following your previous comment:
>   https://lore.kernel.org/bpf/CAEf4BzbPeQbURZOD93TgPudOk3JD4odsZ9uwriNkrphes9V4dg@mail.gmail.com/
>

yeah, I didn't anticipate the cookies change at that time, but now it
became obvious

> I like the idea that syms/addrs/cookies stay together,
> because they are all related to cnt.. but yes, it's
> 'breaking' KABI in between these patches
>
> jirka
>
> >
> >
> > >                                 __u32           cnt;
> > >                                 __u32           flags;
> > >                         } kprobe_multi;
> > > --
> > > 2.35.1
> > >

Re: [PATCH 05/10] bpf: Add cookie support to programs attached with kprobe multi link

[Jiri Olsa]

On Mon, Mar 07, 2022 at 05:23:31PM -0800, Andrii Nakryiko wrote:
> On Sun, Mar 6, 2022 at 9:29 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Fri, Mar 04, 2022 at 03:11:08PM -0800, Andrii Nakryiko wrote:
> > > On Tue, Feb 22, 2022 at 9:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > >
> > > > Adding support to call bpf_get_attach_cookie helper from
> > > > kprobe programs attached with kprobe multi link.
> > > >
> > > > The cookie is provided by array of u64 values, where each
> > > > value is paired with provided function address or symbol
> > > > with the same array index.
> > > >
> > > > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >  include/linux/sort.h           |   2 +
> > > >  include/uapi/linux/bpf.h       |   1 +
> > > >  kernel/trace/bpf_trace.c       | 103 ++++++++++++++++++++++++++++++++-
> > > >  lib/sort.c                     |   2 +-
> > > >  tools/include/uapi/linux/bpf.h |   1 +
> > > >  5 files changed, 107 insertions(+), 2 deletions(-)
> > > >
> > >
> > > [...]
> > >
> > > >  BPF_CALL_1(bpf_get_attach_cookie_trace, void *, ctx)
> > > >  {
> > > >         struct bpf_trace_run_ctx *run_ctx;
> > > > @@ -1297,7 +1312,9 @@ kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> > > >                         &bpf_get_func_ip_proto_kprobe_multi :
> > > >                         &bpf_get_func_ip_proto_kprobe;
> > > >         case BPF_FUNC_get_attach_cookie:
> > > > -               return &bpf_get_attach_cookie_proto_trace;
> > > > +               return prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI ?
> > > > +                       &bpf_get_attach_cookie_proto_kmulti :
> > > > +                       &bpf_get_attach_cookie_proto_trace;
> > > >         default:
> > > >                 return bpf_tracing_func_proto(func_id, prog);
> > > >         }
> > > > @@ -2203,6 +2220,9 @@ struct bpf_kprobe_multi_link {
> > > >         struct bpf_link link;
> > > >         struct fprobe fp;
> > > >         unsigned long *addrs;
> > > > +       struct bpf_run_ctx run_ctx;
> > >
> > > clever, I like it! Keep in mind, though, that this trick can only be
> > > used here because this run_ctx is read-only (I'd leave the comment
> > > here about this, I didn't realize immediately that this approach can't
> > > be used for run_ctx that needs to be modified).
> >
> > hum, I don't see it at the moment.. I'll check on that and add the
> > comment or come up with more questions ;-)
> 
> if run_ctx is used to store some information, it has to be per program
> execution (private to a single bpf program run, just like bpf
> program's stack). So you can't just reuse bpf_link for that, because
> bpf_link is shared across all CPUs and thus (potentially) across
> multiple simultaneous prog runs

ok, I'll put some comments in here about that

thanks,
jirka

[PATCH 06/10] libbpf: Add libbpf_kallsyms_parse function

[Jiri Olsa]

Move the kallsyms parsing in internal libbpf_kallsyms_parse
function, so it can be used from other places.

It will be used in following changes.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/lib/bpf/libbpf.c          | 62 ++++++++++++++++++++-------------
 tools/lib/bpf/libbpf_internal.h |  5 +++
 2 files changed, 43 insertions(+), 24 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index ad43b6ce825e..fb530b004a0d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -7172,12 +7172,10 @@ static int bpf_object__sanitize_maps(struct bpf_object *obj)
 	return 0;
 }
 
-static int bpf_object__read_kallsyms_file(struct bpf_object *obj)
+int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *ctx)
 {
 	char sym_type, sym_name[500];
 	unsigned long long sym_addr;
-	const struct btf_type *t;
-	struct extern_desc *ext;
 	int ret, err = 0;
 	FILE *f;
 
@@ -7196,35 +7194,51 @@ static int bpf_object__read_kallsyms_file(struct bpf_object *obj)
 		if (ret != 3) {
 			pr_warn("failed to read kallsyms entry: %d\n", ret);
 			err = -EINVAL;
-			goto out;
+			break;
 		}
 
-		ext = find_extern_by_name(obj, sym_name);
-		if (!ext || ext->type != EXT_KSYM)
-			continue;
-
-		t = btf__type_by_id(obj->btf, ext->btf_id);
-		if (!btf_is_var(t))
-			continue;
-
-		if (ext->is_set && ext->ksym.addr != sym_addr) {
-			pr_warn("extern (ksym) '%s' resolution is ambiguous: 0x%llx or 0x%llx\n",
-				sym_name, ext->ksym.addr, sym_addr);
-			err = -EINVAL;
-			goto out;
-		}
-		if (!ext->is_set) {
-			ext->is_set = true;
-			ext->ksym.addr = sym_addr;
-			pr_debug("extern (ksym) %s=0x%llx\n", sym_name, sym_addr);
-		}
+		err = cb(sym_addr, sym_type, sym_name, ctx);
+		if (err)
+			break;
 	}
 
-out:
 	fclose(f);
 	return err;
 }
 
+static int kallsyms_cb(unsigned long long sym_addr, char sym_type,
+		       const char *sym_name, void *ctx)
+{
+	struct bpf_object *obj = ctx;
+	const struct btf_type *t;
+	struct extern_desc *ext;
+
+	ext = find_extern_by_name(obj, sym_name);
+	if (!ext || ext->type != EXT_KSYM)
+		return 0;
+
+	t = btf__type_by_id(obj->btf, ext->btf_id);
+	if (!btf_is_var(t))
+		return 0;
+
+	if (ext->is_set && ext->ksym.addr != sym_addr) {
+		pr_warn("extern (ksym) '%s' resolution is ambiguous: 0x%llx or 0x%llx\n",
+			sym_name, ext->ksym.addr, sym_addr);
+		return -EINVAL;
+	}
+	if (!ext->is_set) {
+		ext->is_set = true;
+		ext->ksym.addr = sym_addr;
+		pr_debug("extern (ksym) %s=0x%llx\n", sym_name, sym_addr);
+	}
+	return 0;
+}
+
+static int bpf_object__read_kallsyms_file(struct bpf_object *obj)
+{
+	return libbpf_kallsyms_parse(kallsyms_cb, obj);
+}
+
 static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
 			    __u16 kind, struct btf **res_btf,
 			    struct module_btf **res_mod_btf)
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index 4fda8bdf0a0d..b6247dc7f8eb 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -449,6 +449,11 @@ __s32 btf__find_by_name_kind_own(const struct btf *btf, const char *type_name,
 
 extern enum libbpf_strict_mode libbpf_mode;
 
+typedef int (*kallsyms_cb_t)(unsigned long long sym_addr, char sym_type,
+			     const char *sym_name, void *ctx);
+
+int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *arg);
+
 /* handle direct returned errors */
 static inline int libbpf_err(int ret)
 {
-- 
2.35.1

Re: [PATCH 06/10] libbpf: Add libbpf_kallsyms_parse function

[Andrii Nakryiko]

On Tue, Feb 22, 2022 at 9:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Move the kallsyms parsing in internal libbpf_kallsyms_parse
> function, so it can be used from other places.
>
> It will be used in following changes.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

LGTM

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


>  tools/lib/bpf/libbpf.c          | 62 ++++++++++++++++++++-------------
>  tools/lib/bpf/libbpf_internal.h |  5 +++
>  2 files changed, 43 insertions(+), 24 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index ad43b6ce825e..fb530b004a0d 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -7172,12 +7172,10 @@ static int bpf_object__sanitize_maps(struct bpf_object *obj)
>         return 0;
>  }
>
> -static int bpf_object__read_kallsyms_file(struct bpf_object *obj)
> +int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *ctx)
>  {
>         char sym_type, sym_name[500];
>         unsigned long long sym_addr;
> -       const struct btf_type *t;
> -       struct extern_desc *ext;
>         int ret, err = 0;
>         FILE *f;
>
> @@ -7196,35 +7194,51 @@ static int bpf_object__read_kallsyms_file(struct bpf_object *obj)
>                 if (ret != 3) {
>                         pr_warn("failed to read kallsyms entry: %d\n", ret);
>                         err = -EINVAL;
> -                       goto out;
> +                       break;
>                 }
>
> -               ext = find_extern_by_name(obj, sym_name);
> -               if (!ext || ext->type != EXT_KSYM)
> -                       continue;
> -
> -               t = btf__type_by_id(obj->btf, ext->btf_id);
> -               if (!btf_is_var(t))
> -                       continue;
> -
> -               if (ext->is_set && ext->ksym.addr != sym_addr) {
> -                       pr_warn("extern (ksym) '%s' resolution is ambiguous: 0x%llx or 0x%llx\n",
> -                               sym_name, ext->ksym.addr, sym_addr);
> -                       err = -EINVAL;
> -                       goto out;
> -               }
> -               if (!ext->is_set) {
> -                       ext->is_set = true;
> -                       ext->ksym.addr = sym_addr;
> -                       pr_debug("extern (ksym) %s=0x%llx\n", sym_name, sym_addr);
> -               }
> +               err = cb(sym_addr, sym_type, sym_name, ctx);
> +               if (err)
> +                       break;
>         }
>
> -out:
>         fclose(f);
>         return err;
>  }
>
> +static int kallsyms_cb(unsigned long long sym_addr, char sym_type,
> +                      const char *sym_name, void *ctx)
> +{
> +       struct bpf_object *obj = ctx;
> +       const struct btf_type *t;
> +       struct extern_desc *ext;
> +
> +       ext = find_extern_by_name(obj, sym_name);
> +       if (!ext || ext->type != EXT_KSYM)
> +               return 0;
> +
> +       t = btf__type_by_id(obj->btf, ext->btf_id);
> +       if (!btf_is_var(t))
> +               return 0;
> +
> +       if (ext->is_set && ext->ksym.addr != sym_addr) {
> +               pr_warn("extern (ksym) '%s' resolution is ambiguous: 0x%llx or 0x%llx\n",
> +                       sym_name, ext->ksym.addr, sym_addr);
> +               return -EINVAL;
> +       }
> +       if (!ext->is_set) {
> +               ext->is_set = true;
> +               ext->ksym.addr = sym_addr;
> +               pr_debug("extern (ksym) %s=0x%llx\n", sym_name, sym_addr);
> +       }
> +       return 0;
> +}
> +
> +static int bpf_object__read_kallsyms_file(struct bpf_object *obj)
> +{
> +       return libbpf_kallsyms_parse(kallsyms_cb, obj);
> +}
> +
>  static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
>                             __u16 kind, struct btf **res_btf,
>                             struct module_btf **res_mod_btf)
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index 4fda8bdf0a0d..b6247dc7f8eb 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -449,6 +449,11 @@ __s32 btf__find_by_name_kind_own(const struct btf *btf, const char *type_name,
>
>  extern enum libbpf_strict_mode libbpf_mode;
>
> +typedef int (*kallsyms_cb_t)(unsigned long long sym_addr, char sym_type,
> +                            const char *sym_name, void *ctx);
> +
> +int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *arg);
> +
>  /* handle direct returned errors */
>  static inline int libbpf_err(int ret)
>  {
> --
> 2.35.1
>

[PATCH 07/10] libbpf: Add bpf_link_create support for multi kprobes

[Jiri Olsa]

Adding new kprobe_multi struct to bpf_link_create_opts object
to pass multiple kprobe data to link_create attr uapi.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/lib/bpf/bpf.c | 7 +++++++
 tools/lib/bpf/bpf.h | 9 ++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 418b259166f8..5e180def2cef 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -853,6 +853,13 @@ int bpf_link_create(int prog_fd, int target_fd,
 		if (!OPTS_ZEROED(opts, perf_event))
 			return libbpf_err(-EINVAL);
 		break;
+	case BPF_TRACE_KPROBE_MULTI:
+		attr.link_create.kprobe_multi.syms = OPTS_GET(opts, kprobe_multi.syms, 0);
+		attr.link_create.kprobe_multi.addrs = OPTS_GET(opts, kprobe_multi.addrs, 0);
+		attr.link_create.kprobe_multi.cookies = OPTS_GET(opts, kprobe_multi.cookies, 0);
+		attr.link_create.kprobe_multi.cnt = OPTS_GET(opts, kprobe_multi.cnt, 0);
+		attr.link_create.kprobe_multi.flags = OPTS_GET(opts, kprobe_multi.flags, 0);
+		break;
 	default:
 		if (!OPTS_ZEROED(opts, flags))
 			return libbpf_err(-EINVAL);
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 16b21757b8bf..bd285a8f3420 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -413,10 +413,17 @@ struct bpf_link_create_opts {
 		struct {
 			__u64 bpf_cookie;
 		} perf_event;
+		struct {
+			__u64 syms;
+			__u64 addrs;
+			__u64 cookies;
+			__u32 cnt;
+			__u32 flags;
+		} kprobe_multi;
 	};
 	size_t :0;
 };
-#define bpf_link_create_opts__last_field perf_event
+#define bpf_link_create_opts__last_field kprobe_multi.flags
 
 LIBBPF_API int bpf_link_create(int prog_fd, int target_fd,
 			       enum bpf_attach_type attach_type,
-- 
2.35.1

Re: [PATCH 07/10] libbpf: Add bpf_link_create support for multi kprobes

[Andrii Nakryiko]

On Tue, Feb 22, 2022 at 9:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding new kprobe_multi struct to bpf_link_create_opts object
> to pass multiple kprobe data to link_create attr uapi.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/lib/bpf/bpf.c | 7 +++++++
>  tools/lib/bpf/bpf.h | 9 ++++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 418b259166f8..5e180def2cef 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -853,6 +853,13 @@ int bpf_link_create(int prog_fd, int target_fd,
>                 if (!OPTS_ZEROED(opts, perf_event))
>                         return libbpf_err(-EINVAL);
>                 break;
> +       case BPF_TRACE_KPROBE_MULTI:
> +               attr.link_create.kprobe_multi.syms = OPTS_GET(opts, kprobe_multi.syms, 0);
> +               attr.link_create.kprobe_multi.addrs = OPTS_GET(opts, kprobe_multi.addrs, 0);
> +               attr.link_create.kprobe_multi.cookies = OPTS_GET(opts, kprobe_multi.cookies, 0);
> +               attr.link_create.kprobe_multi.cnt = OPTS_GET(opts, kprobe_multi.cnt, 0);
> +               attr.link_create.kprobe_multi.flags = OPTS_GET(opts, kprobe_multi.flags, 0);
> +               break;
>         default:
>                 if (!OPTS_ZEROED(opts, flags))
>                         return libbpf_err(-EINVAL);
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index 16b21757b8bf..bd285a8f3420 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -413,10 +413,17 @@ struct bpf_link_create_opts {
>                 struct {
>                         __u64 bpf_cookie;
>                 } perf_event;
> +               struct {
> +                       __u64 syms;
> +                       __u64 addrs;
> +                       __u64 cookies;

hm, I think we can and should use proper types here, no?

const char **syms;
const void **addrs;
const __u64 *cookies;

?




> +                       __u32 cnt;
> +                       __u32 flags;
> +               } kprobe_multi;
>         };
>         size_t :0;
>  };
> -#define bpf_link_create_opts__last_field perf_event
> +#define bpf_link_create_opts__last_field kprobe_multi.flags
>
>  LIBBPF_API int bpf_link_create(int prog_fd, int target_fd,
>                                enum bpf_attach_type attach_type,
> --
> 2.35.1
>

Re: [PATCH 07/10] libbpf: Add bpf_link_create support for multi kprobes

[Jiri Olsa]

On Fri, Mar 04, 2022 at 03:11:16PM -0800, Andrii Nakryiko wrote:
> On Tue, Feb 22, 2022 at 9:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding new kprobe_multi struct to bpf_link_create_opts object
> > to pass multiple kprobe data to link_create attr uapi.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/lib/bpf/bpf.c | 7 +++++++
> >  tools/lib/bpf/bpf.h | 9 ++++++++-
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > index 418b259166f8..5e180def2cef 100644
> > --- a/tools/lib/bpf/bpf.c
> > +++ b/tools/lib/bpf/bpf.c
> > @@ -853,6 +853,13 @@ int bpf_link_create(int prog_fd, int target_fd,
> >                 if (!OPTS_ZEROED(opts, perf_event))
> >                         return libbpf_err(-EINVAL);
> >                 break;
> > +       case BPF_TRACE_KPROBE_MULTI:
> > +               attr.link_create.kprobe_multi.syms = OPTS_GET(opts, kprobe_multi.syms, 0);
> > +               attr.link_create.kprobe_multi.addrs = OPTS_GET(opts, kprobe_multi.addrs, 0);
> > +               attr.link_create.kprobe_multi.cookies = OPTS_GET(opts, kprobe_multi.cookies, 0);
> > +               attr.link_create.kprobe_multi.cnt = OPTS_GET(opts, kprobe_multi.cnt, 0);
> > +               attr.link_create.kprobe_multi.flags = OPTS_GET(opts, kprobe_multi.flags, 0);
> > +               break;
> >         default:
> >                 if (!OPTS_ZEROED(opts, flags))
> >                         return libbpf_err(-EINVAL);
> > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> > index 16b21757b8bf..bd285a8f3420 100644
> > --- a/tools/lib/bpf/bpf.h
> > +++ b/tools/lib/bpf/bpf.h
> > @@ -413,10 +413,17 @@ struct bpf_link_create_opts {
> >                 struct {
> >                         __u64 bpf_cookie;
> >                 } perf_event;
> > +               struct {
> > +                       __u64 syms;
> > +                       __u64 addrs;
> > +                       __u64 cookies;
> 
> hm, I think we can and should use proper types here, no?
> 
> const char **syms;
> const void **addrs;
> const __u64 *cookies;
> 
> ?

right, will change

thanks,
jirka

[PATCH 08/10] libbpf: Add bpf_program__attach_kprobe_opts support for multi kprobes

[Jiri Olsa]

Adding support to bpf_program__attach_kprobe_opts to attach kprobes
to multiple functions.

If the kprobe program has BPF_TRACE_KPROBE_MULTI as expected_attach_type
it will use the new kprobe_multi link to attach the program. In this case
it will use 'func_name' as pattern for functions to attach.

Adding also new section types 'kprobe.multi' and kretprobe.multi'
that allows to specify wildcards (*?) for functions, like:

  SEC("kprobe.multi/bpf_fentry_test*")
  SEC("kretprobe.multi/bpf_fentry_test?")

This will set kprobe's expected_attach_type to BPF_TRACE_KPROBE_MULTI,
and attach it to functions provided by the function pattern.

Using glob_match from selftests/bpf/test_progs.c and adding support to
match '?' based on original perf code.

Cc: Masami Hiramatsu <mhiramat@redhat.com>
Cc: Yucong Sun <fallentree@fb.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/lib/bpf/libbpf.c | 130 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 125 insertions(+), 5 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index fb530b004a0d..9bee2d70b99d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -8622,6 +8622,8 @@ static const struct bpf_sec_def section_defs[] = {
 	SEC_DEF("uprobe/",		KPROBE,	0, SEC_NONE),
 	SEC_DEF("kretprobe/",		KPROBE, 0, SEC_NONE, attach_kprobe),
 	SEC_DEF("uretprobe/",		KPROBE, 0, SEC_NONE),
+	SEC_DEF("kprobe.multi/",	KPROBE,	BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe),
+	SEC_DEF("kretprobe.multi/",	KPROBE,	BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe),
 	SEC_DEF("tc",			SCHED_CLS, 0, SEC_NONE),
 	SEC_DEF("classifier",		SCHED_CLS, 0, SEC_NONE | SEC_SLOPPY_PFX | SEC_DEPRECATED),
 	SEC_DEF("action",		SCHED_ACT, 0, SEC_NONE | SEC_SLOPPY_PFX),
@@ -10038,6 +10040,113 @@ static int perf_event_kprobe_open_legacy(const char *probe_name, bool retprobe,
 	return pfd;
 }
 
+/* Adapted from perf/util/string.c */
+static bool glob_match(const char *str, const char *pat)
+{
+	while (*str && *pat && *pat != '*') {
+		if (*pat == '?') {      /* Matches any single character */
+			str++;
+			pat++;
+			continue;
+		}
+		if (*str != *pat)
+			return false;
+		str++;
+		pat++;
+	}
+	/* Check wild card */
+	if (*pat == '*') {
+		while (*pat == '*')
+			pat++;
+		if (!*pat) /* Tail wild card matches all */
+			return true;
+		while (*str)
+			if (glob_match(str++, pat))
+				return true;
+	}
+	return !*str && !*pat;
+}
+
+struct kprobe_multi_resolve {
+	const char *name;
+	__u64 *addrs;
+	size_t cap;
+	size_t cnt;
+};
+
+static int
+resolve_kprobe_multi_cb(unsigned long long sym_addr, char sym_type,
+			const char *sym_name, void *ctx)
+{
+	struct kprobe_multi_resolve *res = ctx;
+	int err;
+
+	if (!glob_match(sym_name, res->name))
+		return 0;
+
+	err = libbpf_ensure_mem((void **)&res->addrs, &res->cap, sizeof(__u64),
+				res->cnt + 1);
+	if (err)
+		return err;
+
+	res->addrs[res->cnt++] = sym_addr;
+	return 0;
+}
+
+static struct bpf_link *
+attach_kprobe_multi_opts(const struct bpf_program *prog,
+		   const char *func_pattern,
+		   const struct bpf_kprobe_opts *kopts)
+{
+	DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts);
+	struct kprobe_multi_resolve res = {
+		.name = func_pattern,
+	};
+	struct bpf_link *link = NULL;
+	char errmsg[STRERR_BUFSIZE];
+	int err, link_fd, prog_fd;
+	bool retprobe;
+
+	err = libbpf_kallsyms_parse(resolve_kprobe_multi_cb, &res);
+	if (err)
+		goto error;
+	if (!res.cnt) {
+		err = -ENOENT;
+		goto error;
+	}
+
+	retprobe = OPTS_GET(kopts, retprobe, false);
+
+	opts.kprobe_multi.addrs = ptr_to_u64(res.addrs);
+	opts.kprobe_multi.cnt = res.cnt;
+	opts.flags = retprobe ? BPF_F_KPROBE_MULTI_RETURN : 0;
+
+	link = calloc(1, sizeof(*link));
+	if (!link) {
+		err = -ENOMEM;
+		goto error;
+	}
+	link->detach = &bpf_link__detach_fd;
+
+	prog_fd = bpf_program__fd(prog);
+	link_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, &opts);
+	if (link_fd < 0) {
+		err = -errno;
+		pr_warn("prog '%s': failed to attach to %s: %s\n",
+			prog->name, res.name,
+			libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
+		goto error;
+	}
+	link->fd = link_fd;
+	free(res.addrs);
+	return link;
+
+error:
+	free(link);
+	free(res.addrs);
+	return libbpf_err_ptr(err);
+}
+
 struct bpf_link *
 bpf_program__attach_kprobe_opts(const struct bpf_program *prog,
 				const char *func_name,
@@ -10054,6 +10163,9 @@ bpf_program__attach_kprobe_opts(const struct bpf_program *prog,
 	if (!OPTS_VALID(opts, bpf_kprobe_opts))
 		return libbpf_err_ptr(-EINVAL);
 
+	if (prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI)
+		return attach_kprobe_multi_opts(prog, func_name, opts);
+
 	retprobe = OPTS_GET(opts, retprobe, false);
 	offset = OPTS_GET(opts, offset, 0);
 	pe_opts.bpf_cookie = OPTS_GET(opts, bpf_cookie, 0);
@@ -10122,19 +10234,27 @@ struct bpf_link *bpf_program__attach_kprobe(const struct bpf_program *prog,
 static struct bpf_link *attach_kprobe(const struct bpf_program *prog, long cookie)
 {
 	DECLARE_LIBBPF_OPTS(bpf_kprobe_opts, opts);
+	const char *func_name = NULL;
 	unsigned long offset = 0;
 	struct bpf_link *link;
-	const char *func_name;
 	char *func;
 	int n, err;
 
-	opts.retprobe = str_has_pfx(prog->sec_name, "kretprobe/");
-	if (opts.retprobe)
+	opts.retprobe = str_has_pfx(prog->sec_name, "kretprobe");
+
+	if (str_has_pfx(prog->sec_name, "kretprobe/"))
 		func_name = prog->sec_name + sizeof("kretprobe/") - 1;
-	else
+	else if (str_has_pfx(prog->sec_name, "kprobe/"))
 		func_name = prog->sec_name + sizeof("kprobe/") - 1;
+	else if (str_has_pfx(prog->sec_name, "kretprobe.multi/"))
+		func_name = prog->sec_name + sizeof("kretprobe.multi/") - 1;
+	else if (str_has_pfx(prog->sec_name, "kprobe.multi/"))
+		func_name = prog->sec_name + sizeof("kprobe.multi/") - 1;
+
+	if (!func_name)
+		return libbpf_err_ptr(-EINVAL);
 
-	n = sscanf(func_name, "%m[a-zA-Z0-9_.]+%li", &func, &offset);
+	n = sscanf(func_name, "%m[a-zA-Z0-9_.*?]+%li", &func, &offset);
 	if (n < 1) {
 		err = -EINVAL;
 		pr_warn("kprobe name is invalid: %s\n", func_name);
-- 
2.35.1

Re: [PATCH 08/10] libbpf: Add bpf_program__attach_kprobe_opts support for multi kprobes

[Andrii Nakryiko]

On Tue, Feb 22, 2022 at 9:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support to bpf_program__attach_kprobe_opts to attach kprobes
> to multiple functions.
>
> If the kprobe program has BPF_TRACE_KPROBE_MULTI as expected_attach_type
> it will use the new kprobe_multi link to attach the program. In this case
> it will use 'func_name' as pattern for functions to attach.
>
> Adding also new section types 'kprobe.multi' and kretprobe.multi'
> that allows to specify wildcards (*?) for functions, like:
>
>   SEC("kprobe.multi/bpf_fentry_test*")
>   SEC("kretprobe.multi/bpf_fentry_test?")
>
> This will set kprobe's expected_attach_type to BPF_TRACE_KPROBE_MULTI,
> and attach it to functions provided by the function pattern.
>
> Using glob_match from selftests/bpf/test_progs.c and adding support to
> match '?' based on original perf code.
>
> Cc: Masami Hiramatsu <mhiramat@redhat.com>
> Cc: Yucong Sun <fallentree@fb.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/lib/bpf/libbpf.c | 130 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 125 insertions(+), 5 deletions(-)
>

[...]

> +static struct bpf_link *
> +attach_kprobe_multi_opts(const struct bpf_program *prog,
> +                  const char *func_pattern,
> +                  const struct bpf_kprobe_opts *kopts)
> +{
> +       DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts);

nit: just LIBBPF_OPTS


> +       struct kprobe_multi_resolve res = {
> +               .name = func_pattern,
> +       };
> +       struct bpf_link *link = NULL;
> +       char errmsg[STRERR_BUFSIZE];
> +       int err, link_fd, prog_fd;
> +       bool retprobe;
> +
> +       err = libbpf_kallsyms_parse(resolve_kprobe_multi_cb, &res);

hm... I think as a generic API we should support three modes of
specifying attachment target:


1. glob-based (very convenient, I agree)
2. array of function names (very convenient when I know specific set
of functions)
3. array of addresses (advanced use case, so probably will be rarely used).



So I wonder if it's better to have a separate
bpf_program__attach_kprobe_multi() API for this, instead of doing both
inside bpf_program__attach_kprobe()...

In such case bpf_program__attach_kprobe() could either fail if
expected attach type is BPF_TRACE_KPROBE_MULTI or it can redirect to
attach_kprobe_multi with func_name as a pattern or just single
function (let's think which one makes more sense)

Let's at least think about this


> +       if (err)
> +               goto error;
> +       if (!res.cnt) {
> +               err = -ENOENT;
> +               goto error;
> +       }
> +
> +       retprobe = OPTS_GET(kopts, retprobe, false);
> +
> +       opts.kprobe_multi.addrs = ptr_to_u64(res.addrs);
> +       opts.kprobe_multi.cnt = res.cnt;
> +       opts.flags = retprobe ? BPF_F_KPROBE_MULTI_RETURN : 0;

this should be opts.kprobe_multi.flags

> +
> +       link = calloc(1, sizeof(*link));
> +       if (!link) {
> +               err = -ENOMEM;
> +               goto error;
> +       }
> +       link->detach = &bpf_link__detach_fd;
> +
> +       prog_fd = bpf_program__fd(prog);
> +       link_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, &opts);
> +       if (link_fd < 0) {
> +               err = -errno;
> +               pr_warn("prog '%s': failed to attach to %s: %s\n",

"to attach multi-kprobe for '%s': %s" ?

> +                       prog->name, res.name,
> +                       libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> +               goto error;
> +       }
> +       link->fd = link_fd;
> +       free(res.addrs);
> +       return link;
> +
> +error:
> +       free(link);
> +       free(res.addrs);
> +       return libbpf_err_ptr(err);
> +}
> +
>  struct bpf_link *
>  bpf_program__attach_kprobe_opts(const struct bpf_program *prog,
>                                 const char *func_name,
> @@ -10054,6 +10163,9 @@ bpf_program__attach_kprobe_opts(const struct bpf_program *prog,
>         if (!OPTS_VALID(opts, bpf_kprobe_opts))
>                 return libbpf_err_ptr(-EINVAL);
>
> +       if (prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI)
> +               return attach_kprobe_multi_opts(prog, func_name, opts);
> +
>         retprobe = OPTS_GET(opts, retprobe, false);
>         offset = OPTS_GET(opts, offset, 0);
>         pe_opts.bpf_cookie = OPTS_GET(opts, bpf_cookie, 0);

see how you don't support cookies (plural) and this offset doesn't
make sense for multi-kprobe. Separate API is necessary to expose all
the possibilities and functionality.

> @@ -10122,19 +10234,27 @@ struct bpf_link *bpf_program__attach_kprobe(const struct bpf_program *prog,
>  static struct bpf_link *attach_kprobe(const struct bpf_program *prog, long cookie)
>  {
>         DECLARE_LIBBPF_OPTS(bpf_kprobe_opts, opts);
> +       const char *func_name = NULL;
>         unsigned long offset = 0;
>         struct bpf_link *link;
> -       const char *func_name;
>         char *func;
>         int n, err;
>
> -       opts.retprobe = str_has_pfx(prog->sec_name, "kretprobe/");
> -       if (opts.retprobe)
> +       opts.retprobe = str_has_pfx(prog->sec_name, "kretprobe");
> +
> +       if (str_has_pfx(prog->sec_name, "kretprobe/"))
>                 func_name = prog->sec_name + sizeof("kretprobe/") - 1;
> -       else
> +       else if (str_has_pfx(prog->sec_name, "kprobe/"))
>                 func_name = prog->sec_name + sizeof("kprobe/") - 1;
> +       else if (str_has_pfx(prog->sec_name, "kretprobe.multi/"))
> +               func_name = prog->sec_name + sizeof("kretprobe.multi/") - 1;
> +       else if (str_has_pfx(prog->sec_name, "kprobe.multi/"))
> +               func_name = prog->sec_name + sizeof("kprobe.multi/") - 1;

starts to feel that we should find '/' and then do strcmp(), instead
of this duplication of strings?

> +
> +       if (!func_name)
> +               return libbpf_err_ptr(-EINVAL);
>
> -       n = sscanf(func_name, "%m[a-zA-Z0-9_.]+%li", &func, &offset);
> +       n = sscanf(func_name, "%m[a-zA-Z0-9_.*?]+%li", &func, &offset);

'*' and '?' are still invalid for non-multi-kprobe...


>         if (n < 1) {
>                 err = -EINVAL;
>                 pr_warn("kprobe name is invalid: %s\n", func_name);
> --
> 2.35.1
>

Re: [PATCH 08/10] libbpf: Add bpf_program__attach_kprobe_opts support for multi kprobes

[Jiri Olsa]

On Fri, Mar 04, 2022 at 03:11:19PM -0800, Andrii Nakryiko wrote:
> On Tue, Feb 22, 2022 at 9:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding support to bpf_program__attach_kprobe_opts to attach kprobes
> > to multiple functions.
> >
> > If the kprobe program has BPF_TRACE_KPROBE_MULTI as expected_attach_type
> > it will use the new kprobe_multi link to attach the program. In this case
> > it will use 'func_name' as pattern for functions to attach.
> >
> > Adding also new section types 'kprobe.multi' and kretprobe.multi'
> > that allows to specify wildcards (*?) for functions, like:
> >
> >   SEC("kprobe.multi/bpf_fentry_test*")
> >   SEC("kretprobe.multi/bpf_fentry_test?")
> >
> > This will set kprobe's expected_attach_type to BPF_TRACE_KPROBE_MULTI,
> > and attach it to functions provided by the function pattern.
> >
> > Using glob_match from selftests/bpf/test_progs.c and adding support to
> > match '?' based on original perf code.
> >
> > Cc: Masami Hiramatsu <mhiramat@redhat.com>
> > Cc: Yucong Sun <fallentree@fb.com>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/lib/bpf/libbpf.c | 130 +++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 125 insertions(+), 5 deletions(-)
> >
> 
> [...]
> 
> > +static struct bpf_link *
> > +attach_kprobe_multi_opts(const struct bpf_program *prog,
> > +                  const char *func_pattern,
> > +                  const struct bpf_kprobe_opts *kopts)
> > +{
> > +       DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts);
> 
> nit: just LIBBPF_OPTS

ok

> 
> 
> > +       struct kprobe_multi_resolve res = {
> > +               .name = func_pattern,
> > +       };
> > +       struct bpf_link *link = NULL;
> > +       char errmsg[STRERR_BUFSIZE];
> > +       int err, link_fd, prog_fd;
> > +       bool retprobe;
> > +
> > +       err = libbpf_kallsyms_parse(resolve_kprobe_multi_cb, &res);
> 
> hm... I think as a generic API we should support three modes of
> specifying attachment target:
> 
> 
> 1. glob-based (very convenient, I agree)
> 2. array of function names (very convenient when I know specific set
> of functions)
> 3. array of addresses (advanced use case, so probably will be rarely used).
> 
> 
> 
> So I wonder if it's better to have a separate
> bpf_program__attach_kprobe_multi() API for this, instead of doing both
> inside bpf_program__attach_kprobe()...
> 
> In such case bpf_program__attach_kprobe() could either fail if
> expected attach type is BPF_TRACE_KPROBE_MULTI or it can redirect to
> attach_kprobe_multi with func_name as a pattern or just single
> function (let's think which one makes more sense)
> 
> Let's at least think about this

I think it would make the code more clear, how about this:

	struct bpf_kprobe_multi_opts {
		/* size of this struct, for forward/backward compatiblity */
		size_t sz;

		const char **funcs;
		const unsigned long *addrs;
		const u64 *cookies;
		int cnt;
		bool retprobe;
		size_t :0;
	};

	bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
					      const char *pattern,
					      const struct bpf_kprobe_multi_opts *opts);


if pattern is NULL we'd use opts data:

	bpf_program__attach_kprobe_multi_opts(prog, "ksys_*", NULL);
	bpf_program__attach_kprobe_multi_opts(prog, NULL, &opts);

to have '2. array of function names' as direct function argument,
we'd need to add 'cnt' as well, so I think it's better to have it
in opts, and have just pattern for quick/convenient call without opts

> 
> 
> > +       if (err)
> > +               goto error;
> > +       if (!res.cnt) {
> > +               err = -ENOENT;
> > +               goto error;
> > +       }
> > +
> > +       retprobe = OPTS_GET(kopts, retprobe, false);
> > +
> > +       opts.kprobe_multi.addrs = ptr_to_u64(res.addrs);
> > +       opts.kprobe_multi.cnt = res.cnt;
> > +       opts.flags = retprobe ? BPF_F_KPROBE_MULTI_RETURN : 0;
> 
> this should be opts.kprobe_multi.flags

ugh, now I'm curious how kretprobes passed in tests ;-)

> 
> > +
> > +       link = calloc(1, sizeof(*link));
> > +       if (!link) {
> > +               err = -ENOMEM;
> > +               goto error;
> > +       }
> > +       link->detach = &bpf_link__detach_fd;
> > +
> > +       prog_fd = bpf_program__fd(prog);
> > +       link_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, &opts);
> > +       if (link_fd < 0) {
> > +               err = -errno;
> > +               pr_warn("prog '%s': failed to attach to %s: %s\n",
> 
> "to attach multi-kprobe for '%s': %s" ?

ok

> 
> > +                       prog->name, res.name,
> > +                       libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> > +               goto error;
> > +       }
> > +       link->fd = link_fd;
> > +       free(res.addrs);
> > +       return link;
> > +
> > +error:
> > +       free(link);
> > +       free(res.addrs);
> > +       return libbpf_err_ptr(err);
> > +}
> > +
> >  struct bpf_link *
> >  bpf_program__attach_kprobe_opts(const struct bpf_program *prog,
> >                                 const char *func_name,
> > @@ -10054,6 +10163,9 @@ bpf_program__attach_kprobe_opts(const struct bpf_program *prog,
> >         if (!OPTS_VALID(opts, bpf_kprobe_opts))
> >                 return libbpf_err_ptr(-EINVAL);
> >
> > +       if (prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI)
> > +               return attach_kprobe_multi_opts(prog, func_name, opts);
> > +
> >         retprobe = OPTS_GET(opts, retprobe, false);
> >         offset = OPTS_GET(opts, offset, 0);
> >         pe_opts.bpf_cookie = OPTS_GET(opts, bpf_cookie, 0);
> 
> see how you don't support cookies (plural) and this offset doesn't
> make sense for multi-kprobe. Separate API is necessary to expose all
> the possibilities and functionality.
> 
> > @@ -10122,19 +10234,27 @@ struct bpf_link *bpf_program__attach_kprobe(const struct bpf_program *prog,
> >  static struct bpf_link *attach_kprobe(const struct bpf_program *prog, long cookie)
> >  {
> >         DECLARE_LIBBPF_OPTS(bpf_kprobe_opts, opts);
> > +       const char *func_name = NULL;
> >         unsigned long offset = 0;
> >         struct bpf_link *link;
> > -       const char *func_name;
> >         char *func;
> >         int n, err;
> >
> > -       opts.retprobe = str_has_pfx(prog->sec_name, "kretprobe/");
> > -       if (opts.retprobe)
> > +       opts.retprobe = str_has_pfx(prog->sec_name, "kretprobe");
> > +
> > +       if (str_has_pfx(prog->sec_name, "kretprobe/"))
> >                 func_name = prog->sec_name + sizeof("kretprobe/") - 1;
> > -       else
> > +       else if (str_has_pfx(prog->sec_name, "kprobe/"))
> >                 func_name = prog->sec_name + sizeof("kprobe/") - 1;
> > +       else if (str_has_pfx(prog->sec_name, "kretprobe.multi/"))
> > +               func_name = prog->sec_name + sizeof("kretprobe.multi/") - 1;
> > +       else if (str_has_pfx(prog->sec_name, "kprobe.multi/"))
> > +               func_name = prog->sec_name + sizeof("kprobe.multi/") - 1;
> 
> starts to feel that we should find '/' and then do strcmp(), instead
> of this duplication of strings?

ok, another reason to separate the api

thanks,
jirka

Re: [PATCH 08/10] libbpf: Add bpf_program__attach_kprobe_opts support for multi kprobes

[Andrii Nakryiko]

On Sun, Mar 6, 2022 at 9:29 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Fri, Mar 04, 2022 at 03:11:19PM -0800, Andrii Nakryiko wrote:
> > On Tue, Feb 22, 2022 at 9:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Adding support to bpf_program__attach_kprobe_opts to attach kprobes
> > > to multiple functions.
> > >
> > > If the kprobe program has BPF_TRACE_KPROBE_MULTI as expected_attach_type
> > > it will use the new kprobe_multi link to attach the program. In this case
> > > it will use 'func_name' as pattern for functions to attach.
> > >
> > > Adding also new section types 'kprobe.multi' and kretprobe.multi'
> > > that allows to specify wildcards (*?) for functions, like:
> > >
> > >   SEC("kprobe.multi/bpf_fentry_test*")
> > >   SEC("kretprobe.multi/bpf_fentry_test?")
> > >
> > > This will set kprobe's expected_attach_type to BPF_TRACE_KPROBE_MULTI,
> > > and attach it to functions provided by the function pattern.
> > >
> > > Using glob_match from selftests/bpf/test_progs.c and adding support to
> > > match '?' based on original perf code.
> > >
> > > Cc: Masami Hiramatsu <mhiramat@redhat.com>
> > > Cc: Yucong Sun <fallentree@fb.com>
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  tools/lib/bpf/libbpf.c | 130 +++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 125 insertions(+), 5 deletions(-)
> > >
> >
> > [...]
> >
> > > +static struct bpf_link *
> > > +attach_kprobe_multi_opts(const struct bpf_program *prog,
> > > +                  const char *func_pattern,
> > > +                  const struct bpf_kprobe_opts *kopts)
> > > +{
> > > +       DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts);
> >
> > nit: just LIBBPF_OPTS
>
> ok
>
> >
> >
> > > +       struct kprobe_multi_resolve res = {
> > > +               .name = func_pattern,
> > > +       };
> > > +       struct bpf_link *link = NULL;
> > > +       char errmsg[STRERR_BUFSIZE];
> > > +       int err, link_fd, prog_fd;
> > > +       bool retprobe;
> > > +
> > > +       err = libbpf_kallsyms_parse(resolve_kprobe_multi_cb, &res);
> >
> > hm... I think as a generic API we should support three modes of
> > specifying attachment target:
> >
> >
> > 1. glob-based (very convenient, I agree)
> > 2. array of function names (very convenient when I know specific set
> > of functions)
> > 3. array of addresses (advanced use case, so probably will be rarely used).
> >
> >
> >
> > So I wonder if it's better to have a separate
> > bpf_program__attach_kprobe_multi() API for this, instead of doing both
> > inside bpf_program__attach_kprobe()...
> >
> > In such case bpf_program__attach_kprobe() could either fail if
> > expected attach type is BPF_TRACE_KPROBE_MULTI or it can redirect to
> > attach_kprobe_multi with func_name as a pattern or just single
> > function (let's think which one makes more sense)
> >
> > Let's at least think about this
>
> I think it would make the code more clear, how about this:
>
>         struct bpf_kprobe_multi_opts {
>                 /* size of this struct, for forward/backward compatiblity */
>                 size_t sz;
>
>                 const char **funcs;

naming nit: func_names (to oppose it to "func_pattern")? Or just
"names" to be in line with "addrs" (but then "pattern" instead of
"func_pattern"? with kprobe it's always about functions, so this
"func_" everywhere is a bit redundant)

>                 const unsigned long *addrs;
>                 const u64 *cookies;
>                 int cnt;

nit: let's use size_t


>                 bool retprobe;
>                 size_t :0;
>         };
>
>         bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
>                                               const char *pattern,
>                                               const struct bpf_kprobe_multi_opts *opts);
>
>
> if pattern is NULL we'd use opts data:
>
>         bpf_program__attach_kprobe_multi_opts(prog, "ksys_*", NULL);
>         bpf_program__attach_kprobe_multi_opts(prog, NULL, &opts);
>
> to have '2. array of function names' as direct function argument,
> we'd need to add 'cnt' as well, so I think it's better to have it
> in opts, and have just pattern for quick/convenient call without opts
>

yeah, naming pattern as direct argument for common use case makes
sense. Let's go with this scheme


[...]

Re: [PATCH 08/10] libbpf: Add bpf_program__attach_kprobe_opts support for multi kprobes

[Jiri Olsa]

On Mon, Mar 07, 2022 at 05:28:54PM -0800, Andrii Nakryiko wrote:
> On Sun, Mar 6, 2022 at 9:29 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Fri, Mar 04, 2022 at 03:11:19PM -0800, Andrii Nakryiko wrote:
> > > On Tue, Feb 22, 2022 at 9:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > >
> > > > Adding support to bpf_program__attach_kprobe_opts to attach kprobes
> > > > to multiple functions.
> > > >
> > > > If the kprobe program has BPF_TRACE_KPROBE_MULTI as expected_attach_type
> > > > it will use the new kprobe_multi link to attach the program. In this case
> > > > it will use 'func_name' as pattern for functions to attach.
> > > >
> > > > Adding also new section types 'kprobe.multi' and kretprobe.multi'
> > > > that allows to specify wildcards (*?) for functions, like:
> > > >
> > > >   SEC("kprobe.multi/bpf_fentry_test*")
> > > >   SEC("kretprobe.multi/bpf_fentry_test?")
> > > >
> > > > This will set kprobe's expected_attach_type to BPF_TRACE_KPROBE_MULTI,
> > > > and attach it to functions provided by the function pattern.
> > > >
> > > > Using glob_match from selftests/bpf/test_progs.c and adding support to
> > > > match '?' based on original perf code.
> > > >
> > > > Cc: Masami Hiramatsu <mhiramat@redhat.com>
> > > > Cc: Yucong Sun <fallentree@fb.com>
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >  tools/lib/bpf/libbpf.c | 130 +++++++++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 125 insertions(+), 5 deletions(-)
> > > >
> > >
> > > [...]
> > >
> > > > +static struct bpf_link *
> > > > +attach_kprobe_multi_opts(const struct bpf_program *prog,
> > > > +                  const char *func_pattern,
> > > > +                  const struct bpf_kprobe_opts *kopts)
> > > > +{
> > > > +       DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts);
> > >
> > > nit: just LIBBPF_OPTS
> >
> > ok
> >
> > >
> > >
> > > > +       struct kprobe_multi_resolve res = {
> > > > +               .name = func_pattern,
> > > > +       };
> > > > +       struct bpf_link *link = NULL;
> > > > +       char errmsg[STRERR_BUFSIZE];
> > > > +       int err, link_fd, prog_fd;
> > > > +       bool retprobe;
> > > > +
> > > > +       err = libbpf_kallsyms_parse(resolve_kprobe_multi_cb, &res);
> > >
> > > hm... I think as a generic API we should support three modes of
> > > specifying attachment target:
> > >
> > >
> > > 1. glob-based (very convenient, I agree)
> > > 2. array of function names (very convenient when I know specific set
> > > of functions)
> > > 3. array of addresses (advanced use case, so probably will be rarely used).
> > >
> > >
> > >
> > > So I wonder if it's better to have a separate
> > > bpf_program__attach_kprobe_multi() API for this, instead of doing both
> > > inside bpf_program__attach_kprobe()...
> > >
> > > In such case bpf_program__attach_kprobe() could either fail if
> > > expected attach type is BPF_TRACE_KPROBE_MULTI or it can redirect to
> > > attach_kprobe_multi with func_name as a pattern or just single
> > > function (let's think which one makes more sense)
> > >
> > > Let's at least think about this
> >
> > I think it would make the code more clear, how about this:
> >
> >         struct bpf_kprobe_multi_opts {
> >                 /* size of this struct, for forward/backward compatiblity */
> >                 size_t sz;
> >
> >                 const char **funcs;
> 
> naming nit: func_names (to oppose it to "func_pattern")? Or just
> "names" to be in line with "addrs" (but then "pattern" instead of
> "func_pattern"? with kprobe it's always about functions, so this
> "func_" everywhere is a bit redundant)

ok

> 
> >                 const unsigned long *addrs;
> >                 const u64 *cookies;
> >                 int cnt;
> 
> nit: let's use size_t

ok

> 
> 
> >                 bool retprobe;
> >                 size_t :0;
> >         };
> >
> >         bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
> >                                               const char *pattern,
> >                                               const struct bpf_kprobe_multi_opts *opts);
> >
> >
> > if pattern is NULL we'd use opts data:
> >
> >         bpf_program__attach_kprobe_multi_opts(prog, "ksys_*", NULL);
> >         bpf_program__attach_kprobe_multi_opts(prog, NULL, &opts);
> >
> > to have '2. array of function names' as direct function argument,
> > we'd need to add 'cnt' as well, so I think it's better to have it
> > in opts, and have just pattern for quick/convenient call without opts
> >
> 
> yeah, naming pattern as direct argument for common use case makes
> sense. Let's go with this scheme

great, I'll make the changes

thanks,
jirka

[PATCH 09/10] selftest/bpf: Add kprobe_multi attach test

[Jiri Olsa]

Adding kprobe_multi attach test that uses new fprobe interface to
attach kprobe program to multiple functions.

The test is attaching programs to bpf_fentry_test* functions and
uses single trampoline program bpf_prog_test_run to trigger
bpf_fentry_test* functions.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../bpf/prog_tests/kprobe_multi_test.c        | 115 ++++++++++++++++++
 .../selftests/bpf/progs/kprobe_multi.c        |  58 +++++++++
 2 files changed, 173 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
 create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi.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
new file mode 100644
index 000000000000..f5567c3865d4
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include "kprobe_multi.skel.h"
+#include "trace_helpers.h"
+
+static void test_skel_api(void)
+{
+	LIBBPF_OPTS(bpf_test_run_opts, topts);
+	struct kprobe_multi *skel = NULL;
+	int err, prog_fd;
+
+	skel = kprobe_multi__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "kprobe_multi__open_and_load"))
+		goto cleanup;
+
+	err = kprobe_multi__attach(skel);
+	if (!ASSERT_OK(err, "kprobe_multi__attach"))
+		goto cleanup;
+
+	prog_fd = bpf_program__fd(skel->progs.test1);
+	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->test2_result, 8, "test2_result");
+	ASSERT_EQ(skel->bss->test3_result, 8, "test3_result");
+
+cleanup:
+	kprobe_multi__destroy(skel);
+}
+
+static void test_link_api(struct bpf_link_create_opts *opts)
+{
+	LIBBPF_OPTS(bpf_test_run_opts, topts);
+	int err, prog_fd, link1_fd = -1, link2_fd = -1;
+	struct kprobe_multi *skel = NULL;
+
+	skel = kprobe_multi__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "fentry_raw_skel_load"))
+		goto cleanup;
+
+	prog_fd = bpf_program__fd(skel->progs.test2);
+	link1_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, opts);
+	if (!ASSERT_GE(link1_fd, 0, "link_fd"))
+		goto cleanup;
+
+	opts->kprobe_multi.flags = BPF_F_KPROBE_MULTI_RETURN;
+	prog_fd = bpf_program__fd(skel->progs.test3);
+	link2_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, opts);
+	if (!ASSERT_GE(link2_fd, 0, "link_fd"))
+		goto cleanup;
+
+	skel->bss->test2_result = 0;
+	skel->bss->test3_result = 0;
+
+	prog_fd = bpf_program__fd(skel->progs.test1);
+	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->test2_result, 8, "test2_result");
+	ASSERT_EQ(skel->bss->test3_result, 8, "test3_result");
+
+cleanup:
+	if (link1_fd != -1)
+		close(link1_fd);
+	if (link2_fd != -1)
+		close(link2_fd);
+	kprobe_multi__destroy(skel);
+}
+
+static void test_link_api_addrs(void)
+{
+	DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts);
+	__u64 addrs[8];
+
+	kallsyms_find("bpf_fentry_test1", &addrs[0]);
+	kallsyms_find("bpf_fentry_test2", &addrs[1]);
+	kallsyms_find("bpf_fentry_test3", &addrs[2]);
+	kallsyms_find("bpf_fentry_test4", &addrs[3]);
+	kallsyms_find("bpf_fentry_test5", &addrs[4]);
+	kallsyms_find("bpf_fentry_test6", &addrs[5]);
+	kallsyms_find("bpf_fentry_test7", &addrs[6]);
+	kallsyms_find("bpf_fentry_test8", &addrs[7]);
+
+	opts.kprobe_multi.addrs = (__u64) addrs;
+	opts.kprobe_multi.cnt = 8;
+	test_link_api(&opts);
+}
+
+static void test_link_api_syms(void)
+{
+	DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts);
+	const char *syms[8] = {
+		"bpf_fentry_test1",
+		"bpf_fentry_test2",
+		"bpf_fentry_test3",
+		"bpf_fentry_test4",
+		"bpf_fentry_test5",
+		"bpf_fentry_test6",
+		"bpf_fentry_test7",
+		"bpf_fentry_test8",
+	};
+
+	opts.kprobe_multi.syms = (__u64) syms;
+	opts.kprobe_multi.cnt = 8;
+	test_link_api(&opts);
+}
+
+void test_kprobe_multi_test(void)
+{
+	test_skel_api();
+	test_link_api_syms();
+	test_link_api_addrs();
+}
diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi.c b/tools/testing/selftests/bpf/progs/kprobe_multi.c
new file mode 100644
index 000000000000..71318c65931c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/kprobe_multi.c
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.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;
+
+/* No tests, just to trigger bpf_fentry_test* through tracing test_run */
+SEC("fentry/bpf_modify_return_test")
+int BPF_PROG(test1)
+{
+	return 0;
+}
+
+__u64 test2_result = 0;
+
+SEC("kprobe.multi/bpf_fentry_test?")
+int test2(struct pt_regs *ctx)
+{
+	__u64 addr = bpf_get_func_ip(ctx);
+
+	test2_result += (const void *) addr == &bpf_fentry_test1 ||
+			(const void *) addr == &bpf_fentry_test2 ||
+			(const void *) addr == &bpf_fentry_test3 ||
+			(const void *) addr == &bpf_fentry_test4 ||
+			(const void *) addr == &bpf_fentry_test5 ||
+			(const void *) addr == &bpf_fentry_test6 ||
+			(const void *) addr == &bpf_fentry_test7 ||
+			(const void *) addr == &bpf_fentry_test8;
+	return 0;
+}
+
+__u64 test3_result = 0;
+
+SEC("kretprobe.multi/bpf_fentry_test*")
+int test3(struct pt_regs *ctx)
+{
+	__u64 addr = bpf_get_func_ip(ctx);
+
+	test3_result += (const void *) addr == &bpf_fentry_test1 ||
+			(const void *) addr == &bpf_fentry_test2 ||
+			(const void *) addr == &bpf_fentry_test3 ||
+			(const void *) addr == &bpf_fentry_test4 ||
+			(const void *) addr == &bpf_fentry_test5 ||
+			(const void *) addr == &bpf_fentry_test6 ||
+			(const void *) addr == &bpf_fentry_test7 ||
+			(const void *) addr == &bpf_fentry_test8;
+	return 0;
+}
-- 
2.35.1

Re: [PATCH 09/10] selftest/bpf: Add kprobe_multi attach test

[Andrii Nakryiko]

On Tue, Feb 22, 2022 at 9:08 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding kprobe_multi attach test that uses new fprobe interface to
> attach kprobe program to multiple functions.
>
> The test is attaching programs to bpf_fentry_test* functions and
> uses single trampoline program bpf_prog_test_run to trigger
> bpf_fentry_test* functions.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

subj typo: selftest -> selftests

>  .../bpf/prog_tests/kprobe_multi_test.c        | 115 ++++++++++++++++++
>  .../selftests/bpf/progs/kprobe_multi.c        |  58 +++++++++
>  2 files changed, 173 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
>  create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi.c
>

[...]

> +
> +static void test_link_api_addrs(void)
> +{
> +       DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts);
> +       __u64 addrs[8];
> +
> +       kallsyms_find("bpf_fentry_test1", &addrs[0]);
> +       kallsyms_find("bpf_fentry_test2", &addrs[1]);
> +       kallsyms_find("bpf_fentry_test3", &addrs[2]);
> +       kallsyms_find("bpf_fentry_test4", &addrs[3]);
> +       kallsyms_find("bpf_fentry_test5", &addrs[4]);
> +       kallsyms_find("bpf_fentry_test6",  &addrs[5]);
> +       kallsyms_find("bpf_fentry_test7", &addrs[6]);
> +       kallsyms_find("bpf_fentry_test8", &addrs[7]);

ASSERT_OK() that symbols are found? It also sucks that we re-parse
kallsyms so much...

maybe use load_kallsyms() to pre-cache? We should also teach
load_kallsyms() to not reload kallsyms more than once

> +
> +       opts.kprobe_multi.addrs = (__u64) addrs;
> +       opts.kprobe_multi.cnt = 8;

ARRAY_SIZE()?

> +       test_link_api(&opts);
> +}
> +
> +static void test_link_api_syms(void)
> +{
> +       DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts);

nit: just LIBBPF_OPTS

> +       const char *syms[8] = {
> +               "bpf_fentry_test1",
> +               "bpf_fentry_test2",
> +               "bpf_fentry_test3",
> +               "bpf_fentry_test4",
> +               "bpf_fentry_test5",
> +               "bpf_fentry_test6",
> +               "bpf_fentry_test7",
> +               "bpf_fentry_test8",
> +       };
> +
> +       opts.kprobe_multi.syms = (__u64) syms;
> +       opts.kprobe_multi.cnt = 8;

ARRAY_SIZE() ?

> +       test_link_api(&opts);
> +}
> +
> +void test_kprobe_multi_test(void)
> +{
> +       test_skel_api();
> +       test_link_api_syms();
> +       test_link_api_addrs();

model as subtests?


> +}
> diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi.c b/tools/testing/selftests/bpf/progs/kprobe_multi.c
> new file mode 100644
> index 000000000000..71318c65931c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/kprobe_multi.c
> @@ -0,0 +1,58 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.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;
> +
> +/* No tests, just to trigger bpf_fentry_test* through tracing test_run */
> +SEC("fentry/bpf_modify_return_test")
> +int BPF_PROG(test1)
> +{
> +       return 0;
> +}
> +
> +__u64 test2_result = 0;
> +
> +SEC("kprobe.multi/bpf_fentry_test?")
> +int test2(struct pt_regs *ctx)
> +{
> +       __u64 addr = bpf_get_func_ip(ctx);
> +
> +       test2_result += (const void *) addr == &bpf_fentry_test1 ||
> +                       (const void *) addr == &bpf_fentry_test2 ||
> +                       (const void *) addr == &bpf_fentry_test3 ||
> +                       (const void *) addr == &bpf_fentry_test4 ||
> +                       (const void *) addr == &bpf_fentry_test5 ||
> +                       (const void *) addr == &bpf_fentry_test6 ||
> +                       (const void *) addr == &bpf_fentry_test7 ||
> +                       (const void *) addr == &bpf_fentry_test8;
> +       return 0;
> +}
> +
> +__u64 test3_result = 0;
> +
> +SEC("kretprobe.multi/bpf_fentry_test*")
> +int test3(struct pt_regs *ctx)
> +{
> +       __u64 addr = bpf_get_func_ip(ctx);
> +
> +       test3_result += (const void *) addr == &bpf_fentry_test1 ||
> +                       (const void *) addr == &bpf_fentry_test2 ||
> +                       (const void *) addr == &bpf_fentry_test3 ||
> +                       (const void *) addr == &bpf_fentry_test4 ||
> +                       (const void *) addr == &bpf_fentry_test5 ||
> +                       (const void *) addr == &bpf_fentry_test6 ||
> +                       (const void *) addr == &bpf_fentry_test7 ||
> +                       (const void *) addr == &bpf_fentry_test8;
> +       return 0;
> +}
> --
> 2.35.1
>

Re: [PATCH 09/10] selftest/bpf: Add kprobe_multi attach test

[Jiri Olsa]

On Fri, Mar 04, 2022 at 03:11:23PM -0800, Andrii Nakryiko wrote:
> On Tue, Feb 22, 2022 at 9:08 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding kprobe_multi attach test that uses new fprobe interface to
> > attach kprobe program to multiple functions.
> >
> > The test is attaching programs to bpf_fentry_test* functions and
> > uses single trampoline program bpf_prog_test_run to trigger
> > bpf_fentry_test* functions.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> 
> subj typo: selftest -> selftests

ok

> 
> >  .../bpf/prog_tests/kprobe_multi_test.c        | 115 ++++++++++++++++++
> >  .../selftests/bpf/progs/kprobe_multi.c        |  58 +++++++++
> >  2 files changed, 173 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi.c
> >
> 
> [...]
> 
> > +
> > +static void test_link_api_addrs(void)
> > +{
> > +       DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts);
> > +       __u64 addrs[8];
> > +
> > +       kallsyms_find("bpf_fentry_test1", &addrs[0]);
> > +       kallsyms_find("bpf_fentry_test2", &addrs[1]);
> > +       kallsyms_find("bpf_fentry_test3", &addrs[2]);
> > +       kallsyms_find("bpf_fentry_test4", &addrs[3]);
> > +       kallsyms_find("bpf_fentry_test5", &addrs[4]);
> > +       kallsyms_find("bpf_fentry_test6",  &addrs[5]);
> > +       kallsyms_find("bpf_fentry_test7", &addrs[6]);
> > +       kallsyms_find("bpf_fentry_test8", &addrs[7]);
> 
> ASSERT_OK() that symbols are found? It also sucks that we re-parse
> kallsyms so much...

ok

> 
> maybe use load_kallsyms() to pre-cache? We should also teach
> load_kallsyms() to not reload kallsyms more than once

true, it saved many cycles in bpftrace ;-) will check


> 
> > +
> > +       opts.kprobe_multi.addrs = (__u64) addrs;
> > +       opts.kprobe_multi.cnt = 8;
> 
> ARRAY_SIZE()?

ok

> 
> > +       test_link_api(&opts);
> > +}
> > +
> > +static void test_link_api_syms(void)
> > +{
> > +       DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts);
> 
> nit: just LIBBPF_OPTS

ok

> 
> > +       const char *syms[8] = {
> > +               "bpf_fentry_test1",
> > +               "bpf_fentry_test2",
> > +               "bpf_fentry_test3",
> > +               "bpf_fentry_test4",
> > +               "bpf_fentry_test5",
> > +               "bpf_fentry_test6",
> > +               "bpf_fentry_test7",
> > +               "bpf_fentry_test8",
> > +       };
> > +
> > +       opts.kprobe_multi.syms = (__u64) syms;
> > +       opts.kprobe_multi.cnt = 8;
> 
> ARRAY_SIZE() ?

ok

> 
> > +       test_link_api(&opts);
> > +}
> > +
> > +void test_kprobe_multi_test(void)
> > +{
> > +       test_skel_api();
> > +       test_link_api_syms();
> > +       test_link_api_addrs();
> 
> model as subtests?

ok

thanks,
jirka

[PATCH 10/10] selftest/bpf: Add kprobe_multi test for bpf_cookie values

[Jiri Olsa]

Adding bpf_cookie test for programs attached by kprobe_multi links.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../selftests/bpf/prog_tests/bpf_cookie.c     | 72 +++++++++++++++++++
 .../bpf/progs/kprobe_multi_bpf_cookie.c       | 62 ++++++++++++++++
 2 files changed, 134 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_bpf_cookie.c

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
index cd10df6cd0fc..edfb9f8736c6 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
@@ -7,6 +7,7 @@
 #include <unistd.h>
 #include <test_progs.h>
 #include "test_bpf_cookie.skel.h"
+#include "kprobe_multi_bpf_cookie.skel.h"
 
 /* uprobe attach point */
 static void trigger_func(void)
@@ -63,6 +64,75 @@ static void kprobe_subtest(struct test_bpf_cookie *skel)
 	bpf_link__destroy(retlink2);
 }
 
+static void kprobe_multi_subtest(void)
+{
+	DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts);
+	int err, prog_fd, link1_fd = -1, link2_fd = -1;
+	LIBBPF_OPTS(bpf_test_run_opts, topts);
+	struct kprobe_multi_bpf_cookie *skel = NULL;
+	__u64 addrs[8], cookies[8];
+
+	skel = kprobe_multi_bpf_cookie__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "fentry_raw_skel_load"))
+		goto cleanup;
+
+	kallsyms_find("bpf_fentry_test1", &addrs[0]);
+	kallsyms_find("bpf_fentry_test2", &addrs[1]);
+	kallsyms_find("bpf_fentry_test3", &addrs[2]);
+	kallsyms_find("bpf_fentry_test4", &addrs[3]);
+	kallsyms_find("bpf_fentry_test5", &addrs[4]);
+	kallsyms_find("bpf_fentry_test6", &addrs[5]);
+	kallsyms_find("bpf_fentry_test7", &addrs[6]);
+	kallsyms_find("bpf_fentry_test8", &addrs[7]);
+
+	cookies[0] = 1;
+	cookies[1] = 2;
+	cookies[2] = 3;
+	cookies[3] = 4;
+	cookies[4] = 5;
+	cookies[5] = 6;
+	cookies[6] = 7;
+	cookies[7] = 8;
+
+	opts.kprobe_multi.addrs = ptr_to_u64(&addrs);
+	opts.kprobe_multi.cnt = 8;
+	opts.kprobe_multi.cookies = ptr_to_u64(&cookies);
+	prog_fd = bpf_program__fd(skel->progs.test2);
+
+	link1_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, &opts);
+	if (!ASSERT_GE(link1_fd, 0, "link1_fd"))
+		return;
+
+	cookies[0] = 8;
+	cookies[1] = 7;
+	cookies[2] = 6;
+	cookies[3] = 5;
+	cookies[4] = 4;
+	cookies[5] = 3;
+	cookies[6] = 2;
+	cookies[7] = 1;
+
+	opts.flags = BPF_F_KPROBE_MULTI_RETURN;
+	prog_fd = bpf_program__fd(skel->progs.test3);
+
+	link2_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, &opts);
+	if (!ASSERT_GE(link2_fd, 0, "link2_fd"))
+		goto cleanup;
+
+	prog_fd = bpf_program__fd(skel->progs.test1);
+	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->test2_result, 8, "test2_result");
+	ASSERT_EQ(skel->bss->test3_result, 8, "test3_result");
+
+cleanup:
+	close(link1_fd);
+	close(link2_fd);
+	kprobe_multi_bpf_cookie__destroy(skel);
+}
+
 static void uprobe_subtest(struct test_bpf_cookie *skel)
 {
 	DECLARE_LIBBPF_OPTS(bpf_uprobe_opts, opts);
@@ -249,6 +319,8 @@ void test_bpf_cookie(void)
 
 	if (test__start_subtest("kprobe"))
 		kprobe_subtest(skel);
+	if (test__start_subtest("multi_kprobe"))
+		kprobe_multi_subtest();
 	if (test__start_subtest("uprobe"))
 		uprobe_subtest(skel);
 	if (test__start_subtest("tracepoint"))
diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi_bpf_cookie.c b/tools/testing/selftests/bpf/progs/kprobe_multi_bpf_cookie.c
new file mode 100644
index 000000000000..d6f8454ba093
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/kprobe_multi_bpf_cookie.c
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.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;
+
+/* No tests, just to trigger bpf_fentry_test* through tracing test_run */
+SEC("fentry/bpf_modify_return_test")
+int BPF_PROG(test1)
+{
+	return 0;
+}
+
+__u64 test2_result = 0;
+
+SEC("kprobe.multi/bpf_fentry_tes??")
+int test2(struct pt_regs *ctx)
+{
+	__u64 cookie = bpf_get_attach_cookie(ctx);
+	__u64 addr = bpf_get_func_ip(ctx);
+
+	test2_result += (const void *) addr == &bpf_fentry_test1 && cookie == 1;
+	test2_result += (const void *) addr == &bpf_fentry_test2 && cookie == 2;
+	test2_result += (const void *) addr == &bpf_fentry_test3 && cookie == 3;
+	test2_result += (const void *) addr == &bpf_fentry_test4 && cookie == 4;
+	test2_result += (const void *) addr == &bpf_fentry_test5 && cookie == 5;
+	test2_result += (const void *) addr == &bpf_fentry_test6 && cookie == 6;
+	test2_result += (const void *) addr == &bpf_fentry_test7 && cookie == 7;
+	test2_result += (const void *) addr == &bpf_fentry_test8 && cookie == 8;
+
+	return 0;
+}
+
+__u64 test3_result = 0;
+
+SEC("kretprobe.multi/bpf_fentry_test*")
+int test3(struct pt_regs *ctx)
+{
+	__u64 cookie = bpf_get_attach_cookie(ctx);
+	__u64 addr = bpf_get_func_ip(ctx);
+
+	test3_result += (const void *) addr == &bpf_fentry_test1 && cookie == 8;
+	test3_result += (const void *) addr == &bpf_fentry_test2 && cookie == 7;
+	test3_result += (const void *) addr == &bpf_fentry_test3 && cookie == 6;
+	test3_result += (const void *) addr == &bpf_fentry_test4 && cookie == 5;
+	test3_result += (const void *) addr == &bpf_fentry_test5 && cookie == 4;
+	test3_result += (const void *) addr == &bpf_fentry_test6 && cookie == 3;
+	test3_result += (const void *) addr == &bpf_fentry_test7 && cookie == 2;
+	test3_result += (const void *) addr == &bpf_fentry_test8 && cookie == 1;
+
+	return 0;
+}
-- 
2.35.1

Re: [PATCH 10/10] selftest/bpf: Add kprobe_multi test for bpf_cookie values

[Andrii Nakryiko]

On Tue, Feb 22, 2022 at 9:08 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding bpf_cookie test for programs attached by kprobe_multi links.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../selftests/bpf/prog_tests/bpf_cookie.c     | 72 +++++++++++++++++++
>  .../bpf/progs/kprobe_multi_bpf_cookie.c       | 62 ++++++++++++++++
>  2 files changed, 134 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_bpf_cookie.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> index cd10df6cd0fc..edfb9f8736c6 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> @@ -7,6 +7,7 @@
>  #include <unistd.h>
>  #include <test_progs.h>
>  #include "test_bpf_cookie.skel.h"
> +#include "kprobe_multi_bpf_cookie.skel.h"
>
>  /* uprobe attach point */
>  static void trigger_func(void)
> @@ -63,6 +64,75 @@ static void kprobe_subtest(struct test_bpf_cookie *skel)
>         bpf_link__destroy(retlink2);
>  }
>
> +static void kprobe_multi_subtest(void)
> +{
> +       DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts);
> +       int err, prog_fd, link1_fd = -1, link2_fd = -1;
> +       LIBBPF_OPTS(bpf_test_run_opts, topts);

consistency ftw, LIBBPF_OPTS


> +       struct kprobe_multi_bpf_cookie *skel = NULL;
> +       __u64 addrs[8], cookies[8];
> +

[..]

> diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi_bpf_cookie.c b/tools/testing/selftests/bpf/progs/kprobe_multi_bpf_cookie.c
> new file mode 100644
> index 000000000000..d6f8454ba093
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/kprobe_multi_bpf_cookie.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.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;
> +
> +/* No tests, just to trigger bpf_fentry_test* through tracing test_run */
> +SEC("fentry/bpf_modify_return_test")
> +int BPF_PROG(test1)
> +{
> +       return 0;
> +}
> +
> +__u64 test2_result = 0;
> +
> +SEC("kprobe.multi/bpf_fentry_tes??")
> +int test2(struct pt_regs *ctx)
> +{
> +       __u64 cookie = bpf_get_attach_cookie(ctx);
> +       __u64 addr = bpf_get_func_ip(ctx);
> +
> +       test2_result += (const void *) addr == &bpf_fentry_test1 && cookie == 1;
> +       test2_result += (const void *) addr == &bpf_fentry_test2 && cookie == 2;
> +       test2_result += (const void *) addr == &bpf_fentry_test3 && cookie == 3;
> +       test2_result += (const void *) addr == &bpf_fentry_test4 && cookie == 4;
> +       test2_result += (const void *) addr == &bpf_fentry_test5 && cookie == 5;
> +       test2_result += (const void *) addr == &bpf_fentry_test6 && cookie == 6;
> +       test2_result += (const void *) addr == &bpf_fentry_test7 && cookie == 7;
> +       test2_result += (const void *) addr == &bpf_fentry_test8 && cookie == 8;

this is not parallel mode friendly

let's filter by pid, but also it's best to do count locally and just
assign it (so that multiple calls of the program still produce the
same value, instead of constantly increasing global variable with each
run)


> +
> +       return 0;
> +}
> +
> +__u64 test3_result = 0;
> +
> +SEC("kretprobe.multi/bpf_fentry_test*")
> +int test3(struct pt_regs *ctx)
> +{
> +       __u64 cookie = bpf_get_attach_cookie(ctx);
> +       __u64 addr = bpf_get_func_ip(ctx);
> +
> +       test3_result += (const void *) addr == &bpf_fentry_test1 && cookie == 8;
> +       test3_result += (const void *) addr == &bpf_fentry_test2 && cookie == 7;
> +       test3_result += (const void *) addr == &bpf_fentry_test3 && cookie == 6;
> +       test3_result += (const void *) addr == &bpf_fentry_test4 && cookie == 5;
> +       test3_result += (const void *) addr == &bpf_fentry_test5 && cookie == 4;
> +       test3_result += (const void *) addr == &bpf_fentry_test6 && cookie == 3;
> +       test3_result += (const void *) addr == &bpf_fentry_test7 && cookie == 2;
> +       test3_result += (const void *) addr == &bpf_fentry_test8 && cookie == 1;
> +
> +       return 0;
> +}
> --
> 2.35.1
>

Re: [PATCH 10/10] selftest/bpf: Add kprobe_multi test for bpf_cookie values

[Jiri Olsa]

On Fri, Mar 04, 2022 at 03:11:26PM -0800, Andrii Nakryiko wrote:
> On Tue, Feb 22, 2022 at 9:08 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding bpf_cookie test for programs attached by kprobe_multi links.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../selftests/bpf/prog_tests/bpf_cookie.c     | 72 +++++++++++++++++++
> >  .../bpf/progs/kprobe_multi_bpf_cookie.c       | 62 ++++++++++++++++
> >  2 files changed, 134 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_bpf_cookie.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> > index cd10df6cd0fc..edfb9f8736c6 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> > @@ -7,6 +7,7 @@
> >  #include <unistd.h>
> >  #include <test_progs.h>
> >  #include "test_bpf_cookie.skel.h"
> > +#include "kprobe_multi_bpf_cookie.skel.h"
> >
> >  /* uprobe attach point */
> >  static void trigger_func(void)
> > @@ -63,6 +64,75 @@ static void kprobe_subtest(struct test_bpf_cookie *skel)
> >         bpf_link__destroy(retlink2);
> >  }
> >
> > +static void kprobe_multi_subtest(void)
> > +{
> > +       DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts);
> > +       int err, prog_fd, link1_fd = -1, link2_fd = -1;
> > +       LIBBPF_OPTS(bpf_test_run_opts, topts);
> 
> consistency ftw, LIBBPF_OPTS

ok

> 
> 
> > +       struct kprobe_multi_bpf_cookie *skel = NULL;
> > +       __u64 addrs[8], cookies[8];
> > +
> 
> [..]
> 
> > diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi_bpf_cookie.c b/tools/testing/selftests/bpf/progs/kprobe_multi_bpf_cookie.c
> > new file mode 100644
> > index 000000000000..d6f8454ba093
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/kprobe_multi_bpf_cookie.c
> > @@ -0,0 +1,62 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/bpf.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.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;
> > +
> > +/* No tests, just to trigger bpf_fentry_test* through tracing test_run */
> > +SEC("fentry/bpf_modify_return_test")
> > +int BPF_PROG(test1)
> > +{
> > +       return 0;
> > +}
> > +
> > +__u64 test2_result = 0;
> > +
> > +SEC("kprobe.multi/bpf_fentry_tes??")
> > +int test2(struct pt_regs *ctx)
> > +{
> > +       __u64 cookie = bpf_get_attach_cookie(ctx);
> > +       __u64 addr = bpf_get_func_ip(ctx);
> > +
> > +       test2_result += (const void *) addr == &bpf_fentry_test1 && cookie == 1;
> > +       test2_result += (const void *) addr == &bpf_fentry_test2 && cookie == 2;
> > +       test2_result += (const void *) addr == &bpf_fentry_test3 && cookie == 3;
> > +       test2_result += (const void *) addr == &bpf_fentry_test4 && cookie == 4;
> > +       test2_result += (const void *) addr == &bpf_fentry_test5 && cookie == 5;
> > +       test2_result += (const void *) addr == &bpf_fentry_test6 && cookie == 6;
> > +       test2_result += (const void *) addr == &bpf_fentry_test7 && cookie == 7;
> > +       test2_result += (const void *) addr == &bpf_fentry_test8 && cookie == 8;
> 
> this is not parallel mode friendly
> 
> let's filter by pid, but also it's best to do count locally and just
> assign it (so that multiple calls of the program still produce the
> same value, instead of constantly increasing global variable with each
> run)

ah I did not think of the paralel run, right, will change

thanks,
jirka

Re: [PATCHv2 bpf-next 0/8] bpf: Add kprobe multi link

[Andrii Nakryiko]

On Tue, Feb 22, 2022 at 9:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> hi,
> this patchset adds new link type BPF_TRACE_KPROBE_MULTI that attaches
> kprobe program through fprobe API [1] instroduced by Masami.
>
> The fprobe API allows to attach probe on multiple functions at once very
> fast, because it works on top of ftrace. On the other hand this limits
> the probe point to the function entry or return.
>
>
> With bpftrace support I see following attach speed:
>
>   # perf stat --null -r 5 ./src/bpftrace -e 'kprobe:x* { } i:ms:1 { exit(); } '
>   Attaching 2 probes...
>   Attaching 3342 functions
>   ...
>
>   1.4960 +- 0.0285 seconds time elapsed  ( +-  1.91% )
>
>
> v2 changes:
>   - based on latest fprobe changes [1]
>   - renaming the uapi interface to kprobe multi
>   - adding support for sort_r to pass user pointer for swap functions
>     and using that in cookie support to keep just single functions array
>   - moving new link to kernel/trace/bpf_trace.c file
>   - using single fprobe callback function for entry and exit
>   - using kvzalloc, libbpf_ensure_mem functions
>   - adding new k[ret]probe.multi sections instead of using current kprobe
>   - used glob_match from test_progs.c, added '?' matching
>   - move bpf_get_func_ip verifier inline change to seprate change
>   - couple of other minor fixes
>
>

I think it's shaping up pretty well. Great work, Jiri! Can't wait to
adopt this in retsnoop. See below about dependency on Masami's
patches.

> Also available at:
>   https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
>   bpf/kprobe_multi
>
> thanks,
> jirka
>
>
> [1] https://lore.kernel.org/bpf/164458044634.586276.3261555265565111183.stgit@devnote2/

Masami, Jiri, Steven, what would be the logistics here? What's the
plan for getting this upstream? Any idea about timelines? I really
hope it won't take as long as it took for kretprobe stack trace
capturing fixes last year to land. Can we take Masami's changes
through bpf-next tree? If yes, Steven, can you please review and give
your acks? Thanks for understanding!

> ---
> Jiri Olsa (10):
>       lib/sort: Add priv pointer to swap function
>       bpf: Add multi kprobe link
>       bpf: Add bpf_get_func_ip kprobe helper for multi kprobe link
>       bpf: Add support to inline bpf_get_func_ip helper on x86
>       bpf: Add cookie support to programs attached with kprobe multi link
>       libbpf: Add libbpf_kallsyms_parse function
>       libbpf: Add bpf_link_create support for multi kprobes
>       libbpf: Add bpf_program__attach_kprobe_opts support for multi kprobes
>       selftest/bpf: Add kprobe_multi attach test
>       selftest/bpf: Add kprobe_multi test for bpf_cookie values
>
>  include/linux/bpf_types.h                                   |   1 +
>  include/linux/sort.h                                        |   4 +-
>  include/linux/trace_events.h                                |   6 ++
>  include/linux/types.h                                       |   1 +
>  include/uapi/linux/bpf.h                                    |  14 ++++
>  kernel/bpf/syscall.c                                        |  26 ++++++--
>  kernel/bpf/verifier.c                                       |  21 +++++-
>  kernel/trace/bpf_trace.c                                    | 331 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  lib/sort.c                                                  |  42 +++++++++---
>  tools/include/uapi/linux/bpf.h                              |  14 ++++
>  tools/lib/bpf/bpf.c                                         |   7 ++
>  tools/lib/bpf/bpf.h                                         |   9 ++-
>  tools/lib/bpf/libbpf.c                                      | 192 +++++++++++++++++++++++++++++++++++++++++++++--------
>  tools/lib/bpf/libbpf_internal.h                             |   5 ++
>  tools/testing/selftests/bpf/prog_tests/bpf_cookie.c         |  72 ++++++++++++++++++++
>  tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c  | 115 ++++++++++++++++++++++++++++++++
>  tools/testing/selftests/bpf/progs/kprobe_multi.c            |  58 ++++++++++++++++
>  tools/testing/selftests/bpf/progs/kprobe_multi_bpf_cookie.c |  62 +++++++++++++++++
>  18 files changed, 930 insertions(+), 50 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
>  create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi.c
>  create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_bpf_cookie.c

Re: [PATCHv2 bpf-next 0/8] bpf: Add kprobe multi link

[Steven Rostedt]

On Fri, 4 Mar 2022 15:10:55 -0800
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> Masami, Jiri, Steven, what would be the logistics here? What's the
> plan for getting this upstream? Any idea about timelines? I really
> hope it won't take as long as it took for kretprobe stack trace
> capturing fixes last year to land. Can we take Masami's changes
> through bpf-next tree? If yes, Steven, can you please review and give
> your acks? Thanks for understanding!

Yeah, I'll start looking at it this week. I just started a new job and
that's been taking up a lot of my time and limiting what I can look at
upstream.

-- Steve

Re: [PATCHv2 bpf-next 0/8] bpf: Add kprobe multi link

[Masami Hiramatsu]

On Sat, 5 Mar 2022 20:09:39 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 4 Mar 2022 15:10:55 -0800
> Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> 
> > Masami, Jiri, Steven, what would be the logistics here? What's the
> > plan for getting this upstream? Any idea about timelines? I really
> > hope it won't take as long as it took for kretprobe stack trace
> > capturing fixes last year to land. Can we take Masami's changes
> > through bpf-next tree? If yes, Steven, can you please review and give
> > your acks? Thanks for understanding!
> 
> Yeah, I'll start looking at it this week. I just started a new job and
> that's been taking up a lot of my time and limiting what I can look at
> upstream.

Let me update my series, I found some issues in the selftest.
I'll send v9 soon.

Thank you!

-- 
Masami Hiramatsu

Re: [PATCHv2 bpf-next 0/8] bpf: Add kprobe multi link

[Andrii Nakryiko]

On Sat, Mar 5, 2022 at 5:33 PM Masami Hiramatsu
<masami.hiramatsu@gmail.com> wrote:
>
> On Sat, 5 Mar 2022 20:09:39 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > On Fri, 4 Mar 2022 15:10:55 -0800
> > Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > > Masami, Jiri, Steven, what would be the logistics here? What's the
> > > plan for getting this upstream? Any idea about timelines? I really
> > > hope it won't take as long as it took for kretprobe stack trace
> > > capturing fixes last year to land. Can we take Masami's changes
> > > through bpf-next tree? If yes, Steven, can you please review and give
> > > your acks? Thanks for understanding!
> >
> > Yeah, I'll start looking at it this week. I just started a new job and

Thanks, Steven. Greatly appreciated!

> > that's been taking up a lot of my time and limiting what I can look at
> > upstream.
>
> Let me update my series, I found some issues in the selftest.
> I'll send v9 soon.

I haven't checked, but if you haven't based your patches off of
bpf-next tree, please do so in the next revision, so that we can land
your patch set and Jiri's patch set on top without any issues. Thanks!

>
> Thank you!
>
> --
> Masami Hiramatsu