netdev - Re: [PATCH bpf-next 13/14] bpf: export function to write into verifier log buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1e803eae-0048-8638-fb7d-0292100daeb3@iogearbox.net>
Date:   Wed, 10 Jan 2018 11:32:30 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        alexei.starovoitov@...il.com
Cc:     netdev@...r.kernel.org, oss-drivers@...ronome.com,
        Quentin Monnet <quentin.monnet@...ronome.com>
Subject: Re: [PATCH bpf-next 13/14] bpf: export function to write into
 verifier log buffer

On 01/10/2018 06:07 AM, Jakub Kicinski wrote:
> From: Quentin Monnet <quentin.monnet@...ronome.com>
> 
> Rename the BPF verifier `verbose()` to `bpf_verifier_log_write()` and
> export it, so that other components (in particular, drivers for BPF
> offload) can reuse the user buffer log to dump error messages at
> verification time.
> 
> Renaming `verbose()` was necessary in order to avoid a name so generic
> to be exported to the global namespace. However to prevent too much pain
> for backports, the calls to `verbose()` in the kernel BPF verifier were
> not changed. Instead, add a `#define verbose bpf_verifier_log_write`.
> Another solution could consist in making a wrapper around `verbose()`,
> but since it is a variadic function, I don't see a clean way without
> creating two identical wrappers, one for the verifier and one to export.
> I opted for a simple #define for now.
> 
> Signed-off-by: Quentin Monnet <quentin.monnet@...ronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> ---
>  include/linux/bpf_verifier.h |  3 +++
>  kernel/bpf/verifier.c        | 14 ++++++++++----
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 2feb218c001d..6b66cd1aa0b9 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -192,6 +192,9 @@ struct bpf_verifier_env {
>  	u32 subprog_cnt;
>  };
>  
> +__printf(2, 3) void bpf_verifier_log_write(struct bpf_verifier_env *env,
> +					   const char *fmt, ...);
> +
>  static inline struct bpf_reg_state *cur_regs(struct bpf_verifier_env *env)
>  {
>  	struct bpf_verifier_state *cur = env->cur_state;
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d921ab387b0b..e674944811d1 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -169,11 +169,11 @@ struct bpf_call_arg_meta {
>  static DEFINE_MUTEX(bpf_verifier_lock);
>  
>  /* log_level controls verbosity level of eBPF verifier.
> - * verbose() is used to dump the verification trace to the log, so the user
> - * can figure out what's wrong with the program
> + * 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
>   */
> -static __printf(2, 3) void verbose(struct bpf_verifier_env *env,
> -				   const char *fmt, ...)
> +__printf(2, 3) void bpf_verifier_log_write(struct bpf_verifier_env *env,
> +					   const char *fmt, ...)
>  {
>  	struct bpf_verifer_log *log = &env->log;
>  	unsigned int n;
> @@ -197,6 +197,12 @@ static __printf(2, 3) void verbose(struct bpf_verifier_env *env,
>  	else
>  		log->ubuf = NULL;
>  }
> +EXPORT_SYMBOL_GPL(bpf_verifier_log_write);
> +/* 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 #define below.
> + */
> +#define verbose bpf_verifier_log_write

Series looks good to me, one minor nit (or rather question) below:

Any objections to properly use function aliasing instead of define?

E.g. something along these lines:

static __printf(2, 3) void verbose(struct bpf_verifier_env *env,
				   const char *fmt, ...)
       __attribute__((alias("bpf_verifier_log_write")));

By the way, for some strange reason the series didn't make it into patchwork:

  http://patchwork.ozlabs.org/project/netdev/list/?state=*&page=1

I can only see patch 1/14 did arrive, although I got the whole series in my inbox:

  http://patchwork.ozlabs.org/patch/857939/

Could you try to resend? Hopefully it will appear on 2nd try.

Thanks,
Daniel

>  static bool type_is_pkt_pointer(enum bpf_reg_type type)
>  {
>