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 <[email protected]> > --- > 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 >
