diff mbox series

[bpf-next,v2] selftests/bpf: test for bpf_get_file_path() from raw tracepoint

Message ID 20191101130817.11744-1-ethercflow@gmail.com
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series [bpf-next,v2] selftests/bpf: test for bpf_get_file_path() from raw tracepoint | expand

Commit Message

Wenbo Zhang Nov. 1, 2019, 1:08 p.m. UTC
trace fstat events by raw tracepoint sys_enter:newfstat, and handle events
only produced by fd2path_loadgen, the fd2path_loadgen call fstat on several
different types of files to test bpf_get_file_path's feature.
---
 tools/testing/selftests/bpf/Makefile          |   8 +-
 tools/testing/selftests/bpf/fd2path_loadgen.c |  75 ++++++++++
 .../selftests/bpf/prog_tests/get_file_path.c  | 130 ++++++++++++++++++
 .../selftests/bpf/progs/test_get_file_path.c  |  58 ++++++++
 4 files changed, 269 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/fd2path_loadgen.c
 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

Comments

Andrii Nakryiko Nov. 2, 2019, 5:48 a.m. UTC | #1
On Fri, Nov 1, 2019 at 6:08 AM Wenbo Zhang <ethercflow@gmail.com> wrote:
>
> trace fstat events by raw tracepoint sys_enter:newfstat, and handle events
> only produced by fd2path_loadgen, the fd2path_loadgen call fstat on several
> different types of files to test bpf_get_file_path's feature.
> ---

Unless there is a real reason for all this complexity (in which case,
please spell it out in commit or comments), I think this could be so
much simpler.

- you don't have to use perf_buffer to pass data back, just use global data;
- you can add a filter for PID to only capture data triggered by test
process and avoid the noise;
- why all those set_affinity dances? Is it just because you used
existing perf_buffer test which did that to specifically test
perf_buffer delivering data across every CPU core? Seems like your
test doesn't care about that...
- do we really need a separate binary generating hundreds of syscalls?
It's hard to synchronize with test and it seems much simpler to just
trigger necessary syscalls synchronously from the test itself, no?

I have a bunch of more minutia nits, but most of them will go away if
you simplify your testing approach anyway, so I'll postpone them till
then.

>  tools/testing/selftests/bpf/Makefile          |   8 +-
>  tools/testing/selftests/bpf/fd2path_loadgen.c |  75 ++++++++++
>  .../selftests/bpf/prog_tests/get_file_path.c  | 130 ++++++++++++++++++
>  .../selftests/bpf/progs/test_get_file_path.c  |  58 ++++++++
>  4 files changed, 269 insertions(+), 2 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/fd2path_loadgen.c
>  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
>

[...]
Wenbo Zhang Nov. 3, 2019, 9:57 a.m. UTC | #2
> Unless there is a real reason for all this complexity (in which case,
> please spell it out in commit or comments), I think this could be so
> much simpler.

Yes, it could be so much simpler than this implement.

> - you don't have to use perf_buffer to pass data back, just use global data;
> - you can add a filter for PID to only capture data triggered by test
> process and avoid the noise;

Yes, I'll use map instead of perf buffer to communicate.

> - why all those set_affinity dances? Is it just because you used
> existing perf_buffer test which did that to specifically test
> perf_buffer delivering data across every CPU core? Seems like your
> test doesn't care about that...

I used fd2path_loadgen to get hundreds of syscalls between multi usleep, which
may cause it's sched to different cores, but as you say, this test
doesn't care about
that... I'll remove them with perf buffer.

> - do we really need a separate binary generating hundreds of syscalls?
> It's hard to synchronize with test and it seems much simpler to just
> trigger necessary syscalls synchronously from the test itself, no?

This is my fault, necessary syscalls synchronously from the test
itself is enough.

> I have a bunch of more minutia nits, but most of them will go away if
> you simplify your testing approach anyway, so I'll postpone them till
> then.

Thanks a lot. I'll modify a simplified version then recommitted.

Andrii Nakryiko <andrii.nakryiko@gmail.com> 于2019年11月2日周六 下午1:49写道:

