Re: [PATCH v3 7/7] selftests/bpf: add tests for the page_owner bpf_iter target

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/bpf: ASSERT_OK_PTR() on strstr() and strchr() passes on NULL
--

commit c911a68fd56ba0ae206e2cf17e238a8881f03bd2
Author: Zhen Ni <zhen.ni@easystack.cn>

selftests/bpf: add tests for the page_owner bpf_iter target

This commit adds a sleepable BPF program and a userspace test runner to
exercise the seq_file and open-coded page_owner BPF iterators and their
associated kfuncs. It also enables CONFIG_PAGE_OWNER in the BPF selftests
configuration.

> diff --git a/tools/testing/selftests/bpf/prog_tests/page_owner_iter.c b/tools/testing/selftests/bpf/prog_tests/page_owner_iter.c
> new file mode 100644
> index 0000000000000..3684ca3089355
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/page_owner_iter.c
[ ... ]
> +/* Validate bpf_page_owner_get_memcg_info() results from the open-coded run. */
> +static void check_memcg_skel(struct bpf_iter_page_owner *skel)
> +{
[ ... ]
> +	if (skel->bss->memcg_first_name[0])
> +		ASSERT_OK_PTR(strstr(skel->bss->memcg_first_path,
> +				    skel->bss->memcg_first_name),
> +			      "memcg_name_in_path");
> +}
> +
> +void serial_test_page_owner_iter(void)
> +{
[ ... ]
> +	ASSERT_GT(skel->bss->stack_bytes, 0, "stack_snprint_ok");
> +	ASSERT_OK_PTR(strstr(skel->bss->stack_text, "+0x"), "stack_frames");
> +	ASSERT_OK_PTR(strchr(skel->bss->stack_text, '\n'), "stack_lines");

[Severity: Medium]
Will these ASSERT_OK_PTR() checks in serial_test_page_owner_iter() and
check_memcg_skel() in tools/testing/selftests/bpf/prog_tests/page_owner_iter.c
actually catch a NULL return from strstr() or strchr()?

In tools/testing/selftests/bpf/test_progs.h, ASSERT_OK_PTR() checks the
pointer via libbpf_get_error():

    #define ASSERT_OK_PTR(ptr, name) ({                                 \
            static int duration = 0;                                    \
            const void *___res = (ptr);                                 \
            int ___err = libbpf_get_error(___res);                      \
            bool ___ok = ___err == 0;                                   \
            CHECK(!___ok, (name), "unexpected error: %d\n", ___err);    \
            ___ok;                                                      \
    })

And in tools/lib/bpf/libbpf.c, libbpf_get_error() returns -errno when
given NULL:

    long libbpf_get_error(const void *ptr)
    {
            if (!IS_ERR_OR_NULL(ptr))
                    return 0;

            if (IS_ERR(ptr))
                    errno = -PTR_ERR(ptr);

            return -errno;
    }

Because strstr() and strchr() do not set errno when returning NULL on a
non-match, if errno is 0 after bpf_prog_test_run_opts() succeeds,
libbpf_get_error(NULL) returns 0 and ASSERT_OK_PTR() silently passes even
when "+0x", '\n', or memcg_first_name is missing.

Should these checks use ASSERT_NEQ(..., NULL, ...) instead?

> +
> +	check_memcg_skel(skel);
> +
> +destroy:
> +	bpf_iter_page_owner__destroy(skel);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021112437.1799213-1-zhen.ni@easystack.cn?part=7