| Message ID | 20191017150032.14359-5-cneirabustos@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | BPF: New helper to obtain namespace data from current task | expand |
On 10/17/19 8:00 AM, Carlos Neira wrote: > Self tests added for new helper > > Signed-off-by: Carlos Neira <cneirabustos@gmail.com> > --- > .../bpf/prog_tests/get_ns_current_pid_tgid.c | 96 +++++++++++++++++++ > .../bpf/progs/get_ns_current_pid_tgid_kern.c | 53 ++++++++++ > 2 files changed, 149 insertions(+) > create mode 100644 tools/testing/selftests/bpf/prog_tests/get_ns_current_pid_tgid.c > create mode 100644 tools/testing/selftests/bpf/progs/get_ns_current_pid_tgid_kern.c > > diff --git a/tools/testing/selftests/bpf/prog_tests/get_ns_current_pid_tgid.c b/tools/testing/selftests/bpf/prog_tests/get_ns_current_pid_tgid.c > new file mode 100644 > index 000000000000..48d9785f89d0 > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/get_ns_current_pid_tgid.c > @@ -0,0 +1,96 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (c) 2019 Carlos Neira cneirabustos@gmail.com */ > +#include <test_progs.h> > +#include <sys/stat.h> > +#include <sys/types.h> > +#include <sys/stat.h> > +#include <unistd.h> > +#include <sys/syscall.h> > + > +struct bss { > + __u64 dev; > + __u64 ino; > + __u64 pidtgid; Maybe pid_tgid? > + __u64 userpidtgid; Maybe user_pid_tgid? > +} data; > + > +void test_get_ns_current_pid_tgid(void) > +{ > + const char *probe_name = "raw_tracepoint/sys_enter"; > + const char *file = "get_ns_current_pid_tgid_kern.o"; > + struct bpf_object_load_attr load_attr = {}; > + struct bpf_link *link = NULL; > + struct bpf_program *prog; > + struct bpf_map *bss_map; > + struct bpf_object *obj; > + int err, duration = 0; > + const __u32 key = 0; > + struct stat st; > + __u64 id; > + > + obj = bpf_object__open(file); > + if (CHECK(IS_ERR_OR_NULL(obj), "obj_open", > + "failed to open '%s': %ld\n", > + file, PTR_ERR(obj))) > + goto cleanup; > + > + prog = bpf_object__find_program_by_title(obj, probe_name); > + if (CHECK(!prog, "find_probe", > + "prog '%s' not found\n", probe_name)) > + goto cleanup; > + > + bpf_program__set_type(prog, BPF_PROG_TYPE_RAW_TRACEPOINT); Do we need this? I thought libbpf should automatically infer program type from section name? > + > + load_attr.obj = obj; > + load_attr.log_level = 0; > + load_attr.target_btf_path = NULL; > + err = bpf_object__load_xattr(&load_attr); > + if (CHECK(err, "obj_load", > + "failed to load prog '%s': %d\n", > + probe_name, err)) > + goto cleanup; Your load_attr only has 'obj', you could use bpf_object__load for simplicity. > + > + link = bpf_program__attach_raw_tracepoint(prog, "sys_enter"); > + if (CHECK(IS_ERR(link), "attach_raw_tp", "err %ld\n", > + PTR_ERR(link))) > + goto cleanup; > + > + bss_map = bpf_object__find_map_by_name(obj, "ns_data_map"); > + if (CHECK(!bss_map, "find_bss_map", "failed\n")) > + goto cleanup; > + > + memset(&data, 0, sizeof(data)); > + pid_t tid = syscall(SYS_gettid); > + pid_t pid = getpid(); > + > + id = (__u64) tid << 32 | pid; > + data.userpidtgid = id; > + > + if (CHECK(stat("/proc/self/ns/pid", &st), "stat","failed\n")) > + goto cleanup; > + > + data.dev = st.st_dev; > + data.ino = st.st_ino; > + > + err = bpf_map_update_elem(bpf_map__fd(bss_map), &key, &data, 0); > + if (CHECK(err, "setting_bss", "failed to set bss data: %d\n", err)) > + goto cleanup; Typically, we would like to do map_update_elem first and then do attach_raw_tracepoint. This will ensure updated elem is seen even for the first invocation of the program. In your case, since you ignore all unmatched version, so I won't insist if there is no revision needed. Since you need respin any way, I suggest to switch the order between bpf_map_update_elem and attach_raw_tracepoint, which is a good practice any way. > + > + /* trigger some syscalls */ > + usleep(1); > + > + err = bpf_map_lookup_elem(bpf_map__fd(bss_map), &key, &data); > + if (CHECK(err, "set_bss", "failed to get bss data: %d\n", err)) > + goto cleanup; > + > + if (CHECK(id != data.pidtgid, "Compare user pid/tgid vs. bpf pid/tgid", > + "User pid/tgid %llu EBPF pid/tgid %llu\n", id, data.pidtgid)) > + goto cleanup; > +cleanup: > + > + if (!IS_ERR_OR_NULL(link)) { > + bpf_link__destroy(link); > + link = NULL; > + } > + bpf_object__close(obj); > +} > diff --git a/tools/testing/selftests/bpf/progs/get_ns_current_pid_tgid_kern.c b/tools/testing/selftests/bpf/progs/get_ns_current_pid_tgid_kern.c > new file mode 100644 > index 000000000000..1fd847b63105 > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/get_ns_current_pid_tgid_kern.c > @@ -0,0 +1,53 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (c) 2019 Carlos Neira cneirabustos@gmail.com */ > + > +#include <linux/bpf.h> > +#include <stdint.h> > +#include "bpf_helpers.h" > +#include "bpf_core_read.h" You do not need "bpf_core_read.h" here. > + > +struct res { > + __u64 dev; > + __u64 ino; > + __u64 pidtgid; > + __u64 userpidtgid; > +}; > + > +struct { > + __uint(type, BPF_MAP_TYPE_ARRAY); > + __uint(max_entries, 1); > + __type(key, __u32); > + __type(value, struct res); > +} ns_data_map SEC(".maps"); In you code, you use ns_data_map which has max_entries = 1. In this case, static volatile variable can be used to simplify the bpf program. You have 'static struct res data' which will turn into a map as well. So now you have two maps to hold the nsdata. I suggest to remove the above ns_data_map. You can take a look at the below commit for an example: commit 666b2c10ee9d51f14d04c416a14b1cb6fd0846e4 Author: Andrii Nakryiko <andriin@fb.com> Date: Wed Oct 9 13:14:58 2019 -0700 selftests/bpf: Add read-only map values propagation tests Add tests checking that verifier does proper constant propagation for read-only maps. If constant propagation didn't work, skipp_loop and part_loop BPF programs would be rejected due to BPF verifier otherwise not being able to prove they ever complete. With constant propagation, though, they are succesfully validated as properly terminating loops. > + > +static struct res data; > + > +SEC("raw_tracepoint/sys_enter") > +int trace(void *ctx) > +{ > + __u64 nspidtgid, expected_pid; > + struct bpf_pidns_info nsdata; > + const __u32 key = 0; You can remove "const" here, not very useful. > + struct res *pres; > + > + pres = bpf_map_lookup_elem(&ns_data_map, &key); > + if (!pres) > + return 0; > + > + if (bpf_get_ns_current_pid_tgid(pres->dev, pres->ino, &nsdata, > + sizeof(struct bpf_pidns_info))) > + return 0; > + > + nspidtgid = (__u64)nsdata.tgid << 32 | nsdata.pid; > + expected_pid = pres->userpidtgid; > + > + if (expected_pid != nspidtgid) > + return 0; > + > + data.pidtgid = nspidtgid; > + bpf_map_update_elem(&ns_data_map, &key, &data, 0); > + > + return 0; > +} > + > +char _license[] SEC("license") = "GPL"; >
On Sat, Oct 19, 2019 at 1:58 AM Yonghong Song <yhs@fb.com> wrote: > > > > On 10/17/19 8:00 AM, Carlos Neira wrote: > > Self tests added for new helper > > > > Signed-off-by: Carlos Neira <cneirabustos@gmail.com> > > --- > > .../bpf/prog_tests/get_ns_current_pid_tgid.c | 96 +++++++++++++++++++ > > .../bpf/progs/get_ns_current_pid_tgid_kern.c | 53 ++++++++++ It looks like typical naming convention is: prog_test/<something>.c progs/test_<something>.c Let's keep this consistent. I'm about to do a bit smarter Makefile that will capture this convention, so it's good to have less exception to create. Thanks! Otherwise, besides what Yonghong mentioned, this look good to me. > > 2 files changed, 149 insertions(+) > > create mode 100644 tools/testing/selftests/bpf/prog_tests/get_ns_current_pid_tgid.c > > create mode 100644 tools/testing/selftests/bpf/progs/get_ns_current_pid_tgid_kern.c > > [...] > > + prog = bpf_object__find_program_by_title(obj, probe_name); > > + if (CHECK(!prog, "find_probe", > > + "prog '%s' not found\n", probe_name)) > > + goto cleanup; > > + > > + bpf_program__set_type(prog, BPF_PROG_TYPE_RAW_TRACEPOINT); > > Do we need this? I thought libbpf should automatically > infer program type from section name? We used to, until the patch set that Daniel landed today. Now it can be dropped. > > > + > > + load_attr.obj = obj; > > + load_attr.log_level = 0; > > + load_attr.target_btf_path = NULL; > > + err = bpf_object__load_xattr(&load_attr); > > + if (CHECK(err, "obj_load", > > + "failed to load prog '%s': %d\n", > > + probe_name, err)) > > + goto cleanup; > [...]
On Mon, Oct 21, 2019 at 11:20:01AM -0700, Andrii Nakryiko wrote: > On Sat, Oct 19, 2019 at 1:58 AM Yonghong Song <yhs@fb.com> wrote: > > > > > > > > On 10/17/19 8:00 AM, Carlos Neira wrote: > > > Self tests added for new helper > > > > > > Signed-off-by: Carlos Neira <cneirabustos@gmail.com> > > > --- > > > .../bpf/prog_tests/get_ns_current_pid_tgid.c | 96 +++++++++++++++++++ > > > .../bpf/progs/get_ns_current_pid_tgid_kern.c | 53 ++++++++++ > > It looks like typical naming convention is: > > prog_test/<something>.c > progs/test_<something>.c > > Let's keep this consistent. I'm about to do a bit smarter Makefile > that will capture this convention, so it's good to have less exception > to create. Thanks! > > Otherwise, besides what Yonghong mentioned, this look good to me. > > > > > 2 files changed, 149 insertions(+) > > > create mode 100644 tools/testing/selftests/bpf/prog_tests/get_ns_current_pid_tgid.c > > > create mode 100644 tools/testing/selftests/bpf/progs/get_ns_current_pid_tgid_kern.c > > > > > [...] > > > > + prog = bpf_object__find_program_by_title(obj, probe_name); > > > + if (CHECK(!prog, "find_probe", > > > + "prog '%s' not found\n", probe_name)) > > > + goto cleanup; > > > + > > > + bpf_program__set_type(prog, BPF_PROG_TYPE_RAW_TRACEPOINT); > > > > Do we need this? I thought libbpf should automatically > > infer program type from section name? > > We used to, until the patch set that Daniel landed today. Now it can be dropped. > > > > > > + > > > + load_attr.obj = obj; > > > + load_attr.log_level = 0; > > > + load_attr.target_btf_path = NULL; > > > + err = bpf_object__load_xattr(&load_attr); > > > + if (CHECK(err, "obj_load", > > > + "failed to load prog '%s': %d\n", > > > + probe_name, err)) > > > + goto cleanup; > > > > [...] Thanks Andrii, I have a doubt, I don't find in prog_tests/rdonly_map.c where is "test_rdo.bss" defined ?, is called in line 43 but I'm missing how to is it used as I don't see it defined. Bests
On Mon, Oct 21, 2019 at 12:14 PM Carlos Antonio Neira Bustos <cneirabustos@gmail.com> wrote: > > On Mon, Oct 21, 2019 at 11:20:01AM -0700, Andrii Nakryiko wrote: > > On Sat, Oct 19, 2019 at 1:58 AM Yonghong Song <yhs@fb.com> wrote: > > > > > > > > > > > > On 10/17/19 8:00 AM, Carlos Neira wrote: > > > > Self tests added for new helper > > > > > > > > Signed-off-by: Carlos Neira <cneirabustos@gmail.com> > > > > --- > > > > .../bpf/prog_tests/get_ns_current_pid_tgid.c | 96 +++++++++++++++++++ > > > > .../bpf/progs/get_ns_current_pid_tgid_kern.c | 53 ++++++++++ > > > > It looks like typical naming convention is: > > > > prog_test/<something>.c > > progs/test_<something>.c > > > > Let's keep this consistent. I'm about to do a bit smarter Makefile > > that will capture this convention, so it's good to have less exception > > to create. Thanks! > > > > Otherwise, besides what Yonghong mentioned, this look good to me. > > > > > > > > 2 files changed, 149 insertions(+) > > > > create mode 100644 tools/testing/selftests/bpf/prog_tests/get_ns_current_pid_tgid.c > > > > create mode 100644 tools/testing/selftests/bpf/progs/get_ns_current_pid_tgid_kern.c > > > > > > > > [...] > > > > > > + prog = bpf_object__find_program_by_title(obj, probe_name); > > > > + if (CHECK(!prog, "find_probe", > > > > + "prog '%s' not found\n", probe_name)) > > > > + goto cleanup; > > > > + > > > > + bpf_program__set_type(prog, BPF_PROG_TYPE_RAW_TRACEPOINT); > > > > > > Do we need this? I thought libbpf should automatically > > > infer program type from section name? > > > > We used to, until the patch set that Daniel landed today. Now it can be dropped. > > > > > > > > > + > > > > + load_attr.obj = obj; > > > > + load_attr.log_level = 0; > > > > + load_attr.target_btf_path = NULL; > > > > + err = bpf_object__load_xattr(&load_attr); > > > > + if (CHECK(err, "obj_load", > > > > + "failed to load prog '%s': %d\n", > > > > + probe_name, err)) > > > > + goto cleanup; > > > > > > > [...] > > Thanks Andrii, > I have a doubt, I don't find in prog_tests/rdonly_map.c where is "test_rdo.bss" defined ?, is called in line 43 but I'm missing how to is it used as I don't see it defined. > This map is created by libbpf implicitly from global variables used by BPF object. You just look it up by name, set its value to whatever you need global variables to be set up to, and that value will be available to BPF program. From BPF program side, when you update global variable, that value can be read from user space using that same test_rdo.bss map. Does it make sense? > Bests
On Mon, Oct 21, 2019 at 12:18:33PM -0700, Andrii Nakryiko wrote: > On Mon, Oct 21, 2019 at 12:14 PM Carlos Antonio Neira Bustos > <cneirabustos@gmail.com> wrote: > > > > On Mon, Oct 21, 2019 at 11:20:01AM -0700, Andrii Nakryiko wrote: > > > On Sat, Oct 19, 2019 at 1:58 AM Yonghong Song <yhs@fb.com> wrote: > > > > > > > > > > > > > > > > On 10/17/19 8:00 AM, Carlos Neira wrote: > > > > > Self tests added for new helper > > > > > > > > > > Signed-off-by: Carlos Neira <cneirabustos@gmail.com> > > > > > --- > > > > > .../bpf/prog_tests/get_ns_current_pid_tgid.c | 96 +++++++++++++++++++ > > > > > .../bpf/progs/get_ns_current_pid_tgid_kern.c | 53 ++++++++++ > > > > > > It looks like typical naming convention is: > > > > > > prog_test/<something>.c > > > progs/test_<something>.c > > > > > > Let's keep this consistent. I'm about to do a bit smarter Makefile > > > that will capture this convention, so it's good to have less exception > > > to create. Thanks! > > > > > > Otherwise, besides what Yonghong mentioned, this look good to me. > > > > > > > > > > > 2 files changed, 149 insertions(+) > > > > > create mode 100644 tools/testing/selftests/bpf/prog_tests/get_ns_current_pid_tgid.c > > > > > create mode 100644 tools/testing/selftests/bpf/progs/get_ns_current_pid_tgid_kern.c > > > > > > > > > > > [...] > > > > > > > > + prog = bpf_object__find_program_by_title(obj, probe_name); > > > > > + if (CHECK(!prog, "find_probe", > > > > > + "prog '%s' not found\n", probe_name)) > > > > > + goto cleanup; > > > > > + > > > > > + bpf_program__set_type(prog, BPF_PROG_TYPE_RAW_TRACEPOINT); > > > > > > > > Do we need this? I thought libbpf should automatically > > > > infer program type from section name? > > > > > > We used to, until the patch set that Daniel landed today. Now it can be dropped. > > > > > > > > > > > > + > > > > > + load_attr.obj = obj; > > > > > + load_attr.log_level = 0; > > > > > + load_attr.target_btf_path = NULL; > > > > > + err = bpf_object__load_xattr(&load_attr); > > > > > + if (CHECK(err, "obj_load", > > > > > + "failed to load prog '%s': %d\n", > > > > > + probe_name, err)) > > > > > + goto cleanup; > > > > > > > > > > [...] > > > > Thanks Andrii, > > I have a doubt, I don't find in prog_tests/rdonly_map.c where is "test_rdo.bss" defined ?, is called in line 43 but I'm missing how to is it used as I don't see it defined. > > > > This map is created by libbpf implicitly from global variables used by > BPF object. You just look it up by name, set its value to whatever you > need global variables to be set up to, and that value will be > available to BPF program. From BPF program side, when you update > global variable, that value can be read from user space using that > same test_rdo.bss map. Does it make sense? > > > Bests Thanks for the explanation Andrii, now it works!.
diff --git a/tools/testing/selftests/bpf/prog_tests/get_ns_current_pid_tgid.c b/tools/testing/selftests/bpf/prog_tests/get_ns_current_pid_tgid.c new file mode 100644 index 000000000000..48d9785f89d0 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/get_ns_current_pid_tgid.c @@ -0,0 +1,96 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2019 Carlos Neira cneirabustos@gmail.com */ +#include <test_progs.h> +#include <sys/stat.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <unistd.h> +#include <sys/syscall.h> + +struct bss { + __u64 dev; + __u64 ino; + __u64 pidtgid; + __u64 userpidtgid; +} data; + +void test_get_ns_current_pid_tgid(void) +{ + const char *probe_name = "raw_tracepoint/sys_enter"; + const char *file = "get_ns_current_pid_tgid_kern.o"; + struct bpf_object_load_attr load_attr = {}; + struct bpf_link *link = NULL; + struct bpf_program *prog; + struct bpf_map *bss_map; + struct bpf_object *obj; + int err, duration = 0; + const __u32 key = 0; + struct stat st; + __u64 id; + + obj = bpf_object__open(file); + if (CHECK(IS_ERR_OR_NULL(obj), "obj_open", + "failed to open '%s': %ld\n", + file, PTR_ERR(obj))) + goto cleanup; + + prog = bpf_object__find_program_by_title(obj, probe_name); + if (CHECK(!prog, "find_probe", + "prog '%s' not found\n", probe_name)) + goto cleanup; + + bpf_program__set_type(prog, BPF_PROG_TYPE_RAW_TRACEPOINT); + + load_attr.obj = obj; + load_attr.log_level = 0; + load_attr.target_btf_path = NULL; + err = bpf_object__load_xattr(&load_attr); + if (CHECK(err, "obj_load", + "failed to load prog '%s': %d\n", + probe_name, err)) + goto cleanup; + + link = bpf_program__attach_raw_tracepoint(prog, "sys_enter"); + if (CHECK(IS_ERR(link), "attach_raw_tp", "err %ld\n", + PTR_ERR(link))) + goto cleanup; + + bss_map = bpf_object__find_map_by_name(obj, "ns_data_map"); + if (CHECK(!bss_map, "find_bss_map", "failed\n")) + goto cleanup; + + memset(&data, 0, sizeof(data)); + pid_t tid = syscall(SYS_gettid); + pid_t pid = getpid(); + + id = (__u64) tid << 32 | pid; + data.userpidtgid = id; + + if (CHECK(stat("/proc/self/ns/pid", &st), "stat","failed\n")) + goto cleanup; + + data.dev = st.st_dev; + data.ino = st.st_ino; + + err = bpf_map_update_elem(bpf_map__fd(bss_map), &key, &data, 0); + if (CHECK(err, "setting_bss", "failed to set bss data: %d\n", err)) + goto cleanup; + + /* trigger some syscalls */ + usleep(1); + + err = bpf_map_lookup_elem(bpf_map__fd(bss_map), &key, &data); + if (CHECK(err, "set_bss", "failed to get bss data: %d\n", err)) + goto cleanup; + + if (CHECK(id != data.pidtgid, "Compare user pid/tgid vs. bpf pid/tgid", + "User pid/tgid %llu EBPF pid/tgid %llu\n", id, data.pidtgid)) + goto cleanup; +cleanup: + + if (!IS_ERR_OR_NULL(link)) { + bpf_link__destroy(link); + link = NULL; + } + bpf_object__close(obj); +} diff --git a/tools/testing/selftests/bpf/progs/get_ns_current_pid_tgid_kern.c b/tools/testing/selftests/bpf/progs/get_ns_current_pid_tgid_kern.c new file mode 100644 index 000000000000..1fd847b63105 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/get_ns_current_pid_tgid_kern.c @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2019 Carlos Neira cneirabustos@gmail.com */ + +#include <linux/bpf.h> +#include <stdint.h> +#include "bpf_helpers.h" +#include "bpf_core_read.h" + +struct res { + __u64 dev; + __u64 ino; + __u64 pidtgid; + __u64 userpidtgid; +}; + +struct { + __uint(type, BPF_MAP_TYPE_ARRAY); + __uint(max_entries, 1); + __type(key, __u32); + __type(value, struct res); +} ns_data_map SEC(".maps"); + +static struct res data; + +SEC("raw_tracepoint/sys_enter") +int trace(void *ctx) +{ + __u64 nspidtgid, expected_pid; + struct bpf_pidns_info nsdata; + const __u32 key = 0; + struct res *pres; + + pres = bpf_map_lookup_elem(&ns_data_map, &key); + if (!pres) + return 0; + + if (bpf_get_ns_current_pid_tgid(pres->dev, pres->ino, &nsdata, + sizeof(struct bpf_pidns_info))) + return 0; + + nspidtgid = (__u64)nsdata.tgid << 32 | nsdata.pid; + expected_pid = pres->userpidtgid; + + if (expected_pid != nspidtgid) + return 0; + + data.pidtgid = nspidtgid; + bpf_map_update_elem(&ns_data_map, &key, &data, 0); + + return 0; +} + +char _license[] SEC("license") = "GPL";
Self tests added for new helper Signed-off-by: Carlos Neira <cneirabustos@gmail.com> --- .../bpf/prog_tests/get_ns_current_pid_tgid.c | 96 +++++++++++++++++++ .../bpf/progs/get_ns_current_pid_tgid_kern.c | 53 ++++++++++ 2 files changed, 149 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/get_ns_current_pid_tgid.c create mode 100644 tools/testing/selftests/bpf/progs/get_ns_current_pid_tgid_kern.c