From 7337224fc150b3b762190425399ac0e8dee380d1 Mon Sep 17 00:00:00 2001 From: Martin KaFai Lau Date: Wed, 5 Dec 2018 17:35:43 -0800 Subject: [PATCH 1/4] bpf: Improve the info.func_info and info.func_info_rec_size behavior 1) When bpf_dump_raw_ok() == false and the kernel can provide >=1 func_info to the userspace, the current behavior is setting the info.func_info_cnt to 0 instead of setting info.func_info to 0. It is different from the behavior in jited_func_lens/nr_jited_func_lens, jited_ksyms/nr_jited_ksyms...etc. This patch fixes it. (i.e. set func_info to 0 instead of func_info_cnt to 0 when bpf_dump_raw_ok() == false). 2) When the userspace passed in info.func_info_cnt == 0, the kernel will set the expected func_info size back to the info.func_info_rec_size. It is a way for the userspace to learn the kernel expected func_info_rec_size introduced in commit 838e96904ff3 ("bpf: Introduce bpf_func_info"). An exception is the kernel expected size is not set when func_info is not available for a bpf_prog. This makes the returned info.func_info_rec_size has different values depending on the returned value of info.func_info_cnt. This patch sets the kernel expected size to info.func_info_rec_size independent of the info.func_info_cnt. 3) The current logic only rejects invalid func_info_rec_size if func_info_cnt is non zero. This patch also rejects invalid nonzero info.func_info_rec_size and not equal to the kernel expected size. 4) Set info.btf_id as long as prog->aux->btf != NULL. That will setup the later copy_to_user() codes look the same as others which then easier to understand and maintain. prog->aux->btf is not NULL only if prog->aux->func_info_cnt > 0. Breaking up info.btf_id from prog->aux->func_info_cnt is needed for the later line info patch anyway. A similar change is made to bpf_get_prog_name(). Fixes: 838e96904ff3 ("bpf: Introduce bpf_func_info") Signed-off-by: Martin KaFai Lau Acked-by: Yonghong Song Signed-off-by: Alexei Starovoitov --- kernel/bpf/core.c | 2 +- kernel/bpf/syscall.c | 44 +++++++++++++++++++------------------------- 2 files changed, 20 insertions(+), 26 deletions(-) diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index 86817ab204e8..628b3970a49b 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -410,7 +410,7 @@ static void bpf_get_prog_name(const struct bpf_prog *prog, char *sym) sym = bin2hex(sym, prog->tag, sizeof(prog->tag)); /* prog->aux->name will be ignored if full btf name is available */ - if (prog->aux->btf) { + if (prog->aux->func_info_cnt) { type = btf_type_by_id(prog->aux->btf, prog->aux->func_info[prog->aux->func_idx].type_id); func_name = btf_name_by_offset(prog->aux->btf, type->name_off); diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 4445d0d084d8..aa05aa38f4a8 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -2083,6 +2083,12 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog, return -EFAULT; } + if ((info.func_info_cnt || info.func_info_rec_size) && + info.func_info_rec_size != sizeof(struct bpf_func_info)) + return -EINVAL; + + info.func_info_rec_size = sizeof(struct bpf_func_info); + if (!capable(CAP_SYS_ADMIN)) { info.jited_prog_len = 0; info.xlated_prog_len = 0; @@ -2226,35 +2232,23 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog, } } - if (prog->aux->btf) { - u32 krec_size = sizeof(struct bpf_func_info); - u32 ucnt, urec_size; - + if (prog->aux->btf) info.btf_id = btf_id(prog->aux->btf); - ucnt = info.func_info_cnt; - info.func_info_cnt = prog->aux->func_info_cnt; - urec_size = info.func_info_rec_size; - info.func_info_rec_size = krec_size; - if (ucnt) { - /* expect passed-in urec_size is what the kernel expects */ - if (urec_size != info.func_info_rec_size) - return -EINVAL; + ulen = info.func_info_cnt; + info.func_info_cnt = prog->aux->func_info_cnt; + if (info.func_info_cnt && ulen) { + if (bpf_dump_raw_ok()) { + char __user *user_finfo; - if (bpf_dump_raw_ok()) { - char __user *user_finfo; - - user_finfo = u64_to_user_ptr(info.func_info); - ucnt = min_t(u32, info.func_info_cnt, ucnt); - if (copy_to_user(user_finfo, prog->aux->func_info, - krec_size * ucnt)) - return -EFAULT; - } else { - info.func_info_cnt = 0; - } + user_finfo = u64_to_user_ptr(info.func_info); + ulen = min_t(u32, info.func_info_cnt, ulen); + if (copy_to_user(user_finfo, prog->aux->func_info, + info.func_info_rec_size * ulen)) + return -EFAULT; + } else { + info.func_info = 0; } - } else { - info.func_info_cnt = 0; } done: From d30d42e08c76cb9323ec6121190eb026b07f773b Mon Sep 17 00:00:00 2001 From: Martin KaFai Lau Date: Wed, 5 Dec 2018 17:35:44 -0800 Subject: [PATCH 2/4] bpf: Change insn_offset to insn_off in bpf_func_info The later patch will introduce "struct bpf_line_info" which has member "line_off" and "file_off" referring back to the string section in btf. The line_"off" and file_"off" are more consistent to the naming convention in btf.h that means "offset" (e.g. name_off in "struct btf_type"). The to-be-added "struct bpf_line_info" also has another member, "insn_off" which is the same as the "insn_offset" in "struct bpf_func_info". Hence, this patch renames "insn_offset" to "insn_off" for "struct bpf_func_info". Signed-off-by: Martin KaFai Lau Acked-by: Yonghong Song Signed-off-by: Alexei Starovoitov --- include/uapi/linux/bpf.h | 2 +- kernel/bpf/verifier.c | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index c8e1eeee2c5f..a84fd232d934 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -2991,7 +2991,7 @@ struct bpf_flow_keys { }; struct bpf_func_info { - __u32 insn_offset; + __u32 insn_off; __u32 type_id; }; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 71988337ac14..7658c61c1a88 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -4707,24 +4707,24 @@ static int check_btf_func(struct bpf_prog *prog, struct bpf_verifier_env *env, goto free_btf; } - /* check insn_offset */ + /* check insn_off */ if (i == 0) { - if (krecord[i].insn_offset) { + if (krecord[i].insn_off) { verbose(env, - "nonzero insn_offset %u for the first func info record", - krecord[i].insn_offset); + "nonzero insn_off %u for the first func info record", + krecord[i].insn_off); ret = -EINVAL; goto free_btf; } - } else if (krecord[i].insn_offset <= prev_offset) { + } else if (krecord[i].insn_off <= prev_offset) { verbose(env, "same or smaller insn offset (%u) than previous func info record (%u)", - krecord[i].insn_offset, prev_offset); + krecord[i].insn_off, prev_offset); ret = -EINVAL; goto free_btf; } - if (env->subprog_info[i].start != krecord[i].insn_offset) { + if (env->subprog_info[i].start != krecord[i].insn_off) { verbose(env, "func_info BTF section doesn't match subprog layout in BPF program\n"); ret = -EINVAL; goto free_btf; @@ -4739,7 +4739,7 @@ static int check_btf_func(struct bpf_prog *prog, struct bpf_verifier_env *env, goto free_btf; } - prev_offset = krecord[i].insn_offset; + prev_offset = krecord[i].insn_off; urecord += urec_size; } @@ -4762,7 +4762,7 @@ static void adjust_btf_func(struct bpf_verifier_env *env) return; for (i = 0; i < env->subprog_cnt; i++) - env->prog->aux->func_info[i].insn_offset = env->subprog_info[i].start; + env->prog->aux->func_info[i].insn_off = env->subprog_info[i].start; } /* check %cur's range satisfies %old's */ From 555249df711b146a2898949ac80503be29fe7340 Mon Sep 17 00:00:00 2001 From: Martin KaFai Lau Date: Wed, 5 Dec 2018 17:35:45 -0800 Subject: [PATCH 3/4] bpf: tools: Sync uapi bpf.h for the name changes in bpf_func_info This patch sync the name changes in bpf_func_info to the tools/. Signed-off-by: Martin KaFai Lau Acked-by: Yonghong Song Signed-off-by: Alexei Starovoitov --- tools/include/uapi/linux/bpf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 64262890feb2..16263e8827fc 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -2991,7 +2991,7 @@ struct bpf_flow_keys { }; struct bpf_func_info { - __u32 insn_offset; + __u32 insn_off; __u32 type_id; }; From 84ecc1f98ca7ce28ede9bc5cc70a557fdfa09caa Mon Sep 17 00:00:00 2001 From: Martin KaFai Lau Date: Wed, 5 Dec 2018 17:35:47 -0800 Subject: [PATCH 4/4] bpf: Expect !info.func_info and insn_off name changes in test_btf/libbpf/bpftool Similar to info.jited_*, info.func_info could be 0 if bpf_dump_raw_ok() == false. This patch makes changes to test_btf and bpftool to expect info.func_info could be 0. This patch also makes the needed changes for s/insn_offset/insn_off/. Signed-off-by: Martin KaFai Lau Acked-by: Yonghong Song Signed-off-by: Alexei Starovoitov --- tools/bpf/bpftool/prog.c | 7 +++++++ tools/bpf/bpftool/xlated_dumper.c | 4 ++-- tools/lib/bpf/btf.c | 12 ++++++------ tools/testing/selftests/bpf/test_btf.c | 8 +++++++- 4 files changed, 22 insertions(+), 9 deletions(-) diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c index 54c8dbf05c9c..a9a51123454c 100644 --- a/tools/bpf/bpftool/prog.c +++ b/tools/bpf/bpftool/prog.c @@ -589,6 +589,13 @@ static int do_dump(int argc, char **argv) goto err_free; } + if (func_info && !info.func_info) { + /* kernel.kptr_restrict is set. No func_info available. */ + free(func_info); + func_info = NULL; + finfo_cnt = 0; + } + if ((member_len == &info.jited_prog_len && info.jited_prog_insns == 0) || (member_len == &info.xlated_prog_len && diff --git a/tools/bpf/bpftool/xlated_dumper.c b/tools/bpf/bpftool/xlated_dumper.c index e06ac0286a75..131ecd175533 100644 --- a/tools/bpf/bpftool/xlated_dumper.c +++ b/tools/bpf/bpftool/xlated_dumper.c @@ -261,7 +261,7 @@ void dump_xlated_json(struct dump_data *dd, void *buf, unsigned int len, jsonw_start_object(json_wtr); if (btf && record) { - if (record->insn_offset == i) { + if (record->insn_off == i) { btf_dumper_type_only(btf, record->type_id, func_sig, sizeof(func_sig)); @@ -330,7 +330,7 @@ void dump_xlated_plain(struct dump_data *dd, void *buf, unsigned int len, } if (btf && record) { - if (record->insn_offset == i) { + if (record->insn_off == i) { btf_dumper_type_only(btf, record->type_id, func_sig, sizeof(func_sig)); diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c index c2d641f3e16e..85d6446cf832 100644 --- a/tools/lib/bpf/btf.c +++ b/tools/lib/bpf/btf.c @@ -45,7 +45,7 @@ struct btf_ext { /* The minimum bpf_func_info checked by the loader */ struct bpf_func_info_min { - __u32 insn_offset; + __u32 insn_off; __u32 type_id; }; @@ -670,7 +670,7 @@ int btf_ext__reloc_init(struct btf *btf, struct btf_ext *btf_ext, memcpy(data, sinfo->data, records_len); - /* adjust the insn_offset, the data in .BTF.ext is + /* 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. * @@ -681,7 +681,7 @@ int btf_ext__reloc_init(struct btf *btf, struct btf_ext *btf_ext, struct bpf_func_info_min *record; record = data + i * record_size; - record->insn_offset /= sizeof(struct bpf_insn); + record->insn_off /= sizeof(struct bpf_insn); } *func_info = data; @@ -722,15 +722,15 @@ int btf_ext__reloc(struct btf *btf, struct btf_ext *btf_ext, return -ENOMEM; memcpy(data + existing_flen, sinfo->data, records_len); - /* adjust insn_offset only, the rest data will be passed + /* adjust insn_off only, the rest data will be passed * to the kernel. */ for (i = 0; i < sinfo->num_func_info; i++) { struct bpf_func_info_min *record; record = data + existing_flen + i * record_size; - record->insn_offset = - record->insn_offset / sizeof(struct bpf_insn) + + record->insn_off = + record->insn_off / sizeof(struct bpf_insn) + insns_cnt; } *func_info = data; diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c index bae7308b7ec5..ff0952ea757a 100644 --- a/tools/testing/selftests/bpf/test_btf.c +++ b/tools/testing/selftests/bpf/test_btf.c @@ -3156,7 +3156,7 @@ static struct btf_func_type_test { }, { - .descr = "func_type (Incorrect bpf_func_info.insn_offset)", + .descr = "func_type (Incorrect bpf_func_info.insn_off)", .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] */ @@ -3303,6 +3303,12 @@ static int do_test_func_type(int test_num) goto done; } + if (CHECK(!info.func_info, + "info.func_info == 0. kernel.kptr_restrict is set?")) { + err = -1; + goto done; + } + finfo = func_info; for (i = 0; i < 2; i++) { if (CHECK(finfo->type_id != test->func_info[i][1],