Message ID | 20190205194823.1805324-1-yhs@fb.com |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | tools/bpf: two changes in libbpf | expand |
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@fb.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. 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. 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.
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@fb.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.
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; diff --git a/tools/testing/selftests/bpf/test_sock.c b/tools/testing/selftests/bpf/test_sock.c index 561ffb6d6433..fb679ac3d4b0 100644 --- a/tools/testing/selftests/bpf/test_sock.c +++ b/tools/testing/selftests/bpf/test_sock.c @@ -20,6 +20,7 @@ #define MAX_INSNS 512 char bpf_log_buf[BPF_LOG_BUF_SIZE]; +static bool verbose = false; struct sock_test { const char *descr; @@ -325,6 +326,7 @@ static int load_sock_prog(const struct bpf_insn *prog, enum bpf_attach_type attach_type) { struct bpf_load_program_attr attr; + int ret; memset(&attr, 0, sizeof(struct bpf_load_program_attr)); attr.prog_type = BPF_PROG_TYPE_CGROUP_SOCK; @@ -332,8 +334,13 @@ static int load_sock_prog(const struct bpf_insn *prog, attr.insns = prog; attr.insns_cnt = probe_prog_length(attr.insns); attr.license = "GPL"; + attr.log_level = 2; - return bpf_load_program_xattr(&attr, bpf_log_buf, BPF_LOG_BUF_SIZE); + ret = bpf_load_program_xattr(&attr, bpf_log_buf, BPF_LOG_BUF_SIZE); + if (verbose && ret < 0) + fprintf(stderr, "%s\n", bpf_log_buf); + + return ret; } static int attach_sock_prog(int cgfd, int progfd,