[PATCH v2 bpf-next 0/9] libbpf random fixes

[PATCH v2 bpf-next 0/9] libbpf random fixes

[Andrii Nakryiko]

This patch set is a collection of unrelated fixes for libbpf.

Patch #1 fixes detection of corrupted BPF section w/ instructions.
Patch #2 fixes possible errno clobbering.
Patch #3 simplifies endianness check and brings it in line with few other
similar checks in libbpf.
Patch #4 adds check for failed map name retrieval from ELF symbol name.
Patch #5 fixes return error code to be negative.
Patch #6 fixes using valid fd (0) as a marker of missing associated BTF.
Patch #7 removes redundant logic in two places.
Patch #8 fixes typos in comments and debug output, and fixes formatting.
Patch #9 unwraps a bunch of multi-line statements and comments.

v1->v2:
  - patch #1 simplifications (Song);


Andrii Nakryiko (9):
  libbpf: fix detection of corrupted BPF instructions section
  libbpf: preserve errno before calling into user callback
  libbpf: simplify endianness check
  libbpf: check map name retrieved from ELF
  libbpf: fix error code returned on corrupted ELF
  libbpf: use negative fd to specify missing BTF
  libbpf: simplify two pieces of logic
  libbpf: typo and formatting fixes
  libbpf: reduce unnecessary line wrapping

 tools/lib/bpf/libbpf.c | 148 +++++++++++++++++------------------------
 1 file changed, 60 insertions(+), 88 deletions(-)

-- 
2.17.1

[PATCH v2 bpf-next 1/9] libbpf: fix detection of corrupted BPF instructions section

[Andrii Nakryiko]

