From: Song Liu <liu.song.a23@gmail.com>
To: Andrii Nakryiko <andriin@fb.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.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 2/8] libbpf: extract BTF loading and simplify ELF parsing logic
Date: Sat, 15 Jun 2019 13:25:54 -0700 [thread overview]
Message-ID: <CAPhsuW6kAN=gMjtXiAJazDFTszuq4xE-9OQTP_GhDX2cxym0NQ@mail.gmail.com> (raw)
In-Reply-To: <20190611044747.44839-3-andriin@fb.com>
On Mon, Jun 10, 2019 at 9:49 PM Andrii Nakryiko <andriin@fb.com> wrote:
>
> As a preparation for adding BTF-based BPF map loading, extract .BTF and
> .BTF.ext loading logic. Also simplify error handling in
> bpf_object__elf_collect() by returning early, as there is no common
> clean up to be done.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> tools/lib/bpf/libbpf.c | 137 ++++++++++++++++++++++-------------------
> 1 file changed, 75 insertions(+), 62 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index ba89d9727137..9e39a0a33aeb 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1078,6 +1078,58 @@ static void bpf_object__sanitize_btf_ext(struct bpf_object *obj)
> }
> }
>
> +static int bpf_object__load_btf(struct bpf_object *obj,
> + Elf_Data *btf_data,
> + Elf_Data *btf_ext_data)
> +{
> + int err = 0;
> +
> + if (btf_data) {
> + obj->btf = btf__new(btf_data->d_buf, btf_data->d_size);
> + if (IS_ERR(obj->btf)) {
> + pr_warning("Error loading ELF section %s: %d.\n",
> + BTF_ELF_SEC, err);
> + goto out;
If we goto out here, we will return 0.
> + }
> + err = btf__finalize_data(obj, obj->btf);
> + if (err) {
> + pr_warning("Error finalizing %s: %d.\n",
> + BTF_ELF_SEC, err);
> + goto out;
> + }
> + bpf_object__sanitize_btf(obj);
> + err = btf__load(obj->btf);
> + if (err) {
> + pr_warning("Error loading %s into kernel: %d.\n",
> + BTF_ELF_SEC, err);
> + goto out;
> + }
> + }
> + if (btf_ext_data) {
> + if (!obj->btf) {
> + pr_debug("Ignore ELF section %s because its depending ELF section %s is not found.\n",
> + BTF_EXT_ELF_SEC, BTF_ELF_SEC);
> + goto out;
We will also return 0 when goto out here.
> + }
> + obj->btf_ext = btf_ext__new(btf_ext_data->d_buf,
> + btf_ext_data->d_size);
> + if (IS_ERR(obj->btf_ext)) {
> + pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n",
> + BTF_EXT_ELF_SEC, PTR_ERR(obj->btf_ext));
> + obj->btf_ext = NULL;
> + goto out;
And, here. And we will not free obj->btf.
> + }
> + bpf_object__sanitize_btf_ext(obj);
> + }
> +out:
> + if (err || IS_ERR(obj->btf)) {
> + if (!IS_ERR_OR_NULL(obj->btf))
> + btf__free(obj->btf);
> + obj->btf = NULL;
> + }
> + return 0;
> +}
> +
> static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
> {
> Elf *elf = obj->efile.elf;
> @@ -1102,24 +1154,21 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
> if (gelf_getshdr(scn, &sh) != &sh) {
> pr_warning("failed to get section(%d) header from %s\n",
> idx, obj->path);
> - err = -LIBBPF_ERRNO__FORMAT;
> - goto out;
> + return -LIBBPF_ERRNO__FORMAT;
> }
>
> name = elf_strptr(elf, ep->e_shstrndx, sh.sh_name);
> if (!name) {
> pr_warning("failed to get section(%d) name from %s\n",
> idx, obj->path);
> - err = -LIBBPF_ERRNO__FORMAT;
> - goto out;
> + return -LIBBPF_ERRNO__FORMAT;
> }
>
> data = elf_getdata(scn, 0);
> if (!data) {
> pr_warning("failed to get section(%d) data from %s(%s)\n",
> idx, name, obj->path);
> - err = -LIBBPF_ERRNO__FORMAT;
> - goto out;
> + return -LIBBPF_ERRNO__FORMAT;
> }
> pr_debug("section(%d) %s, size %ld, link %d, flags %lx, type=%d\n",
> idx, name, (unsigned long)data->d_size,
> @@ -1130,10 +1179,14 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
> err = bpf_object__init_license(obj,
> data->d_buf,
> data->d_size);
> + if (err)
> + return err;
> } else if (strcmp(name, "version") == 0) {
> err = bpf_object__init_kversion(obj,
> data->d_buf,
> data->d_size);
> + if (err)
> + return err;
> } else if (strcmp(name, "maps") == 0) {
> obj->efile.maps_shndx = idx;
> } else if (strcmp(name, BTF_ELF_SEC) == 0) {
> @@ -1144,11 +1197,10 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
> if (obj->efile.symbols) {
> pr_warning("bpf: multiple SYMTAB in %s\n",
> obj->path);
> - err = -LIBBPF_ERRNO__FORMAT;
> - } else {
> - obj->efile.symbols = data;
> - obj->efile.strtabidx = sh.sh_link;
> + return -LIBBPF_ERRNO__FORMAT;
> }
> + obj->efile.symbols = data;
> + obj->efile.strtabidx = sh.sh_link;
> } else if (sh.sh_type == SHT_PROGBITS && data->d_size > 0) {
> if (sh.sh_flags & SHF_EXECINSTR) {
> if (strcmp(name, ".text") == 0)
> @@ -1162,6 +1214,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
>
> pr_warning("failed to alloc program %s (%s): %s",
> name, obj->path, cp);
> + return err;
> }
> } else if (strcmp(name, ".data") == 0) {
> obj->efile.data = data;
> @@ -1173,8 +1226,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
> pr_debug("skip section(%d) %s\n", idx, name);
> }
> } else if (sh.sh_type == SHT_REL) {
> + int nr_reloc = obj->efile.nr_reloc;
> void *reloc = obj->efile.reloc;
> - int nr_reloc = obj->efile.nr_reloc + 1;
> int sec = sh.sh_info; /* points to other section */
>
> /* Only do relo for section with exec instructions */
> @@ -1184,79 +1237,39 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
> continue;
> }
>
> - reloc = reallocarray(reloc, nr_reloc,
> + reloc = reallocarray(reloc, nr_reloc + 1,
> sizeof(*obj->efile.reloc));
> if (!reloc) {
> pr_warning("realloc failed\n");
> - err = -ENOMEM;
> - } else {
> - int n = nr_reloc - 1;
> + return -ENOMEM;
> + }
>
> - obj->efile.reloc = reloc;
> - obj->efile.nr_reloc = nr_reloc;
> + obj->efile.reloc = reloc;
> + obj->efile.nr_reloc++;
>
> - obj->efile.reloc[n].shdr = sh;
> - obj->efile.reloc[n].data = data;
> - }
> + obj->efile.reloc[nr_reloc].shdr = sh;
> + obj->efile.reloc[nr_reloc].data = data;
> } else if (sh.sh_type == SHT_NOBITS && strcmp(name, ".bss") == 0) {
> obj->efile.bss = data;
> obj->efile.bss_shndx = idx;
> } else {
> pr_debug("skip section(%d) %s\n", idx, name);
> }
> - if (err)
> - goto out;
> }
>
> if (!obj->efile.strtabidx || obj->efile.strtabidx >= idx) {
> pr_warning("Corrupted ELF file: index of strtab invalid\n");
> return -LIBBPF_ERRNO__FORMAT;
> }
> - if (btf_data) {
> - obj->btf = btf__new(btf_data->d_buf, btf_data->d_size);
> - if (IS_ERR(obj->btf)) {
> - pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n",
> - BTF_ELF_SEC, PTR_ERR(obj->btf));
> - obj->btf = NULL;
> - } else {
> - err = btf__finalize_data(obj, obj->btf);
> - if (!err) {
> - bpf_object__sanitize_btf(obj);
> - err = btf__load(obj->btf);
> - }
> - if (err) {
> - pr_warning("Error finalizing and loading %s into kernel: %d. Ignored and continue.\n",
> - BTF_ELF_SEC, err);
> - btf__free(obj->btf);
> - obj->btf = NULL;
> - err = 0;
> - }
> - }
> - }
> - if (btf_ext_data) {
> - if (!obj->btf) {
> - pr_debug("Ignore ELF section %s because its depending ELF section %s is not found.\n",
> - BTF_EXT_ELF_SEC, BTF_ELF_SEC);
> - } else {
> - obj->btf_ext = btf_ext__new(btf_ext_data->d_buf,
> - btf_ext_data->d_size);
> - if (IS_ERR(obj->btf_ext)) {
> - pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n",
> - BTF_EXT_ELF_SEC,
> - PTR_ERR(obj->btf_ext));
> - obj->btf_ext = NULL;
> - } else {
> - bpf_object__sanitize_btf_ext(obj);
> - }
> - }
> - }
> + err = bpf_object__load_btf(obj, btf_data, btf_ext_data);
> + if (err)
> + return err;
> if (bpf_object__has_maps(obj)) {
> err = bpf_object__init_maps(obj, flags);
> if (err)
> - goto out;
> + return err;
> }
> err = bpf_object__init_prog_names(obj);
> -out:
> return err;
> }
>
> --
> 2.17.1
>
next prev parent reply other threads:[~2019-06-15 20:26 UTC|newest]
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 [this message]
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
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 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='CAPhsuW6kAN=gMjtXiAJazDFTszuq4xE-9OQTP_GhDX2cxym0NQ@mail.gmail.com' \
--to=liu.song.a23@gmail.com \
--cc=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=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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.