| Message ID | 20190207061550.1045583-1-yhs@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next,v2] tools/bpf: add log_level to bpf_load_program_attr | expand |
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 addition non-zero log_level and !log_buf should be immediate error without calling kernel?
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.
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; 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..6ffdd79bea89 100644 --- a/tools/lib/bpf/bpf.h +++ b/tools/lib/bpf/bpf.h @@ -85,6 +85,7 @@ struct bpf_load_program_attr { __u32 line_info_rec_size; const void *line_info; __u32 line_info_cnt; + __u32 log_level; }; /* Flags to direct loading requirements */ 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,
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.