[PATCH bpf-next] libbpf: Extend linker API to support in-memory ELF files

BPF List
 help / color / mirror / Atom feed

* [PATCH bpf-next] libbpf: Extend linker API to support in-memory ELF files
@ 2024-11-20 17:02 Alastair Robertson
  2024-11-21 23:19 ` Andrii Nakryiko
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Alastair Robertson @ 2024-11-20 17:02 UTC (permalink / raw)
  To: bpf, andrii; +Cc: Alastair Robertson

The new_fd, add_fd and finalize_fd functions correspond to the original
new, add_file and finalize functions, but accept an FD instead of a file
name. This gives API consumers the option of using anonymous
files/memfds to avoid writing ELFs to disk.

This new API will be useful for performing linking as part of
bpftrace's JIT compilation.

The add_buf function is a convenience wrapper that does the work of
creating a memfd for the caller.

Signed-off-by: Alastair Robertson <ajor@meta.com>
---
 tools/lib/bpf/libbpf.h   |   9 ++
 tools/lib/bpf/libbpf.map |   4 +
 tools/lib/bpf/linker.c   | 229 +++++++++++++++++++++++++++++----------
 3 files changed, 185 insertions(+), 57 deletions(-)

diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index b2ce3a72b11d..aae8f954c4fc 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -1796,10 +1796,19 @@ struct bpf_linker_file_opts {
 struct bpf_linker;
 
 LIBBPF_API struct bpf_linker *bpf_linker__new(const char *filename, struct bpf_linker_opts *opts);
+LIBBPF_API struct bpf_linker *bpf_linker__new_fd(const char *name, int fd,
+						 struct bpf_linker_opts *opts);
 LIBBPF_API int bpf_linker__add_file(struct bpf_linker *linker,
 				    const char *filename,
 				    const struct bpf_linker_file_opts *opts);
+LIBBPF_API int bpf_linker__add_fd(struct bpf_linker *linker,
+				  const char *name, int fd,
+				  const struct bpf_linker_file_opts *opts);
+LIBBPF_API int bpf_linker__add_buf(struct bpf_linker *linker, const char *name,
+				   void *buffer, int buffer_sz,
+				   const struct bpf_linker_file_opts *opts);
 LIBBPF_API int bpf_linker__finalize(struct bpf_linker *linker);
+LIBBPF_API int bpf_linker__finalize_fd(struct bpf_linker *linker);
 LIBBPF_API void bpf_linker__free(struct bpf_linker *linker);
 
 /*
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 54b6f312cfa8..e767f34c1d08 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -432,4 +432,8 @@ LIBBPF_1.5.0 {
 } LIBBPF_1.4.0;
 
 LIBBPF_1.6.0 {
+		bpf_linker__new_fd;
+		bpf_linker__add_fd;
+		bpf_linker__add_buf;
+		bpf_linker__finalize_fd;
 } LIBBPF_1.5.0;
diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
index cf71d149fe26..6571ed8b858f 100644
--- a/tools/lib/bpf/linker.c
+++ b/tools/lib/bpf/linker.c
@@ -4,6 +4,8 @@
  *
  * Copyright (c) 2021 Facebook
  */
+#define _GNU_SOURCE
+
 #include <stdbool.h>
 #include <stddef.h>
 #include <stdio.h>
@@ -16,6 +18,7 @@
 #include <elf.h>
 #include <libelf.h>
 #include <fcntl.h>
+#include <sys/mman.h>
 #include "libbpf.h"
 #include "btf.h"
 #include "libbpf_internal.h"
@@ -157,9 +160,9 @@ struct bpf_linker {
 #define pr_warn_elf(fmt, ...)									\
 	libbpf_print(LIBBPF_WARN, "libbpf: " fmt ": %s\n", ##__VA_ARGS__, elf_errmsg(-1))
 
-static int init_output_elf(struct bpf_linker *linker, const char *file);
+static int init_output_elf(struct bpf_linker *linker);
 
-static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
+static int linker_load_obj_file(struct bpf_linker *linker,
 				const struct bpf_linker_file_opts *opts,
 				struct src_obj *obj);
 static int linker_sanity_check_elf(struct src_obj *obj);
@@ -233,9 +236,56 @@ struct bpf_linker *bpf_linker__new(const char *filename, struct bpf_linker_opts
 	if (!linker)
 		return errno = ENOMEM, NULL;
 
-	linker->fd = -1;
+	linker->filename = strdup(filename);
+	if (!linker->filename)
+		return errno = ENOMEM, NULL;
+
+	linker->fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC, 0644);
+	if (linker->fd < 0) {
+		err = -errno;
+		pr_warn("failed to create '%s': %d\n", filename, err);
+		goto err_out;
+	}
+
+	err = init_output_elf(linker);
+	if (err)
+		goto err_out;
+
+	return linker;
+
+err_out:
+	bpf_linker__free(linker);
+	return errno = -err, NULL;
+}
+
+struct bpf_linker *bpf_linker__new_fd(const char *name, int fd,
+				      struct bpf_linker_opts *opts)
+{
+	struct bpf_linker *linker;
+	int err;
+
+	if (fd < 0)
+		return errno = EINVAL, NULL;
+
+	if (!OPTS_VALID(opts, bpf_linker_opts))
+		return errno = EINVAL, NULL;
+
+	if (elf_version(EV_CURRENT) == EV_NONE) {
+		pr_warn_elf("libelf initialization failed");
+		return errno = EINVAL, NULL;
+	}
+
+	linker = calloc(1, sizeof(*linker));
+	if (!linker)
+		return errno = ENOMEM, NULL;
+
+	linker->filename = strdup(name);
+	if (!linker->filename)
+		return errno = ENOMEM, NULL;
+
+	linker->fd = fd;
 
-	err = init_output_elf(linker, filename);
+	err = init_output_elf(linker);
 	if (err)
 		goto err_out;
 
@@ -294,23 +344,12 @@ static Elf64_Sym *add_new_sym(struct bpf_linker *linker, size_t *sym_idx)
 	return sym;
 }
 
-static int init_output_elf(struct bpf_linker *linker, const char *file)
+static int init_output_elf(struct bpf_linker *linker)
 {
 	int err, str_off;
 	Elf64_Sym *init_sym;
 	struct dst_sec *sec;
 
-	linker->filename = strdup(file);
-	if (!linker->filename)
-		return -ENOMEM;
-
-	linker->fd = open(file, O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC, 0644);
-	if (linker->fd < 0) {
-		err = -errno;
-		pr_warn("failed to create '%s': %s\n", file, errstr(err));
-		return err;
-	}
-
 	linker->elf = elf_begin(linker->fd, ELF_C_WRITE, NULL);
 	if (!linker->elf) {
 		pr_warn_elf("failed to create ELF object");
@@ -436,10 +475,9 @@ static int init_output_elf(struct bpf_linker *linker, const char *file)
 	return 0;
 }
 
-int bpf_linker__add_file(struct bpf_linker *linker, const char *filename,
-			 const struct bpf_linker_file_opts *opts)
+static int linker_add_common(struct bpf_linker *linker, struct src_obj *obj,
+			     const struct bpf_linker_file_opts *opts)
 {
-	struct src_obj obj = {};
 	int err = 0;
 
 	if (!OPTS_VALID(opts, bpf_linker_file_opts))
@@ -448,25 +486,91 @@ int bpf_linker__add_file(struct bpf_linker *linker, const char *filename,
 	if (!linker->elf)
 		return libbpf_err(-EINVAL);
 
-	err = err ?: linker_load_obj_file(linker, filename, opts, &obj);
-	err = err ?: linker_append_sec_data(linker, &obj);
-	err = err ?: linker_append_elf_syms(linker, &obj);
-	err = err ?: linker_append_elf_relos(linker, &obj);
-	err = err ?: linker_append_btf(linker, &obj);
-	err = err ?: linker_append_btf_ext(linker, &obj);
+	err = err ?: linker_load_obj_file(linker, opts, obj);
+	err = err ?: linker_append_sec_data(linker, obj);
+	err = err ?: linker_append_elf_syms(linker, obj);
+	err = err ?: linker_append_elf_relos(linker, obj);
+	err = err ?: linker_append_btf(linker, obj);
+	err = err ?: linker_append_btf_ext(linker, obj);
 
 	/* free up src_obj resources */
-	free(obj.btf_type_map);
-	btf__free(obj.btf);
-	btf_ext__free(obj.btf_ext);
-	free(obj.secs);
-	free(obj.sym_map);
-	if (obj.elf)
-		elf_end(obj.elf);
+	free(obj->btf_type_map);
+	btf__free(obj->btf);
+	btf_ext__free(obj->btf_ext);
+	free(obj->secs);
+	free(obj->sym_map);
+	if (obj->elf)
+		elf_end(obj->elf);
+	/* leave obj->fd for the caller to clean up if appropriate */
+
+	return libbpf_err(err);
+}
+
+int bpf_linker__add_file(struct bpf_linker *linker, const char *filename,
+			 const struct bpf_linker_file_opts *opts)
+{
+	struct src_obj obj = {};
+	int ret;
+
+	obj.filename = filename;
+	obj.fd = open(filename, O_RDONLY | O_CLOEXEC);
+	if (obj.fd < 0) {
+		pr_warn("failed to open file '%s': %s\n", filename, errstr(errno));
+		return -errno;
+	}
+
+	ret = linker_add_common(linker, &obj, opts);
+
 	if (obj.fd >= 0)
 		close(obj.fd);
 
-	return libbpf_err(err);
+	return ret;
+}
+
+int bpf_linker__add_fd(struct bpf_linker *linker, const char *name, int fd,
+		       const struct bpf_linker_file_opts *opts)
+{
+	struct src_obj obj = {};
+
+	if (fd < 0)
+		return libbpf_err(-EINVAL);
+
+	obj.filename = name;
+	obj.fd = fd;
+
+	return linker_add_common(linker, &obj, opts);
+}
+
+int bpf_linker__add_buf(struct bpf_linker *linker, const char *name,
+			void *buffer, int buffer_sz,
+			const struct bpf_linker_file_opts *opts)
+{
+	struct src_obj obj = {};
+	int written, ret;
+
+	obj.filename = name;
+	obj.fd = memfd_create(name, 0);
+	if (obj.fd < 0) {
+		pr_warn("failed to create memfd '%s': %s\n", name, errstr(errno));
+		return -errno;
+	}
+
+	written = 0;
+	while (written < buffer_sz) {
+		ret = write(obj.fd, buffer, buffer_sz);
+		if (ret < 0) {
+			pr_warn("failed to write '%s': %s\n", name, errstr(errno));
+			return -errno;
+		}
+		written += ret;
+	}
+
+	ret = linker_add_common(linker, &obj, opts);
+
+	if (obj.fd >= 0)
+		close(obj.fd);
+
+	return ret;
 }
 
 static bool is_dwarf_sec_name(const char *name)
@@ -534,7 +638,7 @@ static struct src_sec *add_src_sec(struct src_obj *obj, const char *sec_name)
 	return sec;
 }
 
-static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
+static int linker_load_obj_file(struct bpf_linker *linker,
 				const struct bpf_linker_file_opts *opts,
 				struct src_obj *obj)
 {
@@ -554,20 +658,12 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
 #error "Unknown __BYTE_ORDER__"
 #endif
 
-	pr_debug("linker: adding object file '%s'...\n", filename);
-
-	obj->filename = filename;
+	pr_debug("linker: adding object file '%s'...\n", obj->filename);
 
-	obj->fd = open(filename, O_RDONLY | O_CLOEXEC);
-	if (obj->fd < 0) {
-		err = -errno;
-		pr_warn("failed to open file '%s': %s\n", filename, errstr(err));
-		return err;
-	}
 	obj->elf = elf_begin(obj->fd, ELF_C_READ_MMAP, NULL);
 	if (!obj->elf) {
 		err = -errno;
-		pr_warn_elf("failed to parse ELF file '%s'", filename);
+		pr_warn_elf("failed to parse ELF file '%s'", obj->filename);
 		return err;
 	}
 
@@ -575,7 +671,7 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
 	ehdr = elf64_getehdr(obj->elf);
 	if (!ehdr) {
 		err = -errno;
-		pr_warn_elf("failed to get ELF header for %s", filename);
+		pr_warn_elf("failed to get ELF header for %s", obj->filename);
 		return err;
 	}
 
@@ -583,7 +679,7 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
 	obj_byteorder = ehdr->e_ident[EI_DATA];
 	if (obj_byteorder != ELFDATA2LSB && obj_byteorder != ELFDATA2MSB) {
 		err = -EOPNOTSUPP;
-		pr_warn("unknown byte order of ELF file %s\n", filename);
+		pr_warn("unknown byte order of ELF file %s\n", obj->filename);
 		return err;
 	}
 	if (link_byteorder == ELFDATANONE) {
@@ -593,7 +689,7 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
 			 obj_byteorder == ELFDATA2MSB ? "big" : "little");
 	} else if (link_byteorder != obj_byteorder) {
 		err = -EOPNOTSUPP;
-		pr_warn("byte order mismatch with ELF file %s\n", filename);
+		pr_warn("byte order mismatch with ELF file %s\n", obj->filename);
 		return err;
 	}
 
@@ -601,13 +697,13 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
 	    || ehdr->e_machine != EM_BPF
 	    || ehdr->e_ident[EI_CLASS] != ELFCLASS64) {
 		err = -EOPNOTSUPP;
-		pr_warn_elf("unsupported kind of ELF file %s", filename);
+		pr_warn_elf("unsupported kind of ELF file %s", obj->filename);
 		return err;
 	}
 
 	if (elf_getshdrstrndx(obj->elf, &obj->shstrs_sec_idx)) {
 		err = -errno;
-		pr_warn_elf("failed to get SHSTRTAB section index for %s", filename);
+		pr_warn_elf("failed to get SHSTRTAB section index for %s", obj->filename);
 		return err;
 	}
 
@@ -620,7 +716,7 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
 		if (!shdr) {
 			err = -errno;
 			pr_warn_elf("failed to get section #%zu header for %s",
-				    sec_idx, filename);
+				    sec_idx, obj->filename);
 			return err;
 		}
 
@@ -628,7 +724,7 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
 		if (!sec_name) {
 			err = -errno;
 			pr_warn_elf("failed to get section #%zu name for %s",
-				    sec_idx, filename);
+				    sec_idx, obj->filename);
 			return err;
 		}
 
@@ -636,7 +732,7 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
 		if (!data) {
 			err = -errno;
 			pr_warn_elf("failed to get section #%zu (%s) data from %s",
-				    sec_idx, sec_name, filename);
+				    sec_idx, sec_name, obj->filename);
 			return err;
 		}
 
@@ -672,7 +768,7 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
 				err = libbpf_get_error(obj->btf);
 				if (err) {
 					pr_warn("failed to parse .BTF from %s: %s\n",
-						filename, errstr(err));
+						obj->filename, errstr(err));
 					return err;
 				}
 				sec->skipped = true;
@@ -683,7 +779,7 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
 				err = libbpf_get_error(obj->btf_ext);
 				if (err) {
 					pr_warn("failed to parse .BTF.ext from '%s': %s\n",
-						filename, errstr(err));
+						obj->filename, errstr(err));
 					return err;
 				}
 				sec->skipped = true;
@@ -700,7 +796,7 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
 			break;
 		default:
 			pr_warn("unrecognized section #%zu (%s) in %s\n",
-				sec_idx, sec_name, filename);
+				sec_idx, sec_name, obj->filename);
 			err = -EINVAL;
 			return err;
 		}
@@ -2634,7 +2730,7 @@ static int linker_append_btf_ext(struct bpf_linker *linker, struct src_obj *obj)
 	return 0;
 }
 
-int bpf_linker__finalize(struct bpf_linker *linker)
+int linker_finalize_common(struct bpf_linker *linker)
 {
 	struct dst_sec *sec;
 	size_t strs_sz;
@@ -2693,9 +2789,28 @@ int bpf_linker__finalize(struct bpf_linker *linker)
 	}
 
 	elf_end(linker->elf);
+	linker->elf = NULL;
+
+	/* leave linker->fd for the caller to close if appropriate */
+
+	return 0;
+}
+
+int bpf_linker__finalize(struct bpf_linker *linker)
+{
+	linker_finalize_common(linker);
+
 	close(linker->fd);
+	linker->fd = -1;
 
-	linker->elf = NULL;
+	return 0;
+}
+
+int bpf_linker__finalize_fd(struct bpf_linker *linker)
+{
+	linker_finalize_common(linker);
+
+	/* linker->fd was opened by the caller, so do not close it here */
 	linker->fd = -1;
 
 	return 0;
-- 
2.43.5


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH bpf-next] libbpf: Extend linker API to support in-memory ELF files
  2024-11-20 17:02 [PATCH bpf-next] libbpf: Extend linker API to support in-memory ELF files Alastair Robertson
@ 2024-11-21 23:19 ` Andrii Nakryiko
  2024-11-22  1:46 ` kernel test robot
  2024-11-22  2:17 ` kernel test robot
  2 siblings, 0 replies; 4+ messages in thread
