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 <y...@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.
