netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v2] tools/bpf: add log_level to bpf_load_program_attr
@ 2019-02-07  6:15 Yonghong Song
  2019-02-07  6:53 ` Alexei Starovoitov
  0 siblings, 1 reply; 3+ messages in thread
From: Yonghong Song @ 2019-02-07  6:15 UTC (permalink / raw)
  To: netdev; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Yonghong Song

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;
 	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,
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH bpf-next v2] tools/bpf: add log_level to bpf_load_program_attr
  2019-02-07  6:15 [PATCH bpf-next v2] tools/bpf: add log_level to bpf_load_program_attr Yonghong Song
@ 2019-02-07  6:53 ` Alexei Starovoitov
  2019-02-07  7:45   ` Yonghong Song
  0 siblings, 1 reply; 3+ messages in thread
From: Alexei Starovoitov @ 2019-02-07  6:53 UTC (permalink / raw)
  To: Yonghong Song; +Cc: netdev, Alexei Starovoitov, Daniel Borkmann, kernel-team

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?


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH bpf-next v2] tools/bpf: add log_level to bpf_load_program_attr
  2019-02-07  6:53 ` Alexei Starovoitov
@ 2019-02-07  7:45   ` Yonghong Song
  0 siblings, 0 replies; 3+ messages in thread
From: Yonghong Song @ 2019-02-07  7:45 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Kernel Team



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.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2019-02-07  7:45 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-07  6:15 [PATCH bpf-next v2] tools/bpf: add log_level to bpf_load_program_attr Yonghong Song
2019-02-07  6:53 ` Alexei Starovoitov
2019-02-07  7:45   ` Yonghong Song

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).