[PATCH bpf-next 0/8] bpf: Fixes for kprobe multi on kernel modules

[PATCH bpf-next 0/8] bpf: Fixes for kprobe multi on kernel modules

[Jiri Olsa]

hi,
Martynas reported kprobe _multi link does not resolve symbols
from kernel modules, which attach by address works.

In addition while fixing that I realized we do not take module
reference if the module has kprobe_multi link on top of it and
can be removed.

There's mo crash related to this, it will silently disappear from
ftrace tables, while kprobe_multi link stays up with no data.

This patchset has fixes for both issues.

thanks,
jirka


---
Jiri Olsa (8):
      kallsyms: Make module_kallsyms_on_each_symbol generally available
      ftrace: Add support to resolve module symbols in ftrace_lookup_symbols
      bpf: Rename __bpf_kprobe_multi_cookie_cmp to bpf_kprobe_multi_addrs_cmp
      bpf: Take module reference on kprobe_multi link
      selftests/bpf: Add load_kallsyms_refresh function
      selftests/bpf: Add bpf_testmod_fentry_* functions
      selftests/bpf: Add kprobe_multi kmod link api tests
      selftests/bpf: Add kprobe_multi check to module attach test

 include/linux/module.h                                             |   9 ++++++++
 kernel/module/kallsyms.c                                           |   2 --
 kernel/trace/bpf_trace.c                                           | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 kernel/trace/ftrace.c                                              |  11 +++++----
 tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c              |  24 +++++++++++++++++++
 tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c |  94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/prog_tests/module_attach.c             |   7 ++++++
 tools/testing/selftests/bpf/progs/kprobe_multi.c                   |  51 ++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/test_module_attach.c             |   6 +++++
 tools/testing/selftests/bpf/trace_helpers.c                        |  20 ++++++++++------
 tools/testing/selftests/bpf/trace_helpers.h                        |   2 ++
 11 files changed, 315 insertions(+), 17 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c

[PATCH bpf-next 1/8] kallsyms: Make module_kallsyms_on_each_symbol generally available

[Jiri Olsa]

Making module_kallsyms_on_each_symbol generally available, so it
can be used outside CONFIG_LIVEPATCH option in following changes.

Rather than adding another ifdef option let's make the function
generally available (when CONFIG_KALLSYMS and CONFIG_MODULES
options are defined).

Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/module.h   | 9 +++++++++
 kernel/module/kallsyms.c | 2 --
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index ec61fb53979a..35876e89eb93 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -879,8 +879,17 @@ static inline bool module_sig_ok(struct module *module)
 }
 #endif	/* CONFIG_MODULE_SIG */
 
+#if defined(CONFIG_MODULES) && defined(CONFIG_KALLSYMS)
 int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
 					     struct module *, unsigned long),
 				   void *data);
+#else
+static inline int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
+						 struct module *, unsigned long),
+						 void *data)
+{
+	return -EOPNOTSUPP;
+}
+#endif  /* CONFIG_MODULES && CONFIG_KALLSYMS */
 
 #endif /* _LINUX_MODULE_H */
diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
index f5c5c9175333..4523f99b0358 100644
--- a/kernel/module/kallsyms.c
+++ b/kernel/module/kallsyms.c
@@ -494,7 +494,6 @@ unsigned long module_kallsyms_lookup_name(const char *name)
 	return ret;
 }
 
-#ifdef CONFIG_LIVEPATCH
 int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
 					     struct module *, unsigned long),
 				   void *data)
@@ -531,4 +530,3 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
 	mutex_unlock(&module_mutex);
 	return ret;
 }
-#endif /* CONFIG_LIVEPATCH */
-- 
2.37.3

Re: [PATCH bpf-next 1/8] kallsyms: Make module_kallsyms_on_each_symbol generally available

[Song Liu]

On Sun, Oct 9, 2022 at 2:59 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Making module_kallsyms_on_each_symbol generally available, so it
> can be used outside CONFIG_LIVEPATCH option in following changes.
>
> Rather than adding another ifdef option let's make the function
> generally available (when CONFIG_KALLSYMS and CONFIG_MODULES
> options are defined).
>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <song@kernel.org>

[PATCH bpf-next 2/8] ftrace: Add support to resolve module symbols in ftrace_lookup_symbols

[Jiri Olsa]

Currently ftrace_lookup_symbols iterates only over core symbols,
adding module_kallsyms_on_each_symbol call to check on modules
symbols as well.

Also removing 'args.found == args.cnt' condition, because it's
already checked in kallsyms_callback function.

Also removing 'err < 0' check, because both *kallsyms_on_each_symbol
functions do not return error.

Reported-by: Martynas Pumputis <m@lambda.lt>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/trace/ftrace.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 447d2e2a8549..6efdba4666f4 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -8292,17 +8292,18 @@ static int kallsyms_callback(void *data, const char *name,
 int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs)
 {
 	struct kallsyms_data args;
-	int err;
+	int found_all;
 
 	memset(addrs, 0, sizeof(*addrs) * cnt);
 	args.addrs = addrs;
 	args.syms = sorted_syms;
 	args.cnt = cnt;
 	args.found = 0;
-	err = kallsyms_on_each_symbol(kallsyms_callback, &args);
-	if (err < 0)
-		return err;
-	return args.found == args.cnt ? 0 : -ESRCH;
+	found_all = kallsyms_on_each_symbol(kallsyms_callback, &args);
+	if (found_all)
+		return 0;
+	found_all = module_kallsyms_on_each_symbol(kallsyms_callback, &args);
+	return found_all ? 0 : -ESRCH;
 }
 
 #ifdef CONFIG_SYSCTL
-- 
2.37.3

Re: [PATCH bpf-next 2/8] ftrace: Add support to resolve module symbols in ftrace_lookup_symbols

[Song Liu]

On Sun, Oct 9, 2022 at 3:00 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Currently ftrace_lookup_symbols iterates only over core symbols,
> adding module_kallsyms_on_each_symbol call to check on modules
> symbols as well.
>
> Also removing 'args.found == args.cnt' condition, because it's
> already checked in kallsyms_callback function.
>
> Also removing 'err < 0' check, because both *kallsyms_on_each_symbol
> functions do not return error.
>
> Reported-by: Martynas Pumputis <m@lambda.lt>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <song@kernel.org>

> ---
>  kernel/trace/ftrace.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 447d2e2a8549..6efdba4666f4 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -8292,17 +8292,18 @@ static int kallsyms_callback(void *data, const char *name,
>  int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs)
>  {
>         struct kallsyms_data args;
> -       int err;
> +       int found_all;
>
>         memset(addrs, 0, sizeof(*addrs) * cnt);
>         args.addrs = addrs;
>         args.syms = sorted_syms;
>         args.cnt = cnt;
>         args.found = 0;
> -       err = kallsyms_on_each_symbol(kallsyms_callback, &args);
> -       if (err < 0)
> -               return err;
> -       return args.found == args.cnt ? 0 : -ESRCH;
> +       found_all = kallsyms_on_each_symbol(kallsyms_callback, &args);
> +       if (found_all)
> +               return 0;
> +       found_all = module_kallsyms_on_each_symbol(kallsyms_callback, &args);
> +       return found_all ? 0 : -ESRCH;

We probably need some comments about kallsym_callback and
ftrace_lookup_symbols. It took me a while to confirm the logic for
found_all.

>  }
>
>  #ifdef CONFIG_SYSCTL
> --
> 2.37.3
>

Re: [PATCH bpf-next 2/8] ftrace: Add support to resolve module symbols in ftrace_lookup_symbols

[Jiri Olsa]

On Tue, Oct 11, 2022 at 12:05:47AM -0700, Song Liu wrote:
> On Sun, Oct 9, 2022 at 3:00 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Currently ftrace_lookup_symbols iterates only over core symbols,
> > adding module_kallsyms_on_each_symbol call to check on modules
> > symbols as well.
> >
> > Also removing 'args.found == args.cnt' condition, because it's
> > already checked in kallsyms_callback function.
> >
> > Also removing 'err < 0' check, because both *kallsyms_on_each_symbol
> > functions do not return error.
> >
> > Reported-by: Martynas Pumputis <m@lambda.lt>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> Acked-by: Song Liu <song@kernel.org>
> 
> > ---
> >  kernel/trace/ftrace.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 447d2e2a8549..6efdba4666f4 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -8292,17 +8292,18 @@ static int kallsyms_callback(void *data, const char *name,
> >  int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs)
> >  {
> >         struct kallsyms_data args;
> > -       int err;
> > +       int found_all;
> >
> >         memset(addrs, 0, sizeof(*addrs) * cnt);
> >         args.addrs = addrs;
> >         args.syms = sorted_syms;
> >         args.cnt = cnt;
> >         args.found = 0;
> > -       err = kallsyms_on_each_symbol(kallsyms_callback, &args);
> > -       if (err < 0)
> > -               return err;
> > -       return args.found == args.cnt ? 0 : -ESRCH;
> > +       found_all = kallsyms_on_each_symbol(kallsyms_callback, &args);
> > +       if (found_all)
> > +               return 0;
> > +       found_all = module_kallsyms_on_each_symbol(kallsyms_callback, &args);
> > +       return found_all ? 0 : -ESRCH;
> 
> We probably need some comments about kallsym_callback and
> ftrace_lookup_symbols. It took me a while to confirm the logic for
> found_all.

ok, I wrote some info in the changelog, but I'll put it
as comment in the code as well

jirka

> 
> >  }
> >
> >  #ifdef CONFIG_SYSCTL
> > --
> > 2.37.3
> >

[PATCH bpf-next 3/8] bpf: Rename __bpf_kprobe_multi_cookie_cmp to bpf_kprobe_multi_addrs_cmp

[Jiri Olsa]

