Date:   Fri, 18 Oct 2019 17:33:15 +0000
From:   Yonghong Song <yhs@...com>
To:     Carlos Neira <cneirabustos@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "ebiederm@...ssion.com" <ebiederm@...ssion.com>,
        "brouer@...hat.com" <brouer@...hat.com>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>
Subject: Re: [PATCH v14 4/5] tools/testing/selftests/bpf: Add self-tests for
 new helper.



On 10/17/19 8:00 AM, Carlos Neira wrote:
> Self tests added for new helper
> 
> Signed-off-by: Carlos Neira <cneirabustos@...il.com>
> ---
>   .../bpf/prog_tests/get_ns_current_pid_tgid.c  | 96 +++++++++++++++++++
>   .../bpf/progs/get_ns_current_pid_tgid_kern.c  | 53 ++++++++++
>   2 files changed, 149 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/get_ns_current_pid_tgid.c
>   create mode 100644 tools/testing/selftests/bpf/progs/get_ns_current_pid_tgid_kern.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/get_ns_current_pid_tgid.c b/tools/testing/selftests/bpf/prog_tests/get_ns_current_pid_tgid.c
> new file mode 100644
> index 000000000000..48d9785f89d0
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/get_ns_current_pid_tgid.c
> @@ -0,0 +1,96 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2019 Carlos Neira cneirabustos@...il.com */
> +#include <test_progs.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +#include <sys/syscall.h>
> +
> +struct bss {
> +	__u64 dev;
> +	__u64 ino;
> +	__u64 pidtgid;
Maybe pid_tgid?

> +	__u64 userpidtgid;

Maybe user_pid_tgid?

> +} data;
> +
> +void test_get_ns_current_pid_tgid(void)
> +{
> +	const char *probe_name = "raw_tracepoint/sys_enter";
> +	const char *file = "get_ns_current_pid_tgid_kern.o";
> +	struct bpf_object_load_attr load_attr = {};
> +	struct bpf_link *link = NULL;
> +	struct bpf_program *prog;
> +	struct bpf_map *bss_map;
> +	struct bpf_object *obj;
> +	int err, duration = 0;
> +	const __u32 key = 0;
> +	struct stat st;
> +	__u64 id;
> +
> +	obj = bpf_object__open(file);
> +	if (CHECK(IS_ERR_OR_NULL(obj), "obj_open",
> +		  "failed to open '%s': %ld\n",
> +		  file, PTR_ERR(obj)))
> +		goto cleanup;
> +
> +	prog = bpf_object__find_program_by_title(obj, probe_name);
> +	if (CHECK(!prog, "find_probe",
> +		  "prog '%s' not found\n", probe_name))
> +		goto cleanup;
> +
> +	bpf_program__set_type(prog, BPF_PROG_TYPE_RAW_TRACEPOINT);

Do we need this? I thought libbpf should automatically
infer program type from section name?

> +
> +	load_attr.obj = obj;
> +	load_attr.log_level = 0;
> +	load_attr.target_btf_path = NULL;
> +	err = bpf_object__load_xattr(&load_attr);
> +	if (CHECK(err, "obj_load",
> +		  "failed to load prog '%s': %d\n",
> +		  probe_name, err))
> +		goto cleanup;

Your load_attr only has 'obj', you could use bpf_object__load
for simplicity.

> +
> +	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;
> +
> +	bss_map = bpf_object__find_map_by_name(obj, "ns_data_map");
> +	if (CHECK(!bss_map, "find_bss_map", "failed\n"))
> +		goto cleanup;
> +
> +	memset(&data, 0, sizeof(data));
> +	pid_t tid = syscall(SYS_gettid);
> +	pid_t pid = getpid();
> +
> +	id = (__u64) tid << 32 | pid;
> +	data.userpidtgid = id;
> +
> +	if (CHECK(stat("/proc/self/ns/pid", &st), "stat","failed\n"))
> +		goto cleanup;
> +
> +	data.dev = st.st_dev;
> +	data.ino = st.st_ino;
> +
> +	err = bpf_map_update_elem(bpf_map__fd(bss_map), &key, &data, 0);
> +	if (CHECK(err, "setting_bss", "failed to set bss data: %d\n", err))
> +		goto cleanup;

Typically, we would like to do map_update_elem first and then
do attach_raw_tracepoint. This will ensure updated elem is seen
even for the first invocation of the program.

In your case, since you ignore all unmatched version, so
I won't insist if there is no revision needed.

Since you need respin any way, I suggest to switch the
order between bpf_map_update_elem and attach_raw_tracepoint, which is a 
good practice any way.

> +
> +	/* trigger some syscalls */
> +	usleep(1);
> +
> +	err = bpf_map_lookup_elem(bpf_map__fd(bss_map), &key, &data);
> +	if (CHECK(err, "set_bss", "failed to get bss data: %d\n", err))
> +		goto cleanup;
> +
> +	if (CHECK(id != data.pidtgid, "Compare user pid/tgid vs. bpf pid/tgid",
> +		  "User pid/tgid %llu EBPF pid/tgid %llu\n", id, data.pidtgid))
> +		goto cleanup;
> +cleanup:
> +
> +	if (!IS_ERR_OR_NULL(link)) {
> +		bpf_link__destroy(link);
> +		link = NULL;
> +	}
> +	bpf_object__close(obj);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/get_ns_current_pid_tgid_kern.c b/tools/testing/selftests/bpf/progs/get_ns_current_pid_tgid_kern.c
> new file mode 100644
> index 000000000000..1fd847b63105
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/get_ns_current_pid_tgid_kern.c
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2019 Carlos Neira cneirabustos@...il.com */
> +
> +#include <linux/bpf.h>
> +#include <stdint.h>
> +#include "bpf_helpers.h"
> +#include "bpf_core_read.h"

You do not need "bpf_core_read.h" here.

> +
> +struct res {
> +	__u64 dev;
> +	__u64 ino;
> +	__u64 pidtgid;
> +	__u64 userpidtgid;
> +};
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_ARRAY);
> +	__uint(max_entries, 1);
> +	__type(key, __u32);
> +	__type(value, struct res);
> +} ns_data_map SEC(".maps");

