[PATCH v14 0/5] BPF: New helper to obtain namespace data from current task

Currently bpf_get_current_pid_tgid(), is used to do pid filtering in bcc's
scripts but this helper returns the pid as seen by the root namespace which is
fine when a bcc script is not executed inside a container.
When the process of interest is inside a container, pid filtering will not work
if bpf_get_current_pid_tgid() is used.
This helper addresses this limitation returning the pid as it's seen by the current
namespace where the script is executing.

In the future different pid_ns files may belong to different devices, according to the
discussion between Eric Biederman and Yonghong in 2017 Linux plumbers conference.
To address that situation the helper requires inum and dev_t from /proc/self/ns/pid.
This helper has the same use cases as bpf_get_current_pid_tgid() as it can be
used to do pid filtering even inside a container.

Changes from V13:

- refactored selftests
- refactored ebpf helper

Signed-off-by: Carlos Neira <redacted>


Carlos Neira (5):
  fs/nsfs.c: added ns_match
  bpf: added new helper bpf_get_ns_current_pid_tgid
  tools: Added bpf_get_ns_current_pid_tgid helper
  tools/testing/selftests/bpf: Add self-tests for new  helper.
  bpf_helpers_doc.py: Add struct bpf_pidns_info to known types

 fs/nsfs.c                                     |  8 ++
 include/linux/bpf.h                           |  1 +
 include/linux/proc_ns.h                       |  2 +
 include/uapi/linux/bpf.h                      | 20 +++-
 kernel/bpf/core.c                             |  1 +
 kernel/bpf/helpers.c                          | 45 +++++++++
 kernel/trace/bpf_trace.c                      |  2 +
 scripts/bpf_helpers_doc.py                    |  1 +
 tools/include/uapi/linux/bpf.h                | 20 +++-
 .../bpf/prog_tests/get_ns_current_pid_tgid.c  | 96 +++++++++++++++++++
 .../bpf/progs/get_ns_current_pid_tgid_kern.c  | 53 ++++++++++
 11 files changed, 247 insertions(+), 2 deletions(-)
 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

-- 
2.20.1

ns_match returns true if the namespace inode and dev_t matches the ones
provided by the caller.

Signed-off-by: Carlos Neira <redacted>
---
 fs/nsfs.c               | 8 ++++++++
 include/linux/proc_ns.h | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/fs/nsfs.c b/fs/nsfs.c
index a0431642c6b5..256f6295d33d 100644
--- a/fs/nsfs.c
+++ b/fs/nsfs.c

@@ -245,6 +245,14 @@ struct file *proc_ns_fget(int fd)
 	return ERR_PTR(-EINVAL);
 }
 
