On 01/10/2018 06:07 AM, Jakub Kicinski wrote:
> From: Quentin Monnet
>
> 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
> Reviewed-by: Jakub Kicinski
> ---
> 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)
> {
>
