[PATCH v2 bpf-next 0/2] bpf: keep track of verifier insn_processed

This is a followup to discussion around RFC patchset "bpf: keep track of
prog verification stats" [0]. The RFC elaborates on my usecase, but to
summarize: keeping track of verifier stats for programs as they - and
the kernels they run on - change over time can help developers of
individual programs and BPF kernel folks.

The RFC added a verif_stats to the uapi which contained most of the info
which verifier prints currently. Feedback here was to avoid polluting
uapi with stats that might be meaningless after major changes to the
verifier, but that insn_processed or conceptually similar number would
exist in the long term and was safe to expose.

So let's expose just insn_processed via bpf_prog_info and fdinfo for now
and explore good ways of getting more complicated stats in the future.

[0] https://lore.kernel.org/bpf/20210920151112.3770991-1-davemarchevsky@fb.com/

v1->v2:
  * Rename uapi field from insn_processed to verified_insns [Daniel]
  * use 31 bits of existing bitfield space in bpf_prog_info [Daniel]
  * change underlying type from 64-> 32 bits [Daniel]

Dave Marchevsky (2):
  bpf: add verified_insns to bpf_prog_info and fdinfo
  selftests/bpf: add verif_stats test

 include/linux/bpf.h                           |  1 +
 include/uapi/linux/bpf.h                      |  2 +-
 kernel/bpf/syscall.c                          |  8 +++--
 kernel/bpf/verifier.c                         |  1 +
 tools/include/uapi/linux/bpf.h                |  2 +-
 .../selftests/bpf/prog_tests/verif_stats.c    | 31 +++++++++++++++++++
 6 files changed, 41 insertions(+), 4 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/verif_stats.c

-- 
2.30.2

This stat is currently printed in the verifier log and not stored
anywhere. To ease consumption of this data, add a field to bpf_prog_aux
so it can be exposed via BPF_OBJ_GET_INFO_BY_FD and fdinfo.

Signed-off-by: Dave Marchevsky <redacted>
---
 include/linux/bpf.h            | 1 +
 include/uapi/linux/bpf.h       | 2 +-
 kernel/bpf/syscall.c           | 8 ++++++--
 kernel/bpf/verifier.c          | 1 +
 tools/include/uapi/linux/bpf.h | 2 +-
 5 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index d604c8251d88..c93fd845a758 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h

@@ -887,6 +887,7 @@ struct bpf_prog_aux {
 	struct bpf_prog *prog;
 	struct user_struct *user;
 	u64 load_time; /* ns since boottime */
+	u32 verified_insns;
 	struct bpf_map *cgroup_storage[MAX_BPF_CGROUP_STORAGE_TYPE];
 	char name[BPF_OBJ_NAME_LEN];
 #ifdef CONFIG_SECURITY

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 6fc59d61937a..d053fc7e7995 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h

@@ -5591,7 +5591,7 @@ struct bpf_prog_info {
 	char name[BPF_OBJ_NAME_LEN];
 	__u32 ifindex;
 	__u32 gpl_compatible:1;
-	__u32 :31; /* alignment pad */
+	__u32 verified_insns:31;
 	__u64 netns_dev;
 	__u64 netns_ino;
 	__u32 nr_jited_ksyms;

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 4e50c0bfdb7d..5beb321b3b3b 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c

@@ -1848,7 +1848,8 @@ static void bpf_prog_show_fdinfo(struct seq_file *m, struct file *filp)
 		   "prog_id:\t%u\n"
 		   "run_time_ns:\t%llu\n"
 		   "run_cnt:\t%llu\n"
-		   "recursion_misses:\t%llu\n",
+		   "recursion_misses:\t%llu\n"
+		   "verified_insns:\t%u\n",
 		   prog->type,
 		   prog->jited,
 		   prog_tag,

@@ -1856,7 +1857,8 @@ static void bpf_prog_show_fdinfo(struct seq_file *m, struct file *filp)
 		   prog->aux->id,
 		   stats.nsecs,
 		   stats.cnt,
-		   stats.misses);
+		   stats.misses,
+		   prog->aux->verified_insns);
 }
 #endif
 

