bpf: libbpf: Refactor and bug fix on the bpf_func_info loading logic

iamkafai · Alexei Starovoitov · commit f0187f0b17fa · 2018-12-09T13:54:38.000-08:00
This patch refactor and fix a bug in the libbpf's bpf_func_info loading logic. The bug fix and refactoring are targeting the same commit 2993e05 ("tools/bpf: add support to read .BTF.ext sections") which is in the bpf-next branch. 1) In bpf_load_program_xattr(), it should retry when errno == E2BIG regardless of log_buf and log_buf_sz. This patch fixes it. 2) btf_ext__reloc_init() and btf_ext__reloc() are essentially the same except btf_ext__reloc_init() always has insns_cnt == 0. Hence, btf_ext__reloc_init() is removed. btf_ext__reloc() is also renamed to btf_ext__reloc_func_info() to get ready for the line_info support in the next patch. 3) Consolidate func_info section logic from "btf_ext_parse_hdr()", "btf_ext_validate_func_info()" and "btf_ext__new()" to a new function "btf_ext_copy_func_info()" such that similar logic can be reused by the later libbpf's line_info patch. 4) The next line_info patch will store line_info_cnt instead of line_info_len in the bpf_program because the kernel is taking line_info_cnt also. It will save a few "len" to "cnt" conversions and will also save some function args. Hence, this patch also makes bpf_program to store func_info_cnt instead of func_info_len. 5) btf_ext depends on btf. e.g. the func_info's type_id in ".BTF.ext" is not useful when ".BTF" is absent. This patch only init the obj->btf_ext pointer after it has successfully init the obj->btf pointer. This can avoid always checking "obj->btf && obj->btf_ext" together for accessing ".BTF.ext". Checking "obj->btf_ext" alone will do. 6) Move "struct btf_sec_func_info" from btf.h to btf.c. There is no external usage outside btf.c. Fixes: 2993e05 ("tools/bpf: add support to read .BTF.ext sections") Signed-off-by: Martin KaFai Lau <kafai@fb.com> Acked-by: Yonghong Song <yhs@fb.com> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
@@ -205,7 +205,7 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
 	       min(name_len, BPF_OBJ_NAME_LEN - 1));
 
 	fd = sys_bpf(BPF_PROG_LOAD, &attr, sizeof(attr));
-	if (fd >= 0 || !log_buf || !log_buf_sz)
+	if (fd >= 0)
 		return fd;
 
 	/* After bpf_prog_load, the kernel may modify certain attributes
@@ -244,10 +244,13 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
 
 		fd = sys_bpf(BPF_PROG_LOAD, &attr, sizeof(attr));
 
-		if (fd >= 0 || !log_buf || !log_buf_sz)
+		if (fd >= 0)
 			goto done;
 	}
 
+	if (!log_buf || !log_buf_sz)
+		goto done;
+
 	/* Try again with log */
 	attr.log_buf = ptr_to_u64(log_buf);
 	attr.log_size = log_buf_sz;
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
@@ -43,6 +43,13 @@ struct btf_ext {
 	__u32 func_info_len;
 };
 
+struct btf_sec_func_info {
+	__u32	sec_name_off;
+	__u32	num_func_info;
+	/* Followed by num_func_info number of bpf func_info records */
+	__u8	data[0];
+};
+
 /* The minimum bpf_func_info checked by the loader */
 struct bpf_func_info_min {
 	__u32   insn_off;
@@ -479,41 +486,66 @@ int btf__get_from_id(__u32 id, struct btf **btf)
 	return err;
 }
 
