Re: [PATCH bpf-next v2] tools/bpf: add log_level to bpf_load_program_attr

[Yonghong Song <yhs@fb.com>]

On 2/6/19 10:53 PM, Alexei Starovoitov wrote:
> On Wed, Feb 06, 2019 at 10:15:50PM -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
>> supported log_level is 0, 1, and 2.
>>
>> 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@fb.com>
>> ---
>>   tools/lib/bpf/bpf.c                     | 7 ++++++-
>>   tools/lib/bpf/bpf.h                     | 1 +
>>   tools/testing/selftests/bpf/test_sock.c | 9 ++++++++-
>>   3 files changed, 15 insertions(+), 2 deletions(-)
>>
>> Changelog:
>>    v1 -> v2:
>>      . make log_level as the last member of struct bpf_load_program_attr.
>>      . return -EINVAL if bpf_load_program_attr.log_level > 2.
>>
>> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
>> index 3defad77dc7a..6734c167279d 100644
>> --- a/tools/lib/bpf/bpf.c
>> +++ b/tools/lib/bpf/bpf.c
>> @@ -214,12 +214,17 @@ 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;
>>   
>>   	if (!load_attr)
>>   		return -EINVAL;
>>   
>> +	log_level = load_attr->log_level;
>> +	if (log_level > 2)
>> +		return -EINVAL;
>> +
>>   	name_len = load_attr->name ? strlen(load_attr->name) : 0;
>>   
>>   	bzero(&attr, sizeof(attr));
>> @@ -292,7 +297,7 @@ 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;
>> +	attr.log_level = (log_level == 2) ? log_level : 1;
> 
> I think that if user specified non zero log_level in xattr
> they probably want to get the log even when program was successfully loaded?
> Whereas above hunk will make an effect only when program is rejected.

In most cases, user wants to get log only when error happens.
But user specifying log_level=1 && non-null log_buf could indicate
intention to get the log even when successful.

To support all use cases regarding to log_level, we can do:
   . if log_level = 0, log_buf != NULL, existing behavior
     first without log; if fails, try with log_level=1.
   . if log_level=1/2, only one try with corresponding log_level.

> In addition non-zero log_level and !log_buf should be immediate error
> without calling kernel?

This makes sense. log_level, log_buf and log_buf_sz all need to be
consistent. The consistency of log_buf and log_buf_sz is not currently 
checked and left to kernel, so I did the same for log_level. I can add 
checks for all of them.