Re: [PATCHv3 bpf-next 5/8] selftests/bpf: Test user memory mmap kfuncs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/bpf: call to undefined get_procmap_info() breaks build
- [Medium] selftests/bpf: user_mem_test() calls non-existent user mmap kfuncs
--

commit 602c938f4e2233efed73c162a7e2295814283b95
Author: Jiri Olsa <jolsa@kernel.org>

selftests/bpf: Test user memory mmap kfuncs

This commit adds BPF selftests for bpf_user_anon_mmap() and bpf_user_munmap()
invoked from a sleepable uprobe, checking that created mappings have the
expected permissions and that unmapped addresses are removed.

> diff --git a/tools/testing/selftests/bpf/prog_tests/user_mem.c b/tools/testing/selftests/bpf/prog_tests/user_mem.c
> new file mode 100644
> index 0000000000000..eadf668f7056b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/user_mem.c
> @@ -0,0 +1,111 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +#include <sys/mman.h>
> +#include <unistd.h>
> +
> +#include "user_mem.skel.h"
> +#include "trace_helpers.h"

[ ... ]

> +static bool check_user_mem_vma(void *addr, unsigned long size, const char *perms)
> +{
> +	size_t start, end;
> +	char vma_perms[5];
> +	int err;
> +
> +	err = get_procmap_info(addr, &start, &end, vma_perms);
> +	if (!ASSERT_OK(err, "user_mem_vma"))
> +		return false;
> +
> +	/* VMA can be merged, so ensure it covers at least the requested range. */
> +	if (!ASSERT_GE(end - (size_t)addr, size, "vma_size"))
> +		return false;
> +
> +	return ASSERT_STREQ(vma_perms, perms, "vma_perms");
> +}
> +
> +static bool check_user_mem_unmapped(void *addr)
> +{
> +	size_t start, end;
> +	int err;
> +
> +	err = get_procmap_info(addr, &start, &end, NULL);
> +	return ASSERT_EQ(err, -ESRCH, "user_mem_unmapped");
> +}

[Severity: Medium]
Was a prerequisite patch adding get_procmap_info() omitted from this series?

Both check_user_mem_vma() and check_user_mem_unmapped() call
get_procmap_info(), and tools/testing/selftests/bpf/prog_tests/user_mem.c
includes trace_helpers.h, but neither tools/testing/selftests/bpf/trace_helpers.h
nor tools/testing/selftests/bpf/trace_helpers.c declares or defines
get_procmap_info().

Because the BPF selftests are compiled with -Werror, does this cause building
test_progs to fail with an implicit function declaration error and an
undefined reference at link time?

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/user_mem.c b/tools/testing/selftests/bpf/progs/user_mem.c
> new file mode 100644
> index 0000000000000..e3b0689f5f2a5
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/user_mem.c
> @@ -0,0 +1,43 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <vmlinux.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +#include "bpf_misc.h"
> +
> +char _license[] SEC("license") = "GPL";
> +
> +extern unsigned long bpf_user_anon_mmap(unsigned long len, unsigned long prot) __ksym;
> +extern int bpf_user_munmap(unsigned long addr, unsigned long len) __ksym;

[ ... ]

> +SEC("uprobe.s//proc/self/exe:user_mem_trigger")
> +int user_mem_test(struct pt_regs *ctx)
> +{
> +	if ((bpf_get_current_pid_tgid() >> 32) != monitored_pid)
> +		return 0;
> +
> +	switch (action) {
> +	case USER_MEM_MMAP:
> +		user_addr = bpf_user_anon_mmap(map_len, map_prot);
> +		break;
> +	case USER_MEM_UNMAP:
> +		ret = bpf_user_munmap(user_addr, map_len);
> +		break;
> +	}

[Severity: Medium]
Are the kernel definitions for bpf_user_anon_mmap() and bpf_user_munmap()
missing from the tree?

In tools/testing/selftests/bpf/progs/user_mem.c, user_mem_test() invokes
bpf_user_anon_mmap() and bpf_user_munmap() declared as strong __ksym externs,
and test_user_mem() in tools/testing/selftests/bpf/prog_tests/user_mem.c
loads the program via user_mem__open_and_load():

    skel = user_mem__open_and_load();
    if (!ASSERT_OK_PTR(skel, "open_and_load"))
        return;

Since neither bpf_user_anon_mmap nor bpf_user_munmap is defined or registered
in any BTF kfunc set in the kernel or selftest kernel modules, won't
user_mem__open_and_load() fail unconditionally when libbpf tries to resolve
their BTF IDs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105102343.311331-6-jolsa@kernel.org?part=1