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 <[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.
>
> 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
> >>
