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: <52f5342f-a2a4-cc84-3486-803025904ad9@netronome.com>
Date:   Wed, 10 Jan 2018 12:25:19 +0000
From:   Quentin Monnet <quentin.monnet@...ronome.com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        alexei.starovoitov@...il.com
Cc:     netdev@...r.kernel.org, oss-drivers@...ronome.com
Subject: Re: [PATCH bpf-next 13/14] bpf: export function to write into
 verifier log buffer

Hi Daniel,

2018-01-10 11:32 UTC+0100 ~ Daniel Borkmann <daniel@...earbox.net>
> 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/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index d921ab387b0b..e674944811d1 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c

[...]

>> @@ -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")));

No objection at all, I simply did not know I could do that.

> 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

I'll send a v2 of the series with the fix for function aliasing,
hopefully patchwork will take it. Thanks a lot!

Quentin