>
> On Fri, Nov 1, 2019 at 6:08 AM Wenbo Zhang <ethercflow@gmail.com> wrote:
> >
> > trace fstat events by raw tracepoint sys_enter:newfstat, and handle events
> > only produced by fd2path_loadgen, the fd2path_loadgen call fstat on several
> > different types of files to test bpf_get_file_path's feature.
> > ---
>
> Unless there is a real reason for all this complexity (in which case,
> please spell it out in commit or comments), I think this could be so
> much simpler.
>
> - you don't have to use perf_buffer to pass data back, just use global data;
> - you can add a filter for PID to only capture data triggered by test
> process and avoid the noise;
> - why all those set_affinity dances? Is it just because you used
> existing perf_buffer test which did that to specifically test
> perf_buffer delivering data across every CPU core? Seems like your
> test doesn't care about that...
> - do we really need a separate binary generating hundreds of syscalls?
> It's hard to synchronize with test and it seems much simpler to just
> trigger necessary syscalls synchronously from the test itself, no?
>
> I have a bunch of more minutia nits, but most of them will go away if
> you simplify your testing approach anyway, so I'll postpone them till
> then.
>
> >  tools/testing/selftests/bpf/Makefile          |   8 +-
> >  tools/testing/selftests/bpf/fd2path_loadgen.c |  75 ++++++++++
> >  .../selftests/bpf/prog_tests/get_file_path.c  | 130 ++++++++++++++++++
> >  .../selftests/bpf/progs/test_get_file_path.c  |  58 ++++++++
> >  4 files changed, 269 insertions(+), 2 deletions(-)
> >  create mode 100644 tools/testing/selftests/bpf/fd2path_loadgen.c
> >  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 mbox series

Patch

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index b334a6db15c1..6c7e5cabc4e6 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -72,7 +72,7 @@  TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
 	flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \
 	test_lirc_mode2_user
 
-TEST_CUSTOM_PROGS = urandom_read
+TEST_CUSTOM_PROGS = urandom_read fd2path_loadgen
 
 include ../lib.mk
 
@@ -92,6 +92,9 @@  $(OUTPUT)/urandom_read: urandom_read.c
 $(OUTPUT)/test_stub.o: test_stub.c
 	$(CC) -c $(CFLAGS) -o $@ $<
 
+$(OUTPUT)/fd2path_loadgen: fd2path_loadgen.c
+	$(CC) -o $@ $< -Wl,--build-id
+
 BPFOBJ := $(OUTPUT)/libbpf.a
 
 $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED): $(OUTPUT)/test_stub.o $(BPFOBJ)
@@ -266,7 +269,8 @@  TRUNNER_TESTS_DIR := prog_tests
 TRUNNER_BPF_PROGS_DIR := progs
 TRUNNER_EXTRA_SOURCES := test_progs.c cgroup_helpers.c trace_helpers.c	\
 			 flow_dissector_load.h
-TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read				\
+TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read 				\
+		       $(OUTPUT)/fd2path_loadgen                        \
 		       $(wildcard progs/btf_dump_test_case_*.c)
 TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
 TRUNNER_BPF_CFLAGS := -I. -I$(OUTPUT) $(BPF_CFLAGS) $(CLANG_CFLAGS)