Ensure that size of a section w/ BPF instruction is exactly a multiple
of BPF instruction size.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/libbpf.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index ca4432f5b067..c6c9d632624a 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -349,8 +349,11 @@ static int
 bpf_program__init(void *data, size_t size, char *section_name, int idx,
 		  struct bpf_program *prog)
 {
-	if (size < sizeof(struct bpf_insn)) {
-		pr_warning("corrupted section '%s'\n", section_name);
+	const size_t bpf_insn_sz = sizeof(struct bpf_insn);
+
+	if (size == 0 || size % bpf_insn_sz) {
+		pr_warning("corrupted section '%s', size: %zu\n",
+			   section_name, size);
 		return -EINVAL;
 	}
 
@@ -376,9 +379,8 @@ bpf_program__init(void *data, size_t size, char *section_name, int idx,
 			   section_name);
 		goto errout;
 	}
-	prog->insns_cnt = size / sizeof(struct bpf_insn);
-	memcpy(prog->insns, data,
-	       prog->insns_cnt * sizeof(struct bpf_insn));
+	prog->insns_cnt = size / bpf_insn_sz;
+	memcpy(prog->insns, data, size);
 	prog->idx = idx;
 	prog->instances.fds = NULL;
 	prog->instances.nr = -1;
-- 
2.17.1

[PATCH v2 bpf-next 2/9] libbpf: preserve errno before calling into user callback

[Andrii Nakryiko]

pr_warning ultimately may call into user-provided callback function,
which can clobber errno value, so we need to save it before that.

Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/libbpf.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index c6c9d632624a..5af331cb8e4f 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -565,12 +565,12 @@ static int bpf_object__elf_init(struct bpf_object *obj)
 	} else {
 		obj->efile.fd = open(obj->path, O_RDONLY);
 		if (obj->efile.fd < 0) {
-			char errmsg[STRERR_BUFSIZE];
-			char *cp = libbpf_strerror_r(errno, errmsg,
-						     sizeof(errmsg));
+			char errmsg[STRERR_BUFSIZE], *cp;
 
+			err = -errno;
+			cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
 			pr_warning("failed to open %s: %s\n", obj->path, cp);
-			return -errno;
+			return err;
 		}
 
 		obj->efile.elf = elf_begin(obj->efile.fd,
-- 
2.17.1

[PATCH v2 bpf-next 3/9] libbpf: simplify endianness check

[Andrii Nakryiko]

Rewrite endianness check to use "more canonical" way, using
compiler-defined macros, similar to few other places in libbpf. It also
is more obvious and shorter.

Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/libbpf.c | 37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 5af331cb8e4f..fe700ef1135d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -607,31 +607,18 @@ static int bpf_object__elf_init(struct bpf_object *obj)
 	return err;
 }
 
-static int
-bpf_object__check_endianness(struct bpf_object *obj)
-{
-	static unsigned int const endian = 1;
-
-	switch (obj->efile.ehdr.e_ident[EI_DATA]) {
-	case ELFDATA2LSB:
-		/* We are big endian, BPF obj is little endian. */
-		if (*(unsigned char const *)&endian != 1)
-			goto mismatch;
-		break;
-
-	case ELFDATA2MSB:
-		/* We are little endian, BPF obj is big endian. */
-		if (*(unsigned char const *)&endian != 0)
-			goto mismatch;
-		break;
-	default:
-		return -LIBBPF_ERRNO__ENDIAN;
-	}
-
-	return 0;
-
-mismatch:
-	pr_warning("Error: endianness mismatch.\n");
+static int bpf_object__check_endianness(struct bpf_object *obj)
+{
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+	if (obj->efile.ehdr.e_ident[EI_DATA] == ELFDATA2LSB)
+		return 0;
+#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+	if (obj->efile.ehdr.e_ident[EI_DATA] == ELFDATA2MSB)
+		return 0;
+#else
+# error "Unrecognized __BYTE_ORDER__"
+#endif
+	pr_warning("endianness mismatch.\n");
 	return -LIBBPF_ERRNO__ENDIAN;
 }
 
-- 
2.17.1

[PATCH v2 bpf-next 4/9] libbpf: check map name retrieved from ELF

[Andrii Nakryiko]

Validate there was no error retrieving symbol name corresponding to
a BPF map.

Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/libbpf.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index fe700ef1135d..e3c0144e454f 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -920,6 +920,11 @@ bpf_object__init_maps(struct bpf_object *obj, int flags)
 		map_name = elf_strptr(obj->efile.elf,
 				      obj->efile.strtabidx,
 				      sym.st_name);
+		if (!map_name) {
+			pr_warning("failed to get map #%d name sym string for obj %s\n",
+				   map_idx, obj->path);
+			return -LIBBPF_ERRNO__FORMAT;
+		}
 
 		obj->maps[map_idx].libbpf_type = LIBBPF_MAP_UNSPEC;
 		obj->maps[map_idx].offset = sym.st_value;
-- 
2.17.1

[PATCH v2 bpf-next 5/9] libbpf: fix error code returned on corrupted ELF

[Andrii Nakryiko]

All of libbpf errors are negative, except this one. Fix it.

Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.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 e3c0144e454f..c972fa10271f 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1221,7 +1221,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
 
 	if (!obj->efile.strtabidx || obj->efile.strtabidx >= idx) {
 		pr_warning("Corrupted ELF file: index of strtab invalid\n");
-		return LIBBPF_ERRNO__FORMAT;
+		return -LIBBPF_ERRNO__FORMAT;
 	}
 	if (btf_data) {
 		obj->btf = btf__new(btf_data->d_buf, btf_data->d_size);
-- 
2.17.1

[PATCH v2 bpf-next 6/9] libbpf: use negative fd to specify missing BTF

[Andrii Nakryiko]

0 is a valid FD, so it's better to initialize it to -1, as is done in
other places. Also, technically, BTF type ID 0 is valid (it's a VOID
type), so it's more reliable to check btf_fd, instead of
btf_key_type_id, to determine if there is any BTF associated with a map.

Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/libbpf.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index c972fa10271f..a27a0351e595 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1751,7 +1751,7 @@ bpf_object__create_maps(struct bpf_object *obj)
 		create_attr.key_size = def->key_size;
 		create_attr.value_size = def->value_size;
 		create_attr.max_entries = def->max_entries;
-		create_attr.btf_fd = 0;
+		create_attr.btf_fd = -1;
 		create_attr.btf_key_type_id = 0;
 		create_attr.btf_value_type_id = 0;
 		if (bpf_map_type__is_map_in_map(def->type) &&
@@ -1765,11 +1765,11 @@ bpf_object__create_maps(struct bpf_object *obj)
 		}
 
 		*pfd = bpf_create_map_xattr(&create_attr);
-		if (*pfd < 0 && create_attr.btf_key_type_id) {
+		if (*pfd < 0 && create_attr.btf_fd >= 0) {
 			cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
 			pr_warning("Error in bpf_create_map_xattr(%s):%s(%d). Retrying without BTF.\n",
 				   map->name, cp, errno);
-			create_attr.btf_fd = 0;
+			create_attr.btf_fd = -1;
 			create_attr.btf_key_type_id = 0;
 			create_attr.btf_value_type_id = 0;
 			map->btf_key_type_id = 0;
@@ -2053,6 +2053,9 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
 	char *log_buf;
 	int ret;
 
+	if (!insns || !insns_cnt)
+		return -EINVAL;
+
 	memset(&load_attr, 0, sizeof(struct bpf_load_program_attr));
 	load_attr.prog_type = prog->type;
 	load_attr.expected_attach_type = prog->expected_attach_type;
@@ -2063,7 +2066,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
 	load_attr.license = license;
 	load_attr.kern_version = kern_version;
 	load_attr.prog_ifindex = prog->prog_ifindex;
-	load_attr.prog_btf_fd = prog->btf_fd >= 0 ? prog->btf_fd : 0;
+	load_attr.prog_btf_fd = prog->btf_fd;
 	load_attr.func_info = prog->func_info;
 	load_attr.func_info_rec_size = prog->func_info_rec_size;
 	load_attr.func_info_cnt = prog->func_info_cnt;
@@ -2072,8 +2075,6 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
 	load_attr.line_info_cnt = prog->line_info_cnt;
 	load_attr.log_level = prog->log_level;
 	load_attr.prog_flags = prog->prog_flags;
-	if (!load_attr.insns || !load_attr.insns_cnt)
-		return -EINVAL;
 
 retry_load:
 	log_buf = malloc(log_buf_size);
-- 
2.17.1

[PATCH v2 bpf-next 7/9] libbpf: simplify two pieces of logic

[Andrii Nakryiko]

Extra check for type is unnecessary in first case.

Extra zeroing is unnecessary, as snprintf guarantees that it will
zero-terminate string.

Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/libbpf.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index a27a0351e595..5ea84ab69db1 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1430,8 +1430,7 @@ bpf_program__collect_reloc(struct bpf_program *prog, GElf_Shdr *shdr,
 				if (maps[map_idx].libbpf_type != type)
 					continue;
 				if (type != LIBBPF_MAP_UNSPEC ||
-				    (type == LIBBPF_MAP_UNSPEC &&
-				     maps[map_idx].offset == sym.st_value)) {
+				    maps[map_idx].offset == sym.st_value) {
 					pr_debug("relocation: find map %zd (%s) for insn %u\n",
 						 map_idx, maps[map_idx].name, insn_idx);
 					break;
@@ -2354,7 +2353,6 @@ struct bpf_object *bpf_object__open_buffer(void *obj_buf,
 		snprintf(tmp_name, sizeof(tmp_name), "%lx-%lx",
 			 (unsigned long)obj_buf,
 			 (unsigned long)obj_buf_sz);
-		tmp_name[sizeof(tmp_name) - 1] = '\0';
 		name = tmp_name;
 	}
 	pr_debug("loading object '%s' from buffer\n",
-- 
2.17.1

[PATCH v2 bpf-next 8/9] libbpf: typo and formatting fixes

[Andrii Nakryiko]

A bunch of typo and formatting fixes.

Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/libbpf.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 5ea84ab69db1..6bd7dd544e41 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -505,7 +505,7 @@ static struct bpf_object *bpf_object__new(const char *path,
 
 	obj->efile.fd = -1;
 	/*
-	 * Caller of this function should also calls
+	 * Caller of this function should also call
 	 * bpf_object__elf_finish() after data collection to return
 	 * obj_buf to user. If not, we should duplicate the buffer to
 	 * avoid user freeing them before elf finish.
@@ -574,8 +574,7 @@ static int bpf_object__elf_init(struct bpf_object *obj)
 		}
 
 		obj->efile.elf = elf_begin(obj->efile.fd,
-				LIBBPF_ELF_C_READ_MMAP,
-				NULL);
+					   LIBBPF_ELF_C_READ_MMAP, NULL);
 	}
 
 	if (!obj->efile.elf) {
@@ -594,9 +593,9 @@ static int bpf_object__elf_init(struct bpf_object *obj)
 	ep = &obj->efile.ehdr;
 
 	/* Old LLVM set e_machine to EM_NONE */
-	if ((ep->e_type != ET_REL) || (ep->e_machine && (ep->e_machine != EM_BPF))) {
-		pr_warning("%s is not an eBPF object file\n",
-			obj->path);
+	if (ep->e_type != ET_REL ||
+	    (ep->e_machine && ep->e_machine != EM_BPF)) {
+		pr_warning("%s is not an eBPF object file\n", obj->path);
 		err = -LIBBPF_ERRNO__FORMAT;
 		goto errout;
 	}
@@ -1438,7 +1437,7 @@ bpf_program__collect_reloc(struct bpf_program *prog, GElf_Shdr *shdr,
 			}
 
 			if (map_idx >= nr_maps) {
-				pr_warning("bpf relocation: map_idx %d large than %d\n",
+				pr_warning("bpf relocation: map_idx %d larger than %d\n",
 					   (int)map_idx, (int)nr_maps - 1);
 				return -LIBBPF_ERRNO__RELOC;
 			}
@@ -1797,7 +1796,7 @@ bpf_object__create_maps(struct bpf_object *obj)
 			}
 		}
 
-		pr_debug("create map %s: fd=%d\n", map->name, *pfd);
+		pr_debug("created map %s: fd=%d\n", map->name, *pfd);
 	}
 
 	return 0;
-- 
2.17.1

[PATCH v2 bpf-next 9/9] libbpf: reduce unnecessary line wrapping

[Andrii Nakryiko]

There are a bunch of lines of code or comments that are unnecessary
wrapped into multi-lines. Fix that without violating any code
guidelines.

Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/libbpf.c | 52 +++++++++++++-----------------------------
 1 file changed, 16 insertions(+), 36 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 6bd7dd544e41..9042506acb65 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -497,8 +497,7 @@ static struct bpf_object *bpf_object__new(const char *path,
 
 	strcpy(obj->path, path);
 	/* Using basename() GNU version which doesn't modify arg. */
-	strncpy(obj->name, basename((void *)path),
-		sizeof(obj->name) - 1);
+	strncpy(obj->name, basename((void *)path), sizeof(obj->name) - 1);
 	end = strchr(obj->name, '.');
 	if (end)
 		*end = 0;
@@ -578,15 +577,13 @@ static int bpf_object__elf_init(struct bpf_object *obj)
 	}
 
 	if (!obj->efile.elf) {
-		pr_warning("failed to open %s as ELF file\n",
-				obj->path);
+		pr_warning("failed to open %s as ELF file\n", obj->path);
 		err = -LIBBPF_ERRNO__LIBELF;
 		goto errout;
 	}
 
 	if (!gelf_getehdr(obj->efile.elf, &obj->efile.ehdr)) {
-		pr_warning("failed to get EHDR from %s\n",
-				obj->path);
+		pr_warning("failed to get EHDR from %s\n", obj->path);
 		err = -LIBBPF_ERRNO__FORMAT;
 		goto errout;
 	}
@@ -622,18 +619,15 @@ static int bpf_object__check_endianness(struct bpf_object *obj)
 }
 
 static int
-bpf_object__init_license(struct bpf_object *obj,
-			 void *data, size_t size)
+bpf_object__init_license(struct bpf_object *obj, void *data, size_t size)
 {
-	memcpy(obj->license, data,
-	       min(size, sizeof(obj->license) - 1));
+	memcpy(obj->license, data, min(size, sizeof(obj->license) - 1));
 	pr_debug("license of %s is %s\n", obj->path, obj->license);
 	return 0;
 }
 
 static int
-bpf_object__init_kversion(struct bpf_object *obj,
-			  void *data, size_t size)
+bpf_object__init_kversion(struct bpf_object *obj, void *data, size_t size)
 {
 	__u32 kver;
 
@@ -643,8 +637,7 @@ bpf_object__init_kversion(struct bpf_object *obj,
 	}
 	memcpy(&kver, data, sizeof(kver));
 	obj->kern_version = kver;
-	pr_debug("kernel version of %s is %x\n", obj->path,
-		 obj->kern_version);
+	pr_debug("kernel version of %s is %x\n", obj->path, obj->kern_version);
 	return 0;
 }
 
@@ -800,8 +793,7 @@ bpf_object__init_internal_map(struct bpf_object *obj, struct bpf_map *map,
 	def->key_size = sizeof(int);
 	def->value_size = data->d_size;
 	def->max_entries = 1;
-	def->map_flags = type == LIBBPF_MAP_RODATA ?
-			 BPF_F_RDONLY_PROG : 0;
+	def->map_flags = type == LIBBPF_MAP_RODATA ? BPF_F_RDONLY_PROG : 0;
 	if (data_buff) {
 		*data_buff = malloc(data->d_size);
 		if (!*data_buff) {
@@ -816,8 +808,7 @@ bpf_object__init_internal_map(struct bpf_object *obj, struct bpf_map *map,
 	return 0;
 }
 
-static int
-bpf_object__init_maps(struct bpf_object *obj, int flags)
+static int bpf_object__init_maps(struct bpf_object *obj, int flags)
 {
 	int i, map_idx, map_def_sz = 0, nr_syms, nr_maps = 0, nr_maps_glob = 0;
 	bool strict = !(flags & MAPS_RELAX_COMPAT);
@@ -1098,8 +1089,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
 
 	/* Elf is corrupted/truncated, avoid calling elf_strptr. */
 	if (!elf_rawdata(elf_getscn(elf, ep->e_shstrndx), NULL)) {
-		pr_warning("failed to get e_shstrndx from %s\n",
-			   obj->path);
+		pr_warning("failed to get e_shstrndx from %s\n", obj->path);
 		return -LIBBPF_ERRNO__FORMAT;
 	}
 
@@ -1340,8 +1330,7 @@ bpf_program__collect_reloc(struct bpf_program *prog, GElf_Shdr *shdr,
 	size_t nr_maps = obj->nr_maps;
 	int i, nrels;
 
-	pr_debug("collecting relocating info for: '%s'\n",
-		 prog->section_name);
+	pr_debug("collecting relocating info for: '%s'\n", prog->section_name);
 	nrels = shdr->sh_size / shdr->sh_entsize;
 
 	prog->reloc_desc = malloc(sizeof(*prog->reloc_desc) * nrels);
@@ -1366,9 +1355,7 @@ bpf_program__collect_reloc(struct bpf_program *prog, GElf_Shdr *shdr,
 			return -LIBBPF_ERRNO__FORMAT;
 		}
 
-		if (!gelf_getsym(symbols,
-				 GELF_R_SYM(rel.r_info),
-				 &sym)) {
+		if (!gelf_getsym(symbols, GELF_R_SYM(rel.r_info), &sym)) {
 			pr_warning("relocation: symbol %"PRIx64" not found\n",
 				   GELF_R_SYM(rel.r_info));
 			return -LIBBPF_ERRNO__FORMAT;
@@ -1817,18 +1804,14 @@ check_btf_ext_reloc_err(struct bpf_program *prog, int err,
 	if (btf_prog_info) {
 		/*
 		 * Some info has already been found but has problem
-		 * in the last btf_ext reloc.  Must have to error
-		 * out.
+		 * in the last btf_ext reloc. Must have to error out.
 		 */
 		pr_warning("Error in relocating %s for sec %s.\n",
 			   info_name, prog->section_name);
 		return err;
 	}
 
-	/*
-	 * Have problem loading the very first info.  Ignore
-	 * the rest.
-	 */
+	/* Have problem loading the very first info. Ignore the rest. */
 	pr_warning("Cannot find %s for main program sec %s. Ignore all %s.\n",
 		   info_name, prog->section_name, info_name);
 	return 0;
@@ -2032,9 +2015,7 @@ static int bpf_object__collect_reloc(struct bpf_object *obj)
 			return -LIBBPF_ERRNO__RELOC;
 		}
 
-		err = bpf_program__collect_reloc(prog,
-						 shdr, data,
-						 obj);
+		err = bpf_program__collect_reloc(prog, shdr, data, obj);
 		if (err)
 			return err;
 	}
@@ -2354,8 +2335,7 @@ struct bpf_object *bpf_object__open_buffer(void *obj_buf,
 			 (unsigned long)obj_buf_sz);
 		name = tmp_name;
 	}
-	pr_debug("loading object '%s' from buffer\n",
-		 name);
+	pr_debug("loading object '%s' from buffer\n", name);
 
 	return __bpf_object__open(name, obj_buf, obj_buf_sz, true, true);
 }
-- 
2.17.1

Re: [PATCH v2 bpf-next 1/9] libbpf: fix detection of corrupted BPF instructions section

[Song Liu]

> On May 29, 2019, at 10:36 AM, Andrii Nakryiko <andriin@fb.com> wrote:
> 
> Ensure that size of a section w/ BPF instruction is exactly a multiple
> of BPF instruction size.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

Acked-by: Song Liu <songliubraving@fb.com>

> ---
> tools/lib/bpf/libbpf.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index ca4432f5b067..c6c9d632624a 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -349,8 +349,11 @@ static int
> bpf_program__init(void *data, size_t size, char *section_name, int idx,
> 		  struct bpf_program *prog)
> {
> -	if (size < sizeof(struct bpf_insn)) {
> -		pr_warning("corrupted section '%s'\n", section_name);
> +	const size_t bpf_insn_sz = sizeof(struct bpf_insn);
> +
> +	if (size == 0 || size % bpf_insn_sz) {
> +		pr_warning("corrupted section '%s', size: %zu\n",
> +			   section_name, size);
> 		return -EINVAL;
> 	}
> 
> @@ -376,9 +379,8 @@ bpf_program__init(void *data, size_t size, char *section_name, int idx,
> 			   section_name);
> 		goto errout;
> 	}
> -	prog->insns_cnt = size / sizeof(struct bpf_insn);
> -	memcpy(prog->insns, data,
> -	       prog->insns_cnt * sizeof(struct bpf_insn));
> +	prog->insns_cnt = size / bpf_insn_sz;
> +	memcpy(prog->insns, data, size);
> 	prog->idx = idx;
> 	prog->instances.fds = NULL;
> 	prog->instances.nr = -1;
> -- 
> 2.17.1
> 

Re: [PATCH v2 bpf-next 0/9] libbpf random fixes

[Daniel Borkmann]

On 05/29/2019 07:36 PM, Andrii Nakryiko wrote:
> This patch set is a collection of unrelated fixes for libbpf.
> 
> Patch #1 fixes detection of corrupted BPF section w/ instructions.
> Patch #2 fixes possible errno clobbering.
> Patch #3 simplifies endianness check and brings it in line with few other
> similar checks in libbpf.
> Patch #4 adds check for failed map name retrieval from ELF symbol name.
> Patch #5 fixes return error code to be negative.
> Patch #6 fixes using valid fd (0) as a marker of missing associated BTF.
> Patch #7 removes redundant logic in two places.
> Patch #8 fixes typos in comments and debug output, and fixes formatting.
> Patch #9 unwraps a bunch of multi-line statements and comments.
> 
> v1->v2:
>   - patch #1 simplifications (Song);
> 
> 
> Andrii Nakryiko (9):
>   libbpf: fix detection of corrupted BPF instructions section
>   libbpf: preserve errno before calling into user callback
>   libbpf: simplify endianness check
>   libbpf: check map name retrieved from ELF
>   libbpf: fix error code returned on corrupted ELF
>   libbpf: use negative fd to specify missing BTF
>   libbpf: simplify two pieces of logic
>   libbpf: typo and formatting fixes
>   libbpf: reduce unnecessary line wrapping
> 
>  tools/lib/bpf/libbpf.c | 148 +++++++++++++++++------------------------
>  1 file changed, 60 insertions(+), 88 deletions(-)
> 

Applied, thanks!

Re: [PATCH v2 bpf-next 6/9] libbpf: use negative fd to specify missing BTF

[Stanislav Fomichev]

On 05/29, Andrii Nakryiko wrote:
> 0 is a valid FD, so it's better to initialize it to -1, as is done in
> other places. Also, technically, BTF type ID 0 is valid (it's a VOID
> type), so it's more reliable to check btf_fd, instead of
> btf_key_type_id, to determine if there is any BTF associated with a map.
> 
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
>  tools/lib/bpf/libbpf.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index c972fa10271f..a27a0351e595 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1751,7 +1751,7 @@ bpf_object__create_maps(struct bpf_object *obj)
>  		create_attr.key_size = def->key_size;
>  		create_attr.value_size = def->value_size;
>  		create_attr.max_entries = def->max_entries;
> -		create_attr.btf_fd = 0;
> +		create_attr.btf_fd = -1;
>  		create_attr.btf_key_type_id = 0;
>  		create_attr.btf_value_type_id = 0;
>  		if (bpf_map_type__is_map_in_map(def->type) &&
> @@ -1765,11 +1765,11 @@ bpf_object__create_maps(struct bpf_object *obj)
>  		}
>  
>  		*pfd = bpf_create_map_xattr(&create_attr);
> -		if (*pfd < 0 && create_attr.btf_key_type_id) {
> +		if (*pfd < 0 && create_attr.btf_fd >= 0) {
>  			cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
>  			pr_warning("Error in bpf_create_map_xattr(%s):%s(%d). Retrying without BTF.\n",
>  				   map->name, cp, errno);
> -			create_attr.btf_fd = 0;
> +			create_attr.btf_fd = -1;
This breaks libbpf compatibility with the older kernels. If the kernel
doesn't know about btf_fd and we set it to -1, then CHECK_ATTR
fails :-(

Any objections to converting BTF retries to bpf_capabilities and then
knowingly passing bft_fd==0 or proper fd?

>  			create_attr.btf_key_type_id = 0;
>  			create_attr.btf_value_type_id = 0;
>  			map->btf_key_type_id = 0;
> @@ -2053,6 +2053,9 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
>  	char *log_buf;
>  	int ret;
>  
> +	if (!insns || !insns_cnt)
> +		return -EINVAL;
> +
>  	memset(&load_attr, 0, sizeof(struct bpf_load_program_attr));
>  	load_attr.prog_type = prog->type;
>  	load_attr.expected_attach_type = prog->expected_attach_type;
> @@ -2063,7 +2066,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
>  	load_attr.license = license;
>  	load_attr.kern_version = kern_version;
>  	load_attr.prog_ifindex = prog->prog_ifindex;
> -	load_attr.prog_btf_fd = prog->btf_fd >= 0 ? prog->btf_fd : 0;
> +	load_attr.prog_btf_fd = prog->btf_fd;
>  	load_attr.func_info = prog->func_info;
>  	load_attr.func_info_rec_size = prog->func_info_rec_size;
>  	load_attr.func_info_cnt = prog->func_info_cnt;
> @@ -2072,8 +2075,6 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
>  	load_attr.line_info_cnt = prog->line_info_cnt;
>  	load_attr.log_level = prog->log_level;
>  	load_attr.prog_flags = prog->prog_flags;
> -	if (!load_attr.insns || !load_attr.insns_cnt)
> -		return -EINVAL;
>  
>  retry_load:
>  	log_buf = malloc(log_buf_size);
> -- 
> 2.17.1
> 

Re: [PATCH v2 bpf-next 6/9] libbpf: use negative fd to specify missing BTF

[Stanislav Fomichev]

On 07/02, Stanislav Fomichev wrote:
> On 05/29, Andrii Nakryiko wrote:
> > 0 is a valid FD, so it's better to initialize it to -1, as is done in
> > other places. Also, technically, BTF type ID 0 is valid (it's a VOID
> > type), so it's more reliable to check btf_fd, instead of
> > btf_key_type_id, to determine if there is any BTF associated with a map.
> > 
> > Acked-by: Song Liu <songliubraving@fb.com>
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> >  tools/lib/bpf/libbpf.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index c972fa10271f..a27a0351e595 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -1751,7 +1751,7 @@ bpf_object__create_maps(struct bpf_object *obj)
> >  		create_attr.key_size = def->key_size;
> >  		create_attr.value_size = def->value_size;
> >  		create_attr.max_entries = def->max_entries;
> > -		create_attr.btf_fd = 0;
> > +		create_attr.btf_fd = -1;
> >  		create_attr.btf_key_type_id = 0;
> >  		create_attr.btf_value_type_id = 0;
> >  		if (bpf_map_type__is_map_in_map(def->type) &&
> > @@ -1765,11 +1765,11 @@ bpf_object__create_maps(struct bpf_object *obj)
> >  		}
> >  
> >  		*pfd = bpf_create_map_xattr(&create_attr);
> > -		if (*pfd < 0 && create_attr.btf_key_type_id) {
> > +		if (*pfd < 0 && create_attr.btf_fd >= 0) {
> >  			cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
> >  			pr_warning("Error in bpf_create_map_xattr(%s):%s(%d). Retrying without BTF.\n",
> >  				   map->name, cp, errno);
> > -			create_attr.btf_fd = 0;
> > +			create_attr.btf_fd = -1;
> This breaks libbpf compatibility with the older kernels. If the kernel
> doesn't know about btf_fd and we set it to -1, then CHECK_ATTR
> fails :-(
> 
> Any objections to converting BTF retries to bpf_capabilities and then
> knowingly passing bft_fd==0 or proper fd?
Oh, nevermind, it looks like you fixed it already in e55d54f43d3f.

> >  			create_attr.btf_key_type_id = 0;
> >  			create_attr.btf_value_type_id = 0;
> >  			map->btf_key_type_id = 0;
> > @@ -2053,6 +2053,9 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> >  	char *log_buf;
> >  	int ret;
> >  
> > +	if (!insns || !insns_cnt)
> > +		return -EINVAL;
> > +
> >  	memset(&load_attr, 0, sizeof(struct bpf_load_program_attr));
> >  	load_attr.prog_type = prog->type;
> >  	load_attr.expected_attach_type = prog->expected_attach_type;
> > @@ -2063,7 +2066,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> >  	load_attr.license = license;
> >  	load_attr.kern_version = kern_version;
> >  	load_attr.prog_ifindex = prog->prog_ifindex;
> > -	load_attr.prog_btf_fd = prog->btf_fd >= 0 ? prog->btf_fd : 0;
> > +	load_attr.prog_btf_fd = prog->btf_fd;
> >  	load_attr.func_info = prog->func_info;
> >  	load_attr.func_info_rec_size = prog->func_info_rec_size;
> >  	load_attr.func_info_cnt = prog->func_info_cnt;
> > @@ -2072,8 +2075,6 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> >  	load_attr.line_info_cnt = prog->line_info_cnt;
> >  	load_attr.log_level = prog->log_level;
> >  	load_attr.prog_flags = prog->prog_flags;
> > -	if (!load_attr.insns || !load_attr.insns_cnt)
> > -		return -EINVAL;
> >  
> >  retry_load:
> >  	log_buf = malloc(log_buf_size);
> > -- 
> > 2.17.1
> >