netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next 0/8] misc fixes for bpf_iter
@ 2020-05-12 15:52 Yonghong Song
  2020-05-12 15:52 ` [PATCH bpf-next 1/8] tools/bpf: selftests : explain bpf_iter test failures with llvm 10.0.0 Yonghong Song
                   ` (7 more replies)
  0 siblings, 8 replies; 16+ messages in thread
From: Yonghong Song @ 2020-05-12 15:52 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, Martin KaFai Lau, netdev
  Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team

Commit ae24345da54e ("bpf: Implement an interface to register
bpf_iter targets") and its subsequent commits in the same patch set
introduced bpf iterator, a way to run bpf program when iterating
kernel data structures.

This patch set addressed some followup issues. One big change
is to allow target to pass ctx arg register types to verifier
for verification purpose. Please see individual patch for details.

Yonghong Song (8):
  tools/bpf: selftests : explain bpf_iter test failures with llvm 10.0.0
  bpf: change btf_iter func proto prefix to "bpf_iter_"
  bpf: add comments to interpret bpf_prog return values
  bpf: add WARN_ONCE if bpf_seq_read show() return a positive number
  bpf: net: refactor bpf_iter target registration
  bpf: change func bpf_iter_unreg_target() signature
  bpf: enable bpf_iter targets registering ctx argument types
  samples/bpf: remove compiler warnings

 include/linux/bpf.h                    | 20 +++++++++---
 include/net/ip6_fib.h                  |  7 ++++
 kernel/bpf/bpf_iter.c                  | 44 +++++++++++++++-----------
 kernel/bpf/btf.c                       | 15 ++++++---
 kernel/bpf/map_iter.c                  | 23 ++++++++------
 kernel/bpf/task_iter.c                 | 42 ++++++++++++++++--------
 kernel/bpf/verifier.c                  |  1 -
 net/ipv6/ip6_fib.c                     |  5 ---
 net/ipv6/route.c                       | 25 +++++++++------
 net/netlink/af_netlink.c               | 23 ++++++++------
 samples/bpf/offwaketime_kern.c         |  4 +--
 samples/bpf/sockex2_kern.c             |  4 +--
 samples/bpf/sockex3_kern.c             |  4 +--
 tools/lib/bpf/libbpf.c                 |  2 +-
 tools/testing/selftests/bpf/README.rst | 43 +++++++++++++++++++++++++
 15 files changed, 178 insertions(+), 84 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/README.rst

-- 
2.24.1


^ permalink raw reply	[flat|nested] 16+ messages in thread
* [PATCH bpf-next 1/8] tools/bpf: selftests : explain bpf_iter test failures with llvm 10.0.0
  2020-05-12 15:52 [PATCH bpf-next 0/8] misc fixes for bpf_iter Yonghong Song
@ 2020-05-12 15:52 ` Yonghong Song
  2020-05-12 15:52 ` [PATCH bpf-next 2/8] bpf: change btf_iter func proto prefix to "bpf_iter_" Yonghong Song
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 16+ messages in thread
From: Yonghong Song @ 2020-05-12 15:52 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, Martin KaFai Lau, netdev
  Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Alexei Starovoitov

Commit 6879c042e105 ("tools/bpf: selftests: Add bpf_iter selftests")
added self tests for bpf_iter feature. But two subtests
ipv6_route and netlink needs llvm latest 10.x release branch
or trunk due to a bug in llvm BPF backend. This patch added
the file README.rst to document these two failures
so people using llvm 10.0.0 can be aware of them.

Suggested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Yonghong Song <yhs@fb.com>
---
 tools/testing/selftests/bpf/README.rst | 43 ++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/README.rst

diff --git a/tools/testing/selftests/bpf/README.rst b/tools/testing/selftests/bpf/README.rst
new file mode 100644
index 000000000000..0f67f1b470b0
--- /dev/null
+++ b/tools/testing/selftests/bpf/README.rst
@@ -0,0 +1,43 @@
+==================
+BPF Selftest Notes
+==================
+
+Additional information about selftest failures are
+documented here.
+
+bpf_iter test failures with clang/llvm 10.0.0
+=============================================
+
+With clang/llvm 10.0.0, the following two bpf_iter tests failed:
+  * ``bpf_iter/ipv6_route``
+  * ``bpf_iter/netlink``
+
+The symptom for ``bpf_iter/ipv6_route`` looks like
+
+.. code-block:: c
+
+  2: (79) r8 = *(u64 *)(r1 +8)
+  ...
+  14: (bf) r2 = r8
+  15: (0f) r2 += r1
+  ; BPF_SEQ_PRINTF(seq, "%pi6 %02x ", &rt->fib6_dst.addr, rt->fib6_dst.plen);
+  16: (7b) *(u64 *)(r8 +64) = r2
+  only read is supported
+
+The symptom for ``bpf_iter/netlink`` looks like
+
+.. code-block:: c
+
+  ; struct netlink_sock *nlk = ctx->sk;
+  2: (79) r7 = *(u64 *)(r1 +8)
+  ...
+  15: (bf) r2 = r7
+  16: (0f) r2 += r1
+  ; BPF_SEQ_PRINTF(seq, "%pK %-3d ", s, s->sk_protocol);
+  17: (7b) *(u64 *)(r7 +0) = r2
+  only read is supported
+
+This is due to a llvm BPF backend bug. The fix 
+  https://reviews.llvm.org/D78466
+has been pushed to llvm 10.x release branch and will be
+available in 10.0.1. The fix is available in llvm 11.0.0 trunk.
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH bpf-next 2/8] bpf: change btf_iter func proto prefix to "bpf_iter_"
  2020-05-12 15:52 [PATCH bpf-next 0/8] misc fixes for bpf_iter Yonghong Song
  2020-05-12 15:52 ` [PATCH bpf-next 1/8] tools/bpf: selftests : explain bpf_iter test failures with llvm 10.0.0 Yonghong Song
@ 2020-05-12 15:52 ` Yonghong Song
  2020-05-12 22:21   ` Andrii Nakryiko
  2020-05-12 15:52 ` [PATCH bpf-next 3/8] bpf: add comments to interpret bpf_prog return values Yonghong Song
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 16+ messages in thread
From: Yonghong Song @ 2020-05-12 15:52 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, Martin KaFai Lau, netdev
  Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Alexei Starovoitov

This is to be consistent with tracing and lsm programs
which have prefix "bpf_trace_" and "bpf_lsm_" respectively.

Suggested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Yonghong Song <yhs@fb.com>
---
 include/linux/bpf.h    | 6 +++---
 tools/lib/bpf/libbpf.c | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index cf4b6e44f2bc..ab94dfd8826f 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1131,10 +1131,10 @@ struct bpf_link *bpf_link_get_from_fd(u32 ufd);
 int bpf_obj_pin_user(u32 ufd, const char __user *pathname);
 int bpf_obj_get_user(const char __user *pathname, int flags);
 
-#define BPF_ITER_FUNC_PREFIX "__bpf_iter__"
+#define BPF_ITER_FUNC_PREFIX "bpf_iter_"
 #define DEFINE_BPF_ITER_FUNC(target, args...)			\