From: Andrii Nakryiko @ 2024-11-21 23:19 UTC (permalink / raw)
  To: Alastair Robertson; +Cc: bpf, andrii

On Wed, Nov 20, 2024 at 9:02 AM Alastair Robertson <ajor@meta.com> wrote:
>
> The new_fd, add_fd and finalize_fd functions correspond to the original
> new, add_file and finalize functions, but accept an FD instead of a file
> name. This gives API consumers the option of using anonymous
> files/memfds to avoid writing ELFs to disk.
>
> This new API will be useful for performing linking as part of
> bpftrace's JIT compilation.
>
> The add_buf function is a convenience wrapper that does the work of
> creating a memfd for the caller.
>
> Signed-off-by: Alastair Robertson <ajor@meta.com>
> ---
>  tools/lib/bpf/libbpf.h   |   9 ++
>  tools/lib/bpf/libbpf.map |   4 +
>  tools/lib/bpf/linker.c   | 229 +++++++++++++++++++++++++++++----------
>  3 files changed, 185 insertions(+), 57 deletions(-)
>

Hey Alastair,

I think adding these new APIs makes sense, but I'll nitpick a bit below.

But overall it would be good to extract preparatory stuff like change
to obj->filename everywhere and stuff like this that's pretty
widespread, but very distracting. I'll point it out as I go through
the patch below.


> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index b2ce3a72b11d..aae8f954c4fc 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -1796,10 +1796,19 @@ struct bpf_linker_file_opts {
>  struct bpf_linker;
>
>  LIBBPF_API struct bpf_linker *bpf_linker__new(const char *filename, struct bpf_linker_opts *opts);
> +LIBBPF_API struct bpf_linker *bpf_linker__new_fd(const char *name, int fd,
> +                                                struct bpf_linker_opts *opts);

API nitpick, given `int fd` is the main thing, let's put it as a first argument.

As for the "name" parameter, it's only going to be used for logging
and error reporting, right? I'm more inclined to add it into
bpf_linker_opts as some sort of name override field. It will keep the
API clean, we can always have some default "fd=%d" name, unless the
user provides a more meaningful (for them) name. And for
filename-based APIs we can still use that name as an override (if it's
provided).

>  LIBBPF_API int bpf_linker__add_file(struct bpf_linker *linker,
>                                     const char *filename,
>                                     const struct bpf_linker_file_opts *opts);
> +LIBBPF_API int bpf_linker__add_fd(struct bpf_linker *linker,
> +                                 const char *name, int fd,

same nit about fd being first param, and question if we need "name" at all

> +                                 const struct bpf_linker_file_opts *opts);
> +LIBBPF_API int bpf_linker__add_buf(struct bpf_linker *linker, const char *name,
> +                                  void *buffer, int buffer_sz,
> +                                  const struct bpf_linker_file_opts *opts);

ditto, buffer + buffer_sz should come right after linker (and let's
shorten to "buf" and "buf_sz"?)

>  LIBBPF_API int bpf_linker__finalize(struct bpf_linker *linker);
> +LIBBPF_API int bpf_linker__finalize_fd(struct bpf_linker *linker);

this I don't think we need, let's see below

>  LIBBPF_API void bpf_linker__free(struct bpf_linker *linker);
>
>  /*
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 54b6f312cfa8..e767f34c1d08 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -432,4 +432,8 @@ LIBBPF_1.5.0 {
>  } LIBBPF_1.4.0;
>
>  LIBBPF_1.6.0 {
> +               bpf_linker__new_fd;
> +               bpf_linker__add_fd;
> +               bpf_linker__add_buf;
> +               bpf_linker__finalize_fd;

this should be sorted

>  } LIBBPF_1.5.0;
> diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> index cf71d149fe26..6571ed8b858f 100644
> --- a/tools/lib/bpf/linker.c
> +++ b/tools/lib/bpf/linker.c
> @@ -4,6 +4,8 @@
>   *
>   * Copyright (c) 2021 Facebook
>   */
> +#define _GNU_SOURCE

please add #ifndef guard around, we have/had issues with _GNU_SOURCE
being re-defined
> +
>  #include <stdbool.h>
>  #include <stddef.h>
>  #include <stdio.h>
> @@ -16,6 +18,7 @@
>  #include <elf.h>
>  #include <libelf.h>
>  #include <fcntl.h>
> +#include <sys/mman.h>
>  #include "libbpf.h"
>  #include "btf.h"
>  #include "libbpf_internal.h"
> @@ -157,9 +160,9 @@ struct bpf_linker {
>  #define pr_warn_elf(fmt, ...)                                                                  \
>         libbpf_print(LIBBPF_WARN, "libbpf: " fmt ": %s\n", ##__VA_ARGS__, elf_errmsg(-1))
>
> -static int init_output_elf(struct bpf_linker *linker, const char *file);
> +static int init_output_elf(struct bpf_linker *linker);
>
> -static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
> +static int linker_load_obj_file(struct bpf_linker *linker,
>                                 const struct bpf_linker_file_opts *opts,
>                                 struct src_obj *obj);
>  static int linker_sanity_check_elf(struct src_obj *obj);
> @@ -233,9 +236,56 @@ struct bpf_linker *bpf_linker__new(const char *filename, struct bpf_linker_opts
>         if (!linker)
>                 return errno = ENOMEM, NULL;
>
> -       linker->fd = -1;
> +       linker->filename = strdup(filename);
> +       if (!linker->filename)
> +               return errno = ENOMEM, NULL;
> +
> +       linker->fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC, 0644);
> +       if (linker->fd < 0) {
> +               err = -errno;
> +               pr_warn("failed to create '%s': %d\n", filename, err);
> +               goto err_out;
> +       }
> +
> +       err = init_output_elf(linker);
> +       if (err)
> +               goto err_out;
> +
> +       return linker;
> +
> +err_out:
> +       bpf_linker__free(linker);
> +       return errno = -err, NULL;
> +}
> +
> +struct bpf_linker *bpf_linker__new_fd(const char *name, int fd,
> +                                     struct bpf_linker_opts *opts)
> +{
> +       struct bpf_linker *linker;
> +       int err;
> +
> +       if (fd < 0)
> +               return errno = EINVAL, NULL;
> +
> +       if (!OPTS_VALID(opts, bpf_linker_opts))
> +               return errno = EINVAL, NULL;
> +
> +       if (elf_version(EV_CURRENT) == EV_NONE) {
> +               pr_warn_elf("libelf initialization failed");
> +               return errno = EINVAL, NULL;
> +       }
> +
> +       linker = calloc(1, sizeof(*linker));
> +       if (!linker)
> +               return errno = ENOMEM, NULL;
> +
> +       linker->filename = strdup(name);
> +       if (!linker->filename)
> +               return errno = ENOMEM, NULL;
> +
> +       linker->fd = fd;
>
> -       err = init_output_elf(linker, filename);
> +       err = init_output_elf(linker);
>         if (err)
>                 goto err_out;
>
> @@ -294,23 +344,12 @@ static Elf64_Sym *add_new_sym(struct bpf_linker *linker, size_t *sym_idx)
>         return sym;
>  }
>
> -static int init_output_elf(struct bpf_linker *linker, const char *file)
> +static int init_output_elf(struct bpf_linker *linker)
>  {
>         int err, str_off;
>         Elf64_Sym *init_sym;
>         struct dst_sec *sec;
>
> -       linker->filename = strdup(file);
> -       if (!linker->filename)
> -               return -ENOMEM;
> -
> -       linker->fd = open(file, O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC, 0644);
> -       if (linker->fd < 0) {
> -               err = -errno;
> -               pr_warn("failed to create '%s': %s\n", file, errstr(err));
> -               return err;
> -       }
> -
>         linker->elf = elf_begin(linker->fd, ELF_C_WRITE, NULL);
>         if (!linker->elf) {
>                 pr_warn_elf("failed to create ELF object");
> @@ -436,10 +475,9 @@ static int init_output_elf(struct bpf_linker *linker, const char *file)
>         return 0;
>  }
>
> -int bpf_linker__add_file(struct bpf_linker *linker, const char *filename,
> -                        const struct bpf_linker_file_opts *opts)
> +static int linker_add_common(struct bpf_linker *linker, struct src_obj *obj,
> +                            const struct bpf_linker_file_opts *opts)

it seems like bpf_linker__add_file() should be implemented on top of
bpf_linker__add_fd() + clean up on error, do we need
"linker_add_common"?

>  {
> -       struct src_obj obj = {};
>         int err = 0;
>
>         if (!OPTS_VALID(opts, bpf_linker_file_opts))
> @@ -448,25 +486,91 @@ int bpf_linker__add_file(struct bpf_linker *linker, const char *filename,
>         if (!linker->elf)
>                 return libbpf_err(-EINVAL);
>
> -       err = err ?: linker_load_obj_file(linker, filename, opts, &obj);
> -       err = err ?: linker_append_sec_data(linker, &obj);
> -       err = err ?: linker_append_elf_syms(linker, &obj);
> -       err = err ?: linker_append_elf_relos(linker, &obj);
> -       err = err ?: linker_append_btf(linker, &obj);
> -       err = err ?: linker_append_btf_ext(linker, &obj);
> +       err = err ?: linker_load_obj_file(linker, opts, obj);
> +       err = err ?: linker_append_sec_data(linker, obj);
> +       err = err ?: linker_append_elf_syms(linker, obj);
> +       err = err ?: linker_append_elf_relos(linker, obj);
> +       err = err ?: linker_append_btf(linker, obj);
> +       err = err ?: linker_append_btf_ext(linker, obj);
>
>         /* free up src_obj resources */
> -       free(obj.btf_type_map);
> -       btf__free(obj.btf);
> -       btf_ext__free(obj.btf_ext);
> -       free(obj.secs);
> -       free(obj.sym_map);
> -       if (obj.elf)
> -               elf_end(obj.elf);
> +       free(obj->btf_type_map);
> +       btf__free(obj->btf);
> +       btf_ext__free(obj->btf_ext);
> +       free(obj->secs);
> +       free(obj->sym_map);
> +       if (obj->elf)
> +               elf_end(obj->elf);
> +       /* leave obj->fd for the caller to clean up if appropriate */
> +
> +       return libbpf_err(err);
> +}
> +
> +int bpf_linker__add_file(struct bpf_linker *linker, const char *filename,
> +                        const struct bpf_linker_file_opts *opts)
> +{
> +       struct src_obj obj = {};
> +       int ret;
> +
> +       obj.filename = filename;
> +       obj.fd = open(filename, O_RDONLY | O_CLOEXEC);
> +       if (obj.fd < 0) {
> +               pr_warn("failed to open file '%s': %s\n", filename, errstr(errno));
> +               return -errno;
> +       }
> +
> +       ret = linker_add_common(linker, &obj, opts);

this can be just bpf_linker__add_fd() call, no?

> +
>         if (obj.fd >= 0)
>                 close(obj.fd);
>
> -       return libbpf_err(err);
> +       return ret;
> +}
> +
> +int bpf_linker__add_fd(struct bpf_linker *linker, const char *name, int fd,
> +                      const struct bpf_linker_file_opts *opts)
> +{
> +       struct src_obj obj = {};
> +
> +       if (fd < 0)
> +               return libbpf_err(-EINVAL);
> +
> +       obj.filename = name;
> +       obj.fd = fd;
> +
> +       return linker_add_common(linker, &obj, opts);
> +}
> +
> +int bpf_linker__add_buf(struct bpf_linker *linker, const char *name,
> +                       void *buffer, int buffer_sz,
> +                       const struct bpf_linker_file_opts *opts)
> +{
> +       struct src_obj obj = {};
> +       int written, ret;
> +
> +       obj.filename = name;
> +       obj.fd = memfd_create(name, 0);
> +       if (obj.fd < 0) {
> +               pr_warn("failed to create memfd '%s': %s\n", name, errstr(errno));
> +               return -errno;
> +       }
> +
> +       written = 0;
> +       while (written < buffer_sz) {
> +               ret = write(obj.fd, buffer, buffer_sz);
> +               if (ret < 0) {
> +                       pr_warn("failed to write '%s': %s\n", name, errstr(errno));
> +                       return -errno;
> +               }
> +               written += ret;
> +       }
> +
> +       ret = linker_add_common(linker, &obj, opts);

ditto, bpf_linker__add_fd() should be fine

> +
> +       if (obj.fd >= 0)
> +               close(obj.fd);
> +
> +       return ret;
>  }
>
>  static bool is_dwarf_sec_name(const char *name)
> @@ -534,7 +638,7 @@ static struct src_sec *add_src_sec(struct src_obj *obj, const char *sec_name)
>         return sec;
>  }
>
> -static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
> +static int linker_load_obj_file(struct bpf_linker *linker,
>                                 const struct bpf_linker_file_opts *opts,
>                                 struct src_obj *obj)
>  {
> @@ -554,20 +658,12 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
>  #error "Unknown __BYTE_ORDER__"
>  #endif
>
> -       pr_debug("linker: adding object file '%s'...\n", filename);
> -
> -       obj->filename = filename;
> +       pr_debug("linker: adding object file '%s'...\n", obj->filename);
>

so it would be nice to refactor this obj->filename as an initial patch
(and whatever other "cosmetic" changes necessary to add new APIs),
that way we can have more focused patch for new APIs

> -       obj->fd = open(filename, O_RDONLY | O_CLOEXEC);
> -       if (obj->fd < 0) {
> -               err = -errno;
> -               pr_warn("failed to open file '%s': %s\n", filename, errstr(err));
> -               return err;
> -       }
>         obj->elf = elf_begin(obj->fd, ELF_C_READ_MMAP, NULL);
>         if (!obj->elf) {
>                 err = -errno;
> -               pr_warn_elf("failed to parse ELF file '%s'", filename);
> +               pr_warn_elf("failed to parse ELF file '%s'", obj->filename);
>                 return err;
>         }
>
> @@ -575,7 +671,7 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
>         ehdr = elf64_getehdr(obj->elf);
>         if (!ehdr) {
>                 err = -errno;
> -               pr_warn_elf("failed to get ELF header for %s", filename);
> +               pr_warn_elf("failed to get ELF header for %s", obj->filename);
>                 return err;
>         }
>
> @@ -583,7 +679,7 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
>         obj_byteorder = ehdr->e_ident[EI_DATA];
>         if (obj_byteorder != ELFDATA2LSB && obj_byteorder != ELFDATA2MSB) {
>                 err = -EOPNOTSUPP;
> -               pr_warn("unknown byte order of ELF file %s\n", filename);
> +               pr_warn("unknown byte order of ELF file %s\n", obj->filename);
>                 return err;
>         }
>         if (link_byteorder == ELFDATANONE) {
> @@ -593,7 +689,7 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
>                          obj_byteorder == ELFDATA2MSB ? "big" : "little");
>         } else if (link_byteorder != obj_byteorder) {
>                 err = -EOPNOTSUPP;
> -               pr_warn("byte order mismatch with ELF file %s\n", filename);
> +               pr_warn("byte order mismatch with ELF file %s\n", obj->filename);
>                 return err;
>         }
>
> @@ -601,13 +697,13 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
>             || ehdr->e_machine != EM_BPF
>             || ehdr->e_ident[EI_CLASS] != ELFCLASS64) {
>                 err = -EOPNOTSUPP;
> -               pr_warn_elf("unsupported kind of ELF file %s", filename);
> +               pr_warn_elf("unsupported kind of ELF file %s", obj->filename);
>                 return err;
>         }
>
>         if (elf_getshdrstrndx(obj->elf, &obj->shstrs_sec_idx)) {
>                 err = -errno;
> -               pr_warn_elf("failed to get SHSTRTAB section index for %s", filename);
> +               pr_warn_elf("failed to get SHSTRTAB section index for %s", obj->filename);
>                 return err;
>         }
>
> @@ -620,7 +716,7 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
>                 if (!shdr) {
>                         err = -errno;
>                         pr_warn_elf("failed to get section #%zu header for %s",
> -                                   sec_idx, filename);
> +                                   sec_idx, obj->filename);
>                         return err;
>                 }
>
> @@ -628,7 +724,7 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
>                 if (!sec_name) {
>                         err = -errno;
>                         pr_warn_elf("failed to get section #%zu name for %s",
> -                                   sec_idx, filename);
> +                                   sec_idx, obj->filename);
>                         return err;
>                 }
>
> @@ -636,7 +732,7 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
>                 if (!data) {
>                         err = -errno;
>                         pr_warn_elf("failed to get section #%zu (%s) data from %s",
> -                                   sec_idx, sec_name, filename);
> +                                   sec_idx, sec_name, obj->filename);
>                         return err;
>                 }
>
> @@ -672,7 +768,7 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
>                                 err = libbpf_get_error(obj->btf);
>                                 if (err) {
>                                         pr_warn("failed to parse .BTF from %s: %s\n",
> -                                               filename, errstr(err));
> +                                               obj->filename, errstr(err));
>                                         return err;
>                                 }
>                                 sec->skipped = true;
> @@ -683,7 +779,7 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
>                                 err = libbpf_get_error(obj->btf_ext);
>                                 if (err) {
>                                         pr_warn("failed to parse .BTF.ext from '%s': %s\n",
> -                                               filename, errstr(err));
> +                                               obj->filename, errstr(err));
>                                         return err;
>                                 }
>                                 sec->skipped = true;
> @@ -700,7 +796,7 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
>                         break;
>                 default:
>                         pr_warn("unrecognized section #%zu (%s) in %s\n",
> -                               sec_idx, sec_name, filename);
> +                               sec_idx, sec_name, obj->filename);
>                         err = -EINVAL;
>                         return err;
>                 }