+/* Returns true if current namespace matches dev/ino.
+ */
+bool ns_match(const struct ns_common *ns, dev_t dev, ino_t ino)
+{
+	return ((ns->inum == ino) && (nsfs_mnt->mnt_sb->s_dev == dev));
+}
+
+
 static int nsfs_show_path(struct seq_file *seq, struct dentry *dentry)
 {
 	struct inode *inode = d_inode(dentry);

diff --git a/include/linux/proc_ns.h b/include/linux/proc_ns.h
index d31cb6215905..1da9f33489f3 100644
--- a/include/linux/proc_ns.h
+++ b/include/linux/proc_ns.h

@@ -82,6 +82,8 @@ typedef struct ns_common *ns_get_path_helper_t(void *);
 extern void *ns_get_path_cb(struct path *path, ns_get_path_helper_t ns_get_cb,
 			    void *private_data);
 
+extern bool ns_match(const struct ns_common *ns, dev_t dev, ino_t ino);
+
 extern int ns_get_name(char *buf, size_t size, struct task_struct *task,
 			const struct proc_ns_operations *ns_ops);
 extern void nsfs_init(void);

-- 
2.20.1

New bpf helper bpf_get_ns_current_pid_tgid,
This helper will return pid and tgid from current task
which namespace matches dev_t and inode number provided,
this will allows us to instrument a process inside a container.

Signed-off-by: Carlos Neira <redacted>
---
 include/linux/bpf.h      |  1 +
 include/uapi/linux/bpf.h | 20 +++++++++++++++++-
 kernel/bpf/core.c        |  1 +
 kernel/bpf/helpers.c     | 45 ++++++++++++++++++++++++++++++++++++++++
 kernel/trace/bpf_trace.c |  2 ++
 5 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 5b9d22338606..231001475504 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h

@@ -1055,6 +1055,7 @@ extern const struct bpf_func_proto bpf_get_local_storage_proto;
 extern const struct bpf_func_proto bpf_strtol_proto;
 extern const struct bpf_func_proto bpf_strtoul_proto;
 extern const struct bpf_func_proto bpf_tcp_sock_proto;
+extern const struct bpf_func_proto bpf_get_ns_current_pid_tgid_proto;
 
 /* Shared helpers among cBPF and eBPF. */
 void bpf_user_rnd_init_once(void);

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index a65c3b0c6935..a17583ae9aa3 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h

@@ -2750,6 +2750,19 @@ union bpf_attr {
  *		**-EOPNOTSUPP** kernel configuration does not enable SYN cookies
  *
  *		**-EPROTONOSUPPORT** IP packet version is not 4 or 6
+ *
+ * u64 bpf_get_ns_current_pid_tgid(u64 dev, u64 ino, struct bpf_pidns_info *nsdata, u32 size)
+ *	Description
+ *		Returns 0 on success, values for *pid* and *tgid* as seen from the current
+ *		*namespace* will be returned in *nsdata*.
+ *
+ *		On failure, the returned value is one of the following:
+ *
+ *		**-EINVAL** if dev and inum supplied don't match dev_t and inode number
+ *              with nsfs of current task, or if dev conversion to dev_t lost high bits.
+ *
+ *		**-ENOENT** if /proc/self/ns does not exists.
+ *
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\

@@ -2862,7 +2875,8 @@ union bpf_attr {
 	FN(sk_storage_get),		\
 	FN(sk_storage_delete),		\
 	FN(send_signal),		\
-	FN(tcp_gen_syncookie),
+	FN(tcp_gen_syncookie),          \
+	FN(get_ns_current_pid_tgid),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call

@@ -3613,4 +3627,8 @@ struct bpf_sockopt {
 	__s32	retval;
 };
 
+struct bpf_pidns_info {
+	__u32 pid;
+	__u32 tgid;
+};
 #endif /* _UAPI__LINUX_BPF_H__ */

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 66088a9e9b9e..b2fd5358f472 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c

@@ -2042,6 +2042,7 @@ const struct bpf_func_proto bpf_get_current_uid_gid_proto __weak;
 const struct bpf_func_proto bpf_get_current_comm_proto __weak;
 const struct bpf_func_proto bpf_get_current_cgroup_id_proto __weak;
 const struct bpf_func_proto bpf_get_local_storage_proto __weak;
+const struct bpf_func_proto bpf_get_ns_current_pid_tgid_proto __weak;
 
 const struct bpf_func_proto * __weak bpf_get_trace_printk_proto(void)
 {

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 5e28718928ca..5477ad984d7c 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c

@@ -11,6 +11,8 @@
 #include <linux/uidgid.h>
 #include <linux/filter.h>
 #include <linux/ctype.h>
+#include <linux/pid_namespace.h>
+#include <linux/proc_ns.h>
 
 #include "../../lib/kstrtox.h"
 

@@ -487,3 +489,46 @@ const struct bpf_func_proto bpf_strtoul_proto = {
 	.arg4_type	= ARG_PTR_TO_LONG,
 };
 #endif
+
+BPF_CALL_4(bpf_get_ns_current_pid_tgid, u64, dev, u64, ino,
+	   struct bpf_pidns_info *, nsdata, u32, size)
+{
+	struct task_struct *task = current;
+	struct pid_namespace *pidns;
+	int err = -EINVAL;
+
+	if (unlikely(size != sizeof(struct bpf_pidns_info)))
+		goto clear;
+
+	if (unlikely((u64)(dev_t)dev != dev))
+		goto clear;
+
+	if (unlikely(!task))
+		goto clear;
+
+	pidns = task_active_pid_ns(task);
+	if (unlikely(!pidns)) {
+		err = -ENOENT;
+		goto clear;
+	}
+
+	if (!ns_match(&pidns->ns, (dev_t)dev, ino))
+		goto clear;
+
+	nsdata->pid = task_pid_nr_ns(task, pidns);
+	nsdata->tgid = task_tgid_nr_ns(task, pidns);
+	return 0;
+clear:
+	memset((void *)nsdata, 0, (size_t) size);
+	return err;
+}
+
+const struct bpf_func_proto bpf_get_ns_current_pid_tgid_proto = {
+	.func		= bpf_get_ns_current_pid_tgid,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_ANYTHING,
+	.arg2_type	= ARG_ANYTHING,
+	.arg3_type      = ARG_PTR_TO_UNINIT_MEM,
+	.arg4_type      = ARG_CONST_SIZE,
+};

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 44bd08f2443b..32331a1dcb6d 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c

@@ -735,6 +735,8 @@ tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 #endif
 	case BPF_FUNC_send_signal:
 		return &bpf_send_signal_proto;
+	case BPF_FUNC_get_ns_current_pid_tgid:
+		return &bpf_get_ns_current_pid_tgid_proto;
 	default:
 		return NULL;
 	}

-- 
2.20.1

sync tools/include/uapi/linux/bpf.h to include new helper.

Signed-off-by: Carlos Neira <redacted>
---
 tools/include/uapi/linux/bpf.h | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a65c3b0c6935..a17583ae9aa3 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h

@@ -2750,6 +2750,19 @@ union bpf_attr {
  *		**-EOPNOTSUPP** kernel configuration does not enable SYN cookies
  *
  *		**-EPROTONOSUPPORT** IP packet version is not 4 or 6
+ *
+ * u64 bpf_get_ns_current_pid_tgid(u64 dev, u64 ino, struct bpf_pidns_info *nsdata, u32 size)
+ *	Description
+ *		Returns 0 on success, values for *pid* and *tgid* as seen from the current
+ *		*namespace* will be returned in *nsdata*.
+ *
+ *		On failure, the returned value is one of the following:
+ *
+ *		**-EINVAL** if dev and inum supplied don't match dev_t and inode number
+ *              with nsfs of current task, or if dev conversion to dev_t lost high bits.
+ *
+ *		**-ENOENT** if /proc/self/ns does not exists.
+ *
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\

@@ -2862,7 +2875,8 @@ union bpf_attr {
 	FN(sk_storage_get),		\
 	FN(sk_storage_delete),		\
 	FN(send_signal),		\
-	FN(tcp_gen_syncookie),
+	FN(tcp_gen_syncookie),          \
+	FN(get_ns_current_pid_tgid),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call

@@ -3613,4 +3627,8 @@ struct bpf_sockopt {
 	__s32	retval;
 };
 
+struct bpf_pidns_info {
+	__u32 pid;
+	__u32 tgid;
+};
 #endif /* _UAPI__LINUX_BPF_H__ */

-- 
2.20.1

Self tests added for new helper

Signed-off-by: Carlos Neira <redacted>
---
 .../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;
+	__u64 userpidtgid;
+} 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);
+
+	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;
+
+	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;
+
+	/* 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"
+
+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");
+
+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;
+	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";

-- 
2.20.1

Add struct bpf_pidns_info to known types

Signed-off-by: Carlos Neira <redacted>
---
 scripts/bpf_helpers_doc.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/bpf_helpers_doc.py b/scripts/bpf_helpers_doc.py
index 7df9ce598ff9..d42eb185810f 100755
--- a/scripts/bpf_helpers_doc.py
+++ b/scripts/bpf_helpers_doc.py

@@ -450,6 +450,7 @@ class PrinterHelpers(Printer):
             'struct sk_reuseport_md',
             'struct sockaddr',
             'struct tcphdr',
+            'struct bpf_pidns_info',
     }
     mapped_types = {
             'u8': '__u8',

-- 
2.20.1

On Thu, Oct 17, 2019 at 12:00:28PM -0300, Carlos Neira wrote:

The above could be a single line comment.
Perhaps using kdoc format would be appropriate here.

+bool ns_match(const struct ns_common *ns, dev_t dev, ino_t ino)
+{
+	return ((ns->inum == ino) && (nsfs_mnt->mnt_sb->s_dev == dev));

The parentheses on the line above seem unnecessary.

On 10/17/19 8:00 AM, Carlos Neira wrote:

New bpf helper bpf_get_ns_current_pid_tgid,
This helper will return pid and tgid from current task
which namespace matches dev_t and inode number provided,
this will allows us to instrument a process inside a container.

Signed-off-by: Carlos Neira <redacted>

You need to rebase the whole series, a new helper skb_output
is just added.

-EINVAL/-ENOENT may be returned, so let us have return type "int" instead.

+ *	Description
+ *		Returns 0 on success, values for *pid* and *tgid* as seen from the current
+ *		*namespace* will be returned in *nsdata*.
+ *
+ *		On failure, the returned value is one of the following:
+ *
+ *		**-EINVAL** if dev and inum supplied don't match dev_t and inode number
+ *              with nsfs of current task, or if dev conversion to dev_t lost high bits.
+ *
+ *		**-ENOENT** if /proc/self/ns does not exists.

Let us do not hard code the /proc/self/ns path. Just mention that the 
pidns does not exist for the current task.

On 10/17/19 8:00 AM, Carlos Neira wrote:

The same return type and description changes as suggested in my
previous comment.

On 10/17/19 8:00 AM, Carlos Neira wrote:

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.

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 [off-list ref]
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";

On 10/17/19 8:00 AM, Carlos Neira wrote:

looks like the names are sorted. please put it into appropriate place.

      }
      mapped_types = {
              'u8': '__u8',

On Sat, Oct 19, 2019 at 1:58 AM Yonghong Song [off-list ref] wrote:

On 10/17/19 8:00 AM, Carlos Neira wrote:

quoted

Self tests added for new helper

Signed-off-by: Carlos Neira <redacted>
---
  .../bpf/prog_tests/get_ns_current_pid_tgid.c  | 96 +++++++++++++++++++
  .../bpf/progs/get_ns_current_pid_tgid_kern.c  | 53 ++++++++++

It looks like typical naming convention is:

prog_test/<something>.c
progs/test_<something>.c

Let's keep this consistent. I'm about to do a bit smarter Makefile
that will capture this convention, so it's good to have less exception
to create. Thanks!

Otherwise, besides what Yonghong mentioned, this look good to me.

quoted

  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

[...]

quoted

+     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?

We used to, until the patch set that Daniel landed today. Now it can be dropped.

quoted

+
+     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;

[...]

On Mon, Oct 21, 2019 at 11:20:01AM -0700, Andrii Nakryiko wrote:

On Sat, Oct 19, 2019 at 1:58 AM Yonghong Song [off-list ref] wrote:

quoted

On 10/17/19 8:00 AM, Carlos Neira wrote:

quoted

Self tests added for new helper

Signed-off-by: Carlos Neira <redacted>
---
  .../bpf/prog_tests/get_ns_current_pid_tgid.c  | 96 +++++++++++++++++++
  .../bpf/progs/get_ns_current_pid_tgid_kern.c  | 53 ++++++++++

It looks like typical naming convention is:

prog_test/<something>.c
progs/test_<something>.c

Let's keep this consistent. I'm about to do a bit smarter Makefile
that will capture this convention, so it's good to have less exception
to create. Thanks!

Otherwise, besides what Yonghong mentioned, this look good to me.

quoted

quoted

  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

[...]

quoted

quoted

+     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?

We used to, until the patch set that Daniel landed today. Now it can be dropped.

quoted

quoted

+
+     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;

[...]

Thanks Andrii,
I have a doubt, I don't find in prog_tests/rdonly_map.c  where is "test_rdo.bss" defined ?, is called in line 43 but I'm missing how to is it used as I don't see it defined.

Bests

On Mon, Oct 21, 2019 at 12:14 PM Carlos Antonio Neira Bustos
[off-list ref] wrote:

On Mon, Oct 21, 2019 at 11:20:01AM -0700, Andrii Nakryiko wrote:

quoted

On Sat, Oct 19, 2019 at 1:58 AM Yonghong Song [off-list ref] wrote:

quoted

On 10/17/19 8:00 AM, Carlos Neira wrote:

quoted

Self tests added for new helper

Signed-off-by: Carlos Neira <redacted>
---
  .../bpf/prog_tests/get_ns_current_pid_tgid.c  | 96 +++++++++++++++++++
  .../bpf/progs/get_ns_current_pid_tgid_kern.c  | 53 ++++++++++

It looks like typical naming convention is:

prog_test/<something>.c
progs/test_<something>.c

Let's keep this consistent. I'm about to do a bit smarter Makefile
that will capture this convention, so it's good to have less exception
to create. Thanks!

Otherwise, besides what Yonghong mentioned, this look good to me.

quoted

quoted

  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

[...]

quoted

quoted

+     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?

We used to, until the patch set that Daniel landed today. Now it can be dropped.

quoted

quoted

+
+     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;

[...]

Thanks Andrii,
I have a doubt, I don't find in prog_tests/rdonly_map.c  where is "test_rdo.bss" defined ?, is called in line 43 but I'm missing how to is it used as I don't see it defined.

This map is created by libbpf implicitly from global variables used by
BPF object. You just look it up by name, set its value to whatever you
need global variables to be set up to, and that value will be
available to BPF program. From BPF program side, when you update
global variable, that value can be read from user space using that
same test_rdo.bss map. Does it make sense?

Bests

On Mon, Oct 21, 2019 at 12:18:33PM -0700, Andrii Nakryiko wrote:

On Mon, Oct 21, 2019 at 12:14 PM Carlos Antonio Neira Bustos
[off-list ref] wrote:

quoted

On Mon, Oct 21, 2019 at 11:20:01AM -0700, Andrii Nakryiko wrote:

quoted

On Sat, Oct 19, 2019 at 1:58 AM Yonghong Song [off-list ref] wrote:

quoted

On 10/17/19 8:00 AM, Carlos Neira wrote:

quoted

Self tests added for new helper

Signed-off-by: Carlos Neira <redacted>
---
  .../bpf/prog_tests/get_ns_current_pid_tgid.c  | 96 +++++++++++++++++++
  .../bpf/progs/get_ns_current_pid_tgid_kern.c  | 53 ++++++++++

It looks like typical naming convention is:

prog_test/<something>.c
progs/test_<something>.c

Let's keep this consistent. I'm about to do a bit smarter Makefile
that will capture this convention, so it's good to have less exception
to create. Thanks!

Otherwise, besides what Yonghong mentioned, this look good to me.

quoted

quoted

  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

[...]

quoted

quoted

+     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?

We used to, until the patch set that Daniel landed today. Now it can be dropped.

quoted

quoted

+
+     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;

[...]

Thanks Andrii,
I have a doubt, I don't find in prog_tests/rdonly_map.c  where is "test_rdo.bss" defined ?, is called in line 43 but I'm missing how to is it used as I don't see it defined.

This map is created by libbpf implicitly from global variables used by
BPF object. You just look it up by name, set its value to whatever you
need global variables to be set up to, and that value will be
available to BPF program. From BPF program side, when you update
global variable, that value can be read from user space using that
same test_rdo.bss map. Does it make sense?

quoted

Bests

Thanks for the explanation Andrii, now it works!.