On Wed, Oct 23, 2019 at 03:02:51AM +0000, Yonghong Song wrote:
>
>
> 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.
Thanks, Yonghong.
Do I need to re-send the series of patches as v16 ? or I could reply to this thread addressing your comments for patch 4/5.
Thanks again for your support.
Bests
