netdev - Re: [PATCH bpf-next 2/2] tools/bpf: add log_level to bpf_load_program

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] [day] [month] [year] [list]

Message-ID: <01df888f-87cb-6d73-426a-2d1f61447d46@fb.com>
Date:   Wed, 6 Feb 2019 05:51:48 +0000
From:   Yonghong Song <yhs@...com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next 2/2] tools/bpf: add log_level to
 bpf_load_program_attr



On 2/5/19 6:26 PM, Alexei Starovoitov wrote:
> On Tue, Feb 05, 2019 at 11:48:23AM -0800, Yonghong Song wrote:
>> The kernel verifier has three levels of logs:
>>      0: no logs
>>      1: logs mostly useful
>>    > 1: verbose
>>
>> Current libbpf API functions bpf_load_program_xattr() and
>> bpf_load_program() cannot specify log_level.
>> The bcc, however, provides an interface for user to
>> specify log_level 2 for verbose output.
>>
>> This patch added log_level into structure
>> bpf_load_program_attr, so users, including bcc, can use
>> bpf_load_program_xattr() to change log_level.
>>
>> The bpf selftest test_sock.c is modified to enable log_level = 2.
>> If the "verbose" in test_sock.c is changed to true,
>> the test will output logs like below:
>>    $ ./test_sock
>>    func#0 @0
>>    0: R1=ctx(id=0,off=0,imm=0) R10=fp0,call_-1
>>    0: (bf) r6 = r1
>>    1: R1=ctx(id=0,off=0,imm=0) R6_w=ctx(id=0,off=0,imm=0) R10=fp0,call_-1
>>    1: (61) r7 = *(u32 *)(r6 +28)
>>    invalid bpf_context access off=28 size=4
>>
>>    Test case: bind4 load with invalid access: src_ip6 .. [PASS]
>>    ...
>>    Test case: bind6 allow all .. [PASS]
>>    Summary: 16 PASSED, 0 FAILED
>>
>> Some test_sock tests are negative tests and verbose verifier
>> log will be printed out as shown in the above.
>>
>> Signed-off-by: Yonghong Song <yhs@...com>
>> ---
>>   tools/lib/bpf/bpf.c                     | 4 +++-
>>   tools/lib/bpf/bpf.h                     | 1 +
>>   tools/testing/selftests/bpf/test_sock.c | 9 ++++++++-
>>   3 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
>> index 3defad77dc7a..78aa8c2b1a5c 100644
>> --- a/tools/lib/bpf/bpf.c
>> +++ b/tools/lib/bpf/bpf.c
>> @@ -214,6 +214,7 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
>>   {
>>   	void *finfo = NULL, *linfo = NULL;
>>   	union bpf_attr attr;
>> +	__u32 log_level;
>>   	__u32 name_len;
>>   	int fd;
>>   
>> @@ -292,7 +293,8 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
>>   	/* Try again with log */
>>   	attr.log_buf = ptr_to_u64(log_buf);
>>   	attr.log_size = log_buf_sz;
>> -	attr.log_level = 1;
>> +	log_level = load_attr->log_level;
>> +	attr.log_level = (log_level >= 2) ? log_level : 1;
>>   	log_buf[0] = 0;
>>   	fd = sys_bpf_prog_load(&attr, sizeof(attr));
>>   done:
>> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
>> index ed09eed2dc3b..15a8e22e8eae 100644
>> --- a/tools/lib/bpf/bpf.h
>> +++ b/tools/lib/bpf/bpf.h
>> @@ -76,6 +76,7 @@ struct bpf_load_program_attr {
>>   	const struct bpf_insn *insns;
>>   	size_t insns_cnt;
>>   	const char *license;
>> +	__u32 log_level;
>>   	__u32 kern_version;
>>   	__u32 prog_ifindex;
>>   	__u32 prog_btf_fd;
> 
> this will break binary compatibility in libbpf api.
> Please add new fields always to the end of *_attr structs.

I felt that we were still at 0.0.x stage and still could tweak
the api a little bit so I used the above sequence to
mimic the kernel prog_load attr sequence. I do agree that
in general we should add to the end of data structure.
I can change to the end of structure we still decided
if exposing log_level=2 is needed.

> 
> Also why not to silence bcc instead?
> Let it treat log_level > 1 as log_level=1
> I don't think anyone but the most extreme verifier hacker used level=2.

In general, that is true. I just used the feature a couple of
weeks ago to toubleshoot a verifier error... But most cases
log_level=1 should be sufficient.

> Personally I don't remember when was the last time I used it.
> It seem like a niche feature that we can safely remove in bcc.
Will discuss a little more in bcc community and then make
a decision.