On Mon, Sep 04, 2023 at 09:01:20AM +0800, Rong Tao wrote:
> From: Rong Tao <rongtao@xxxxxxxx>
>
> 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. At the same
> time, add APIs:
>
> load_kallsyms_local()
> ksym_search_local()
> ksym_get_addr_local()
missing free_kallsyms_local
> diff --git a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> index 9d768e083714..13e618317c8b 100644
> --- a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> @@ -302,16 +302,18 @@ void test_fill_link_info(void)
> {
> struct test_fill_link_info *skel;
> int i;
> + struct ksyms *ksyms;
>
> skel = test_fill_link_info__open_and_load();
> if (!ASSERT_OK_PTR(skel, "skel_open"))
> return;
>
> /* load kallsyms to compare the addr */
> - if (!ASSERT_OK(load_kallsyms_refresh(), "load_kallsyms_refresh"))
> + ksyms = load_kallsyms_local();
> + if (!ASSERT_OK_PTR(ksyms, "load_kallsyms_local"))
> goto cleanup;
actually I don't see why this test should need refreshed kallsyms,
it doesn't load/unload testmod, I think it can use global ksyms
>
> - kprobe_addr = ksym_get_addr(KPROBE_FUNC);
> + kprobe_addr = ksym_get_addr_local(ksyms, KPROBE_FUNC);
> if (test__start_subtest("kprobe_link_info"))
> test_kprobe_fill_link_info(skel, BPF_PERF_EVENT_KPROBE, false);
> if (test__start_subtest("kretprobe_link_info"))
> @@ -329,7 +331,7 @@ void test_fill_link_info(void)
>
> qsort(kmulti_syms, KMULTI_CNT, sizeof(kmulti_syms[0]), symbols_cmp_r);
> for (i = 0; i < KMULTI_CNT; i++)
> - kmulti_addrs[i] = ksym_get_addr(kmulti_syms[i]);
> + kmulti_addrs[i] = ksym_get_addr_local(ksyms, kmulti_syms[i]);
> if (test__start_subtest("kprobe_multi_link_info"))
> test_kprobe_multi_fill_link_info(skel, false, false);
> if (test__start_subtest("kretprobe_multi_link_info"))
> @@ -339,4 +341,5 @@ void test_fill_link_info(void)
>
> cleanup:
> test_fill_link_info__destroy(skel);
> + free_kallsyms_local(ksyms);
> }
> 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..532b05ae2da4 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
> @@ -4,6 +4,8 @@
> #include "trace_helpers.h"
> #include "bpf/libbpf_internal.h"
>
> +static struct ksyms *ksyms;
> +
> static void kprobe_multi_testmod_check(struct kprobe_multi *skel)
> {
> ASSERT_EQ(skel->bss->kprobe_testmod_test1_result, 1, "kprobe_test1_result");
> @@ -50,12 +52,12 @@ static void test_testmod_attach_api_addrs(void)
> LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
> unsigned long long addrs[3];
>
> - addrs[0] = ksym_get_addr("bpf_testmod_fentry_test1");
> - ASSERT_NEQ(addrs[0], 0, "ksym_get_addr");
> - addrs[1] = ksym_get_addr("bpf_testmod_fentry_test2");
> - ASSERT_NEQ(addrs[1], 0, "ksym_get_addr");
> - addrs[2] = ksym_get_addr("bpf_testmod_fentry_test3");
> - ASSERT_NEQ(addrs[2], 0, "ksym_get_addr");
> + addrs[0] = ksym_get_addr_local(ksyms, "bpf_testmod_fentry_test1");
> + ASSERT_NEQ(addrs[0], 0, "ksym_get_addr_local");
> + addrs[1] = ksym_get_addr_local(ksyms, "bpf_testmod_fentry_test2");
> + ASSERT_NEQ(addrs[1], 0, "ksym_get_addr_local");
> + addrs[2] = ksym_get_addr_local(ksyms, "bpf_testmod_fentry_test3");
> + ASSERT_NEQ(addrs[2], 0, "ksym_get_addr_local");
>
> opts.addrs = (const unsigned long *) addrs;
> opts.cnt = ARRAY_SIZE(addrs);
> @@ -79,11 +81,19 @@ static void test_testmod_attach_api_syms(void)
>
> void serial_test_kprobe_multi_testmod_test(void)
> {
> - if (!ASSERT_OK(load_kallsyms_refresh(), "load_kallsyms_refresh"))
> + ksyms = load_kallsyms_local();
> + if (!ASSERT_OK_PTR(ksyms, "load_kallsyms_local"))
> return;
>
> if (test__start_subtest("testmod_attach_api_syms"))
> test_testmod_attach_api_syms();
> +
> + ksyms = load_kallsyms_refresh(ksyms);
> + if (!ASSERT_OK_PTR(ksyms, "load_kallsyms_refresh"))
> + return;
hm, this refresh is not needed right? the test got the fresh kallsyms above
and both test_testmod_attach_api_syms and test_testmod_attach_api_addrs
should be happy
> +
> if (test__start_subtest("testmod_attach_api_addrs"))
> test_testmod_attach_api_addrs();
> +
> + free_kallsyms_local(ksyms);
> }
> diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
> index f83d9f65c65b..d64c4ef336e1 100644
> --- a/tools/testing/selftests/bpf/trace_helpers.c
> +++ b/tools/testing/selftests/bpf/trace_helpers.c
> @@ -14,104 +14,171 @@
> #include <linux/limits.h>
> #include <libelf.h>
> #include <gelf.h>
> +#include "bpf/libbpf_internal.h"
>
> #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;
> +struct ksyms {
> + struct ksym *syms;
> + size_t sym_cap;
> + size_t sym_cnt;
> +};
> +
> +static struct ksyms *ksyms;
> +
> +static int ksyms__add_symbol(struct ksyms *ksyms, const char *name,
> + unsigned long addr)
extra white space in here ^^^
> +{
> + void *tmp;
> +
> + tmp = strdup(name);
> + if (!tmp)
> + return -ENOMEM;
> + ksyms->syms[ksyms->sym_cnt].addr = addr;
> + ksyms->syms[ksyms->sym_cnt].name = tmp;
> +
> + ksyms->sym_cnt++;
> +
> + return 0;
> +}
> +
> +void free_kallsyms_local(struct ksyms *ksyms)
> +{
> + unsigned int i;
> +
> + if (!ksyms)
> + return;
> +
> + if (!ksyms->syms) {
> + free(ksyms);
> + return;
> + }
> +
> + for (i = 0; i < ksyms->sym_cnt; i++)
> + free(ksyms->syms[i].name);
> + free(ksyms->syms);
> + free(ksyms);
> +}
>
> static int ksym_cmp(const void *p1, const void *p2)
> {
> return ((struct ksym *)p1)->addr - ((struct ksym *)p2)->addr;
> }
>
> -int load_kallsyms_refresh(void)
> +struct ksyms *load_kallsyms_refresh(struct ksyms *ksyms)
> {
> FILE *f;
> char func[256], buf[256];
> char symbol;
> void *addr;
> - int i = 0;
> + int ret;
>
> - sym_cnt = 0;
> + /* flush kallsyms, free the previously allocated dynamic memory */
> + free_kallsyms_local(ksyms);
>
> f = fopen("/proc/kallsyms", "r");
> if (!f)
> - return -ENOENT;
> + return NULL;
> +
> + ksyms = calloc(1, sizeof(struct ksyms));
> + if (!ksyms)
> + return NULL;
I wonder it could be easier not to allocate ksyms, but just let the
caller to pass the pointer to 'ksym' variable on stack.. but there
might be other problems, so I guess this is fine
>
> while (fgets(buf, sizeof(buf), f)) {
> if (sscanf(buf, "%p %c %s", &addr, &symbol, func) != 3)
> break;
> if (!addr)
> continue;
> - if (i >= MAX_SYMS)
> - return -EFBIG;
>
> - syms[i].addr = (long) addr;
> - syms[i].name = strdup(func);
> - i++;
> + ret = libbpf_ensure_mem((void **) &ksyms->syms, &ksyms->sym_cap,
> + sizeof(struct ksym), ksyms->sym_cnt + 1);
> + if (ret)
> + goto error;
> + ret = ksyms__add_symbol(ksyms, func, (unsigned long)addr);
> + if (ret)
> + goto error;
> }
> fclose(f);
> - sym_cnt = i;
> - qsort(syms, sym_cnt, sizeof(struct ksym), ksym_cmp);
> - return 0;
> + qsort(ksyms->syms, ksyms->sym_cnt, sizeof(struct ksym), ksym_cmp);
> + return ksyms;
> +
> +error:
> + free_kallsyms_local(ksyms);
> + return NULL;
> +}
> +
> +struct ksyms *load_kallsyms_local(void)
> +{
> + return load_kallsyms_refresh(NULL);
I think we can have just:
struct ksyms *load_kallsyms_local(struct ksyms *ksyms);
I don't see reason for load_kallsyms_refresh functions, perhaps it's
leftover because I don't see the function in the changelog
thanks,
jirka