Renaming __bpf_kprobe_multi_cookie_cmp to bpf_kprobe_multi_addrs_cmp,
because it's more suitable to current and upcoming code.

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

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 688552df95ca..9be1a2b6b53b 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2545,7 +2545,7 @@ static void bpf_kprobe_multi_cookie_swap(void *a, void *b, int size, const void
 	swap(*cookie_a, *cookie_b);
 }
 
-static int __bpf_kprobe_multi_cookie_cmp(const void *a, const void *b)
+static int bpf_kprobe_multi_addrs_cmp(const void *a, const void *b)
 {
 	const unsigned long *addr_a = a, *addr_b = b;
 
@@ -2556,7 +2556,7 @@ static int __bpf_kprobe_multi_cookie_cmp(const void *a, const void *b)
 
 static int bpf_kprobe_multi_cookie_cmp(const void *a, const void *b, const void *priv)
 {
-	return __bpf_kprobe_multi_cookie_cmp(a, b);
+	return bpf_kprobe_multi_addrs_cmp(a, b);
 }
 
 static u64 bpf_kprobe_multi_cookie(struct bpf_run_ctx *ctx)
@@ -2574,7 +2574,7 @@ static u64 bpf_kprobe_multi_cookie(struct bpf_run_ctx *ctx)
 		return 0;
 	entry_ip = run_ctx->entry_ip;
 	addr = bsearch(&entry_ip, link->addrs, link->cnt, sizeof(entry_ip),
-		       __bpf_kprobe_multi_cookie_cmp);
+		       bpf_kprobe_multi_addrs_cmp);
 	if (!addr)
 		return 0;
 	cookie = link->cookies + (addr - link->addrs);
-- 
2.37.3

Re: [PATCH bpf-next 3/8] bpf: Rename __bpf_kprobe_multi_cookie_cmp to bpf_kprobe_multi_addrs_cmp

[Song Liu]

On Sun, Oct 9, 2022 at 3:00 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Renaming __bpf_kprobe_multi_cookie_cmp to bpf_kprobe_multi_addrs_cmp,
> because it's more suitable to current and upcoming code.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <song@kernel.org>

> ---
>  kernel/trace/bpf_trace.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 688552df95ca..9be1a2b6b53b 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2545,7 +2545,7 @@ static void bpf_kprobe_multi_cookie_swap(void *a, void *b, int size, const void
>         swap(*cookie_a, *cookie_b);
>  }
>
> -static int __bpf_kprobe_multi_cookie_cmp(const void *a, const void *b)
> +static int bpf_kprobe_multi_addrs_cmp(const void *a, const void *b)
>  {
>         const unsigned long *addr_a = a, *addr_b = b;
>
> @@ -2556,7 +2556,7 @@ static int __bpf_kprobe_multi_cookie_cmp(const void *a, const void *b)
>
>  static int bpf_kprobe_multi_cookie_cmp(const void *a, const void *b, const void *priv)
>  {
> -       return __bpf_kprobe_multi_cookie_cmp(a, b);
> +       return bpf_kprobe_multi_addrs_cmp(a, b);
>  }
>
>  static u64 bpf_kprobe_multi_cookie(struct bpf_run_ctx *ctx)
> @@ -2574,7 +2574,7 @@ static u64 bpf_kprobe_multi_cookie(struct bpf_run_ctx *ctx)
>                 return 0;
>         entry_ip = run_ctx->entry_ip;
>         addr = bsearch(&entry_ip, link->addrs, link->cnt, sizeof(entry_ip),
> -                      __bpf_kprobe_multi_cookie_cmp);
> +                      bpf_kprobe_multi_addrs_cmp);
>         if (!addr)
>                 return 0;
>         cookie = link->cookies + (addr - link->addrs);
> --
> 2.37.3
>

[PATCH bpf-next 4/8] bpf: Take module reference on kprobe_multi link

[Jiri Olsa]

Currently we allow to create kprobe multi link on function from kernel
module, but we don't take the module reference to ensure it's not
unloaded while we are tracing it.

The multi kprobe link is based on fprobe/ftrace layer which takes
different approach and releases ftrace hooks when module is unloaded
even if there's tracer registered on top of it.

Adding code that gathers all the related modules for the link and takes
their references before it's attached. All kernel module references are
released after link is unregistered.

Note that we do it the same way already for trampoline probes
(but for single address).

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

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 9be1a2b6b53b..f3d7565fee79 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2447,6 +2447,8 @@ struct bpf_kprobe_multi_link {
 	unsigned long *addrs;
 	u64 *cookies;
 	u32 cnt;
+	struct module **mods;
+	u32 mods_cnt;
 };
 
 struct bpf_kprobe_multi_run_ctx {
@@ -2502,6 +2504,14 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
 	return err;
 }
 
+static void kprobe_multi_put_modules(struct module **mods, u32 cnt)
+{
+	u32 i;
+
+	for (i = 0; i < cnt; i++)
+		module_put(mods[i]);
+}
+
 static void free_user_syms(struct user_syms *us)
 {
 	kvfree(us->syms);
@@ -2514,6 +2524,7 @@ static void bpf_kprobe_multi_link_release(struct bpf_link *link)
 
 	kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
 	unregister_fprobe(&kmulti_link->fp);
+	kprobe_multi_put_modules(kmulti_link->mods, kmulti_link->mods_cnt);
 }
 
 static void bpf_kprobe_multi_link_dealloc(struct bpf_link *link)
@@ -2523,6 +2534,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->mods);
 	kfree(kmulti_link);
 }
 
@@ -2658,6 +2670,80 @@ static void symbols_swap_r(void *a, void *b, int size, const void *priv)
 	}
 }
 
+struct module_addr_args {
+	unsigned long *addrs;
+	u32 addrs_cnt;
+	struct module **mods;
+	int mods_cnt;
+	int mods_alloc;
+};
+
+static int module_callback(void *data, const char *name,
+			   struct module *mod, unsigned long addr)
+{
+	struct module_addr_args *args = data;
+	bool realloc = !args->mods;
+	struct module **mods;
+
+	/* We iterate all modules symbols and for each we:
+	 * - search for it in provided addresses array
+	 * - if found we check if we already have the module pointer stored
+	 *   (we iterate modules sequentially, so we can check just the last
+	 *   module pointer)
+	 * - take module reference and store it
+	 */
+	if (!bsearch(&addr, args->addrs, args->addrs_cnt, sizeof(unsigned long),
+		       bpf_kprobe_multi_addrs_cmp))
+		return 0;
+
+	if (args->mods) {
+		struct module *prev = NULL;
+
+		if (args->mods_cnt > 1)
+			prev = args->mods[args->mods_cnt - 1];
+		if (prev == mod)
+			return 0;
+		if (args->mods_cnt == args->mods_alloc)
+			realloc = true;
+	}
+
+	if (realloc) {
+		args->mods_alloc += 100;
+		mods = krealloc_array(args->mods, args->mods_alloc, sizeof(*mods), GFP_KERNEL);
+		if (!mods)
+			return -ENOMEM;
+		args->mods = mods;
+	}
+
+	if (!try_module_get(mod))
+		return -EINVAL;
+
+	args->mods[args->mods_cnt] = mod;
+	args->mods_cnt++;
+	return 0;
+}
+
+static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u32 addrs_cnt)
+{
+	struct module_addr_args args = {
+		.addrs     = addrs,
+		.addrs_cnt = addrs_cnt,
+	};
+	int err;
+
+	/* We return either err < 0 in case of error, ... */
+	err = module_kallsyms_on_each_symbol(module_callback, &args);
+	if (err) {
+		kprobe_multi_put_modules(args.mods, args.mods_cnt);
+		kfree(args.mods);
+		return err;
+	}
+
+	/* or number of modules found if everything is ok. */
+	*mods = args.mods;
+	return args.mods_cnt;
+}
+
 int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
 {
 	struct bpf_kprobe_multi_link *link = NULL;
@@ -2768,7 +2854,21 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 		       bpf_kprobe_multi_cookie_cmp,
 		       bpf_kprobe_multi_cookie_swap,
 		       link);
+	} else {
+		/*
+		 * We need to sort addrs array even if there are no cookies
+		 * provided, to allow bsearch in get_modules_for_addrs.
+		 */
+		sort(addrs, cnt, sizeof(*addrs),
+		       bpf_kprobe_multi_addrs_cmp, NULL);
+	}
+
+	err = get_modules_for_addrs(&link->mods, addrs, cnt);
+	if (err < 0) {
+		bpf_link_cleanup(&link_primer);
+		return err;
 	}
+	link->mods_cnt = err;
 
 	err = register_fprobe_ips(&link->fp, addrs, cnt);
 	if (err) {
-- 
2.37.3

Re: [PATCH bpf-next 4/8] bpf: Take module reference on kprobe_multi link

[Song Liu]

On Sun, Oct 9, 2022 at 3:00 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Currently we allow to create kprobe multi link on function from kernel
> module, but we don't take the module reference to ensure it's not
> unloaded while we are tracing it.
>
> The multi kprobe link is based on fprobe/ftrace layer which takes
> different approach and releases ftrace hooks when module is unloaded
> even if there's tracer registered on top of it.
>
> Adding code that gathers all the related modules for the link and takes
> their references before it's attached. All kernel module references are
> released after link is unregistered.
>
> Note that we do it the same way already for trampoline probes
> (but for single address).
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

[...]

> +       }
> +
> +       if (realloc) {
> +               args->mods_alloc += 100;

100 seems arbitrary and too big to me.

Other than this, LGTM

Acked-by: Song Liu <song@kernel.org>

> +               mods = krealloc_array(args->mods, args->mods_alloc, sizeof(*mods), GFP_KERNEL);
> +               if (!mods)
> +                       return -ENOMEM;
> +               args->mods = mods;
> +       }
> +
> +       if (!try_module_get(mod))
> +               return -EINVAL;
> +
> +       args->mods[args->mods_cnt] = mod;
> +       args->mods_cnt++;
> +       return 0;
> +}

