On Wed, Dec 4, 2024 at 8:11 AM Alastair Robertson <ajor@xxxxxxxx> 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@xxxxxxxx>
> ---
> 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
>
