[PATCH bpf-next 0/6] bpf: various fixes and improvements

[PATCH bpf-next 0/6] bpf: various fixes and improvements

[Jakub Kicinski]

Hi!

This series combines a number of random improvements ranging from
libbpf to nfp driver.  NFP patches make better use of the verifier
log.  There is a requested adjustment to the map offload code, and
a warning fix for a W=1 build to the disassembler.  Quentin also
fixes the libbpf program type detection, while Jiong allows the use
of libbfd compiled from source.

Jakub Kicinski (3):
  bpf: offload: make bpf_offload_dev_match() reject host+host case
  bpf: annotate bpf_insn_print_t with __printf
  nfp: bpf: print map lookup problems into verifier log

Jiong Wang (1):
  tools: bpftool: add -DPACKAGE when including bfd.h

Quentin Monnet (2):
  libbpf: fix string comparison for guessing eBPF program type
  nfp: bpf: reject program on instructions unknown to the JIT compiler

 drivers/net/ethernet/netronome/nfp/bpf/jit.c      |  5 +++++
 drivers/net/ethernet/netronome/nfp/bpf/main.h     |  1 +
 drivers/net/ethernet/netronome/nfp/bpf/verifier.c | 20 ++++++++++++++------
 kernel/bpf/disasm.h                               |  4 ++--
 kernel/bpf/offload.c                              |  4 +---
 tools/bpf/bpftool/Makefile                        |  2 +-
 tools/build/feature/Makefile                      |  2 +-
 tools/lib/bpf/libbpf.c                            |  2 +-
 8 files changed, 26 insertions(+), 14 deletions(-)

-- 
2.15.1

[PATCH bpf-next 1/6] bpf: offload: make bpf_offload_dev_match() reject host+host case

[Jakub Kicinski]

Daniel suggests it would be more logical for bpf_offload_dev_match()
to return false is either the program or the map are not offloaded,
rather than treating the both not offloaded case as a "matching
CPU/host device".

This makes no functional difference today, since verifier only calls
bpf_offload_dev_match() when one of the objects is offloaded.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 kernel/bpf/offload.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
index 453785fa1881..a88cebf368bf 100644
--- a/kernel/bpf/offload.c
+++ b/kernel/bpf/offload.c
@@ -395,10 +395,8 @@ bool bpf_offload_dev_match(struct bpf_prog *prog, struct bpf_map *map)
 	struct bpf_prog_offload *offload;
 	bool ret;
 
-	if (!!bpf_prog_is_dev_bound(prog->aux) != !!bpf_map_is_dev_bound(map))
+	if (!bpf_prog_is_dev_bound(prog->aux) || !bpf_map_is_dev_bound(map))
 		return false;
-	if (!bpf_prog_is_dev_bound(prog->aux))
-		return true;
 
 	down_read(&bpf_devs_lock);
 	offload = prog->aux->offload;
-- 
2.15.1

[PATCH bpf-next 2/6] bpf: annotate bpf_insn_print_t with __printf

[Jakub Kicinski]

Functions of type bpf_insn_print_t take printf-like format
string, mark the type accordingly.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 kernel/bpf/disasm.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/disasm.h b/kernel/bpf/disasm.h
index e0857d016f89..266fe8ee542b 100644
--- a/kernel/bpf/disasm.h
+++ b/kernel/bpf/disasm.h
@@ -29,8 +29,8 @@ extern const char *const bpf_class_string[8];
 
 const char *func_id_name(int id);
 
