linux-kernel - Re: [PATCHv2 perf/core 2/2] tools lib bpf: Sync with samples/bpf/libbpf

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <582D11B0.80208@huawei.com>
Date:   Thu, 17 Nov 2016 10:10:56 +0800
From:   "Wangnan (F)" <wangnan0@...wei.com>
To:     Joe Stringer <joe@....org>, <linux-kernel@...r.kernel.org>
CC:     <netdev@...r.kernel.org>, <ast@...com>, <daniel@...earbox.net>,
        <acme@...nel.org>
Subject: Re: [PATCHv2 perf/core 2/2] tools lib bpf: Sync with
 samples/bpf/libbpf

I'm also working on improving bpf.c. Please have a look at:

https://lkml.org/lkml/2016/11/14/1078

Since bpf.c is simple, I think we can add more functions and fixes
gradually, instead of a full copy.

See my inline comment below.

On 2016/11/17 1:43, Joe Stringer wrote:
> Extend the tools/ version of libbpf to include all of the functionality
> provided in the samples/bpf version.
>
> Signed-off-by: Joe Stringer <joe@....org>
> ---
> v2: Don't shift non-bpf changes across.
>      Various type cleanups, removal of extraneous declarations
> ---
>   tools/lib/bpf/bpf.c    | 107 ++++++++++++++++++++------
>   tools/lib/bpf/bpf.h    | 202 +++++++++++++++++++++++++++++++++++++++++++++++--
>   tools/lib/bpf/libbpf.c |   3 +-
>   3 files changed, 279 insertions(+), 33 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 4212ed62235b..5e061851ac00 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -20,10 +20,17 @@
>    */
>   
>   #include <stdlib.h>
> -#include <memory.h>
> +#include <stdio.h>
>   #include <unistd.h>
>   #include <asm/unistd.h>
> +#include <string.h>
> +#include <linux/netlink.h>
>   #include <linux/bpf.h>
> +#include <errno.h>
> +#include <net/ethernet.h>
> +#include <net/if.h>
> +#include <linux/if_packet.h>
> +#include <arpa/inet.h>
>   #include "bpf.h"
>   

Why we need these network related headers?

>   /*
> @@ -53,24 +60,71 @@ static int sys_bpf(enum bpf_cmd cmd, union bpf_attr *attr,
>   	return syscall(__NR_bpf, cmd, attr, size);
>   }
>   
> -int bpf_create_map(enum bpf_map_type map_type, int key_size,
> -		   int value_size, int max_entries)
> +int bpf_create_map(enum bpf_map_type map_type, int key_size, int value_size,
> +		   int max_entries, int map_flags)
>   {
> -	union bpf_attr attr;
> +	union bpf_attr attr = {
> +		.map_type = map_type,
> +		.key_size = key_size,
> +		.value_size = value_size,
> +		.max_entries = max_entries,
> +		.map_flags = map_flags,
> +	};
>   
> -	memset(&attr, '\0', sizeof(attr));
> +	return sys_bpf(BPF_MAP_CREATE, &attr, sizeof(attr));
> +}
>   

I lost map_flags in original bpf.c. Thanks to your patch. map_flags is 
useful
when creating BPF_MAP_TYPE_PERCPU_HASH: BPF_F_NO_PREALLOC is meanful in this
case.

Although it is okay in samples, I still prefer a explicit bzero() or 
memset(),
because kernel checks if unused field in this union is zero. However 
I'll check
c standard to see how unused field would be initialized.


<SNIP>

> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index e8ba54087497..4dba36995771 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -23,16 +23,202 @@
>   
>   #include <linux/bpf.h>
>   
> +struct bpf_insn;
> +
>   int bpf_create_map(enum bpf_map_type map_type, int key_size, int value_size,
> -		   int max_entries);
> +		   int max_entries, int map_flags);
> +int bpf_update_elem(int fd, void *key, void *value, unsigned long long flags);
> +int bpf_lookup_elem(int fd, void *key, void *value);
> +int bpf_delete_elem(int fd, void *key);
> +int bpf_get_next_key(int fd, void *key, void *next_key);
> +
> +int bpf_load_program(enum bpf_prog_type prog_type,
> +		     const struct bpf_insn *insns, int insn_len,
> +		     const char *license, int kern_version,
> +		     char *log_buf, size_t log_buf_sz);
> +
> +int bpf_obj_pin(int fd, const char *pathname);
> +int bpf_obj_get(const char *pathname);
>   
> -/* Recommend log buffer size */
>   #define BPF_LOG_BUF_SIZE 65536
> -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 bpf_map_update_elem(int fd, void *key, void *value,
> -			u64 flags);
> +/* ALU ops on registers, bpf_add|sub|...: dst_reg += src_reg */
> +
> +#define BPF_ALU64_REG(OP, DST, SRC)				\
> +	((struct bpf_insn) {					\
> +		.code  = BPF_ALU64 | BPF_OP(OP) | BPF_X,	\
> +		.dst_reg = DST,					\
> +		.src_reg = SRC,					\
> +		.off   = 0,					\
> +		.imm   = 0 })
> +

Should we define these macros here? They are in include/linux/filter.h
and duplicated in tools/include/linux/filter.h. Redefining them here
would cause conflict.

Thank you.