From mboxrd@z Thu Jan 1 00:00:00 1970 From: Martin KaFai Lau Subject: Re: [PATCH bpf-next 2/7] bpf: btf: Change how section is supported in btf_header Date: Mon, 21 May 2018 14:47:31 -0700 Message-ID: <20180521214729.vllrcu6w4uu3ltxx@kafai-mbp.dhcp.thefacebook.com> References: <20180519001650.4043980-1-kafai@fb.com> <20180519001650.4043980-3-kafai@fb.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Cc: , Alexei Starovoitov , Daniel Borkmann , To: Yonghong Song Return-path: Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:42760 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932194AbeEUVr7 (ORCPT ); Mon, 21 May 2018 17:47:59 -0400 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Mon, May 21, 2018 at 01:15:24PM -0700, Yonghong Song wrote: > > > On 5/18/18 5:16 PM, Martin KaFai Lau wrote: > > There are currently unused section descriptions in the btf_header. Those > > sections are here to support future BTF use cases. For example, the > > func section (func_off) is to support function signature (e.g. the BPF > > prog function signature). > > > > Instead of spelling out all potential sections up-front in the btf_header. > > This patch makes changes to btf_header such that extending it (e.g. adding > > a section) is possible later. The unused ones can be removed for now and > > they can be added back later. > > > > This patch: > > 1. adds a hdr_len to the btf_header. It will allow adding > > sections (and other info like parent_label and parent_name) > > later. The check is similar to the existing bpf_attr. > > If a user passes in a longer hdr_len, the kernel > > ensures the extra tailing bytes are 0. > > > > 2. allows the section order in the BTF object to be > > different from its sec_off order in btf_header. > > > > 3. each sec_off is followed by a sec_len. It must not have gap or > > overlapping among sections. > > > > The string section is ensured to be at the end due to the 4 bytes > > alignment requirement of the type section. > > > > The above changes will allow enough flexibility to > > add new sections (and other info) to the btf_header later. > > > > This patch also removes an unnecessary !err check > > at the end of btf_parse(). > > > > Signed-off-by: Martin KaFai Lau > > --- > > include/uapi/linux/btf.h | 8 +- > > kernel/bpf/btf.c | 207 +++++++++++++++++++++++++++++++++++------------ > > 2 files changed, 158 insertions(+), 57 deletions(-) > > > > diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h > > index bcb56ee47014..4fa479741a02 100644 > > --- a/include/uapi/linux/btf.h > > +++ b/include/uapi/linux/btf.h > > @@ -12,15 +12,11 @@ struct btf_header { > > __u16 magic; > > __u8 version; > > __u8 flags; > > - > > - __u32 parent_label; > > - __u32 parent_name; > > + __u32 hdr_len; > > /* All offsets are in bytes relative to the end of this header */ > > - __u32 label_off; /* offset of label section */ > > - __u32 object_off; /* offset of data object section*/ > > - __u32 func_off; /* offset of function section */ > > __u32 type_off; /* offset of type section */ > > + __u32 type_len; /* length of type section */ > > __u32 str_off; /* offset of string section */ > > __u32 str_len; /* length of string section */ > > }; > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > index ded10ab47b8a..536e5981ad8c 100644 > > --- a/kernel/bpf/btf.c > > +++ b/kernel/bpf/btf.c > > @@ -12,6 +12,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > @@ -184,15 +185,13 @@ static DEFINE_IDR(btf_idr); > > static DEFINE_SPINLOCK(btf_idr_lock); > > struct btf { > > - union { > > - struct btf_header *hdr; > > - void *data; > > - }; > > + void *data; > > struct btf_type **types; > > u32 *resolved_ids; > > u32 *resolved_sizes; > > const char *strings; > > void *nohdr_data; > > + struct btf_header hdr; > > u32 nr_types; > > u32 types_size; > > u32 data_size; > > @@ -227,6 +226,12 @@ enum resolve_mode { > > }; > > #define MAX_RESOLVE_DEPTH 32 > > +#define NR_SECS 2 > > Not sure whether it is necessary to define NR_SECS 2 here or not. > See below. > > > + > > +struct btf_sec_info { > > + u32 off; > > + u32 len; > > +}; > > struct btf_verifier_env { > > struct btf *btf; > > @@ -418,14 +423,14 @@ static const struct btf_kind_operations *btf_type_ops(const struct btf_type *t) > > static bool btf_name_offset_valid(const struct btf *btf, u32 offset) > > { > > return !BTF_STR_TBL_ELF_ID(offset) && > > - BTF_STR_OFFSET(offset) < btf->hdr->str_len; > > + BTF_STR_OFFSET(offset) < btf->hdr.str_len; > > } > > static const char *btf_name_by_offset(const struct btf *btf, u32 offset) > > { > > if (!BTF_STR_OFFSET(offset)) > > return "(anon)"; > > - else if (BTF_STR_OFFSET(offset) < btf->hdr->str_len) > > + else if (BTF_STR_OFFSET(offset) < btf->hdr.str_len) > > return &btf->strings[BTF_STR_OFFSET(offset)]; > > else > > return "(invalid-name-offset)"; > > @@ -536,7 +541,8 @@ static void btf_verifier_log_member(struct btf_verifier_env *env, > > __btf_verifier_log(log, "\n"); > > } > > -static void btf_verifier_log_hdr(struct btf_verifier_env *env) > > +static void btf_verifier_log_hdr(struct btf_verifier_env *env, > > + u32 btf_data_size) > > { > > struct bpf_verifier_log *log = &env->log; > > const struct btf *btf = env->btf; > > @@ -545,19 +551,16 @@ static void btf_verifier_log_hdr(struct btf_verifier_env *env) > > if (!bpf_verifier_log_needed(log)) > > return; > > - hdr = btf->hdr; > > + hdr = &btf->hdr; > > __btf_verifier_log(log, "magic: 0x%x\n", hdr->magic); > > __btf_verifier_log(log, "version: %u\n", hdr->version); > > __btf_verifier_log(log, "flags: 0x%x\n", hdr->flags); > > - __btf_verifier_log(log, "parent_label: %u\n", hdr->parent_label); > > - __btf_verifier_log(log, "parent_name: %u\n", hdr->parent_name); > > - __btf_verifier_log(log, "label_off: %u\n", hdr->label_off); > > - __btf_verifier_log(log, "object_off: %u\n", hdr->object_off); > > - __btf_verifier_log(log, "func_off: %u\n", hdr->func_off); > > + __btf_verifier_log(log, "hdr_len: %u\n", hdr->hdr_len); > > __btf_verifier_log(log, "type_off: %u\n", hdr->type_off); > > + __btf_verifier_log(log, "type_len: %u\n", hdr->type_len); > > __btf_verifier_log(log, "str_off: %u\n", hdr->str_off); > > __btf_verifier_log(log, "str_len: %u\n", hdr->str_len); > > - __btf_verifier_log(log, "btf_total_size: %u\n", btf->data_size); > > + __btf_verifier_log(log, "btf_total_size: %u\n", btf_data_size); > > } > > static int btf_add_type(struct btf_verifier_env *env, struct btf_type *t) > > @@ -1754,9 +1757,9 @@ static int btf_check_all_metas(struct btf_verifier_env *env) > > struct btf_header *hdr; > > void *cur, *end; > > - hdr = btf->hdr; > > + hdr = &btf->hdr; > > cur = btf->nohdr_data + hdr->type_off; > > - end = btf->nohdr_data + hdr->str_off; > > + end = btf->nohdr_data + hdr->type_len; > > env->log_type_id = 1; > > while (cur < end) { > > @@ -1866,8 +1869,20 @@ static int btf_check_all_types(struct btf_verifier_env *env) > > static int btf_parse_type_sec(struct btf_verifier_env *env) > > { > > + const struct btf_header *hdr = &env->btf->hdr; > > int err; > > + /* Type section must align to 4 bytes */ > > + if (hdr->type_off & (sizeof(u32) - 1)) { > > + btf_verifier_log(env, "Unaligned type_off"); > > + return -EINVAL; > > + } > > + > > + if (!hdr->type_len) { > > + btf_verifier_log(env, "No type found"); > > + return -EINVAL; > > + } > > + > > err = btf_check_all_metas(env); > > if (err) > > return err; > > @@ -1881,10 +1896,15 @@ static int btf_parse_str_sec(struct btf_verifier_env *env) > > struct btf *btf = env->btf; > > const char *start, *end; > > - hdr = btf->hdr; > > + hdr = &btf->hdr; > > start = btf->nohdr_data + hdr->str_off; > > end = start + hdr->str_len; > > + if (end != btf->data + btf->data_size) { > > + btf_verifier_log(env, "String section is not at the end"); > > + return -EINVAL; > > + } > > + > > if (!hdr->str_len || hdr->str_len - 1 > BTF_MAX_NAME_OFFSET || > > start[0] || end[-1]) { > > btf_verifier_log(env, "Invalid string section"); > > @@ -1896,20 +1916,119 @@ static int btf_parse_str_sec(struct btf_verifier_env *env) > > return 0; > > } > > -static int btf_parse_hdr(struct btf_verifier_env *env) > > +static const size_t btf_sec_info_offset[] = { > > + offsetof(struct btf_header, type_off), > > + offsetof(struct btf_header, str_off), > > +}; > > Maybe we can define NR_SECS is > ARRAY_SIZE(btf_sec_info_offset)/sizeof(size_t)? > > > + > > +static int btf_sec_info_cmp(const void *a, const void *b) > > { > > + const struct btf_sec_info *x = a; > > + const struct btf_sec_info *y = b; > > + > > + return (int)(x->off - y->off) ? : (int)(x->len - y->len); > > +} > > + > > +static int btf_check_sec_info(struct btf_verifier_env *env, > > + u32 btf_data_size) > > +{ > > + struct btf_sec_info secs[NR_SECS]; > > + u32 total, expected_total, i; > > const struct btf_header *hdr; > > - struct btf *btf = env->btf; > > - u32 meta_left; > > + const struct btf *btf; > > + > > + BUILD_BUG_ON(ARRAY_SIZE(btf_sec_info_offset) != NR_SECS); > > If we define NR_SECS depending on the size of btf_sec_info_offset, this > BUILD_BUG_ON is not needed. Agree, BUILD_BUG_ON() can be avoided. Will change. > > > + > > + btf = env->btf; > > + hdr = &btf->hdr; > > + > > + /* Populate the secs from hdr */ > > + for (i = 0; i < NR_SECS; i++) > > + secs[i] = *(struct btf_sec_info *)((void *)hdr + > > + btf_sec_info_offset[i]); > > + > > + sort(secs, NR_SECS, sizeof(struct btf_sec_info), > > + btf_sec_info_cmp, NULL); > > + > > + /* Check for gaps and overlap among sections */ > > + total = 0; > > + expected_total = btf_data_size - hdr->hdr_len; > > + for (i = 0; i < NR_SECS; i++) { > > + if (expected_total < secs[i].off) { > > + btf_verifier_log(env, "Invalid section offset"); > > + return -EINVAL; > > + } > > + if (total < secs[i].off) { > > + /* gap */ > > + btf_verifier_log(env, "Unsupported section found"); > > + return -EINVAL; > > + } > > + if (total > secs[i].off) { > > + btf_verifier_log(env, "Section overlap found"); > > + return -EINVAL; > > + } > > + if (expected_total - total < secs[i].len) { > > + btf_verifier_log(env, > > + "Total section length too long"); > > + return -EINVAL; > > + } > > + total += secs[i].len; > > + } > > + > > + /* There is data other than hdr and known sections */ > > + if (expected_total != total) { > > + btf_verifier_log(env, "Unsupported section found"); > > + return -EINVAL; > > + } > > Does this patch try to address compatibility issue? That is, the old btf > format with 2 sections should still work with future kernel with more > than 2 sections? The above comparision seems suggesting that newer kernel > will not support non-compatible number of sections? Yes, it should. When an older btf passed in, the newer sec_off and sec_len in btf->hdr will be zeros. They will still pass the for loop. > > > + > > + return 0; > > +} > > + > > +static int btf_parse_hdr(struct btf_verifier_env *env, void __user *btf_data, > > + u32 btf_data_size) > > +{ > > + const struct btf_header *hdr; > > + u32 hdr_len, hdr_copy; > > + struct btf_min_header { > > + u16 magic; > > + u8 version; > > + u8 flags; > > + u32 hdr_len; > > + } __user *min_hdr; > > This is the partial structure with the first four fields > for struct btf_header. > It is okay, but I feel a little bit duplication here. > > Looks like only sizeof(*min_hdr) and min_hdr->hdr_len is used. > Maybe we can just cast bpf_data to bpf_header and > sizeof(*min_hdr) being offsetof(bpf_data, type_off), and > min_hdr->hdr_len being bpf_data->hdr_len. > > Do this work? offsetof() was what I had. I found a few lines of offsetof() become so long that is actually hard to read. Also, I think spelling out the 'btf_min_header' name here is helpful, so I did not leave it as anon struct. Agree that there is duplication but it will never be changed once we release it to uapi. I can make some comments here instead. > > > + struct btf *btf; > > + int err; > > + > > + btf = env->btf; > > + min_hdr = btf_data; > > + > > + if (btf_data_size < sizeof(*min_hdr)) { > > + btf_verifier_log(env, "hdr_len not found"); > > + return -EINVAL; > > + } > > - if (btf->data_size < sizeof(*hdr)) { > > + if (get_user(hdr_len, &min_hdr->hdr_len)) > > + return -EFAULT; > > + > > + if (btf_data_size < hdr_len) { > > btf_verifier_log(env, "btf_header not found"); > > return -EINVAL; > > } > > - btf_verifier_log_hdr(env); > > + err = bpf_check_uarg_tail_zero(btf_data, sizeof(btf->hdr), hdr_len); > > + if (err) { > > + if (err == -E2BIG) > > + btf_verifier_log(env, "Unsupported btf_header"); > > + return err; > > + } > > + > > + hdr_copy = min_t(u32, hdr_len, sizeof(btf->hdr)); > > + if (copy_from_user(&btf->hdr, btf_data, hdr_copy)) > > + return -EFAULT; > > + > > + hdr = &btf->hdr; > > + > > + btf_verifier_log_hdr(env, btf_data_size); > > - hdr = btf->hdr; > > if (hdr->magic != BTF_MAGIC) { > > btf_verifier_log(env, "Invalid magic"); > > return -EINVAL; > > @@ -1925,26 +2044,14 @@ static int btf_parse_hdr(struct btf_verifier_env *env) > > return -ENOTSUPP; > > } > > - meta_left = btf->data_size - sizeof(*hdr); > > - if (!meta_left) { > > + if (btf_data_size == hdr->hdr_len) { > > btf_verifier_log(env, "No data"); > > return -EINVAL; > > } > > - if (meta_left < hdr->type_off || hdr->str_off <= hdr->type_off || > > - /* Type section must align to 4 bytes */ > > - hdr->type_off & (sizeof(u32) - 1)) { > > - btf_verifier_log(env, "Invalid type_off"); > > - return -EINVAL; > > - } > > - > > - if (meta_left < hdr->str_off || > > - meta_left - hdr->str_off < hdr->str_len) { > > - btf_verifier_log(env, "Invalid str_off or str_len"); > > - return -EINVAL; > > - } > > - > > - btf->nohdr_data = btf->hdr + 1; > > + err = btf_check_sec_info(env, btf_data_size); > > + if (err) > > + return err; > > return 0; > > } > > @@ -1987,6 +2094,11 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size, > > err = -ENOMEM; > > goto errout; > > } > > + env->btf = btf; > > + > > + err = btf_parse_hdr(env, btf_data, btf_data_size); > > + if (err) > > + goto errout; > > data = kvmalloc(btf_data_size, GFP_KERNEL | __GFP_NOWARN); > > if (!data) { > > @@ -1996,18 +2108,13 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size, > > btf->data = data; > > btf->data_size = btf_data_size; > > + btf->nohdr_data = btf->data + btf->hdr.hdr_len; > > if (copy_from_user(data, btf_data, btf_data_size)) { > > err = -EFAULT; > > goto errout; > > } > > - env->btf = btf; > > - > > - err = btf_parse_hdr(env); > > - if (err) > > - goto errout; > > - > > err = btf_parse_str_sec(env); > > if (err) > > goto errout; > > @@ -2016,16 +2123,14 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size, > > if (err) > > goto errout; > > - if (!err && log->level && bpf_verifier_log_full(log)) { > > + if (log->level && bpf_verifier_log_full(log)) { > > err = -ENOSPC; > > goto errout; > > } > > - if (!err) { > > - btf_verifier_env_free(env); > > - refcount_set(&btf->refcnt, 1); > > - return btf; > > - } > > + btf_verifier_env_free(env); > > + refcount_set(&btf->refcnt, 1); > > + return btf; > > errout: > > btf_verifier_env_free(env); > >