bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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 v14 4/5] tools/testing/selftests/bpf: Add self-tests for new helper.
Date: Fri, 18 Oct 2019 17:33:15 +0000	[thread overview]
Message-ID: <d88ce3ca-d235-cd9c-c1a9-c2d01a01541d@fb.com> (raw)
In-Reply-To: <20191017150032.14359-5-cneirabustos@gmail.com>



On 10/17/19 8:00 AM, Carlos Neira wrote:
> Self tests added for new helper
> 
> Signed-off-by: Carlos Neira <cneirabustos@gmail.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@gmail.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@gmail.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@fb.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";
> 

  reply	other threads:[~2019-10-18 17:33 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-17 15:00 [PATCH v14 0/5] BPF: New helper to obtain namespace data from current task Carlos Neira
2019-10-17 15:00 ` [PATCH v14 1/5] fs/nsfs.c: added ns_match Carlos Neira
2019-10-18  9:24   ` Simon Horman
2019-10-17 15:00 ` [PATCH v14 2/5] bpf: added new helper bpf_get_ns_current_pid_tgid Carlos Neira
2019-10-18 17:10   ` Yonghong Song
2019-10-17 15:00 ` [PATCH v14 3/5] tools: Added bpf_get_ns_current_pid_tgid helper Carlos Neira
2019-10-18 17:11   ` Yonghong Song
2019-10-17 15:00 ` [PATCH v14 4/5] tools/testing/selftests/bpf: Add self-tests for new helper Carlos Neira
2019-10-18 17:33   ` Yonghong Song [this message]
2019-10-21 18:20     ` Andrii Nakryiko
2019-10-21 19:14       ` Carlos Antonio Neira Bustos
2019-10-21 19:18         ` Andrii Nakryiko
2019-10-22 16:50           ` Carlos Antonio Neira Bustos
2019-10-17 15:00 ` [PATCH v14 5/5] bpf_helpers_doc.py: Add struct bpf_pidns_info to known types Carlos Neira
2019-10-18 17:34   ` 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=d88ce3ca-d235-cd9c-c1a9-c2d01a01541d@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).