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

List overview All Threads
Download

newer

older

[PATCH v3 0/2] selftests/bpf:...

[PATCH v1] selftests/ftrace: Skip...

Lin Yikai

18 Jul 2024 18 Jul '24

11:51 a.m.

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

Show replies by date

Lin Yikai

18 Jul 18 Jul

11:51 a.m.

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

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

Andrii Nakryiko

19 Jul 19 Jul

6:55 p.m.

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

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

Lin Yikai

18 Jul 18 Jul

11:51 a.m.

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

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

Martin KaFai Lau

5:16 p.m.

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

113

days inactive

114

days old

linux-kselftest-mirror@lists.linaro.org

4 comments

participants

tags (0)

participants (3)

Andrii Nakryiko
Lin Yikai
Martin KaFai Lau