> I like this much better, so much simpler BPF program and userspace
> parts has less unnecessary moving pieces. I just have minor nits
> below.
Thank you for your patience, you're so sweet. I've read your comments
and will fix them and resubmit as patchset.
> you can just pass NULL for opts, as you are not really using any
Agree, and I found if opt is NULL,
> const char *obj_file = "./test_get_file_path.o";
must be changed to "test_get_file_path.o", or bpf_obj_name_cpy will
return -EINVAL. :)
> your patch subject says "raw tracepoint", but you are really using a
> normal tracepoint here
Sorry for this, I'll make it keep up with the code
> see comment below, you can just use getpid(), as this test is single-threaded
> 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.
I found I was thinking too much. I thought test_progs maybe
multi-threaded one day,
so I used SYS_gettid. I feel that it is very bad now. It will make
people misunderstand that it
is multi-threaded. Thank you for point it out.
> nit: unnecessary parens around check
Agree, I'll fix this.
Thank you.
Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> 于2019年11月18日周一 下午2:45写道：
>
> On Fri, Nov 15, 2019 at 5:59 AM Wenbo Zhang <ethercflow@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
>
> 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
> >
