| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZNUnxJ26/4QfvoC+@google.com>
Date: Thu, 10 Aug 2023 11:09:08 -0700
From: Stanislav Fomichev <sdf@...gle.com>
To: Rong Tao <rtoax@...mail.com>
Cc: ast@...nel.org, rongtao@...tc.cn,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau <martin.lau@...ux.dev>,
Song Liu <song@...nel.org>,
Yonghong Song <yonghong.song@...ux.dev>,
John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...nel.org>, Hao Luo <haoluo@...gle.com>,
Jiri Olsa <jolsa@...nel.org>, Mykola Lysenko <mykolal@...com>,
Shuah Khan <shuah@...nel.org>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
Ilya Leoshkevich <iii@...ux.ibm.com>,
"Daniel Müller" <deso@...teo.net>,
"Steven Rostedt (Google)" <rostedt@...dmis.org>,
Ross Zwisler <zwisler@...gle.com>,
Manu Bretelle <chantr4@...il.com>,
Xu Kuohai <xukuohai@...wei.com>,
Eduard Zingerman <eddyz87@...il.com>,
"open list:BPF [GENERAL] (Safe Dynamic Programs and Tools)"
<bpf@...r.kernel.org>, open list <linux-kernel@...r.kernel.org>,
"open list:KERNEL SELFTEST FRAMEWORK"
<linux-kselftest@...r.kernel.org>,
"moderated list:ARM/STM32 ARCHITECTURE"
<linux-stm32@...md-mailman.stormreply.com>,
"moderated list:ARM/STM32 ARCHITECTURE"
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH bpf-next] selftests/bpf: trace_helpers.c: optimize
kallsyms cache
On 08/10, Rong Tao wrote:
> From: Rong Tao <rongtao@...tc.cn>
>
> Static ksyms often have problems because the number of symbols exceeds the
> MAX_SYMS limit. Like changing the MAX_SYMS from 300000 to 400000 in
> commit e76a014334a6("selftests/bpf: Bump and validate MAX_SYMS") solves
> the problem somewhat, but it's not the perfect way.
>
> This commit uses dynamic memory allocation, which completely solves the
> problem caused by the limitation of the number of kallsyms.
Thank you for doing this! I do remember complaining about it on the last
"let's bump the limit" patch :-D
> Signed-off-by: Rong Tao <rongtao@...tc.cn>
> ---
> samples/bpf/offwaketime_user.c | 1 +
> samples/bpf/sampleip_user.c | 1 +
> samples/bpf/spintest_user.c | 1 +
> samples/bpf/task_fd_query_user.c | 1 +
> samples/bpf/trace_event_user.c | 1 +
> .../selftests/bpf/prog_tests/bpf_cookie.c | 1 +
> .../bpf/prog_tests/get_stack_raw_tp.c | 1 +
> .../bpf/prog_tests/kprobe_multi_test.c | 2 +
> .../prog_tests/kprobe_multi_testmod_test.c | 2 +
> tools/testing/selftests/bpf/trace_helpers.c | 74 ++++++++++++-------
> tools/testing/selftests/bpf/trace_helpers.h | 1 +
> 11 files changed, 60 insertions(+), 26 deletions(-)
>
> diff --git a/samples/bpf/offwaketime_user.c b/samples/bpf/offwaketime_user.c
> index b6eedcb98fb9..5e6934f2d932 100644
> --- a/samples/bpf/offwaketime_user.c
> +++ b/samples/bpf/offwaketime_user.c
> @@ -149,5 +149,6 @@ int main(int argc, char **argv)
> bpf_link__destroy(links[i]);
>
> bpf_object__close(obj);
> + free_kallsyms();
Do we really need to free the symbols? IOW, is it enough to load them
once and keep them around until test_progs dies?
If we do, I wonder what would happen when test_progs runs in parallel mode?
Feels like if we're adding this alloc/free, then load_symbols
should return some object which free_kallsyms should free?
And ksym_get_addr would also work on that object, not on the global
state..
> return 0;
> }
> diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
> index 9283f47844fb..548ca1e9fcac 100644
> --- a/samples/bpf/sampleip_user.c
> +++ b/samples/bpf/sampleip_user.c
> @@ -230,5 +230,6 @@ int main(int argc, char **argv)
>
> free(links);
> bpf_object__close(obj);
> + free_kallsyms();
> return error;
> }
> diff --git a/samples/bpf/spintest_user.c b/samples/bpf/spintest_user.c
> index aadac14f748a..895a64afaf78 100644
> --- a/samples/bpf/spintest_user.c
> +++ b/samples/bpf/spintest_user.c
> @@ -88,5 +88,6 @@ int main(int ac, char **argv)
> bpf_link__destroy(links[j]);
>
> bpf_object__close(obj);
> + free_kallsyms();
> return 0;
> }
> diff --git a/samples/bpf/task_fd_query_user.c b/samples/bpf/task_fd_query_user.c
> index 1e61f2180470..3eb9477541fb 100644
> --- a/samples/bpf/task_fd_query_user.c
> +++ b/samples/bpf/task_fd_query_user.c
> @@ -419,5 +419,6 @@ int main(int argc, char **argv)
> bpf_link__destroy(links[i]);
>
> bpf_object__close(obj);
> + free_kallsyms();
> return err;
> }
> diff --git a/samples/bpf/trace_event_user.c b/samples/bpf/trace_event_user.c
> index 9664749bf618..a8b9343126fa 100644
> --- a/samples/bpf/trace_event_user.c
> +++ b/samples/bpf/trace_event_user.c
> @@ -348,5 +348,6 @@ int main(int argc, char **argv)
>
> cleanup:
> bpf_object__close(obj);
> + free_kallsyms();
> err_exit(error);
> }
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> index 26b2d1bffdfd..4786396e554b 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> @@ -170,6 +170,7 @@ static void kprobe_multi_link_api_subtest(void)
> cleanup:
> close(link1_fd);
> close(link2_fd);
> + free_kallsyms();
> kprobe_multi__destroy(skel);
> }
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
> index 858e0575f502..4e1c564746e1 100644
> --- a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
> @@ -146,4 +146,5 @@ void test_get_stack_raw_tp(void)
> bpf_link__destroy(link);
> perf_buffer__free(pb);
> bpf_object__close(obj);
> + free_kallsyms();
> }
> diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> index 2173c4bb555e..d6c0b5f2f887 100644
> --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> @@ -488,4 +488,6 @@ void test_kprobe_multi_test(void)
> test_attach_api_syms();
> if (test__start_subtest("attach_api_fails"))
> test_attach_api_fails();
> +
> + free_kallsyms();
> }
> 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
> index 1fbe7e4ac00a..c25f262832b7 100644
> --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c
> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c
> @@ -86,4 +86,6 @@ void serial_test_kprobe_multi_testmod_test(void)
> test_testmod_attach_api_syms();
> if (test__start_subtest("testmod_attach_api_addrs"))
> test_testmod_attach_api_addrs();
> +
> + free_kallsyms();
> }
> diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
> index f83d9f65c65b..08986f70836b 100644
> --- a/tools/testing/selftests/bpf/trace_helpers.c
> +++ b/tools/testing/selftests/bpf/trace_helpers.c
> @@ -18,9 +18,32 @@
> #define TRACEFS_PIPE "/sys/kernel/tracing/trace_pipe"
> #define DEBUGFS_PIPE "/sys/kernel/debug/tracing/trace_pipe"
>
> -#define MAX_SYMS 400000
> -static struct ksym syms[MAX_SYMS];
> -static int sym_cnt;
> +static struct {
> + struct ksym *syms;
> + unsigned int sym_cnt;
> +} ksyms = {
> + .syms = NULL,
> + .sym_cnt = 0,
> +};
> +
> +static int ksyms__add_symbol(const char *name, unsigned long addr)
> +{
> + void *tmp;
> + unsigned int cnt = ksyms.sym_cnt;
> +
> + cnt++;
> + tmp = realloc(ksyms.syms, sizeof(struct ksym) * cnt);
> + if (!tmp)
> + return -ENOMEM;
Should we do the usual len/capacity scheme here to amortize the cost
of realloc (like doubling capacity when we reach it)?
Calling realloc on every symbol doesn't seem right.
Powered by blists - more mailing lists