On Sun, Oct 9, 2022 at 3:00 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Currently we allow to create kprobe multi link on function from kernel
> module, but we don't take the module reference to ensure it's not
> unloaded while we are tracing it.
>
> The multi kprobe link is based on fprobe/ftrace layer which takes
> different approach and releases ftrace hooks when module is unloaded
> even if there's tracer registered on top of it.
>
> Adding code that gathers all the related modules for the link and takes
> their references before it's attached. All kernel module references are
> released after link is unregistered.
>
> Note that we do it the same way already for trampoline probes
> (but for single address).
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/trace/bpf_trace.c | 100 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 100 insertions(+)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 9be1a2b6b53b..f3d7565fee79 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2447,6 +2447,8 @@ struct bpf_kprobe_multi_link {
>         unsigned long *addrs;
>         u64 *cookies;
>         u32 cnt;
> +       struct module **mods;
> +       u32 mods_cnt;
>  };
>
>  struct bpf_kprobe_multi_run_ctx {
> @@ -2502,6 +2504,14 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
>         return err;
>  }
>
> +static void kprobe_multi_put_modules(struct module **mods, u32 cnt)
> +{
> +       u32 i;
> +
> +       for (i = 0; i < cnt; i++)
> +               module_put(mods[i]);
> +}
> +
>  static void free_user_syms(struct user_syms *us)
>  {
>         kvfree(us->syms);
> @@ -2514,6 +2524,7 @@ static void bpf_kprobe_multi_link_release(struct bpf_link *link)
>
>         kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
>         unregister_fprobe(&kmulti_link->fp);
> +       kprobe_multi_put_modules(kmulti_link->mods, kmulti_link->mods_cnt);
>  }
>
>  static void bpf_kprobe_multi_link_dealloc(struct bpf_link *link)
> @@ -2523,6 +2534,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->mods);
>         kfree(kmulti_link);
>  }
>
> @@ -2658,6 +2670,80 @@ static void symbols_swap_r(void *a, void *b, int size, const void *priv)
>         }
>  }
>
> +struct module_addr_args {
> +       unsigned long *addrs;
> +       u32 addrs_cnt;
> +       struct module **mods;
> +       int mods_cnt;
> +       int mods_alloc;
> +};
> +
> +static int module_callback(void *data, const char *name,
> +                          struct module *mod, unsigned long addr)
> +{
> +       struct module_addr_args *args = data;
> +       bool realloc = !args->mods;
> +       struct module **mods;
> +
> +       /* We iterate all modules symbols and for each we:
> +        * - search for it in provided addresses array
> +        * - if found we check if we already have the module pointer stored
> +        *   (we iterate modules sequentially, so we can check just the last
> +        *   module pointer)
> +        * - take module reference and store it
> +        */
> +       if (!bsearch(&addr, args->addrs, args->addrs_cnt, sizeof(unsigned long),
> +                      bpf_kprobe_multi_addrs_cmp))
> +               return 0;
> +
> +       if (args->mods) {
> +               struct module *prev = NULL;
> +
> +               if (args->mods_cnt > 1)
> +                       prev = args->mods[args->mods_cnt - 1];
> +               if (prev == mod)
> +                       return 0;
> +               if (args->mods_cnt == args->mods_alloc)
> +                       realloc = true;
> +       }
> +
> +       if (realloc) {
> +               args->mods_alloc += 100;
> +               mods = krealloc_array(args->mods, args->mods_alloc, sizeof(*mods), GFP_KERNEL);
> +               if (!mods)
> +                       return -ENOMEM;
> +               args->mods = mods;
> +       }
> +
> +       if (!try_module_get(mod))
> +               return -EINVAL;
> +
> +       args->mods[args->mods_cnt] = mod;
> +       args->mods_cnt++;
> +       return 0;
> +}
> +
> +static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u32 addrs_cnt)
> +{
> +       struct module_addr_args args = {
> +               .addrs     = addrs,
> +               .addrs_cnt = addrs_cnt,
> +       };
> +       int err;
> +
> +       /* We return either err < 0 in case of error, ... */
> +       err = module_kallsyms_on_each_symbol(module_callback, &args);
> +       if (err) {
> +               kprobe_multi_put_modules(args.mods, args.mods_cnt);
> +               kfree(args.mods);
> +               return err;
> +       }
> +
> +       /* or number of modules found if everything is ok. */
> +       *mods = args.mods;
> +       return args.mods_cnt;
> +}
> +
>  int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
>  {
>         struct bpf_kprobe_multi_link *link = NULL;
> @@ -2768,7 +2854,21 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>                        bpf_kprobe_multi_cookie_cmp,
>                        bpf_kprobe_multi_cookie_swap,
>                        link);
> +       } else {
> +               /*
> +                * We need to sort addrs array even if there are no cookies
> +                * provided, to allow bsearch in get_modules_for_addrs.
> +                */
> +               sort(addrs, cnt, sizeof(*addrs),
> +                      bpf_kprobe_multi_addrs_cmp, NULL);
> +       }
> +
> +       err = get_modules_for_addrs(&link->mods, addrs, cnt);
> +       if (err < 0) {
> +               bpf_link_cleanup(&link_primer);
> +               return err;
>         }
> +       link->mods_cnt = err;
>
>         err = register_fprobe_ips(&link->fp, addrs, cnt);
>         if (err) {
> --
> 2.37.3
>

Re: [PATCH bpf-next 4/8] bpf: Take module reference on kprobe_multi link

[Jiri Olsa]

On Tue, Oct 11, 2022 at 12:16:22AM -0700, Song Liu wrote:
> On Sun, Oct 9, 2022 at 3:00 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Currently we allow to create kprobe multi link on function from kernel
> > module, but we don't take the module reference to ensure it's not
> > unloaded while we are tracing it.
> >
> > The multi kprobe link is based on fprobe/ftrace layer which takes
> > different approach and releases ftrace hooks when module is unloaded
> > even if there's tracer registered on top of it.
> >
> > Adding code that gathers all the related modules for the link and takes
> > their references before it's attached. All kernel module references are
> > released after link is unregistered.
> >
> > Note that we do it the same way already for trampoline probes
> > (but for single address).
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> [...]
> 
> > +       }
> > +
> > +       if (realloc) {
> > +               args->mods_alloc += 100;
> 
> 100 seems arbitrary and too big to me.

[jolsa@krava linux-qemu]$ lsmod | wc -l
192

I'll check if we can get actuall modules count and base the
allocation on that

jirka

> 
> Other than this, LGTM
> 
> Acked-by: Song Liu <song@kernel.org>
> 
> > +               mods = krealloc_array(args->mods, args->mods_alloc, sizeof(*mods), GFP_KERNEL);
> > +               if (!mods)
> > +                       return -ENOMEM;
> > +               args->mods = mods;
> > +       }
> > +
> > +       if (!try_module_get(mod))
> > +               return -EINVAL;
> > +
> > +       args->mods[args->mods_cnt] = mod;
> > +       args->mods_cnt++;
> > +       return 0;
> > +}
> 
> On Sun, Oct 9, 2022 at 3:00 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Currently we allow to create kprobe multi link on function from kernel
> > module, but we don't take the module reference to ensure it's not
> > unloaded while we are tracing it.
> >
> > The multi kprobe link is based on fprobe/ftrace layer which takes
> > different approach and releases ftrace hooks when module is unloaded
> > even if there's tracer registered on top of it.
> >
> > Adding code that gathers all the related modules for the link and takes
> > their references before it's attached. All kernel module references are
> > released after link is unregistered.
> >
> > Note that we do it the same way already for trampoline probes
> > (but for single address).
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/trace/bpf_trace.c | 100 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 100 insertions(+)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 9be1a2b6b53b..f3d7565fee79 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2447,6 +2447,8 @@ struct bpf_kprobe_multi_link {
> >         unsigned long *addrs;
> >         u64 *cookies;
> >         u32 cnt;
> > +       struct module **mods;
> > +       u32 mods_cnt;
> >  };
> >
> >  struct bpf_kprobe_multi_run_ctx {
> > @@ -2502,6 +2504,14 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
> >         return err;
> >  }
> >
> > +static void kprobe_multi_put_modules(struct module **mods, u32 cnt)
> > +{
> > +       u32 i;
> > +
> > +       for (i = 0; i < cnt; i++)
> > +               module_put(mods[i]);
> > +}
> > +
> >  static void free_user_syms(struct user_syms *us)
> >  {
> >         kvfree(us->syms);
> > @@ -2514,6 +2524,7 @@ static void bpf_kprobe_multi_link_release(struct bpf_link *link)
> >
> >         kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
> >         unregister_fprobe(&kmulti_link->fp);
> > +       kprobe_multi_put_modules(kmulti_link->mods, kmulti_link->mods_cnt);
> >  }
> >
> >  static void bpf_kprobe_multi_link_dealloc(struct bpf_link *link)
> > @@ -2523,6 +2534,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->mods);
> >         kfree(kmulti_link);
> >  }
> >
> > @@ -2658,6 +2670,80 @@ static void symbols_swap_r(void *a, void *b, int size, const void *priv)
> >         }
> >  }
> >
> > +struct module_addr_args {
> > +       unsigned long *addrs;
> > +       u32 addrs_cnt;
> > +       struct module **mods;
> > +       int mods_cnt;
> > +       int mods_alloc;
> > +};
> > +
> > +static int module_callback(void *data, const char *name,
> > +                          struct module *mod, unsigned long addr)
> > +{
> > +       struct module_addr_args *args = data;
> > +       bool realloc = !args->mods;
> > +       struct module **mods;
> > +
> > +       /* We iterate all modules symbols and for each we:
> > +        * - search for it in provided addresses array
> > +        * - if found we check if we already have the module pointer stored
> > +        *   (we iterate modules sequentially, so we can check just the last
> > +        *   module pointer)
> > +        * - take module reference and store it
> > +        */
> > +       if (!bsearch(&addr, args->addrs, args->addrs_cnt, sizeof(unsigned long),
> > +                      bpf_kprobe_multi_addrs_cmp))
> > +               return 0;
> > +
> > +       if (args->mods) {
> > +               struct module *prev = NULL;
> > +
> > +               if (args->mods_cnt > 1)
> > +                       prev = args->mods[args->mods_cnt - 1];
> > +               if (prev == mod)
> > +                       return 0;
> > +               if (args->mods_cnt == args->mods_alloc)
> > +                       realloc = true;
> > +       }
> > +
> > +       if (realloc) {
> > +               args->mods_alloc += 100;
> > +               mods = krealloc_array(args->mods, args->mods_alloc, sizeof(*mods), GFP_KERNEL);
> > +               if (!mods)
> > +                       return -ENOMEM;
> > +               args->mods = mods;
> > +       }
> > +
> > +       if (!try_module_get(mod))
> > +               return -EINVAL;
> > +
> > +       args->mods[args->mods_cnt] = mod;
> > +       args->mods_cnt++;
> > +       return 0;
> > +}
> > +
> > +static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u32 addrs_cnt)
> > +{
> > +       struct module_addr_args args = {
> > +               .addrs     = addrs,
> > +               .addrs_cnt = addrs_cnt,
> > +       };
> > +       int err;
> > +
> > +       /* We return either err < 0 in case of error, ... */
> > +       err = module_kallsyms_on_each_symbol(module_callback, &args);
> > +       if (err) {
> > +               kprobe_multi_put_modules(args.mods, args.mods_cnt);
> > +               kfree(args.mods);
> > +               return err;
> > +       }
> > +
> > +       /* or number of modules found if everything is ok. */
> > +       *mods = args.mods;
> > +       return args.mods_cnt;
> > +}
> > +
> >  int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> >  {
> >         struct bpf_kprobe_multi_link *link = NULL;
> > @@ -2768,7 +2854,21 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> >                        bpf_kprobe_multi_cookie_cmp,
> >                        bpf_kprobe_multi_cookie_swap,
> >                        link);
> > +       } else {
> > +               /*
> > +                * We need to sort addrs array even if there are no cookies
> > +                * provided, to allow bsearch in get_modules_for_addrs.
> > +                */
> > +               sort(addrs, cnt, sizeof(*addrs),
> > +                      bpf_kprobe_multi_addrs_cmp, NULL);
> > +       }
> > +
> > +       err = get_modules_for_addrs(&link->mods, addrs, cnt);
> > +       if (err < 0) {
> > +               bpf_link_cleanup(&link_primer);
> > +               return err;
> >         }
> > +       link->mods_cnt = err;
> >
> >         err = register_fprobe_ips(&link->fp, addrs, cnt);
> >         if (err) {
> > --
> > 2.37.3
> >

Re: [PATCH bpf-next 4/8] bpf: Take module reference on kprobe_multi link

[Andrii Nakryiko]

On Sun, Oct 9, 2022 at 3:00 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Currently we allow to create kprobe multi link on function from kernel
> module, but we don't take the module reference to ensure it's not
> unloaded while we are tracing it.
>
> The multi kprobe link is based on fprobe/ftrace layer which takes
> different approach and releases ftrace hooks when module is unloaded
> even if there's tracer registered on top of it.
>
> Adding code that gathers all the related modules for the link and takes
> their references before it's attached. All kernel module references are
> released after link is unregistered.
>
> Note that we do it the same way already for trampoline probes
> (but for single address).
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/trace/bpf_trace.c | 100 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 100 insertions(+)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 9be1a2b6b53b..f3d7565fee79 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2447,6 +2447,8 @@ struct bpf_kprobe_multi_link {
>         unsigned long *addrs;
>         u64 *cookies;
>         u32 cnt;
> +       struct module **mods;
> +       u32 mods_cnt;
>  };
>
>  struct bpf_kprobe_multi_run_ctx {
> @@ -2502,6 +2504,14 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
>         return err;
>  }
>
> +static void kprobe_multi_put_modules(struct module **mods, u32 cnt)
> +{
> +       u32 i;
> +
> +       for (i = 0; i < cnt; i++)
> +               module_put(mods[i]);
> +}
> +
>  static void free_user_syms(struct user_syms *us)
>  {
>         kvfree(us->syms);
> @@ -2514,6 +2524,7 @@ static void bpf_kprobe_multi_link_release(struct bpf_link *link)
>
>         kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
>         unregister_fprobe(&kmulti_link->fp);
> +       kprobe_multi_put_modules(kmulti_link->mods, kmulti_link->mods_cnt);
>  }
>
>  static void bpf_kprobe_multi_link_dealloc(struct bpf_link *link)
> @@ -2523,6 +2534,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->mods);
>         kfree(kmulti_link);
>  }
>
> @@ -2658,6 +2670,80 @@ static void symbols_swap_r(void *a, void *b, int size, const void *priv)
>         }
>  }
>
> +struct module_addr_args {
> +       unsigned long *addrs;
> +       u32 addrs_cnt;
> +       struct module **mods;
> +       int mods_cnt;
> +       int mods_alloc;
> +};
> +
> +static int module_callback(void *data, const char *name,
> +                          struct module *mod, unsigned long addr)
> +{
> +       struct module_addr_args *args = data;
> +       bool realloc = !args->mods;
> +       struct module **mods;
> +
> +       /* We iterate all modules symbols and for each we:
> +        * - search for it in provided addresses array
> +        * - if found we check if we already have the module pointer stored
> +        *   (we iterate modules sequentially, so we can check just the last
> +        *   module pointer)
> +        * - take module reference and store it
> +        */
> +       if (!bsearch(&addr, args->addrs, args->addrs_cnt, sizeof(unsigned long),

nit: sizeof(addr) is shorter and will stay in sync with addr variable?

> +                      bpf_kprobe_multi_addrs_cmp))
> +               return 0;
> +
> +       if (args->mods) {
> +               struct module *prev = NULL;
> +
> +               if (args->mods_cnt > 1)
> +                       prev = args->mods[args->mods_cnt - 1];

doesn't args->mods != NULL imply that args->mods_cnt > 1?

> +               if (prev == mod)
> +                       return 0;
> +               if (args->mods_cnt == args->mods_alloc)

nit: in libbpf we consistently use the cnt and cap (capacity)
terminology for this, "mods_alloc" reads like a bool flag or something

> +                       realloc = true;
> +       }
> +
> +       if (realloc) {
> +               args->mods_alloc += 100;

agree with Song, this looks pretty arbitrary and quite large. Again,
from libbpf experience, we do something like:

mods_alloc = max(16, mods_alloc * 3 / 2);

so grow by 50%, but start of with reasonable 16-element array. We can
use similar approach here.

> +               mods = krealloc_array(args->mods, args->mods_alloc, sizeof(*mods), GFP_KERNEL);
> +               if (!mods)
> +                       return -ENOMEM;
> +               args->mods = mods;
> +       }

Previous two blocks read pretty convoluted. Isn't it equivalent to simpler:

if (args->mods && args->mods[args->mods_cnt - 1] == mod)
    return 0;

if (args->mods_cnt == args->mods_alloc /* but I'd use mods_cap */) {
    /* realloc here */
}

> +
> +       if (!try_module_get(mod))
> +               return -EINVAL;
> +
> +       args->mods[args->mods_cnt] = mod;
> +       args->mods_cnt++;
> +       return 0;
> +}
> +

