From: Yonghong Song <yhs@fb.com>
To: Carlos Neira <cneirabustos@gmail.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Cc: "ebiederm@xmission.com" <ebiederm@xmission.com>,
"brouer@redhat.com" <brouer@redhat.com>,
"bpf@vger.kernel.org" <bpf@vger.kernel.org>
Subject: Re: [PATCH v15 4/5] tools/testing/selftests/bpf: Add self-tests for new helper.
Date: Wed, 23 Oct 2019 03:02:51 +0000 [thread overview]
Message-ID: <e3138bca-14b1-6fcf-12be-992462abe0ce@fb.com> (raw)
In-Reply-To: <20191022191751.3780-5-cneirabustos@gmail.com>
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@gmail.com>
LGTM Ack with a few nits below.
Acked-by: Yonghong Song <yhs@fb.com>
> ---
> .../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@gmail.com */
> +#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@gmail.com */
> +
> +#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.
next prev parent reply other threads:[~2019-10-23 3:03 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-22 19:17 [PATCH v15 0/5] BPF: New helper to obtain namespace data from current task Carlos Neira
2019-10-22 19:17 ` [PATCH v15 1/5] fs/nsfs.c: added ns_match Carlos Neira
2019-10-23 3:05 ` Yonghong Song
2019-10-28 15:34 ` Yonghong Song
2019-10-31 22:31 ` [Review Request] " Yonghong Song
2019-11-12 15:18 ` Yonghong Song
2019-11-25 14:03 ` Carlos Antonio Neira Bustos
2019-10-22 19:17 ` [PATCH v15 2/5] bpf: added new helper bpf_get_ns_current_pid_tgid Carlos Neira
2019-10-23 2:51 ` Yonghong Song
2019-10-22 19:17 ` [PATCH v15 3/5] tools: Added bpf_get_ns_current_pid_tgid helper Carlos Neira
2019-10-23 2:51 ` Yonghong Song
2019-10-22 19:17 ` [PATCH v15 4/5] tools/testing/selftests/bpf: Add self-tests for new helper Carlos Neira
2019-10-23 3:02 ` Yonghong Song [this message]
2019-10-23 14:42 ` Carlos Antonio Neira Bustos
2019-10-23 15:20 ` Yonghong Song
2019-10-22 19:17 ` [PATCH v15 5/5] bpf_helpers_doc.py: Add struct bpf_pidns_info to known types Carlos Neira
2019-10-23 3:03 ` Yonghong Song
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e3138bca-14b1-6fcf-12be-992462abe0ce@fb.com \
--to=yhs@fb.com \
--cc=bpf@vger.kernel.org \
--cc=brouer@redhat.com \
--cc=cneirabustos@gmail.com \
--cc=ebiederm@xmission.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).