Date:   Sun, 17 Nov 2019 22:45:26 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Wenbo Zhang <ethercflow@...il.com>
Cc:     bpf <bpf@...r.kernel.org>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Yonghong Song <yhs@...com>, Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next v4] selftests/bpf: test for bpf_get_file_path()
 from raw tracepoint

On Fri, Nov 15, 2019 at 5:59 AM Wenbo Zhang <ethercflow@...il.com> wrote:
>
> trace fstat events by raw tracepoint sys_enter:newfstat, and handle events
> only produced by test_file_get_path, which call fstat on several different
> types of files to test bpf_get_file_path's feature.
>
> v3->v4: addressed Andrii's feedback
> - use a set of fd instead of fds array
> - use global variables instead of maps (in v3, I mistakenly thought that
> the bpf maps are global variables.)
> - remove uncessary global variable path_info_index
> - remove fd compare as the fstat's order is fixed
>
> v2->v3: addressed Andrii's feedback
> - use global data instead of perf_buffer to simplified code
>
> v1->v2: addressed Daniel's feedback
> - rename bpf_fd2path to bpf_get_file_path to be consistent with other
> helper's names
>
> Signed-off-by: Wenbo Zhang <ethercflow@...il.com>
> ---

I like this much better, so much simpler BPF program and userspace
parts has less unnecessary moving pieces. I just have minor nits
below.

But I think you should send this patch together with the patch adding
helper as one patch series. Both patches are either going to get
accepted together, or declined together; having them as a patch set is
more meaningful.

>  .../selftests/bpf/prog_tests/get_file_path.c  | 173 ++++++++++++++++++
>  .../selftests/bpf/progs/test_get_file_path.c  |  43 +++++
>  2 files changed, 216 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/get_file_path.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_get_file_path.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/get_file_path.c b/tools/testing/selftests/bpf/prog_tests/get_file_path.c
> new file mode 100644
> index 000000000000..446ee4dd20e2
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/get_file_path.c
> @@ -0,0 +1,173 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define _GNU_SOURCE
> +#include <test_progs.h>
> +#include <sys/stat.h>
> +#include <linux/sched.h>
> +#include <sys/syscall.h>
> +
> +#define MAX_PATH_LEN           128
> +#define MAX_FDS                        7
> +#define MAX_EVENT_NUM          16
> +
> +static struct file_path_test_data {
> +       pid_t pid;
> +       __u32 cnt;
> +       __u32 fds[MAX_EVENT_NUM];
> +       char paths[MAX_EVENT_NUM][MAX_PATH_LEN];
> +} src, dst;
> +
> +static inline int set_pathname(int fd)
> +{
> +       char buf[MAX_PATH_LEN];
> +
> +       snprintf(buf, MAX_PATH_LEN, "/proc/%d/fd/%d", src.pid, fd);
> +       src.fds[src.cnt] = fd;
> +       return readlink(buf, src.paths[src.cnt++], MAX_PATH_LEN);
> +}
> +
> +static int trigger_fstat_events(pid_t pid)
> +{
> +       int pipefd[2] = { -1, -1 };
> +       int sockfd = -1, procfd = -1, devfd = -1;
> +       int localfd = -1, indicatorfd = -1;
> +       struct stat fileStat;
> +       int ret = -1;
> +
> +       /* unmountable pseudo-filesystems */
> +       if (CHECK_FAIL(pipe(pipefd) < 0))
> +               return ret;
> +       /* unmountable pseudo-filesystems */
> +       sockfd = socket(AF_INET, SOCK_STREAM, 0);
> +       if (CHECK_FAIL(sockfd < 0))
> +               goto out_close;
> +       /* mountable pseudo-filesystems */
> +       procfd = open("/proc/self/comm", O_RDONLY);
> +       if (CHECK_FAIL(procfd < 0))
> +               goto out_close;
> +       devfd = open("/dev/urandom", O_RDONLY);
> +       if (CHECK_FAIL(devfd < 0))
> +               goto out_close;
> +       localfd = open("/tmp/fd2path_loadgen.txt", O_CREAT|O_RDONLY);
> +       if (CHECK_FAIL(localfd < 0))
> +               goto out_close;
> +       /* bpf_get_file_path will return path with (deleted) */
> +       remove("/tmp/fd2path_loadgen.txt");
> +       indicatorfd = open("/tmp/", O_PATH);
> +       if (CHECK_FAIL(indicatorfd < 0))
> +               goto out_close;
> +
> +       src.pid = pid;
> +
> +       ret = set_pathname(pipefd[0]);
> +       if (CHECK_FAIL(ret < 0))
> +               goto out_close;
> +       ret = set_pathname(pipefd[1]);
> +       if (CHECK_FAIL(ret < 0))
> +               goto out_close;
> +       ret = set_pathname(sockfd);
> +       if (CHECK_FAIL(ret < 0))
> +               goto out_close;
> +       ret = set_pathname(procfd);
> +       if (CHECK_FAIL(ret < 0))
> +               goto out_close;
> +       ret = set_pathname(devfd);
> +       if (CHECK_FAIL(ret < 0))
> +               goto out_close;
> +       ret = set_pathname(localfd);
> +       if (CHECK_FAIL(ret < 0))
> +               goto out_close;
> +       ret = set_pathname(indicatorfd);
> +       if (CHECK_FAIL(ret < 0))
> +               goto out_close;
> +
> +       fstat(pipefd[0], &fileStat);
> +       fstat(pipefd[1], &fileStat);
> +       fstat(sockfd, &fileStat);
> +       fstat(procfd, &fileStat);
> +       fstat(devfd, &fileStat);
> +       fstat(localfd, &fileStat);
> +       fstat(indicatorfd, &fileStat);
> +
> +out_close:
> +       close(indicatorfd);
> +       close(localfd);
> +       close(devfd);
> +       close(procfd);
> +       close(sockfd);
> +       close(pipefd[1]);
> +       close(pipefd[0]);
> +
> +       return ret;
> +}
> +
> +void test_get_file_path(void)
> +{
> +       const char *prog_name = "tracepoint/syscalls/sys_enter_newfstat";
> +       const char *obj_file = "./test_get_file_path.o";
> +       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts, );
> +       int err, results_map_fd, duration = 0;
> +       struct bpf_program *tp_prog = NULL;
> +       struct bpf_link *tp_link = NULL;
> +       struct bpf_object *obj = NULL;
> +       const int zero = 0;
> +
> +       obj = bpf_object__open_file(obj_file, &opts);

