BPF Archive on lore.kernel.org
 help / color / Atom feed
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
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
>

  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 [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 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='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

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