| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20201215204224.GA15407@localhost>
Date: Tue, 15 Dec 2020 17:42:25 -0300
From: Carlos Antonio Neira Bustos <cneirabustos@...il.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: Networking <netdev@...r.kernel.org>,
Andrii Nakryiko <andriin@...com>, Yonghong Song <yhs@...com>,
"Eric W. Biederman" <ebiederm@...ssion.com>,
Jesper Dangaard Brouer <brouer@...hat.com>,
bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH v8 bpf-next] bpf/selftests: fold
test_current_pid_tgid_new_ns into test_progs.
Andrii,
Thank you very much for checking this out!, you were right the test was
incorrect. I'll work on your feedback and resend.
Thanks!
On Tue, Dec 15, 2020 at 11:05:50AM -0800, Andrii Nakryiko wrote:
> On Mon, Dec 14, 2020 at 10:39 AM Carlos Neira <cneirabustos@...il.com> wrote:
> >
> > Currently tests for bpf_get_ns_current_pid_tgid() are outside test_progs.
> > This change folds test cases into test_progs.
> >
> > Changes from V7:
> > - Rebased changes.
> > - Changed function scope.
> >
> > Signed-off-by: Carlos Neira <cneirabustos@...il.com>
> > Acked-by: Andrii Nakryiko <andriin@...com>
>
> please drop my ack for the next version given the changes requested, thanks!
>
> > ---
> > tools/testing/selftests/bpf/.gitignore | 1 -
> > tools/testing/selftests/bpf/Makefile | 3 +-
> > .../bpf/prog_tests/ns_current_pid_tgid.c | 148 ++++++++++------
> > .../bpf/progs/test_ns_current_pid_tgid.c | 26 +--
> > .../bpf/test_current_pid_tgid_new_ns.c | 160 ------------------
> > 5 files changed, 105 insertions(+), 233 deletions(-)
> > delete mode 100644 tools/testing/selftests/bpf/test_current_pid_tgid_new_ns.c
> >
>
> [...]
>
> >
> > - obj = bpf_object__open_file(file, NULL);
> > - if (CHECK(IS_ERR(obj), "obj_open", "err %ld\n", PTR_ERR(obj)))
> > - return;
> > + skel = test_ns_current_pid_tgid__open_and_load();
> > + CHECK(!skel, "skel_open_load", "failed to load skeleton\n");
> > + goto cleanup;
>
> Are you sure your test is doing what you think it's doing? Try running
> `sudo ./test_progs -t ns_current_pid_tgid -v` and see if you get all
> the CHECK()s you expect.
>
> It's a long way of saying that you are missing `if ()` and just
> unconditionally clean up after opening and loading the skeleton.
>
> And if the skeleton is not open_and_load()'ed successfully, there is
> nothing to clean up, btw.
>
> >
> > - err = bpf_object__load(obj);
> > - if (CHECK(err, "obj_load", "err %d errno %d\n", err, errno))
> > - goto cleanup;
> > + tid = syscall(SYS_gettid);
> > + pid = getpid();
> > +
> > + id = ((__u64)tid << 32) | pid;
> >
> > - bss_map = bpf_object__find_map_by_name(obj, "test_ns_.bss");
> > - if (CHECK(!bss_map, "find_bss_map", "failed\n"))
> > + err = stat("/proc/self/ns/pid", &st);
> > + if (CHECK(err, "stat", "failed /proc/self/ns/pid: %d", err))
> > goto cleanup;
> >
> > - prog = bpf_object__find_program_by_title(obj, probe_name);
> > - if (CHECK(!prog, "find_prog", "prog '%s' not found\n",
> > - probe_name))
> > + bss = skel->bss;
> > + bss->dev = st.st_dev;
> > + bss->ino = st.st_ino;
> > + bss->user_pid_tgid = 0;
> > +
> > + err = test_ns_current_pid_tgid__attach(skel);
> > + if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
> > goto cleanup;
> >
> > - memset(&bss, 0, sizeof(bss));
> > - pid_t tid = syscall(SYS_gettid);
> > - pid_t pid = getpid();
> > + /* trigger tracepoint */
> > + usleep(1);
> >
> > - id = (__u64) tid << 32 | pid;
> > - bss.user_pid_tgid = id;
> > + CHECK(bss->user_pid_tgid != id, "pid/tgid", "got %llu != exp %llu\n",
> > + bss->user_pid_tgid, id);
> > +cleanup:
> > + test_ns_current_pid_tgid__destroy(skel);
> > +}
> >
> > - if (CHECK_FAIL(stat("/proc/self/ns/pid", &st))) {
> > - perror("Failed to stat /proc/self/ns/pid");
> > +static int newns_pidtgid(void *arg)
>
> nit: pid_tgid?
>
> > +{
> > + struct test_ns_current_pid_tgid__bss *bss;
> > + int pidns_fd = 0, err = 0, duration = 0;
> > + struct test_ns_current_pid_tgid *skel;
> > + pid_t pid, tid;
> > + struct stat st;
> > + __u64 id;
> > +
> > + skel = test_ns_current_pid_tgid__open_and_load();
> > + if (!skel) {
> > + perror("Failed to load skeleton");
>
> please use CHECK() or ASSERT_OK_PTR() instead of perror()
>
> > goto cleanup;
> > }
> >
> > - bss.dev = st.st_dev;
> > - bss.ino = st.st_ino;
> > + tid = syscall(SYS_gettid);
> > + pid = getpid();
> > + id = ((__u64) tid << 32) | pid;
> >
>
> [...]
>
> > +
> > +static void test_ns_current_pid_tgid_new_ns(void)
> > +{
> > + int wstatus, duration = 0;
> > + pid_t cpid;
> > +
> > + cpid = clone(newns_pidtgid,
> > + child_stack + STACK_SIZE,
> > + CLONE_NEWPID | SIGCHLD, NULL);
>
> formatting here and below for wrapped arguments looks wrong. Please
> double-check whitespaces and align arguments. There is also
> `scripts/checkpatch.pl -f <path-to-file>` which will help.
>
> > +
> > + if (CHECK(cpid == -1, "clone", strerror(errno)))
> > + exit(EXIT_FAILURE);
> > +
> > + if (CHECK(waitpid(cpid, &wstatus, 0) == -1, "waitpid",
> > + strerror(errno))) {
> > + exit(EXIT_FAILURE);
> > + }
> > +
> > + CHECK(WEXITSTATUS(wstatus) != 0, "newns_pidtgid",
> > + "failed");
> > +}
> > +
> > +void test_ns_current_pid_tgid(void)
> > +{
> > + if (test__start_subtest("ns_current_pid_tgid_global_ns"))
> > + test_ns_current_pid_tgid_global_ns();
> > + if (test__start_subtest("ns_current_pid_tgid_new_ns"))
> > + test_ns_current_pid_tgid_new_ns();
> > }
> > 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
> > index 1dca70a6de2f..0daa12db0d83 100644
> > --- a/tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c
> > +++ b/tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c
> > @@ -5,31 +5,19 @@
> > #include <stdint.h>
> > #include <bpf/bpf_helpers.h>
> >
> > -static volatile struct {
> > - __u64 dev;
> > - __u64 ino;
> > - __u64 pid_tgid;
> > - __u64 user_pid_tgid;
> > -} res;
> > +__u64 user_pid_tgid = 0;
>
> imo, no need to combine pid and tgid into a single u64, why not using
> two separate global variables and keep it simple?
>
> > +__u64 dev = 0;
> > +__u64 ino = 0;
> >
> > SEC("raw_tracepoint/sys_enter")
> > -int trace(void *ctx)
> > +int handler(const 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)))
> > + if (bpf_get_ns_current_pid_tgid(dev, ino, &nsdata,
> > + sizeof(struct bpf_pidns_info)))
>
> here as well, wrapped argument looks misaligned (unless it's my email client)
>
> > return 0;
> > -
> > - ns_pid_tgid = (__u64)nsdata.tgid << 32 | nsdata.pid;
> > - expected_pid = res.user_pid_tgid;
> > -
>
> [...]
Powered by blists - more mailing lists