* [PATCH bpf-next 0/8] bpf: Fixes for kprobe multi on kernel modules
@ 2022-10-09 21:59 Jiri Olsa
2022-10-09 21:59 ` [PATCH bpf-next 1/8] kallsyms: Make module_kallsyms_on_each_symbol generally available Jiri Olsa
` (7 more replies)
0 siblings, 8 replies; 24+ messages in thread
From: Jiri Olsa @ 2022-10-09 21:59 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Christoph Hellwig,
Masami Hiramatsu, Martynas Pumputis
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH bpf-next 1/8] kallsyms: Make module_kallsyms_on_each_symbol generally available
2022-10-09 21:59 [PATCH bpf-next 0/8] bpf: Fixes for kprobe multi on kernel modules Jiri Olsa
@ 2022-10-09 21:59 ` Jiri Olsa
2022-10-11 6:56 ` Song Liu
2022-10-09 21:59 ` [PATCH bpf-next 2/8] ftrace: Add support to resolve module symbols in ftrace_lookup_symbols Jiri Olsa
` (6 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Jiri Olsa @ 2022-10-09 21:59 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: Christoph Hellwig, bpf, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Masami Hiramatsu, Martynas Pumputis
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH bpf-next 2/8] ftrace: Add support to resolve module symbols in ftrace_lookup_symbols
2022-10-09 21:59 [PATCH bpf-next 0/8] bpf: Fixes for kprobe multi on kernel modules Jiri Olsa
2022-10-09 21:59 ` [PATCH bpf-next 1/8] kallsyms: Make module_kallsyms_on_each_symbol generally available Jiri Olsa
@ 2022-10-09 21:59 ` Jiri Olsa
2022-10-11 7:05 ` Song Liu
2022-10-09 21:59 ` [PATCH bpf-next 3/8] bpf: Rename __bpf_kprobe_multi_cookie_cmp to bpf_kprobe_multi_addrs_cmp Jiri Olsa
` (5 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Jiri Olsa @ 2022-10-09 21:59 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: Martynas Pumputis, bpf, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Christoph Hellwig, Masami Hiramatsu
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH bpf-next 3/8] bpf: Rename __bpf_kprobe_multi_cookie_cmp to bpf_kprobe_multi_addrs_cmp
2022-10-09 21:59 [PATCH bpf-next 0/8] bpf: Fixes for kprobe multi on kernel modules Jiri Olsa
2022-10-09 21:59 ` [PATCH bpf-next 1/8] kallsyms: Make module_kallsyms_on_each_symbol generally available Jiri Olsa
2022-10-09 21:59 ` [PATCH bpf-next 2/8] ftrace: Add support to resolve module symbols in ftrace_lookup_symbols Jiri Olsa
@ 2022-10-09 21:59 ` Jiri Olsa
2022-10-11 7:06 ` Song Liu
2022-10-09 21:59 ` [PATCH bpf-next 4/8] bpf: Take module reference on kprobe_multi link Jiri Olsa
` (4 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Jiri Olsa @ 2022-10-09 21:59 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Christoph Hellwig,
Masami Hiramatsu, Martynas Pumputis
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH bpf-next 4/8] bpf: Take module reference on kprobe_multi link
2022-10-09 21:59 [PATCH bpf-next 0/8] bpf: Fixes for kprobe multi on kernel modules Jiri Olsa
` (2 preceding siblings ...)
2022-10-09 21:59 ` [PATCH bpf-next 3/8] bpf: Rename __bpf_kprobe_multi_cookie_cmp to bpf_kprobe_multi_addrs_cmp Jiri Olsa
@ 2022-10-09 21:59 ` Jiri Olsa
2022-10-11 7:16 ` Song Liu
2022-10-13 18:50 ` Andrii Nakryiko
2022-10-09 21:59 ` [PATCH bpf-next 5/8] selftests/bpf: Add load_kallsyms_refresh function Jiri Olsa
` (3 subsequent siblings)
7 siblings, 2 replies; 24+ messages in thread
From: Jiri Olsa @ 2022-10-09 21:59 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Christoph Hellwig,
Masami Hiramatsu, Martynas Pumputis
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH bpf-next 5/8] selftests/bpf: Add load_kallsyms_refresh function
2022-10-09 21:59 [PATCH bpf-next 0/8] bpf: Fixes for kprobe multi on kernel modules Jiri Olsa
` (3 preceding siblings ...)
2022-10-09 21:59 ` [PATCH bpf-next 4/8] bpf: Take module reference on kprobe_multi link Jiri Olsa
@ 2022-10-09 21:59 ` Jiri Olsa
2022-10-11 7:17 ` Song Liu
2022-10-09 21:59 ` [PATCH bpf-next 6/8] selftests/bpf: Add bpf_testmod_fentry_* functions Jiri Olsa
` (2 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Jiri Olsa @ 2022-10-09 21:59 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Christoph Hellwig,
Masami Hiramatsu, Martynas Pumputis
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH bpf-next 6/8] selftests/bpf: Add bpf_testmod_fentry_* functions
2022-10-09 21:59 [PATCH bpf-next 0/8] bpf: Fixes for kprobe multi on kernel modules Jiri Olsa
` (4 preceding siblings ...)
2022-10-09 21:59 ` [PATCH bpf-next 5/8] selftests/bpf: Add load_kallsyms_refresh function Jiri Olsa
@ 2022-10-09 21:59 ` Jiri Olsa
2022-10-11 7:24 ` Song Liu
2022-10-09 21:59 ` [PATCH bpf-next 7/8] selftests/bpf: Add kprobe_multi kmod link api tests Jiri Olsa
2022-10-09 21:59 ` [PATCH bpf-next 8/8] selftests/bpf: Add kprobe_multi check to module attach test Jiri Olsa
7 siblings, 1 reply; 24+ messages in thread
From: Jiri Olsa @ 2022-10-09 21:59 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Christoph Hellwig,
Masami Hiramatsu, Martynas Pumputis
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH bpf-next 7/8] selftests/bpf: Add kprobe_multi kmod link api tests
2022-10-09 21:59 [PATCH bpf-next 0/8] bpf: Fixes for kprobe multi on kernel modules Jiri Olsa
` (5 preceding siblings ...)
2022-10-09 21:59 ` [PATCH bpf-next 6/8] selftests/bpf: Add bpf_testmod_fentry_* functions Jiri Olsa
@ 2022-10-09 21:59 ` Jiri Olsa
2022-10-11 7:27 ` Song Liu
2022-10-13 19:06 ` Andrii Nakryiko
2022-10-09 21:59 ` [PATCH bpf-next 8/8] selftests/bpf: Add kprobe_multi check to module attach test Jiri Olsa
7 siblings, 2 replies; 24+ messages in thread
From: Jiri Olsa @ 2022-10-09 21:59 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Christoph Hellwig,
Masami Hiramatsu, Martynas Pumputis
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH bpf-next 8/8] selftests/bpf: Add kprobe_multi check to module attach test
2022-10-09 21:59 [PATCH bpf-next 0/8] bpf: Fixes for kprobe multi on kernel modules Jiri Olsa
` (6 preceding siblings ...)
2022-10-09 21:59 ` [PATCH bpf-next 7/8] selftests/bpf: Add kprobe_multi kmod link api tests Jiri Olsa
@ 2022-10-09 21:59 ` Jiri Olsa
2022-10-11 7:27 ` Song Liu
7 siblings, 1 reply; 24+ messages in thread
From: Jiri Olsa @ 2022-10-09 21:59 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Christoph Hellwig,
Masami Hiramatsu, Martynas Pumputis
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next 1/8] kallsyms: Make module_kallsyms_on_each_symbol generally available
2022-10-09 21:59 ` [PATCH bpf-next 1/8] kallsyms: Make module_kallsyms_on_each_symbol generally available Jiri Olsa
@ 2022-10-11 6:56 ` Song Liu
0 siblings, 0 replies; 24+ messages in thread
From: Song Liu @ 2022-10-11 6:56 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Christoph Hellwig, bpf, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Masami Hiramatsu, Martynas Pumputis
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>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next 2/8] ftrace: Add support to resolve module symbols in ftrace_lookup_symbols
2022-10-09 21:59 ` [PATCH bpf-next 2/8] ftrace: Add support to resolve module symbols in ftrace_lookup_symbols Jiri Olsa
@ 2022-10-11 7:05 ` Song Liu
2022-10-11 10:07 ` Jiri Olsa
0 siblings, 1 reply; 24+ messages in thread
From: Song Liu @ 2022-10-11 7:05 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martynas Pumputis, bpf, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Christoph Hellwig, Masami Hiramatsu
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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next 3/8] bpf: Rename __bpf_kprobe_multi_cookie_cmp to bpf_kprobe_multi_addrs_cmp
2022-10-09 21:59 ` [PATCH bpf-next 3/8] bpf: Rename __bpf_kprobe_multi_cookie_cmp to bpf_kprobe_multi_addrs_cmp Jiri Olsa
@ 2022-10-11 7:06 ` Song Liu
0 siblings, 0 replies; 24+ messages in thread
From: Song Liu @ 2022-10-11 7:06 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Christoph Hellwig,
Masami Hiramatsu, Martynas Pumputis
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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next 4/8] bpf: Take module reference on kprobe_multi link
2022-10-09 21:59 ` [PATCH bpf-next 4/8] bpf: Take module reference on kprobe_multi link Jiri Olsa
@ 2022-10-11 7:16 ` Song Liu
2022-10-11 10:09 ` Jiri Olsa
2022-10-13 18:50 ` Andrii Nakryiko
1 sibling, 1 reply; 24+ messages in thread
From: Song Liu @ 2022-10-11 7:16 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Christoph Hellwig,
Masami Hiramatsu, Martynas Pumputis
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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next 5/8] selftests/bpf: Add load_kallsyms_refresh function
2022-10-09 21:59 ` [PATCH bpf-next 5/8] selftests/bpf: Add load_kallsyms_refresh function Jiri Olsa
@ 2022-10-11 7:17 ` Song Liu
0 siblings, 0 replies; 24+ messages in thread
From: Song Liu @ 2022-10-11 7:17 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Christoph Hellwig,
Masami Hiramatsu, Martynas Pumputis
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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next 6/8] selftests/bpf: Add bpf_testmod_fentry_* functions
2022-10-09 21:59 ` [PATCH bpf-next 6/8] selftests/bpf: Add bpf_testmod_fentry_* functions Jiri Olsa
@ 2022-10-11 7:24 ` Song Liu
0 siblings, 0 replies; 24+ messages in thread
From: Song Liu @ 2022-10-11 7:24 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Christoph Hellwig,
Masami Hiramatsu, Martynas Pumputis
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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next 7/8] selftests/bpf: Add kprobe_multi kmod link api tests
2022-10-09 21:59 ` [PATCH bpf-next 7/8] selftests/bpf: Add kprobe_multi kmod link api tests Jiri Olsa
@ 2022-10-11 7:27 ` Song Liu
2022-10-11 10:09 ` Jiri Olsa
2022-10-13 19:06 ` Andrii Nakryiko
1 sibling, 1 reply; 24+ messages in thread
From: Song Liu @ 2022-10-11 7:27 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Christoph Hellwig,
Masami Hiramatsu, Martynas Pumputis
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);
> +}
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next 8/8] selftests/bpf: Add kprobe_multi check to module attach test
2022-10-09 21:59 ` [PATCH bpf-next 8/8] selftests/bpf: Add kprobe_multi check to module attach test Jiri Olsa
@ 2022-10-11 7:27 ` Song Liu
0 siblings, 0 replies; 24+ messages in thread
From: Song Liu @ 2022-10-11 7:27 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Christoph Hellwig,
Masami Hiramatsu, Martynas Pumputis
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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next 2/8] ftrace: Add support to resolve module symbols in ftrace_lookup_symbols
2022-10-11 7:05 ` Song Liu
@ 2022-10-11 10:07 ` Jiri Olsa
0 siblings, 0 replies; 24+ messages in thread
From: Jiri Olsa @ 2022-10-11 10:07 UTC (permalink / raw)
To: Song Liu
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martynas Pumputis, bpf, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Christoph Hellwig, Masami Hiramatsu
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
> >
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next 4/8] bpf: Take module reference on kprobe_multi link
2022-10-11 7:16 ` Song Liu
@ 2022-10-11 10:09 ` Jiri Olsa
0 siblings, 0 replies; 24+ messages in thread
From: Jiri Olsa @ 2022-10-11 10:09 UTC (permalink / raw)
To: Song Liu
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Christoph Hellwig,
Masami Hiramatsu, Martynas Pumputis
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
> >
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next 7/8] selftests/bpf: Add kprobe_multi kmod link api tests
2022-10-11 7:27 ` Song Liu
@ 2022-10-11 10:09 ` Jiri Olsa
0 siblings, 0 replies; 24+ messages in thread
From: Jiri Olsa @ 2022-10-11 10:09 UTC (permalink / raw)
To: Song Liu
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Christoph Hellwig,
Masami Hiramatsu, Martynas Pumputis
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);
> > +}
> >
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next 4/8] bpf: Take module reference on kprobe_multi link
2022-10-09 21:59 ` [PATCH bpf-next 4/8] bpf: Take module reference on kprobe_multi link Jiri Olsa
2022-10-11 7:16 ` Song Liu
@ 2022-10-13 18:50 ` Andrii Nakryiko
2022-10-14 10:17 ` Jiri Olsa
1 sibling, 1 reply; 24+ messages in thread
From: Andrii Nakryiko @ 2022-10-13 18:50 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Christoph Hellwig,
Masami Hiramatsu, Martynas Pumputis
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;
> +}
> +
[...]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next 7/8] selftests/bpf: Add kprobe_multi kmod link api tests
2022-10-09 21:59 ` [PATCH bpf-next 7/8] selftests/bpf: Add kprobe_multi kmod link api tests Jiri Olsa
2022-10-11 7:27 ` Song Liu
@ 2022-10-13 19:06 ` Andrii Nakryiko
2022-10-14 10:25 ` Jiri Olsa
1 sibling, 1 reply; 24+ messages in thread
From: Andrii Nakryiko @ 2022-10-13 19:06 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Christoph Hellwig,
Masami Hiramatsu, Martynas Pumputis
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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next 4/8] bpf: Take module reference on kprobe_multi link
2022-10-13 18:50 ` Andrii Nakryiko
@ 2022-10-14 10:17 ` Jiri Olsa
0 siblings, 0 replies; 24+ messages in thread
From: Jiri Olsa @ 2022-10-14 10:17 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Christoph Hellwig,
Masami Hiramatsu, Martynas Pumputis
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next 7/8] selftests/bpf: Add kprobe_multi kmod link api tests
2022-10-13 19:06 ` Andrii Nakryiko
@ 2022-10-14 10:25 ` Jiri Olsa
0 siblings, 0 replies; 24+ messages in thread
From: Jiri Olsa @ 2022-10-14 10:25 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Christoph Hellwig,
Masami Hiramatsu, Martynas Pumputis
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
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2022-10-14 10:25 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-09 21:59 [PATCH bpf-next 0/8] bpf: Fixes for kprobe multi on kernel modules Jiri Olsa
2022-10-09 21:59 ` [PATCH bpf-next 1/8] kallsyms: Make module_kallsyms_on_each_symbol generally available Jiri Olsa
2022-10-11 6:56 ` Song Liu
2022-10-09 21:59 ` [PATCH bpf-next 2/8] ftrace: Add support to resolve module symbols in ftrace_lookup_symbols Jiri Olsa
2022-10-11 7:05 ` Song Liu
2022-10-11 10:07 ` Jiri Olsa
2022-10-09 21:59 ` [PATCH bpf-next 3/8] bpf: Rename __bpf_kprobe_multi_cookie_cmp to bpf_kprobe_multi_addrs_cmp Jiri Olsa
2022-10-11 7:06 ` Song Liu
2022-10-09 21:59 ` [PATCH bpf-next 4/8] bpf: Take module reference on kprobe_multi link Jiri Olsa
2022-10-11 7:16 ` Song Liu
2022-10-11 10:09 ` Jiri Olsa
2022-10-13 18:50 ` Andrii Nakryiko
2022-10-14 10:17 ` Jiri Olsa
2022-10-09 21:59 ` [PATCH bpf-next 5/8] selftests/bpf: Add load_kallsyms_refresh function Jiri Olsa
2022-10-11 7:17 ` Song Liu
2022-10-09 21:59 ` [PATCH bpf-next 6/8] selftests/bpf: Add bpf_testmod_fentry_* functions Jiri Olsa
2022-10-11 7:24 ` Song Liu
2022-10-09 21:59 ` [PATCH bpf-next 7/8] selftests/bpf: Add kprobe_multi kmod link api tests Jiri Olsa
2022-10-11 7:27 ` Song Liu
2022-10-11 10:09 ` Jiri Olsa
2022-10-13 19:06 ` Andrii Nakryiko
2022-10-14 10:25 ` Jiri Olsa
2022-10-09 21:59 ` [PATCH bpf-next 8/8] selftests/bpf: Add kprobe_multi check to module attach test Jiri Olsa
2022-10-11 7:27 ` Song Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).