| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAEf4BzbAbkTL74w_MgkKyP2ZunacJ6PF-OzEUxPpkX7ss+x8mg@mail.gmail.com>
Date: Tue, 2 Jul 2019 12:29:18 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Y Song <ys114321@...il.com>
Cc: Andrii Nakryiko <andriin@...com>, Alexei Starovoitov <ast@...com>,
Daniel Borkmann <daniel@...earbox.net>,
bpf <bpf@...r.kernel.org>, netdev <netdev@...r.kernel.org>,
Song Liu <songliubraving@...com>
Subject: Re: [PATCH v2 bpf-next 1/4] libbpf: capture value in BTF type info
for BTF-defined map defs
On Tue, Jul 2, 2019 at 12:19 PM Y Song <ys114321@...il.com> wrote:
>
> On Fri, Jun 28, 2019 at 8:26 AM Andrii Nakryiko <andriin@...com> wrote:
> >
> > Change BTF-defined map definitions to capture compile-time integer
> > values as part of BTF type definition, to avoid split of key/value type
> > information and actual type/size/flags initialization for maps.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@...com>
> > ---
> > tools/lib/bpf/libbpf.c | 58 ++++++++++++++++++++----------------------
> > 1 file changed, 28 insertions(+), 30 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 6e6ebef11ba3..9e099ecb2c2b 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -1028,40 +1028,40 @@ static const struct btf_type *skip_mods_and_typedefs(const struct btf *btf,
> > }
> > }
> >
> > -static bool get_map_field_int(const char *map_name,
> > - const struct btf *btf,
> > +/*
> > + * Fetch integer attribute of BTF map definition. Such attributes are
> > + * represented using a pointer to an array, in which dimensionality of array
> > + * encodes specified integer value. E.g., int (*type)[BPF_MAP_TYPE_ARRAY];
> > + * encodes `type => BPF_MAP_TYPE_ARRAY` key/value pair completely using BTF
> > + * type definition, while using only sizeof(void *) space in ELF data section.
> > + */
> > +static bool get_map_field_int(const char *map_name, const struct btf *btf,
> > const struct btf_type *def,
> > - const struct btf_member *m,
> > - const void *data, __u32 *res) {
> > + const struct btf_member *m, __u32 *res) {
> > const struct btf_type *t = skip_mods_and_typedefs(btf, m->type);
> > const char *name = btf__name_by_offset(btf, m->name_off);
> > - __u32 int_info = *(const __u32 *)(const void *)(t + 1);
> > + const struct btf_array *arr_info;
> > + const struct btf_type *arr_t;
> >
> > - if (BTF_INFO_KIND(t->info) != BTF_KIND_INT) {
> > - pr_warning("map '%s': attr '%s': expected INT, got %u.\n",
> > + if (BTF_INFO_KIND(t->info) != BTF_KIND_PTR) {
> > + pr_warning("map '%s': attr '%s': expected PTR, got %u.\n",
> > map_name, name, BTF_INFO_KIND(t->info));
> > return false;
> > }
> > - if (t->size != 4 || BTF_INT_BITS(int_info) != 32 ||
> > - BTF_INT_OFFSET(int_info)) {
> > - pr_warning("map '%s': attr '%s': expected 32-bit non-bitfield integer, "
> > - "got %u-byte (%d-bit) one with bit offset %d.\n",
> > - map_name, name, t->size, BTF_INT_BITS(int_info),
> > - BTF_INT_OFFSET(int_info));
> > - return false;
> > - }
> > - if (BTF_INFO_KFLAG(def->info) && BTF_MEMBER_BITFIELD_SIZE(m->offset)) {
> > - pr_warning("map '%s': attr '%s': bitfield is not supported.\n",
> > - map_name, name);
> > +
> > + arr_t = btf__type_by_id(btf, t->type);
> > + if (!arr_t) {
> > + pr_warning("map '%s': attr '%s': type [%u] not found.\n",
> > + map_name, name, t->type);
> > return false;
> > }
> > - if (m->offset % 32) {
> > - pr_warning("map '%s': attr '%s': unaligned fields are not supported.\n",
> > - map_name, name);
> > + if (BTF_INFO_KIND(arr_t->info) != BTF_KIND_ARRAY) {
> > + pr_warning("map '%s': attr '%s': expected ARRAY, got %u.\n",
> > + map_name, name, BTF_INFO_KIND(arr_t->info));
> > return false;
> > }
> > -
> > - *res = *(const __u32 *)(data + m->offset / 8);
> > + arr_info = (const void *)(arr_t + 1);
> > + *res = arr_info->nelems;
>
> Here, we use number of array elements (__u32 type) as the value.
>
> But we have
> #define __int(name, val) int (*name)[val]
> which suggests that "val" have type "int".
>
> Do you think using __uint(name, val) may be more consistent?
> If this is something to be enforced, it may be worthwhile to check
> array element type should be __uint, just in case that in the
> future we have different array element type (e.g., int)
> for different purpose.
Currently we don't care about array element type at all. It seems
highly unlikely that we'll use array element type as some sort of
flag, as it would be **way** to much unobvious magic. I'm a bit
worried that there could be some inconsistencies in how compiler emits
or omits SIGNED flag on BTF_KIND_INT, so I'd rather minimize amount of
possible confusion here and not enforce that.
As for __uint vs __int, you are right, the value we can encode is
always unsigned, so it might be a good idea to do __uint instead, I'll
post an updated version.
>
> > return true;
> > }
> >
> > @@ -1074,7 +1074,6 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> > const struct btf_var_secinfo *vi;
> > const struct btf_var *var_extra;
> > const struct btf_member *m;
> > - const void *def_data;
> > const char *map_name;
> > struct bpf_map *map;
> > int vlen, i;
> > @@ -1131,7 +1130,6 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> > pr_debug("map '%s': at sec_idx %d, offset %zu.\n",
> > map_name, map->sec_idx, map->sec_offset);
> >
> > - def_data = data->d_buf + vi->offset;
> > vlen = BTF_INFO_VLEN(def->info);
> > m = (const void *)(def + 1);
> > for (i = 0; i < vlen; i++, m++) {
> > @@ -1144,19 +1142,19 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> > }
> > if (strcmp(name, "type") == 0) {
> > if (!get_map_field_int(map_name, obj->btf, def, m,
> > - def_data, &map->def.type))
> > + &map->def.type))
> > return -EINVAL;
> > pr_debug("map '%s': found type = %u.\n",
> > map_name, map->def.type);
> > } else if (strcmp(name, "max_entries") == 0) {
> > if (!get_map_field_int(map_name, obj->btf, def, m,
> > - def_data, &map->def.max_entries))
> > + &map->def.max_entries))
> > return -EINVAL;
> > pr_debug("map '%s': found max_entries = %u.\n",
> > map_name, map->def.max_entries);
> > } else if (strcmp(name, "map_flags") == 0) {
> > if (!get_map_field_int(map_name, obj->btf, def, m,
> > - def_data, &map->def.map_flags))
> > + &map->def.map_flags))
> > return -EINVAL;
> > pr_debug("map '%s': found map_flags = %u.\n",
> > map_name, map->def.map_flags);
> > @@ -1164,7 +1162,7 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> > __u32 sz;
> >
> > if (!get_map_field_int(map_name, obj->btf, def, m,
> > - def_data, &sz))
> > + &sz))
> > return -EINVAL;
> > pr_debug("map '%s': found key_size = %u.\n",
> > map_name, sz);
> > @@ -1207,7 +1205,7 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> > __u32 sz;
> >
> > if (!get_map_field_int(map_name, obj->btf, def, m,
> > - def_data, &sz))
> > + &sz))
> > return -EINVAL;
> > pr_debug("map '%s': found value_size = %u.\n",
> > map_name, sz);
> > --
> > 2.17.1
> >
Powered by blists - more mailing lists