diff --git a/tools/testing/selftests/bpf/fd2path_loadgen.c b/tools/testing/selftests/bpf/fd2path_loadgen.c
new file mode 100644
index 000000000000..afa9d6b233b2
--- /dev/null
+++ b/tools/testing/selftests/bpf/fd2path_loadgen.c
@@ -0,0 +1,75 @@ 
+// SPDX-License-Identifier: GPL-2.0
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <sys/socket.h>
+#include <alloca.h>
+#include <sys/stat.h>
+
+enum FS_TYPE {
+	PIPE_0,
+	PIPE_1,
+	SOCK,
+	PROC,
+	DEV,
+	LOCAL,
+	INDICATOR,
+	MAX_FDS
+};
+
+#ifndef MAX_LOOP_TIMES
+#define MAX_LOOP_TIMES		100
+#endif
+
+int main(int argc, char *argv[])
+{
+	int *fds = alloca(sizeof(int) * MAX_FDS);
+	int *pipefd = fds;
+	int *sockfd = fds + SOCK;
+	int *procfd = fds + PROC;
+	int *devfd = fds + DEV;
+	int *localfd = fds + LOCAL;
+	int *indicatorfd = fds + INDICATOR;
+	int times = MAX_LOOP_TIMES;
+
+	/* unmountable pseudo-filesystems */
+	if (pipe(pipefd) < 0)
+		return 1;
+
+	/* unmountable pseudo-filesystems */
+	*sockfd = socket(AF_INET, SOCK_STREAM, 0);
+	if (sockfd < 0)
+		return 1;
+
+	/* mountable pseudo-filesystems */
+	*procfd = open("/proc/self/comm", O_RDONLY);
+	if (procfd < 0)
+		return 1;
+
+	*devfd = open("/dev/urandom", O_RDONLY);
+	if (devfd < 0)
+		return 1;
+
+	*localfd = open("/tmp/fd2path_loadgen.txt", O_CREAT|O_RDONLY);
+	if (localfd < 0)
+		return 1;
+
+	*indicatorfd = open("/tmp/", O_PATH);
+
+	while (times--) {
+		struct stat fileStat;
+
+		for (int i = 0; i < MAX_FDS; i++) {
+			fstat(fds[i], &fileStat);
+			usleep(1);
+		}
+	}
+
+	for (int i = 0; i < MAX_FDS; i++)
+		close(fds[i]);
+
+	remove("/tmp/fd2path_loadgen.txt");
+
+	return 0;
+}
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..00e96151faaa
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/get_file_path.c
@@ -0,0 +1,130 @@ 
+// SPDX-License-Identifier: GPL-2.0
+#define _GNU_SOURCE
+#include <pthread.h>
+#include <test_progs.h>
+
+#ifndef MAX_PATH_LENGTH
+#define MAX_PATH_LENGTH		128
+#endif
+
+#ifndef TASK_COMM_LEN
+#define TASK_COMM_LEN		16
+#endif
+
+#ifndef MAX_STAT_EVENTS
+#define MAX_STAT_EVENTS		64ull
+#endif
+
+struct get_path_trace_t {
+	int pid;
+	unsigned long fd;
+	char comm[TASK_COMM_LEN];
+	char path[MAX_PATH_LENGTH];
+};
+
+static const char *loadgen = "./fd2path_loadgen";
+static int exp_cnt = MAX_STAT_EVENTS;
+
+void *thread_loadgen(void *arg)
+{
+	assert(system(loadgen) == 0);
+	return NULL;
+}
+
+static void get_path_print_output(void *ctx, int cpu, void *data, __u32 size)
+{
+	struct get_path_trace_t *e = data;
+	char pathname[MAX_PATH_LENGTH] = {'0'};
+	char buf[MAX_PATH_LENGTH] = {'0'};
+	int ret, duration = 0;
+
+	if (strncmp(e->comm, &loadgen[2], MAX_PATH_LENGTH))
+		return;
+	snprintf(pathname, MAX_PATH_LENGTH, "/proc/%d/fd/%lu", e->pid, e->fd);
+	readlink(pathname, buf, MAX_PATH_LENGTH);
+	exp_cnt--;
+	ret = strncmp(buf, e->path, MAX_PATH_LENGTH);
+	CHECK(ret != 0, "get_file_path", "failed to get path: %lu->%s\n",
+			e->fd, e->path);
+}
+
+void test_get_file_path(void)
+{
+	const char *prog_name = "raw_tracepoint/sys_enter:newfstat";
+	const char *file = "./test_get_file_path.o";
+	int err, nr_cpus, duration = 0;
+	struct perf_buffer_opts pb_opts = {};
+	struct perf_buffer *pb = NULL;
+	struct bpf_map *perf_buf_map;
+	cpu_set_t cpu_set, cpu_seen;
+	struct bpf_link *link = NULL;
+	struct timespec tv = {0, 10};
+	struct bpf_program *prog;
+	struct bpf_object *obj;
+	pthread_t t = 0;
+
+	obj = bpf_object__open_file(file, NULL);
+	if (CHECK(IS_ERR(obj), "obj_open", "err %ld\n", PTR_ERR(obj)))
+		return;
+
+	err = bpf_object__load(obj);
+	if (CHECK(err, "obj_load", "err %d errno %d\n", err, errno))
+		goto out_close;
+
+	prog = bpf_object__find_program_by_title(obj, prog_name);
+	if (CHECK(!prog, "find_prog", "prog '%s' not found\n", prog_name))
+		goto out_close;
+
+	link = bpf_program__attach_raw_tracepoint(prog, "sys_enter");
+	if (CHECK(IS_ERR(link), "attach_tp", "err %ld\n", PTR_ERR(link)))
+		goto out_close;
+
+	nr_cpus = libbpf_num_possible_cpus();
+	if (CHECK(nr_cpus < 0, "nr_cpus", "err %d\n", nr_cpus))
+		goto out_close;
+
+	CPU_ZERO(&cpu_seen);
+	for (int i = 0; i < nr_cpus; i++) {
+		CPU_ZERO(&cpu_set);
+		CPU_SET(i, &cpu_set);
+
+		err = pthread_setaffinity_np(pthread_self(), sizeof(cpu_set),
+				&cpu_set);
+		if (err && CHECK(err, "set_affinity", "cpu #%d, err %d\n",
+				i, err))
+			goto out_detach;
+
+		usleep(1);
+	}
+
+	perf_buf_map = bpf_object__find_map_by_name(obj, "perfmap");
+	if (CHECK(!perf_buf_map, "bpf_find_map", "not found\n"))
+		goto out_close;
+
+	pb_opts.sample_cb = get_path_print_output;
+	pb_opts.ctx = &cpu_seen;
+	pb = perf_buffer__new(bpf_map__fd(perf_buf_map), 1, &pb_opts);
+	if (CHECK(IS_ERR(pb), "perf_buf__new", "err %ld\n", PTR_ERR(pb)))
+		goto out_detach;
+
+	pthread_create(&t, NULL, thread_loadgen, NULL);
+
+	/* trigger some fstat syscall action */
+	for (int i = 0; i < MAX_STAT_EVENTS; i++)
+		nanosleep(&tv, NULL);
+
+	while (exp_cnt > 0) {
+		err = perf_buffer__poll(pb, 100);
+		if (err < 0 && CHECK(err < 0, "pb__poll", "err %d\n", err))
+			goto out_free_pb;
+	}
+
+out_free_pb:
+	perf_buffer__free(pb);
+out_detach:
+	bpf_link__destroy(link);
+out_close:
+	bpf_object__close(obj);
+
+	pthread_join(t, NULL);
+}
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..2d3efb6b71f2
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_get_file_path.c
@@ -0,0 +1,58 @@ 
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <linux/ptrace.h>
+#include <stdbool.h>
+#include <string.h>
+#include "bpf_helpers.h"
+
+#ifndef MAX_PATH_LENGTH
+#define MAX_PATH_LENGTH		128
+#endif
+
+#ifndef TASK_COMM_LEN
+#define TASK_COMM_LEN		16
+#endif
+
+struct path_trace_t {
+	int pid;
+	unsigned long fd;
+	char comm[TASK_COMM_LEN];
+	char path[MAX_PATH_LENGTH];
+};
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY);
+	__uint(max_entries, 128);
+	__uint(key_size, sizeof(int));
+	__uint(value_size, sizeof(__u32));
+} perfmap SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PERCPU_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, __u32);
+	__type(value, struct path_trace_t);
+} pathdata_map SEC(".maps");
+
+SEC("raw_tracepoint/sys_enter:newfstat")
+int bpf_prog(struct bpf_raw_tracepoint_args *ctx)
+{
+	struct path_trace_t *data;
+	struct pt_regs *regs;
+	__u32 key = 0;
+
+	data = bpf_map_lookup_elem(&pathdata_map, &key);
+	if (!data)
+		return 0;
+	data->pid = bpf_get_current_pid_tgid() >> 32;
+	regs = (struct pt_regs *)ctx->args[0];
+	bpf_probe_read(&data->fd, sizeof(data->fd), &regs->rdi);
+	bpf_get_current_comm(&data->comm, TASK_COMM_LEN);
+	if (bpf_get_file_path(data->path, MAX_PATH_LENGTH, data->fd) < 0)
+		return 0;
+	bpf_perf_event_output(ctx, &perfmap, BPF_F_CURRENT_CPU,
+			data, sizeof(*data));
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";