Re: [PATCH bpf-next 1/2] bpf: add insn_processed to bpf_prog_info and fdinfo

[Daniel Borkmann <daniel@iogearbox.net>]

On 10/7/21 10:09 AM, Dave Marchevsky 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 <davemarchevsky@fb.com>
> ---
>   include/linux/bpf.h            | 1 +
>   include/uapi/linux/bpf.h       | 1 +
>   kernel/bpf/syscall.c           | 8 ++++++--
>   kernel/bpf/verifier.c          | 1 +
>   tools/include/uapi/linux/bpf.h | 1 +
>   5 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index d604c8251d88..921ad62b892c 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 */
> +	u64 verif_insn_processed;

nit: why u64 and not u32?

>   	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..89be6ecf9204 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -5613,6 +5613,7 @@ struct bpf_prog_info {
>   	__u64 run_time_ns;
>   	__u64 run_cnt;
>   	__u64 recursion_misses;
> +	__u64 verif_insn_processed;

There's a '__u32 :31; /* alignment pad */' which could be reused. Given this
is uapi, I'd probably just name it 'insn_processed' or 'verified_insns' (maybe
the latter is more appropriate) to avoid abbreviation on verif_ which may not
be obvious.

>   } __attribute__((aligned(8)));
>   
>   struct bpf_map_info {
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 4e50c0bfdb7d..ea452ced2296 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"
> +		   "verif_insn_processed:\t%llu\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->verif_insn_processed);
>   }
>   #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.verif_insn_processed = prog->aux->verif_insn_processed;

Bit off-topic, but stack depth might be useful as well.

> +
>   	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..9ca301191d78 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->verif_insn_processed = 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..89be6ecf9204 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -5613,6 +5613,7 @@ struct bpf_prog_info {
>   	__u64 run_time_ns;
>   	__u64 run_cnt;
>   	__u64 recursion_misses;
> +	__u64 verif_insn_processed;
>   } __attribute__((aligned(8)));
>   
>   struct bpf_map_info {
>