[PATCH bpf-next v1 0/3] add bpf_file_d_path helper and selftests

[PATCH bpf-next v1 0/3] add bpf_file_d_path helper and selftests

[Lin Yikai]

v1:
 - patch 2:
   - [1/2] add bpf_file_d_path helper
   - [2/2] add selftest to it

Hi, we are looking to add the "bpf_file_d_path" helper, 
used to retrieve the path from a struct file object.
	bpf_file_d_path(void *file, char *dst, u32 size);
	
It's worth noting that the "file" parameter is defined as "void*" type.

* Our problems *
Previously, we encountered issues 
on some user-space operating systems(OS):

1.Difficulty using vmlinux.h
(1) The OS lacks support for bpftool.
We can not use:
"bpftool btf dump file /sys/kernel/btf/vmlinux format c > vmlinux.h".
Bpftool need a separate complex cross-compilation environment to build.

(2) Many duplicate definitions between OS and vmlinux.h.

(3) The vmlinux.h size is large (2.8MB on arm64/Android), 
causing increased ebpf prog size and user space consumption.

2.The "struct file" has many internal variables and definitions,
and maybe change along with Linux version iterations,
making it hard to copy it to OS.


* Benefits of this commit *
1.There is no need to include vmlinux.h or redefine "struct file".

For example, with bpf on kprobe, 
we can directly pass param "(void*)PT_REGS_PARM1(pt_regs)"
to "bpf_file_d_path" helper in order to retrieve the path.


Appreciate your review and assistance. Thank you.
Yikai


Lin Yikai (2):
  bpf: Add bpf_file_d_path helper
  selftests/bpf:Adding test for bpf_file_d_path helper

 include/uapi/linux/bpf.h                      |  20 +++
 kernel/trace/bpf_trace.c                      |  34 ++++++
 tools/include/uapi/linux/bpf.h                |  20 +++
 .../selftests/bpf/prog_tests/file_d_path.c    | 115 ++++++++++++++++++
 .../selftests/bpf/progs/test_file_d_path.c    |  32 +++++
 5 files changed, 221 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/file_d_path.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_file_d_path.c

-- 
2.34.1

[PATCH bpf-next v1 1/3] bpf: Add bpf_file_d_path helper

[Lin Yikai]

Add the "bpf_file_d_path" helper function
to retrieve the path from a struct file object.
But there is no need to include vmlinux.h
or reference the definition of struct file.

Additionally, update the bpf.h tools uapi header.