-typedef void (*bpf_insn_print_t)(struct bpf_verifier_env *env,
-				 const char *, ...);
+typedef __printf(2, 3) void (*bpf_insn_print_t)(struct bpf_verifier_env *env,
+						const char *, ...);
 typedef const char *(*bpf_insn_revmap_call_t)(void *private_data,
 					      const struct bpf_insn *insn);
 typedef const char *(*bpf_insn_print_imm_t)(void *private_data,
-- 
2.15.1

[PATCH bpf-next 3/6] tools: bpftool: add -DPACKAGE when including bfd.h

[Jakub Kicinski]

From: Jiong Wang <jiong.wang@netronome.com>

bfd.h is requiring including of config.h except when PACKAGE or
PACKAGE_VERSION are defined.

  /* PR 14072: Ensure that config.h is included first.  */
  #if !defined PACKAGE && !defined PACKAGE_VERSION
  #error config.h must be included before this header
  #endif

This check has been introduced since May-2012. It doesn't show up in bfd.h
on some Linux distribution, probably because distributions have remove it
when building the package.

However, sometimes the user might just build libfd from source code then
link bpftool against it. For this case, bfd.h will be original that we need
to define PACKAGE or PACKAGE_VERSION.

Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
---
 tools/bpf/bpftool/Makefile   | 2 +-
 tools/build/feature/Makefile | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index 2237bc43f71c..26901ec87361 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -39,7 +39,7 @@ CC = gcc
 
 CFLAGS += -O2
 CFLAGS += -W -Wall -Wextra -Wno-unused-parameter -Wshadow
-CFLAGS += -D__EXPORTED_HEADERS__ -I$(srctree)/tools/include/uapi -I$(srctree)/tools/include -I$(srctree)/tools/lib/bpf -I$(srctree)/kernel/bpf/
+CFLAGS += -DPACKAGE='"bpftool"' -D__EXPORTED_HEADERS__ -I$(srctree)/tools/include/uapi -I$(srctree)/tools/include -I$(srctree)/tools/lib/bpf -I$(srctree)/kernel/bpf/
 CFLAGS += -DBPFTOOL_VERSION='"$(BPFTOOL_VERSION)"'
 LIBS = -lelf -lbfd -lopcodes $(LIBBPF)
 
diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile
index 17f2c73fff8b..bc715f6ac320 100644
--- a/tools/build/feature/Makefile
+++ b/tools/build/feature/Makefile
@@ -190,7 +190,7 @@ FLAGS_PERL_EMBED=$(PERL_EMBED_CCOPTS) $(PERL_EMBED_LDOPTS)
 	$(BUILD) -DPACKAGE='"perf"' -lbfd -lz -liberty -ldl
 
 $(OUTPUT)test-disassembler-four-args.bin:
-	$(BUILD) -lbfd -lopcodes
+	$(BUILD) -DPACKAGE='"perf"' -lbfd -lopcodes
 
 $(OUTPUT)test-liberty.bin:
 	$(CC) $(CFLAGS) -Wall -Werror -o $@ test-libbfd.c -DPACKAGE='"perf"' $(LDFLAGS) -lbfd -ldl -liberty
-- 
2.15.1

[PATCH bpf-next 4/6] libbpf: fix string comparison for guessing eBPF program type

[Jakub Kicinski]

From: Quentin Monnet <quentin.monnet@netronome.com>

libbpf is able to deduce the type of a program from the name of the ELF
section in which it is located. However, the comparison is made on the
first n characters, n being determined with sizeof() applied to the
reference string (e.g. "xdp"). When such section names are supposed to
receive a suffix separated with a slash (e.g. "kprobe/"), using sizeof()
takes the final NUL character of the reference string into account,
which implies that both strings must be equal. Instead, the desired
behaviour would consist in taking the length of the string, *without*
accounting for the ending NUL character, and to make sure the reference
string is a prefix to the ELF section name.

Subtract 1 to the total size of the string for obtaining the length for
the comparison.

Fixes: 583c90097f72 ("libbpf: add ability to guess program type based on section name")
Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/lib/bpf/libbpf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index e9c4b7cabcf2..30c776375118 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1803,7 +1803,7 @@ BPF_PROG_TYPE_FNS(tracepoint, BPF_PROG_TYPE_TRACEPOINT);
 BPF_PROG_TYPE_FNS(xdp, BPF_PROG_TYPE_XDP);
 BPF_PROG_TYPE_FNS(perf_event, BPF_PROG_TYPE_PERF_EVENT);
 
-#define BPF_PROG_SEC(string, type) { string, sizeof(string), type }
+#define BPF_PROG_SEC(string, type) { string, sizeof(string) - 1, type }
 static const struct {
 	const char *sec;
 	size_t len;
-- 
2.15.1

[PATCH bpf-next 5/6] nfp: bpf: print map lookup problems into verifier log

[Jakub Kicinski]

Use the verifier log to output error messages if map lookup
can't be offloaded.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Acked-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/bpf/verifier.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
index 741438896cc7..81dab462456c 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
@@ -132,22 +132,24 @@ nfp_bpf_check_call(struct nfp_prog *nfp_prog, struct bpf_verifier_env *env,
 
 	case BPF_FUNC_map_lookup_elem:
 		if (!bpf->helpers.map_lookup) {
-			pr_info("map_lookup: not supported by FW\n");
+			pr_vlog(env, "map_lookup: not supported by FW\n");
 			return -EOPNOTSUPP;
 		}
 		if (reg2->type != PTR_TO_STACK) {
-			pr_info("map_lookup: unsupported key ptr type %d\n",
+			pr_vlog(env,
+				"map_lookup: unsupported key ptr type %d\n",
 				reg2->type);
 			return -EOPNOTSUPP;
 		}
 		if (!tnum_is_const(reg2->var_off)) {
-			pr_info("map_lookup: variable key pointer\n");
+			pr_vlog(env, "map_lookup: variable key pointer\n");
 			return -EOPNOTSUPP;
 		}
 
 		off = reg2->var_off.value + reg2->off;
 		if (-off % 4) {
-			pr_info("map_lookup: unaligned stack pointer %lld\n",
+			pr_vlog(env,
+				"map_lookup: unaligned stack pointer %lld\n",
 				-off);
 			return -EOPNOTSUPP;
 		}
@@ -160,7 +162,7 @@ nfp_bpf_check_call(struct nfp_prog *nfp_prog, struct bpf_verifier_env *env,
 		meta->arg2_var_off |= off != old_off;
 
 		if (meta->arg1.map_ptr != reg1->map_ptr) {
-			pr_info("map_lookup: called for different map\n");
+			pr_vlog(env, "map_lookup: called for different map\n");
 			return -EOPNOTSUPP;
 		}
 		break;
@@ -263,7 +265,7 @@ nfp_bpf_check_ptr(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta,
 
 	if (reg->type == PTR_TO_MAP_VALUE) {
 		if (is_mbpf_store(meta)) {
-			pr_info("map writes not supported\n");
+			pr_vlog(env, "map writes not supported\n");
 			return -EOPNOTSUPP;
 		}
 	}
-- 
2.15.1

[PATCH bpf-next 6/6] nfp: bpf: reject program on instructions unknown to the JIT compiler

[Jakub Kicinski]

From: Quentin Monnet <quentin.monnet@netronome.com>

If an eBPF instruction is unknown to the driver JIT compiler, we can
reject the program at verification time.

Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Jiong Wang <jiong.wang@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/bpf/jit.c      | 5 +++++
 drivers/net/ethernet/netronome/nfp/bpf/main.h     | 1 +
 drivers/net/ethernet/netronome/nfp/bpf/verifier.c | 6 ++++++
 3 files changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/jit.c b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
index cdc949fabe98..56451edf01c2 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/jit.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
@@ -2907,6 +2907,11 @@ void nfp_bpf_jit_prepare(struct nfp_prog *nfp_prog, unsigned int cnt)
 	}
 }
 
+bool nfp_bpf_supported_opcode(u8 code)
+{
+	return !!instr_cb[code];
+}
+
 void *nfp_bpf_relo_for_vnic(struct nfp_prog *nfp_prog, struct nfp_bpf_vnic *bv)
 {
 	unsigned int i;
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.h b/drivers/net/ethernet/netronome/nfp/bpf/main.h
index 80855d43b25e..424fe8338105 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.h
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.h
@@ -324,6 +324,7 @@ struct nfp_bpf_vnic {
 
 void nfp_bpf_jit_prepare(struct nfp_prog *nfp_prog, unsigned int cnt);
 int nfp_bpf_jit(struct nfp_prog *prog);
+bool nfp_bpf_supported_opcode(u8 code);
 
 extern const struct bpf_prog_offload_ops nfp_bpf_analyzer_ops;
 
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
index 81dab462456c..479f602887e9 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
@@ -290,6 +290,12 @@ nfp_verify_insn(struct bpf_verifier_env *env, int insn_idx, int prev_insn_idx)
 	meta = nfp_bpf_goto_meta(nfp_prog, meta, insn_idx, env->prog->len);
 	nfp_prog->verifier_meta = meta;
 
+	if (!nfp_bpf_supported_opcode(meta->insn.code)) {
+		pr_vlog(env, "instruction %#02x not supported\n",
+			meta->insn.code);
+		return -EINVAL;
+	}
+
 	if (meta->insn.src_reg >= MAX_BPF_REG ||
 	    meta->insn.dst_reg >= MAX_BPF_REG) {
 		pr_vlog(env, "program uses extended registers - jit hardening?\n");
-- 
2.15.1

Re: [PATCH bpf-next 0/6] bpf: various fixes and improvements

[Daniel Borkmann]

On 01/17/2018 12:51 AM, Jakub Kicinski wrote:
> Hi!
> 
> This series combines a number of random improvements ranging from
> libbpf to nfp driver.  NFP patches make better use of the verifier
> log.  There is a requested adjustment to the map offload code, and
> a warning fix for a W=1 build to the disassembler.  Quentin also
> fixes the libbpf program type detection, while Jiong allows the use
> of libbfd compiled from source.

I did apply the series to bpf-next, thanks guys!