[...]

Re: [PATCH bpf-next 4/8] bpf: Take module reference on kprobe_multi link

[Jiri Olsa]

On Thu, Oct 13, 2022 at 11:50:54AM -0700, Andrii Nakryiko wrote:
> On Sun, Oct 9, 2022 at 3:00 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Currently we allow to create kprobe multi link on function from kernel
> > module, but we don't take the module reference to ensure it's not
> > unloaded while we are tracing it.
> >
> > The multi kprobe link is based on fprobe/ftrace layer which takes
> > different approach and releases ftrace hooks when module is unloaded
> > even if there's tracer registered on top of it.
> >
> > Adding code that gathers all the related modules for the link and takes
> > their references before it's attached. All kernel module references are
> > released after link is unregistered.
> >
> > Note that we do it the same way already for trampoline probes
> > (but for single address).
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/trace/bpf_trace.c | 100 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 100 insertions(+)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 9be1a2b6b53b..f3d7565fee79 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2447,6 +2447,8 @@ struct bpf_kprobe_multi_link {
> >         unsigned long *addrs;
> >         u64 *cookies;
> >         u32 cnt;
> > +       struct module **mods;
> > +       u32 mods_cnt;
> >  };
> >
> >  struct bpf_kprobe_multi_run_ctx {
> > @@ -2502,6 +2504,14 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
> >         return err;
> >  }
> >
> > +static void kprobe_multi_put_modules(struct module **mods, u32 cnt)
> > +{
> > +       u32 i;
> > +
> > +       for (i = 0; i < cnt; i++)
> > +               module_put(mods[i]);
> > +}
> > +
> >  static void free_user_syms(struct user_syms *us)
> >  {
> >         kvfree(us->syms);
> > @@ -2514,6 +2524,7 @@ static void bpf_kprobe_multi_link_release(struct bpf_link *link)
> >
> >         kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
> >         unregister_fprobe(&kmulti_link->fp);
> > +       kprobe_multi_put_modules(kmulti_link->mods, kmulti_link->mods_cnt);
> >  }
> >
> >  static void bpf_kprobe_multi_link_dealloc(struct bpf_link *link)
> > @@ -2523,6 +2534,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->mods);
> >         kfree(kmulti_link);
> >  }
> >
> > @@ -2658,6 +2670,80 @@ static void symbols_swap_r(void *a, void *b, int size, const void *priv)
> >         }
> >  }
> >
> > +struct module_addr_args {
> > +       unsigned long *addrs;
> > +       u32 addrs_cnt;
> > +       struct module **mods;
> > +       int mods_cnt;
> > +       int mods_alloc;
> > +};
> > +
> > +static int module_callback(void *data, const char *name,
> > +                          struct module *mod, unsigned long addr)
> > +{
> > +       struct module_addr_args *args = data;
> > +       bool realloc = !args->mods;
> > +       struct module **mods;
> > +
> > +       /* We iterate all modules symbols and for each we:
> > +        * - search for it in provided addresses array
> > +        * - if found we check if we already have the module pointer stored
> > +        *   (we iterate modules sequentially, so we can check just the last
> > +        *   module pointer)
> > +        * - take module reference and store it
> > +        */
> > +       if (!bsearch(&addr, args->addrs, args->addrs_cnt, sizeof(unsigned long),
> 
> nit: sizeof(addr) is shorter and will stay in sync with addr variable?

ok

> 
> > +                      bpf_kprobe_multi_addrs_cmp))
> > +               return 0;
> > +
> > +       if (args->mods) {
> > +               struct module *prev = NULL;
> > +
> > +               if (args->mods_cnt > 1)
> > +                       prev = args->mods[args->mods_cnt - 1];
> 
> doesn't args->mods != NULL imply that args->mods_cnt > 1?
> 
> > +               if (prev == mod)
> > +                       return 0;
> > +               if (args->mods_cnt == args->mods_alloc)
> 
> nit: in libbpf we consistently use the cnt and cap (capacity)
> terminology for this, "mods_alloc" reads like a bool flag or something

ok

> 
> > +                       realloc = true;
> > +       }
> > +
> > +       if (realloc) {
> > +               args->mods_alloc += 100;
> 
> agree with Song, this looks pretty arbitrary and quite large. Again,
> from libbpf experience, we do something like:
> 
> mods_alloc = max(16, mods_alloc * 3 / 2);
> 
> so grow by 50%, but start of with reasonable 16-element array. We can
> use similar approach here.

ok

> 
> > +               mods = krealloc_array(args->mods, args->mods_alloc, sizeof(*mods), GFP_KERNEL);
> > +               if (!mods)
> > +                       return -ENOMEM;
> > +               args->mods = mods;
> > +       }
> 
> Previous two blocks read pretty convoluted. Isn't it equivalent to simpler:
> 
> if (args->mods && args->mods[args->mods_cnt - 1] == mod)
>     return 0;
> 
> if (args->mods_cnt == args->mods_alloc /* but I'd use mods_cap */) {
>     /* realloc here */
> }

sure, I can chage to that

thanks,
jirka

[PATCH bpf-next 5/8] selftests/bpf: Add load_kallsyms_refresh function

[Jiri Olsa]

Adding load_kallsyms_refresh function to re-read symbols from
/proc/kallsyms file.

This will be needed to get proper functions addresses from
bpf_testmod.ko module, which is loaded/unloaded several times
during the tests run, so symbols might be already old when
we need to use them.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/testing/selftests/bpf/trace_helpers.c | 20 +++++++++++++-------
 tools/testing/selftests/bpf/trace_helpers.h |  2 ++
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
index 9c4be2cdb21a..09a16a77bae4 100644
--- a/tools/testing/selftests/bpf/trace_helpers.c
+++ b/tools/testing/selftests/bpf/trace_helpers.c
@@ -23,7 +23,7 @@ static int ksym_cmp(const void *p1, const void *p2)
 	return ((struct ksym *)p1)->addr - ((struct ksym *)p2)->addr;
 }
 
