Re: [PATCH bpf-next 03/10] bpf: process in-kernel BTF

From: Martin Lau <kafai@fb.com>
To: Alexei Starovoitov <ast@kernel.org>
Cc: "davem@davemloft.net" <davem@davemloft.net>,
	"daniel@iogearbox.net" <daniel@iogearbox.net>,
	"x86@kernel.org" <x86@kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"bpf@vger.kernel.org" <bpf@vger.kernel.org>,
	Kernel Team <Kernel-team@fb.com>
Subject: Re: [PATCH bpf-next 03/10] bpf: process in-kernel BTF
Date: Wed, 9 Oct 2019 20:51:56 +0000	[thread overview]
Message-ID: <20191009205152.kfdkm2pvbyiwfelf@kafai-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <20191005050314.1114330-4-ast@kernel.org>

On Fri, Oct 04, 2019 at 10:03:07PM -0700, Alexei Starovoitov wrote:
> If in-kernel BTF exists parse it and prepare 'struct btf *btf_vmlinux'
> for further use by the verifier.
> In-kernel BTF is trusted just like kallsyms and other build artifacts
> embedded into vmlinux.
> Yet run this BTF image through BTF verifier to make sure
> that it is valid and it wasn't mangled during the build.
> 
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  include/linux/bpf_verifier.h |  4 ++-
>  include/linux/btf.h          |  1 +
>  kernel/bpf/btf.c             | 66 ++++++++++++++++++++++++++++++++++++
>  kernel/bpf/verifier.c        | 18 ++++++++++
>  4 files changed, 88 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 26a6d58ca78c..432ba8977a0a 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -330,10 +330,12 @@ static inline bool bpf_verifier_log_full(const struct bpf_verifier_log *log)
>  #define BPF_LOG_STATS	4
>  #define BPF_LOG_LEVEL	(BPF_LOG_LEVEL1 | BPF_LOG_LEVEL2)
>  #define BPF_LOG_MASK	(BPF_LOG_LEVEL | BPF_LOG_STATS)
> +#define BPF_LOG_KERNEL (BPF_LOG_MASK + 1)
>  
>  static inline bool bpf_verifier_log_needed(const struct bpf_verifier_log *log)
>  {
> -	return log->level && log->ubuf && !bpf_verifier_log_full(log);
> +	return (log->level && log->ubuf && !bpf_verifier_log_full(log)) ||
> +		log->level == BPF_LOG_KERNEL;
>  }
>  
>  #define BPF_MAX_SUBPROGS 256
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 64cdf2a23d42..55d43bc856be 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -56,6 +56,7 @@ bool btf_type_is_void(const struct btf_type *t);
>  #ifdef CONFIG_BPF_SYSCALL
>  const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id);
>  const char *btf_name_by_offset(const struct btf *btf, u32 offset);
> +struct btf *btf_parse_vmlinux(void);
>  #else
>  static inline const struct btf_type *btf_type_by_id(const struct btf *btf,
>  						    u32 type_id)
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 29c7c06c6bd6..848f9d4b9d7e 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -698,6 +698,9 @@ __printf(4, 5) static void __btf_verifier_log_type(struct btf_verifier_env *env,
>  	if (!bpf_verifier_log_needed(log))
>  		return;
>  
> +	if (log->level == BPF_LOG_KERNEL && !fmt)
> +		return;
> +
>  	__btf_verifier_log(log, "[%u] %s %s%s",
>  			   env->log_type_id,
>  			   btf_kind_str[kind],
> @@ -735,6 +738,8 @@ static void btf_verifier_log_member(struct btf_verifier_env *env,
>  	if (!bpf_verifier_log_needed(log))
>  		return;
>  
> +	if (log->level == BPF_LOG_KERNEL && !fmt)
> +		return;
>  	/* The CHECK_META phase already did a btf dump.
>  	 *
>  	 * If member is logged again, it must hit an error in
> @@ -777,6 +782,8 @@ static void btf_verifier_log_vsi(struct btf_verifier_env *env,
>  
>  	if (!bpf_verifier_log_needed(log))
>  		return;
> +	if (log->level == BPF_LOG_KERNEL && !fmt)
> +		return;
>  	if (env->phase != CHECK_META)
>  		btf_verifier_log_type(env, datasec_type, NULL);
>  
> @@ -802,6 +809,8 @@ static void btf_verifier_log_hdr(struct btf_verifier_env *env,
>  	if (!bpf_verifier_log_needed(log))
>  		return;
>  
> +	if (log->level == BPF_LOG_KERNEL)
> +		return;
>  	hdr = &btf->hdr;
>  	__btf_verifier_log(log, "magic: 0x%x\n", hdr->magic);
>  	__btf_verifier_log(log, "version: %u\n", hdr->version);
> @@ -2406,6 +2415,8 @@ static s32 btf_enum_check_meta(struct btf_verifier_env *env,
>  		}
>  
>  
> +		if (env->log.level == BPF_LOG_KERNEL)
> +			continue;
>  		btf_verifier_log(env, "\t%s val=%d\n",
>  				 __btf_name_by_offset(btf, enums[i].name_off),
>  				 enums[i].val);
> @@ -3367,6 +3378,61 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
>  	return ERR_PTR(err);
>  }
>  
> +extern char __weak _binary__btf_vmlinux_bin_start[];
> +extern char __weak _binary__btf_vmlinux_bin_end[];
> +
> +struct btf *btf_parse_vmlinux(void)
> +{
> +	struct btf_verifier_env *env = NULL;
> +	struct bpf_verifier_log *log;
> +	struct btf *btf = NULL;
> +	int err;
> +
> +	env = kzalloc(sizeof(*env), GFP_KERNEL | __GFP_NOWARN);
> +	if (!env)
> +		return ERR_PTR(-ENOMEM);
> +
> +	log = &env->log;
> +	log->level = BPF_LOG_KERNEL;
> +
> +	btf = kzalloc(sizeof(*btf), GFP_KERNEL | __GFP_NOWARN);
> +	if (!btf) {
> +		err = -ENOMEM;
> +		goto errout;
> +	}
> +	env->btf = btf;
> +
> +	btf->data = _binary__btf_vmlinux_bin_start;
> +	btf->data_size = _binary__btf_vmlinux_bin_end -
> +		_binary__btf_vmlinux_bin_start;
> +
> +	err = btf_parse_hdr(env);
> +	if (err)
> +		goto errout;
> +
> +	btf->nohdr_data = btf->data + btf->hdr.hdr_len;
> +
> +	err = btf_parse_str_sec(env);
> +	if (err)
> +		goto errout;
> +
> +	err = btf_check_all_metas(env);
> +	if (err)
> +		goto errout;
> +
Considering btf_vmlinux is already safe, any concern in making an extra
call to btf_check_all_types()?

Having resolved_ids and resolved_sizes available will
be handy in my later patch.

> +	btf_verifier_env_free(env);
> +	refcount_set(&btf->refcnt, 1);
> +	return btf;
> +
> +errout:
> +	btf_verifier_env_free(env);
> +	if (btf) {
> +		kvfree(btf->types);
> +		kfree(btf);
> +	}
> +	return ERR_PTR(err);
> +}
> +