In you code, you use ns_data_map which has max_entries = 1.
In this case, static volatile variable can be used to
simplify the bpf program. You have
'static struct res data' which will turn into a map
as well. So now you have two maps to hold the nsdata.
I suggest to remove the above ns_data_map.

You can take a look at the below commit for an example:

commit 666b2c10ee9d51f14d04c416a14b1cb6fd0846e4
Author: Andrii Nakryiko <andriin@...com>
Date:   Wed Oct 9 13:14:58 2019 -0700

     selftests/bpf: Add read-only map values propagation tests

     Add tests checking that verifier does proper constant propagation for
     read-only maps. If constant propagation didn't work, skipp_loop and
     part_loop BPF programs would be rejected due to BPF verifier otherwise
     not being able to prove they ever complete. With constant propagation,
     though, they are succesfully validated as properly terminating loops.

> +
> +static struct res data;
> +
> +SEC("raw_tracepoint/sys_enter")
> +int trace(void *ctx)
> +{
> +	__u64  nspidtgid, expected_pid;
> +	struct bpf_pidns_info nsdata;
> +	const __u32 key = 0;

You can remove "const" here, not very useful.

> +	struct res *pres;
> +
> +	pres = bpf_map_lookup_elem(&ns_data_map, &key);
> +	if (!pres)
> +		return 0;
> +
> +	if (bpf_get_ns_current_pid_tgid(pres->dev, pres->ino, &nsdata,
> +		   sizeof(struct bpf_pidns_info)))
> +		return 0;
> +
> +	nspidtgid = (__u64)nsdata.tgid << 32 | nsdata.pid;
> +	expected_pid = pres->userpidtgid;
> +
> +	if (expected_pid != nspidtgid)
> +		return 0;
> +
> +	data.pidtgid = nspidtgid;
> +	bpf_map_update_elem(&ns_data_map, &key, &data, 0);
> +
> +	return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> 

Powered by blists - more mailing lists