-int load_kallsyms(void)
+int load_kallsyms_refresh(void)
 {
 	FILE *f;
 	char func[256], buf[256];
@@ -31,12 +31,7 @@ int load_kallsyms(void)
 	void *addr;
 	int i = 0;
 
-	/*
-	 * This is called/used from multiplace places,
-	 * load symbols just once.
-	 */
-	if (sym_cnt)
-		return 0;
+	sym_cnt = 0;
 
 	f = fopen("/proc/kallsyms", "r");
 	if (!f)
@@ -57,6 +52,17 @@ int load_kallsyms(void)
 	return 0;
 }
 
+int load_kallsyms(void)
+{
+	/*
+	 * This is called/used from multiplace places,
+	 * load symbols just once.
+	 */
+	if (sym_cnt)
+		return 0;
+	return load_kallsyms_refresh();
+}
+
 struct ksym *ksym_search(long key)
 {
 	int start = 0, end = sym_cnt;
diff --git a/tools/testing/selftests/bpf/trace_helpers.h b/tools/testing/selftests/bpf/trace_helpers.h
index 238a9c98cde2..53efde0e2998 100644
--- a/tools/testing/selftests/bpf/trace_helpers.h
+++ b/tools/testing/selftests/bpf/trace_helpers.h
@@ -10,6 +10,8 @@ struct ksym {
 };
 
 int load_kallsyms(void);
+int load_kallsyms_refresh(void);
+
 struct ksym *ksym_search(long key);
 long ksym_get_addr(const char *name);
 
-- 
2.37.3

Re: [PATCH bpf-next 5/8] selftests/bpf: Add load_kallsyms_refresh function

[Song Liu]

On Sun, Oct 9, 2022 at 3:00 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding load_kallsyms_refresh function to re-read symbols from
> /proc/kallsyms file.
>
> This will be needed to get proper functions addresses from
> bpf_testmod.ko module, which is loaded/unloaded several times
> during the tests run, so symbols might be already old when
> we need to use them.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <song@kernel.org>

> ---
>  tools/testing/selftests/bpf/trace_helpers.c | 20 +++++++++++++-------
>  tools/testing/selftests/bpf/trace_helpers.h |  2 ++
>  2 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
> index 9c4be2cdb21a..09a16a77bae4 100644
> --- a/tools/testing/selftests/bpf/trace_helpers.c
> +++ b/tools/testing/selftests/bpf/trace_helpers.c
> @@ -23,7 +23,7 @@ static int ksym_cmp(const void *p1, const void *p2)
>         return ((struct ksym *)p1)->addr - ((struct ksym *)p2)->addr;
>  }
>
> -int load_kallsyms(void)
> +int load_kallsyms_refresh(void)
>  {
>         FILE *f;
>         char func[256], buf[256];
> @@ -31,12 +31,7 @@ int load_kallsyms(void)
>         void *addr;
>         int i = 0;
>
> -       /*
> -        * This is called/used from multiplace places,
> -        * load symbols just once.
> -        */
> -       if (sym_cnt)
> -               return 0;
> +       sym_cnt = 0;
>
>         f = fopen("/proc/kallsyms", "r");
>         if (!f)
> @@ -57,6 +52,17 @@ int load_kallsyms(void)
>         return 0;
>  }
>
> +int load_kallsyms(void)
> +{
> +       /*
> +        * This is called/used from multiplace places,
> +        * load symbols just once.
> +        */
> +       if (sym_cnt)
> +               return 0;
> +       return load_kallsyms_refresh();
> +}
> +
>  struct ksym *ksym_search(long key)
>  {
>         int start = 0, end = sym_cnt;
> diff --git a/tools/testing/selftests/bpf/trace_helpers.h b/tools/testing/selftests/bpf/trace_helpers.h
> index 238a9c98cde2..53efde0e2998 100644
> --- a/tools/testing/selftests/bpf/trace_helpers.h
> +++ b/tools/testing/selftests/bpf/trace_helpers.h
> @@ -10,6 +10,8 @@ struct ksym {
>  };
>
>  int load_kallsyms(void);
> +int load_kallsyms_refresh(void);
> +
>  struct ksym *ksym_search(long key);
>  long ksym_get_addr(const char *name);
>
> --
> 2.37.3
>

[PATCH bpf-next 6/8] selftests/bpf: Add bpf_testmod_fentry_* functions

[Jiri Olsa]

Adding 3 bpf_testmod_fentry_* functions to have a way to test
kprobe multi link on kernel module. They follow bpf_fentry_test*
functions prototypes/code.

Adding equivalent functions to all bpf_fentry_test* does not
seems necessary at the moment, could be added later.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index a6021d6117b5..5085fea3cac5 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -128,6 +128,23 @@ __weak noinline struct file *bpf_testmod_return_ptr(int arg)
 	}
 }
 
+noinline int bpf_testmod_fentry_test1(int a)
+{
+	return a + 1;
+}
+
+noinline int bpf_testmod_fentry_test2(int a, u64 b)
+{
+	return a + b;
+}
+
+noinline int bpf_testmod_fentry_test3(char a, int b, u64 c)
+{
+	return a + b + c;
+}
+
+int bpf_testmod_fentry_ok;
+
 noinline ssize_t
 bpf_testmod_test_read(struct file *file, struct kobject *kobj,
 		      struct bin_attribute *bin_attr,
@@ -167,6 +184,13 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj,
 			return snprintf(buf, len, "%d\n", writable.val);
 	}
 
+	if (bpf_testmod_fentry_test1(1) != 2 ||
+	    bpf_testmod_fentry_test2(2, 3) != 5 ||
+	    bpf_testmod_fentry_test3(4, 5, 6) != 15)
+		goto out;
+
+	bpf_testmod_fentry_ok = 1;
+out:
 	return -EIO; /* always fail */
 }
 EXPORT_SYMBOL(bpf_testmod_test_read);
-- 
2.37.3

Re: [PATCH bpf-next 6/8] selftests/bpf: Add bpf_testmod_fentry_* functions

[Song Liu]

On Sun, Oct 9, 2022 at 3:00 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding 3 bpf_testmod_fentry_* functions to have a way to test
> kprobe multi link on kernel module. They follow bpf_fentry_test*
> functions prototypes/code.
>
> Adding equivalent functions to all bpf_fentry_test* does not
> seems necessary at the moment, could be added later.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <song@kernel.org>

