On Tue, Dec 02, 2025 at 05:30:15PM +0800, Shuran Liu wrote: > Hi Matt, > > Thanks a lot for the review and for re-sending your Reviewed-by tag. > > In the next version of the series I’ll add your > > Reviewed-by: Matt Bobrowski <[email protected]> > > to the patch that introduces the new selftest, and I’ll also make sure to > remove /tmp/bpf_d_path_test in the test cleanup path as you suggested.
SGTM. > I also noticed that the CI is currently failing due to the `#pragma unroll` > around the loop in prog_lsm(). Would you prefer that I simply drop the pragma > in the next version, given that the loop bound is small and constant anyway, > or is there a better way you’d recommend to handle this? Yeah, I don't think the use of this directive is required here given the iteration count is tiny. Alternatively, perhaps you could switch over to using a more BPF verifier preferred alternative (i.e. bpf_for() or better yet and simpler bpf_repeat())? > > On Tue, Dec 02, 2025 at 00:59:45AM -0800, Matt Bobrowski wrote： > > > > On Tue, Dec 02, 2025 at 03:54:41PM +0800, Shuran Liu wrote: > >> Add a regression test for bpf_d_path() when invoked from an LSM program. > >> The test attaches to the bprm_check_security hook, calls bpf_d_path() on > >> the binary being executed, and verifies that a simple prefix comparison on > >> the returned pathname behaves correctly after the fix in patch 1. > >> > >> To avoid nondeterminism, the LSM program now filters based on the > >> expected PID, which is populated from userspace before the test binary is > >> executed. This prevents unrelated processes that also trigger the > >> bprm_check_security LSM hook from overwriting test results. Parent and > >> child processes are synchronized through a pipe to ensure the PID is set > >> before the child execs the test binary. > >> > >> Per review feedback, the new test is merged into the existing d_path > >> selftest rather than adding new prog_tests/ or progs/ files. > >> > >> Co-developed-by: Zesen Liu <[email protected]> > >> Signed-off-by: Zesen Liu <[email protected]> > >> Co-developed-by: Peili Gao <[email protected]> > >> Signed-off-by: Peili Gao <[email protected]> > >> Co-developed-by: Haoran Ni <[email protected]> > >> Signed-off-by: Haoran Ni <[email protected]> > >> Signed-off-by: Shuran Liu <[email protected]> > > > > Feel free to add: > > > > Reviewed-by: Matt Bobrowski <[email protected] > > <mailto:[email protected]>> > > > >> --- > >> .../testing/selftests/bpf/prog_tests/d_path.c | 64 +++++++++++++++++++ > >> .../testing/selftests/bpf/progs/test_d_path.c | 33 ++++++++++ > >> 2 files changed, 97 insertions(+) > >> > >> diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c > >> b/tools/testing/selftests/bpf/prog_tests/d_path.c > >> index ccc768592e66..2909ca3bae0f 100644 > >> --- a/tools/testing/selftests/bpf/prog_tests/d_path.c > >> +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c > >> @@ -195,6 +195,67 @@ static void test_d_path_check_types(void) > >> test_d_path_check_types__destroy(skel); > >> } > >> > >> +static void test_d_path_lsm(void) > >> +{ > >> + struct test_d_path *skel; > >> + int err; > >> + int pipefd[2]; > >> + pid_t pid; > >> + > >> + skel = test_d_path__open_and_load(); > >> + if (!ASSERT_OK_PTR(skel, "d_path skeleton failed")) > >> + return; > >> + > >> + err = test_d_path__attach(skel); > >> + if (!ASSERT_OK(err, "attach failed")) > >> + goto cleanup; > >> + > >> + /* Prepare the test binary */ > >> + system("cp /bin/true /tmp/bpf_d_path_test 2>/dev/null || :"); > > > > I'd much prefer if we also cleaned up after ourselves, but it's not > > that much of an issue I guess. > > > >> + if (!ASSERT_OK(pipe(pipefd), "pipe failed")) > >> + goto cleanup; > >> + > >> + pid = fork(); > >> + if (!ASSERT_GE(pid, 0, "fork failed")) { > >> + close(pipefd[0]); > >> + close(pipefd[1]); > >> + goto cleanup; > >> + } > >> + > >> + if (pid == 0) { > >> + /* Child */ > >> + char buf; > >> + > >> + close(pipefd[1]); > >> + /* Wait for parent to set PID in BPF map */ > >> + if (read(pipefd[0], &buf, 1) != 1) > >> + exit(1); > >> + close(pipefd[0]); > >> + execl("/tmp/bpf_d_path_test", "/tmp/bpf_d_path_test", NULL); > >> + exit(1); > >> + } > >> + > >> + /* Parent */ > >> + close(pipefd[0]); > >> + > >> + /* Update BPF map with child PID */ > >> + skel->bss->my_pid = pid; > >> + > >> + /* Signal child to proceed */ > >> + write(pipefd[1], "G", 1); > >> + close(pipefd[1]); > >> + > >> + /* Wait for child */ > >> + waitpid(pid, NULL, 0); > >> + > >> + ASSERT_EQ(skel->bss->called_lsm, 1, "lsm hook called"); > >> + ASSERT_EQ(skel->bss->lsm_match, 1, "lsm match"); > >> + > >> +cleanup: > >> + test_d_path__destroy(skel); > >> +} > >> + > >> void test_d_path(void) > >> { > >> if (test__start_subtest("basic")) > >> @@ -205,4 +266,7 @@ void test_d_path(void) > >> > >> if (test__start_subtest("check_alloc_mem")) > >> test_d_path_check_types(); > >> + > >> + if (test__start_subtest("lsm")) > >> + test_d_path_lsm(); > >> } > >> diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c > >> b/tools/testing/selftests/bpf/progs/test_d_path.c > >> index 84e1f883f97b..7f65c282069a 100644 > >> --- a/tools/testing/selftests/bpf/progs/test_d_path.c > >> +++ b/tools/testing/selftests/bpf/progs/test_d_path.c > >> @@ -17,6 +17,8 @@ int rets_close[MAX_FILES] = {}; > >> > >> int called_stat = 0; > >> int called_close = 0; > >> +int called_lsm = 0; > >> +int lsm_match = 0; > >> > >> SEC("fentry/security_inode_getattr") > >> int BPF_PROG(prog_stat, struct path *path, struct kstat *stat, > >> @@ -62,4 +64,35 @@ int BPF_PROG(prog_close, struct file *file, void *id) > >> return 0; > >> } > >> > >> +SEC("lsm/bprm_check_security") > >> +int BPF_PROG(prog_lsm, struct linux_binprm *bprm) > >> +{ > >> + pid_t pid = bpf_get_current_pid_tgid() >> 32; > >> + char path[MAX_PATH_LEN] = {}; > >> + int ret; > >> + > >> + if (pid != my_pid) > >> + return 0; > >> + > >> + called_lsm = 1; > >> + ret = bpf_d_path(&bprm->file->f_path, path, MAX_PATH_LEN); > >> + if (ret < 0) > >> + return 0; > >> + > >> + { > >> + static const char target_dir[] = "/tmp/"; > >> + > >> +#pragma unroll > >> + for (int i = 0; i < sizeof(target_dir) - 1; i++) { > >> + if (path[i] != target_dir[i]) { > >> + lsm_match = -1; /* mismatch */ > >> + return 0; > >> + } > >> + } > >> + } > >> + > >> + lsm_match = 1; /* prefix match */ > >> + return 0; > >> +} > >> + > >> char _license[] SEC("license") = "GPL"; > >> -- > >> 2.52.0 >
