Re: [PATCH 1/2] bpf: Remove struct bpf_verifier_env argument from print_bpf_insn

[Quentin Monnet <quentin.monnet@netronome.com>]

2018-03-22 14:32 UTC+0100 ~ Jiri Olsa <jolsa@redhat.com>
> On Thu, Mar 22, 2018 at 10:34:18AM +0100, Daniel Borkmann wrote:
>> On 03/21/2018 07:37 PM, Jiri Olsa wrote:
>>> On Wed, Mar 21, 2018 at 05:25:33PM +0000, Quentin Monnet wrote:
>>>> 2018-03-21 16:02 UTC+0100 ~ Jiri Olsa <jolsa@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 <jolsa@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?
>>>
>>> yea, but as it was pointer for pointer switch I did not
>>> see any problem with that.. I'll check more
>>
>> Ok, holding off for now until we have clarification. Other option could also
>> be to make it void *private_data everywhere and for the kernel writer then
>> do struct bpf_verifier_env *env = private_data.
> 
> can't find much info about the alias behaviour for this
> case.. so how about having separate function for the
> print_cb like below.. I still need to test it

I have nothing against splitting the function. This is a solution I
considered for verbose() and bpf_verifier_log_write(), before switching
to function alias (on Daniel's suggestion) because the code would be
identical for the two separate functions at that time. But with your
change they would not have the same signature anymore, so it sounds
reasonable. Just a note below...

> 
> thanks,
> jirka
> 
> 
> ---
>  kernel/bpf/disasm.c   | 52 +++++++++++++++++++++++++--------------------------
>  kernel/bpf/disasm.h   |  5 +----
>  kernel/bpf/verifier.c | 41 +++++++++++++++++++++++++++++-----------
>  3 files changed, 57 insertions(+), 41 deletions(-)
> 

[...]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index e9f7c20691c1..69bf7590877c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -168,23 +168,16 @@ struct bpf_call_arg_meta {
>  
>  static DEFINE_MUTEX(bpf_verifier_lock);
>  
> -/* log_level controls verbosity level of eBPF verifier.
> - * bpf_verifier_log_write() is used to dump the verification trace to the log,
> - * so the user can figure out what's wrong with the program
> - */
> -__printf(2, 3) void bpf_verifier_log_write(struct bpf_verifier_env *env,
> -					   const char *fmt, ...)
> +static void log_write(struct bpf_verifier_env *env, const char *fmt,
> +		      va_list args)
>  {
>  	struct bpf_verifer_log *log = &env->log;
>  	unsigned int n;
> -	va_list args;
>  
>  	if (!log->level || !log->ubuf || bpf_verifier_log_full(log))
>  		return;
>  
> -	va_start(args, fmt);
>  	n = vscnprintf(log->kbuf, BPF_VERIFIER_TMP_LOG_SIZE, fmt, args);
> -	va_end(args);
>  
>  	WARN_ONCE(n >= BPF_VERIFIER_TMP_LOG_SIZE - 1,
>  		  "verifier log line truncated - local buffer too short\n");
> @@ -197,7 +190,32 @@ __printf(2, 3) void bpf_verifier_log_write(struct bpf_verifier_env *env,
>  	else
>  		log->ubuf = NULL;
>  }
> +
> +/* log_level controls verbosity level of eBPF verifier.
> + * bpf_verifier_log_write() is used to dump the verification trace to the log,
> + * so the user can figure out what's wrong with the program
> + */
> +__printf(2, 3) void bpf_verifier_log_write(struct bpf_verifier_env *env,
> +					   const char *fmt, ...)
> +{
> +	va_list args;
> +
> +	va_start(args, fmt);
> +	log_write(env, fmt, args);
> +	va_end(args);
> +}
>  EXPORT_SYMBOL_GPL(bpf_verifier_log_write);
> +
> +__printf(2, 3) static void print_ins(void *private_data,
> +				     const char *fmt, ...)

Unless I am mistaken, you could just call this one "verbose()" and
simply remove the function alias?

> +{
> +	va_list args;
> +
> +	va_start(args, fmt);
> +	log_write(private_data, fmt, args);
> +	va_end(args);
> +}
> +
>  /* Historically bpf_verifier_log_write was called verbose, but the name was too
>   * generic for symbol export. The function was renamed, but not the calls in
>   * the verifier to avoid complicating backports. Hence the alias below.
> @@ -4599,11 +4617,12 @@ static int do_check(struct bpf_verifier_env *env)
>  
>  		if (env->log.level) {
>  			const struct bpf_insn_cbs cbs = {
> -				.cb_print	= verbose,
> +				.cb_print	= print_ins,
> +				.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)) {
>