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

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

[Alastair Robertson]

This gives API consumers the option of using anonymous files/memfds to
avoid writing temporary ELFs to disk, which will be useful for performing
linking as part of bpftrace's JIT compilation.

v2:
- Split into two commits
- Replace non-required "name" parameters with new optional opt->filename
  field
- Implement bpf_linker__add_file/bpf_linker__add_buf on top of
  bpf_linker__add_fd
- Remove bpf_linker__finalize_fd and instead have libbpf keep track of
  whether the linker's fd is owned by the linker

Alastair Robertson (2):
  libbpf: Pull file-opening logic up to top-level functions
  libbpf: Extend linker API to support in-memory ELF files

 tools/lib/bpf/libbpf.h   |  12 ++-
 tools/lib/bpf/libbpf.map |   3 +
 tools/lib/bpf/linker.c   | 207 ++++++++++++++++++++++++++++++---------
 3 files changed, 176 insertions(+), 46 deletions(-)

-- 
2.43.5

[PATCH bpf-next v2 1/2] libbpf: Pull file-opening logic up to top-level functions

[Alastair Robertson]

Move the filename arguments and file-descriptor handling from
init_output_elf() and linker_load_obj_file() and instead handle them
at the top-level in bpf_linker__new() and bpf_linker__add_file().

This will allow the inner functions to be shared with a new,
non-filename-based, API in the next commit.

Signed-off-by: Alastair Robertson <ajor@meta.com>
---
 tools/lib/bpf/linker.c | 78 ++++++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 40 deletions(-)

diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
index cf71d149fe26..375896a94e6a 100644
--- a/tools/lib/bpf/linker.c
+++ b/tools/lib/bpf/linker.c
@@ -157,9 +157,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 +233,18 @@ 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, filename);
+	err = init_output_elf(linker);
 	if (err)
 		goto err_out;
 
@@ -294,23 +303,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");
@@ -440,7 +438,7 @@ int bpf_linker__add_file(struct bpf_linker *linker, const char *filename,
 			 const struct bpf_linker_file_opts *opts)
 {
 	struct src_obj obj = {};
-	int err = 0;
+	int err = 0, fd;
 
 	if (!OPTS_VALID(opts, bpf_linker_file_opts))
 		return libbpf_err(-EINVAL);
@@ -448,7 +446,15 @@ 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);
+	fd = open(filename, O_RDONLY | O_CLOEXEC);
+	if (fd < 0) {
+		pr_warn("failed to open file '%s': %s\n", filename, errstr(errno));
+		return -errno;
+	}
+
+	obj.fd = fd;
+
+	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);
@@ -534,7 +540,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 +560,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 +573,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 +581,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 +591,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 +599,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 +618,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 +626,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 +634,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 +670,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 +681,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 +698,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;
 		}
-- 
2.43.5

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

[Alastair Robertson]

The new_fd and add_fd functions correspond to the original new and
add_file 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   |  12 +++-
 tools/lib/bpf/libbpf.map |   3 +
 tools/lib/bpf/linker.c   | 143 ++++++++++++++++++++++++++++++++++++---
 3 files changed, 145 insertions(+), 13 deletions(-)

diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index b2ce3a72b11d..7a88830a3431 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -1784,21 +1784,29 @@ enum libbpf_tristate {
 struct bpf_linker_opts {
 	/* size of this struct, for forward/backward compatibility */
 	size_t sz;
+	const char *filename;
 };
-#define bpf_linker_opts__last_field sz
+#define bpf_linker_opts__last_field filename
 
 struct bpf_linker_file_opts {
 	/* size of this struct, for forward/backward compatibility */
 	size_t sz;
+	const char *filename;
 };
-#define bpf_linker_file_opts__last_field sz
+#define bpf_linker_file_opts__last_field filename
 
 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(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, int fd,
+				  const struct bpf_linker_file_opts *opts);
+LIBBPF_API int bpf_linker__add_buf(struct bpf_linker *linker, const char *name,
+				   void *buf, int buf_sz,
+				   const struct bpf_linker_file_opts *opts);
 LIBBPF_API int bpf_linker__finalize(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..23f2a30778f0 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -432,4 +432,7 @@ LIBBPF_1.5.0 {
 } LIBBPF_1.4.0;
 
 LIBBPF_1.6.0 {
+		bpf_linker__add_buf;
+		bpf_linker__add_fd;
+		bpf_linker__new_fd;
 } LIBBPF_1.5.0;
diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
index 375896a94e6a..fd98469fa20d 100644
--- a/tools/lib/bpf/linker.c
+++ b/tools/lib/bpf/linker.c
@@ -4,6 +4,10 @@
  *
  * Copyright (c) 2021 Facebook
  */
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE
+#endif
+
 #include <stdbool.h>
 #include <stddef.h>
 #include <stdio.h>
@@ -16,6 +20,7 @@
 #include <elf.h>
 #include <libelf.h>
 #include <fcntl.h>
+#include <sys/mman.h>
 #include "libbpf.h"
 #include "btf.h"
 #include "libbpf_internal.h"
@@ -152,6 +157,8 @@ struct bpf_linker {
 	/* global (including extern) ELF symbols */
 	int glob_sym_cnt;
 	struct glob_sym *glob_syms;
+
+	bool fd_is_owned;
 };
 
 #define pr_warn_elf(fmt, ...)									\
@@ -243,6 +250,54 @@ struct bpf_linker *bpf_linker__new(const char *filename, struct bpf_linker_opts
 		pr_warn("failed to create '%s': %d\n", filename, err);
 		goto err_out;
 	}
+	linker->fd_is_owned = true;
+
+	err = init_output_elf(linker);
+	if (err)
+		goto err_out;
+
+	return linker;
+
+err_out:
+	bpf_linker__free(linker);
+	return errno = -err, NULL;
+}
+
+#define LINKER_MAX_FD_NAME_SIZE 24
+
+struct bpf_linker *bpf_linker__new_fd(int fd, struct bpf_linker_opts *opts)
+{
+	struct bpf_linker *linker;
+	const char *filename;
+	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;
+
+	filename = OPTS_GET(opts, filename, NULL);
+	if (filename) {
+		linker->filename = strdup(filename);
+	} else {
+		linker->filename = malloc(LINKER_MAX_FD_NAME_SIZE);
+		if (!linker->filename)
+			return errno = ENOMEM, NULL;
+		snprintf(linker->filename, LINKER_MAX_FD_NAME_SIZE, "fd:%d", fd);
+	}
+
+	linker->fd = fd;
+	linker->fd_is_owned = false;
 
 	err = init_output_elf(linker);
 	if (err)
@@ -435,16 +490,15 @@ static int init_output_elf(struct bpf_linker *linker)
 }
 
 int bpf_linker__add_file(struct bpf_linker *linker, const char *filename,
-			 const struct bpf_linker_file_opts *opts)
+			 const struct bpf_linker_file_opts *input_opts)
 {
-	struct src_obj obj = {};
-	int err = 0, fd;
+	int fd, ret;
 
-	if (!OPTS_VALID(opts, bpf_linker_file_opts))
-		return libbpf_err(-EINVAL);
+	LIBBPF_OPTS(bpf_linker_file_opts, opts);
 
-	if (!linker->elf)
-		return libbpf_err(-EINVAL);
+	if (input_opts)
+		opts = *input_opts;
+	opts.filename = filename;
 
 	fd = open(filename, O_RDONLY | O_CLOEXEC);
 	if (fd < 0) {
@@ -452,6 +506,37 @@ int bpf_linker__add_file(struct bpf_linker *linker, const char *filename,
 		return -errno;
 	}
 
+	ret = bpf_linker__add_fd(linker, fd, &opts);
+
+	close(fd);
+
+	return ret;
+}
+
+int bpf_linker__add_fd(struct bpf_linker *linker, int fd,
+		       const struct bpf_linker_file_opts *opts)
+{
+	struct src_obj obj = {};
+	const char *filename;
+	char name[LINKER_MAX_FD_NAME_SIZE];
+	int err = 0;
+
+	if (!OPTS_VALID(opts, bpf_linker_file_opts))
+		return libbpf_err(-EINVAL);
+
+	if (!linker->elf)
+		return libbpf_err(-EINVAL);
+
+	if (fd < 0)
+		return libbpf_err(-EINVAL);
+
+	filename = OPTS_GET(opts, filename, NULL);
+	if (filename) {
+		obj.filename = filename;
+	} else {
+		snprintf(name, sizeof(name), "fd:%d", fd);
+		obj.filename = name;
+	}
 	obj.fd = fd;
 
 	err = err ?: linker_load_obj_file(linker, opts, &obj);
@@ -469,12 +554,47 @@ int bpf_linker__add_file(struct bpf_linker *linker, const char *filename,
 	free(obj.sym_map);
 	if (obj.elf)
 		elf_end(obj.elf);
-	if (obj.fd >= 0)
-		close(obj.fd);
+	/* leave obj.fd for the caller to clean up if appropriate */
 
 	return libbpf_err(err);
 }
 
+int bpf_linker__add_buf(struct bpf_linker *linker, const char *name,
+			void *buf, int buf_sz,
+			const struct bpf_linker_file_opts *input_opts)
+{
+	int fd, written, ret;
+
+	LIBBPF_OPTS(bpf_linker_file_opts, opts);
+
+	if (input_opts)
+		opts = *input_opts;
+	opts.filename = name;
+
+	fd = memfd_create(name, 0);
+	if (fd < 0) {
+		pr_warn("failed to create memfd '%s': %s\n", name, errstr(errno));
+		return -errno;
+	}
+
+	written = 0;
+	while (written < buf_sz) {
+		ret = write(fd, buf, buf_sz);
+		if (ret < 0) {
+			pr_warn("failed to write '%s': %s\n", name, errstr(errno));
+			return -errno;
+		}
+		written += ret;
+	}
+
+	ret = bpf_linker__add_fd(linker, fd, &opts);
+
+	if (fd >= 0)
+		close(fd);
+
+	return ret;
+}
+
 static bool is_dwarf_sec_name(const char *name)
 {
 	/* approximation, but the actual list is too long */
@@ -2691,9 +2811,10 @@ int bpf_linker__finalize(struct bpf_linker *linker)
 	}
 
 	elf_end(linker->elf);
-	close(linker->fd);
-
 	linker->elf = NULL;
+
+	if (linker->fd_is_owned)
+		close(linker->fd);
 	linker->fd = -1;
 
 	return 0;
-- 
2.43.5

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

[Andrii Nakryiko]

On Wed, Dec 4, 2024 at 8:11 AM Alastair Robertson <ajor@meta.com> wrote:
>
> The new_fd and add_fd functions correspond to the original new and
> add_file 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   |  12 +++-
>  tools/lib/bpf/libbpf.map |   3 +
>  tools/lib/bpf/linker.c   | 143 ++++++++++++++++++++++++++++++++++++---
>  3 files changed, 145 insertions(+), 13 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index b2ce3a72b11d..7a88830a3431 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -1784,21 +1784,29 @@ enum libbpf_tristate {
>  struct bpf_linker_opts {
>         /* size of this struct, for forward/backward compatibility */
>         size_t sz;
> +       const char *filename;
>  };
> -#define bpf_linker_opts__last_field sz
> +#define bpf_linker_opts__last_field filename
>
>  struct bpf_linker_file_opts {
>         /* size of this struct, for forward/backward compatibility */
>         size_t sz;
> +       const char *filename;
>  };
> -#define bpf_linker_file_opts__last_field sz
> +#define bpf_linker_file_opts__last_field filename
>
>  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(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, int fd,
> +                                 const struct bpf_linker_file_opts *opts);
> +LIBBPF_API int bpf_linker__add_buf(struct bpf_linker *linker, const char *name,
> +                                  void *buf, int buf_sz,
> +                                  const struct bpf_linker_file_opts *opts);
>  LIBBPF_API int bpf_linker__finalize(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..23f2a30778f0 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -432,4 +432,7 @@ LIBBPF_1.5.0 {
>  } LIBBPF_1.4.0;
>
>  LIBBPF_1.6.0 {

this should have "global:", see other version sections

> +               bpf_linker__add_buf;
> +               bpf_linker__add_fd;
> +               bpf_linker__new_fd;
>  } LIBBPF_1.5.0;
> diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> index 375896a94e6a..fd98469fa20d 100644
> --- a/tools/lib/bpf/linker.c
> +++ b/tools/lib/bpf/linker.c
> @@ -4,6 +4,10 @@
>   *
>   * Copyright (c) 2021 Facebook
>   */
> +#ifndef _GNU_SOURCE
> +#define _GNU_SOURCE
> +#endif
> +
>  #include <stdbool.h>
>  #include <stddef.h>
>  #include <stdio.h>
> @@ -16,6 +20,7 @@
>  #include <elf.h>
>  #include <libelf.h>
>  #include <fcntl.h>
> +#include <sys/mman.h>
>  #include "libbpf.h"
>  #include "btf.h"
>  #include "libbpf_internal.h"
> @@ -152,6 +157,8 @@ struct bpf_linker {
>         /* global (including extern) ELF symbols */
>         int glob_sym_cnt;
>         struct glob_sym *glob_syms;
> +
> +       bool fd_is_owned;
>  };
>
>  #define pr_warn_elf(fmt, ...)                                                                  \
> @@ -243,6 +250,54 @@ struct bpf_linker *bpf_linker__new(const char *filename, struct bpf_linker_opts
>                 pr_warn("failed to create '%s': %d\n", filename, err);
>                 goto err_out;
>         }
> +       linker->fd_is_owned = true;
> +
> +       err = init_output_elf(linker);
> +       if (err)
> +               goto err_out;
> +
> +       return linker;
> +
> +err_out:
> +       bpf_linker__free(linker);
> +       return errno = -err, NULL;
> +}
> +
> +#define LINKER_MAX_FD_NAME_SIZE 24

meh, overkill to add the constant, see below

> +
> +struct bpf_linker *bpf_linker__new_fd(int fd, struct bpf_linker_opts *opts)
> +{
> +       struct bpf_linker *linker;
> +       const char *filename;
> +       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;
> +
> +       filename = OPTS_GET(opts, filename, NULL);
> +       if (filename) {
> +               linker->filename = strdup(filename);
> +       } else {
> +               linker->filename = malloc(LINKER_MAX_FD_NAME_SIZE);
> +               if (!linker->filename)
> +                       return errno = ENOMEM, NULL;

so you didn't do strdup() result check for the case when filename is
specified. And you are not cleaning up calloc() on error. I'd
restructure this a bit differently and avoid LINKER_MAX_FD_NAME_SIZE:


char buf[32];

...

filename = OPTS_GET(opts, filename, NULL);
if (!filename) {
    snprintf(buf, sizeof(buf), "fd:%d", fd);
    filename = buf;
}
linker->filename = strdup(filename);
if (!linker->filename) {
    err = -ENOMEM;
    goto err_out;
}

WDYT?

> +               snprintf(linker->filename, LINKER_MAX_FD_NAME_SIZE, "fd:%d", fd);
> +       }
> +
> +       linker->fd = fd;
> +       linker->fd_is_owned = false;
>
>         err = init_output_elf(linker);
>         if (err)
> @@ -435,16 +490,15 @@ static int init_output_elf(struct bpf_linker *linker)
>  }
>
>  int bpf_linker__add_file(struct bpf_linker *linker, const char *filename,
> -                        const struct bpf_linker_file_opts *opts)
> +                        const struct bpf_linker_file_opts *input_opts)

don't rename, why?

>  {
> -       struct src_obj obj = {};
> -       int err = 0, fd;
> +       int fd, ret;
>
> -       if (!OPTS_VALID(opts, bpf_linker_file_opts))
> -               return libbpf_err(-EINVAL);
> +       LIBBPF_OPTS(bpf_linker_file_opts, opts);

this is a variable declaration, no empty lines between variable declarations

>
> -       if (!linker->elf)
> -               return libbpf_err(-EINVAL);
> +       if (input_opts)
> +               opts = *input_opts;

this is not OK due to backwards/forward compat issues (different sizes
of struct that user code knows about and libbpf expects). This bit me
before with skeletons, let's not repeat the same mistake.

but this does suck that filename hint has to be passed through opts,
so perhaps we do need a common function that won't rely on opts
struct. I.e.,

bpf_linker_add_file(linker, fd, filename) /* no other options are
supported, so nothing extra to pass */

and then bpf_linker__add_fd() and bpf_linker__add_file() both call
into bpf_linker_add_file() after checking opts for validity and
extracting filename/fd

pw-bot: cr

> +       opts.filename = filename;
>
>         fd = open(filename, O_RDONLY | O_CLOEXEC);
>         if (fd < 0) {
> @@ -452,6 +506,37 @@ int bpf_linker__add_file(struct bpf_linker *linker, const char *filename,
>                 return -errno;
>         }
>
> +       ret = bpf_linker__add_fd(linker, fd, &opts);
> +
> +       close(fd);
> +
> +       return ret;
> +}
> +
> +int bpf_linker__add_fd(struct bpf_linker *linker, int fd,
> +                      const struct bpf_linker_file_opts *opts)
> +{
> +       struct src_obj obj = {};
> +       const char *filename;
> +       char name[LINKER_MAX_FD_NAME_SIZE];
> +       int err = 0;
> +
> +       if (!OPTS_VALID(opts, bpf_linker_file_opts))
> +               return libbpf_err(-EINVAL);
> +
> +       if (!linker->elf)
> +               return libbpf_err(-EINVAL);
> +
> +       if (fd < 0)
> +               return libbpf_err(-EINVAL);
> +
> +       filename = OPTS_GET(opts, filename, NULL);
> +       if (filename) {
> +               obj.filename = filename;
> +       } else {
> +               snprintf(name, sizeof(name), "fd:%d", fd);
> +               obj.filename = name;
> +       }
>         obj.fd = fd;
>
>         err = err ?: linker_load_obj_file(linker, opts, &obj);
> @@ -469,12 +554,47 @@ int bpf_linker__add_file(struct bpf_linker *linker, const char *filename,
>         free(obj.sym_map);
>         if (obj.elf)
>                 elf_end(obj.elf);
> -       if (obj.fd >= 0)
> -               close(obj.fd);
> +       /* leave obj.fd for the caller to clean up if appropriate */
>
>         return libbpf_err(err);
>  }
>
> +int bpf_linker__add_buf(struct bpf_linker *linker, const char *name,

why is the buffer name passed as an argument instead of through
opts.filename? let's keep it simple and consistent

and if user didn't care to pass opts.filename, just do some
"mem:%p+%zu", buf, buf_sz thing

> +                       void *buf, int buf_sz,

size_t for buf_sz

> +                       const struct bpf_linker_file_opts *input_opts)

nit: "opts", keep it short and consistent

> +{
> +       int fd, written, ret;
> +
> +       LIBBPF_OPTS(bpf_linker_file_opts, opts);

same about empty lines between variable declarations

> +
> +       if (input_opts)
> +               opts = *input_opts;
> +       opts.filename = name;
> +

and that bpf_linker_add_file() common function should be easily used
here as well, right?

> +       fd = memfd_create(name, 0);
> +       if (fd < 0) {
> +               pr_warn("failed to create memfd '%s': %s\n", name, errstr(errno));
> +               return -errno;
> +       }
> +
> +       written = 0;
> +       while (written < buf_sz) {
> +               ret = write(fd, buf, buf_sz);
> +               if (ret < 0) {
> +                       pr_warn("failed to write '%s': %s\n", name, errstr(errno));
> +                       return -errno;
> +               }
> +               written += ret;
> +       }
> +
> +       ret = bpf_linker__add_fd(linker, fd, &opts);
> +
> +       if (fd >= 0)
> +               close(fd);
> +
> +       return ret;
> +}
> +
>  static bool is_dwarf_sec_name(const char *name)
>  {
>         /* approximation, but the actual list is too long */
> @@ -2691,9 +2811,10 @@ int bpf_linker__finalize(struct bpf_linker *linker)
>         }
>
>         elf_end(linker->elf);
> -       close(linker->fd);
> -
>         linker->elf = NULL;
> +
> +       if (linker->fd_is_owned)

you should do the same check in bpf_linker__free(), no?

> +               close(linker->fd);
>         linker->fd = -1;
>
>         return 0;
> --
> 2.43.5
>

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

[Alastair Robertson]

> >  {
> > -       struct src_obj obj =3D {};
> > -       int err =3D 0, fd;
> > +       int fd, ret;
> >
> > -       if (!OPTS_VALID(opts, bpf_linker_file_opts))
> > -               return libbpf_err(-EINVAL);
> > +       LIBBPF_OPTS(bpf_linker_file_opts, opts);
> 
> this is a variable declaration, no empty lines between variable declaration=
> s

I'd originally written it without the extra empty line but got complaints from
checkpatch.pl. Is it ok to just ignore its warnings?


> > +int bpf_linker__add_buf(struct bpf_linker *linker, const char *name,
> 
> why is the buffer name passed as an argument instead of through
> opts.filename? let's keep it simple and consistent
> 
> and if user didn't care to pass opts.filename, just do some
> "mem:%p+%zu", buf, buf_sz thing

Just because memfd_create() requires a filename so I was treating it as a
required argument for this function too. Happy to change it to this
suggestion though.

All other comments make sense and I'll address them in the next patch.

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

[Andrii Nakryiko]

On Thu, Dec 5, 2024 at 9:23 AM Alastair Robertson <ajor@meta.com> wrote:
>
> > >  {
> > > -       struct src_obj obj =3D {};
> > > -       int err =3D 0, fd;
> > > +       int fd, ret;
> > >
> > > -       if (!OPTS_VALID(opts, bpf_linker_file_opts))
> > > -               return libbpf_err(-EINVAL);
> > > +       LIBBPF_OPTS(bpf_linker_file_opts, opts);
> >
> > this is a variable declaration, no empty lines between variable declaration=
> > s
>
> I'd originally written it without the extra empty line but got complaints from
> checkpatch.pl. Is it ok to just ignore its warnings?
>

yep, if they are unreasonable :) checkpatch.pl is a guidance, not the
final authority

>
> > > +int bpf_linker__add_buf(struct bpf_linker *linker, const char *name,
> >
> > why is the buffer name passed as an argument instead of through
> > opts.filename? let's keep it simple and consistent
> >
> > and if user didn't care to pass opts.filename, just do some
> > "mem:%p+%zu", buf, buf_sz thing
>
> Just because memfd_create() requires a filename so I was treating it as a
> required argument for this function too. Happy to change it to this
> suggestion though.

but memfd_create() is an internal implementation detail, so let's not
leak that into public API

>
> All other comments make sense and I'll address them in the next patch.