as I mentioned all of the above logging changes don't have to be in this patch

> @@ -2634,7 +2730,7 @@ static int linker_append_btf_ext(struct bpf_linker *linker, struct src_obj *obj)
>         return 0;
>  }
>
> -int bpf_linker__finalize(struct bpf_linker *linker)
> +int linker_finalize_common(struct bpf_linker *linker)
>  {
>         struct dst_sec *sec;
>         size_t strs_sz;
> @@ -2693,9 +2789,28 @@ int bpf_linker__finalize(struct bpf_linker *linker)
>         }
>
>         elf_end(linker->elf);
> +       linker->elf = NULL;
> +
> +       /* leave linker->fd for the caller to close if appropriate */
> +
> +       return 0;
> +}
> +
> +int bpf_linker__finalize(struct bpf_linker *linker)
> +{
> +       linker_finalize_common(linker);
> +
>         close(linker->fd);
> +       linker->fd = -1;
>
> -       linker->elf = NULL;
> +       return 0;
> +}
> +
> +int bpf_linker__finalize_fd(struct bpf_linker *linker)
> +{


so as I mentioned, I don't think we need an extra finalize variant. We
can just remember in linker struct whether we own fd or it was just
passed to use, and then existing bpf_linker__finalize() will just
check that and skip close, if necessary

This will also make it harder to mismatch new and finalize APIs.

pw-bot: cr

> +       linker_finalize_common(linker);
> +
> +       /* linker->fd was opened by the caller, so do not close it here */
>         linker->fd = -1;
>
>         return 0;
> --
> 2.43.5
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH bpf-next] libbpf: Extend linker API to support in-memory ELF files
  2024-11-20 17:02 [PATCH bpf-next] libbpf: Extend linker API to support in-memory ELF files Alastair Robertson
  2024-11-21 23:19 ` Andrii Nakryiko
@ 2024-11-22  1:46 ` kernel test robot
  2024-11-22  2:17 ` kernel test robot
  2 siblings, 0 replies; 4+ messages in thread
From: kernel test robot @ 2024-11-22  1:46 UTC (permalink / raw)
  To: Alastair Robertson, bpf, andrii; +Cc: oe-kbuild-all, Alastair Robertson

Hi Alastair,

kernel test robot noticed the following build errors:

[auto build test ERROR on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Alastair-Robertson/libbpf-Extend-linker-API-to-support-in-memory-ELF-files/20241121-152231
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20241120170206.2592931-1-ajor%40meta.com
patch subject: [PATCH bpf-next] libbpf: Extend linker API to support in-memory ELF files
config: sparc-randconfig-002-20241122 (https://download.01.org/0day-ci/archive/20241122/202411220938.1TZjtSM3-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241122/202411220938.1TZjtSM3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411220938.1TZjtSM3-lkp@intel.com/

All errors (new ones prefixed by >>):

>> linker.c:2733:5: error: no previous prototype for 'linker_finalize_common' [-Werror=missing-prototypes]
    2733 | int linker_finalize_common(struct bpf_linker *linker)
         |     ^~~~~~~~~~~~~~~~~~~~~~
   cc1: all warnings being treated as errors
   make[6]: *** [tools/build/Makefile.build:105: tools/bpf/resolve_btfids/libbpf/staticobjs/linker.o] Error 1
   make[6]: *** Waiting for unfinished jobs....
   make[5]: *** [Makefile:165: tools/bpf/resolve_btfids/libbpf/staticobjs/libbpf-in.o] Error 2
   make[4]: *** [Makefile:63: tools/bpf/resolve_btfids//libbpf/libbpf.a] Error 2
   make[3]: *** [Makefile:76: bpf/resolve_btfids] Error 2 shuffle=1440844219
   make[2]: *** [Makefile:1370: tools/bpf/resolve_btfids] Error 2 shuffle=1440844219
   <stdin>:1519:2: warning: #warning syscall clone3 not implemented [-Wcpp]
   make[2]: Target 'prepare' not remade because of errors.
   make[1]: *** [Makefile:224: __sub-make] Error 2 shuffle=1440844219
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [Makefile:224: __sub-make] Error 2 shuffle=1440844219
   make: Target 'prepare' not remade because of errors.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH bpf-next] libbpf: Extend linker API to support in-memory ELF files
  2024-11-20 17:02 [PATCH bpf-next] libbpf: Extend linker API to support in-memory ELF files Alastair Robertson
  2024-11-21 23:19 ` Andrii Nakryiko
  2024-11-22  1:46 ` kernel test robot
@ 2024-11-22  2:17 ` kernel test robot
  2 siblings, 0 replies; 4+ messages in thread