-static int btf_ext_validate_func_info(const void *finfo, __u32 size,
-				      btf_print_fn_t err_log)
+static int btf_ext_copy_func_info(struct btf_ext *btf_ext,
+				  __u8 *data, __u32 data_size,
+				  btf_print_fn_t err_log)
 {
-	int sec_hdrlen = sizeof(struct btf_sec_func_info);
-	__u32 size_left, num_records, record_size;
+	const struct btf_ext_header *hdr = (struct btf_ext_header *)data;
 	const struct btf_sec_func_info *sinfo;
-	__u64 total_record_size;
+	__u32 info_left, record_size;
+	/* The start of the info sec (including the __u32 record_size). */
+	const void *info;
+
+	/* data and data_size do not include btf_ext_header from now on */
+	data = data + hdr->hdr_len;
+	data_size -= hdr->hdr_len;
+
+	if (hdr->func_info_off & 0x03) {
+		elog("BTF.ext func_info section is not aligned to 4 bytes\n");
+		return -EINVAL;
+	}
+
+	if (data_size < hdr->func_info_off ||
+	    hdr->func_info_len > data_size - hdr->func_info_off) {
+		elog("func_info section (off:%u len:%u) is beyond the end of the ELF section .BTF.ext\n",
+		     hdr->func_info_off, hdr->func_info_len);
+		return -EINVAL;
+	}
+
+	info = data + hdr->func_info_off;
+	info_left = hdr->func_info_len;
 
 	/* At least a func_info record size */
-	if (size < sizeof(__u32)) {
+	if (info_left < sizeof(__u32)) {
 		elog("BTF.ext func_info record size not found");
 		return -EINVAL;
 	}
 
-	/* The record size needs to meet below minimum standard */
-	record_size = *(__u32 *)finfo;
+	/* The record size needs to meet the minimum standard */
+	record_size = *(__u32 *)info;
 	if (record_size < sizeof(struct bpf_func_info_min) ||
-	    record_size % sizeof(__u32)) {
+	    record_size & 0x03) {
 		elog("BTF.ext func_info invalid record size");
 		return -EINVAL;
 	}
 
-	sinfo = finfo + sizeof(__u32);
-	size_left = size - sizeof(__u32);
+	sinfo = info + sizeof(__u32);
+	info_left -= sizeof(__u32);
 
 	/* If no func_info records, return failure now so .BTF.ext
 	 * won't be used.
 	 */
-	if (!size_left) {
+	if (!info_left) {
 		elog("BTF.ext no func info records");
 		return -EINVAL;
 	}
 
-	while (size_left) {
-		if (size_left < sec_hdrlen) {
+	while (info_left) {
+		unsigned int sec_hdrlen = sizeof(struct btf_sec_func_info);
+		__u64 total_record_size;
+		__u32 num_records;
+
+		if (info_left < sec_hdrlen) {
 			elog("BTF.ext func_info header not found");
 			return -EINVAL;
 		}
@@ -526,24 +558,30 @@ static int btf_ext_validate_func_info(const void *finfo, __u32 size,
 
 		total_record_size = sec_hdrlen +
 				    (__u64)num_records * record_size;
-		if (size_left < total_record_size) {
+		if (info_left < total_record_size) {
 			elog("incorrect BTF.ext num_func_info");
 			return -EINVAL;
 		}
 
-		size_left -= total_record_size;
+		info_left -= total_record_size;
 		sinfo = (void *)sinfo + total_record_size;
 	}
 
+	btf_ext->func_info_len = hdr->func_info_len - sizeof(__u32);
+	btf_ext->func_info_rec_size = record_size;
+	btf_ext->func_info = malloc(btf_ext->func_info_len);
+	if (!btf_ext->func_info)
+		return -ENOMEM;
+	memcpy(btf_ext->func_info, info + sizeof(__u32),
+	       btf_ext->func_info_len);
+
 	return 0;
 }
 
 static int btf_ext_parse_hdr(__u8 *data, __u32 data_size,
 			     btf_print_fn_t err_log)
 {
 	const struct btf_ext_header *hdr = (struct btf_ext_header *)data;
-	__u32 meta_left, last_func_info_pos;
-	void *finfo;
 
 	if (data_size < offsetof(struct btf_ext_header, func_info_off) ||
 	    data_size < hdr->hdr_len) {
@@ -566,34 +604,12 @@ static int btf_ext_parse_hdr(__u8 *data, __u32 data_size,
 		return -ENOTSUP;
 	}
 
-	meta_left = data_size - hdr->hdr_len;
-	if (!meta_left) {
+	if (data_size == hdr->hdr_len) {
 		elog("BTF.ext has no data\n");
 		return -EINVAL;
 	}
 
-	if (meta_left < hdr->func_info_off) {
-		elog("Invalid BTF.ext func_info section offset:%u\n",
-		     hdr->func_info_off);
-		return -EINVAL;
-	}
-
-	if (hdr->func_info_off & 0x03) {
-		elog("BTF.ext func_info section is not aligned to 4 bytes\n");
-		return -EINVAL;
-	}
-
-	last_func_info_pos = hdr->hdr_len + hdr->func_info_off +
-			     hdr->func_info_len;
-	if (last_func_info_pos > data_size) {
-		elog("Invalid BTF.ext func_info section size:%u\n",
-		     hdr->func_info_len);
-		return -EINVAL;
-	}
-
-	finfo = data + hdr->hdr_len + hdr->func_info_off;
-	return btf_ext_validate_func_info(finfo, hdr->func_info_len,
-					  err_log);
+	return 0;
 }
 
 void btf_ext__free(struct btf_ext *btf_ext)
@@ -607,10 +623,7 @@ void btf_ext__free(struct btf_ext *btf_ext)
 
 struct btf_ext *btf_ext__new(__u8 *data, __u32 size, btf_print_fn_t err_log)
 {
-	const struct btf_ext_header *hdr;
 	struct btf_ext *btf_ext;
-	void *org_fdata, *fdata;
-	__u32 hdrlen, size_u32;
 	int err;
 
 	err = btf_ext_parse_hdr(data, size, err_log);
@@ -621,81 +634,18 @@ struct btf_ext *btf_ext__new(__u8 *data, __u32 size, btf_print_fn_t err_log)
 	if (!btf_ext)
 		return ERR_PTR(-ENOMEM);
 
-	hdr = (const struct btf_ext_header *)data;
-	hdrlen = hdr->hdr_len;
-	size_u32 = sizeof(__u32);
-	fdata = malloc(hdr->func_info_len - size_u32);
-	if (!fdata) {
-		free(btf_ext);
-		return ERR_PTR(-ENOMEM);
+	err = btf_ext_copy_func_info(btf_ext, data, size, err_log);
+	if (err) {
+		btf_ext__free(btf_ext);
+		return ERR_PTR(err);
 	}
 
-	/* remember record size and copy rest of func_info data */
-	org_fdata = data + hdrlen + hdr->func_info_off;
-	btf_ext->func_info_rec_size = *(__u32 *)org_fdata;
-	memcpy(fdata, org_fdata + size_u32, hdr->func_info_len - size_u32);
-	btf_ext->func_info = fdata;
-	btf_ext->func_info_len = hdr->func_info_len - size_u32;
-
 	return btf_ext;
 }
 
-int btf_ext__reloc_init(struct btf *btf, struct btf_ext *btf_ext,
-			const char *sec_name, void **func_info,
-			__u32 *func_info_rec_size, __u32 *func_info_len)
-{
-	__u32 sec_hdrlen = sizeof(struct btf_sec_func_info);
-	__u32 i, record_size, records_len;
-	struct btf_sec_func_info *sinfo;
-	const char *info_sec_name;
-	__s64 remain_len;
-	void *data;
-
-	record_size = btf_ext->func_info_rec_size;
-	sinfo = btf_ext->func_info;
-	remain_len = btf_ext->func_info_len;
-
-	while (remain_len > 0) {
-		records_len = sinfo->num_func_info * record_size;
-		info_sec_name = btf__name_by_offset(btf, sinfo->sec_name_off);
-		if (strcmp(info_sec_name, sec_name)) {
-			remain_len -= sec_hdrlen + records_len;
-			sinfo = (void *)sinfo + sec_hdrlen + records_len;
-			continue;
-		}
-
-		data = malloc(records_len);
-		if (!data)
-			return -ENOMEM;
-
-		memcpy(data, sinfo->data, records_len);
-
-		/* adjust the insn_off, the data in .BTF.ext is
-		 * the actual byte offset, and the kernel expects
-		 * the offset in term of bpf_insn.
-		 *
-		 * adjust the insn offset only, the rest data will
-		 * be passed to kernel.
-		 */
-		for (i = 0; i < sinfo->num_func_info; i++) {
-			struct bpf_func_info_min *record;
-
-			record = data + i * record_size;
-			record->insn_off /= sizeof(struct bpf_insn);
-		}
-
-		*func_info = data;
-		*func_info_len = records_len;
-		*func_info_rec_size = record_size;
-		return 0;
-	}
-
-	return -EINVAL;
-}
-
-int btf_ext__reloc(struct btf *btf, struct btf_ext *btf_ext,
-		   const char *sec_name, __u32 insns_cnt,
-		   void **func_info, __u32 *func_info_len)
+int btf_ext__reloc_func_info(struct btf *btf, struct btf_ext *btf_ext,
+			     const char *sec_name, __u32 insns_cnt,
+			     void **func_info, __u32 *cnt)
 {
 	__u32 sec_hdrlen = sizeof(struct btf_sec_func_info);
 	__u32 i, record_size, existing_flen, records_len;
@@ -716,7 +666,7 @@ int btf_ext__reloc(struct btf *btf, struct btf_ext *btf_ext,
 			continue;
 		}
 
-		existing_flen = *func_info_len;
+		existing_flen = (*cnt) * record_size;
 		data = realloc(*func_info, existing_flen + records_len);
 		if (!data)
 			return -ENOMEM;
@@ -734,9 +684,14 @@ int btf_ext__reloc(struct btf *btf, struct btf_ext *btf_ext,
 				insns_cnt;
 		}
 		*func_info = data;
-		*func_info_len = existing_flen + records_len;
+		*cnt += sinfo->num_func_info;
 		return 0;
 	}
 
-	return -EINVAL;
+	return -ENOENT;
+}
+
+__u32 btf_ext__func_info_rec_size(const struct btf_ext *btf_ext)
+{
+	return btf_ext->func_info_rec_size;
 }
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
@@ -53,13 +53,6 @@ struct btf_ext_header {
 	__u32	func_info_len;
 };
 
-struct btf_sec_func_info {
-	__u32	sec_name_off;
-	__u32	num_func_info;
-	/* Followed by num_func_info number of bpf func_info records */
-	__u8	data[0];
-};
-
 typedef int (*btf_print_fn_t)(const char *, ...)
 	__attribute__((format(printf, 1, 2)));
 
@@ -77,12 +70,10 @@ LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
 
 struct btf_ext *btf_ext__new(__u8 *data, __u32 size, btf_print_fn_t err_log);
 void btf_ext__free(struct btf_ext *btf_ext);
-int btf_ext__reloc_init(struct btf *btf, struct btf_ext *btf_ext,
-			const char *sec_name, void **func_info,
-			__u32 *func_info_rec_size, __u32 *func_info_len);
-int btf_ext__reloc(struct btf *btf, struct btf_ext *btf_ext,
-		   const char *sec_name, __u32 insns_cnt, void **func_info,
-		   __u32 *func_info_len);
+int btf_ext__reloc_func_info(struct btf *btf, struct btf_ext *btf_ext,
+			     const char *sec_name, __u32 insns_cnt,
+			     void **func_info, __u32 *func_info_len);
+__u32 btf_ext__func_info_rec_size(const struct btf_ext *btf_ext);
 
 #ifdef __cplusplus
 } /* extern "C" */
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