> ---
>  .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> index a6021d6117b5..5085fea3cac5 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -128,6 +128,23 @@ __weak noinline struct file *bpf_testmod_return_ptr(int arg)
>         }
>  }
>
> +noinline int bpf_testmod_fentry_test1(int a)
> +{
> +       return a + 1;
> +}
> +
> +noinline int bpf_testmod_fentry_test2(int a, u64 b)
> +{
> +       return a + b;
> +}
> +
> +noinline int bpf_testmod_fentry_test3(char a, int b, u64 c)
> +{
> +       return a + b + c;
> +}
> +
> +int bpf_testmod_fentry_ok;
> +
>  noinline ssize_t
>  bpf_testmod_test_read(struct file *file, struct kobject *kobj,
>                       struct bin_attribute *bin_attr,
> @@ -167,6 +184,13 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj,
>                         return snprintf(buf, len, "%d\n", writable.val);
>         }
>
> +       if (bpf_testmod_fentry_test1(1) != 2 ||
> +           bpf_testmod_fentry_test2(2, 3) != 5 ||
> +           bpf_testmod_fentry_test3(4, 5, 6) != 15)
> +               goto out;
> +
> +       bpf_testmod_fentry_ok = 1;
> +out:
>         return -EIO; /* always fail */
>  }
>  EXPORT_SYMBOL(bpf_testmod_test_read);
> --
> 2.37.3
>

[PATCH bpf-next 7/8] selftests/bpf: Add kprobe_multi kmod link api tests

[Jiri Olsa]

Adding kprobe_multi kmod link api tests that attach bpf_testmod
functions via kprobe_multi link API.

Running it as serial test, because we don't want other tests to
reload bpf_testmod while it's running.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../prog_tests/kprobe_multi_testmod_test.c    | 94 +++++++++++++++++++
 .../selftests/bpf/progs/kprobe_multi.c        | 51 ++++++++++
 2 files changed, 145 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c

diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c
new file mode 100644
index 000000000000..5fe02572650a
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c
@@ -0,0 +1,94 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include "kprobe_multi.skel.h"
+#include "trace_helpers.h"
+#include "bpf/libbpf_internal.h"
+
+static void kprobe_multi_testmod_check(struct kprobe_multi *skel)
+{
+	ASSERT_EQ(skel->bss->kprobe_testmod_test1_result, 1, "kprobe_test1_result");
+	ASSERT_EQ(skel->bss->kprobe_testmod_test2_result, 1, "kprobe_test2_result");
+	ASSERT_EQ(skel->bss->kprobe_testmod_test3_result, 1, "kprobe_test3_result");
+
+	ASSERT_EQ(skel->bss->kretprobe_testmod_test1_result, 1, "kretprobe_test1_result");
+	ASSERT_EQ(skel->bss->kretprobe_testmod_test2_result, 1, "kretprobe_test2_result");
+	ASSERT_EQ(skel->bss->kretprobe_testmod_test3_result, 1, "kretprobe_test3_result");
+}
+
+static void test_testmod_link_api(struct bpf_link_create_opts *opts)
+{
+	int 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;
+
+	skel->bss->pid = getpid();
+	prog_fd = bpf_program__fd(skel->progs.test_kprobe_testmod);
+	link1_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, opts);
+	if (!ASSERT_GE(link1_fd, 0, "link_fd1"))
+		goto cleanup;
+
+	opts->kprobe_multi.flags = BPF_F_KPROBE_MULTI_RETURN;
+	prog_fd = bpf_program__fd(skel->progs.test_kretprobe_testmod);
+	link2_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, opts);
+	if (!ASSERT_GE(link2_fd, 0, "link_fd2"))
+		goto cleanup;
+
+	ASSERT_OK(trigger_module_test_read(1), "trigger_read");
+	kprobe_multi_testmod_check(skel);
+
+cleanup:
+	if (link1_fd != -1)
+		close(link1_fd);
+	if (link2_fd != -1)
+		close(link2_fd);
+	kprobe_multi__destroy(skel);
+}
+
+#define GET_ADDR(__sym, __addr) ({					\
+	__addr = ksym_get_addr(__sym);					\
+	if (!ASSERT_NEQ(__addr, 0, "kallsyms load failed for " #__sym))	\
+		return;							\
+})
+
+static void test_testmod_link_api_addrs(void)
+{
+	LIBBPF_OPTS(bpf_link_create_opts, opts);
+	unsigned long long addrs[3];
+
+	GET_ADDR("bpf_testmod_fentry_test1", addrs[0]);
+	GET_ADDR("bpf_testmod_fentry_test2", addrs[1]);
+	GET_ADDR("bpf_testmod_fentry_test3", addrs[2]);
+
+	opts.kprobe_multi.addrs = (const unsigned long *) addrs;
+	opts.kprobe_multi.cnt = ARRAY_SIZE(addrs);
+
+	test_testmod_link_api(&opts);
+}
+
+static void test_testmod_link_api_syms(void)
+{
+	LIBBPF_OPTS(bpf_link_create_opts, opts);
+	const char *syms[3] = {
+		"bpf_testmod_fentry_test1",
+		"bpf_testmod_fentry_test2",
+		"bpf_testmod_fentry_test3",
+	};
+
+	opts.kprobe_multi.syms = syms;
+	opts.kprobe_multi.cnt = ARRAY_SIZE(syms);
+	test_testmod_link_api(&opts);
+}
+
+void serial_test_kprobe_multi_testmod_test(void)
+{
+	if (!ASSERT_OK(load_kallsyms_refresh(), "load_kallsyms_refresh"))
+		return;
+
+	if (test__start_subtest("testmod_link_api_syms"))
+		test_testmod_link_api_syms();
+	if (test__start_subtest("testmod_link_api_addrs"))
+		test_testmod_link_api_addrs();
+}
diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi.c b/tools/testing/selftests/bpf/progs/kprobe_multi.c
index 98c3399e15c0..b3c54ec13a45 100644
--- a/tools/testing/selftests/bpf/progs/kprobe_multi.c
+++ b/tools/testing/selftests/bpf/progs/kprobe_multi.c
@@ -110,3 +110,54 @@ int test_kretprobe_manual(struct pt_regs *ctx)
 	kprobe_multi_check(ctx, true);
 	return 0;
 }
+
+extern const void bpf_testmod_fentry_test1 __ksym;
+extern const void bpf_testmod_fentry_test2 __ksym;
+extern const void bpf_testmod_fentry_test3 __ksym;
+
+__u64 kprobe_testmod_test1_result = 0;
+__u64 kprobe_testmod_test2_result = 0;
+__u64 kprobe_testmod_test3_result = 0;
+
+__u64 kretprobe_testmod_test1_result = 0;
+__u64 kretprobe_testmod_test2_result = 0;
+__u64 kretprobe_testmod_test3_result = 0;
+
+static void kprobe_multi_testmod_check(void *ctx, bool is_return)
+{
+	if (bpf_get_current_pid_tgid() >> 32 != pid)
+		return;
+
+	__u64 addr = bpf_get_func_ip(ctx);
+
+#define SET(__var, __addr) ({				\
+	if ((const void *) addr == __addr)		\
+		__var = 1;				\
+})
+
+	if (is_return) {
+		SET(kretprobe_testmod_test1_result, &bpf_testmod_fentry_test1);
+		SET(kretprobe_testmod_test2_result, &bpf_testmod_fentry_test2);
+		SET(kretprobe_testmod_test3_result, &bpf_testmod_fentry_test3);
+	} else {
+		SET(kprobe_testmod_test1_result, &bpf_testmod_fentry_test1);
+		SET(kprobe_testmod_test2_result, &bpf_testmod_fentry_test2);
+		SET(kprobe_testmod_test3_result, &bpf_testmod_fentry_test3);
+	}
+
+#undef SET
+}
+
+SEC("kprobe.multi")
+int test_kprobe_testmod(struct pt_regs *ctx)
+{
+	kprobe_multi_testmod_check(ctx, false);
+	return 0;
+}
+
+SEC("kretprobe.multi")
+int test_kretprobe_testmod(struct pt_regs *ctx)
+{
+	kprobe_multi_testmod_check(ctx, true);
+	return 0;
+}
-- 
2.37.3

Re: [PATCH bpf-next 7/8] selftests/bpf: Add kprobe_multi kmod link api tests

[Song Liu]

On Sun, Oct 9, 2022 at 3:01 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding kprobe_multi kmod link api tests that attach bpf_testmod
> functions via kprobe_multi link API.
>
> Running it as serial test, because we don't want other tests to
> reload bpf_testmod while it's running.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../prog_tests/kprobe_multi_testmod_test.c    | 94 +++++++++++++++++++
>  .../selftests/bpf/progs/kprobe_multi.c        | 51 ++++++++++
>  2 files changed, 145 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c
> new file mode 100644
> index 000000000000..5fe02572650a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c
> @@ -0,0 +1,94 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <test_progs.h>
> +#include "kprobe_multi.skel.h"
> +#include "trace_helpers.h"
> +#include "bpf/libbpf_internal.h"
> +
> +static void kprobe_multi_testmod_check(struct kprobe_multi *skel)
> +{
> +       ASSERT_EQ(skel->bss->kprobe_testmod_test1_result, 1, "kprobe_test1_result");
> +       ASSERT_EQ(skel->bss->kprobe_testmod_test2_result, 1, "kprobe_test2_result");
> +       ASSERT_EQ(skel->bss->kprobe_testmod_test3_result, 1, "kprobe_test3_result");
> +
> +       ASSERT_EQ(skel->bss->kretprobe_testmod_test1_result, 1, "kretprobe_test1_result");
> +       ASSERT_EQ(skel->bss->kretprobe_testmod_test2_result, 1, "kretprobe_test2_result");
> +       ASSERT_EQ(skel->bss->kretprobe_testmod_test3_result, 1, "kretprobe_test3_result");
> +}
> +
> +static void test_testmod_link_api(struct bpf_link_create_opts *opts)
> +{
> +       int 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;

nit: we can just return here.

Other than this:

Acked-by: Song Liu <song@kernel.org>

> +
> +       skel->bss->pid = getpid();
> +       prog_fd = bpf_program__fd(skel->progs.test_kprobe_testmod);
> +       link1_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, opts);
> +       if (!ASSERT_GE(link1_fd, 0, "link_fd1"))
> +               goto cleanup;
> +
> +       opts->kprobe_multi.flags = BPF_F_KPROBE_MULTI_RETURN;
> +       prog_fd = bpf_program__fd(skel->progs.test_kretprobe_testmod);
> +       link2_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, opts);
> +       if (!ASSERT_GE(link2_fd, 0, "link_fd2"))
> +               goto cleanup;
> +
> +       ASSERT_OK(trigger_module_test_read(1), "trigger_read");
> +       kprobe_multi_testmod_check(skel);
> +
> +cleanup:
> +       if (link1_fd != -1)
> +               close(link1_fd);
> +       if (link2_fd != -1)
> +               close(link2_fd);
> +       kprobe_multi__destroy(skel);
> +}
>