you can just pass NULL for opts, as you are not really using any

> +       if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj)))
> +               return;
> +
> +       tp_prog = bpf_object__find_program_by_title(obj, prog_name);
> +       if (CHECK(!tp_prog, "find_tp",
> +                 "prog '%s' not found\n", prog_name))
> +               goto cleanup;
> +
> +       err = bpf_object__load(obj);
> +       if (CHECK(err, "obj_load", "err %d\n", err))
> +               goto cleanup;
> +
> +       results_map_fd = bpf_find_map(__func__, obj, "test_get.bss");
> +       if (CHECK(results_map_fd < 0, "find_bss_map",
> +                 "err %d\n", results_map_fd))
> +               goto cleanup;
> +
> +       tp_link = bpf_program__attach_tracepoint(tp_prog, "syscalls",
> +                                                "sys_enter_newfstat");

your patch subject says "raw tracepoint", but you are really using a
normal tracepoint here

> +       if (CHECK(IS_ERR(tp_link), "attach_tp",
> +                 "err %ld\n", PTR_ERR(tp_link))) {
> +               tp_link = NULL;
> +               goto cleanup;
> +       }
> +
> +       dst.pid = syscall(SYS_gettid);

see comment below, you can just use getpid(), as this test is single-threaded

> +       err = bpf_map_update_elem(results_map_fd, &zero, &dst, 0);
> +       if (CHECK(err, "update_elem",
> +                 "failed to set pid filter: %d\n", err))
> +               goto cleanup;
> +
> +       err = trigger_fstat_events(dst.pid);
> +       if (CHECK_FAIL(err < 0))
> +               goto cleanup;
> +
> +       err = bpf_map_lookup_elem(results_map_fd, &zero, &dst);
> +       if (CHECK(err, "get_results",
> +                 "failed to get results: %d\n", err))
> +               goto cleanup;
> +
> +       for (int i = 0; i < MAX_FDS; i++) {
> +               if (i < 3) {
> +                       CHECK((dst.paths[i][0] != '\0'), "get_file_path",

nit: unnecessary parens around check

> +                              "failed to filter fs [%d]: %u(%s) vs %u(%s)\n",
> +                              i, src.fds[i], src.paths[i], dst.fds[i],
> +                              dst.paths[i]);
> +               } else {
> +                       err = strncmp(src.paths[i], dst.paths[i], MAX_PATH_LEN);
> +                       CHECK(err != 0, "get_file_path",
> +                              "failed to get path[%d]: %u(%s) vs %u(%s)\n",
> +                              i, src.fds[i], src.paths[i], dst.fds[i],
> +                              dst.paths[i]);
> +               }
> +       }
> +
> +cleanup:
> +       bpf_link__destroy(tp_link);
> +       bpf_object__close(obj);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_get_file_path.c b/tools/testing/selftests/bpf/progs/test_get_file_path.c
> new file mode 100644
> index 000000000000..c006fa05e32b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_get_file_path.c
> @@ -0,0 +1,43 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bpf.h>
> +#include <linux/ptrace.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include "bpf_helpers.h"
> +#include "bpf_tracing.h"
> +
> +#define MAX_PATH_LEN           128
> +#define MAX_EVENT_NUM          16
> +
> +static struct file_path_test_data {
> +       pid_t pid;
> +       __u32 cnt;
> +       __u32 fds[MAX_EVENT_NUM];
> +       char paths[MAX_EVENT_NUM][MAX_PATH_LEN];
> +} data;
> +
> +struct sys_enter_newfstat_args {
> +       unsigned long long pad1;
> +       unsigned long long pad2;
> +       unsigned int fd;
> +};
> +
> +SEC("tracepoint/syscalls/sys_enter_newfstat")
> +int bpf_prog(struct sys_enter_newfstat_args *args)
> +{
> +       pid_t pid = bpf_get_current_pid_tgid();

you don't have to use thread id, given test_progs is single-threaded,
so can do (bpf_get_current_pid_tgid() >> 32) here and use getpid() in
userspace part.

> +
> +       if (pid != data.pid)
> +               return 0;
> +       if (data.cnt >= MAX_EVENT_NUM)
> +               return 0;
> +
> +       data.fds[data.cnt] = args->fd;
> +       bpf_get_file_path(data.paths[data.cnt], MAX_PATH_LEN, args->fd);
> +       data.cnt++;
> +
> +       return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.17.1
>

Powered by blists - more mailing lists