Re: [PATCH bpf-next v2 6/6] selftests/proc: Add KALLSYMS_QUERY tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/proc: main() bypasses kselftest reporting and ksft_exit_*() wrappers
- [Medium] selftests/proc: main() skips silently with bare KSFT_SKIP and no reason message
--

commit 309c42ade27ce014cdfb65d22ab97d40bf7dcd32
Author: Samuel Wu <wusamuel@google.com>

selftests/proc: Add KALLSYMS_QUERY tests

This commit adds a selftest in tools/testing/selftests/proc/proc-kallsyms-query.c
to exercise symbol-to-address and address-to-symbol lookups via the
kallsyms_query ioctl on /proc/kallsyms.

> diff --git a/tools/testing/selftests/proc/proc-kallsyms-query.c b/tools/testing/selftests/proc/proc-kallsyms-query.c
> new file mode 100644
> index 0000000000000..229cde432d6aa
> --- /dev/null
> +++ b/tools/testing/selftests/proc/proc-kallsyms-query.c

[ ... ]

> +int main(void)
> +{
> +	const char *sym_name = "vfs_read";
> +	char resolved_name[KSYM_NAME_LEN];
> +	char long_name[KSYM_NAME_LEN + 1];
> +	size_t page_size = sysconf(_SC_PAGESIZE);
> +	unsigned long long exp_addr;
> +	struct kallsyms_query_ext q1;
> +	struct kallsyms_query q;
> +	char *page_buf;
> +	int err;
> +	int fd;
> +
> +	fd = open("/proc/kallsyms", O_RDONLY | O_CLOEXEC);
> +	if (fd < 0) {
> +		if (errno == ENOENT)
> +			return KSFT_SKIP;
> +		ksft_exit_fail_msg("open(/proc/kallsyms) failed: %s\n",
> +				   strerror(errno));
> +	}
> +
> +	if (!find_vmlinux_symbol(sym_name, &exp_addr)) {
> +		close(fd);
> +		return KSFT_SKIP;
> +	}
> +
> +	/* CASE 1: Valid vmlinux symbol query (or -EPERM when addresses are hidden) */
> +	err = query_symbol(fd, sym_name, &q);
> +	if (err == -ENOTTY) {
> +		close(fd);
> +		return KSFT_SKIP;
> +	}

[Severity: Medium]
In main() in tools/testing/selftests/proc/proc-kallsyms-query.c, when
opening /proc/kallsyms fails because the file does not exist, when
find_vmlinux_symbol() does not find vfs_read, or when query_symbol() fails
because the ioctl is not supported by the running kernel, the function
returns directly without printing a skip message.

Unlike the hidden-address check right below that calls ksft_exit_skip() with
a reason string, returning silently here leaves test logs with no indication
of which prerequisite was missing.

Should these three skip paths call ksft_exit_skip() with a descriptive
reason message instead?

> +	if (!exp_addr) {
> +		assert(err == -EPERM);
> +		close(fd);
> +		ksft_exit_skip("kallsyms addresses are hidden, only -EPERM checked\n");
> +	}
> +	assert(err == 0);
> +	assert(q.sym_addr == exp_addr);

[ ... ]

> +	/* CASE 16: KALLSYMS_QUERY_BY_ADDR invalid inputs return -EINVAL or -EFAULT */
> +	assert(query_addr(fd, exp_addr, resolved_name, 0, &q) == -EINVAL);
> +	assert(query_addr(fd, exp_addr, NULL, sizeof(resolved_name), &q) == -EFAULT);
> +
> +	close(fd);
> +	return 0;
> +}

[Severity: Medium]
Although tools/testing/selftests/proc/proc-kallsyms-query.c includes
kselftest.h and uses ksft_exit_fail_msg() and ksft_exit_skip() in main(),
it never calls ksft_print_header() or ksft_set_plan(), evaluates all 16 test
cases with libc assert(), and exits main() with a bare return 0.

When run under kselftest runners or automated test parsers, this produces no
plan, per-test result lines, or summary output on completion, and aborts the
process via assert() on failure instead of reporting structured test failure
results.

Could main() use the kselftest.h reporting helpers (such as
ksft_print_header(), ksft_set_plan(), ksft_test_result(), and
ksft_exit_pass() or ksft_finished()) or the kselftest_harness.h macros
instead of raw assert() and return 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023001603.560210-8-wusamuel@google.com?part=6