Signed-off-by: Lin Yikai <yikai.lin@vivo.com>
---
 include/uapi/linux/bpf.h       | 20 ++++++++++++++++++++
 kernel/trace/bpf_trace.c       | 34 ++++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h | 20 ++++++++++++++++++++
 3 files changed, 74 insertions(+)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 35bcf52dbc65..7e5cec61a877 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -5792,6 +5792,25 @@ union bpf_attr {
  *		0 on success.
  *
  *		**-ENOENT** if the bpf_local_storage cannot be found.
+ *
+ * long bpf_file_d_path(void *file, char *dst, u32 size)
+ *	Description
+ *		Return full path for the given *file* object.
+ *
+ *		In order to solve issues where certain eBPF programs can not include
+ *		the definition of struct file or vmlinux.h
+ *		due to their complexity and conflicts on some operating system,
+ *		the variable *file* here is declared as type void*
+ *		instead of the traditional struct file*.
+ *		It will be forcibly converted into type struct file* later.
+ *
+ *		If the path is larger than *size*, then only *size*
+ *		bytes will be copied to *dst*
+ *
+ *	Return
+ *		On success, the strictly positive length of the string,
+ *		including the trailing NULL character. On error, a negative
+ *		value.
  */
 #define ___BPF_FUNC_MAPPER(FN, ctx...)			\
 	FN(unspec, 0, ##ctx)				\
@@ -6006,6 +6025,7 @@ union bpf_attr {
 	FN(user_ringbuf_drain, 209, ##ctx)		\
 	FN(cgrp_storage_get, 210, ##ctx)		\
 	FN(cgrp_storage_delete, 211, ##ctx)		\
+	FN(file_d_path, 212, ##ctx)			\
 	/* */
 
 /* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index cd098846e251..70fde7f20e97 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1257,6 +1257,38 @@ static const struct bpf_func_proto bpf_get_func_arg_cnt_proto = {
 	.arg1_type	= ARG_PTR_TO_CTX,
 };
 
+BPF_CALL_3(bpf_file_d_path, void *, file, char*, dst, u32, size)
+{
+	long len;
+	struct file copy;
+	char *ptr;
+
+	if (!size)
+		return 0;
+
+	len = copy_from_kernel_nofault(&copy, (struct file *)file, sizeof(struct file));
+	if (len < 0)
+		return len;
+
+	ptr = d_path(&(copy.f_path), dst, size);
+	if (IS_ERR(ptr)) {
+		len = PTR_ERR(ptr);
+	} else {
+		len = dst + size - ptr;
+		memmove(dst, ptr, len);
+	}
+	return len;
+}
+
+const struct bpf_func_proto bpf_file_d_path_proto = {
+	.func		= bpf_file_d_path,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_ANYTHING,
+	.arg2_type	= ARG_PTR_TO_MEM,
+	.arg3_type	= ARG_CONST_SIZE_OR_ZERO,
+};
+
 #ifdef CONFIG_KEYS
 __bpf_kfunc_start_defs();
 
@@ -1629,6 +1661,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_find_vma_proto;
 	case BPF_FUNC_trace_vprintk:
 		return bpf_get_trace_vprintk_proto();
+	case BPF_FUNC_file_d_path:
+		return &bpf_file_d_path_proto;
 	default:
 		return bpf_base_func_proto(func_id, prog);
 	}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 35bcf52dbc65..7e5cec61a877 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -5792,6 +5792,25 @@ union bpf_attr {
  *		0 on success.
  *
  *		**-ENOENT** if the bpf_local_storage cannot be found.
+ *
+ * long bpf_file_d_path(void *file, char *dst, u32 size)
+ *	Description
+ *		Return full path for the given *file* object.
+ *
+ *		In order to solve issues where certain eBPF programs can not include
+ *		the definition of struct file or vmlinux.h
+ *		due to their complexity and conflicts on some operating system,
+ *		the variable *file* here is declared as type void*
+ *		instead of the traditional struct file*.
+ *		It will be forcibly converted into type struct file* later.
+ *
+ *		If the path is larger than *size*, then only *size*
+ *		bytes will be copied to *dst*
+ *
+ *	Return
+ *		On success, the strictly positive length of the string,
+ *		including the trailing NULL character. On error, a negative
+ *		value.
  */
 #define ___BPF_FUNC_MAPPER(FN, ctx...)			\
 	FN(unspec, 0, ##ctx)				\
@@ -6006,6 +6025,7 @@ union bpf_attr {
 	FN(user_ringbuf_drain, 209, ##ctx)		\
 	FN(cgrp_storage_get, 210, ##ctx)		\
 	FN(cgrp_storage_delete, 211, ##ctx)		\
+	FN(file_d_path, 212, ##ctx)			\
 	/* */
 
 /* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
-- 
2.34.1

Re: [PATCH bpf-next v1 1/3] bpf: Add bpf_file_d_path helper

[Andrii Nakryiko]

On Thu, Jul 18, 2024 at 4:53 AM Lin Yikai <yikai.lin@vivo.com> wrote:
>
> Add the "bpf_file_d_path" helper function
> to retrieve the path from a struct file object.
> But there is no need to include vmlinux.h
> or reference the definition of struct file.
>
> Additionally, update the bpf.h tools uapi header.
>
> Signed-off-by: Lin Yikai <yikai.lin@vivo.com>
> ---
>  include/uapi/linux/bpf.h       | 20 ++++++++++++++++++++
>  kernel/trace/bpf_trace.c       | 34 ++++++++++++++++++++++++++++++++++
>  tools/include/uapi/linux/bpf.h | 20 ++++++++++++++++++++
>  3 files changed, 74 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 35bcf52dbc65..7e5cec61a877 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -5792,6 +5792,25 @@ union bpf_attr {
>   *             0 on success.
>   *
>   *             **-ENOENT** if the bpf_local_storage cannot be found.
> + *
> + * long bpf_file_d_path(void *file, char *dst, u32 size)
> + *     Description
> + *             Return full path for the given *file* object.
> + *
> + *             In order to solve issues where certain eBPF programs can not include
> + *             the definition of struct file or vmlinux.h
> + *             due to their complexity and conflicts on some operating system,
> + *             the variable *file* here is declared as type void*
> + *             instead of the traditional struct file*.
> + *             It will be forcibly converted into type struct file* later.
> + *
> + *             If the path is larger than *size*, then only *size*
> + *             bytes will be copied to *dst*
> + *
> + *     Return
> + *             On success, the strictly positive length of the string,
> + *             including the trailing NULL character. On error, a negative
> + *             value.
>   */
>  #define ___BPF_FUNC_MAPPER(FN, ctx...)                 \
>         FN(unspec, 0, ##ctx)                            \
> @@ -6006,6 +6025,7 @@ union bpf_attr {
>         FN(user_ringbuf_drain, 209, ##ctx)              \
>         FN(cgrp_storage_get, 210, ##ctx)                \
>         FN(cgrp_storage_delete, 211, ##ctx)             \
> +       FN(file_d_path, 212, ##ctx)                     \
>         /* */
>
>  /* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index cd098846e251..70fde7f20e97 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1257,6 +1257,38 @@ static const struct bpf_func_proto bpf_get_func_arg_cnt_proto = {
>         .arg1_type      = ARG_PTR_TO_CTX,
>  };
>
> +BPF_CALL_3(bpf_file_d_path, void *, file, char*, dst, u32, size)
> +{
> +       long len;
> +       struct file copy;
> +       char *ptr;
> +
> +       if (!size)
> +               return 0;
> +
> +       len = copy_from_kernel_nofault(&copy, (struct file *)file, sizeof(struct file));
> +       if (len < 0)
> +               return len;
> +
> +       ptr = d_path(&(copy.f_path), dst, size);
> +       if (IS_ERR(ptr)) {
> +               len = PTR_ERR(ptr);
> +       } else {
> +               len = dst + size - ptr;
> +               memmove(dst, ptr, len);
> +       }
> +       return len;
> +}
> +
> +const struct bpf_func_proto bpf_file_d_path_proto = {
> +       .func           = bpf_file_d_path,
> +       .gpl_only       = false,
> +       .ret_type       = RET_INTEGER,
> +       .arg1_type      = ARG_ANYTHING,

you can't just accept any random value as `struct file *`, this is a
complete no-go. It will have to accept some sort of PTR_TRUSTED
argument, be added as kfunc, etc, etc. We had earlier discussion
around this, I don't remember all the details, but this is definitely
not the way forward.

> +       .arg2_type      = ARG_PTR_TO_MEM,
> +       .arg3_type      = ARG_CONST_SIZE_OR_ZERO,
> +};
> +
>  #ifdef CONFIG_KEYS
>  __bpf_kfunc_start_defs();
>
> @@ -1629,6 +1661,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>                 return &bpf_find_vma_proto;
>         case BPF_FUNC_trace_vprintk:
>                 return bpf_get_trace_vprintk_proto();
> +       case BPF_FUNC_file_d_path:
> +               return &bpf_file_d_path_proto;
>         default:
>                 return bpf_base_func_proto(func_id, prog);
>         }
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 35bcf52dbc65..7e5cec61a877 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -5792,6 +5792,25 @@ union bpf_attr {
>   *             0 on success.
>   *
>   *             **-ENOENT** if the bpf_local_storage cannot be found.
> + *
> + * long bpf_file_d_path(void *file, char *dst, u32 size)
> + *     Description
> + *             Return full path for the given *file* object.
> + *
> + *             In order to solve issues where certain eBPF programs can not include
> + *             the definition of struct file or vmlinux.h
> + *             due to their complexity and conflicts on some operating system,
> + *             the variable *file* here is declared as type void*
> + *             instead of the traditional struct file*.
> + *             It will be forcibly converted into type struct file* later.
> + *
> + *             If the path is larger than *size*, then only *size*
> + *             bytes will be copied to *dst*
> + *
> + *     Return
> + *             On success, the strictly positive length of the string,
> + *             including the trailing NULL character. On error, a negative
> + *             value.
>   */
>  #define ___BPF_FUNC_MAPPER(FN, ctx...)                 \
>         FN(unspec, 0, ##ctx)                            \
> @@ -6006,6 +6025,7 @@ union bpf_attr {
>         FN(user_ringbuf_drain, 209, ##ctx)              \
>         FN(cgrp_storage_get, 210, ##ctx)                \
>         FN(cgrp_storage_delete, 211, ##ctx)             \
> +       FN(file_d_path, 212, ##ctx)                     \
>         /* */
>
>  /* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
> --
> 2.34.1
>

[PATCH bpf-next v1 2/3] selftests/bpf:Adding test for bpf_file_d_path helper

[Lin Yikai]

Hi, Adding test for bpf_file_d_path helper.

It passed the test in my environment using QEMU
(./test_progs -t file_d_path)

Below are some partial results:
'''
  + [ -x /etc/rcS.d/S50-startup ]
  + /etc/rcS.d/S50-startup
  ./test_progs -t file_d_path
  Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
'''

Signed-off-by: Lin Yikai <yikai.lin@vivo.com>
---
 .../selftests/bpf/prog_tests/file_d_path.c    | 115 ++++++++++++++++++
 .../selftests/bpf/progs/test_file_d_path.c    |  32 +++++
 2 files changed, 147 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/file_d_path.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_file_d_path.c

diff --git a/tools/testing/selftests/bpf/prog_tests/file_d_path.c b/tools/testing/selftests/bpf/prog_tests/file_d_path.c
new file mode 100644
index 000000000000..ba76d9467f3e
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/file_d_path.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include <sys/syscall.h>
+
+#include "test_file_d_path.skel.h"
+
+/* Compatible with older versions of glibc begin */
+#ifndef __NR_close_range
+#ifdef __alpha__
+#define __NR_close_range 546
+#else
+#define __NR_close_range 436
+#endif
+#endif
+
+#define close_fd(fd) syscall(__NR_close_range, fd, fd, 0)
+/* Compatible with older versions of glibc end */
+
+
+#define MAX_PATH_LEN	256
+#define TEST_FILES_NUM		2
+
+static int duration;
+
+static struct {
+	__u32 cnt;
+	char paths[TEST_FILES_NUM][MAX_PATH_LEN];
+} record;
+
+static int set_pathname(int fd, pid_t pid)
+{
+	char buf[MAX_PATH_LEN];
+
+	snprintf(buf, MAX_PATH_LEN, "/proc/%d/fd/%d", pid, fd);
+	return readlink(buf, record.paths[record.cnt++], MAX_PATH_LEN);
+}
+
+static int trigger_filp_close(pid_t pid)
+{
+	int ret = -1;
+	const char *comm_path = "/proc/self/comm";
+	int commfd = -1;
+	const char *tmp_path = "/tmp/test_bpf_file_d_path.txt";
+	int tmpfd = -1;
+
+	/* open file */
+	commfd = open(comm_path, O_RDONLY);
+	if (CHECK(commfd < 0, "test_file_d_path", "open %s failed\n", comm_path))
+		goto fd_close;
+
+	tmpfd = open(tmp_path, O_CREAT | O_RDONLY, 0644);
+	if (CHECK(tmpfd < 0, "test_file_d_path", "open %s failed\n", tmp_path))
+		goto fd_close;
+	remove(tmp_path);
+
+	/* record file */
+	memset(&record, 0, sizeof(record));
+	ret = set_pathname(commfd, pid);
+	if (CHECK(ret < 0, "test_file_d_path", "set_pathname failed for commfd\n"))
+		goto fd_close;
+	ret = set_pathname(tmpfd, pid);
+	if (CHECK(ret < 0, "test_file_d_path", "set_pathname failed for tmpfd\n"))
+		goto fd_close;
+
+	ret = 0;
+	/* close file */
+fd_close:
+	if (commfd != -1)
+		close_fd(commfd);
+	if (tmpfd != -1)
+		close_fd(tmpfd);
+	return ret;
+}
+
+static void test_base(void)
+{
+	int err = -1;
+	struct test_file_d_path__bss *bss;
+	struct test_file_d_path *skel;
+
+	skel = test_file_d_path__open_and_load();
+	if (CHECK(!skel, "open_and_load", "load file_d_path skeleton failed\n"))
+		goto cleanup;
+
+	err = test_file_d_path__attach(skel);
+	if (CHECK(err, "attach", "attach file_d_path failed: %s\n", strerror(errno)))
+		goto cleanup;
+
+	bss = skel->bss;
+	bss->monitor_pid = getpid();
+
+	err = trigger_filp_close(bss->monitor_pid);
+	if (err < 0)
+		goto cleanup;
+
+	if (CHECK(bss->bpf_called_cnt != TEST_FILES_NUM,
+		"bpf_called_cnt",
+		"prog called times diff from with the expectations\n"))
+		goto cleanup;
+
+	for (int i = 0; i < TEST_FILES_NUM; i++) {
+		CHECK(strncmp(record.paths[i], bss->bpf_paths_close[i], MAX_PATH_LEN),
+			"bpf_paths_close",
+			"the paths diff from the expectations: id=[%d], path: %s vs %s\n",
+			i, record.paths[i], bss->bpf_paths_close[i]);
+	}
+
+cleanup:
+	test_file_d_path__destroy(skel);
+}
+
+void test_file_d_path(void)
+{
+	test_base();
+}
diff --git a/tools/testing/selftests/bpf/progs/test_file_d_path.c b/tools/testing/selftests/bpf/progs/test_file_d_path.c
new file mode 100644
index 000000000000..8db2bcd1179f
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_file_d_path.c
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+#define MAX_PATH_LEN	256
+#define TEST_FILES_NUM		2
+
+pid_t monitor_pid = 0;
+
+__u32 bpf_called_cnt = 0;
+char bpf_paths_close[TEST_FILES_NUM][MAX_PATH_LEN] = {0};
+
+SEC("kprobe/filp_close")
+int test_bpf_file_to_path(struct pt_regs *regs)
+{
+	void *file = NULL;
+	pid_t cur_pid = bpf_get_current_pid_tgid() >> 32;
+
+	if (cur_pid != monitor_pid)
+		return 0;
+
+	if (bpf_called_cnt >= TEST_FILES_NUM)
+		return 0;
+
+	file = (void *)PT_REGS_PARM1(regs);
+	bpf_file_d_path(file, bpf_paths_close[bpf_called_cnt++], MAX_PATH_LEN);
+
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.34.1

Re: [PATCH bpf-next v1 0/3] add bpf_file_d_path helper and selftests

[Martin KaFai Lau]

On 7/18/24 4:51 AM, Lin Yikai wrote:
> v1:
>   - patch 2:
>     - [1/2] add bpf_file_d_path helper
>     - [2/2] add selftest to it
> 
> Hi, we are looking to add the "bpf_file_d_path" helper,
> used to retrieve the path from a struct file object.
> 	bpf_file_d_path(void *file, char *dst, u32 size);
> 	
> It's worth noting that the "file" parameter is defined as "void*" type.
> 
> * Our problems *
> Previously, we encountered issues
> on some user-space operating systems(OS):
> 
> 1.Difficulty using vmlinux.h
> (1) The OS lacks support for bpftool.
> We can not use:
> "bpftool btf dump file /sys/kernel/btf/vmlinux format c > vmlinux.h".
> Bpftool need a separate complex cross-compilation environment to build.
> 
> (2) Many duplicate definitions between OS and vmlinux.h.
> 
> (3) The vmlinux.h size is large (2.8MB on arm64/Android),
> causing increased ebpf prog size and user space consumption.

The compiled bpf prog size is increased by 2.8MB because it included vmlinux.h?

> 
> 2.The "struct file" has many internal variables and definitions,
> and maybe change along with Linux version iterations,
> making it hard to copy it to OS.

If vmlinux.h is not convenience in your use case, you can try to define "struct 
file" with __attribute__((preserve_access_index)) and the libbpf will adjust the 
bpf prog against the running kernel.

There was a discussion a year ago about bpf helpers freeze. No new helper can be 
added since then. The same goes for this one.

pw-bot: cr