| Message ID | 20191121175900.3486133-1-andriin@fb.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] selftests/bpf: ensure core_reloc_kernel is reading test_progs's data only | expand |
Andrii Nakryiko wrote: > test_core_reloc_kernel.c selftest is the only CO-RE test that reads and > returns for validation calling thread's information (pid, tgid, comm). Thus it > has to make sure that only test_prog's invocations are honored. > > Fixes: df36e621418b ("selftests/bpf: add CO-RE relocs testing setup") > Reported-by: Alexei Starovoitov <ast@kernel.org> > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > --- > .../selftests/bpf/prog_tests/core_reloc.c | 16 +++++++++++----- > .../selftests/bpf/progs/test_core_reloc_kernel.c | 4 ++++ > 2 files changed, 15 insertions(+), 5 deletions(-) > Looks good. Acked-by: John Fastabend <john.fastabend@gmail.com>
On Thu, Nov 21, 2019 at 9:59 AM Andrii Nakryiko <andriin@fb.com> wrote: > > test_core_reloc_kernel.c selftest is the only CO-RE test that reads and > returns for validation calling thread's information (pid, tgid, comm). Thus it > has to make sure that only test_prog's invocations are honored. > > Fixes: df36e621418b ("selftests/bpf: add CO-RE relocs testing setup") > Reported-by: Alexei Starovoitov <ast@kernel.org> > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > --- > .../selftests/bpf/prog_tests/core_reloc.c | 16 +++++++++++----- > .../selftests/bpf/progs/test_core_reloc_kernel.c | 4 ++++ > 2 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/core_reloc.c b/tools/testing/selftests/bpf/prog_tests/core_reloc.c > index ec9e2fdd6b89..05fe85281ff7 100644 > --- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c > +++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c > @@ -2,6 +2,7 @@ > #include <test_progs.h> > #include "progs/core_reloc_types.h" > #include <sys/mman.h> > +#include <sys/syscall.h> > > #define STRUCT_TO_CHAR_PTR(struct_name) (const char *)&(struct struct_name) > > @@ -452,6 +453,7 @@ static struct core_reloc_test_case test_cases[] = { > struct data { > char in[256]; > char out[256]; > + uint64_t my_pid_tgid; > }; > > static size_t roundup_page(size_t sz) > @@ -471,9 +473,12 @@ void test_core_reloc(void) > struct bpf_map *data_map; > struct bpf_program *prog; > struct bpf_object *obj; > + uint64_t my_pid_tgid; > struct data *data; > void *mmap_data = NULL; > > + my_pid_tgid = getpid() | ((uint64_t)syscall(SYS_gettid) << 32); > + > for (i = 0; i < ARRAY_SIZE(test_cases); i++) { > test_case = &test_cases[i]; > if (!test__start_subtest(test_case->case_name)) > @@ -517,11 +522,6 @@ void test_core_reloc(void) > goto cleanup; > } > > - link = bpf_program__attach_raw_tracepoint(prog, tp_name); > - if (CHECK(IS_ERR(link), "attach_raw_tp", "err %ld\n", > - PTR_ERR(link))) > - goto cleanup; > - > data_map = bpf_object__find_map_by_name(obj, "test_cor.bss"); > if (CHECK(!data_map, "find_data_map", "data map not found\n")) > goto cleanup; > @@ -537,6 +537,12 @@ void test_core_reloc(void) > > memset(mmap_data, 0, sizeof(*data)); > memcpy(data->in, test_case->input, test_case->input_len); > + data->my_pid_tgid = my_pid_tgid; > + > + link = bpf_program__attach_raw_tracepoint(prog, tp_name); > + if (CHECK(IS_ERR(link), "attach_raw_tp", "err %ld\n", > + PTR_ERR(link))) > + goto cleanup; > > /* trigger test run */ > usleep(1); > diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c > index a4b5e0562ed5..d2fe8f337846 100644 > --- a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c > +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c > @@ -11,6 +11,7 @@ char _license[] SEC("license") = "GPL"; > static volatile struct data { > char in[256]; > char out[256]; > + uint64_t my_pid_tgid; > } data; There was a conflict here, since global data support patchset was already applied. I resolved it and applied to bpf-next. Thanks
On Thu, Nov 21, 2019 at 11:27 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Nov 21, 2019 at 9:59 AM Andrii Nakryiko <andriin@fb.com> wrote: > > > > test_core_reloc_kernel.c selftest is the only CO-RE test that reads and > > returns for validation calling thread's information (pid, tgid, comm). Thus it > > has to make sure that only test_prog's invocations are honored. > > > > Fixes: df36e621418b ("selftests/bpf: add CO-RE relocs testing setup") > > Reported-by: Alexei Starovoitov <ast@kernel.org> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > > --- > > .../selftests/bpf/prog_tests/core_reloc.c | 16 +++++++++++----- > > .../selftests/bpf/progs/test_core_reloc_kernel.c | 4 ++++ > > 2 files changed, 15 insertions(+), 5 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/core_reloc.c b/tools/testing/selftests/bpf/prog_tests/core_reloc.c > > index ec9e2fdd6b89..05fe85281ff7 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c > > +++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c > > @@ -2,6 +2,7 @@ > > #include <test_progs.h> > > #include "progs/core_reloc_types.h" > > #include <sys/mman.h> > > +#include <sys/syscall.h> > > > > #define STRUCT_TO_CHAR_PTR(struct_name) (const char *)&(struct struct_name) > > > > @@ -452,6 +453,7 @@ static struct core_reloc_test_case test_cases[] = { > > struct data { > > char in[256]; > > char out[256]; > > + uint64_t my_pid_tgid; > > }; > > > > static size_t roundup_page(size_t sz) > > @@ -471,9 +473,12 @@ void test_core_reloc(void) > > struct bpf_map *data_map; > > struct bpf_program *prog; > > struct bpf_object *obj; > > + uint64_t my_pid_tgid; > > struct data *data; > > void *mmap_data = NULL; > > > > + my_pid_tgid = getpid() | ((uint64_t)syscall(SYS_gettid) << 32); > > + > > for (i = 0; i < ARRAY_SIZE(test_cases); i++) { > > test_case = &test_cases[i]; > > if (!test__start_subtest(test_case->case_name)) > > @@ -517,11 +522,6 @@ void test_core_reloc(void) > > goto cleanup; > > } > > > > - link = bpf_program__attach_raw_tracepoint(prog, tp_name); > > - if (CHECK(IS_ERR(link), "attach_raw_tp", "err %ld\n", > > - PTR_ERR(link))) > > - goto cleanup; > > - > > data_map = bpf_object__find_map_by_name(obj, "test_cor.bss"); > > if (CHECK(!data_map, "find_data_map", "data map not found\n")) > > goto cleanup; > > @@ -537,6 +537,12 @@ void test_core_reloc(void) > > > > memset(mmap_data, 0, sizeof(*data)); > > memcpy(data->in, test_case->input, test_case->input_len); > > + data->my_pid_tgid = my_pid_tgid; > > + > > + link = bpf_program__attach_raw_tracepoint(prog, tp_name); > > + if (CHECK(IS_ERR(link), "attach_raw_tp", "err %ld\n", > > + PTR_ERR(link))) > > + goto cleanup; > > > > /* trigger test run */ > > usleep(1); > > diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c > > index a4b5e0562ed5..d2fe8f337846 100644 > > --- a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c > > +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c > > @@ -11,6 +11,7 @@ char _license[] SEC("license") = "GPL"; > > static volatile struct data { > > char in[256]; > > char out[256]; > > + uint64_t my_pid_tgid; > > } data; > > There was a conflict here, since global data support patchset was > already applied. > I resolved it and applied to bpf-next. > Thanks Ah, right, thanks!
diff --git a/tools/testing/selftests/bpf/prog_tests/core_reloc.c b/tools/testing/selftests/bpf/prog_tests/core_reloc.c index ec9e2fdd6b89..05fe85281ff7 100644 --- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c +++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c @@ -2,6 +2,7 @@ #include <test_progs.h> #include "progs/core_reloc_types.h" #include <sys/mman.h> +#include <sys/syscall.h> #define STRUCT_TO_CHAR_PTR(struct_name) (const char *)&(struct struct_name) @@ -452,6 +453,7 @@ static struct core_reloc_test_case test_cases[] = { struct data { char in[256]; char out[256]; + uint64_t my_pid_tgid; }; static size_t roundup_page(size_t sz) @@ -471,9 +473,12 @@ void test_core_reloc(void) struct bpf_map *data_map; struct bpf_program *prog; struct bpf_object *obj; + uint64_t my_pid_tgid; struct data *data; void *mmap_data = NULL; + my_pid_tgid = getpid() | ((uint64_t)syscall(SYS_gettid) << 32); + for (i = 0; i < ARRAY_SIZE(test_cases); i++) { test_case = &test_cases[i]; if (!test__start_subtest(test_case->case_name)) @@ -517,11 +522,6 @@ void test_core_reloc(void) goto cleanup; } - link = bpf_program__attach_raw_tracepoint(prog, tp_name); - if (CHECK(IS_ERR(link), "attach_raw_tp", "err %ld\n", - PTR_ERR(link))) - goto cleanup; - data_map = bpf_object__find_map_by_name(obj, "test_cor.bss"); if (CHECK(!data_map, "find_data_map", "data map not found\n")) goto cleanup; @@ -537,6 +537,12 @@ void test_core_reloc(void) memset(mmap_data, 0, sizeof(*data)); memcpy(data->in, test_case->input, test_case->input_len); + data->my_pid_tgid = my_pid_tgid; + + link = bpf_program__attach_raw_tracepoint(prog, tp_name); + if (CHECK(IS_ERR(link), "attach_raw_tp", "err %ld\n", + PTR_ERR(link))) + goto cleanup; /* trigger test run */ usleep(1); diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c index a4b5e0562ed5..d2fe8f337846 100644 --- a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c @@ -11,6 +11,7 @@ char _license[] SEC("license") = "GPL"; static volatile struct data { char in[256]; char out[256]; + uint64_t my_pid_tgid; } data; struct core_reloc_kernel_output { @@ -38,6 +39,9 @@ int test_core_kernel(void *ctx) uint32_t real_tgid = (uint32_t)pid_tgid; int pid, tgid; + if (data.my_pid_tgid != pid_tgid) + return 0; + if (CORE_READ(&pid, &task->pid) || CORE_READ(&tgid, &task->tgid)) return 1;
test_core_reloc_kernel.c selftest is the only CO-RE test that reads and returns for validation calling thread's information (pid, tgid, comm). Thus it has to make sure that only test_prog's invocations are honored. Fixes: df36e621418b ("selftests/bpf: add CO-RE relocs testing setup") Reported-by: Alexei Starovoitov <ast@kernel.org> Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- .../selftests/bpf/prog_tests/core_reloc.c | 16 +++++++++++----- .../selftests/bpf/progs/test_core_reloc_kernel.c | 4 ++++ 2 files changed, 15 insertions(+), 5 deletions(-)