Re: [PATCH bpf-next v6 5/5] selftests/bpf: Test mm_struct user memory kfuncs with linux_binprm

[bot+bpf-ci]

> diff --git a/tools/testing/selftests/bpf/prog_tests/copy_from_user_bprm.c b/tools/testing/selftests/bpf/prog_tests/copy_from_user_bprm.c
> new file mode 100644
> index 000000000000..b2325b193576
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/copy_from_user_bprm.c
> @@ -0,0 +1,72 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <errno.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +
> +#include <test_progs.h>
> +
> +#include "copy_from_user_bprm.skel.h"

[ ... ]

> +	if (!child) {
> +		char *const argv[] = { arg0, arg1, NULL };
> +		char *const envp[] = { env0, env1, NULL };
> +
> +		skel->bss->monitored_pid = getpid();
> +		execve("/bin/true", argv, envp);
> +		_exit(errno);
> +	}

This isn't a bug, but would execvp("true", ...) like test_lsm.c does be
more portable here than the hardcoded /bin/true? With the absolute path,
hosts where only /usr/bin/true exists would see execve() fail with ENOENT.

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/copy_from_user_bprm.c b/tools/testing/selftests/bpf/progs/copy_from_user_bprm.c
> new file mode 100644
> index 000000000000..b334a157419e
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/copy_from_user_bprm.c
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include "vmlinux.h"
> +
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +#include <errno.h>
> +#include "bpf_misc.h"

This isn't a bug, but is anything from bpf_misc.h actually needed here,
or could that include be dropped? The test-tag macros, __sink, the
__clobber_* and __imm_* helpers, SYS_PREFIX and CAP_* defines all appear
unreferenced; __ksym, __kconfig and __weak come from bpf/bpf_helpers.h,
u32/u64 and BPF_F_PAD_ZEROS from vmlinux.h, and BPF_PROG from
bpf/bpf_tracing.h.

[ ... ]

> +	data_len = bprm->exec - bprm->p;
> +	data_len_match = data_len == sizeof(expected_data);
> +
> +	invalid_flags_ret = bpf_copy_from_user_mm(data,
> +						  sizeof(data), (void *)bprm->p, mm, ~0ULL);
> +
> +	copy_ret = bpf_copy_from_user_mm(data, sizeof(data), (void *)bprm->p,
> +					 mm, 0);

This isn't a bug, but could the first call be wrapped the same way as
the copy_ret call just below, so both fit in 80 columns? The first
bpf_copy_from_user_mm() call breaks after the single data argument and
pushes the remaining four arguments onto a 92-column continuation line,
while the identical call three lines below is wrapped conventionally at
the last argument that fits.

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34245694915