Re: [PATCH bpf-next 7/8] selftests/bpf: Add kprobe_multi kmod link api tests

[Jiri Olsa]

On Tue, Oct 11, 2022 at 12:27:12AM -0700, Song Liu wrote:
> On Sun, Oct 9, 2022 at 3:01 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding kprobe_multi kmod link api tests that attach bpf_testmod
> > functions via kprobe_multi link API.
> >
> > Running it as serial test, because we don't want other tests to
> > reload bpf_testmod while it's running.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../prog_tests/kprobe_multi_testmod_test.c    | 94 +++++++++++++++++++
> >  .../selftests/bpf/progs/kprobe_multi.c        | 51 ++++++++++
> >  2 files changed, 145 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c
> > new file mode 100644
> > index 000000000000..5fe02572650a
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c
> > @@ -0,0 +1,94 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <test_progs.h>
> > +#include "kprobe_multi.skel.h"
> > +#include "trace_helpers.h"
> > +#include "bpf/libbpf_internal.h"
> > +
> > +static void kprobe_multi_testmod_check(struct kprobe_multi *skel)
> > +{
> > +       ASSERT_EQ(skel->bss->kprobe_testmod_test1_result, 1, "kprobe_test1_result");
> > +       ASSERT_EQ(skel->bss->kprobe_testmod_test2_result, 1, "kprobe_test2_result");
> > +       ASSERT_EQ(skel->bss->kprobe_testmod_test3_result, 1, "kprobe_test3_result");
> > +
> > +       ASSERT_EQ(skel->bss->kretprobe_testmod_test1_result, 1, "kretprobe_test1_result");
> > +       ASSERT_EQ(skel->bss->kretprobe_testmod_test2_result, 1, "kretprobe_test2_result");
> > +       ASSERT_EQ(skel->bss->kretprobe_testmod_test3_result, 1, "kretprobe_test3_result");
> > +}
> > +
> > +static void test_testmod_link_api(struct bpf_link_create_opts *opts)
> > +{
> > +       int 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;
> 
> nit: we can just return here.

right, will change

thanks,
jirka

> 
> Other than this:
> 
> Acked-by: Song Liu <song@kernel.org>
> 
> > +
> > +       skel->bss->pid = getpid();
> > +       prog_fd = bpf_program__fd(skel->progs.test_kprobe_testmod);
> > +       link1_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, opts);
> > +       if (!ASSERT_GE(link1_fd, 0, "link_fd1"))
> > +               goto cleanup;
> > +
> > +       opts->kprobe_multi.flags = BPF_F_KPROBE_MULTI_RETURN;
> > +       prog_fd = bpf_program__fd(skel->progs.test_kretprobe_testmod);
> > +       link2_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, opts);
> > +       if (!ASSERT_GE(link2_fd, 0, "link_fd2"))
> > +               goto cleanup;
> > +
> > +       ASSERT_OK(trigger_module_test_read(1), "trigger_read");
> > +       kprobe_multi_testmod_check(skel);
> > +
> > +cleanup:
> > +       if (link1_fd != -1)
> > +               close(link1_fd);
> > +       if (link2_fd != -1)
> > +               close(link2_fd);
> > +       kprobe_multi__destroy(skel);
> > +}
> >

Re: [PATCH bpf-next 7/8] selftests/bpf: Add kprobe_multi kmod link api tests

[Andrii Nakryiko]

On Sun, Oct 9, 2022 at 3:00 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding kprobe_multi kmod link api tests that attach bpf_testmod
> functions via kprobe_multi link API.
>
> Running it as serial test, because we don't want other tests to
> reload bpf_testmod while it's running.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../prog_tests/kprobe_multi_testmod_test.c    | 94 +++++++++++++++++++
>  .../selftests/bpf/progs/kprobe_multi.c        | 51 ++++++++++
>  2 files changed, 145 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c
> new file mode 100644
> index 000000000000..5fe02572650a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c
> @@ -0,0 +1,94 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <test_progs.h>
> +#include "kprobe_multi.skel.h"
> +#include "trace_helpers.h"
> +#include "bpf/libbpf_internal.h"
> +
> +static void kprobe_multi_testmod_check(struct kprobe_multi *skel)
> +{
> +       ASSERT_EQ(skel->bss->kprobe_testmod_test1_result, 1, "kprobe_test1_result");
> +       ASSERT_EQ(skel->bss->kprobe_testmod_test2_result, 1, "kprobe_test2_result");
> +       ASSERT_EQ(skel->bss->kprobe_testmod_test3_result, 1, "kprobe_test3_result");
> +
> +       ASSERT_EQ(skel->bss->kretprobe_testmod_test1_result, 1, "kretprobe_test1_result");
> +       ASSERT_EQ(skel->bss->kretprobe_testmod_test2_result, 1, "kretprobe_test2_result");
> +       ASSERT_EQ(skel->bss->kretprobe_testmod_test3_result, 1, "kretprobe_test3_result");
> +}
> +
> +static void test_testmod_link_api(struct bpf_link_create_opts *opts)
> +{
> +       int 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;
> +
> +       skel->bss->pid = getpid();
> +       prog_fd = bpf_program__fd(skel->progs.test_kprobe_testmod);
> +       link1_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, opts);
> +       if (!ASSERT_GE(link1_fd, 0, "link_fd1"))
> +               goto cleanup;
> +
> +       opts->kprobe_multi.flags = BPF_F_KPROBE_MULTI_RETURN;
> +       prog_fd = bpf_program__fd(skel->progs.test_kretprobe_testmod);
> +       link2_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, opts);
> +       if (!ASSERT_GE(link2_fd, 0, "link_fd2"))
> +               goto cleanup;
> +

any reason to not use bpf_program__attach_kprobe_multi_ops() and
instead use low-level bpf_link_create?

> +       ASSERT_OK(trigger_module_test_read(1), "trigger_read");
> +       kprobe_multi_testmod_check(skel);
> +
> +cleanup:
> +       if (link1_fd != -1)
> +               close(link1_fd);
> +       if (link2_fd != -1)
> +               close(link2_fd);

you don't need to even do this if you stick to high-level attach APIs

> +       kprobe_multi__destroy(skel);
> +}
> +
> +#define GET_ADDR(__sym, __addr) ({                                     \
> +       __addr = ksym_get_addr(__sym);                                  \
> +       if (!ASSERT_NEQ(__addr, 0, "kallsyms load failed for " #__sym)) \
> +               return;                                                 \
> +})

macro for this? why? just make understanding the code and debugging
it, if necessary, harder. You don't even need that return, just lookup
and ASSERT_NEQ(). Go to symbol #2 and do the same. If something goes
wrong you'll have three failed ASSERT_NEQs, which is totally fine.

> +
> +static void test_testmod_link_api_addrs(void)
> +{
> +       LIBBPF_OPTS(bpf_link_create_opts, opts);
> +       unsigned long long addrs[3];
> +
> +       GET_ADDR("bpf_testmod_fentry_test1", addrs[0]);
> +       GET_ADDR("bpf_testmod_fentry_test2", addrs[1]);
> +       GET_ADDR("bpf_testmod_fentry_test3", addrs[2]);
> +
> +       opts.kprobe_multi.addrs = (const unsigned long *) addrs;
> +       opts.kprobe_multi.cnt = ARRAY_SIZE(addrs);
> +
> +       test_testmod_link_api(&opts);
> +}
> +
> +static void test_testmod_link_api_syms(void)
> +{
> +       LIBBPF_OPTS(bpf_link_create_opts, opts);
> +       const char *syms[3] = {
> +               "bpf_testmod_fentry_test1",
> +               "bpf_testmod_fentry_test2",
> +               "bpf_testmod_fentry_test3",
> +       };
> +
> +       opts.kprobe_multi.syms = syms;
> +       opts.kprobe_multi.cnt = ARRAY_SIZE(syms);
> +       test_testmod_link_api(&opts);
> +}
> +
> +void serial_test_kprobe_multi_testmod_test(void)
> +{
> +       if (!ASSERT_OK(load_kallsyms_refresh(), "load_kallsyms_refresh"))
> +               return;
> +
> +       if (test__start_subtest("testmod_link_api_syms"))
> +               test_testmod_link_api_syms();
> +       if (test__start_subtest("testmod_link_api_addrs"))
> +               test_testmod_link_api_addrs();
> +}
> diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi.c b/tools/testing/selftests/bpf/progs/kprobe_multi.c
> index 98c3399e15c0..b3c54ec13a45 100644
> --- a/tools/testing/selftests/bpf/progs/kprobe_multi.c
> +++ b/tools/testing/selftests/bpf/progs/kprobe_multi.c
> @@ -110,3 +110,54 @@ int test_kretprobe_manual(struct pt_regs *ctx)
>         kprobe_multi_check(ctx, true);
>         return 0;
>  }
> +
> +extern const void bpf_testmod_fentry_test1 __ksym;
> +extern const void bpf_testmod_fentry_test2 __ksym;
> +extern const void bpf_testmod_fentry_test3 __ksym;
> +
> +__u64 kprobe_testmod_test1_result = 0;
> +__u64 kprobe_testmod_test2_result = 0;
> +__u64 kprobe_testmod_test3_result = 0;
> +
> +__u64 kretprobe_testmod_test1_result = 0;
> +__u64 kretprobe_testmod_test2_result = 0;
> +__u64 kretprobe_testmod_test3_result = 0;
> +
> +static void kprobe_multi_testmod_check(void *ctx, bool is_return)
> +{
> +       if (bpf_get_current_pid_tgid() >> 32 != pid)
> +               return;
> +
> +       __u64 addr = bpf_get_func_ip(ctx);
> +
> +#define SET(__var, __addr) ({                          \
> +       if ((const void *) addr == __addr)              \
> +               __var = 1;                              \
> +})
> +

