[PATCH v3 0/5] Miscellaneous fixes for BPF (perf tree)

[PATCH v3 0/5] Miscellaneous fixes for BPF (perf tree)

[Mickaël Salaün]

This series brings some fixes and small improvements to the BPF library and
samples.

This is intended for the perf tree and apply on e06094ab6755 ("Merge
remote-tracking branch 'tip/perf/urgent' into perf/core").

Changes since v2:
* add this cover letter

Changes since v1:
* exclude patches not intended for the perf tree

Regards,

Mickaël Salaün (5):
  bpf: Add missing header to the library
  bpf: Simplify bpf_load_program() error handling in the library
  samples/bpf: Ignore already processed ELF sections
  samples/bpf: Reset global variables
  samples/bpf: Add missing header

 samples/bpf/bpf_load.c     |  7 +++++++
 samples/bpf/tracex5_kern.c |  1 +
 tools/lib/bpf/bpf.c        | 18 ++++++------------
 tools/lib/bpf/bpf.h        |  1 +
 4 files changed, 15 insertions(+), 12 deletions(-)

-- 
2.11.0

[PATCH v3 1/5] bpf: Add missing header to the library

[Mickaël Salaün]

Include stddef.h to define size_t.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
Cc: Alexei Starovoitov <ast@fb.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Wang Nan <wangnan0@huawei.com>
---
 tools/lib/bpf/bpf.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index a2f9853dd882..df6e186da788 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -22,6 +22,7 @@
 #define __BPF_BPF_H
 
 #include <linux/bpf.h>
+#include <stddef.h>
 
 int bpf_create_map(enum bpf_map_type map_type, int key_size, int value_size,
 		   int max_entries, __u32 map_flags);
-- 
2.11.0

Re: [PATCH v3 1/5] bpf: Add missing header to the library

[Wangnan (F)]

On 2017/2/8 4:56, Mickaël Salaün wrote:
> Include stddef.h to define size_t.
>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Cc: Alexei Starovoitov <ast@fb.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Wang Nan <wangnan0@huawei.com>
> ---
>   tools/lib/bpf/bpf.h | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index a2f9853dd882..df6e186da788 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -22,6 +22,7 @@
>   #define __BPF_BPF_H
>   
>   #include <linux/bpf.h>
> +#include <stddef.h>
>   
>   int bpf_create_map(enum bpf_map_type map_type, int key_size, int value_size,
>   		   int max_entries, __u32 map_flags);
Looks good to me.

Thank you.

Re: [PATCH v3 1/5] bpf: Add missing header to the library

[Arnaldo Carvalho de Melo]

Em Wed, Feb 08, 2017 at 10:47:18AM +0800, Wangnan (F) escreveu:
> >+++ b/tools/lib/bpf/bpf.h
> >@@ -22,6 +22,7 @@
> >  #define __BPF_BPF_H
> >  #include <linux/bpf.h>
> >+#include <stddef.h>
> >  int bpf_create_map(enum bpf_map_type map_type, int key_size, int value_size,
> >  		   int max_entries, __u32 map_flags);
> Looks good to me.
> 
> Thank you.

Applied, took the "Thank you" as an "Acked-by: Wang",

Regards,

- Arnaldo

[PATCH v3 2/5] bpf: Simplify bpf_load_program() error handling in the library

[Mickaël Salaün]

Do not call a second time bpf(2) when a program load failed.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
Cc: Alexei Starovoitov <ast@fb.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Wang Nan <wangnan0@huawei.com>
---
 tools/lib/bpf/bpf.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 3ddb58a36d3c..fda3f494f1cd 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -73,7 +73,6 @@ int bpf_load_program(enum bpf_prog_type type, struct bpf_insn *insns,
 		     size_t insns_cnt, char *license,
 		     __u32 kern_version, char *log_buf, size_t log_buf_sz)
 {
-	int fd;
 	union bpf_attr attr;
 
 	bzero(&attr, sizeof(attr));
@@ -81,20 +80,15 @@ int bpf_load_program(enum bpf_prog_type type, struct bpf_insn *insns,
 	attr.insn_cnt = (__u32)insns_cnt;
 	attr.insns = ptr_to_u64(insns);
 	attr.license = ptr_to_u64(license);
-	attr.log_buf = ptr_to_u64(NULL);
-	attr.log_size = 0;
-	attr.log_level = 0;
+	attr.log_buf = ptr_to_u64(log_buf);
+	attr.log_size = log_buf_sz;
 	attr.kern_version = kern_version;
 
-	fd = sys_bpf(BPF_PROG_LOAD, &attr, sizeof(attr));
-	if (fd >= 0 || !log_buf || !log_buf_sz)
-		return fd;
+	if (log_buf && log_buf_sz > 0) {
+		attr.log_level = 1;
+		log_buf[0] = 0;
+	}
 
-	/* Try again with log */
-	attr.log_buf = ptr_to_u64(log_buf);
-	attr.log_size = log_buf_sz;
-	attr.log_level = 1;
-	log_buf[0] = 0;
 	return sys_bpf(BPF_PROG_LOAD, &attr, sizeof(attr));
 }
 
-- 
2.11.0

Re: [PATCH v3 2/5] bpf: Simplify bpf_load_program() error handling in the library

[Wangnan (F)]

On 2017/2/8 4:56, Mickaël Salaün wrote:
> Do not call a second time bpf(2) when a program load failed.

BPF_PROG_LOAD should success most of the time. Setting log_level to
0 by default and require log buffer when failure can make it faster
in normal case.

Thank you.

> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Cc: Alexei Starovoitov <ast@fb.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Wang Nan <wangnan0@huawei.com>
> ---
>   tools/lib/bpf/bpf.c | 18 ++++++------------
>   1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 3ddb58a36d3c..fda3f494f1cd 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -73,7 +73,6 @@ int bpf_load_program(enum bpf_prog_type type, struct bpf_insn *insns,
>   		     size_t insns_cnt, char *license,
>   		     __u32 kern_version, char *log_buf, size_t log_buf_sz)
>   {
> -	int fd;
>   	union bpf_attr attr;
>   
>   	bzero(&attr, sizeof(attr));
> @@ -81,20 +80,15 @@ int bpf_load_program(enum bpf_prog_type type, struct bpf_insn *insns,
>   	attr.insn_cnt = (__u32)insns_cnt;
>   	attr.insns = ptr_to_u64(insns);
>   	attr.license = ptr_to_u64(license);
> -	attr.log_buf = ptr_to_u64(NULL);
> -	attr.log_size = 0;
> -	attr.log_level = 0;
> +	attr.log_buf = ptr_to_u64(log_buf);
> +	attr.log_size = log_buf_sz;
>   	attr.kern_version = kern_version;
>   
> -	fd = sys_bpf(BPF_PROG_LOAD, &attr, sizeof(attr));
> -	if (fd >= 0 || !log_buf || !log_buf_sz)
> -		return fd;
> +	if (log_buf && log_buf_sz > 0) {
> +		attr.log_level = 1;
> +		log_buf[0] = 0;
> +	}
>   
> -	/* Try again with log */
> -	attr.log_buf = ptr_to_u64(log_buf);
> -	attr.log_size = log_buf_sz;
> -	attr.log_level = 1;
> -	log_buf[0] = 0;
>   	return sys_bpf(BPF_PROG_LOAD, &attr, sizeof(attr));
>   }
>   

Re: [PATCH v3 2/5] bpf: Simplify bpf_load_program() error handling in the library

[Mickaël Salaün]

[-- Attachment #1.1: Type: text/plain, Size: 2243 bytes --]


On 08/02/2017 03:35, Wangnan (F) wrote:
> 
> 
> On 2017/2/8 4:56, Mickaël Salaün wrote:
>> Do not call a second time bpf(2) when a program load failed.
> 
> BPF_PROG_LOAD should success most of the time. Setting log_level to
> 0 by default and require log buffer when failure can make it faster
> in normal case.

OK, I'm not sure if this is relevant for a test but I'm going to remove
this patch.

Thanks

> 
> Thank you.
> 
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Cc: Alexei Starovoitov <ast@fb.com>
>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: Wang Nan <wangnan0@huawei.com>
>> ---
>>   tools/lib/bpf/bpf.c | 18 ++++++------------
>>   1 file changed, 6 insertions(+), 12 deletions(-)
>>
>> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
>> index 3ddb58a36d3c..fda3f494f1cd 100644
>> --- a/tools/lib/bpf/bpf.c
>> +++ b/tools/lib/bpf/bpf.c
>> @@ -73,7 +73,6 @@ int bpf_load_program(enum bpf_prog_type type, struct
>> bpf_insn *insns,
>>                size_t insns_cnt, char *license,
>>                __u32 kern_version, char *log_buf, size_t log_buf_sz)
>>   {
>> -    int fd;
>>       union bpf_attr attr;
>>         bzero(&attr, sizeof(attr));
>> @@ -81,20 +80,15 @@ int bpf_load_program(enum bpf_prog_type type,
>> struct bpf_insn *insns,
>>       attr.insn_cnt = (__u32)insns_cnt;
>>       attr.insns = ptr_to_u64(insns);
>>       attr.license = ptr_to_u64(license);
>> -    attr.log_buf = ptr_to_u64(NULL);
>> -    attr.log_size = 0;
>> -    attr.log_level = 0;
>> +    attr.log_buf = ptr_to_u64(log_buf);
>> +    attr.log_size = log_buf_sz;
>>       attr.kern_version = kern_version;
>>   -    fd = sys_bpf(BPF_PROG_LOAD, &attr, sizeof(attr));
>> -    if (fd >= 0 || !log_buf || !log_buf_sz)
>> -        return fd;
>> +    if (log_buf && log_buf_sz > 0) {
>> +        attr.log_level = 1;
>> +        log_buf[0] = 0;
>> +    }
>>   -    /* Try again with log */
>> -    attr.log_buf = ptr_to_u64(log_buf);
>> -    attr.log_size = log_buf_sz;
>> -    attr.log_level = 1;
>> -    log_buf[0] = 0;
>>       return sys_bpf(BPF_PROG_LOAD, &attr, sizeof(attr));
>>   }
>>   
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 3/5] samples/bpf: Ignore already processed ELF sections

[Mickaël Salaün]

Add a missing check for the map fixup loop.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
Cc: Alexei Starovoitov <ast@fb.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
---
 samples/bpf/bpf_load.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
index 396e204888b3..e04fe09d7c2e 100644
--- a/samples/bpf/bpf_load.c
+++ b/samples/bpf/bpf_load.c
@@ -328,6 +328,8 @@ int load_bpf_file(char *path)
 
 	/* load programs that need map fixup (relocations) */
 	for (i = 1; i < ehdr.e_shnum; i++) {
+		if (processed_sec[i])
+			continue;
 
 		if (get_sec(elf, i, &ehdr, &shname, &shdr, &data))
 			continue;
-- 
2.11.0

[PATCH v3 4/5] samples/bpf: Reset global variables

[Mickaël Salaün]

Before loading a new ELF, clean previous kernel version, license and
processed sections.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
Cc: Alexei Starovoitov <ast@fb.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: David S. Miller <davem@davemloft.net>
---
 samples/bpf/bpf_load.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
index e04fe09d7c2e..b86ee54da2d1 100644
--- a/samples/bpf/bpf_load.c
+++ b/samples/bpf/bpf_load.c
@@ -277,6 +277,11 @@ int load_bpf_file(char *path)
 	Elf_Data *data, *data_prog, *symbols = NULL;
 	char *shname, *shname_prog;
 
+	/* reset global variables */
+	kern_version = 0;
+	memset(license, 0, sizeof(license));
+	memset(processed_sec, 0, sizeof(processed_sec));
+
 	if (elf_version(EV_CURRENT) == EV_NONE)
 		return 1;
 
-- 
2.11.0

[PATCH v3 5/5] samples/bpf: Add missing header

[Mickaël Salaün]

Include unistd.h to define __NR_getuid and __NR_getsid.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
Cc: Alexei Starovoitov <ast@fb.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: David S. Miller <davem@davemloft.net>
---
 samples/bpf/tracex5_kern.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/samples/bpf/tracex5_kern.c b/samples/bpf/tracex5_kern.c
index fd12d7154d42..7e4cf74553ff 100644
--- a/samples/bpf/tracex5_kern.c
+++ b/samples/bpf/tracex5_kern.c
@@ -8,6 +8,7 @@
 #include <linux/version.h>
 #include <uapi/linux/bpf.h>
 #include <uapi/linux/seccomp.h>
+#include <uapi/linux/unistd.h>
 #include "bpf_helpers.h"
 
 #define PROG(F) SEC("kprobe/"__stringify(F)) int bpf_func_##F
-- 
2.11.0