On 10/22/19 12:17 PM, Carlos Neira wrote:
> Self tests added for new helper
Please mention the name of the new helper in the commit message.
>
> Signed-off-by: Carlos Neira <cneirabustos@xxxxxxxxx>
LGTM Ack with a few nits below.
Acked-by: Yonghong Song <yhs@xxxxxx>
> ---
> .../bpf/prog_tests/ns_current_pid_tgid.c | 87 +++++++++++++++++++
> .../bpf/progs/test_ns_current_pid_tgid.c | 37 ++++++++
> 2 files changed, 124 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c b/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c
> new file mode 100644
> index 000000000000..257f18999bb6
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c
> @@ -0,0 +1,87 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2019 Carlos Neira cneirabustos@xxxxxxxxx */
> +#include <test_progs.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include <sys/syscall.h>
> +
> +struct bss {
> + __u64 dev;
> + __u64 ino;
> + __u64 pid_tgid;
> + __u64 user_pid_tgid;
> +};
> +
> +void test_ns_current_pid_tgid(void)
> +{
> + const char *probe_name = "raw_tracepoint/sys_enter";
> + const char *file = "test_ns_current_pid_tgid.o";
> + int err, key = 0, duration = 0;
> + struct bpf_link *link = NULL;
> + struct bpf_program *prog;
> + struct bpf_map *bss_map;
> + struct bpf_object *obj;
> + struct bss bss;
> + struct stat st;
> + __u64 id;
> +
> + 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 cleanup;
> +
> + bss_map = bpf_object__find_map_by_name(obj, "test_ns_.bss");
> + if (CHECK(!bss_map, "find_bss_map", "failed\n"))
> + goto cleanup;
> +
> + prog = bpf_object__find_program_by_title(obj, probe_name);
> + if (CHECK(!prog, "find_prog", "prog '%s' not found\n",
> + probe_name))
> + goto cleanup;
> +
> + memset(&bss, 0, sizeof(bss));
> + pid_t tid = syscall(SYS_gettid);
> + pid_t pid = getpid();
> +
> + id = (__u64) tid << 32 | pid;
> + bss.user_pid_tgid = id;
> +
> + if (CHECK_FAIL(stat("/proc/self/ns/pid", &st))) {
> + perror("Failed to stat /proc/self/ns/pid");
> + goto cleanup;
> + }
> +
> + bss.dev = st.st_dev;
> + bss.ino = st.st_ino;
> +
> + err = bpf_map_update_elem(bpf_map__fd(bss_map), &key, &bss, 0);
> + if (CHECK(err, "setting_bss", "failed to set bss : %d\n", 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;
You already have default link = NULL.
Here, I think you can do
link = NULL;
goto cleanup;
> +
> + /* trigger some syscalls */
> + usleep(1);
> +
> + err = bpf_map_lookup_elem(bpf_map__fd(bss_map), &key, &bss);
> + if (CHECK(err, "set_bss", "failed to get bss : %d\n", err))
> + goto cleanup;
> +
> + if (CHECK(id != bss.pid_tgid, "Compare user pid/tgid vs. bpf pid/tgid",
> + "User pid/tgid %llu EBPF pid/tgid %llu\n", id, bss.pid_tgid))
EBPF -> BPF?
> + goto cleanup;
> +cleanup:
> +
The above empty line can be removed.
> + if (!IS_ERR_OR_NULL(link)) {
With the above suggested change, you only need to check
if (!link)
> + bpf_link__destroy(link);
> + link = NULL;
> + }
> + bpf_object__close(obj);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c b/tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c
> new file mode 100644
> index 000000000000..cdb77eb1a4fb
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c
> @@ -0,0 +1,37 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2019 Carlos Neira cneirabustos@xxxxxxxxx */
> +
> +#include <linux/bpf.h>
> +#include <stdint.h>
> +#include "bpf_helpers.h"
> +
> +static volatile struct {
> + __u64 dev;
> + __u64 ino;
> + __u64 pid_tgid;
> + __u64 user_pid_tgid;
> +} res;
> +
> +SEC("raw_tracepoint/sys_enter")
> +int trace(void *ctx)
> +{
> + __u64 ns_pid_tgid, expected_pid;
> + struct bpf_pidns_info nsdata;
> + __u32 key = 0;
> +
> + if (bpf_get_ns_current_pid_tgid(res.dev, res.ino, &nsdata,
> + sizeof(struct bpf_pidns_info)))
> + return 0;
> +
> + ns_pid_tgid = (__u64)nsdata.tgid << 32 | nsdata.pid;
> + expected_pid = res.user_pid_tgid;
> +
> + if (expected_pid != ns_pid_tgid)
> + return 0;
> +
> + res.pid_tgid = ns_pid_tgid;
> +
> + return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
The new helper does not require GPL, could you double check this?
The above _license should not be necessary.