same feedback, why macro for this? There is nothing repetitive done in it at all

> +       if (is_return) {
> +               SET(kretprobe_testmod_test1_result, &bpf_testmod_fentry_test1);
> +               SET(kretprobe_testmod_test2_result, &bpf_testmod_fentry_test2);
> +               SET(kretprobe_testmod_test3_result, &bpf_testmod_fentry_test3);
> +       } else {
> +               SET(kprobe_testmod_test1_result, &bpf_testmod_fentry_test1);
> +               SET(kprobe_testmod_test2_result, &bpf_testmod_fentry_test2);
> +               SET(kprobe_testmod_test3_result, &bpf_testmod_fentry_test3);
> +       }
> +
> +#undef SET
> +}
> +
> +SEC("kprobe.multi")
> +int test_kprobe_testmod(struct pt_regs *ctx)
> +{
> +       kprobe_multi_testmod_check(ctx, false);
> +       return 0;
> +}
> +
> +SEC("kretprobe.multi")
> +int test_kretprobe_testmod(struct pt_regs *ctx)
> +{
> +       kprobe_multi_testmod_check(ctx, true);
> +       return 0;
> +}
> --
> 2.37.3
>

Re: [PATCH bpf-next 7/8] selftests/bpf: Add kprobe_multi kmod link api tests

[Jiri Olsa]

On Thu, Oct 13, 2022 at 12:06:06PM -0700, Andrii Nakryiko wrote:

SNIP

> > +static void test_testmod_link_api(struct bpf_link_create_opts *opts)
> > +{
> > +       int 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;
> > +
> > +       skel->bss->pid = getpid();
> > +       prog_fd = bpf_program__fd(skel->progs.test_kprobe_testmod);
> > +       link1_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, opts);
> > +       if (!ASSERT_GE(link1_fd, 0, "link_fd1"))
> > +               goto cleanup;
> > +
> > +       opts->kprobe_multi.flags = BPF_F_KPROBE_MULTI_RETURN;
> > +       prog_fd = bpf_program__fd(skel->progs.test_kretprobe_testmod);
> > +       link2_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, opts);
> > +       if (!ASSERT_GE(link2_fd, 0, "link_fd2"))
> > +               goto cleanup;
> > +
> 
> any reason to not use bpf_program__attach_kprobe_multi_ops() and
> instead use low-level bpf_link_create?
> 
> > +       ASSERT_OK(trigger_module_test_read(1), "trigger_read");
> > +       kprobe_multi_testmod_check(skel);
> > +
> > +cleanup:
> > +       if (link1_fd != -1)
> > +               close(link1_fd);
> > +       if (link2_fd != -1)
> > +               close(link2_fd);
> 
> you don't need to even do this if you stick to high-level attach APIs

ok, I guess we can use bpf_program__attach_kprobe_multi_opts here

> 
> > +       kprobe_multi__destroy(skel);
> > +}
> > +
> > +#define GET_ADDR(__sym, __addr) ({                                     \
> > +       __addr = ksym_get_addr(__sym);                                  \
> > +       if (!ASSERT_NEQ(__addr, 0, "kallsyms load failed for " #__sym)) \
> > +               return;                                                 \
> > +})
> 
> macro for this? why? just make understanding the code and debugging
> it, if necessary, harder. You don't even need that return, just lookup
> and ASSERT_NEQ(). Go to symbol #2 and do the same. If something goes
> wrong you'll have three failed ASSERT_NEQs, which is totally fine.

sure

SNIP

> > diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi.c b/tools/testing/selftests/bpf/progs/kprobe_multi.c
> > index 98c3399e15c0..b3c54ec13a45 100644
> > --- a/tools/testing/selftests/bpf/progs/kprobe_multi.c
> > +++ b/tools/testing/selftests/bpf/progs/kprobe_multi.c
> > @@ -110,3 +110,54 @@ int test_kretprobe_manual(struct pt_regs *ctx)
> >         kprobe_multi_check(ctx, true);
> >         return 0;
> >  }
> > +
> > +extern const void bpf_testmod_fentry_test1 __ksym;
> > +extern const void bpf_testmod_fentry_test2 __ksym;
> > +extern const void bpf_testmod_fentry_test3 __ksym;
> > +
> > +__u64 kprobe_testmod_test1_result = 0;
> > +__u64 kprobe_testmod_test2_result = 0;
> > +__u64 kprobe_testmod_test3_result = 0;
> > +
> > +__u64 kretprobe_testmod_test1_result = 0;
> > +__u64 kretprobe_testmod_test2_result = 0;
> > +__u64 kretprobe_testmod_test3_result = 0;
> > +
> > +static void kprobe_multi_testmod_check(void *ctx, bool is_return)
> > +{
> > +       if (bpf_get_current_pid_tgid() >> 32 != pid)
> > +               return;
> > +
> > +       __u64 addr = bpf_get_func_ip(ctx);
> > +
> > +#define SET(__var, __addr) ({                          \
> > +       if ((const void *) addr == __addr)              \
> > +               __var = 1;                              \
> > +})
> > +
> 
> same feedback, why macro for this? There is nothing repetitive done in it at all

ok, will change

thanks,
jirka

[PATCH bpf-next 8/8] selftests/bpf: Add kprobe_multi check to module attach test

[Jiri Olsa]

Adding test that makes sure the kernel module won't be removed
if there's kprobe multi link defined on top of it.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/testing/selftests/bpf/prog_tests/module_attach.c | 7 +++++++
 tools/testing/selftests/bpf/progs/test_module_attach.c | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach.c b/tools/testing/selftests/bpf/prog_tests/module_attach.c
index 6d0e50dcf47c..7fc01ff490db 100644
--- a/tools/testing/selftests/bpf/prog_tests/module_attach.c
+++ b/tools/testing/selftests/bpf/prog_tests/module_attach.c
@@ -103,6 +103,13 @@ void test_module_attach(void)
 	ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module");
 	bpf_link__destroy(link);
 
+	link = bpf_program__attach(skel->progs.kprobe_multi);
+	if (!ASSERT_OK_PTR(link, "attach_kprobe_multi"))
+		goto cleanup;
+
+	ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module");
+	bpf_link__destroy(link);
+
 cleanup:
 	test_module_attach__destroy(skel);
 }
diff --git a/tools/testing/selftests/bpf/progs/test_module_attach.c b/tools/testing/selftests/bpf/progs/test_module_attach.c
index 08628afedb77..8a1b50f3a002 100644
--- a/tools/testing/selftests/bpf/progs/test_module_attach.c
+++ b/tools/testing/selftests/bpf/progs/test_module_attach.c
@@ -110,4 +110,10 @@ int BPF_PROG(handle_fmod_ret,
 	return 0; /* don't override the exit code */
 }
 
+SEC("kprobe.multi/bpf_testmod_test_read")
+int BPF_PROG(kprobe_multi)
+{
+	return 0;
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.37.3

Re: [PATCH bpf-next 8/8] selftests/bpf: Add kprobe_multi check to module attach test

[Song Liu]

On Sun, Oct 9, 2022 at 3:01 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding test that makes sure the kernel module won't be removed
> if there's kprobe multi link defined on top of it.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <song@kernel.org>

> ---
>  tools/testing/selftests/bpf/prog_tests/module_attach.c | 7 +++++++
>  tools/testing/selftests/bpf/progs/test_module_attach.c | 6 ++++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach.c b/tools/testing/selftests/bpf/prog_tests/module_attach.c
> index 6d0e50dcf47c..7fc01ff490db 100644
> --- a/tools/testing/selftests/bpf/prog_tests/module_attach.c
> +++ b/tools/testing/selftests/bpf/prog_tests/module_attach.c
> @@ -103,6 +103,13 @@ void test_module_attach(void)
>         ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module");
>         bpf_link__destroy(link);
>
> +       link = bpf_program__attach(skel->progs.kprobe_multi);
> +       if (!ASSERT_OK_PTR(link, "attach_kprobe_multi"))
> +               goto cleanup;
> +
> +       ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module");
> +       bpf_link__destroy(link);
> +
>  cleanup:
>         test_module_attach__destroy(skel);
>  }
> diff --git a/tools/testing/selftests/bpf/progs/test_module_attach.c b/tools/testing/selftests/bpf/progs/test_module_attach.c
> index 08628afedb77..8a1b50f3a002 100644
> --- a/tools/testing/selftests/bpf/progs/test_module_attach.c
> +++ b/tools/testing/selftests/bpf/progs/test_module_attach.c
> @@ -110,4 +110,10 @@ int BPF_PROG(handle_fmod_ret,
>         return 0; /* don't override the exit code */
>  }
>
> +SEC("kprobe.multi/bpf_testmod_test_read")
> +int BPF_PROG(kprobe_multi)
> +{
> +       return 0;
> +}
> +
>  char _license[] SEC("license") = "GPL";
> --
> 2.37.3
>