@@ -3625,6 +3627,8 @@ static int bpf_prog_get_info_by_fd(struct file *file,
 	info.run_cnt = stats.cnt;
 	info.recursion_misses = stats.misses;
 
+	info.verified_insns = prog->aux->verified_insns;
+
 	if (!bpf_capable()) {
 		info.jited_prog_len = 0;
 		info.xlated_prog_len = 0;

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 20900a1bac12..81c7eecdc5d5 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c

@@ -14038,6 +14038,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr)
 
 	env->verification_time = ktime_get_ns() - start_time;
 	print_verification_stats(env);
+	env->prog->aux->verified_insns = env->insn_processed;
 
 	if (log->level && bpf_verifier_log_full(log))
 		ret = -ENOSPC;

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 6fc59d61937a..d053fc7e7995 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h

@@ -5591,7 +5591,7 @@ struct bpf_prog_info {
 	char name[BPF_OBJ_NAME_LEN];
 	__u32 ifindex;
 	__u32 gpl_compatible:1;
-	__u32 :31; /* alignment pad */
+	__u32 verified_insns:31;
 	__u64 netns_dev;
 	__u64 netns_ino;
 	__u32 nr_jited_ksyms;

-- 
2.30.2

verified_insns field was added to response of bpf_obj_get_info_by_fd
call on a prog. Confirm that it's being populated by loading a simple
program and asking for its info.

Signed-off-by: Dave Marchevsky <redacted>
---
 .../selftests/bpf/prog_tests/verif_stats.c    | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/verif_stats.c

diff --git a/tools/testing/selftests/bpf/prog_tests/verif_stats.c b/tools/testing/selftests/bpf/prog_tests/verif_stats.c
new file mode 100644
index 000000000000..42843db519e5
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/verif_stats.c

@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+
+#include <test_progs.h>
+
+#include "trace_vprintk.lskel.h"
+
+void test_verif_stats(void)
+{
+	__u32 len = sizeof(struct bpf_prog_info);
+	struct bpf_prog_info info = {};
+	struct trace_vprintk *skel;
+	int err;
+
+	skel = trace_vprintk__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "trace_vprintk__open_and_load"))
+		goto cleanup;
+
+	if (!ASSERT_GT(skel->progs.sys_enter.prog_fd, 0, "sys_enter_fd > 0"))
+		goto cleanup;
+
+	err = bpf_obj_get_info_by_fd(skel->progs.sys_enter.prog_fd, &info, &len);
+	if (!ASSERT_OK(err, "bpf_obj_get_info_by_fd"))
+		goto cleanup;
+
+	if (!ASSERT_GT((__u32)info.verified_insns, 0, "verified_insns"))
+		goto cleanup;
+
+cleanup:
+	trace_vprintk__destroy(skel);
+}

-- 
2.30.2

Dave Marchevsky wrote:

This is a followup to discussion around RFC patchset "bpf: keep track of
prog verification stats" [0]. The RFC elaborates on my usecase, but to
summarize: keeping track of verifier stats for programs as they - and
the kernels they run on - change over time can help developers of
individual programs and BPF kernel folks.

The RFC added a verif_stats to the uapi which contained most of the info
which verifier prints currently. Feedback here was to avoid polluting
uapi with stats that might be meaningless after major changes to the
verifier, but that insn_processed or conceptually similar number would
exist in the long term and was safe to expose.

So let's expose just insn_processed via bpf_prog_info and fdinfo for now
and explore good ways of getting more complicated stats in the future.

[0] https://lore.kernel.org/bpf/20210920151112.3770991-1-davemarchevsky@fb.com/

v1->v2:
  * Rename uapi field from insn_processed to verified_insns [Daniel]
  * use 31 bits of existing bitfield space in bpf_prog_info [Daniel]
  * change underlying type from 64-> 32 bits [Daniel]

Dave Marchevsky (2):
  bpf: add verified_insns to bpf_prog_info and fdinfo
  selftests/bpf: add verif_stats test

 include/linux/bpf.h                           |  1 +
 include/uapi/linux/bpf.h                      |  2 +-
 kernel/bpf/syscall.c                          |  8 +++--
 kernel/bpf/verifier.c                         |  1 +
 tools/include/uapi/linux/bpf.h                |  2 +-
 .../selftests/bpf/prog_tests/verif_stats.c    | 31 +++++++++++++++++++
 6 files changed, 41 insertions(+), 4 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/verif_stats.c

-- 
2.30.2

For the series.

Acked-by: John Fastabend <john.fastabend@gmail.com>

On Mon, Oct 11, 2021 at 1:54 PM Dave Marchevsky [off-list ref] wrote:

This stat is currently printed in the verifier log and not stored
anywhere. To ease consumption of this data, add a field to bpf_prog_aux
so it can be exposed via BPF_OBJ_GET_INFO_BY_FD and fdinfo.

