bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Eduard Zingerman <eddyz87@gmail.com>
To: Alan Maguire <alan.maguire@oracle.com>,
	ast@kernel.org,  daniel@iogearbox.net, andrii@kernel.org,
	acme@kernel.org
Cc: martin.lau@linux.dev, song@kernel.org, yhs@fb.com,
	john.fastabend@gmail.com,  kpsingh@kernel.org, sdf@google.com,
	haoluo@google.com, jolsa@kernel.org,  quentin@isovalent.com,
	mykolal@fb.com, bpf@vger.kernel.org
Subject: Re: [RFC bpf-next 4/8] btf: support kernel parsing of BTF with metadata, use it to parse BTF with unknown kinds
Date: Wed, 07 Jun 2023 22:51:49 +0300	[thread overview]
Message-ID: <d2e61cf5ee93f0bb6f6d402208c19be1a7b55838.camel@gmail.com> (raw)
In-Reply-To: <20230531201936.1992188-5-alan.maguire@oracle.com>

On Wed, 2023-05-31 at 21:19 +0100, Alan Maguire wrote:
> Validate metadata if present, and use it to parse BTF with
> unrecognized kinds. Reject BTF that contains a type
> of a kind that is not optional.
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  kernel/bpf/btf.c | 102 +++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 85 insertions(+), 17 deletions(-)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index bd2cac057928..67f42d9ce099 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -257,6 +257,7 @@ struct btf {
>  	struct btf_kfunc_set_tab *kfunc_set_tab;
>  	struct btf_id_dtor_kfunc_tab *dtor_kfunc_tab;
>  	struct btf_struct_metas *struct_meta_tab;
> +	struct btf_metadata *meta_data;
>  
>  	/* split BTF support */
>  	struct btf *base_btf;
> @@ -4965,22 +4966,41 @@ static s32 btf_check_meta(struct btf_verifier_env *env,
>  		return -EINVAL;
>  	}
>  
> -	if (BTF_INFO_KIND(t->info) > BTF_KIND_MAX ||
> -	    BTF_INFO_KIND(t->info) == BTF_KIND_UNKN) {
> -		btf_verifier_log(env, "[%u] Invalid kind:%u",
> -				 env->log_type_id, BTF_INFO_KIND(t->info));
> -		return -EINVAL;
> -	}
> -
>  	if (!btf_name_offset_valid(env->btf, t->name_off)) {
>  		btf_verifier_log(env, "[%u] Invalid name_offset:%u",
>  				 env->log_type_id, t->name_off);
>  		return -EINVAL;
>  	}
>  
> -	var_meta_size = btf_type_ops(t)->check_meta(env, t, meta_left);
> -	if (var_meta_size < 0)
> -		return var_meta_size;
> +	if (BTF_INFO_KIND(t->info) == BTF_KIND_UNKN) {
> +		btf_verifier_log(env, "[%u] Invalid kind:%u",
> +				 env->log_type_id, BTF_INFO_KIND(t->info));
> +		return -EINVAL;
> +	}
> +
> +	if (BTF_INFO_KIND(t->info) > BTF_KIND_MAX && env->btf->meta_data &&
> +	    BTF_INFO_KIND(t->info) < env->btf->meta_data->kind_meta_cnt) {
> +		struct btf_kind_meta *m = &env->btf->meta_data->kind_meta[BTF_INFO_KIND(t->info)];
> +
> +		if (!(m->flags & BTF_KIND_META_OPTIONAL)) {
> +			btf_verifier_log(env, "[%u] unknown but required kind '%s'(%u)",
> +					 env->log_type_id,
> +					 btf_name_by_offset(env->btf, m->name_off),
> +					 BTF_INFO_KIND(t->info));
> +			return -EINVAL;
> +		}
> +		var_meta_size = sizeof(struct btf_type);
> +		var_meta_size += m->info_sz + (btf_type_vlen(t) * m->elem_sz);
> +	} else {
> +		if (BTF_INFO_KIND(t->info) > BTF_KIND_MAX) {
> +			btf_verifier_log(env, "[%u] Invalid kind:%u",
> +					 env->log_type_id, BTF_INFO_KIND(t->info));
> +			return -EINVAL;
> +		}
> +		var_meta_size = btf_type_ops(t)->check_meta(env, t, meta_left);
> +		if (var_meta_size < 0)
> +			return var_meta_size;
> +	}
>  
>  	meta_left -= var_meta_size;
>  
> @@ -5155,7 +5175,8 @@ static int btf_parse_str_sec(struct btf_verifier_env *env)
>  	start = btf->nohdr_data + hdr->str_off;
>  	end = start + hdr->str_len;
>  
> -	if (end != btf->data + btf->data_size) {
> +	if (hdr->hdr_len < sizeof(struct btf_header) &&
> +	    end != btf->data + btf->data_size) {
>  		btf_verifier_log(env, "String section is not at the end");
>  		return -EINVAL;
>  	}
> @@ -5176,9 +5197,47 @@ static int btf_parse_str_sec(struct btf_verifier_env *env)
>  	return 0;
>  }
>  
> +static int btf_parse_meta_sec(struct btf_verifier_env *env)
> +{
> +	const struct btf_header *hdr = &env->btf->hdr;
> +	struct btf *btf = env->btf;
> +	void *start, *end;
> +
> +	if (hdr->hdr_len < sizeof(struct btf_header) ||
> +	    hdr->meta_header.meta_len == 0)
> +		return 0;
> +
> +	/* Meta section must align to 8 bytes */
> +	if (hdr->meta_header.meta_off & (sizeof(u64) - 1)) {
> +		btf_verifier_log(env, "Unaligned meta_off");
> +		return -EINVAL;
> +	}
> +	start = btf->nohdr_data + hdr->meta_header.meta_off;
> +	end = start + hdr->meta_header.meta_len;
> +
> +	if (hdr->meta_header.meta_len < sizeof(struct btf_meta_header)) {
> +		btf_verifier_log(env, "Metadata section is too small");
> +		return -EINVAL;
> +	}
> +	if (end != btf->data + btf->data_size) {
> +		btf_verifier_log(env, "Metadata section is not at the end");
> +		return -EINVAL;
> +	}
> +	btf->meta_data = start;
> +
> +	if (hdr->meta_header.meta_len != sizeof(struct btf_metadata) +
> +					(btf->meta_data->kind_meta_cnt *
> +					 sizeof(struct btf_kind_meta))) {
> +		btf_verifier_log(env, "Metadata section size mismatch");
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
>  static const size_t btf_sec_info_offset[] = {
>  	offsetof(struct btf_header, type_off),
>  	offsetof(struct btf_header, str_off),
> +	offsetof(struct btf_header, meta_header.meta_off),
>  };
>  
>  static int btf_sec_info_cmp(const void *a, const void *b)
> @@ -5193,15 +5252,19 @@ static int btf_check_sec_info(struct btf_verifier_env *env,
>  			      u32 btf_data_size)
>  {
>  	struct btf_sec_info secs[ARRAY_SIZE(btf_sec_info_offset)];
> -	u32 total, expected_total, i;
> +	u32 nr_secs = ARRAY_SIZE(btf_sec_info_offset);
> +	u32 total, expected_total, gap, i;
>  	const struct btf_header *hdr;
>  	const struct btf *btf;
>  
>  	btf = env->btf;
>  	hdr = &btf->hdr;
>  
> +	if (hdr->hdr_len < sizeof(struct btf_header))
> +		nr_secs--;
> +
>  	/* Populate the secs from hdr */
> -	for (i = 0; i < ARRAY_SIZE(btf_sec_info_offset); i++)
> +	for (i = 0; i < nr_secs; i++)
>  		secs[i] = *(struct btf_sec_info *)((void *)hdr +
>  						   btf_sec_info_offset[i]);
>  
> @@ -5216,9 +5279,10 @@ static int btf_check_sec_info(struct btf_verifier_env *env,
>  			btf_verifier_log(env, "Invalid section offset");
>  			return -EINVAL;
>  		}
> -		if (total < secs[i].off) {
> -			/* gap */
> -			btf_verifier_log(env, "Unsupported section found");
> +		gap = secs[i].off - total;
> +		if (gap >= 8) {
> +			/* gap larger than alignment gap */
> +			btf_verifier_log(env, "Unsupported section gap found");

I have two `prog_tests` tests failing with this patch applied:
* btf/btf_header test. Gap between hdr and type
  Here expected string should be updated:

--- a/tools/testing/selftests/bpf/prog_tests/btf.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf.c
@@ -1579,7 +1579,7 @@ static struct btf_raw_test raw_tests[] = {
        .max_entries = 4,
        .btf_load_err = true,
        .type_off_delta = 4,
-       .err_str = "Unsupported section found",
+       .err_str = "Unsupported section gap found",
 },

* btf/btf_header test. Overlap between type and str
  This test expects error string "Section overlap found",
  but instead "Section overlap found" is printed.
  This happens with the following values of local variables:
    
    total=20, secs[2].off=16, gap=-4

  (`gap` is printed as signed using %d).

>  			return -EINVAL;
>  		}
>  		if (total > secs[i].off) {
> @@ -5230,7 +5294,7 @@ static int btf_check_sec_info(struct btf_verifier_env *env,
>  					 "Total section length too long");
>  			return -EINVAL;
>  		}
> -		total += secs[i].len;
> +		total += secs[i].len + gap;
>  	}
>  
>  	/* There is data other than hdr and known sections */
> @@ -5530,6 +5594,10 @@ static struct btf *btf_parse(const union bpf_attr *attr, bpfptr_t uattr, u32 uat
>  	if (err)
>  		goto errout;
>  
> +	err = btf_parse_meta_sec(env);
> +	if (err)
> +		goto errout;
> +
>  	err = btf_parse_type_sec(env);
>  	if (err)
>  		goto errout;


  reply	other threads:[~2023-06-07 19:51 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-31 20:19 [RFC bpf-next 0/8] bpf: support BTF kind metadata to separate Alan Maguire
2023-05-31 20:19 ` [RFC bpf-next 1/8] btf: add kind metadata encoding to UAPI Alan Maguire
2023-06-01  3:53   ` Alexei Starovoitov
2023-06-01 10:36     ` Alan Maguire
2023-06-01 16:53       ` Alexei Starovoitov
2023-06-02 16:32         ` Andrii Nakryiko
2023-06-02 16:34           ` Andrii Nakryiko
2023-06-02 18:11           ` Alexei Starovoitov
2023-06-02 20:33             ` Andrii Nakryiko
2023-06-05 16:14               ` Alexei Starovoitov
2023-06-05 22:38                 ` Andrii Nakryiko
2023-06-06  2:46                   ` Alexei Starovoitov
2023-06-06 11:30                     ` Toke Høiland-Jørgensen
2023-06-07 11:55                       ` Eduard Zingerman
2023-06-07 15:29                         ` Yonghong Song
2023-06-07 16:14                           ` Eduard Zingerman
2023-06-07 21:47                             ` Andrii Nakryiko
2023-06-07 22:05                               ` Eduard Zingerman
2023-06-07 22:34                                 ` Andrii Nakryiko
2023-06-06 16:50                     ` Andrii Nakryiko
2023-06-07  1:16                       ` Alexei Starovoitov
2023-06-07 21:43                         ` Andrii Nakryiko
2023-05-31 20:19 ` [RFC bpf-next 2/8] libbpf: support handling of metadata section in BTF Alan Maguire
2023-06-05 11:01   ` Jiri Olsa
2023-06-05 21:40     ` Andrii Nakryiko
2023-05-31 20:19 ` [RFC bpf-next 3/8] libbpf: use metadata to compute an unknown kind size Alan Maguire
2023-05-31 20:19 ` [RFC bpf-next 4/8] btf: support kernel parsing of BTF with metadata, use it to parse BTF with unknown kinds Alan Maguire
2023-06-07 19:51   ` Eduard Zingerman [this message]
2023-05-31 20:19 ` [RFC bpf-next 5/8] libbpf: add metadata encoding support Alan Maguire
2023-05-31 20:19 ` [RFC bpf-next 6/8] btf: generate metadata for vmlinux/module BTF Alan Maguire
2023-05-31 20:19 ` [RFC bpf-next 7/8] bpftool: add BTF dump "format meta" to dump header/metadata Alan Maguire
2023-06-01 16:33   ` Quentin Monnet
2023-06-02 16:57   ` Andrii Nakryiko
2023-05-31 20:19 ` [RFC bpf-next 8/8] selftests/bpf: test kind encoding/decoding Alan Maguire
2023-05-31 20:19 ` [RFC dwarves] dwarves: encode BTF metadata if --btf_gen_meta is set Alan Maguire

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d2e61cf5ee93f0bb6f6d402208c19be1a7b55838.camel@gmail.com \
    --to=eddyz87@gmail.com \
    --cc=acme@kernel.org \
    --cc=alan.maguire@oracle.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=haoluo@google.com \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=kpsingh@kernel.org \
    --cc=martin.lau@linux.dev \
    --cc=mykolal@fb.com \
    --cc=quentin@isovalent.com \
    --cc=sdf@google.com \
    --cc=song@kernel.org \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).