* [PATCH bpf-next 0/2] tools/bpf: two changes in libbpf
@ 2019-02-05 19:48 Yonghong Song
2019-02-05 19:48 ` [PATCH bpf-next 1/2] tools/bpf: add const qualifier to btf__get_map_kv_tids() map_name parameter Yonghong Song
2019-02-05 19:48 ` [PATCH bpf-next 2/2] tools/bpf: add log_level to bpf_load_program_attr Yonghong Song
0 siblings, 2 replies; 7+ messages in thread
From: Yonghong Song @ 2019-02-05 19:48 UTC (permalink / raw)
To: netdev; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Yonghong Song
Two changes in libbpf. Patch #1 adds "const" qualifier to parameter
"map_name" of function btf__get_map_kv_tids().
Patch #2 adds log_level to structure bpf_load_program_attr so
verifier log level 2 can be used with api function
bpf_load_program_xattr(). Please see individual patches for details.
Yonghong Song (2):
tools/bpf: add const qualifier to btf__get_map_kv_tids() map_name
parameter
tools/bpf: add log_level to bpf_load_program_attr
tools/lib/bpf/bpf.c | 4 +++-
tools/lib/bpf/bpf.h | 1 +
tools/lib/bpf/btf.c | 2 +-
tools/lib/bpf/btf.h | 2 +-
tools/testing/selftests/bpf/test_sock.c | 9 ++++++++-
5 files changed, 14 insertions(+), 4 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH bpf-next 1/2] tools/bpf: add const qualifier to btf__get_map_kv_tids() map_name parameter
2019-02-05 19:48 [PATCH bpf-next 0/2] tools/bpf: two changes in libbpf Yonghong Song
@ 2019-02-05 19:48 ` Yonghong Song
2019-02-06 2:27 ` Alexei Starovoitov
2019-02-05 19:48 ` [PATCH bpf-next 2/2] tools/bpf: add log_level to bpf_load_program_attr Yonghong Song
1 sibling, 1 reply; 7+ messages in thread
From: Yonghong Song @ 2019-02-05 19:48 UTC (permalink / raw)
To: netdev; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Yonghong Song
Commit 96408c43447a ("tools/bpf: implement libbpf btf__get_map_kv_tids() API function")
added the API function btf__get_map_kv_tids():
btf__get_map_kv_tids(const struct btf *btf, char *map_name, ...)
The parameter map_name has type "char *". This is okay inside libbpf library since
the map_name is from bpf_map->name which also has type "char *".
This will be problematic if the caller for map_name already has attribute "const",
e.g., from C++ string.c_str(). It will result in either a warning or an error.
/home/yhs/work/bcc/src/cc/btf.cc:166:51:
error: invalid conversion from ‘const char*’ to ‘char*’ [-fpermissive]
return btf__get_map_kv_tids(btf_, map_name.c_str()
This patch added "const" attributes to map_name parameter.
Fixes: 96408c43447a ("tools/bpf: implement libbpf btf__get_map_kv_tids() API function")
Signed-off-by: Yonghong Song <yhs@fb.com>
---
tools/lib/bpf/btf.c | 2 +-
tools/lib/bpf/btf.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 4949f8840bda..3b3a2959d03a 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -511,7 +511,7 @@ int btf__get_from_id(__u32 id, struct btf **btf)
return err;
}
-int btf__get_map_kv_tids(const struct btf *btf, char *map_name,
+int btf__get_map_kv_tids(const struct btf *btf, const char *map_name,
__u32 expected_key_size, __u32 expected_value_size,
__u32 *key_type_id, __u32 *value_type_id)
{
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 25a9d2db035d..b393da90cc85 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -69,7 +69,7 @@ LIBBPF_API void btf__get_strings(const struct btf *btf, const char **strings,
__u32 *str_len);
LIBBPF_API const char *btf__name_by_offset(const struct btf *btf, __u32 offset);
LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
-LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, char *map_name,
+LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name,
__u32 expected_key_size,
__u32 expected_value_size,
__u32 *key_type_id, __u32 *value_type_id);
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH bpf-next 2/2] tools/bpf: add log_level to bpf_load_program_attr
2019-02-05 19:48 [PATCH bpf-next 0/2] tools/bpf: two changes in libbpf Yonghong Song
2019-02-05 19:48 ` [PATCH bpf-next 1/2] tools/bpf: add const qualifier to btf__get_map_kv_tids() map_name parameter Yonghong Song
@ 2019-02-05 19:48 ` Yonghong Song
2019-02-06 2:26 ` Alexei Starovoitov
1 sibling, 1 reply; 7+ messages in thread
From: Yonghong Song @ 2019-02-05 19:48 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 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;
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] 7+ messages in thread
* Re: [PATCH bpf-next 2/2] tools/bpf: add log_level to bpf_load_program_attr
2019-02-05 19:48 ` [PATCH bpf-next 2/2] tools/bpf: add log_level to bpf_load_program_attr Yonghong Song
@ 2019-02-06 2:26 ` Alexei Starovoitov
2019-02-06 5:51 ` Yonghong Song
0 siblings, 1 reply; 7+ messages in thread
From: Alexei Starovoitov @ 2019-02-06 2:26 UTC (permalink / raw)
To: Yonghong Song; +Cc: netdev, Alexei Starovoitov, Daniel Borkmann, kernel-team
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next 1/2] tools/bpf: add const qualifier to btf__get_map_kv_tids() map_name parameter
2019-02-05 19:48 ` [PATCH bpf-next 1/2] tools/bpf: add const qualifier to btf__get_map_kv_tids() map_name parameter Yonghong Song
@ 2019-02-06 2:27 ` Alexei Starovoitov
2019-02-06 2:41 ` Alexei Starovoitov
0 siblings, 1 reply; 7+ messages in thread
From: Alexei Starovoitov @ 2019-02-06 2:27 UTC (permalink / raw)
To: Yonghong Song; +Cc: netdev, Alexei Starovoitov, Daniel Borkmann, kernel-team
On Tue, Feb 05, 2019 at 11:48:22AM -0800, Yonghong Song wrote:
> Commit 96408c43447a ("tools/bpf: implement libbpf btf__get_map_kv_tids() API function")
> added the API function btf__get_map_kv_tids():
> btf__get_map_kv_tids(const struct btf *btf, char *map_name, ...)
>
> The parameter map_name has type "char *". This is okay inside libbpf library since
> the map_name is from bpf_map->name which also has type "char *".
>
> This will be problematic if the caller for map_name already has attribute "const",
> e.g., from C++ string.c_str(). It will result in either a warning or an error.
>
> /home/yhs/work/bcc/src/cc/btf.cc:166:51:
> error: invalid conversion from ‘const char*’ to ‘char*’ [-fpermissive]
> return btf__get_map_kv_tids(btf_, map_name.c_str()
>
> This patch added "const" attributes to map_name parameter.
>
> Fixes: 96408c43447a ("tools/bpf: implement libbpf btf__get_map_kv_tids() API function")
> Signed-off-by: Yonghong Song <yhs@fb.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next 1/2] tools/bpf: add const qualifier to btf__get_map_kv_tids() map_name parameter
2019-02-06 2:27 ` Alexei Starovoitov
@ 2019-02-06 2:41 ` Alexei Starovoitov
0 siblings, 0 replies; 7+ messages in thread
From: Alexei Starovoitov @ 2019-02-06 2:41 UTC (permalink / raw)
To: Yonghong Song
Cc: Network Development, Alexei Starovoitov, Daniel Borkmann, Kernel Team
On Tue, Feb 5, 2019 at 6:27 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Feb 05, 2019 at 11:48:22AM -0800, Yonghong Song wrote:
> > Commit 96408c43447a ("tools/bpf: implement libbpf btf__get_map_kv_tids() API function")
> > added the API function btf__get_map_kv_tids():
> > btf__get_map_kv_tids(const struct btf *btf, char *map_name, ...)
> >
> > The parameter map_name has type "char *". This is okay inside libbpf library since
> > the map_name is from bpf_map->name which also has type "char *".
> >
> > This will be problematic if the caller for map_name already has attribute "const",
> > e.g., from C++ string.c_str(). It will result in either a warning or an error.
> >
> > /home/yhs/work/bcc/src/cc/btf.cc:166:51:
> > error: invalid conversion from ‘const char*’ to ‘char*’ [-fpermissive]
> > return btf__get_map_kv_tids(btf_, map_name.c_str()
> >
> > This patch added "const" attributes to map_name parameter.
> >
> > Fixes: 96408c43447a ("tools/bpf: implement libbpf btf__get_map_kv_tids() API function")
> > Signed-off-by: Yonghong Song <yhs@fb.com>
>
> Acked-by: Alexei Starovoitov <ast@kernel.org>
Actually just applied this patch alone,
since it's independent from 2nd.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next 2/2] tools/bpf: add log_level to bpf_load_program_attr
2019-02-06 2:26 ` Alexei Starovoitov
@ 2019-02-06 5:51 ` Yonghong Song
0 siblings, 0 replies; 7+ messages in thread
From: Yonghong Song @ 2019-02-06 5:51 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Kernel Team
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-02-06 5:52 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-05 19:48 [PATCH bpf-next 0/2] tools/bpf: two changes in libbpf Yonghong Song
2019-02-05 19:48 ` [PATCH bpf-next 1/2] tools/bpf: add const qualifier to btf__get_map_kv_tids() map_name parameter Yonghong Song
2019-02-06 2:27 ` Alexei Starovoitov
2019-02-06 2:41 ` Alexei Starovoitov
2019-02-05 19:48 ` [PATCH bpf-next 2/2] tools/bpf: add log_level to bpf_load_program_attr Yonghong Song
2019-02-06 2:26 ` Alexei Starovoitov
2019-02-06 5:51 ` Yonghong Song
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.