Signed-off-by: Dave Marchevsky <redacted>
---
 include/linux/bpf.h            | 1 +
 include/uapi/linux/bpf.h       | 2 +-
 kernel/bpf/syscall.c           | 8 ++++++--
 kernel/bpf/verifier.c          | 1 +
 tools/include/uapi/linux/bpf.h | 2 +-
 5 files changed, 10 insertions(+), 4 deletions(-)

[...]

These 31 unused bits seem like a good place to add extra generic
flags, not new counters. E.g., like a sleepable flag. So I wonder if
it's better to use a dedicated u32 field for counters like
verified_insns and keep these reserved fields for boolean flags?

Daniel, I know you proposed to reuse those 31 bits. How strong do you
feel about this? For any other kind of counter we seem to be using a
complete dedicated integer field, so it would be consistent to keep
doing that?

Having a sleepable bit still seems like a good idea, btw :) but it's a
separate change from Dave's.

        __u64 netns_dev;
        __u64 netns_ino;
        __u32 nr_jited_ksyms;
--
2.30.2

On Mon, Oct 11, 2021 at 1:54 PM Dave Marchevsky [off-list ref] wrote:

verified_insns field was added to response of bpf_obj_get_info_by_fd
call on a prog. Confirm that it's being populated by loading a simple
program and asking for its info.

Signed-off-by: Dave Marchevsky <redacted>
---

one nit below

Acked-by: Andrii Nakryiko <andrii@kernel.org>

This can't fail if open_and_load succeeded. If it does happen due to
some bug, then bpf_obj_get_info_by_fd() below will fail.

+
+       err = bpf_obj_get_info_by_fd(skel->progs.sys_enter.prog_fd, &info, &len);
+       if (!ASSERT_OK(err, "bpf_obj_get_info_by_fd"))
+               goto cleanup;
+
+       if (!ASSERT_GT((__u32)info.verified_insns, 0, "verified_insns"))
+               goto cleanup;
+
+cleanup:
+       trace_vprintk__destroy(skel);
+}
--
2.30.2

On 10/18/21 5:22 PM, Andrii Nakryiko wrote:   

On Mon, Oct 11, 2021 at 1:54 PM Dave Marchevsky [off-list ref] wrote:

quoted

This stat is currently printed in the verifier log and not stored
anywhere. To ease consumption of this data, add a field to bpf_prog_aux
so it can be exposed via BPF_OBJ_GET_INFO_BY_FD and fdinfo.

Signed-off-by: Dave Marchevsky <redacted>
---
 include/linux/bpf.h            | 1 +
 include/uapi/linux/bpf.h       | 2 +-
 kernel/bpf/syscall.c           | 8 ++++++--
 kernel/bpf/verifier.c          | 1 +
 tools/include/uapi/linux/bpf.h | 2 +-
 5 files changed, 10 insertions(+), 4 deletions(-)

[...]

quoted

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 6fc59d61937a..d053fc7e7995 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h

@@ -5591,7 +5591,7 @@ struct bpf_prog_info {
        char name[BPF_OBJ_NAME_LEN];
        __u32 ifindex;
        __u32 gpl_compatible:1;
-       __u32 :31; /* alignment pad */
+       __u32 verified_insns:31;

These 31 unused bits seem like a good place to add extra generic
flags, not new counters. E.g., like a sleepable flag. So I wonder if
it's better to use a dedicated u32 field for counters like
verified_insns and keep these reserved fields for boolean flags?

Daniel, I know you proposed to reuse those 31 bits. How strong do you
feel about this? For any other kind of counter we seem to be using a
complete dedicated integer field, so it would be consistent to keep
doing that?

Having a sleepable bit still seems like a good idea, btw :) but it's a
separate change from Dave's.

Re: use padding vs new field, I don't have a strong feeling either way,
but if there are proper flags that could use that space in the near 
future, this combined with consistency with other counters leans me 
towards adding a new field.

Also, when using the bitfield space, clang complains about types in 
selftest assert:

tools/testing/selftests/bpf/prog_tests/verif_stats.c:23:17: error: ‘typeof’ applied to a bit-field
   23 |  if (!ASSERT_GT(info.verified_insns, 0, "verified_insns"))
      |                 ^~~~
./test_progs.h:227:9: note: in definition of macro ‘ASSERT_GT’
  227 |  typeof(actual) ___act = (actual);    \

Which necessitated a __u32 cast in this version of the patchset. Don't think
it would cause issues outside of this specific selftest, but worth noting.

Anyways, sent a v3 of the patchset with 'new field' and other comments
addressed.