From: kernel test robot @ 2024-11-22  2:17 UTC (permalink / raw)
  To: Alastair Robertson, bpf, andrii; +Cc: llvm, oe-kbuild-all, Alastair Robertson

Hi Alastair,

kernel test robot noticed the following build errors:

[auto build test ERROR on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Alastair-Robertson/libbpf-Extend-linker-API-to-support-in-memory-ELF-files/20241121-152231
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20241120170206.2592931-1-ajor%40meta.com
patch subject: [PATCH bpf-next] libbpf: Extend linker API to support in-memory ELF files
config: i386-buildonly-randconfig-001-20241122 (https://download.01.org/0day-ci/archive/20241122/202411221039.4cgeHanS-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241122/202411221039.4cgeHanS-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411221039.4cgeHanS-lkp@intel.com/

All errors (new ones prefixed by >>):

>> linker.c:2733:5: error: no previous prototype for function 'linker_finalize_common' [-Werror,-Wmissing-prototypes]
    2733 | int linker_finalize_common(struct bpf_linker *linker)
         |     ^
   linker.c:2733:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
    2733 | int linker_finalize_common(struct bpf_linker *linker)
         | ^
         | static 
   1 error generated.
   make[6]: *** [tools/build/Makefile.build:105: tools/bpf/resolve_btfids/libbpf/staticobjs/linker.o] Error 1
   make[6]: *** Waiting for unfinished jobs....
   make[5]: *** [Makefile:165: tools/bpf/resolve_btfids/libbpf/staticobjs/libbpf-in.o] Error 2
   make[4]: *** [Makefile:63: tools/bpf/resolve_btfids//libbpf/libbpf.a] Error 2
   make[3]: *** [Makefile:76: bpf/resolve_btfids] Error 2 shuffle=3701184676
   make[2]: *** [Makefile:1370: tools/bpf/resolve_btfids] Error 2 shuffle=3701184676
   In file included from arch/x86/kernel/asm-offsets.c:14:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:21:
   In file included from include/linux/mm.h:2213:
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   1 warning generated.
   make[2]: Target 'prepare' not remade because of errors.
   make[1]: *** [Makefile:224: __sub-make] Error 2 shuffle=3701184676
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [Makefile:224: __sub-make] Error 2 shuffle=3701184676
   make: Target 'prepare' not remade because of errors.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2024-11-22  2:18 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-20 17:02 [PATCH bpf-next] libbpf: Extend linker API to support in-memory ELF files Alastair Robertson
2024-11-21 23:19 ` Andrii Nakryiko
2024-11-22  1:46 ` kernel test robot
2024-11-22  2:17 ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox