Re: [PATCH bpf-next v4 2/2] selftests/bpf: Test faulting build IDs from a sleepable raw tracepoint

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/bpf: race setting target_tgid before target_syscall
- [Medium] selftests/bpf: unchecked signed-unsigned division in subtest_raw_tp()
--

commit 2b4f6f5d972293b188ccb421c050655343b0d0ee
Author: Feng Yang <yangfeng@kylinos.cn>

selftests/bpf: Test faulting build IDs from a sleepable raw tracepoint

This commit extends the build_id selftest with sleepable and non-sleepable
raw tracepoint subtests on sys_enter to verify user stack build ID extraction
when the build ID page is paged out.

> diff --git a/tools/testing/selftests/bpf/prog_tests/build_id.c b/tools/testing/selftests/bpf/prog_tests/build_id.c
> index aec9c8d6bc96b..98e3d09981793 100644
> --- a/tools/testing/selftests/bpf/prog_tests/build_id.c
> +++ b/tools/testing/selftests/bpf/prog_tests/build_id.c
[ ... ]
> +static int run_uprobe_multi_syscall(struct test_build_id *skel)
> +{
> +	char c = 'x';
> +	int pfd[2], status, err;
> +	pid_t pid;
> +
> +	if (pipe(pfd))
> +		return -errno;
> +
> +	pid = fork();
> +	if (pid < 0) {
> +		err = -errno;
> +		close(pfd[0]);
> +		close(pfd[1]);
> +		return err;
> +	}
> +	if (pid == 0) {
> +		close(pfd[1]);
> +		if (read(pfd[0], &c, 1) != 1)
> +			_exit(127);
> +		close(pfd[0]);
> +		execl("./uprobe_multi", "uprobe_multi", "paged-out-syscall", NULL);
> +		_exit(127);
> +	}
> +
> +	close(pfd[0]);
> +	skel->bss->target_tgid = pid;
> +	skel->bss->target_syscall = SYS_getpid;

[Severity: Medium]
Can setting target_tgid before target_syscall in run_uprobe_multi_syscall()
race with the child's read(pfd[0], &c, 1) on architectures where SYS_read is 0
(such as x86_64)?

In subtest_raw_tp(), skel->bss is zero-initialized (so target_syscall is 0)
and rawtp_sleepable or rawtp_nofault is already attached before calling
run_uprobe_multi_syscall():

subtest_raw_tp()
  bpf_program__attach()
  run_uprobe_multi_syscall()
    fork()

If the child enters sys_enter for read(pfd[0], &c, 1) after the parent stores
skel->bss->target_tgid = pid but before skel->bss->target_syscall = SYS_getpid
becomes visible, rawtp_nofault or rawtp_sleepable in
tools/testing/selftests/bpf/progs/test_build_id.c sees:

	if ((u32)(bpf_get_current_pid_tgid() >> 32) != (u32)target_tgid ||
	    id != target_syscall)
		return 0;

	/* Record only the first hit, a later getpid() must not overwrite it. */
	if (res_rawtp_sleepable)
		return 0;

Because id == 0 matches target_syscall == 0, the program records the stack of
the forked test_progs process before execl() and ignores the subsequent
SYS_getpid in uprobe_multi, causing the uprobe_multi build ID assertion to
fail. Should target_syscall be initialized before attaching or before setting
target_tgid?

> +	if (write(pfd[1], &c, 1) != 1) {
[ ... ]
> +static void subtest_raw_tp(bool sleepable)
> +{
[ ... ]
> +	if (sleepable) {
> +		res = skel->bss->res_rawtp_sleepable;
> +		stack = skel->bss->stack_rawtp_sleepable;
> +	} else {
> +		res = skel->bss->res_rawtp_nofault;
> +		stack = skel->bss->stack_rawtp_nofault;
> +	}
> +	frame_cnt = res / sizeof(struct bpf_stack_build_id);
> +	if (env.verbosity >= VERBOSE_NORMAL)
> +		print_stack(stack, frame_cnt);
> +
> +	ASSERT_GT(frame_cnt, 0, "frames");

[Severity: Medium]
If bpf_get_stack() returns a negative errno in res_rawtp_sleepable or
res_rawtp_nofault, does dividing the signed int res by the unsigned size_t
sizeof(struct bpf_stack_build_id) convert res to a large unsigned value before
checking if it succeeded?

On 32-bit architectures, where int and size_t are both 32-bit, a negative res
such as -EFAULT (-14) becomes 0xfffffff2U / 32 = 134217727 in frame_cnt, which
passes ASSERT_GT(frame_cnt, 0, "frames") and overflows the 128-element
stack_rawtp_sleepable or stack_rawtp_nofault array in print_stack(),
stack_has_build_id(), and stack_has_ip(). Even on 64-bit targets, a failed
ASSERT_GT(frame_cnt, 0, "frames") falls through to the subsequent stack checks
instead of jumping to cleanup.

Should subtest_raw_tp() check res before computing frame_cnt, matching
subtest_sleepable() in the same file:

	if (!ASSERT_GT(skel->bss->res_sleepable, 0, "res"))
		goto cleanup;

	stack = skel->bss->stack_sleepable;
	frame_cnt = skel->bss->res_sleepable / sizeof(struct bpf_stack_build_id);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105093212.826479-1-yangfeng59949@163.com?part=2