Re: [PATCH bpf-next 05/13] bpf: get better bpf_prog ksyms based on btf func type_id

[Martin Lau <kafai@fb.com>]

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