-	extern int __bpf_iter__ ## target(args);		\
-	int __init __bpf_iter__ ## target(args) { return 0; }
+	extern int bpf_iter_ ## target(args);			\
+	int __init bpf_iter_ ## target(args) { return 0; }
 
 typedef int (*bpf_iter_init_seq_priv_t)(void *private_data);
 typedef void (*bpf_iter_fini_seq_priv_t)(void *private_data);
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 3da66540b54b..d063c247615f 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -6897,7 +6897,7 @@ static int bpf_object__collect_st_ops_relos(struct bpf_object *obj,
 
 #define BTF_TRACE_PREFIX "btf_trace_"
 #define BTF_LSM_PREFIX "bpf_lsm_"
-#define BTF_ITER_PREFIX "__bpf_iter__"
+#define BTF_ITER_PREFIX "bpf_iter_"
 #define BTF_MAX_NAME_SIZE 128
 
 static int find_btf_by_prefix_kind(const struct btf *btf, const char *prefix,
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH bpf-next 3/8] bpf: add comments to interpret bpf_prog return values
  2020-05-12 15:52 [PATCH bpf-next 0/8] misc fixes for bpf_iter Yonghong Song
  2020-05-12 15:52 ` [PATCH bpf-next 1/8] tools/bpf: selftests : explain bpf_iter test failures with llvm 10.0.0 Yonghong Song
  2020-05-12 15:52 ` [PATCH bpf-next 2/8] bpf: change btf_iter func proto prefix to "bpf_iter_" Yonghong Song
@ 2020-05-12 15:52 ` Yonghong Song
  2020-05-12 22:21   ` Andrii Nakryiko
  2020-05-12 15:52 ` [PATCH bpf-next 4/8] bpf: add WARN_ONCE if bpf_seq_read show() return a positive number Yonghong Song
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 16+ messages in thread
From: Yonghong Song @ 2020-05-12 15:52 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, Martin KaFai Lau, netdev
  Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team

Add a short comment in bpf_iter_run_prog() function to
explain how bpf_prog return value is converted to
seq_ops->show() return value:
  bpf_prog return           seq_ops()->show() return
     0                         0
     1                         -EAGAIN

When show() return value is -EAGAIN, the current
bpf_seq_read() will end. If the current seq_file buffer
is empty, -EAGAIN will return to user space. Otherwise,
the buffer will be copied to user space.
In both cases, the next bpf_seq_read() call will
try to show the same object which returned -EAGAIN
previously.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 kernel/bpf/bpf_iter.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
index 30efd15cd4a0..0a45a6cdfabd 100644
--- a/kernel/bpf/bpf_iter.c
+++ b/kernel/bpf/bpf_iter.c
@@ -526,5 +526,11 @@ int bpf_iter_run_prog(struct bpf_prog *prog, void *ctx)
 	migrate_enable();
 	rcu_read_unlock();
 
+	/* bpf program can only return 0 or 1:
+	 *  0 : okay
+	 *  1 : retry the same object
+	 * The bpf_iter_run_prog() return value
+	 * will be seq_ops->show() return value.
+	 */
 	return ret == 0 ? 0 : -EAGAIN;
 }
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH bpf-next 4/8] bpf: add WARN_ONCE if bpf_seq_read show() return a positive number
  2020-05-12 15:52 [PATCH bpf-next 0/8] misc fixes for bpf_iter Yonghong Song
                   ` (2 preceding siblings ...)
  2020-05-12 15:52 ` [PATCH bpf-next 3/8] bpf: add comments to interpret bpf_prog return values Yonghong Song
@ 2020-05-12 15:52 ` Yonghong Song
  2020-05-12 22:23   ` Andrii Nakryiko
  2020-05-12 15:52 ` [PATCH bpf-next 5/8] bpf: net: refactor bpf_iter target registration Yonghong Song
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 16+ messages in thread
From: Yonghong Song @ 2020-05-12 15:52 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, Martin KaFai Lau, netdev
  Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Alexei Starovoitov

In seq_read() implementation, a positive integer return value
of seq_ops->show() indicates that the current object seq_file
buffer is discarded and next object should be checked.
bpf_seq_read() implemented in a similar way if show()
returns a positive integer value.

But for bpf_seq_read(), show() didn't return positive integer for
all currently supported targets. Let us add a WARN_ONCE for
such cases so we can get an alert when things are changed.

Suggested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Yonghong Song <yhs@fb.com>
---
 kernel/bpf/bpf_iter.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
index 0a45a6cdfabd..b0c8b3bdf3b0 100644
--- a/kernel/bpf/bpf_iter.c
+++ b/kernel/bpf/bpf_iter.c
@@ -120,6 +120,7 @@ static ssize_t bpf_seq_read(struct file *file, char __user *buf, size_t size,
 
 	err = seq->op->show(seq, p);
 	if (err > 0) {
+		WARN_ONCE(1, "seq_ops->show() returns %d\n", err);
 		/* object is skipped, decrease seq_num, so next
 		 * valid object can reuse the same seq_num.
 		 */
@@ -156,6 +157,7 @@ static ssize_t bpf_seq_read(struct file *file, char __user *buf, size_t size,
 
 		err = seq->op->show(seq, p);
 		if (err > 0) {
+			WARN_ONCE(1, "seq_ops->show() returns %d\n", err);
 			bpf_iter_dec_seq_num(seq);
 			seq->count = offs;
 		} else if (err < 0 || seq_has_overflowed(seq)) {
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH bpf-next 5/8] bpf: net: refactor bpf_iter target registration
  2020-05-12 15:52 [PATCH bpf-next 0/8] misc fixes for bpf_iter Yonghong Song
                   ` (3 preceding siblings ...)
  2020-05-12 15:52 ` [PATCH bpf-next 4/8] bpf: add WARN_ONCE if bpf_seq_read show() return a positive number Yonghong Song
@ 2020-05-12 15:52 ` Yonghong Song
  2020-05-12 22:19   ` Andrii Nakryiko
  2020-05-12 15:52 ` [PATCH bpf-next 6/8] bpf: change func bpf_iter_unreg_target() signature Yonghong Song
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 16+ messages in thread
From: Yonghong Song @ 2020-05-12 15:52 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, Martin KaFai Lau, netdev
  Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team

Currently bpf_iter_reg_target takes parameters from target
and allocates memory to save them. This is really not
necessary, esp. in the future we may grow information
passed from targets to bpf_iter manager.

The patch refactors the code so target reg_info
becomes static and bpf_iter manager can just take
a reference to it.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 kernel/bpf/bpf_iter.c    | 29 +++++++++++------------------
 kernel/bpf/map_iter.c    | 18 +++++++++---------
 kernel/bpf/task_iter.c   | 30 ++++++++++++++++--------------
 net/ipv6/route.c         | 18 +++++++++---------
 net/netlink/af_netlink.c | 18 +++++++++---------
 5 files changed, 54 insertions(+), 59 deletions(-)

diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
index b0c8b3bdf3b0..1d203dc7afe2 100644
--- a/kernel/bpf/bpf_iter.c
+++ b/kernel/bpf/bpf_iter.c
@@ -8,11 +8,7 @@
 
 struct bpf_iter_target_info {
 	struct list_head list;
-	const char *target;
-	const struct seq_operations *seq_ops;
-	bpf_iter_init_seq_priv_t init_seq_private;
-	bpf_iter_fini_seq_priv_t fini_seq_private;
-	u32 seq_priv_size;
+	struct bpf_iter_reg *reg_info;
 	u32 btf_id;	/* cached value */
 };
 
@@ -224,8 +220,8 @@ static int iter_release(struct inode *inode, struct file *file)
 	iter_priv = container_of(seq->private, struct bpf_iter_priv_data,
 				 target_private);
 
-	if (iter_priv->tinfo->fini_seq_private)
-		iter_priv->tinfo->fini_seq_private(seq->private);
+	if (iter_priv->tinfo->reg_info->fini_seq_private)
+		iter_priv->tinfo->reg_info->fini_seq_private(seq->private);
 
 	bpf_prog_put(iter_priv->prog);
 	seq->private = iter_priv;
@@ -248,11 +244,7 @@ int bpf_iter_reg_target(struct bpf_iter_reg *reg_info)
 	if (!tinfo)
 		return -ENOMEM;
 
-	tinfo->target = reg_info->target;
-	tinfo->seq_ops = reg_info->seq_ops;
-	tinfo->init_seq_private = reg_info->init_seq_private;
-	tinfo->fini_seq_private = reg_info->fini_seq_private;
-	tinfo->seq_priv_size = reg_info->seq_priv_size;
+	tinfo->reg_info = reg_info;
 	INIT_LIST_HEAD(&tinfo->list);
 
 	mutex_lock(&targets_mutex);
@@ -269,7 +261,7 @@ void bpf_iter_unreg_target(const char *target)
 
 	mutex_lock(&targets_mutex);
 	list_for_each_entry(tinfo, &targets, list) {
-		if (!strcmp(target, tinfo->target)) {
+		if (!strcmp(target, tinfo->reg_info->target)) {
 			list_del(&tinfo->list);
 			kfree(tinfo);
 			found = true;
@@ -305,7 +297,7 @@ bool bpf_iter_prog_supported(struct bpf_prog *prog)
 			supported = true;
 			break;
 		}
-		if (!strcmp(attach_fname + prefix_len, tinfo->target)) {
+		if (!strcmp(attach_fname + prefix_len, tinfo->reg_info->target)) {
 			cache_btf_id(tinfo, prog);
 			supported = true;
 			break;
@@ -433,15 +425,16 @@ static int prepare_seq_file(struct file *file, struct bpf_iter_link *link)
 
 	tinfo = link->tinfo;
 	total_priv_dsize = offsetof(struct bpf_iter_priv_data, target_private) +
-			   tinfo->seq_priv_size;
-	priv_data = __seq_open_private(file, tinfo->seq_ops, total_priv_dsize);
+			   tinfo->reg_info->seq_priv_size;
+	priv_data = __seq_open_private(file, tinfo->reg_info->seq_ops,
+				       total_priv_dsize);
 	if (!priv_data) {
 		err = -ENOMEM;
 		goto release_prog;
 	}
 
-	if (tinfo->init_seq_private) {
-		err = tinfo->init_seq_private(priv_data->target_private);
+	if (tinfo->reg_info->init_seq_private) {
+		err = tinfo->reg_info->init_seq_private(priv_data->target_private);
 		if (err)
 			goto release_seq_file;
 	}
diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
index 8162e0c00b9f..545c7dbb13c9 100644
--- a/kernel/bpf/map_iter.c
+++ b/kernel/bpf/map_iter.c
@@ -81,17 +81,17 @@ static const struct seq_operations bpf_map_seq_ops = {
 	.show	= bpf_map_seq_show,
 };
 
+static struct bpf_iter_reg bpf_map_reg_info = {
+	.target			= "bpf_map",
+	.seq_ops		= &bpf_map_seq_ops,
+	.init_seq_private	= NULL,
+	.fini_seq_private	= NULL,
+	.seq_priv_size		= sizeof(struct bpf_iter_seq_map_info),
+};
+
 static int __init bpf_map_iter_init(void)
 {
-	struct bpf_iter_reg reg_info = {
-		.target			= "bpf_map",
-		.seq_ops		= &bpf_map_seq_ops,
-		.init_seq_private	= NULL,
-		.fini_seq_private	= NULL,
-		.seq_priv_size		= sizeof(struct bpf_iter_seq_map_info),
-	};
-
-	return bpf_iter_reg_target(&reg_info);
+	return bpf_iter_reg_target(&bpf_map_reg_info);
 }
 
 late_initcall(bpf_map_iter_init);
diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index aeed662d8451..ed0b1d6ce688 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -306,22 +306,24 @@ static const struct seq_operations task_file_seq_ops = {
 	.show	= task_file_seq_show,
 };
 
+static struct bpf_iter_reg task_reg_info = {
+	.target			= "task",
+	.seq_ops		= &task_seq_ops,
+	.init_seq_private	= init_seq_pidns,
+	.fini_seq_private	= fini_seq_pidns,
+	.seq_priv_size		= sizeof(struct bpf_iter_seq_task_info),
+};
+
+static struct bpf_iter_reg task_file_reg_info = {
+	.target			= "task_file",
+	.seq_ops		= &task_file_seq_ops,
+	.init_seq_private	= init_seq_pidns,
+	.fini_seq_private	= fini_seq_pidns,
+	.seq_priv_size		= sizeof(struct bpf_iter_seq_task_file_info),
+};
+
 static int __init task_iter_init(void)
 {
-	struct bpf_iter_reg task_file_reg_info = {
-		.target			= "task_file",
-		.seq_ops		= &task_file_seq_ops,
-		.init_seq_private	= init_seq_pidns,
-		.fini_seq_private	= fini_seq_pidns,
-		.seq_priv_size		= sizeof(struct bpf_iter_seq_task_file_info),
-	};
-	struct bpf_iter_reg task_reg_info = {
-		.target			= "task",
-		.seq_ops		= &task_seq_ops,
-		.init_seq_private	= init_seq_pidns,
-		.fini_seq_private	= fini_seq_pidns,
-		.seq_priv_size		= sizeof(struct bpf_iter_seq_task_info),
-	};
 	int ret;
 
 	ret = bpf_iter_reg_target(&task_reg_info);
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 25f6d3e619d0..48e8752d9ad9 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -6397,17 +6397,17 @@ void __init ip6_route_init_special_entries(void)
 #if defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_PROC_FS)
 DEFINE_BPF_ITER_FUNC(ipv6_route, struct bpf_iter_meta *meta, struct fib6_info *rt)
 
+static struct bpf_iter_reg ipv6_route_reg_info = {
+	.target			= "ipv6_route",
+	.seq_ops		= &ipv6_route_seq_ops,
+	.init_seq_private	= bpf_iter_init_seq_net,
+	.fini_seq_private	= bpf_iter_fini_seq_net,
+	.seq_priv_size		= sizeof(struct ipv6_route_iter),
+};
+
 static int __init bpf_iter_register(void)
 {
-	struct bpf_iter_reg reg_info = {
-		.target			= "ipv6_route",
-		.seq_ops		= &ipv6_route_seq_ops,
-		.init_seq_private	= bpf_iter_init_seq_net,
-		.fini_seq_private	= bpf_iter_fini_seq_net,
-		.seq_priv_size		= sizeof(struct ipv6_route_iter),
-	};
-
-	return bpf_iter_reg_target(&reg_info);
+	return bpf_iter_reg_target(&ipv6_route_reg_info);
 }
 
 static void bpf_iter_unregister(void)
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 33cda9baa979..1e2f5ab8c7d7 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2803,17 +2803,17 @@ static const struct rhashtable_params netlink_rhashtable_params = {
 };
 
 #if defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_PROC_FS)
+static struct bpf_iter_reg netlink_reg_info = {
+	.target			= "netlink",
+	.seq_ops		= &netlink_seq_ops,
+	.init_seq_private	= bpf_iter_init_seq_net,
+	.fini_seq_private	= bpf_iter_fini_seq_net,
+	.seq_priv_size		= sizeof(struct nl_seq_iter),
+};
+
 static int __init bpf_iter_register(void)
 {
-	struct bpf_iter_reg reg_info = {
-		.target			= "netlink",
-		.seq_ops		= &netlink_seq_ops,
-		.init_seq_private	= bpf_iter_init_seq_net,
-		.fini_seq_private	= bpf_iter_fini_seq_net,
-		.seq_priv_size		= sizeof(struct nl_seq_iter),
-	};
-
-	return bpf_iter_reg_target(&reg_info);
+	return bpf_iter_reg_target(&netlink_reg_info);
 }
 #endif
 
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH bpf-next 6/8] bpf: change func bpf_iter_unreg_target() signature
  2020-05-12 15:52 [PATCH bpf-next 0/8] misc fixes for bpf_iter Yonghong Song
                   ` (4 preceding siblings ...)
  2020-05-12 15:52 ` [PATCH bpf-next 5/8] bpf: net: refactor bpf_iter target registration Yonghong Song
@ 2020-05-12 15:52 ` Yonghong Song
  2020-05-12 22:24   ` Andrii Nakryiko
  2020-05-12 15:52 ` [PATCH bpf-next 7/8] bpf: enable bpf_iter targets registering ctx argument types Yonghong Song
  2020-05-12 15:52 ` [PATCH bpf-next 8/8] samples/bpf: remove compiler warnings Yonghong Song
  7 siblings, 1 reply; 16+ messages in thread
From: Yonghong Song @ 2020-05-12 15:52 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, Martin KaFai Lau, netdev
  Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team

Change func bpf_iter_unreg_target() parameter from target
name to target reg_info, similar to bpf_iter_reg_target().

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 include/linux/bpf.h   | 2 +-
 kernel/bpf/bpf_iter.c | 4 ++--
 net/ipv6/route.c      | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index ab94dfd8826f..ad1bd13cd34c 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1154,7 +1154,7 @@ struct bpf_iter_meta {
 };
 
 int bpf_iter_reg_target(struct bpf_iter_reg *reg_info);
-void bpf_iter_unreg_target(const char *target);
+void bpf_iter_unreg_target(struct bpf_iter_reg *reg_info);
 bool bpf_iter_prog_supported(struct bpf_prog *prog);
 int bpf_iter_link_attach(const union bpf_attr *attr, struct bpf_prog *prog);
 int bpf_iter_new_fd(struct bpf_link *link);
diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
index 1d203dc7afe2..041f97dcec39 100644
--- a/kernel/bpf/bpf_iter.c
+++ b/kernel/bpf/bpf_iter.c
@@ -254,14 +254,14 @@ int bpf_iter_reg_target(struct bpf_iter_reg *reg_info)
 	return 0;
 }
 
-void bpf_iter_unreg_target(const char *target)
+void bpf_iter_unreg_target(struct bpf_iter_reg *reg_info)
 {
 	struct bpf_iter_target_info *tinfo;
 	bool found = false;
 
 	mutex_lock(&targets_mutex);
 	list_for_each_entry(tinfo, &targets, list) {
-		if (!strcmp(target, tinfo->reg_info->target)) {
+		if (reg_info == tinfo->reg_info) {
 			list_del(&tinfo->list);
 			kfree(tinfo);
 			found = true;
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 48e8752d9ad9..bb8581f9b448 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -6412,7 +6412,7 @@ static int __init bpf_iter_register(void)
 
 static void bpf_iter_unregister(void)
 {
-	bpf_iter_unreg_target("ipv6_route");
+	bpf_iter_unreg_target(&ipv6_route_reg_info);
 }
 #endif
 #endif
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH bpf-next 7/8] bpf: enable bpf_iter targets registering ctx argument types
  2020-05-12 15:52 [PATCH bpf-next 0/8] misc fixes for bpf_iter Yonghong Song
                   ` (5 preceding siblings ...)
  2020-05-12 15:52 ` [PATCH bpf-next 6/8] bpf: change func bpf_iter_unreg_target() signature Yonghong Song
@ 2020-05-12 15:52 ` Yonghong Song
  2020-05-12 22:29   ` Andrii Nakryiko
  2020-05-12 15:52 ` [PATCH bpf-next 8/8] samples/bpf: remove compiler warnings Yonghong Song
  7 siblings, 1 reply; 16+ messages in thread
From: Yonghong Song @ 2020-05-12 15:52 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, Martin KaFai Lau, netdev
  Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team

Commit b121b341e598 ("bpf: Add PTR_TO_BTF_ID_OR_NULL
support") adds a field btf_id_or_null_non0_off to
bpf_prog->aux structure to indicate that the
first ctx argument is PTR_TO_BTF_ID reg_type and
all others are PTR_TO_BTF_ID_OR_NULL.
This approach does not really scale if we have
other different reg types in the future, e.g.,
a pointer to a buffer.

This patch enables bpf_iter targets registering ctx argument
reg types which may be different from the default one.
For example, for pointers to structures, the default reg_type
is PTR_TO_BTF_ID for tracing program. The target can register
a particular pointer type as PTR_TO_BTF_ID_OR_NULL which can
be used by the verifier to enforce accesses.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 include/linux/bpf.h      | 12 +++++++++++-
 include/net/ip6_fib.h    |  7 +++++++
 kernel/bpf/bpf_iter.c    |  5 +++++
 kernel/bpf/btf.c         | 15 ++++++++++-----
 kernel/bpf/map_iter.c    |  5 +++++
 kernel/bpf/task_iter.c   | 12 ++++++++++++
 kernel/bpf/verifier.c    |  1 -
 net/ipv6/ip6_fib.c       |  5 -----
 net/ipv6/route.c         |  5 +++++
 net/netlink/af_netlink.c |  5 +++++
 10 files changed, 60 insertions(+), 12 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index ad1bd13cd34c..da36cdb410bb 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -643,6 +643,12 @@ struct bpf_jit_poke_descriptor {
 	u16 reason;
 };
 
+/* reg_type info for ctx arguments */
+struct bpf_ctx_arg_aux {
+	u32 offset;
+	enum bpf_reg_type reg_type;
+};
+
 struct bpf_prog_aux {
 	atomic64_t refcnt;
 	u32 used_map_cnt;
@@ -654,12 +660,13 @@ struct bpf_prog_aux {
 	u32 func_cnt; /* used by non-func prog as the number of func progs */
 	u32 func_idx; /* 0 for non-func prog, the index in func array for func prog */
 	u32 attach_btf_id; /* in-kernel BTF type id to attach to */
+	u32 ctx_arg_info_size;
+	struct bpf_ctx_arg_aux *ctx_arg_info;
 	struct bpf_prog *linked_prog;
 	bool verifier_zext; /* Zero extensions has been inserted by verifier. */
 	bool offload_requested;
 	bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
 	bool func_proto_unreliable;
-	bool btf_id_or_null_non0_off;
 	enum bpf_tramp_prog_type trampoline_prog_type;
 	struct bpf_trampoline *trampoline;
 	struct hlist_node tramp_hlist;
@@ -1139,12 +1146,15 @@ int bpf_obj_get_user(const char __user *pathname, int flags);
 typedef int (*bpf_iter_init_seq_priv_t)(void *private_data);
 typedef void (*bpf_iter_fini_seq_priv_t)(void *private_data);
 
+#define BPF_ITER_CTX_ARG_MAX 2
 struct bpf_iter_reg {
 	const char *target;
 	const struct seq_operations *seq_ops;
 	bpf_iter_init_seq_priv_t init_seq_private;
 	bpf_iter_fini_seq_priv_t fini_seq_private;
 	u32 seq_priv_size;
+	u32 ctx_arg_info_size;
+	struct bpf_ctx_arg_aux ctx_arg_info[BPF_ITER_CTX_ARG_MAX];
 };
 
 struct bpf_iter_meta {
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 80262d2980f5..870b646c5797 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -540,6 +540,13 @@ static inline bool fib6_metric_locked(struct fib6_info *f6i, int metric)
 	return !!(f6i->fib6_metrics->metrics[RTAX_LOCK - 1] & (1 << metric));
 }
 
+#if IS_BUILTIN(CONFIG_IPV6) && defined(CONFIG_BPF_SYSCALL)
+struct bpf_iter__ipv6_route {
+	__bpf_md_ptr(struct bpf_iter_meta *, meta);
+	__bpf_md_ptr(struct fib6_info *, rt);
+};
+#endif
+
 #ifdef CONFIG_IPV6_MULTIPLE_TABLES
 static inline bool fib6_has_custom_rules(const struct net *net)
 {
diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
index 041f97dcec39..a6db83f41c50 100644
--- a/kernel/bpf/bpf_iter.c
+++ b/kernel/bpf/bpf_iter.c
@@ -305,6 +305,11 @@ bool bpf_iter_prog_supported(struct bpf_prog *prog)
 	}
 	mutex_unlock(&targets_mutex);
 
+	if (supported) {
+		prog->aux->ctx_arg_info_size = tinfo->reg_info->ctx_arg_info_size;
+		prog->aux->ctx_arg_info = tinfo->reg_info->ctx_arg_info;
+	}
+
 	return supported;
 }
 
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index dcd233139294..ec587628a02f 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3694,7 +3694,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 	struct bpf_verifier_log *log = info->log;
 	const struct btf_param *args;
 	u32 nr_args, arg;
-	int ret;
+	int i, ret;
 
 	if (off % 8) {
 		bpf_log(log, "func '%s' offset %d is not multiple of 8\n",
@@ -3790,10 +3790,15 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 		return true;
 
 	/* this is a pointer to another type */
-	if (off != 0 && prog->aux->btf_id_or_null_non0_off)
-		info->reg_type = PTR_TO_BTF_ID_OR_NULL;
-	else
-		info->reg_type = PTR_TO_BTF_ID;
+	info->reg_type = PTR_TO_BTF_ID;
+	for (i = 0; i < prog->aux->ctx_arg_info_size; i++) {
+		struct bpf_ctx_arg_aux *ctx_arg_info = &prog->aux->ctx_arg_info[i];
+
+		if (ctx_arg_info->offset == off) {
+			info->reg_type = ctx_arg_info->reg_type;
+			break;
+		}
+	}
 
 	if (tgt_prog) {
 		ret = btf_translate_to_vmlinux(log, btf, t, tgt_prog->type, arg);
diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
index 545c7dbb13c9..66e1691e7ee7 100644
--- a/kernel/bpf/map_iter.c
+++ b/kernel/bpf/map_iter.c
@@ -87,6 +87,11 @@ static struct bpf_iter_reg bpf_map_reg_info = {
 	.init_seq_private	= NULL,
 	.fini_seq_private	= NULL,
 	.seq_priv_size		= sizeof(struct bpf_iter_seq_map_info),
+	.ctx_arg_info_size	= 1,
+	.ctx_arg_info		= {
+		{ offsetof(struct bpf_iter__bpf_map, map),
+		  PTR_TO_BTF_ID_OR_NULL },
+	},
 };
 
 static int __init bpf_map_iter_init(void)
diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index ed0b1d6ce688..3621e1979c39 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -312,6 +312,11 @@ static struct bpf_iter_reg task_reg_info = {
 	.init_seq_private	= init_seq_pidns,
 	.fini_seq_private	= fini_seq_pidns,
 	.seq_priv_size		= sizeof(struct bpf_iter_seq_task_info),
+	.ctx_arg_info_size	= 1,
+	.ctx_arg_info		= {
+		{ offsetof(struct bpf_iter__task, task),
+		  PTR_TO_BTF_ID_OR_NULL },
+	},
 };
 
 static struct bpf_iter_reg task_file_reg_info = {
@@ -320,6 +325,13 @@ static struct bpf_iter_reg task_file_reg_info = {
 	.init_seq_private	= init_seq_pidns,
 	.fini_seq_private	= fini_seq_pidns,
 	.seq_priv_size		= sizeof(struct bpf_iter_seq_task_file_info),
+	.ctx_arg_info_size	= 2,
+	.ctx_arg_info		= {
+		{ offsetof(struct bpf_iter__task_file, task),
+		  PTR_TO_BTF_ID_OR_NULL },
+		{ offsetof(struct bpf_iter__task_file, file),
+		  PTR_TO_BTF_ID_OR_NULL },
+	},
 };
 
 static int __init task_iter_init(void)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2a1826c76bb6..a3f2af756fd6 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10652,7 +10652,6 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 		prog->aux->attach_func_proto = t;
 		if (!bpf_iter_prog_supported(prog))
 			return -EINVAL;
-		prog->aux->btf_id_or_null_non0_off = true;
 		ret = btf_distill_func_proto(&env->log, btf, t,
 					     tname, &fmodel);
 		return ret;
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index a1fcc0ca21af..250ff52c674e 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -2638,11 +2638,6 @@ static void ipv6_route_native_seq_stop(struct seq_file *seq, void *v)
 }
 
 #if IS_BUILTIN(CONFIG_IPV6) && defined(CONFIG_BPF_SYSCALL)
-struct bpf_iter__ipv6_route {
-	__bpf_md_ptr(struct bpf_iter_meta *, meta);
-	__bpf_md_ptr(struct fib6_info *, rt);
-};
-
 static int ipv6_route_prog_seq_show(struct bpf_prog *prog,
 				    struct bpf_iter_meta *meta,
 				    void *v)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index bb8581f9b448..94b6aaa84db3 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -6403,6 +6403,11 @@ static struct bpf_iter_reg ipv6_route_reg_info = {
 	.init_seq_private	= bpf_iter_init_seq_net,
 	.fini_seq_private	= bpf_iter_fini_seq_net,
 	.seq_priv_size		= sizeof(struct ipv6_route_iter),
+	.ctx_arg_info_size	= 1,
+	.ctx_arg_info		= {
+		{ offsetof(struct bpf_iter__ipv6_route, rt),
+		  PTR_TO_BTF_ID_OR_NULL },
+	},
 };
 
 static int __init bpf_iter_register(void)
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 1e2f5ab8c7d7..3fe1979c0088 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2809,6 +2809,11 @@ static struct bpf_iter_reg netlink_reg_info = {
 	.init_seq_private	= bpf_iter_init_seq_net,
 	.fini_seq_private	= bpf_iter_fini_seq_net,
 	.seq_priv_size		= sizeof(struct nl_seq_iter),
+	.ctx_arg_info_size	= 1,
+	.ctx_arg_info		= {
+		{ offsetof(struct bpf_iter__netlink, sk),
+		  PTR_TO_BTF_ID_OR_NULL },
+	},
 };
 
 static int __init bpf_iter_register(void)
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH bpf-next 8/8] samples/bpf: remove compiler warnings
  2020-05-12 15:52 [PATCH bpf-next 0/8] misc fixes for bpf_iter Yonghong Song
                   ` (6 preceding siblings ...)
  2020-05-12 15:52 ` [PATCH bpf-next 7/8] bpf: enable bpf_iter targets registering ctx argument types Yonghong Song
@ 2020-05-12 15:52 ` Yonghong Song
  2020-05-12 22:30   ` Andrii Nakryiko
  7 siblings, 1 reply; 16+ messages in thread
From: Yonghong Song @ 2020-05-12 15:52 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, Martin KaFai Lau, netdev
  Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team

Commit 5fbc220862fc ("tools/libpf: Add offsetof/container_of macro
in bpf_helpers.h") added macros offsetof/container_of to
bpf_helpers.h. Unfortunately, it caused compilation warnings
below for a few samples/bpf programs:
  In file included from /data/users/yhs/work/net-next/samples/bpf/sockex2_kern.c:4:
  In file included from /data/users/yhs/work/net-next/include/uapi/linux/in.h:24:
  In file included from /data/users/yhs/work/net-next/include/linux/socket.h:8:
  In file included from /data/users/yhs/work/net-next/include/linux/uio.h:8:
  /data/users/yhs/work/net-next/include/linux/kernel.h:992:9: warning: 'container_of' macro redefined [-Wmacro-redefined]
          ^
  /data/users/yhs/work/net-next/tools/lib/bpf/bpf_helpers.h:46:9: note: previous definition is here
          ^
  1 warning generated.
    CLANG-bpf  samples/bpf/sockex3_kern.o

In all these cases, bpf_helpers.h is included first, followed by other
standard headers. The macro container_of is defined unconditionally
in kernel.h, causing the compiler warning.

The fix is to move bpf_helpers.h after standard headers.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 samples/bpf/offwaketime_kern.c | 4 ++--
 samples/bpf/sockex2_kern.c     | 4 ++--
 samples/bpf/sockex3_kern.c     | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/samples/bpf/offwaketime_kern.c b/samples/bpf/offwaketime_kern.c
index c4ec10dbfc3b..d459f73412a4 100644
--- a/samples/bpf/offwaketime_kern.c
+++ b/samples/bpf/offwaketime_kern.c
@@ -5,12 +5,12 @@
  * License as published by the Free Software Foundation.
  */
 #include <uapi/linux/bpf.h>
-#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_tracing.h>
 #include <uapi/linux/ptrace.h>
 #include <uapi/linux/perf_event.h>
 #include <linux/version.h>
 #include <linux/sched.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
 
 #define _(P) ({typeof(P) val; bpf_probe_read(&val, sizeof(val), &P); val;})
 
diff --git a/samples/bpf/sockex2_kern.c b/samples/bpf/sockex2_kern.c
index a41dd520bc53..b7997541f7ee 100644
--- a/samples/bpf/sockex2_kern.c
+++ b/samples/bpf/sockex2_kern.c
@@ -1,12 +1,12 @@
 #include <uapi/linux/bpf.h>
-#include <bpf/bpf_helpers.h>
-#include "bpf_legacy.h"
 #include <uapi/linux/in.h>
 #include <uapi/linux/if.h>
 #include <uapi/linux/if_ether.h>
 #include <uapi/linux/ip.h>
 #include <uapi/linux/ipv6.h>
 #include <uapi/linux/if_tunnel.h>
+#include <bpf/bpf_helpers.h>
+#include "bpf_legacy.h"
 #define IP_MF		0x2000
 #define IP_OFFSET	0x1FFF
 
diff --git a/samples/bpf/sockex3_kern.c b/samples/bpf/sockex3_kern.c
index 36d4dac23549..779a5249c418 100644
--- a/samples/bpf/sockex3_kern.c
+++ b/samples/bpf/sockex3_kern.c
@@ -5,8 +5,6 @@
  * License as published by the Free Software Foundation.
  */
 #include <uapi/linux/bpf.h>
-#include <bpf/bpf_helpers.h>
-#include "bpf_legacy.h"
 #include <uapi/linux/in.h>
 #include <uapi/linux/if.h>
 #include <uapi/linux/if_ether.h>
@@ -14,6 +12,8 @@
 #include <uapi/linux/ipv6.h>
 #include <uapi/linux/if_tunnel.h>
 #include <uapi/linux/mpls.h>
+#include <bpf/bpf_helpers.h>
+#include "bpf_legacy.h"
 #define IP_MF		0x2000
 #define IP_OFFSET	0x1FFF
 
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* Re: [PATCH bpf-next 5/8] bpf: net: refactor bpf_iter target registration
  2020-05-12 15:52 ` [PATCH bpf-next 5/8] bpf: net: refactor bpf_iter target registration Yonghong Song
@ 2020-05-12 22:19   ` Andrii Nakryiko
  0 siblings, 0 replies; 16+ messages in thread
From: Andrii Nakryiko @ 2020-05-12 22:19 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Andrii Nakryiko, bpf, Martin KaFai Lau, Networking,
	Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Tue, May 12, 2020 at 8:56 AM Yonghong Song <yhs@fb.com> wrote:
>
> Currently bpf_iter_reg_target takes parameters from target
> and allocates memory to save them. This is really not
> necessary, esp. in the future we may grow information
> passed from targets to bpf_iter manager.
>
> The patch refactors the code so target reg_info
> becomes static and bpf_iter manager can just take
> a reference to it.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  kernel/bpf/bpf_iter.c    | 29 +++++++++++------------------
>  kernel/bpf/map_iter.c    | 18 +++++++++---------
>  kernel/bpf/task_iter.c   | 30 ++++++++++++++++--------------
>  net/ipv6/route.c         | 18 +++++++++---------
>  net/netlink/af_netlink.c | 18 +++++++++---------
>  5 files changed, 54 insertions(+), 59 deletions(-)
>
> diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
> index b0c8b3bdf3b0..1d203dc7afe2 100644
> --- a/kernel/bpf/bpf_iter.c
> +++ b/kernel/bpf/bpf_iter.c
> @@ -8,11 +8,7 @@
>
>  struct bpf_iter_target_info {
>         struct list_head list;
> -       const char *target;
> -       const struct seq_operations *seq_ops;
> -       bpf_iter_init_seq_priv_t init_seq_private;
> -       bpf_iter_fini_seq_priv_t fini_seq_private;
> -       u32 seq_priv_size;
> +       struct bpf_iter_reg *reg_info;
>         u32 btf_id;     /* cached value */
>  };
>
> @@ -224,8 +220,8 @@ static int iter_release(struct inode *inode, struct file *file)
>         iter_priv = container_of(seq->private, struct bpf_iter_priv_data,
>                                  target_private);
>
> -       if (iter_priv->tinfo->fini_seq_private)
> -               iter_priv->tinfo->fini_seq_private(seq->private);
> +       if (iter_priv->tinfo->reg_info->fini_seq_private)
> +               iter_priv->tinfo->reg_info->fini_seq_private(seq->private);
>
>         bpf_prog_put(iter_priv->prog);
>         seq->private = iter_priv;
> @@ -248,11 +244,7 @@ int bpf_iter_reg_target(struct bpf_iter_reg *reg_info)

const struct bpf_iter_reg *? Can you please also add a comment that
passed struct is supposed to be static variable and live forever (so
not a dynamically allocated nor a stack variable)?

Also all the static struct bpf_iter_reg below should be marked const?

[...]

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH bpf-next 2/8] bpf: change btf_iter func proto prefix to "bpf_iter_"
  2020-05-12 15:52 ` [PATCH bpf-next 2/8] bpf: change btf_iter func proto prefix to "bpf_iter_" Yonghong Song
@ 2020-05-12 22:21   ` Andrii Nakryiko
  0 siblings, 0 replies; 16+ messages in thread
From: Andrii Nakryiko @ 2020-05-12 22:21 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Andrii Nakryiko, bpf, Martin KaFai Lau, Networking,
	Alexei Starovoitov, Daniel Borkmann, Kernel Team,
	Alexei Starovoitov

On Tue, May 12, 2020 at 8:54 AM Yonghong Song <yhs@fb.com> wrote:
>
> This is to be consistent with tracing and lsm programs
> which have prefix "bpf_trace_" and "bpf_lsm_" respectively.
>
> Suggested-by: Alexei Starovoitov <ast@kernel.org>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>


[...]

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH bpf-next 3/8] bpf: add comments to interpret bpf_prog return values
  2020-05-12 15:52 ` [PATCH bpf-next 3/8] bpf: add comments to interpret bpf_prog return values Yonghong Song
@ 2020-05-12 22:21   ` Andrii Nakryiko
  0 siblings, 0 replies; 16+ messages in thread
From: Andrii Nakryiko @ 2020-05-12 22:21 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Andrii Nakryiko, bpf, Martin KaFai Lau, Networking,
	Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Tue, May 12, 2020 at 8:53 AM Yonghong Song <yhs@fb.com> wrote:
>
> Add a short comment in bpf_iter_run_prog() function to
> explain how bpf_prog return value is converted to
> seq_ops->show() return value:
>   bpf_prog return           seq_ops()->show() return
>      0                         0
>      1                         -EAGAIN
>
> When show() return value is -EAGAIN, the current
> bpf_seq_read() will end. If the current seq_file buffer
> is empty, -EAGAIN will return to user space. Otherwise,
> the buffer will be copied to user space.
> In both cases, the next bpf_seq_read() call will
> try to show the same object which returned -EAGAIN
> previously.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

[...]

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH bpf-next 4/8] bpf: add WARN_ONCE if bpf_seq_read show() return a positive number
  2020-05-12 15:52 ` [PATCH bpf-next 4/8] bpf: add WARN_ONCE if bpf_seq_read show() return a positive number Yonghong Song
@ 2020-05-12 22:23   ` Andrii Nakryiko
  0 siblings, 0 replies; 16+ messages in thread
From: Andrii Nakryiko @ 2020-05-12 22:23 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Andrii Nakryiko, bpf, Martin KaFai Lau, Networking,
	Alexei Starovoitov, Daniel Borkmann, Kernel Team,
	Alexei Starovoitov

On Tue, May 12, 2020 at 8:54 AM Yonghong Song <yhs@fb.com> wrote:
>
> In seq_read() implementation, a positive integer return value
> of seq_ops->show() indicates that the current object seq_file
> buffer is discarded and next object should be checked.
> bpf_seq_read() implemented in a similar way if show()
> returns a positive integer value.
>
> But for bpf_seq_read(), show() didn't return positive integer for
> all currently supported targets. Let us add a WARN_ONCE for
> such cases so we can get an alert when things are changed.
>
> Suggested-by: Alexei Starovoitov <ast@kernel.org>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  kernel/bpf/bpf_iter.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
> index 0a45a6cdfabd..b0c8b3bdf3b0 100644
> --- a/kernel/bpf/bpf_iter.c
> +++ b/kernel/bpf/bpf_iter.c
> @@ -120,6 +120,7 @@ static ssize_t bpf_seq_read(struct file *file, char __user *buf, size_t size,
>
>         err = seq->op->show(seq, p);
>         if (err > 0) {
> +               WARN_ONCE(1, "seq_ops->show() returns %d\n", err);

This makes it look like it's a bug or non-safe, honestly. I'd drop the
warning altogether, but if not, probably leaving a comment explaining
why we added WARN_ONCE here and that it's ok to remove it would be
good.

>                 /* object is skipped, decrease seq_num, so next
>                  * valid object can reuse the same seq_num.
>                  */
> @@ -156,6 +157,7 @@ static ssize_t bpf_seq_read(struct file *file, char __user *buf, size_t size,
>
>                 err = seq->op->show(seq, p);
>                 if (err > 0) {
> +                       WARN_ONCE(1, "seq_ops->show() returns %d\n", err);
>                         bpf_iter_dec_seq_num(seq);
>                         seq->count = offs;
>                 } else if (err < 0 || seq_has_overflowed(seq)) {
> --
> 2.24.1
>

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH bpf-next 6/8] bpf: change func bpf_iter_unreg_target() signature
  2020-05-12 15:52 ` [PATCH bpf-next 6/8] bpf: change func bpf_iter_unreg_target() signature Yonghong Song
@ 2020-05-12 22:24   ` Andrii Nakryiko
  0 siblings, 0 replies; 16+ messages in thread
From: Andrii Nakryiko @ 2020-05-12 22:24 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Andrii Nakryiko, bpf, Martin KaFai Lau, Networking,
	Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Tue, May 12, 2020 at 8:53 AM Yonghong Song <yhs@fb.com> wrote:
>
> Change func bpf_iter_unreg_target() parameter from target
> name to target reg_info, similar to bpf_iter_reg_target().
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  include/linux/bpf.h   | 2 +-
>  kernel/bpf/bpf_iter.c | 4 ++--
>  net/ipv6/route.c      | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index ab94dfd8826f..ad1bd13cd34c 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1154,7 +1154,7 @@ struct bpf_iter_meta {
>  };
>
>  int bpf_iter_reg_target(struct bpf_iter_reg *reg_info);
> -void bpf_iter_unreg_target(const char *target);
> +void bpf_iter_unreg_target(struct bpf_iter_reg *reg_info);
>  bool bpf_iter_prog_supported(struct bpf_prog *prog);
>  int bpf_iter_link_attach(const union bpf_attr *attr, struct bpf_prog *prog);
>  int bpf_iter_new_fd(struct bpf_link *link);
> diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
> index 1d203dc7afe2..041f97dcec39 100644
> --- a/kernel/bpf/bpf_iter.c
> +++ b/kernel/bpf/bpf_iter.c
> @@ -254,14 +254,14 @@ int bpf_iter_reg_target(struct bpf_iter_reg *reg_info)
>         return 0;
>  }
>
> -void bpf_iter_unreg_target(const char *target)
> +void bpf_iter_unreg_target(struct bpf_iter_reg *reg_info)

const, otherwise LGTM:

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  {
>         struct bpf_iter_target_info *tinfo;
>         bool found = false;
>
>         mutex_lock(&targets_mutex);
>         list_for_each_entry(tinfo, &targets, list) {
> -               if (!strcmp(target, tinfo->reg_info->target)) {
> +               if (reg_info == tinfo->reg_info) {
>                         list_del(&tinfo->list);
>                         kfree(tinfo);
>                         found = true;
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 48e8752d9ad9..bb8581f9b448 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -6412,7 +6412,7 @@ static int __init bpf_iter_register(void)
>
>  static void bpf_iter_unregister(void)
>  {
> -       bpf_iter_unreg_target("ipv6_route");
> +       bpf_iter_unreg_target(&ipv6_route_reg_info);
>  }
>  #endif
>  #endif
> --
> 2.24.1
>

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH bpf-next 7/8] bpf: enable bpf_iter targets registering ctx argument types
  2020-05-12 15:52 ` [PATCH bpf-next 7/8] bpf: enable bpf_iter targets registering ctx argument types Yonghong Song
@ 2020-05-12 22:29   ` Andrii Nakryiko
  0 siblings, 0 replies; 16+ messages in thread
From: Andrii Nakryiko @ 2020-05-12 22:29 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Andrii Nakryiko, bpf, Martin KaFai Lau, Networking,
	Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Tue, May 12, 2020 at 8:54 AM Yonghong Song <yhs@fb.com> wrote:
>
> Commit b121b341e598 ("bpf: Add PTR_TO_BTF_ID_OR_NULL
> support") adds a field btf_id_or_null_non0_off to
> bpf_prog->aux structure to indicate that the
> first ctx argument is PTR_TO_BTF_ID reg_type and
> all others are PTR_TO_BTF_ID_OR_NULL.
> This approach does not really scale if we have
> other different reg types in the future, e.g.,
> a pointer to a buffer.
>
> This patch enables bpf_iter targets registering ctx argument
> reg types which may be different from the default one.
> For example, for pointers to structures, the default reg_type
> is PTR_TO_BTF_ID for tracing program. The target can register
> a particular pointer type as PTR_TO_BTF_ID_OR_NULL which can
> be used by the verifier to enforce accesses.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---

LGTM. It's cleaner approach than btf_id_or_null_non0_off, of course.
Having field annotations would be even better, but BTF doesn't have
attributes (yet).

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  include/linux/bpf.h      | 12 +++++++++++-
>  include/net/ip6_fib.h    |  7 +++++++
>  kernel/bpf/bpf_iter.c    |  5 +++++
>  kernel/bpf/btf.c         | 15 ++++++++++-----
>  kernel/bpf/map_iter.c    |  5 +++++
>  kernel/bpf/task_iter.c   | 12 ++++++++++++
>  kernel/bpf/verifier.c    |  1 -
>  net/ipv6/ip6_fib.c       |  5 -----
>  net/ipv6/route.c         |  5 +++++
>  net/netlink/af_netlink.c |  5 +++++
>  10 files changed, 60 insertions(+), 12 deletions(-)
>

[...]

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH bpf-next 8/8] samples/bpf: remove compiler warnings
  2020-05-12 15:52 ` [PATCH bpf-next 8/8] samples/bpf: remove compiler warnings Yonghong Song
@ 2020-05-12 22:30   ` Andrii Nakryiko
  0 siblings, 0 replies; 16+ messages in thread
From: Andrii Nakryiko @ 2020-05-12 22:30 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Andrii Nakryiko, bpf, Martin KaFai Lau, Networking,
	Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Tue, May 12, 2020 at 8:54 AM Yonghong Song <yhs@fb.com> wrote:
>
> Commit 5fbc220862fc ("tools/libpf: Add offsetof/container_of macro
> in bpf_helpers.h") added macros offsetof/container_of to
> bpf_helpers.h. Unfortunately, it caused compilation warnings
> below for a few samples/bpf programs:
>   In file included from /data/users/yhs/work/net-next/samples/bpf/sockex2_kern.c:4:
>   In file included from /data/users/yhs/work/net-next/include/uapi/linux/in.h:24:
>   In file included from /data/users/yhs/work/net-next/include/linux/socket.h:8:
>   In file included from /data/users/yhs/work/net-next/include/linux/uio.h:8:
>   /data/users/yhs/work/net-next/include/linux/kernel.h:992:9: warning: 'container_of' macro redefined [-Wmacro-redefined]
>           ^
>   /data/users/yhs/work/net-next/tools/lib/bpf/bpf_helpers.h:46:9: note: previous definition is here
>           ^
>   1 warning generated.
>     CLANG-bpf  samples/bpf/sockex3_kern.o
>
> In all these cases, bpf_helpers.h is included first, followed by other
> standard headers. The macro container_of is defined unconditionally
> in kernel.h, causing the compiler warning.
>
> The fix is to move bpf_helpers.h after standard headers.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---

LGTM.

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  samples/bpf/offwaketime_kern.c | 4 ++--
>  samples/bpf/sockex2_kern.c     | 4 ++--
>  samples/bpf/sockex3_kern.c     | 4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
>

[...]

^ permalink raw reply	[flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-05-12 22:30 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-12 15:52 [PATCH bpf-next 0/8] misc fixes for bpf_iter Yonghong Song
2020-05-12 15:52 ` [PATCH bpf-next 1/8] tools/bpf: selftests : explain bpf_iter test failures with llvm 10.0.0 Yonghong Song
2020-05-12 15:52 ` [PATCH bpf-next 2/8] bpf: change btf_iter func proto prefix to "bpf_iter_" Yonghong Song
2020-05-12 22:21   ` Andrii Nakryiko
2020-05-12 15:52 ` [PATCH bpf-next 3/8] bpf: add comments to interpret bpf_prog return values Yonghong Song
2020-05-12 22:21   ` Andrii Nakryiko
2020-05-12 15:52 ` [PATCH bpf-next 4/8] bpf: add WARN_ONCE if bpf_seq_read show() return a positive number Yonghong Song
2020-05-12 22:23   ` Andrii Nakryiko
2020-05-12 15:52 ` [PATCH bpf-next 5/8] bpf: net: refactor bpf_iter target registration Yonghong Song
2020-05-12 22:19   ` Andrii Nakryiko
2020-05-12 15:52 ` [PATCH bpf-next 6/8] bpf: change func bpf_iter_unreg_target() signature Yonghong Song
2020-05-12 22:24   ` Andrii Nakryiko
2020-05-12 15:52 ` [PATCH bpf-next 7/8] bpf: enable bpf_iter targets registering ctx argument types Yonghong Song
2020-05-12 22:29   ` Andrii Nakryiko
2020-05-12 15:52 ` [PATCH bpf-next 8/8] samples/bpf: remove compiler warnings Yonghong Song
2020-05-12 22:30   ` Andrii Nakryiko

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