BPF Archive on lore.kernel.org
 help / color / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Song Liu <liu.song.a23@gmail.com>
Cc: Andrii Nakryiko <andriin@fb.com>, bpf <bpf@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH bpf-next 6/8] libbpf: allow specifying map definitions using BTF
Date: Mon, 17 Jun 2019 11:33:33 -0700
Message-ID: <CAEf4BzZu6fBsKAsZS5Nmv3HpLsXEoSa-RZE1xvr3LBCWGFJi0g@mail.gmail.com> (raw)
In-Reply-To: <CAPhsuW4-tmiBAji-=zx5gRRweioXTQM__EqaJGTPQ63SLphoUA@mail.gmail.com>

On Sat, Jun 15, 2019 at 3:01 PM Song Liu <liu.song.a23@gmail.com> wrote:
>
> On Mon, Jun 10, 2019 at 9:49 PM Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > This patch adds support for a new way to define BPF maps. It relies on
> > BTF to describe mandatory and optional attributes of a map, as well as
> > captures type information of key and value naturally. This eliminates
> > the need for BPF_ANNOTATE_KV_PAIR hack and ensures key/value sizes are
> > always in sync with the key/value type.
> >

<snip>

> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> >  tools/lib/bpf/btf.h    |   1 +
> >  tools/lib/bpf/libbpf.c | 338 +++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 330 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> > index ba4ffa831aa4..88a52ae56fc6 100644
> > --- a/tools/lib/bpf/btf.h
> > +++ b/tools/lib/bpf/btf.h
> > @@ -17,6 +17,7 @@ extern "C" {
> >
> >  #define BTF_ELF_SEC ".BTF"
> >  #define BTF_EXT_ELF_SEC ".BTF.ext"
> > +#define MAPS_ELF_SEC ".maps"
>
> How about .BTF.maps? Or maybe this doesn't realy matter?

That's not a good name, because it implies those maps are additional
part of BTF stuff (similar to as .BTF.ext is addition to .BTF). And
it's not, it uses BTF, but is not part of it.

I think .maps is good, because it's short and clear. Existing "maps"
used for bpf_map_def will eventually die out, leaving
"officially-looking" ;) .maps.

