Re: [PATCH v15 4/5] tools/testing/selftests/bpf: Add self-tests for new helper.

[Carlos Antonio Neira Bustos <cneirabustos@gmail.com>]

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@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.

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