* [PATCH bpf-next 06/13] tools/bpf: sync kernel uapi bpf.h header to tools directory
2018-10-12 18:54 [PATCH bpf-next 05/13] bpf: get better bpf_prog ksyms based on btf func type_id Yonghong Song
@ 2018-10-12 18:54 ` Yonghong Song
2018-10-12 18:54 ` [PATCH bpf-next 07/13] tools/bpf: add new fields for program load in lib/bpf Yonghong Song
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Yonghong Song @ 2018-10-12 18:54 UTC (permalink / raw)
To: ast, kafai, daniel, netdev; +Cc: kernel-team
The kernel uapi bpf.h is synced to tools directory.
Signed-off-by: Yonghong Song <yhs@fb.com>
---
tools/include/uapi/linux/bpf.h | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index f9187b41dff6..7ebbf4f06a65 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -332,6 +332,9 @@ union bpf_attr {
* (context accesses, allowed helpers, etc).
*/
__u32 expected_attach_type;
+ __u32 prog_btf_fd; /* fd pointing to BTF type data */
+ __u32 func_info_len; /* func_info length */
+ __aligned_u64 func_info; /* func type info */
};
struct { /* anonymous struct used by BPF_OBJ_* commands */
@@ -2585,6 +2588,9 @@ struct bpf_prog_info {
__u32 nr_jited_func_lens;
__aligned_u64 jited_ksyms;
__aligned_u64 jited_func_lens;
+ __u32 btf_id;
+ __u32 nr_jited_func_types;
+ __aligned_u64 jited_func_types;
} __attribute__((aligned(8)));
struct bpf_map_info {
@@ -2896,4 +2902,9 @@ struct bpf_flow_keys {
};
};
+struct bpf_func_info {
+ __u32 insn_offset;
+ __u32 type_id;
+};
+
#endif /* _UAPI__LINUX_BPF_H__ */
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH bpf-next 07/13] tools/bpf: add new fields for program load in lib/bpf
2018-10-12 18:54 [PATCH bpf-next 05/13] bpf: get better bpf_prog ksyms based on btf func type_id Yonghong Song
2018-10-12 18:54 ` [PATCH bpf-next 06/13] tools/bpf: sync kernel uapi bpf.h header to tools directory Yonghong Song
@ 2018-10-12 18:54 ` Yonghong Song
2018-10-12 18:54 ` [PATCH bpf-next 08/13] tools/bpf: extends test_btf to test load/retrieve func_type info Yonghong Song
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Yonghong Song @ 2018-10-12 18:54 UTC (permalink / raw)
To: ast, kafai, daniel, netdev; +Cc: kernel-team
The new fields are added for program load in lib/bpf so
application uses api bpf_load_program_xattr() is able
to load program with btf and func_info data.
This functionality will be used in next patch
by bpf selftest test_btf.
Signed-off-by: Yonghong Song <yhs@fb.com>
---
tools/lib/bpf/bpf.c | 3 +++
tools/lib/bpf/bpf.h | 3 +++
2 files changed, 6 insertions(+)
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index d70a255cb05e..d8d48ab34220 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -196,6 +196,9 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
attr.log_level = 0;
attr.kern_version = load_attr->kern_version;
attr.prog_ifindex = load_attr->prog_ifindex;
+ attr.prog_btf_fd = load_attr->prog_btf_fd;
+ attr.func_info_len = load_attr->func_info_len;
+ attr.func_info = ptr_to_u64(load_attr->func_info);
memcpy(attr.prog_name, load_attr->name,
min(name_len, BPF_OBJ_NAME_LEN - 1));
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 87520a87a75f..d8fe72b22168 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -67,6 +67,9 @@ struct bpf_load_program_attr {
const char *license;
__u32 kern_version;
__u32 prog_ifindex;
+ __u32 prog_btf_fd;
+ __u32 func_info_len;
+ const struct bpf_func_info *func_info;
};
/* Recommend log buffer size */
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH bpf-next 08/13] tools/bpf: extends test_btf to test load/retrieve func_type info
2018-10-12 18:54 [PATCH bpf-next 05/13] bpf: get better bpf_prog ksyms based on btf func type_id Yonghong Song
2018-10-12 18:54 ` [PATCH bpf-next 06/13] tools/bpf: sync kernel uapi bpf.h header to tools directory Yonghong Song
2018-10-12 18:54 ` [PATCH bpf-next 07/13] tools/bpf: add new fields for program load in lib/bpf Yonghong Song
@ 2018-10-12 18:54 ` Yonghong Song
2018-10-12 18:54 ` [PATCH bpf-next 09/13] tools/bpf: add support to read .BTF.ext sections Yonghong Song
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Yonghong Song @ 2018-10-12 18:54 UTC (permalink / raw)
To: ast, kafai, daniel, netdev; +Cc: kernel-team
A two function bpf program is loaded with btf and func_info.
After successful prog load, the bpf_get_info syscall is called
to retrieve prog info to ensure the types returned from the
kernel matches the types passed to the kernel from the
user space.
Several negative tests are also added to test loading/retriving
of func_type info.
Signed-off-by: Yonghong Song <yhs@fb.com>
---
tools/testing/selftests/bpf/test_btf.c | 278 ++++++++++++++++++++++++-
1 file changed, 275 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c
index b6461c3c5e11..e03a8cea4bb7 100644
--- a/tools/testing/selftests/bpf/test_btf.c
+++ b/tools/testing/selftests/bpf/test_btf.c
@@ -5,6 +5,7 @@
#include <linux/btf.h>
#include <linux/err.h>
#include <linux/kernel.h>
+#include <linux/filter.h>
#include <bpf/bpf.h>
#include <sys/resource.h>
#include <libelf.h>
@@ -22,9 +23,13 @@
#include "bpf_rlimit.h"
#include "bpf_util.h"
+#define MAX_INSNS 512
+#define MAX_SUBPROGS 16
+
static uint32_t pass_cnt;
static uint32_t error_cnt;
static uint32_t skip_cnt;
+static bool jit_enabled;
#define CHECK(condition, format...) ({ \
int __ret = !!(condition); \
@@ -60,6 +65,24 @@ static int __base_pr(const char *format, ...)
return err;
}
+static bool is_jit_enabled(void)
+{
+ const char *jit_sysctl = "/proc/sys/net/core/bpf_jit_enable";
+ bool enabled = false;
+ int sysctl_fd;
+
+ sysctl_fd = open(jit_sysctl, 0, O_RDONLY);
+ if (sysctl_fd != -1) {
+ char tmpc;
+
+ if (read(sysctl_fd, &tmpc, sizeof(tmpc)) == 1)
+ enabled = (tmpc != '0');
+ close(sysctl_fd);
+ }
+
+ return enabled;
+}
+
#define BTF_INFO_ENC(kind, root, vlen) \
((!!(root) << 31) | ((kind) << 24) | ((vlen) & BTF_MAX_VLEN))
@@ -103,6 +126,7 @@ static struct args {
bool get_info_test;
bool pprint_test;
bool always_log;
+ bool func_type_test;
} args;
static char btf_log_buf[BTF_LOG_BUF_SIZE];
@@ -2693,16 +2717,256 @@ static int test_pprint(void)
return err;
}
+static struct btf_func_type_test {
+ const char *descr;
+ const char *str_sec;
+ __u32 raw_types[MAX_NR_RAW_TYPES];
+ __u32 str_sec_size;
+ struct bpf_insn insns[MAX_INSNS];
+ __u32 prog_type;
+ struct bpf_func_info func_info[MAX_SUBPROGS];
+ __u32 func_info_len;
+ bool expected_prog_load_failure;
+} func_type_test[] = {
+
+{
+ .descr = "func_type test #1",
+ .str_sec = "\0int\0unsigned int\0funcA\0funcB",
+ .raw_types = {
+ BTF_TYPE_INT_ENC(NAME_TBD, BTF_INT_SIGNED, 0, 32, 4), /* [1] */
+ BTF_TYPE_INT_ENC(NAME_TBD, 0, 0, 32, 4), /* [2] */
+ BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_FUNC, 0, 2), 1), /* [3] */
+ 1, 2,
+ BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_FUNC, 0, 2), 1), /* [4] */
+ 2, 1,
+ BTF_END_RAW,
+ },
+ .str_sec_size = sizeof("\0int\0unsigned int\0funcA\0funcB"),
+ .insns = {
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 2),
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_IMM(BPF_REG_0, 2),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_TRACEPOINT,
+ .func_info = { {0, 3}, {3, 4} },
+ .func_info_len = 2 * sizeof(struct bpf_func_info),
+},
+
+{
+ .descr = "func_type test #2",
+ .str_sec = "\0int\0unsigned int\0funcA\0funcB",
+ .raw_types = {
+ BTF_TYPE_INT_ENC(NAME_TBD, BTF_INT_SIGNED, 0, 32, 4), /* [1] */
+ BTF_TYPE_INT_ENC(NAME_TBD, 0, 0, 32, 4), /* [2] */
+ /* incorrect func type */
+ BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_FUNC_PROTO, 0, 2), 1), /* [3] */
+ 1, 2,
+ BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_FUNC, 0, 2), 1), /* [4] */
+ 2, 1,
+ BTF_END_RAW,
+ },
+ .str_sec_size = sizeof("\0int\0unsigned int\0funcA\0funcB"),
+ .insns = {
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 2),
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_IMM(BPF_REG_0, 2),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_TRACEPOINT,
+ .func_info = { {0, 3}, {3, 4} },
+ .func_info_len = 2 * sizeof(struct bpf_func_info),
+ .expected_prog_load_failure = true,
+},
+
+{
+ .descr = "func_type test #3",
+ .str_sec = "\0int\0unsigned int\0funcA\0funcB",
+ .raw_types = {
+ BTF_TYPE_INT_ENC(NAME_TBD, BTF_INT_SIGNED, 0, 32, 4), /* [1] */
+ BTF_TYPE_INT_ENC(NAME_TBD, 0, 0, 32, 4), /* [2] */
+ BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_FUNC, 0, 2), 1), /* [3] */
+ 1, 2,
+ BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_FUNC, 0, 2), 1), /* [4] */
+ 2, 1,
+ BTF_END_RAW,
+ },
+ .str_sec_size = sizeof("\0int\0unsigned int\0funcA\0funcB"),
+ .insns = {
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 2),
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_IMM(BPF_REG_0, 2),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_TRACEPOINT,
+ .func_info = { {0, 3}, {3, 4} },
+ /* incorrect func_info_len */
+ .func_info_len = sizeof(struct bpf_func_info),
+ .expected_prog_load_failure = true,
+},
+
+{
+ .descr = "func_type test #4",
+ .str_sec = "\0int\0unsigned int\0funcA\0funcB",
+ .raw_types = {
+ BTF_TYPE_INT_ENC(NAME_TBD, BTF_INT_SIGNED, 0, 32, 4), /* [1] */
+ BTF_TYPE_INT_ENC(NAME_TBD, 0, 0, 32, 4), /* [2] */
+ BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_FUNC, 0, 2), 1), /* [3] */
+ 1, 2,
+ BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_FUNC, 0, 2), 1), /* [4] */
+ 2, 1,
+ BTF_END_RAW,
+ },
+ .str_sec_size = sizeof("\0int\0unsigned int\0funcA\0funcB"),
+ .insns = {
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 2),
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_IMM(BPF_REG_0, 2),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_TRACEPOINT,
+ /* incorrect func_info insn_offset */
+ .func_info = { {0, 3}, {2, 4} },
+ .func_info_len = 2 * sizeof(struct bpf_func_info),
+ .expected_prog_load_failure = true,
+},
+
+};
+
+static size_t probe_prog_length(const struct bpf_insn *fp)
+{
+ size_t len;
+
+ for (len = MAX_INSNS - 1; len > 0; --len)
+ if (fp[len].code != 0 || fp[len].imm != 0)
+ break;
+ return len + 1;
+}
+
+static int do_test_func_type(int test_num)
+{
+ const struct btf_func_type_test *test = &func_type_test[test_num];
+ __u32 func_lens[4], func_types[4], info_len;
+ struct bpf_load_program_attr attr = {};
+ unsigned int raw_btf_size;
+ struct bpf_prog_info info = {};
+ int btf_fd, prog_fd, err = 0;
+ void *raw_btf;
+
+ fprintf(stderr, "%s......", test->descr);
+ raw_btf = btf_raw_create(&hdr_tmpl, test->raw_types,
+ test->str_sec, test->str_sec_size,
+ &raw_btf_size);
+
+ if (!raw_btf)
+ return -1;
+
+ *btf_log_buf = '\0';
+ btf_fd = bpf_load_btf(raw_btf, raw_btf_size,
+ btf_log_buf, BTF_LOG_BUF_SIZE,
+ args.always_log);
+ free(raw_btf);
+
+ if (CHECK(btf_fd == -1, "invalid btf_fd errno:%d", errno)) {
+ fprintf(stderr, "%s\n", btf_log_buf);
+ err = -1;
+ goto done;
+ }
+
+ attr.prog_type = test->prog_type;
+ attr.insns = test->insns;
+ attr.insns_cnt = probe_prog_length(attr.insns);
+ attr.license = "GPL";
+ attr.prog_btf_fd = btf_fd;
+ attr.func_info_len = test->func_info_len;
+ attr.func_info = test->func_info;
+
+ *btf_log_buf = '\0';
+ prog_fd = bpf_load_program_xattr(&attr, btf_log_buf,
+ BTF_LOG_BUF_SIZE);
+ if (test->expected_prog_load_failure && prog_fd == -1) {
+ err = 0;
+ goto done;
+ }
+ if (CHECK(prog_fd == -1, "invalid prog_id errno:%d", errno)) {
+ fprintf(stderr, "%s\n", btf_log_buf);
+ err = -1;
+ goto done;
+ }
+ if (!jit_enabled) {
+ skip_cnt++;
+ fprintf(stderr, "SKIPPED, please enable sysctl bpf_jit_enable\n");
+ err = 0;
+ goto done;
+ }
+
+ info_len = sizeof(struct bpf_prog_info);
+ info.nr_jited_func_types = 4;
+ info.nr_jited_func_lens = 4;
+ info.jited_func_types = ptr_to_u64(&func_types[0]);
+ info.jited_func_lens = ptr_to_u64(&func_lens[0]);
+ err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len);
+ if (CHECK(err == -1, "invalid get info errno:%d", errno)) {
+ fprintf(stderr, "%s\n", btf_log_buf);
+ err = -1;
+ goto done;
+ }
+ if (CHECK(info.nr_jited_func_lens != 2,
+ "incorrect info.nr_jited_func_lens %d\n",
+ info.nr_jited_func_lens)) {
+ err = -1;
+ goto done;
+ }
+ if (CHECK(info.nr_jited_func_types != 2,
+ "incorrect info.nr_jited_func_types %d\n",
+ info.nr_jited_func_types)) {
+ err = -1;
+ goto done;
+ }
+ if (CHECK(info.btf_id == 0, "incorrect btf_id = 0")) {
+ err = -1;
+ goto done;
+ }
+ if (CHECK(func_types[0] != test->func_info[0].type_id ||
+ func_types[1] != test->func_info[1].type_id,
+ "incorrect func_types (%u, %u) expected (%u %d)",
+ func_types[0], func_types[1],
+ test->func_info[0].type_id, test->func_info[1].type_id)) {
+ err = -1;
+ goto done;
+ }
+
+done:
+ return err;
+}
+
+static int test_func_type(void)
+{
+ unsigned int i;
+ int err = 0;
+
+ for (i = 0; i < ARRAY_SIZE(func_type_test); i++)
+ err |= count_result(do_test_func_type(i));
+
+ return err;
+}
+
static void usage(const char *cmd)
{
- fprintf(stderr, "Usage: %s [-l] [[-r test_num (1 - %zu)] | [-g test_num (1 - %zu)] | [-f test_num (1 - %zu)] | [-p]]\n",
+ fprintf(stderr, "Usage: %s [-l] [[-r test_num (1 - %zu)] |"
+ " [-g test_num (1 - %zu)] |"
+ " [-f test_num (1 - %zu)] | [-p] | [-k] ]\n",
cmd, ARRAY_SIZE(raw_tests), ARRAY_SIZE(get_info_tests),
ARRAY_SIZE(file_tests));
}
static int parse_args(int argc, char **argv)
{
- const char *optstr = "lpf:r:g:";
+ const char *optstr = "lpkf:r:g:";
int opt;
while ((opt = getopt(argc, argv, optstr)) != -1) {
@@ -2725,6 +2989,9 @@ static int parse_args(int argc, char **argv)
case 'p':
args.pprint_test = true;
break;
+ case 'k':
+ args.func_type_test = true;
+ break;
case 'h':
usage(argv[0]);
exit(0);
@@ -2778,6 +3045,8 @@ int main(int argc, char **argv)
if (args.always_log)
libbpf_set_print(__base_pr, __base_pr, __base_pr);
+ jit_enabled = is_jit_enabled();
+
if (args.raw_test)
err |= test_raw();
@@ -2790,8 +3059,11 @@ int main(int argc, char **argv)
if (args.pprint_test)
err |= test_pprint();
+ if (args.func_type_test)
+ err |= test_func_type();
+
if (args.raw_test || args.get_info_test || args.file_test ||
- args.pprint_test)
+ args.pprint_test || args.func_type_test)
goto done;
err |= test_raw();
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH bpf-next 09/13] tools/bpf: add support to read .BTF.ext sections
2018-10-12 18:54 [PATCH bpf-next 05/13] bpf: get better bpf_prog ksyms based on btf func type_id Yonghong Song
` (2 preceding siblings ...)
2018-10-12 18:54 ` [PATCH bpf-next 08/13] tools/bpf: extends test_btf to test load/retrieve func_type info Yonghong Song
@ 2018-10-12 18:54 ` Yonghong Song
2018-10-15 23:12 ` [PATCH bpf-next 05/13] bpf: get better bpf_prog ksyms based on btf func type_id Martin Lau
2018-10-16 17:59 ` Alexei Starovoitov
5 siblings, 0 replies; 10+ messages in thread
From: Yonghong Song @ 2018-10-12 18:54 UTC (permalink / raw)
To: ast, kafai, daniel, netdev; +Cc: kernel-team
The .BTF section is already available to encode types.
These types can be used for map
pretty print. The whole .BTF will be passed to the
kernel as well for which kernel can verify and return
to the user space for pretty print etc.
Recently landed llvm patch "[BPF] Add BTF generation
for BPF target" (https://reviews.llvm.org/rL344366)
will generate .BTF section and one more section
.BTF.ext. The .BTF.ext section encodes function type
information and line information. For line information,
the actual source code is encoded in the section, which
makes compiler itself as an ideal place for section
generation.
The .BTF section does not depend on any other section,
and .BTF.ext has dependency on .BTF for strings and types.
The .BTF section can be directly loaded into the
kernel, and the .BTF.ext section cannot. The loader
may need to do some relocation and merging,
similar to merging multiple code sections, before
loading into the kernel.
In this patch, only func type info is processed.
The functionality is implemented in libbpf.
In this patch, the header for .BTF.ext is the same
as the one in LLVM
(https://github.com/llvm-mirror/llvm/blob/master/include/llvm/MC/MCBTFContext.h).
Signed-off-by: Yonghong Song <yhs@fb.com>
---
tools/lib/bpf/btf.c | 232 +++++++++++++++++++++++++++++++++++++++++
tools/lib/bpf/btf.h | 31 ++++++
tools/lib/bpf/libbpf.c | 53 +++++++++-
3 files changed, 312 insertions(+), 4 deletions(-)
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 33095fc1860b..4748e0bacd2b 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -37,6 +37,11 @@ struct btf {
int fd;
};
+struct btf_ext {
+ void *func_info;
+ __u32 func_info_len;
+};
+
static int btf_add_type(struct btf *btf, struct btf_type *t)
{
if (btf->types_size - btf->nr_types < 2) {
@@ -397,3 +402,230 @@ const char *btf__name_by_offset(const struct btf *btf, __u32 offset)
else
return NULL;
}
+
+static int btf_ext_validate_func_info(const struct btf_sec_func_info *sinfo,
+ __u32 size, btf_print_fn_t err_log)
+{
+ int sec_hdrlen = sizeof(struct btf_sec_func_info);
+ __u32 record_size = sizeof(struct bpf_func_info);
+ __u32 size_left = size, num_records;
+ __u64 total_record_size;
+
+ while (size_left) {
+ if (size_left < sec_hdrlen) {
+ elog("BTF.ext func_info header not found");
+ return -EINVAL;
+ }
+
+ num_records = sinfo->num_func_info;
+ if (num_records == 0) {
+ elog("incorrect BTF.ext num_func_info");
+ return -EINVAL;
+ }
+
+ total_record_size = sec_hdrlen +
+ (__u64)num_records * record_size;
+ if (size_left < total_record_size) {
+ elog("incorrect BTF.ext num_func_info");
+ return -EINVAL;
+ }
+
+ size_left -= total_record_size;
+ sinfo = (void *)sinfo + total_record_size;
+ }
+
+ 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;
+ const struct btf_sec_func_info *sinfo;
+ __u32 meta_left, last_func_info_pos;
+
+ if (data_size < sizeof(*hdr)) {
+ elog("BTF.ext header not found");
+ return -EINVAL;
+ }
+
+ if (hdr->magic != BTF_MAGIC) {
+ elog("Invalid BTF.ext magic:%x\n", hdr->magic);
+ return -EINVAL;
+ }
+
+ if (hdr->version != BTF_VERSION) {
+ elog("Unsupported BTF.ext version:%u\n", hdr->version);
+ return -ENOTSUP;
+ }
+
+ if (hdr->flags) {
+ elog("Unsupported BTF.ext flags:%x\n", hdr->flags);
+ return -ENOTSUP;
+ }
+
+ meta_left = data_size - sizeof(*hdr);
+ if (!meta_left) {
+ 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 & 0x02) {
+ elog("BTF.ext func_info section is not aligned to 4 bytes\n");
+ return -EINVAL;
+ }
+
+ last_func_info_pos = sizeof(*hdr) + 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;
+ }
+
+ sinfo = (const struct btf_sec_func_info *)(data + sizeof(*hdr) +
+ hdr->func_info_off);
+ return btf_ext_validate_func_info(sinfo, hdr->func_info_len,
+ err_log);
+}
+
+void btf_ext__free(struct btf_ext *btf_ext)
+{
+ if (!btf_ext)
+ return;
+
+ free(btf_ext->func_info);
+ free(btf_ext);
+}
+
+struct btf_ext *btf_ext__new(__u8 *data, __u32 size, btf_print_fn_t err_log)
+{
+ int hdrlen = sizeof(struct btf_ext_header);
+ const struct btf_ext_header *hdr;
+ struct btf_ext *btf_ext;
+ void *fdata;
+ int err;
+
+ err = btf_ext_parse_hdr(data, size, err_log);
+ if (err)
+ return ERR_PTR(err);
+
+ btf_ext = calloc(1, sizeof(struct btf_ext));
+ if (!btf_ext)
+ return ERR_PTR(-ENOMEM);
+
+ hdr = (const struct btf_ext_header *)data;
+ fdata = malloc(hdr->func_info_len);
+ if (!fdata)
+ return ERR_PTR(-ENOMEM);
+
+ memcpy(fdata, data + hdrlen + hdr->func_info_off, hdr->func_info_len);
+ btf_ext->func_info = fdata;
+ btf_ext->func_info_len = hdr->func_info_len;
+
+ return btf_ext;
+}
+
+int btf_ext_reloc_init(struct btf *btf, struct btf_ext *btf_ext,
+ const char *sec_name, __u32 *btf_fd,
+ void **func_info, __u32 *func_info_len)
+{
+ int sec_hdrlen = sizeof(struct btf_sec_func_info);
+ int record_len = sizeof(struct bpf_func_info);
+ struct btf_sec_func_info *sinfo;
+ int i, remain_len, records_len;
+ const char *info_sec_name;
+ void *data;
+
+ if (!btf || !btf_ext) {
+ *btf_fd = 0;
+ *func_info = NULL;
+ *func_info_len = 0;
+ return 0;
+ }
+
+ sinfo = btf_ext->func_info;
+ remain_len = btf_ext->func_info_len;
+ *btf_fd = btf__fd(btf);
+
+ while (remain_len > 0) {
+ records_len = sinfo->num_func_info * record_len;
+ 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_offset, the data in .BTF.ext is
+ * the actual byte offset, and the kernel expects
+ * the offset in term of bpf_insn.
+ */
+ for (i = 0; i < sinfo->num_func_info; i++)
+ ((struct bpf_func_info *)data)[i].insn_offset
+ /= sizeof(struct bpf_insn);
+
+ *func_info = data;
+ *func_info_len = records_len;
+ break;
+ }
+
+ return 0;
+}
+
+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 sec_hdrlen = sizeof(struct btf_sec_func_info);
+ int record_len = sizeof(struct bpf_func_info);
+ __u32 i, remain_len, records_len;
+ struct btf_sec_func_info *sinfo;
+ struct bpf_func_info *record;
+ const char *info_sec_name;
+ __u32 existing_flen;
+ void *data;
+
+ if (!*func_info)
+ return 0;
+
+ sinfo = btf_ext->func_info;
+ remain_len = btf_ext->func_info_len;
+ while (remain_len > 0) {
+ records_len = sinfo->num_func_info * record_len;
+ 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;
+ }
+
+ existing_flen = *func_info_len;
+ data = realloc(*func_info, existing_flen + records_len);
+ if (!data)
+ return -ENOMEM;
+
+ memcpy(data + existing_flen, sinfo->data, records_len);
+ record = data + existing_flen;
+ for (i = 0; i < sinfo->num_func_info; i++)
+ record[i].insn_offset =
+ record[i].insn_offset/sizeof(struct bpf_insn) +
+ insns_cnt;
+ *func_info = data;
+ *func_info_len = existing_flen + records_len;
+ break;
+ }
+
+ return 0;
+}
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 6db5462bb2ef..2bbcc9e41cf5 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -7,10 +7,32 @@
#include <linux/types.h>
#define BTF_ELF_SEC ".BTF"
+#define BTF_EXT_ELF_SEC ".BTF.ext"
struct btf;
+struct btf_ext;
struct btf_type;
+struct btf_ext_header {
+ __u16 magic;
+ __u8 version;
+ __u8 flags;
+ __u32 hdr_len;
+
+ /* All offsets are in bytes relative to the end of this header */
+ __u32 func_info_off;
+ __u32 func_info_len;
+ __u32 line_info_off;
+ __u32 line_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)));
@@ -23,4 +45,13 @@ int btf__resolve_type(const struct btf *btf, __u32 type_id);
int btf__fd(const struct btf *btf);
const char *btf__name_by_offset(const struct btf *btf, __u32 offset);
+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, __u32 *btf_fd,
+ void **func_info, __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);
+
#endif /* __LIBBPF_BTF_H */
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 176cf5523728..9271b06330f7 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -151,6 +151,9 @@ struct bpf_program {
bpf_program_clear_priv_t clear_priv;
enum bpf_attach_type expected_attach_type;
+ __u32 btf_fd;
+ void *func_info;
+ __u32 func_info_len;
};
struct bpf_map {
@@ -207,6 +210,7 @@ struct bpf_object {
struct list_head list;
struct btf *btf;
+ struct btf_ext *btf_ext;
void *priv;
bpf_object_clear_priv_t clear_priv;
@@ -781,6 +785,15 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
BTF_ELF_SEC, PTR_ERR(obj->btf));
obj->btf = NULL;
}
+ } else if (strcmp(name, BTF_EXT_ELF_SEC) == 0) {
+ obj->btf_ext = btf_ext__new(data->d_buf, data->d_size,
+ __pr_debug);
+ if (IS_ERR(obj->btf_ext)) {
+ pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n",
+ BTF_EXT_ELF_SEC,
+ PTR_ERR(obj->btf_ext));
+ obj->btf_ext = NULL;
+ }
} else if (sh.sh_type == SHT_SYMTAB) {
if (obj->efile.symbols) {
pr_warning("bpf: multiple SYMTAB in %s\n",
@@ -1164,6 +1177,7 @@ bpf_program__reloc_text(struct bpf_program *prog, struct bpf_object *obj,
struct bpf_insn *insn, *new_insn;
struct bpf_program *text;
size_t new_cnt;
+ int err;
if (relo->type != RELO_CALL)
return -LIBBPF_ERRNO__RELOC;
@@ -1186,6 +1200,16 @@ bpf_program__reloc_text(struct bpf_program *prog, struct bpf_object *obj,
pr_warning("oom in prog realloc\n");
return -ENOMEM;
}
+
+ err = btf_ext_reloc(obj->btf, obj->btf_ext, text->section_name,
+ prog->insns_cnt, &prog->func_info,
+ &prog->func_info_len);
+ if (err) {
+ pr_warning("error in btf_ext_reloc for sec %s\n",
+ text->section_name);
+ return err;
+ }
+
memcpy(new_insn + prog->insns_cnt, text->insns,
text->insns_cnt * sizeof(*insn));
prog->insns = new_insn;
@@ -1205,7 +1229,19 @@ bpf_program__relocate(struct bpf_program *prog, struct bpf_object *obj)
{
int i, err;
- if (!prog || !prog->reloc_desc)
+ if (!prog)
+ return 0;
+
+ err = btf_ext_reloc_init(obj->btf, obj->btf_ext, prog->section_name,
+ &prog->btf_fd, &prog->func_info,
+ &prog->func_info_len);
+ if (err) {
+ pr_warning("err in btf_ext_reloc_init for sec %s\n",
+ prog->section_name);
+ return err;
+ }
+
+ if (!prog->reloc_desc)
return 0;
for (i = 0; i < prog->nr_reloc; i++) {
@@ -1295,7 +1331,8 @@ static int bpf_object__collect_reloc(struct bpf_object *obj)
static int
load_program(enum bpf_prog_type type, enum bpf_attach_type expected_attach_type,
const char *name, struct bpf_insn *insns, int insns_cnt,
- char *license, __u32 kern_version, int *pfd, int prog_ifindex)
+ char *license, __u32 kern_version, int *pfd, int prog_ifindex,
+ u32 btf_fd, struct bpf_func_info *func_info, u32 func_info_len)
{
struct bpf_load_program_attr load_attr;
char *cp, errmsg[STRERR_BUFSIZE];
@@ -1311,6 +1348,9 @@ load_program(enum bpf_prog_type type, enum bpf_attach_type expected_attach_type,
load_attr.license = license;
load_attr.kern_version = kern_version;
load_attr.prog_ifindex = prog_ifindex;
+ load_attr.prog_btf_fd = btf_fd;
+ load_attr.func_info = func_info;
+ load_attr.func_info_len = func_info_len;
if (!load_attr.insns || !load_attr.insns_cnt)
return -EINVAL;
@@ -1394,7 +1434,9 @@ bpf_program__load(struct bpf_program *prog,
err = load_program(prog->type, prog->expected_attach_type,
prog->name, prog->insns, prog->insns_cnt,
license, kern_version, &fd,
- prog->prog_ifindex);
+ prog->prog_ifindex,
+ prog->btf_fd, prog->func_info,
+ prog->func_info_len);
if (!err)
prog->instances.fds[0] = fd;
goto out;
@@ -1426,7 +1468,9 @@ bpf_program__load(struct bpf_program *prog,
prog->name, result.new_insn_ptr,
result.new_insn_cnt,
license, kern_version, &fd,
- prog->prog_ifindex);
+ prog->prog_ifindex,
+ prog->btf_fd, prog->func_info,
+ prog->func_info_len);
if (err) {
pr_warning("Loading the %dth instance of program '%s' failed\n",
@@ -1835,6 +1879,7 @@ void bpf_object__close(struct bpf_object *obj)
bpf_object__elf_finish(obj);
bpf_object__unload(obj);
btf__free(obj->btf);
+ btf_ext__free(obj->btf_ext);
for (i = 0; i < obj->nr_maps; i++) {
zfree(&obj->maps[i].name);
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next 05/13] bpf: get better bpf_prog ksyms based on btf func type_id
2018-10-12 18:54 [PATCH bpf-next 05/13] bpf: get better bpf_prog ksyms based on btf func type_id Yonghong Song
` (3 preceding siblings ...)
2018-10-12 18:54 ` [PATCH bpf-next 09/13] tools/bpf: add support to read .BTF.ext sections Yonghong Song
@ 2018-10-15 23:12 ` Martin Lau
2018-10-17 0:32 ` Yonghong Song
2018-10-16 17:59 ` Alexei Starovoitov
5 siblings, 1 reply; 10+ messages in thread
From: Martin Lau @ 2018-10-15 23:12 UTC (permalink / raw)
To: Yonghong Song; +Cc: Alexei Starovoitov, daniel, netdev, Kernel Team
On Fri, Oct 12, 2018 at 11:54:42AM -0700, Yonghong Song wrote:
> This patch added interface to load a program with the following
> additional information:
> . prog_btf_fd
> . func_info and func_info_len
> where func_info will provides function range and type_id
> corresponding to each function.
>
> If verifier agrees with function range provided by the user,
> the bpf_prog ksym for each function will use the func name
> provided in the type_id, which is supposed to provide better
> encoding as it is not limited by 16 bytes program name
> limitation and this is better for bpf program which contains
> multiple subprograms.
>
> The bpf_prog_info interface is also extended to
> return btf_id and jited_func_types, so user spaces can
> print out the function prototype for each jited function.
Some nits.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
> include/linux/bpf.h | 2 +
> include/linux/bpf_verifier.h | 1 +
> include/linux/btf.h | 2 +
> include/uapi/linux/bpf.h | 11 +++++
> kernel/bpf/btf.c | 16 +++++++
> kernel/bpf/core.c | 9 ++++
> kernel/bpf/syscall.c | 86 +++++++++++++++++++++++++++++++++++-
> kernel/bpf/verifier.c | 50 +++++++++++++++++++++
> 8 files changed, 176 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 9b558713447f..e9c63ffa01af 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -308,6 +308,8 @@ struct bpf_prog_aux {
> void *security;
> #endif
> struct bpf_prog_offload *offload;
> + struct btf *btf;
> + u32 type_id; /* type id for this prog/func */
> union {
> struct work_struct work;
> struct rcu_head rcu;
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 9e8056ec20fa..e84782ec50ac 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -201,6 +201,7 @@ static inline bool bpf_verifier_log_needed(const struct bpf_verifier_log *log)
> struct bpf_subprog_info {
> u32 start; /* insn idx of function entry point */
> u16 stack_depth; /* max. stack depth used by this function */
> + u32 type_id; /* btf type_id for this subprog */
> };
>
> /* single container for all structs
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index e076c4697049..90e91b52aa90 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -46,5 +46,7 @@ void btf_type_seq_show(const struct btf *btf, u32 type_id, void *obj,
> struct seq_file *m);
> int btf_get_fd_by_id(u32 id);
> u32 btf_id(const struct btf *btf);
> +bool is_btf_func_type(const struct btf *btf, u32 type_id);
> +const char *btf_get_name_by_id(const struct btf *btf, u32 type_id);
>
> #endif
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index f9187b41dff6..7ebbf4f06a65 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -332,6 +332,9 @@ union bpf_attr {
> * (context accesses, allowed helpers, etc).
> */
> __u32 expected_attach_type;
> + __u32 prog_btf_fd; /* fd pointing to BTF type data */
> + __u32 func_info_len; /* func_info length */
> + __aligned_u64 func_info; /* func type info */
> };
>
> struct { /* anonymous struct used by BPF_OBJ_* commands */
> @@ -2585,6 +2588,9 @@ struct bpf_prog_info {
> __u32 nr_jited_func_lens;
> __aligned_u64 jited_ksyms;
> __aligned_u64 jited_func_lens;
> + __u32 btf_id;
> + __u32 nr_jited_func_types;
> + __aligned_u64 jited_func_types;
> } __attribute__((aligned(8)));
>
> struct bpf_map_info {
> @@ -2896,4 +2902,9 @@ struct bpf_flow_keys {
> };
> };
>
> +struct bpf_func_info {
> + __u32 insn_offset;
> + __u32 type_id;
> +};
> +
> #endif /* _UAPI__LINUX_BPF_H__ */
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 794a185f11bf..85b8eeccddbd 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -486,6 +486,15 @@ static const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id)
> return btf->types[type_id];
> }
>
> +bool is_btf_func_type(const struct btf *btf, u32 type_id)
> +{
> + const struct btf_type *type = btf_type_by_id(btf, type_id);
> +
> + if (!type || BTF_INFO_KIND(type->info) != BTF_KIND_FUNC)
> + return false;
> + return true;
> +}
Can btf_type_is_func() (from patch 2) be reused?
The btf_type_by_id() can be done by the caller.
I don't think it worths to add a similar helper
for just one user for now.
The !type check can be added to btf_type_is_func() if
it is needed.
> +
> /*
> * Regular int is not a bit field and it must be either
> * u8/u16/u32/u64.
> @@ -2579,3 +2588,10 @@ u32 btf_id(const struct btf *btf)
> {
> return btf->id;
> }
> +
> +const char *btf_get_name_by_id(const struct btf *btf, u32 type_id)
> +{
> + const struct btf_type *t = btf_type_by_id(btf, type_id);
> +
> + return btf_name_by_offset(btf, t->name_off);
> +}
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 3f5bf1af0826..add3866a120e 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -27,6 +27,7 @@
> #include <linux/random.h>
> #include <linux/moduleloader.h>
> #include <linux/bpf.h>
> +#include <linux/btf.h>
> #include <linux/frame.h>
> #include <linux/rbtree_latch.h>
> #include <linux/kallsyms.h>
> @@ -387,6 +388,7 @@ bpf_get_prog_addr_region(const struct bpf_prog *prog,
> static void bpf_get_prog_name(const struct bpf_prog *prog, char *sym)
> {
> const char *end = sym + KSYM_NAME_LEN;
> + const char *func_name;
>
> BUILD_BUG_ON(sizeof("bpf_prog_") +
> sizeof(prog->tag) * 2 +
> @@ -401,6 +403,13 @@ static void bpf_get_prog_name(const struct bpf_prog *prog, char *sym)
>
> sym += snprintf(sym, KSYM_NAME_LEN, "bpf_prog_");
> sym = bin2hex(sym, prog->tag, sizeof(prog->tag));
> +
> + if (prog->aux->btf) {
> + func_name = btf_get_name_by_id(prog->aux->btf, prog->aux->type_id);
> + snprintf(sym, (size_t)(end - sym), "_%s", func_name);
> + return;
> + }
> +
> if (prog->aux->name[0])
> snprintf(sym, (size_t)(end - sym), "_%s", prog->aux->name);
> else
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 4f416234251f..aa4688a1a137 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1120,6 +1120,7 @@ static void __bpf_prog_put(struct bpf_prog *prog, bool do_idr_lock)
> /* bpf_prog_free_id() must be called first */
> bpf_prog_free_id(prog, do_idr_lock);
> bpf_prog_kallsyms_del_all(prog);
> + btf_put(prog->aux->btf);
>
> call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu);
> }
> @@ -1343,8 +1344,45 @@ bpf_prog_load_check_attach_type(enum bpf_prog_type prog_type,
> }
> }
>
> +static int prog_check_btf(const struct bpf_prog *prog, const struct btf *btf,
> + union bpf_attr *attr)
> +{
> + struct bpf_func_info __user *uinfo, info;
> + int i, nfuncs, usize;
> + u32 prev_offset;
> +
> + usize = sizeof(struct bpf_func_info);
> + if (attr->func_info_len % usize)
> + return -EINVAL;
> +
> + /* func_info section should have increasing and valid insn_offset
> + * and type should be BTF_KIND_FUNC.
> + */
> + nfuncs = attr->func_info_len / usize;
> + uinfo = u64_to_user_ptr(attr->func_info);
> + for (i = 0; i < nfuncs; i++) {
> + if (copy_from_user(&info, uinfo, usize))
> + return -EFAULT;
> +
> + if (!is_btf_func_type(btf, info.type_id))
> + return -EINVAL;
> +
> + if (i == 0) {
> + if (info.insn_offset)
> + return -EINVAL;
> + prev_offset = 0;
> + } else if (info.insn_offset < prev_offset) {
> + return -EINVAL;
> + }
> +
> + prev_offset = info.insn_offset;
> + }
> +
> + return 0;
> +}
> +
> /* last field in 'union bpf_attr' used by this command */
> -#define BPF_PROG_LOAD_LAST_FIELD expected_attach_type
> +#define BPF_PROG_LOAD_LAST_FIELD func_info
>
> static int bpf_prog_load(union bpf_attr *attr)
> {
> @@ -1431,6 +1469,27 @@ static int bpf_prog_load(union bpf_attr *attr)
> if (err)
> goto free_prog;
>
> + /* copy func_info before verifier which may make
> + * some adjustment.
> + */
Is it a left over comment? I don't see the intention of
copying func_info to avoid verifier modification from below.
I could be missing something.
or should the comments be moved to the new "check_btf_func()" below?
> + if (attr->func_info_len) {
> + struct btf *btf;
> +
> + btf = btf_get_by_fd(attr->prog_btf_fd);
> + if (IS_ERR(btf)) {
> + err = PTR_ERR(btf);
> + goto free_prog;
> + }
> +
> + err = prog_check_btf(prog, btf, attr);
> + if (err) {
> + btf_put(btf);
> + goto free_prog;
> + }
> +
> + prog->aux->btf = btf;
> + }
> +
> /* run eBPF verifier */
> err = bpf_check(&prog, attr);
> if (err < 0)
> @@ -1463,6 +1522,7 @@ static int bpf_prog_load(union bpf_attr *attr)
> bpf_prog_kallsyms_del_subprogs(prog);
> free_used_maps(prog->aux);
> free_prog:
> + btf_put(prog->aux->btf);
> bpf_prog_uncharge_memlock(prog);
> free_prog_sec:
> security_bpf_prog_free(prog->aux);
> @@ -2108,6 +2168,30 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
> }
> }
>
> + if (prog->aux->btf) {
> + info.btf_id = btf_id(prog->aux->btf);
> +
> + ulen = info.nr_jited_func_types;
> + info.nr_jited_func_types = prog->aux->func_cnt;
> + if (info.nr_jited_func_types && ulen) {
> + if (bpf_dump_raw_ok()) {
> + u32 __user *user_types;
> + u32 func_type, i;
> +
> + ulen = min_t(u32, info.nr_jited_func_types,
> + ulen);
> + user_types = u64_to_user_ptr(info.jited_func_types);
> + for (i = 0; i < ulen; i++) {
> + func_type = prog->aux->func[i]->aux->type_id;
> + if (put_user(func_type, &user_types[i]))
> + return -EFAULT;
> + }
> + } else {
> + info.jited_func_types = 0;
> + }
> + }
> + }
> +
> done:
> if (copy_to_user(uinfo, &info, info_len) ||
> put_user(info_len, &uattr->info.info_len))
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 3f93a548a642..97c408e84322 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4589,6 +4589,50 @@ static int check_cfg(struct bpf_verifier_env *env)
> return ret;
> }
>
> +static int check_btf_func(struct bpf_prog *prog, struct bpf_verifier_env *env,
> + union bpf_attr *attr)
> +{
> + struct bpf_func_info *data;
> + int i, nfuncs, ret = 0;
> +
> + if (!attr->func_info_len)
> + return 0;
> +
> + nfuncs = attr->func_info_len / sizeof(struct bpf_func_info);
> + if (env->subprog_cnt != nfuncs) {
> + verbose(env, "number of funcs in func_info does not match verifier\n");
> + return -EINVAL;
> + }
> +
> + data = kvmalloc(attr->func_info_len, GFP_KERNEL | __GFP_NOWARN);
> + if (!data) {
> + verbose(env, "no memory to allocate attr func_info\n");
> + return -ENOMEM;
> + }
> +
> + if (copy_from_user(data, u64_to_user_ptr(attr->func_info),
> + attr->func_info_len)) {
> + verbose(env, "memory copy error for attr func_info\n");
> + ret = -EFAULT;
> + goto cleanup;
> + }
Extra indentation.
> +
> + for (i = 0; i < nfuncs; i++) {
> + if (env->subprog_info[i].start != data[i].insn_offset) {
> + verbose(env, "func_info subprog start (%d) does not match verifier (%d)\n",
> + env->subprog_info[i].start, data[i].insn_offset);
The printing args are swapped? env->subprog_info[i].start should
go to the "verifier (%d)"?
and s/%d/%u/
> + ret = -EINVAL;
> + goto cleanup;
> + }
> + env->subprog_info[i].type_id = data[i].type_id;
> + }
> +
> + prog->aux->type_id = data[0].type_id;
> +cleanup:
> + kvfree(data);
> + return ret;
> +}
> +
> /* check %cur's range satisfies %old's */
> static bool range_within(struct bpf_reg_state *old,
> struct bpf_reg_state *cur)
> @@ -5873,6 +5917,8 @@ static int jit_subprogs(struct bpf_verifier_env *env)
> func[i]->aux->name[0] = 'F';
> func[i]->aux->stack_depth = env->subprog_info[i].stack_depth;
> func[i]->jit_requested = 1;
> + func[i]->aux->btf = prog->aux->btf;
> + func[i]->aux->type_id = env->subprog_info[i].type_id;
> func[i] = bpf_int_jit_compile(func[i]);
> if (!func[i]->jited) {
> err = -ENOTSUPP;
> @@ -6307,6 +6353,10 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr)
> if (ret < 0)
> goto skip_full_check;
>
> + ret = check_btf_func(env->prog, env, attr);
> + if (ret < 0)
> + goto skip_full_check;
> +
> ret = do_check(env);
> if (env->cur_state) {
> free_verifier_state(env->cur_state, true);
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next 05/13] bpf: get better bpf_prog ksyms based on btf func type_id
2018-10-15 23:12 ` [PATCH bpf-next 05/13] bpf: get better bpf_prog ksyms based on btf func type_id Martin Lau
@ 2018-10-17 0:32 ` Yonghong Song
2018-10-17 17:14 ` Martin Lau
0 siblings, 1 reply; 10+ messages in thread
From: Yonghong Song @ 2018-10-17 0:32 UTC (permalink / raw)
To: Martin Lau; +Cc: Alexei Starovoitov, daniel, netdev, Kernel Team
On 10/15/18 4:12 PM, Martin Lau wrote:
> On Fri, Oct 12, 2018 at 11:54:42AM -0700, Yonghong Song wrote:
>> This patch added interface to load a program with the following
>> additional information:
>> . prog_btf_fd
>> . func_info and func_info_len
>> where func_info will provides function range and type_id
>> corresponding to each function.
>>
>> If verifier agrees with function range provided by the user,
>> the bpf_prog ksym for each function will use the func name
>> provided in the type_id, which is supposed to provide better
>> encoding as it is not limited by 16 bytes program name
>> limitation and this is better for bpf program which contains
>> multiple subprograms.
>>
>> The bpf_prog_info interface is also extended to
>> return btf_id and jited_func_types, so user spaces can
>> print out the function prototype for each jited function.
> Some nits.
>
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>> include/linux/bpf.h | 2 +
>> include/linux/bpf_verifier.h | 1 +
>> include/linux/btf.h | 2 +
>> include/uapi/linux/bpf.h | 11 +++++
>> kernel/bpf/btf.c | 16 +++++++
>> kernel/bpf/core.c | 9 ++++
>> kernel/bpf/syscall.c | 86 +++++++++++++++++++++++++++++++++++-
>> kernel/bpf/verifier.c | 50 +++++++++++++++++++++
>> 8 files changed, 176 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 9b558713447f..e9c63ffa01af 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -308,6 +308,8 @@ struct bpf_prog_aux {
>> void *security;
>> #endif
>> struct bpf_prog_offload *offload;
>> + struct btf *btf;
>> + u32 type_id; /* type id for this prog/func */
>> union {
>> struct work_struct work;
>> struct rcu_head rcu;
>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>> index 9e8056ec20fa..e84782ec50ac 100644
>> --- a/include/linux/bpf_verifier.h
>> +++ b/include/linux/bpf_verifier.h
>> @@ -201,6 +201,7 @@ static inline bool bpf_verifier_log_needed(const struct bpf_verifier_log *log)
>> struct bpf_subprog_info {
>> u32 start; /* insn idx of function entry point */
>> u16 stack_depth; /* max. stack depth used by this function */
>> + u32 type_id; /* btf type_id for this subprog */
>> };
>>
>> /* single container for all structs
>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>> index e076c4697049..90e91b52aa90 100644
>> --- a/include/linux/btf.h
>> +++ b/include/linux/btf.h
>> @@ -46,5 +46,7 @@ void btf_type_seq_show(const struct btf *btf, u32 type_id, void *obj,
>> struct seq_file *m);
>> int btf_get_fd_by_id(u32 id);
>> u32 btf_id(const struct btf *btf);
>> +bool is_btf_func_type(const struct btf *btf, u32 type_id);
>> +const char *btf_get_name_by_id(const struct btf *btf, u32 type_id);
>>
>> #endif
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index f9187b41dff6..7ebbf4f06a65 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -332,6 +332,9 @@ union bpf_attr {
>> * (context accesses, allowed helpers, etc).
>> */
>> __u32 expected_attach_type;
>> + __u32 prog_btf_fd; /* fd pointing to BTF type data */
>> + __u32 func_info_len; /* func_info length */
>> + __aligned_u64 func_info; /* func type info */
>> };
>>
>> struct { /* anonymous struct used by BPF_OBJ_* commands */
>> @@ -2585,6 +2588,9 @@ struct bpf_prog_info {
>> __u32 nr_jited_func_lens;
>> __aligned_u64 jited_ksyms;
>> __aligned_u64 jited_func_lens;
>> + __u32 btf_id;
>> + __u32 nr_jited_func_types;
>> + __aligned_u64 jited_func_types;
>> } __attribute__((aligned(8)));
>>
>> struct bpf_map_info {
>> @@ -2896,4 +2902,9 @@ struct bpf_flow_keys {
>> };
>> };
>>
>> +struct bpf_func_info {
>> + __u32 insn_offset;
>> + __u32 type_id;
>> +};
>> +
>> #endif /* _UAPI__LINUX_BPF_H__ */
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index 794a185f11bf..85b8eeccddbd 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -486,6 +486,15 @@ static const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id)
>> return btf->types[type_id];
>> }
>>
>> +bool is_btf_func_type(const struct btf *btf, u32 type_id)
>> +{
>> + const struct btf_type *type = btf_type_by_id(btf, type_id);
>> +
>> + if (!type || BTF_INFO_KIND(type->info) != BTF_KIND_FUNC)
>> + return false;
>> + return true;
>> +}
> Can btf_type_is_func() (from patch 2) be reused?
> The btf_type_by_id() can be done by the caller.
> I don't think it worths to add a similar helper
> for just one user for now.
Currently, btf_type_by_id() is not exposed.
bpf/btf.c:
static const struct btf_type *btf_type_by_id(const struct btf *btf, u32
type_id)
Do you want to expose this function as global one?
We cannot put the whole definition in the header as it touches
btf internals.
>
> The !type check can be added to btf_type_is_func() if
> it is needed.
>
>> +
>> /*
>> * Regular int is not a bit field and it must be either
>> * u8/u16/u32/u64.
>> @@ -2579,3 +2588,10 @@ u32 btf_id(const struct btf *btf)
>> {
>> return btf->id;
>> }
>> +
>> +const char *btf_get_name_by_id(const struct btf *btf, u32 type_id)
>> +{
>> + const struct btf_type *t = btf_type_by_id(btf, type_id);
>> +
>> + return btf_name_by_offset(btf, t->name_off);
>> +}
>> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
>> index 3f5bf1af0826..add3866a120e 100644
>> --- a/kernel/bpf/core.c
>> +++ b/kernel/bpf/core.c
>> @@ -27,6 +27,7 @@
>> #include <linux/random.h>
>> #include <linux/moduleloader.h>
>> #include <linux/bpf.h>
>> +#include <linux/btf.h>
>> #include <linux/frame.h>
>> #include <linux/rbtree_latch.h>
>> #include <linux/kallsyms.h>
>> @@ -387,6 +388,7 @@ bpf_get_prog_addr_region(const struct bpf_prog *prog,
>> static void bpf_get_prog_name(const struct bpf_prog *prog, char *sym)
>> {
>> const char *end = sym + KSYM_NAME_LEN;
>> + const char *func_name;
>>
>> BUILD_BUG_ON(sizeof("bpf_prog_") +
>> sizeof(prog->tag) * 2 +
>> @@ -401,6 +403,13 @@ static void bpf_get_prog_name(const struct bpf_prog *prog, char *sym)
>>
>> sym += snprintf(sym, KSYM_NAME_LEN, "bpf_prog_");
>> sym = bin2hex(sym, prog->tag, sizeof(prog->tag));
>> +
>> + if (prog->aux->btf) {
>> + func_name = btf_get_name_by_id(prog->aux->btf, prog->aux->type_id);
>> + snprintf(sym, (size_t)(end - sym), "_%s", func_name);
>> + return;
>> + }
>> +
>> if (prog->aux->name[0])
>> snprintf(sym, (size_t)(end - sym), "_%s", prog->aux->name);
>> else
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 4f416234251f..aa4688a1a137 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -1120,6 +1120,7 @@ static void __bpf_prog_put(struct bpf_prog *prog, bool do_idr_lock)
>> /* bpf_prog_free_id() must be called first */
>> bpf_prog_free_id(prog, do_idr_lock);
>> bpf_prog_kallsyms_del_all(prog);
>> + btf_put(prog->aux->btf);
>>
>> call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu);
>> }
>> @@ -1343,8 +1344,45 @@ bpf_prog_load_check_attach_type(enum bpf_prog_type prog_type,
>> }
>> }
>>
>> +static int prog_check_btf(const struct bpf_prog *prog, const struct btf *btf,
>> + union bpf_attr *attr)
>> +{
>> + struct bpf_func_info __user *uinfo, info;
>> + int i, nfuncs, usize;
>> + u32 prev_offset;
>> +
>> + usize = sizeof(struct bpf_func_info);
>> + if (attr->func_info_len % usize)
>> + return -EINVAL;
>> +
>> + /* func_info section should have increasing and valid insn_offset
>> + * and type should be BTF_KIND_FUNC.
>> + */
>> + nfuncs = attr->func_info_len / usize;
>> + uinfo = u64_to_user_ptr(attr->func_info);
>> + for (i = 0; i < nfuncs; i++) {
>> + if (copy_from_user(&info, uinfo, usize))
>> + return -EFAULT;
>> +
>> + if (!is_btf_func_type(btf, info.type_id))
>> + return -EINVAL;
>> +
>> + if (i == 0) {
>> + if (info.insn_offset)
>> + return -EINVAL;
>> + prev_offset = 0;
>> + } else if (info.insn_offset < prev_offset) {
>> + return -EINVAL;
>> + }
>> +
>> + prev_offset = info.insn_offset;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> /* last field in 'union bpf_attr' used by this command */
>> -#define BPF_PROG_LOAD_LAST_FIELD expected_attach_type
>> +#define BPF_PROG_LOAD_LAST_FIELD func_info
>>
>> static int bpf_prog_load(union bpf_attr *attr)
>> {
>> @@ -1431,6 +1469,27 @@ static int bpf_prog_load(union bpf_attr *attr)
>> if (err)
>> goto free_prog;
>>
>> + /* copy func_info before verifier which may make
>> + * some adjustment.
>> + */
> Is it a left over comment? I don't see the intention of
> copying func_info to avoid verifier modification from below.
> I could be missing something.
>
> or should the comments be moved to the new "check_btf_func()" below?
>
>> + if (attr->func_info_len) {
>> + struct btf *btf;
>> +
>> + btf = btf_get_by_fd(attr->prog_btf_fd);
>> + if (IS_ERR(btf)) {
>> + err = PTR_ERR(btf);
>> + goto free_prog;
>> + }
>> +
>> + err = prog_check_btf(prog, btf, attr);
>> + if (err) {
>> + btf_put(btf);
>> + goto free_prog;
>> + }
>> +
>> + prog->aux->btf = btf;
>> + }
>> +
>> /* run eBPF verifier */
>> err = bpf_check(&prog, attr);
>> if (err < 0)
>> @@ -1463,6 +1522,7 @@ static int bpf_prog_load(union bpf_attr *attr)
>> bpf_prog_kallsyms_del_subprogs(prog);
>> free_used_maps(prog->aux);
>> free_prog:
>> + btf_put(prog->aux->btf);
>> bpf_prog_uncharge_memlock(prog);
>> free_prog_sec:
>> security_bpf_prog_free(prog->aux);
>> @@ -2108,6 +2168,30 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
>> }
>> }
>>
>> + if (prog->aux->btf) {
>> + info.btf_id = btf_id(prog->aux->btf);
>> +
>> + ulen = info.nr_jited_func_types;
>> + info.nr_jited_func_types = prog->aux->func_cnt;
>> + if (info.nr_jited_func_types && ulen) {
>> + if (bpf_dump_raw_ok()) {
>> + u32 __user *user_types;
>> + u32 func_type, i;
>> +
>> + ulen = min_t(u32, info.nr_jited_func_types,
>> + ulen);
>> + user_types = u64_to_user_ptr(info.jited_func_types);
>> + for (i = 0; i < ulen; i++) {
>> + func_type = prog->aux->func[i]->aux->type_id;
>> + if (put_user(func_type, &user_types[i]))
>> + return -EFAULT;
>> + }
>> + } else {
>> + info.jited_func_types = 0;
>> + }
>> + }
>> + }
>> +
>> done:
>> if (copy_to_user(uinfo, &info, info_len) ||
>> put_user(info_len, &uattr->info.info_len))
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 3f93a548a642..97c408e84322 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -4589,6 +4589,50 @@ static int check_cfg(struct bpf_verifier_env *env)
>> return ret;
>> }
>>
>> +static int check_btf_func(struct bpf_prog *prog, struct bpf_verifier_env *env,
>> + union bpf_attr *attr)
>> +{
>> + struct bpf_func_info *data;
>> + int i, nfuncs, ret = 0;
>> +
>> + if (!attr->func_info_len)
>> + return 0;
>> +
>> + nfuncs = attr->func_info_len / sizeof(struct bpf_func_info);
>> + if (env->subprog_cnt != nfuncs) {
>> + verbose(env, "number of funcs in func_info does not match verifier\n");
>> + return -EINVAL;
>> + }
>> +
>> + data = kvmalloc(attr->func_info_len, GFP_KERNEL | __GFP_NOWARN);
>> + if (!data) {
>> + verbose(env, "no memory to allocate attr func_info\n");
>> + return -ENOMEM;
>> + }
>> +
>> + if (copy_from_user(data, u64_to_user_ptr(attr->func_info),
>> + attr->func_info_len)) {
>> + verbose(env, "memory copy error for attr func_info\n");
>> + ret = -EFAULT;
>> + goto cleanup;
>> + }
> Extra indentation.
>
>> +
>> + for (i = 0; i < nfuncs; i++) {
>> + if (env->subprog_info[i].start != data[i].insn_offset) {
>> + verbose(env, "func_info subprog start (%d) does not match verifier (%d)\n",
>> + env->subprog_info[i].start, data[i].insn_offset);
> The printing args are swapped? env->subprog_info[i].start should
> go to the "verifier (%d)"?
>
> and s/%d/%u/
>
>> + ret = -EINVAL;
>> + goto cleanup;
>> + }
>> + env->subprog_info[i].type_id = data[i].type_id;
>> + }
>> +
>> + prog->aux->type_id = data[0].type_id;
>> +cleanup:
>> + kvfree(data);
>> + return ret;
>> +}
>> +
>> /* check %cur's range satisfies %old's */
>> static bool range_within(struct bpf_reg_state *old,
>> struct bpf_reg_state *cur)
>> @@ -5873,6 +5917,8 @@ static int jit_subprogs(struct bpf_verifier_env *env)
>> func[i]->aux->name[0] = 'F';
>> func[i]->aux->stack_depth = env->subprog_info[i].stack_depth;
>> func[i]->jit_requested = 1;
>> + func[i]->aux->btf = prog->aux->btf;
>> + func[i]->aux->type_id = env->subprog_info[i].type_id;
>> func[i] = bpf_int_jit_compile(func[i]);
>> if (!func[i]->jited) {
>> err = -ENOTSUPP;
>> @@ -6307,6 +6353,10 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr)
>> if (ret < 0)
>> goto skip_full_check;
>>
>> + ret = check_btf_func(env->prog, env, attr);
>> + if (ret < 0)
>> + goto skip_full_check;
>> +
>> ret = do_check(env);
>> if (env->cur_state) {
>> free_verifier_state(env->cur_state, true);
>> --
>> 2.17.1
>>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next 05/13] bpf: get better bpf_prog ksyms based on btf func type_id
2018-10-17 0:32 ` Yonghong Song
@ 2018-10-17 17:14 ` Martin Lau
0 siblings, 0 replies; 10+ messages in thread
From: Martin Lau @ 2018-10-17 17:14 UTC (permalink / raw)
To: Yonghong Song; +Cc: Alexei Starovoitov, daniel, netdev, Kernel Team
On Tue, Oct 16, 2018 at 05:32:13PM -0700, Yonghong Song wrote:
>
>
> On 10/15/18 4:12 PM, Martin Lau wrote:
> > On Fri, Oct 12, 2018 at 11:54:42AM -0700, Yonghong Song wrote:
> >> This patch added interface to load a program with the following
> >> additional information:
> >> . prog_btf_fd
> >> . func_info and func_info_len
> >> where func_info will provides function range and type_id
> >> corresponding to each function.
> >>
> >> If verifier agrees with function range provided by the user,
> >> the bpf_prog ksym for each function will use the func name
> >> provided in the type_id, which is supposed to provide better
> >> encoding as it is not limited by 16 bytes program name
> >> limitation and this is better for bpf program which contains
> >> multiple subprograms.
> >>
> >> The bpf_prog_info interface is also extended to
> >> return btf_id and jited_func_types, so user spaces can
> >> print out the function prototype for each jited function.
> > Some nits.
> >
> >>
> >> Signed-off-by: Yonghong Song <yhs@fb.com>
> >> ---
> >> include/linux/bpf.h | 2 +
> >> include/linux/bpf_verifier.h | 1 +
> >> include/linux/btf.h | 2 +
> >> include/uapi/linux/bpf.h | 11 +++++
> >> kernel/bpf/btf.c | 16 +++++++
> >> kernel/bpf/core.c | 9 ++++
> >> kernel/bpf/syscall.c | 86 +++++++++++++++++++++++++++++++++++-
> >> kernel/bpf/verifier.c | 50 +++++++++++++++++++++
> >> 8 files changed, 176 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> >> index 9b558713447f..e9c63ffa01af 100644
> >> --- a/include/linux/bpf.h
> >> +++ b/include/linux/bpf.h
> >> @@ -308,6 +308,8 @@ struct bpf_prog_aux {
> >> void *security;
> >> #endif
> >> struct bpf_prog_offload *offload;
> >> + struct btf *btf;
> >> + u32 type_id; /* type id for this prog/func */
> >> union {
> >> struct work_struct work;
> >> struct rcu_head rcu;
> >> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> >> index 9e8056ec20fa..e84782ec50ac 100644
> >> --- a/include/linux/bpf_verifier.h
> >> +++ b/include/linux/bpf_verifier.h
> >> @@ -201,6 +201,7 @@ static inline bool bpf_verifier_log_needed(const struct bpf_verifier_log *log)
> >> struct bpf_subprog_info {
> >> u32 start; /* insn idx of function entry point */
> >> u16 stack_depth; /* max. stack depth used by this function */
> >> + u32 type_id; /* btf type_id for this subprog */
> >> };
> >>
> >> /* single container for all structs
> >> diff --git a/include/linux/btf.h b/include/linux/btf.h
> >> index e076c4697049..90e91b52aa90 100644
> >> --- a/include/linux/btf.h
> >> +++ b/include/linux/btf.h
> >> @@ -46,5 +46,7 @@ void btf_type_seq_show(const struct btf *btf, u32 type_id, void *obj,
> >> struct seq_file *m);
> >> int btf_get_fd_by_id(u32 id);
> >> u32 btf_id(const struct btf *btf);
> >> +bool is_btf_func_type(const struct btf *btf, u32 type_id);
> >> +const char *btf_get_name_by_id(const struct btf *btf, u32 type_id);
> >>
> >> #endif
> >> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> >> index f9187b41dff6..7ebbf4f06a65 100644
> >> --- a/include/uapi/linux/bpf.h
> >> +++ b/include/uapi/linux/bpf.h
> >> @@ -332,6 +332,9 @@ union bpf_attr {
> >> * (context accesses, allowed helpers, etc).
> >> */
> >> __u32 expected_attach_type;
> >> + __u32 prog_btf_fd; /* fd pointing to BTF type data */
> >> + __u32 func_info_len; /* func_info length */
> >> + __aligned_u64 func_info; /* func type info */
> >> };
> >>
> >> struct { /* anonymous struct used by BPF_OBJ_* commands */
> >> @@ -2585,6 +2588,9 @@ struct bpf_prog_info {
> >> __u32 nr_jited_func_lens;
> >> __aligned_u64 jited_ksyms;
> >> __aligned_u64 jited_func_lens;
> >> + __u32 btf_id;
> >> + __u32 nr_jited_func_types;
> >> + __aligned_u64 jited_func_types;
> >> } __attribute__((aligned(8)));
> >>
> >> struct bpf_map_info {
> >> @@ -2896,4 +2902,9 @@ struct bpf_flow_keys {
> >> };
> >> };
> >>
> >> +struct bpf_func_info {
> >> + __u32 insn_offset;
> >> + __u32 type_id;
> >> +};
> >> +
> >> #endif /* _UAPI__LINUX_BPF_H__ */
> >> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> >> index 794a185f11bf..85b8eeccddbd 100644
> >> --- a/kernel/bpf/btf.c
> >> +++ b/kernel/bpf/btf.c
> >> @@ -486,6 +486,15 @@ static const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id)
> >> return btf->types[type_id];
> >> }
> >>
> >> +bool is_btf_func_type(const struct btf *btf, u32 type_id)
> >> +{
> >> + const struct btf_type *type = btf_type_by_id(btf, type_id);
> >> +
> >> + if (!type || BTF_INFO_KIND(type->info) != BTF_KIND_FUNC)
> >> + return false;
> >> + return true;
> >> +}
> > Can btf_type_is_func() (from patch 2) be reused?
> > The btf_type_by_id() can be done by the caller.
> > I don't think it worths to add a similar helper
> > for just one user for now.
>
> Currently, btf_type_by_id() is not exposed.
>
> bpf/btf.c:
> static const struct btf_type *btf_type_by_id(const struct btf *btf, u32
> type_id)
>
> Do you want to expose this function as global one?
> We cannot put the whole definition in the header as it touches
> btf internals.
btf_type_by_id() does not have to be inline. Not a major issue to block
the v2 though. Just in case there is a v3.
>
> >
> > The !type check can be added to btf_type_is_func() if
> > it is needed.
> >
> >> +
> >> /*
> >> * Regular int is not a bit field and it must be either
> >> * u8/u16/u32/u64.
> >> @@ -2579,3 +2588,10 @@ u32 btf_id(const struct btf *btf)
> >> {
> >> return btf->id;
> >> }
> >> +
> >> +const char *btf_get_name_by_id(const struct btf *btf, u32 type_id)
> >> +{
> >> + const struct btf_type *t = btf_type_by_id(btf, type_id);
> >> +
> >> + return btf_name_by_offset(btf, t->name_off);
> >> +}
> >> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> >> index 3f5bf1af0826..add3866a120e 100644
> >> --- a/kernel/bpf/core.c
> >> +++ b/kernel/bpf/core.c
> >> @@ -27,6 +27,7 @@
> >> #include <linux/random.h>
> >> #include <linux/moduleloader.h>
> >> #include <linux/bpf.h>
> >> +#include <linux/btf.h>
> >> #include <linux/frame.h>
> >> #include <linux/rbtree_latch.h>
> >> #include <linux/kallsyms.h>
> >> @@ -387,6 +388,7 @@ bpf_get_prog_addr_region(const struct bpf_prog *prog,
> >> static void bpf_get_prog_name(const struct bpf_prog *prog, char *sym)
> >> {
> >> const char *end = sym + KSYM_NAME_LEN;
> >> + const char *func_name;
> >>
> >> BUILD_BUG_ON(sizeof("bpf_prog_") +
> >> sizeof(prog->tag) * 2 +
> >> @@ -401,6 +403,13 @@ static void bpf_get_prog_name(const struct bpf_prog *prog, char *sym)
> >>
> >> sym += snprintf(sym, KSYM_NAME_LEN, "bpf_prog_");
> >> sym = bin2hex(sym, prog->tag, sizeof(prog->tag));
> >> +
> >> + if (prog->aux->btf) {
> >> + func_name = btf_get_name_by_id(prog->aux->btf, prog->aux->type_id);
> >> + snprintf(sym, (size_t)(end - sym), "_%s", func_name);
> >> + return;
> >> + }
> >> +
> >> if (prog->aux->name[0])
> >> snprintf(sym, (size_t)(end - sym), "_%s", prog->aux->name);
> >> else
> >> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> >> index 4f416234251f..aa4688a1a137 100644
> >> --- a/kernel/bpf/syscall.c
> >> +++ b/kernel/bpf/syscall.c
> >> @@ -1120,6 +1120,7 @@ static void __bpf_prog_put(struct bpf_prog *prog, bool do_idr_lock)
> >> /* bpf_prog_free_id() must be called first */
> >> bpf_prog_free_id(prog, do_idr_lock);
> >> bpf_prog_kallsyms_del_all(prog);
> >> + btf_put(prog->aux->btf);
> >>
> >> call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu);
> >> }
> >> @@ -1343,8 +1344,45 @@ bpf_prog_load_check_attach_type(enum bpf_prog_type prog_type,
> >> }
> >> }
> >>
> >> +static int prog_check_btf(const struct bpf_prog *prog, const struct btf *btf,
> >> + union bpf_attr *attr)
> >> +{
> >> + struct bpf_func_info __user *uinfo, info;
> >> + int i, nfuncs, usize;
> >> + u32 prev_offset;
> >> +
> >> + usize = sizeof(struct bpf_func_info);
> >> + if (attr->func_info_len % usize)
> >> + return -EINVAL;
> >> +
> >> + /* func_info section should have increasing and valid insn_offset
> >> + * and type should be BTF_KIND_FUNC.
> >> + */
> >> + nfuncs = attr->func_info_len / usize;
> >> + uinfo = u64_to_user_ptr(attr->func_info);
> >> + for (i = 0; i < nfuncs; i++) {
> >> + if (copy_from_user(&info, uinfo, usize))
> >> + return -EFAULT;
> >> +
> >> + if (!is_btf_func_type(btf, info.type_id))
> >> + return -EINVAL;
> >> +
> >> + if (i == 0) {
> >> + if (info.insn_offset)
> >> + return -EINVAL;
> >> + prev_offset = 0;
> >> + } else if (info.insn_offset < prev_offset) {
> >> + return -EINVAL;
> >> + }
> >> +
> >> + prev_offset = info.insn_offset;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> /* last field in 'union bpf_attr' used by this command */
> >> -#define BPF_PROG_LOAD_LAST_FIELD expected_attach_type
> >> +#define BPF_PROG_LOAD_LAST_FIELD func_info
> >>
> >> static int bpf_prog_load(union bpf_attr *attr)
> >> {
> >> @@ -1431,6 +1469,27 @@ static int bpf_prog_load(union bpf_attr *attr)
> >> if (err)
> >> goto free_prog;
> >>
> >> + /* copy func_info before verifier which may make
> >> + * some adjustment.
> >> + */
> > Is it a left over comment? I don't see the intention of
> > copying func_info to avoid verifier modification from below.
> > I could be missing something.
> >
> > or should the comments be moved to the new "check_btf_func()" below?
> >
> >> + if (attr->func_info_len) {
> >> + struct btf *btf;
> >> +
> >> + btf = btf_get_by_fd(attr->prog_btf_fd);
> >> + if (IS_ERR(btf)) {
> >> + err = PTR_ERR(btf);
> >> + goto free_prog;
> >> + }
> >> +
> >> + err = prog_check_btf(prog, btf, attr);
> >> + if (err) {
> >> + btf_put(btf);
> >> + goto free_prog;
> >> + }
> >> +
> >> + prog->aux->btf = btf;
> >> + }
> >> +
> >> /* run eBPF verifier */
> >> err = bpf_check(&prog, attr);
> >> if (err < 0)
> >> @@ -1463,6 +1522,7 @@ static int bpf_prog_load(union bpf_attr *attr)
> >> bpf_prog_kallsyms_del_subprogs(prog);
> >> free_used_maps(prog->aux);
> >> free_prog:
> >> + btf_put(prog->aux->btf);
> >> bpf_prog_uncharge_memlock(prog);
> >> free_prog_sec:
> >> security_bpf_prog_free(prog->aux);
> >> @@ -2108,6 +2168,30 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
> >> }
> >> }
> >>
> >> + if (prog->aux->btf) {
> >> + info.btf_id = btf_id(prog->aux->btf);
> >> +
> >> + ulen = info.nr_jited_func_types;
> >> + info.nr_jited_func_types = prog->aux->func_cnt;
> >> + if (info.nr_jited_func_types && ulen) {
> >> + if (bpf_dump_raw_ok()) {
> >> + u32 __user *user_types;
> >> + u32 func_type, i;
> >> +
> >> + ulen = min_t(u32, info.nr_jited_func_types,
> >> + ulen);
> >> + user_types = u64_to_user_ptr(info.jited_func_types);
> >> + for (i = 0; i < ulen; i++) {
> >> + func_type = prog->aux->func[i]->aux->type_id;
> >> + if (put_user(func_type, &user_types[i]))
> >> + return -EFAULT;
> >> + }
> >> + } else {
> >> + info.jited_func_types = 0;
> >> + }
> >> + }
> >> + }
> >> +
> >> done:
> >> if (copy_to_user(uinfo, &info, info_len) ||
> >> put_user(info_len, &uattr->info.info_len))
> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index 3f93a548a642..97c408e84322 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -4589,6 +4589,50 @@ static int check_cfg(struct bpf_verifier_env *env)
> >> return ret;
> >> }
> >>
> >> +static int check_btf_func(struct bpf_prog *prog, struct bpf_verifier_env *env,
> >> + union bpf_attr *attr)
> >> +{
> >> + struct bpf_func_info *data;
> >> + int i, nfuncs, ret = 0;
> >> +
> >> + if (!attr->func_info_len)
> >> + return 0;
> >> +
> >> + nfuncs = attr->func_info_len / sizeof(struct bpf_func_info);
> >> + if (env->subprog_cnt != nfuncs) {
> >> + verbose(env, "number of funcs in func_info does not match verifier\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + data = kvmalloc(attr->func_info_len, GFP_KERNEL | __GFP_NOWARN);
> >> + if (!data) {
> >> + verbose(env, "no memory to allocate attr func_info\n");
> >> + return -ENOMEM;
> >> + }
> >> +
> >> + if (copy_from_user(data, u64_to_user_ptr(attr->func_info),
> >> + attr->func_info_len)) {
> >> + verbose(env, "memory copy error for attr func_info\n");
> >> + ret = -EFAULT;
> >> + goto cleanup;
> >> + }
> > Extra indentation.
> >
> >> +
> >> + for (i = 0; i < nfuncs; i++) {
> >> + if (env->subprog_info[i].start != data[i].insn_offset) {
> >> + verbose(env, "func_info subprog start (%d) does not match verifier (%d)\n",
> >> + env->subprog_info[i].start, data[i].insn_offset);
> > The printing args are swapped? env->subprog_info[i].start should
> > go to the "verifier (%d)"?
> >
> > and s/%d/%u/
> >
> >> + ret = -EINVAL;
> >> + goto cleanup;
> >> + }
> >> + env->subprog_info[i].type_id = data[i].type_id;
> >> + }
> >> +
> >> + prog->aux->type_id = data[0].type_id;
> >> +cleanup:
> >> + kvfree(data);
> >> + return ret;
> >> +}
> >> +
> >> /* check %cur's range satisfies %old's */
> >> static bool range_within(struct bpf_reg_state *old,
> >> struct bpf_reg_state *cur)
> >> @@ -5873,6 +5917,8 @@ static int jit_subprogs(struct bpf_verifier_env *env)
> >> func[i]->aux->name[0] = 'F';
> >> func[i]->aux->stack_depth = env->subprog_info[i].stack_depth;
> >> func[i]->jit_requested = 1;
> >> + func[i]->aux->btf = prog->aux->btf;
> >> + func[i]->aux->type_id = env->subprog_info[i].type_id;
> >> func[i] = bpf_int_jit_compile(func[i]);
> >> if (!func[i]->jited) {
> >> err = -ENOTSUPP;
> >> @@ -6307,6 +6353,10 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr)
> >> if (ret < 0)
> >> goto skip_full_check;
> >>
> >> + ret = check_btf_func(env->prog, env, attr);
> >> + if (ret < 0)
> >> + goto skip_full_check;
> >> +
> >> ret = do_check(env);
> >> if (env->cur_state) {
> >> free_verifier_state(env->cur_state, true);
> >> --
> >> 2.17.1
> >>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 05/13] bpf: get better bpf_prog ksyms based on btf func type_id
2018-10-12 18:54 [PATCH bpf-next 05/13] bpf: get better bpf_prog ksyms based on btf func type_id Yonghong Song
` (4 preceding siblings ...)
2018-10-15 23:12 ` [PATCH bpf-next 05/13] bpf: get better bpf_prog ksyms based on btf func type_id Martin Lau
@ 2018-10-16 17:59 ` Alexei Starovoitov
2018-10-17 3:24 ` Yonghong Song
5 siblings, 1 reply; 10+ messages in thread
From: Alexei Starovoitov @ 2018-10-16 17:59 UTC (permalink / raw)
To: Yonghong Song; +Cc: ast, kafai, daniel, netdev, kernel-team
On Fri, Oct 12, 2018 at 11:54:42AM -0700, Yonghong Song wrote:
> This patch added interface to load a program with the following
> additional information:
> . prog_btf_fd
> . func_info and func_info_len
> where func_info will provides function range and type_id
> corresponding to each function.
>
> If verifier agrees with function range provided by the user,
> the bpf_prog ksym for each function will use the func name
> provided in the type_id, which is supposed to provide better
> encoding as it is not limited by 16 bytes program name
> limitation and this is better for bpf program which contains
> multiple subprograms.
>
> The bpf_prog_info interface is also extended to
> return btf_id and jited_func_types, so user spaces can
> print out the function prototype for each jited function.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
...
> BUILD_BUG_ON(sizeof("bpf_prog_") +
> sizeof(prog->tag) * 2 +
> @@ -401,6 +403,13 @@ static void bpf_get_prog_name(const struct bpf_prog *prog, char *sym)
>
> sym += snprintf(sym, KSYM_NAME_LEN, "bpf_prog_");
> sym = bin2hex(sym, prog->tag, sizeof(prog->tag));
> +
> + if (prog->aux->btf) {
> + func_name = btf_get_name_by_id(prog->aux->btf, prog->aux->type_id);
> + snprintf(sym, (size_t)(end - sym), "_%s", func_name);
> + return;
Would it make sense to add a comment here that prog->aux->name is ignored
when full btf name is available? (otherwise the same name will appear twice in ksym)
> + }
> +
> if (prog->aux->name[0])
> snprintf(sym, (size_t)(end - sym), "_%s", prog->aux->name);
...
> +static int check_btf_func(struct bpf_prog *prog, struct bpf_verifier_env *env,
> + union bpf_attr *attr)
> +{
> + struct bpf_func_info *data;
> + int i, nfuncs, ret = 0;
> +
> + if (!attr->func_info_len)
> + return 0;
> +
> + nfuncs = attr->func_info_len / sizeof(struct bpf_func_info);
> + if (env->subprog_cnt != nfuncs) {
> + verbose(env, "number of funcs in func_info does not match verifier\n");
'does not match verifier' is hard to make sense of.
How about 'number of funcs in func_info doesn't match number of subprogs' ?
> + return -EINVAL;
> + }
> +
> + data = kvmalloc(attr->func_info_len, GFP_KERNEL | __GFP_NOWARN);
> + if (!data) {
> + verbose(env, "no memory to allocate attr func_info\n");
I don't think we ever print such warnings for memory allocations.
imo this can be removed, since enomem is enough.
> + return -ENOMEM;
> + }
> +
> + if (copy_from_user(data, u64_to_user_ptr(attr->func_info),
> + attr->func_info_len)) {
> + verbose(env, "memory copy error for attr func_info\n");
similar thing. kernel never warns about copy_from_user errors.
> + ret = -EFAULT;
> + goto cleanup;
> + }
> +
> + for (i = 0; i < nfuncs; i++) {
> + if (env->subprog_info[i].start != data[i].insn_offset) {
> + verbose(env, "func_info subprog start (%d) does not match verifier (%d)\n",
> + env->subprog_info[i].start, data[i].insn_offset);
I think printing exact insn offset isn't going to be much help
for regular user to debug it. If this happens, it's likely llvm issue.
How about 'func_info BTF section doesn't match subprog layout in BPF program' ?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next 05/13] bpf: get better bpf_prog ksyms based on btf func type_id
2018-10-16 17:59 ` Alexei Starovoitov
@ 2018-10-17 3:24 ` Yonghong Song
0 siblings, 0 replies; 10+ messages in thread
From: Yonghong Song @ 2018-10-17 3:24 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Alexei Starovoitov, Martin Lau, daniel, netdev, Kernel Team
On 10/16/18 10:59 AM, Alexei Starovoitov wrote:
> On Fri, Oct 12, 2018 at 11:54:42AM -0700, Yonghong Song wrote:
>> This patch added interface to load a program with the following
>> additional information:
>> . prog_btf_fd
>> . func_info and func_info_len
>> where func_info will provides function range and type_id
>> corresponding to each function.
>>
>> If verifier agrees with function range provided by the user,
>> the bpf_prog ksym for each function will use the func name
>> provided in the type_id, which is supposed to provide better
>> encoding as it is not limited by 16 bytes program name
>> limitation and this is better for bpf program which contains
>> multiple subprograms.
>>
>> The bpf_prog_info interface is also extended to
>> return btf_id and jited_func_types, so user spaces can
>> print out the function prototype for each jited function.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
> ...
>> BUILD_BUG_ON(sizeof("bpf_prog_") +
>> sizeof(prog->tag) * 2 +
>> @@ -401,6 +403,13 @@ static void bpf_get_prog_name(const struct bpf_prog *prog, char *sym)
>>
>> sym += snprintf(sym, KSYM_NAME_LEN, "bpf_prog_");
>> sym = bin2hex(sym, prog->tag, sizeof(prog->tag));
>> +
>> + if (prog->aux->btf) {
>> + func_name = btf_get_name_by_id(prog->aux->btf, prog->aux->type_id);
>> + snprintf(sym, (size_t)(end - sym), "_%s", func_name);
>> + return;
>
> Would it make sense to add a comment here that prog->aux->name is ignored
> when full btf name is available? (otherwise the same name will appear twice in ksym)
Will add a comment.
>
>> + }
>> +
>> if (prog->aux->name[0])
>> snprintf(sym, (size_t)(end - sym), "_%s", prog->aux->name);
> ...
>> +static int check_btf_func(struct bpf_prog *prog, struct bpf_verifier_env *env,
>> + union bpf_attr *attr)
>> +{
>> + struct bpf_func_info *data;
>> + int i, nfuncs, ret = 0;
>> +
>> + if (!attr->func_info_len)
>> + return 0;
>> +
>> + nfuncs = attr->func_info_len / sizeof(struct bpf_func_info);
>> + if (env->subprog_cnt != nfuncs) {
>> + verbose(env, "number of funcs in func_info does not match verifier\n");
>
> 'does not match verifier' is hard to make sense of.
> How about 'number of funcs in func_info doesn't match number of subprogs' ?
Sounds good to me.
>
>> + return -EINVAL;
>> + }
>> +
>> + data = kvmalloc(attr->func_info_len, GFP_KERNEL | __GFP_NOWARN);
>> + if (!data) {
>> + verbose(env, "no memory to allocate attr func_info\n");
>
> I don't think we ever print such warnings for memory allocations.
> imo this can be removed, since enomem is enough.
Okay.
>
>> + return -ENOMEM;
>> + }
>> +
>> + if (copy_from_user(data, u64_to_user_ptr(attr->func_info),
>> + attr->func_info_len)) {
>> + verbose(env, "memory copy error for attr func_info\n");
>
> similar thing. kernel never warns about copy_from_user errors.
Okay.
>
>> + ret = -EFAULT;
>> + goto cleanup;
>> + }
>> +
>> + for (i = 0; i < nfuncs; i++) {
>> + if (env->subprog_info[i].start != data[i].insn_offset) {
>> + verbose(env, "func_info subprog start (%d) does not match verifier (%d)\n",
>> + env->subprog_info[i].start, data[i].insn_offset);
>
> I think printing exact insn offset isn't going to be much help
> for regular user to debug it. If this happens, it's likely llvm issue.
> How about 'func_info BTF section doesn't match subprog layout in BPF program' ?
Okay.
>
^ permalink raw reply [flat|nested] 10+ messages in thread