2018-03-21 16:02 UTC+0100 ~ Jiri Olsa <jo...@kernel.org>
> We use print_bpf_insn in user space (bpftool and soon perf),
> so it'd be nice to keep it generic and strip it off the kernel
> struct bpf_verifier_env argument.
>
> This argument can be safely removed, because its users can
> use the struct bpf_insn_cbs::private_data to pass it.
>
> Signed-off-by: Jiri Olsa <jo...@kernel.org>
> ---
> kernel/bpf/disasm.c | 52
> +++++++++++++++++++++++++--------------------------
> kernel/bpf/disasm.h | 5 +----
> kernel/bpf/verifier.c | 6 +++---
> 3 files changed, 30 insertions(+), 33 deletions(-)
>
[...]
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c6eff108aa99..9f27d3fa7259 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -202,8 +202,7 @@ EXPORT_SYMBOL_GPL(bpf_verifier_log_write);
> * generic for symbol export. The function was renamed, but not the calls in
> * the verifier to avoid complicating backports. Hence the alias below.
> */
> -static __printf(2, 3) void verbose(struct bpf_verifier_env *env,
> - const char *fmt, ...)
> +static __printf(2, 3) void verbose(void *private_data, const char *fmt, ...)
> __attribute__((alias("bpf_verifier_log_write")));
Just as a note, verbose() will be aliased to a function whose prototype
differs (bpf_verifier_log_write() still expects a struct
bpf_verifier_env as its first argument). I am not so familiar with
function aliases, could this change be a concern?
Other than this the patch seems good to me.
Quentin
>
> static bool type_is_pkt_pointer(enum bpf_reg_type type)
> @@ -4601,10 +4600,11 @@ static int do_check(struct bpf_verifier_env *env)
> if (env->log.level) {
> const struct bpf_insn_cbs cbs = {
> .cb_print = verbose,
> + .private_data = env,
> };
>
> verbose(env, "%d: ", insn_idx);
> - print_bpf_insn(&cbs, env, insn, env->allow_ptr_leaks);
> + print_bpf_insn(&cbs, insn, env->allow_ptr_leaks);
> }
>
> if (bpf_prog_is_dev_bound(env->prog->aux)) {
>
