diff mbox series

[v5,bpf-next,5/9] libbpf: make btf_parse_elf process .BTF.base transparently

Message ID 20240528122408.3154936-6-alan.maguire@oracle.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series bpf: support resilient split BTF | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/apply fail Patch does not apply to bpf-next-0

Commit Message

Alan Maguire May 28, 2024, 12:24 p.m. UTC 
From: Eduard Zingerman <eddyz87@gmail.com>

Update btf_parse_elf() to check if .BTF.base section is present.
The logic is as follows:

  if .BTF.base section exists:
     distilled_base := btf_new(.BTF.base)
  if distilled_base:
     btf := btf_new(.BTF, .base_btf=distilled_base)
     if base_btf:
        btf_relocate(btf, base_btf)
  else:
     btf := btf_new(.BTF)
  return btf

In other words:
- if .BTF.base section exists, load BTF from it and use it as a base
  for .BTF load;
- if base_btf is specified and .BTF.base section exist, relocate newly
  loaded .BTF against base_btf.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/lib/bpf/btf.c | 151 +++++++++++++++++++++++++++++---------------
 tools/lib/bpf/btf.h |   1 +
 2 files changed, 102 insertions(+), 50 deletions(-)

Comments

Andrii Nakryiko May 31, 2024, 6:57 p.m. UTC | #1 
On Tue, May 28, 2024 at 5:26 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> From: Eduard Zingerman <eddyz87@gmail.com>
>
> Update btf_parse_elf() to check if .BTF.base section is present.
> The logic is as follows:
>
>   if .BTF.base section exists:
>      distilled_base := btf_new(.BTF.base)
>   if distilled_base:
>      btf := btf_new(.BTF, .base_btf=distilled_base)
>      if base_btf:
>         btf_relocate(btf, base_btf)
>   else:
>      btf := btf_new(.BTF)
>   return btf
>
> In other words:
> - if .BTF.base section exists, load BTF from it and use it as a base
>   for .BTF load;
> - if base_btf is specified and .BTF.base section exist, relocate newly
>   loaded .BTF against base_btf.
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  tools/lib/bpf/btf.c | 151 +++++++++++++++++++++++++++++---------------
>  tools/lib/bpf/btf.h |   1 +
>  2 files changed, 102 insertions(+), 50 deletions(-)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index cb762d7a5dd7..b57f74eedda0 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -114,7 +114,10 @@ struct btf {
>         /* a set of unique strings */
>         struct strset *strs_set;
>         /* whether strings are already deduplicated */
> -       bool strs_deduped;
> +       unsigned strs_deduped:1;
> +
> +       /* whether base_btf should be freed in btf_free for this instance */
> +       unsigned owns_base:1;

nit: let's not do bit counting (i.e., bit fields for bool flags) on
rather big things like struct btf, which are only a few of them and
4/8 extra bytes just doesn't matter compared to all the other memory
used for actual data.

>
>         /* BTF object FD, if loaded into kernel */
>         int fd;
> @@ -969,6 +972,8 @@ void btf__free(struct btf *btf)
>         free(btf->raw_data);
>         free(btf->raw_data_swapped);
>         free(btf->type_offs);
> +       if (btf->owns_base)
> +               btf__free(btf->base_btf);
>         free(btf);
>  }
>
> @@ -1084,53 +1089,38 @@ struct btf *btf__new_split(const void *data, __u32 size, struct btf *base_btf)
>         return libbpf_ptr(btf_new(data, size, base_btf));
>  }
>
> -static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
> -                                struct btf_ext **btf_ext)
> +struct elf_sections_info {
> +       Elf_Data *btf_data;
> +       Elf_Data *btf_ext_data;
> +       Elf_Data *btf_base_data;

bikeshedding time: elf_sections_info -> btf_elf_data (or
btf_elf_secs), btf_data -> btf, btf_ext_data -> btf_ext, btf_base_data
-> btf_base ?

> +};
> +
> +static int btf_find_elf_sections(Elf *elf, const char *path, struct elf_sections_info *info)
>  {
> -       Elf_Data *btf_data = NULL, *btf_ext_data = NULL;
> -       int err = 0, fd = -1, idx = 0;
> -       struct btf *btf = NULL;
>         Elf_Scn *scn = NULL;
> -       Elf *elf = NULL;
> +       Elf_Data *data;
>         GElf_Ehdr ehdr;
>         size_t shstrndx;
> +       int idx = 0;

[...]

> +       if (!info.btf_data) {
>                 pr_warn("failed to find '%s' ELF section in %s\n", BTF_ELF_SEC, path);
>                 err = -ENODATA;
>                 goto done;
>         }
> -       btf = btf_new(btf_data->d_buf, btf_data->d_size, base_btf);
> +
> +       if (info.btf_base_data) {
> +               distilled_base_btf = btf_new(info.btf_base_data->d_buf, info.btf_base_data->d_size,
> +                                            NULL);

with the above bikeshedding suggestion, and distilled_base_btf ->
dist_base_btf, let's get it to be a less verbose single-line statement

> +               err = libbpf_get_error(distilled_base_btf);

boo to using libbpf_get_error() in new code. btf_new() is internal, so
IS_ERR()/PTR_ERR(), please

pw-bot: cr


> +               if (err) {
> +                       distilled_base_btf = NULL;
> +                       goto done;
> +               }
> +       }
> +
> +       btf = btf_new(info.btf_data->d_buf, info.btf_data->d_size,
> +                     distilled_base_btf ? distilled_base_btf : base_btf);

dist_base_btf ?: base_btf

>         err = libbpf_get_error(btf);

ditto, IS_ERR/PTR_ERR

>         if (err)
>                 goto done;
>
> +       if (distilled_base_btf && base_btf) {
> +               err = btf__relocate(btf, base_btf);
> +               if (err)
> +                       goto done;
> +               btf__free(distilled_base_btf);
> +               distilled_base_btf = NULL;
> +       }
> +
> +       if (distilled_base_btf)
> +               btf->owns_base = true;

should we reset this to false when changing base in btf__relocate()?

> +
>         switch (gelf_getclass(elf)) {
>         case ELFCLASS32:
>                 btf__set_pointer_size(btf, 4);

[...]
Eduard Zingerman June 1, 2024, 8:04 a.m. UTC | #2 
On Fri, 2024-05-31 at 11:57 -0700, Andrii Nakryiko wrote:

[...]

> > @@ -1084,53 +1089,38 @@ struct btf *btf__new_split(const void *data, __u32 size, struct btf *base_btf)
> >         return libbpf_ptr(btf_new(data, size, base_btf));
> >  }
> > 
> > -static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
> > -                                struct btf_ext **btf_ext)
> > +struct elf_sections_info {
> > +       Elf_Data *btf_data;
> > +       Elf_Data *btf_ext_data;
> > +       Elf_Data *btf_base_data;
> 
> bikeshedding time: elf_sections_info -> btf_elf_data (or
> btf_elf_secs), btf_data -> btf, btf_ext_data -> btf_ext, btf_base_data
> -> btf_base ?

As you and Alan see fit.

[...]

> > -       btf = btf_new(btf_data->d_buf, btf_data->d_size, base_btf);
> > +
> > +       if (info.btf_base_data) {
> > +               distilled_base_btf = btf_new(info.btf_base_data->d_buf, info.btf_base_data->d_size,
> > +                                            NULL);
> 
> with the above bikeshedding suggestion, and distilled_base_btf ->
> dist_base_btf, let's get it to be a less verbose single-line statement
> 
> > +               err = libbpf_get_error(distilled_base_btf);
> 
> boo to using libbpf_get_error() in new code. btf_new() is internal, so
> IS_ERR()/PTR_ERR(), please

Noted.

[...]

> > +       if (distilled_base_btf)
> > +               btf->owns_base = true;
> 
> should we reset this to false when changing base in btf__relocate()?

It should, good catch!

[...]
diff mbox series

Patch

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index cb762d7a5dd7..b57f74eedda0 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -114,7 +114,10 @@  struct btf {
 	/* a set of unique strings */
 	struct strset *strs_set;
 	/* whether strings are already deduplicated */
-	bool strs_deduped;
+	unsigned strs_deduped:1;
+
+	/* whether base_btf should be freed in btf_free for this instance */
+	unsigned owns_base:1;
 
 	/* BTF object FD, if loaded into kernel */
 	int fd;
@@ -969,6 +972,8 @@  void btf__free(struct btf *btf)
 	free(btf->raw_data);
 	free(btf->raw_data_swapped);
 	free(btf->type_offs);
+	if (btf->owns_base)
+		btf__free(btf->base_btf);
 	free(btf);
 }
 
@@ -1084,53 +1089,38 @@  struct btf *btf__new_split(const void *data, __u32 size, struct btf *base_btf)
 	return libbpf_ptr(btf_new(data, size, base_btf));
 }
 
-static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
-				 struct btf_ext **btf_ext)
+struct elf_sections_info {
+	Elf_Data *btf_data;
+	Elf_Data *btf_ext_data;
+	Elf_Data *btf_base_data;
+};
+
+static int btf_find_elf_sections(Elf *elf, const char *path, struct elf_sections_info *info)
 {
-	Elf_Data *btf_data = NULL, *btf_ext_data = NULL;
-	int err = 0, fd = -1, idx = 0;
-	struct btf *btf = NULL;
 	Elf_Scn *scn = NULL;
-	Elf *elf = NULL;
+	Elf_Data *data;
 	GElf_Ehdr ehdr;
 	size_t shstrndx;
+	int idx = 0;
 
-	if (elf_version(EV_CURRENT) == EV_NONE) {
-		pr_warn("failed to init libelf for %s\n", path);
-		return ERR_PTR(-LIBBPF_ERRNO__LIBELF);
-	}
-
-	fd = open(path, O_RDONLY | O_CLOEXEC);
-	if (fd < 0) {
-		err = -errno;
-		pr_warn("failed to open %s: %s\n", path, strerror(errno));
-		return ERR_PTR(err);
-	}
-
-	err = -LIBBPF_ERRNO__FORMAT;
-
-	elf = elf_begin(fd, ELF_C_READ, NULL);
-	if (!elf) {
-		pr_warn("failed to open %s as ELF file\n", path);
-		goto done;
-	}
 	if (!gelf_getehdr(elf, &ehdr)) {
 		pr_warn("failed to get EHDR from %s\n", path);
-		goto done;
+		goto err;
 	}
 
 	if (elf_getshdrstrndx(elf, &shstrndx)) {
 		pr_warn("failed to get section names section index for %s\n",
 			path);
-		goto done;
+		goto err;
 	}
 
 	if (!elf_rawdata(elf_getscn(elf, shstrndx), NULL)) {
 		pr_warn("failed to get e_shstrndx from %s\n", path);
-		goto done;
+		goto err;
 	}
 
 	while ((scn = elf_nextscn(elf, scn)) != NULL) {
+		Elf_Data **field;
 		GElf_Shdr sh;
 		char *name;
 
@@ -1138,43 +1128,103 @@  static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
 		if (gelf_getshdr(scn, &sh) != &sh) {
 			pr_warn("failed to get section(%d) header from %s\n",
 				idx, path);
-			goto done;
+			goto err;
 		}
 		name = elf_strptr(elf, shstrndx, sh.sh_name);
 		if (!name) {
 			pr_warn("failed to get section(%d) name from %s\n",
 				idx, path);
-			goto done;
+			goto err;
 		}
-		if (strcmp(name, BTF_ELF_SEC) == 0) {
-			btf_data = elf_getdata(scn, 0);
-			if (!btf_data) {
-				pr_warn("failed to get section(%d, %s) data from %s\n",
-					idx, name, path);
-				goto done;
-			}
-			continue;
-		} else if (btf_ext && strcmp(name, BTF_EXT_ELF_SEC) == 0) {
-			btf_ext_data = elf_getdata(scn, 0);
-			if (!btf_ext_data) {
-				pr_warn("failed to get section(%d, %s) data from %s\n",
-					idx, name, path);
-				goto done;
-			}
+
+		if (strcmp(name, BTF_ELF_SEC) == 0)
+			field = &info->btf_data;
+		else if (strcmp(name, BTF_EXT_ELF_SEC) == 0)
+			field = &info->btf_ext_data;
+		else if (strcmp(name, BTF_BASE_ELF_SEC) == 0)
+			field = &info->btf_base_data;
+		else
 			continue;
+
+		data = elf_getdata(scn, 0);
+		if (!data) {
+			pr_warn("failed to get section(%d, %s) data from %s\n",
+				idx, name, path);
+			goto err;
 		}
+		*field = data;
 	}
 
-	if (!btf_data) {
+	return 0;
+
+err:
+	return -LIBBPF_ERRNO__FORMAT;
+}
+
+static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
+				 struct btf_ext **btf_ext)
+{
+	struct elf_sections_info info = {};
+	struct btf *distilled_base_btf = NULL;
+	struct btf *btf = NULL;
+	int err = 0, fd = -1;
+	Elf *elf = NULL;
+
+	if (elf_version(EV_CURRENT) == EV_NONE) {
+		pr_warn("failed to init libelf for %s\n", path);
+		return ERR_PTR(-LIBBPF_ERRNO__LIBELF);
+	}
+
+	fd = open(path, O_RDONLY | O_CLOEXEC);
+	if (fd < 0) {
+		err = -errno;
+		pr_warn("failed to open %s: %s\n", path, strerror(errno));
+		return ERR_PTR(err);
+	}
+
+	elf = elf_begin(fd, ELF_C_READ, NULL);
+	if (!elf) {
+		pr_warn("failed to open %s as ELF file\n", path);
+		goto done;
+	}
+
+	err = btf_find_elf_sections(elf, path, &info);
+	if (err)
+		goto done;
+
+	if (!info.btf_data) {
 		pr_warn("failed to find '%s' ELF section in %s\n", BTF_ELF_SEC, path);
 		err = -ENODATA;
 		goto done;
 	}
-	btf = btf_new(btf_data->d_buf, btf_data->d_size, base_btf);
+
+	if (info.btf_base_data) {
+		distilled_base_btf = btf_new(info.btf_base_data->d_buf, info.btf_base_data->d_size,
+					     NULL);
+		err = libbpf_get_error(distilled_base_btf);
+		if (err) {
+			distilled_base_btf = NULL;
+			goto done;
+		}
+	}
+
+	btf = btf_new(info.btf_data->d_buf, info.btf_data->d_size,
+		      distilled_base_btf ? distilled_base_btf : base_btf);
 	err = libbpf_get_error(btf);
 	if (err)
 		goto done;
 
+	if (distilled_base_btf && base_btf) {
+		err = btf__relocate(btf, base_btf);
+		if (err)
+			goto done;
+		btf__free(distilled_base_btf);
+		distilled_base_btf = NULL;
+	}
+
+	if (distilled_base_btf)
+		btf->owns_base = true;
+
 	switch (gelf_getclass(elf)) {
 	case ELFCLASS32:
 		btf__set_pointer_size(btf, 4);
@@ -1187,8 +1237,8 @@  static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
 		break;
 	}
 
-	if (btf_ext && btf_ext_data) {
-		*btf_ext = btf_ext__new(btf_ext_data->d_buf, btf_ext_data->d_size);
+	if (btf_ext && info.btf_ext_data) {
+		*btf_ext = btf_ext__new(info.btf_ext_data->d_buf, info.btf_ext_data->d_size);
 		err = libbpf_get_error(*btf_ext);
 		if (err)
 			goto done;
@@ -1205,6 +1255,7 @@  static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
 
 	if (btf_ext)
 		btf_ext__free(*btf_ext);
+	btf__free(distilled_base_btf);
 	btf__free(btf);
 
 	return ERR_PTR(err);
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 8a93120b7385..b68d216837a9 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -18,6 +18,7 @@  extern "C" {
 
 #define BTF_ELF_SEC ".BTF"
 #define BTF_EXT_ELF_SEC ".BTF.ext"
+#define BTF_BASE_ELF_SEC ".BTF.base"
 #define MAPS_ELF_SEC ".maps"
 
 struct btf;