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 <y...@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
>>
