Message-ID: <20181015231223.cwe3zblnh4wt7xds@kafai-mbp.dhcp.thefacebook.com>
Date:   Mon, 15 Oct 2018 23:12:25 +0000
From:   Martin Lau <kafai@...com>
To:     Yonghong Song <yhs@...com>
CC:     Alexei Starovoitov <ast@...com>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next 05/13] bpf: get better bpf_prog ksyms based on
 btf func type_id

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

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
> 

Powered by blists - more mailing lists