>
> >
> >  struct btf;
> >  struct btf_ext;
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 79a8143240d7..60713bcc2279 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -262,6 +262,7 @@ struct bpf_object {
> >                 } *reloc;
> >                 int nr_reloc;
> >                 int maps_shndx;
> > +               int btf_maps_shndx;
> >                 int text_shndx;
> >                 int data_shndx;
> >                 int rodata_shndx;
> > @@ -514,6 +515,7 @@ static struct bpf_object *bpf_object__new(const char *path,
> >         obj->efile.obj_buf = obj_buf;
> >         obj->efile.obj_buf_sz = obj_buf_sz;
> >         obj->efile.maps_shndx = -1;
> > +       obj->efile.btf_maps_shndx = -1;
> >         obj->efile.data_shndx = -1;
> >         obj->efile.rodata_shndx = -1;
> >         obj->efile.bss_shndx = -1;
> > @@ -1012,6 +1014,297 @@ static int bpf_object__init_user_maps(struct bpf_object *obj, bool strict)
> >         return 0;
> >  }
> >
> > +static const struct btf_type *skip_mods_and_typedefs(const struct btf *btf,
> > +                                                    __u32 id)
> > +{
> > +       const struct btf_type *t = btf__type_by_id(btf, id);
> > +
> > +       while (true) {
> > +               switch (BTF_INFO_KIND(t->info)) {
> > +               case BTF_KIND_VOLATILE:
> > +               case BTF_KIND_CONST:
> > +               case BTF_KIND_RESTRICT:
> > +               case BTF_KIND_TYPEDEF:
> > +                       t = btf__type_by_id(btf, t->type);
> > +                       break;
> > +               default:
> > +                       return t;
> > +               }
> > +       }
> > +}
> > +
> > +static bool get_map_attr_int(const char *map_name,
> > +                            const struct btf *btf,
> > +                            const struct btf_type *def,
> > +                            const struct btf_member *m,
> > +                            const void *data, __u32 *res) {
>
> Can we avoid output pointer by return 0 for invalid types?

Zero is valid value, so can't do that.

>
> > +       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);
> > +
> > +       if (BTF_INFO_KIND(t->info) != BTF_KIND_INT) {
> > +               pr_warning("map '%s': attr '%s': expected INT, 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);
> > +               return false;
> > +       }
> > +       if (m->offset % 32) {
>
> Shall we just do "m->offset > 0" here?

No. m->offset == 0 is OK (first field of a struct), but any
non-aligned int is bad (e.g., bitfields are not supported). I could
have supported byte-aligned integer fields, but don't see much reason
to.

>
> > +               pr_warning("map '%s': attr '%s': unaligned fields are not supported.\n",
> > +                          map_name, name);
> > +               return false;
> > +       }
> > +
> > +       *res = *(const __u32 *)(data + m->offset / 8);
> > +       return true;
> > +}
> > +
> > +static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> > +                                        const struct btf_type *sec,
> > +                                        int var_idx, int sec_idx,
> > +                                        const Elf_Data *data, bool strict)
> > +{
> > +       const struct btf_type *var, *def, *t;
> > +       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;
> > +
> > +       vi = (const struct btf_var_secinfo *)(const void *)(sec + 1) + var_idx;
> > +       var = btf__type_by_id(obj->btf, vi->type);
> > +       var_extra = (const void *)(var + 1);
> > +       map_name = btf__name_by_offset(obj->btf, var->name_off);
> > +       vlen = BTF_INFO_VLEN(var->info);
> > +
> > +       if (map_name == NULL || map_name[0] == '\0') {
> > +               pr_warning("map #%d: empty name.\n", var_idx);
> > +               return -EINVAL;
> > +       }
> > +       if ((__u64)vi->offset + vi->size > data->d_size) {
> > +               pr_warning("map '%s' BTF data is corrupted.\n", map_name);
> > +               return -EINVAL;
> > +       }
> > +       if (BTF_INFO_KIND(var->info) != BTF_KIND_VAR) {
> > +               pr_warning("map '%s': unexpected var kind %u.\n",
> > +                          map_name, BTF_INFO_KIND(var->info));
> > +               return -EINVAL;
> > +       }
> > +       if (var_extra->linkage != BTF_VAR_GLOBAL_ALLOCATED &&
> > +           var_extra->linkage != BTF_VAR_STATIC) {
> > +               pr_warning("map '%s': unsupported var linkage %u.\n",
> > +                          map_name, var_extra->linkage);
> > +               return -EOPNOTSUPP;
> > +       }
> > +
> > +       def = skip_mods_and_typedefs(obj->btf, var->type);
> > +       if (BTF_INFO_KIND(def->info) != BTF_KIND_STRUCT) {
> > +               pr_warning("map '%s': unexpected def kind %u.\n",
> > +                          map_name, BTF_INFO_KIND(var->info));
> > +               return -EINVAL;
> > +       }
> > +       if (def->size > vi->size) {
> > +               pr_warning("map '%s': invalid def size.\n", map_name);
> > +               return -EINVAL;
> > +       }
> > +
> > +       map = bpf_object__add_map(obj);
> > +       if (IS_ERR(map))
> > +               return PTR_ERR(map);
> > +       map->name = strdup(map_name);
> > +       if (!map->name) {
> > +               pr_warning("map '%s': failed to alloc map name.\n", map_name);
> > +               return -ENOMEM;
> > +       }
> > +       map->libbpf_type = LIBBPF_MAP_UNSPEC;
> > +       map->def.type = BPF_MAP_TYPE_UNSPEC;
> > +       map->sec_idx = sec_idx;
> > +       map->sec_offset = vi->offset;
> > +       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++) {
> > +               const char *name = btf__name_by_offset(obj->btf, m->name_off);
> > +
>
> I guess we need to check name == NULL here?

Yep, will check, it can throw off pr_warning below.

>
> > +               if (strcmp(name, "type") == 0) {
> > +                       if (!get_map_attr_int(map_name, obj->btf, def, m,
> > +                                             def_data, &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_attr_int(map_name, obj->btf, def, m,
> > +                                             def_data, &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_attr_int(map_name, obj->btf, def, m,
> > +                                             def_data, &map->def.map_flags))
> > +                               return -EINVAL;
> > +                       pr_debug("map '%s': found map_flags = %u.\n",
> > +                                map_name, map->def.map_flags);
> > +               } else if (strcmp(name, "key_size") == 0) {
> > +                       __u32 sz;
> > +
> > +                       if (!get_map_attr_int(map_name, obj->btf, def, m,
> > +                                             def_data, &sz))
> > +                               return -EINVAL;
> > +                       pr_debug("map '%s': found key_size = %u.\n",
> > +                                map_name, sz);
> > +                       if (map->def.key_size && map->def.key_size != sz) {
> > +                               pr_warning("map '%s': conflictling key size %u != %u.\n",
> > +                                          map_name, map->def.key_size, sz);
> > +                               return -EINVAL;
> > +                       }
> > +                       map->def.key_size = sz;
> > +               } else if (strcmp(name, "key") == 0) {
> > +                       __s64 sz;
> > +
> > +                       t = btf__type_by_id(obj->btf, m->type);
> > +                       if (BTF_INFO_KIND(t->info) != BTF_KIND_PTR) {
>
> check t != NULL?

done

>
> > +                               pr_warning("map '%s': key spec is not PTR: %u.\n",
> > +                                          map_name, BTF_INFO_KIND(t->info));
> > +                               return -EINVAL;
> > +                       }
> > +                       sz = btf__resolve_size(obj->btf, t->type);
> > +                       if (sz < 0) {
> > +                               pr_warning("map '%s': can't determine key size for type [%u]: %lld.\n",
> > +                                          map_name, t->type, sz);
> > +                               return sz;
> > +                       }
> > +                       pr_debug("map '%s': found key [%u], sz = %lld.\n",
> > +                                map_name, t->type, sz);
> > +                       if (map->def.key_size && map->def.key_size != sz) {
> > +                               pr_warning("map '%s': conflictling key size %u != %lld.\n",
> > +                                          map_name, map->def.key_size, sz);
> > +                               return -EINVAL;
> > +                       }
> > +                       map->def.key_size = sz;
> > +                       map->btf_key_type_id = t->type;
> > +               } else if (strcmp(name, "value_size") == 0) {
> > +                       __u32 sz;
> > +
> > +                       if (!get_map_attr_int(map_name, obj->btf, def, m,
> > +                                             def_data, &sz))
> > +                               return -EINVAL;
> > +                       pr_debug("map '%s': found value_size = %u.\n",
> > +                                map_name, sz);
> > +                       if (map->def.value_size && map->def.value_size != sz) {
> > +                               pr_warning("map '%s': conflictling value size %u != %u.\n",
> > +                                          map_name, map->def.value_size, sz);
> > +                               return -EINVAL;
> > +                       }
> > +                       map->def.value_size = sz;
> > +               } else if (strcmp(name, "value") == 0) {
> > +                       __s64 sz;
> > +
> > +                       t = btf__type_by_id(obj->btf, m->type);
>
> ditto.

yep

>
> > +                       if (BTF_INFO_KIND(t->info) != BTF_KIND_PTR) {
> > +                               pr_warning("map '%s': value spec is not PTR: %u.\n",
> > +                                          map_name, BTF_INFO_KIND(t->info));
> > +                               return -EINVAL;
> > +                       }

<snip>

  reply index

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-11  4:47 [PATCH bpf-next 0/8] BTF-defined BPF map definitions Andrii Nakryiko
2019-06-11  4:47 ` [PATCH bpf-next 1/8] libbpf: add common min/max macro to libbpf_internal.h Andrii Nakryiko
2019-06-15 20:26   ` Song Liu
2019-06-11  4:47 ` [PATCH bpf-next 2/8] libbpf: extract BTF loading and simplify ELF parsing logic Andrii Nakryiko
2019-06-15 20:25   ` Song Liu
2019-06-15 20:28     ` Song Liu
2019-06-17 17:46       ` Andrii Nakryiko
2019-06-17 17:24     ` Andrii Nakryiko
2019-06-17 18:07       ` Song Liu
2019-06-17 18:35         ` Andrii Nakryiko
2019-06-11  4:47 ` [PATCH bpf-next 3/8] libbpf: refactor map initialization Andrii Nakryiko
2019-06-11  4:47 ` [PATCH bpf-next 4/8] libbpf: identify maps by section index in addition to offset Andrii Nakryiko
2019-06-11  4:47 ` [PATCH bpf-next 5/8] libbpf: split initialization and loading of BTF Andrii Nakryiko
2019-06-15 21:21   ` Song Liu
2019-06-11  4:47 ` [PATCH bpf-next 6/8] libbpf: allow specifying map definitions using BTF Andrii Nakryiko
2019-06-15 22:00   ` Song Liu
2019-06-17 18:33     ` Andrii Nakryiko [this message]
2019-06-11  4:47 ` [PATCH bpf-next 7/8] selftests/bpf: add test for BTF-defined maps Andrii Nakryiko
2019-06-15 22:05   ` Song Liu
2019-06-11  4:47 ` [PATCH bpf-next 8/8] selftests/bpf: switch tests to BTF-defined map definitions Andrii Nakryiko
2019-06-14 23:23   ` Stanislav Fomichev
2019-06-14 23:43     ` Andrii Nakryiko
2019-06-15  0:01       ` Stanislav Fomichev
2019-06-17 19:30         ` Andrii Nakryiko
2019-06-17 21:07           ` Stanislav Fomichev
2019-06-11  5:21 ` [PATCH bpf-next 0/8] BTF-defined BPF " Andrii Nakryiko

Reply instructions:

You may reply publically 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=CAEf4BzZu6fBsKAsZS5Nmv3HpLsXEoSa-RZE1xvr3LBCWGFJi0g@mail.gmail.com \
    --to=andrii.nakryiko@gmail.com \
    --cc=andriin@fb.com \
    --cc=ast@fb.com \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=kernel-team@fb.com \
    --cc=liu.song.a23@gmail.com \
    --cc=netdev@vger.kernel.org \
    /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

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org bpf@archiver.kernel.org
	public-inbox-index bpf


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/ public-inbox