netdev - Re: [PATCH bpf-next v3] tools/bpf: add log_level to bpf_load_program

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20190208023110.3u4b4zh4qucbkfap@ast-mbp.dhcp.thefacebook.com>
Date:   Thu, 7 Feb 2019 18:31:11 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     netdev@...r.kernel.org, Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>, kernel-team@...com
Subject: Re: [PATCH bpf-next v3] tools/bpf: add log_level to
 bpf_load_program_attr

On Thu, Feb 07, 2019 at 09:34:51AM -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@...com>
> ---
>  tools/lib/bpf/bpf.c                     | 22 +++++++++++++++++-----
>  tools/lib/bpf/bpf.h                     |  1 +
>  tools/testing/selftests/bpf/test_sock.c |  9 ++++++++-
>  3 files changed, 26 insertions(+), 6 deletions(-)
> 
> Changelog:
>   v2 -> v3:
>     . returning error immediately for inconsistent log_level/log_buf/log_buf_sz
>       values
>     . for log_level=1/2, always doing logging during bpf_prog_load.
>   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..3f5290ee1b47 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -214,10 +214,15 @@ 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)
> +	if (!load_attr || !log_buf != !log_buf_sz)
> +		return -EINVAL;
> +
> +	log_level = load_attr->log_level;
> +	if (log_level > 2 || (log_level && !log_buf))
>  		return -EINVAL;
>  
>  	name_len = load_attr->name ? strlen(load_attr->name) : 0;
> @@ -228,9 +233,16 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
>  	attr.insn_cnt = (__u32)load_attr->insns_cnt;
>  	attr.insns = ptr_to_u64(load_attr->insns);
>  	attr.license = ptr_to_u64(load_attr->license);
> -	attr.log_buf = ptr_to_u64(NULL);
> -	attr.log_size = 0;
> -	attr.log_level = 0;
> +
> +	attr.log_level = log_level;
> +	if (log_level) {
> +		attr.log_buf = ptr_to_u64(log_buf);
> +		attr.log_size = log_buf_sz;
> +	} else {
> +		attr.log_buf = ptr_to_u64(NULL);
> +		attr.log_size = 0;
> +	}
> +
>  	attr.kern_version = load_attr->kern_version;
>  	attr.prog_ifindex = load_attr->prog_ifindex;
>  	attr.prog_btf_fd = load_attr->prog_btf_fd;
> @@ -286,7 +298,7 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
>  			goto done;
>  	}
>  
> -	if (!log_buf || !log_buf_sz)
> +	if (log_level || !log_buf)
>  		goto done;
>  
>  	/* Try again with log */
> 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;

If there was a command line flag to toggle 'verbose' it would have made it
easier to test. In this form test_sock.c needs to be manually edited
to see that log_level=2 is actually working.

Applied to bpf-next anyway.