* [PATCH bpf v4 1/2] bpf: mark bpf_d_path() buffer as writeable
2025-12-04 7:46 [PATCH bpf v4 0/2] bpf: fix bpf_d_path() helper prototype Shuran Liu
@ 2025-12-04 7:46 ` Shuran Liu
2025-12-04 7:46 ` [PATCH bpf v4 2/2] selftests/bpf: add regression test for bpf_d_path() Shuran Liu
1 sibling, 0 replies; 6+ messages in thread
From: Shuran Liu @ 2025-12-04 7:46 UTC (permalink / raw)
To: song, mattbobrowski, bpf
Cc: ast, daniel, andrii, martin.lau, eddyz87, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, rostedt, mhiramat,
mathieu.desnoyers, linux-kernel, linux-trace-kernel, dxu,
linux-kselftest, shuah, electronlsr, Zesen Liu, Peili Gao,
Haoran Ni
Commit 37cce22dbd51 ("bpf: verifier: Refactor helper access type
tracking") started distinguishing read vs write accesses performed by
helpers.
The second argument of bpf_d_path() is a pointer to a buffer that the
helper fills with the resulting path. However, its prototype currently
uses ARG_PTR_TO_MEM without MEM_WRITE.
Before 37cce22dbd51, helper accesses were conservatively treated as
potential writes, so this mismatch did not cause issues. Since that
commit, the verifier may incorrectly assume that the buffer contents
are unchanged across the helper call and base its optimizations on this
wrong assumption. This can lead to misbehaviour in BPF programs that
read back the buffer, such as prefix comparisons on the returned path.
Fix this by marking the second argument of bpf_d_path() as
ARG_PTR_TO_MEM | MEM_WRITE so that the verifier correctly models the
write to the caller-provided buffer.
Fixes: 37cce22dbd51 ("bpf: verifier: Refactor helper access type tracking")
Co-developed-by: Zesen Liu <ftyg@live.com>
Signed-off-by: Zesen Liu <ftyg@live.com>
Co-developed-by: Peili Gao <gplhust955@gmail.com>
Signed-off-by: Peili Gao <gplhust955@gmail.com>
Co-developed-by: Haoran Ni <haoran.ni.cs@gmail.com>
Signed-off-by: Haoran Ni <haoran.ni.cs@gmail.com>
Signed-off-by: Shuran Liu <electronlsr@gmail.com>
Reviewed-by: Matt Bobrowski <mattbobrowski@google.com>
---
kernel/trace/bpf_trace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 4f87c16d915a..49e0bdaa7a1b 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -965,7 +965,7 @@ static const struct bpf_func_proto bpf_d_path_proto = {
.ret_type = RET_INTEGER,
.arg1_type = ARG_PTR_TO_BTF_ID,
.arg1_btf_id = &bpf_d_path_btf_ids[0],
- .arg2_type = ARG_PTR_TO_MEM,
+ .arg2_type = ARG_PTR_TO_MEM | MEM_WRITE,
.arg3_type = ARG_CONST_SIZE_OR_ZERO,
.allowed = bpf_d_path_allowed,
};
--
2.52.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH bpf v4 2/2] selftests/bpf: add regression test for bpf_d_path()
2025-12-04 7:46 [PATCH bpf v4 0/2] bpf: fix bpf_d_path() helper prototype Shuran Liu
2025-12-04 7:46 ` [PATCH bpf v4 1/2] bpf: mark bpf_d_path() buffer as writeable Shuran Liu
@ 2025-12-04 7:46 ` Shuran Liu
2025-12-04 12:38 ` Shuran Liu
1 sibling, 1 reply; 6+ messages in thread
From: Shuran Liu @ 2025-12-04 7:46 UTC (permalink / raw)
To: song, mattbobrowski, bpf
Cc: ast, daniel, andrii, martin.lau, eddyz87, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, rostedt, mhiramat,
mathieu.desnoyers, linux-kernel, linux-trace-kernel, dxu,
linux-kselftest, shuah, electronlsr, Zesen Liu, Peili Gao,
Haoran Ni
Add a regression test for bpf_d_path() to cover incorrect verifier
assumptions caused by an incorrect function prototype. The test
attaches to the fallocate hook, calls bpf_d_path() and verifies that
a simple prefix comparison on the returned pathname behaves correctly
after the fix in patch 1. It ensures the verifier does not assume
the buffer remains unwritten.
Co-developed-by: Zesen Liu <ftyg@live.com>
Signed-off-by: Zesen Liu <ftyg@live.com>
Co-developed-by: Peili Gao <gplhust955@gmail.com>
Signed-off-by: Peili Gao <gplhust955@gmail.com>
Co-developed-by: Haoran Ni <haoran.ni.cs@gmail.com>
Signed-off-by: Haoran Ni <haoran.ni.cs@gmail.com>
Signed-off-by: Shuran Liu <electronlsr@gmail.com>
---
.../testing/selftests/bpf/prog_tests/d_path.c | 90 +++++++++++++++----
.../testing/selftests/bpf/progs/test_d_path.c | 23 +++++
2 files changed, 95 insertions(+), 18 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
index ccc768592e66..c725d5258e65 100644
--- a/tools/testing/selftests/bpf/prog_tests/d_path.c
+++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
@@ -38,6 +38,14 @@ static int set_pathname(int fd, pid_t pid)
return readlink(buf, src.paths[src.cnt++], MAX_PATH_LEN);
}
+static inline long syscall_close(int fd)
+{
+ return syscall(__NR_close_range,
+ (unsigned int)fd,
+ (unsigned int)fd,
+ 0u);
+}
+
static int trigger_fstat_events(pid_t pid)
{
int sockfd = -1, procfd = -1, devfd = -1;
@@ -104,36 +112,47 @@ static int trigger_fstat_events(pid_t pid)
/* sys_close no longer triggers filp_close, but we can
* call sys_close_range instead which still does
*/
-#define close(fd) syscall(__NR_close_range, fd, fd, 0)
-
- close(pipefd[0]);
- close(pipefd[1]);
- close(sockfd);
- close(procfd);
- close(devfd);
- close(localfd);
- close(indicatorfd);
-
-#undef close
+ syscall_close(pipefd[0]);
+ syscall_close(pipefd[1]);
+ syscall_close(sockfd);
+ syscall_close(procfd);
+ syscall_close(devfd);
+ syscall_close(localfd);
+ syscall_close(indicatorfd);
return ret;
}
+static void attach_and_load(struct test_d_path **skel)
+{
+ int err;
+
+ *skel = test_d_path__open_and_load();
+ if (CHECK(!*skel, "setup", "d_path skeleton failed\n"))
+ goto cleanup;
+
+ err = test_d_path__attach(*skel);
+ if (CHECK(err, "setup", "attach failed: %d\n", err))
+ goto cleanup;
+
+ (*skel)->bss->my_pid = getpid();
+ return;
+
+cleanup:
+ test_d_path__destroy(*skel);
+ *skel = NULL;
+}
+
static void test_d_path_basic(void)
{
struct test_d_path__bss *bss;
struct test_d_path *skel;
int err;
- skel = test_d_path__open_and_load();
- if (CHECK(!skel, "setup", "d_path skeleton failed\n"))
- goto cleanup;
-
- err = test_d_path__attach(skel);
- if (CHECK(err, "setup", "attach failed: %d\n", err))
+ attach_and_load(&skel);
+ if (!skel)
goto cleanup;
bss = skel->bss;
- bss->my_pid = getpid();
err = trigger_fstat_events(bss->my_pid);
if (err < 0)
@@ -195,6 +214,38 @@ static void test_d_path_check_types(void)
test_d_path_check_types__destroy(skel);
}
+/* Check if the verifier correctly generates code for
+ * accessing the memory modified by d_path helper.
+ */
+static void test_d_path_mem_access(void)
+{
+ int localfd = -1;
+ struct test_d_path__bss *bss;
+ struct test_d_path *skel;
+
+ attach_and_load(&skel);
+ if (!skel)
+ goto cleanup;
+
+ bss = skel->bss;
+
+ localfd = open("/tmp/d_path_loadgen.txt", O_CREAT | O_RDWR, 0644);
+ if (CHECK(localfd < 0, "trigger", "open /tmp/d_path_loadgen.txt failed\n"))
+ goto cleanup;
+
+ if (CHECK(fallocate(localfd, 0, 0, 1024) < 0, "trigger", "fallocate failed\n"))
+ goto cleanup;
+ remove("/tmp/d_path_loadgen.txt");
+
+ if (CHECK(!bss->path_match_fallocate, "check",
+ "failed to match actual opened path"))
+ goto cleanup;
+
+cleanup:
+ syscall_close(localfd);
+ test_d_path__destroy(skel);
+}
+
void test_d_path(void)
{
if (test__start_subtest("basic"))
@@ -205,4 +256,7 @@ void test_d_path(void)
if (test__start_subtest("check_alloc_mem"))
test_d_path_check_types();
+
+ if (test__start_subtest("check_mem_access"))
+ test_d_path_mem_access();
}
diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c b/tools/testing/selftests/bpf/progs/test_d_path.c
index 84e1f883f97b..2f9b4cb67931 100644
--- a/tools/testing/selftests/bpf/progs/test_d_path.c
+++ b/tools/testing/selftests/bpf/progs/test_d_path.c
@@ -17,6 +17,7 @@ int rets_close[MAX_FILES] = {};
int called_stat = 0;
int called_close = 0;
+int path_match_fallocate = 0;
SEC("fentry/security_inode_getattr")
int BPF_PROG(prog_stat, struct path *path, struct kstat *stat,
@@ -62,4 +63,26 @@ int BPF_PROG(prog_close, struct file *file, void *id)
return 0;
}
+SEC("fentry/vfs_fallocate")
+int BPF_PROG(prog_fallocate, struct file *file, int mode, loff_t offset, loff_t len)
+{
+ pid_t pid = bpf_get_current_pid_tgid() >> 32;
+ int ret = 0;
+ char path_fallocate[MAX_PATH_LEN] = {};
+
+ if (pid != my_pid)
+ return 0;
+
+ ret = bpf_d_path(&file->f_path,
+ path_fallocate, MAX_PATH_LEN);
+ if (ret < 0)
+ return 0;
+
+ if (path_fallocate[0] != '/')
+ return 0;
+
+ path_match_fallocate = 1;
+ return 0;
+}
+
char _license[] SEC("license") = "GPL";
--
2.52.0
^ permalink raw reply related [flat|nested] 6+ messages in thread