diff mbox series

[PATCHv3,bpf-next,08/26] libbpf: Add elf_open/elf_close functions

Message ID 20230630083344.984305-9-jolsa@kernel.org (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series bpf: Add multi uprobe link | expand

Checks

Context Check Description
netdev/series_format fail Series longer than 15 patches (and no cover letter)
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers warning 3 maintainers not CCed: kpsingh@kernel.org martin.lau@linux.dev song@kernel.org
netdev/build_clang fail Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: line length of 88 exceeds 80 columns WARNING: line length of 94 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ${{ matrix.test }} on ${{ matrix.arch }} with ${{ matrix.toolchain_full }}
bpf/vmtest-bpf-next-VM_Test-2 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 fail Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-7 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-8 success Logs for veristat

Commit Message

Jiri Olsa June 30, 2023, 8:33 a.m. UTC 
Adding elf_open/elf_close functions and using it in
elf_find_func_offset_from_file function. It will be
used in following changes to save some common code.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/lib/bpf/elf.c        | 59 +++++++++++++++++++++++++-------------
 tools/lib/bpf/libbpf_elf.h |  8 ++++++
 tools/lib/bpf/usdt.c       | 31 ++++++--------------
 3 files changed, 56 insertions(+), 42 deletions(-)

Comments

Andrii Nakryiko July 6, 2023, 11:09 p.m. UTC | #1 
On Fri, Jun 30, 2023 at 1:35 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding elf_open/elf_close functions and using it in
> elf_find_func_offset_from_file function. It will be
> used in following changes to save some common code.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/lib/bpf/elf.c        | 59 +++++++++++++++++++++++++-------------
>  tools/lib/bpf/libbpf_elf.h |  8 ++++++
>  tools/lib/bpf/usdt.c       | 31 ++++++--------------
>  3 files changed, 56 insertions(+), 42 deletions(-)
>

one nit below

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> diff --git a/tools/lib/bpf/elf.c b/tools/lib/bpf/elf.c
> index 2b62b4af28ce..74e35071d22e 100644
> --- a/tools/lib/bpf/elf.c
> +++ b/tools/lib/bpf/elf.c
> @@ -11,6 +11,40 @@
>
>  #define STRERR_BUFSIZE  128
>
> +int elf_open(const char *binary_path, struct elf_fd *elf_fd)
> +{
> +       char errmsg[STRERR_BUFSIZE];
> +       int fd, ret;
> +       Elf *elf;
> +
> +       if (elf_version(EV_CURRENT) == EV_NONE) {
> +               pr_warn("elf: failed to init libelf for %s\n", binary_path);
> +               return -LIBBPF_ERRNO__LIBELF;
> +       }
> +       fd = open(binary_path, O_RDONLY | O_CLOEXEC);
> +       if (fd < 0) {
> +               ret = -errno;
> +               pr_warn("elf: failed to open %s: %s\n", binary_path,
> +                       libbpf_strerror_r(ret, errmsg, sizeof(errmsg)));
> +               return ret;
> +       }
> +       elf = elf_begin(fd, ELF_C_READ_MMAP, NULL);
> +       if (!elf) {
> +               pr_warn("elf: could not read elf from %s: %s\n", binary_path, elf_errmsg(-1));
> +               close(fd);
> +               return -LIBBPF_ERRNO__FORMAT;
> +       }
> +       elf_fd->fd = fd;
> +       elf_fd->elf = elf;
> +       return 0;
> +}
> +
> +void elf_close(struct elf_fd *elf_fd)
> +{
> +       elf_end(elf_fd->elf);
> +       close(elf_fd->fd);

nit: I'd make elf_close() work correctly with a) NULL elf_fd and b)
NULL elf_fd->elf, just to never have to think about this

> +}
> +
>  /* Return next ELF section of sh_type after scn, or first of that type if scn is NULL. */
>  static Elf_Scn *elf_find_next_scn_by_type(Elf *elf, int sh_type, Elf_Scn *scn)
>  {

[...]
Jiri Olsa July 11, 2023, 9:01 a.m. UTC | #2 
On Thu, Jul 06, 2023 at 04:09:29PM -0700, Andrii Nakryiko wrote:
> On Fri, Jun 30, 2023 at 1:35 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding elf_open/elf_close functions and using it in
> > elf_find_func_offset_from_file function. It will be
> > used in following changes to save some common code.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/lib/bpf/elf.c        | 59 +++++++++++++++++++++++++-------------
> >  tools/lib/bpf/libbpf_elf.h |  8 ++++++
> >  tools/lib/bpf/usdt.c       | 31 ++++++--------------
> >  3 files changed, 56 insertions(+), 42 deletions(-)
> >
> 
> one nit below
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> > diff --git a/tools/lib/bpf/elf.c b/tools/lib/bpf/elf.c
> > index 2b62b4af28ce..74e35071d22e 100644
> > --- a/tools/lib/bpf/elf.c
> > +++ b/tools/lib/bpf/elf.c
> > @@ -11,6 +11,40 @@
> >
> >  #define STRERR_BUFSIZE  128
> >
> > +int elf_open(const char *binary_path, struct elf_fd *elf_fd)
> > +{
> > +       char errmsg[STRERR_BUFSIZE];
> > +       int fd, ret;
> > +       Elf *elf;
> > +
> > +       if (elf_version(EV_CURRENT) == EV_NONE) {
> > +               pr_warn("elf: failed to init libelf for %s\n", binary_path);
> > +               return -LIBBPF_ERRNO__LIBELF;
> > +       }
> > +       fd = open(binary_path, O_RDONLY | O_CLOEXEC);
> > +       if (fd < 0) {
> > +               ret = -errno;
> > +               pr_warn("elf: failed to open %s: %s\n", binary_path,
> > +                       libbpf_strerror_r(ret, errmsg, sizeof(errmsg)));
> > +               return ret;
> > +       }
> > +       elf = elf_begin(fd, ELF_C_READ_MMAP, NULL);
> > +       if (!elf) {
> > +               pr_warn("elf: could not read elf from %s: %s\n", binary_path, elf_errmsg(-1));
> > +               close(fd);
> > +               return -LIBBPF_ERRNO__FORMAT;
> > +       }
> > +       elf_fd->fd = fd;
> > +       elf_fd->elf = elf;
> > +       return 0;
> > +}
> > +
> > +void elf_close(struct elf_fd *elf_fd)
> > +{
> > +       elf_end(elf_fd->elf);
> > +       close(elf_fd->fd);
> 
> nit: I'd make elf_close() work correctly with a) NULL elf_fd and b)

right, ok

> NULL elf_fd->elf, just to never have to think about this

there's NULL check in elf_end

thanks,
jirka
diff mbox series

Patch

diff --git a/tools/lib/bpf/elf.c b/tools/lib/bpf/elf.c
index 2b62b4af28ce..74e35071d22e 100644
--- a/tools/lib/bpf/elf.c
+++ b/tools/lib/bpf/elf.c
@@ -11,6 +11,40 @@ 
 
 #define STRERR_BUFSIZE  128
 
+int elf_open(const char *binary_path, struct elf_fd *elf_fd)
+{
+	char errmsg[STRERR_BUFSIZE];
+	int fd, ret;
+	Elf *elf;
+
+	if (elf_version(EV_CURRENT) == EV_NONE) {
+		pr_warn("elf: failed to init libelf for %s\n", binary_path);
+		return -LIBBPF_ERRNO__LIBELF;
+	}
+	fd = open(binary_path, O_RDONLY | O_CLOEXEC);
+	if (fd < 0) {
+		ret = -errno;
+		pr_warn("elf: failed to open %s: %s\n", binary_path,
+			libbpf_strerror_r(ret, errmsg, sizeof(errmsg)));
+		return ret;
+	}
+	elf = elf_begin(fd, ELF_C_READ_MMAP, NULL);
+	if (!elf) {
+		pr_warn("elf: could not read elf from %s: %s\n", binary_path, elf_errmsg(-1));
+		close(fd);
+		return -LIBBPF_ERRNO__FORMAT;
+	}
+	elf_fd->fd = fd;
+	elf_fd->elf = elf;
+	return 0;
+}
+
+void elf_close(struct elf_fd *elf_fd)
+{
+	elf_end(elf_fd->elf);
+	close(elf_fd->fd);
+}
+
 /* Return next ELF section of sh_type after scn, or first of that type if scn is NULL. */
 static Elf_Scn *elf_find_next_scn_by_type(Elf *elf, int sh_type, Elf_Scn *scn)
 {
@@ -171,28 +205,13 @@  long elf_find_func_offset(Elf *elf, const char *binary_path, const char *name)
  */
 long elf_find_func_offset_from_file(const char *binary_path, const char *name)
 {
-	char errmsg[STRERR_BUFSIZE];
+	struct elf_fd elf_fd;
 	long ret = -ENOENT;
-	Elf *elf;
-	int fd;
 
-	fd = open(binary_path, O_RDONLY | O_CLOEXEC);
-	if (fd < 0) {
-		ret = -errno;
-		pr_warn("failed to open %s: %s\n", binary_path,
-			libbpf_strerror_r(ret, errmsg, sizeof(errmsg)));
+	ret = elf_open(binary_path, &elf_fd);
+	if (ret)
 		return ret;
-	}
-	elf = elf_begin(fd, ELF_C_READ_MMAP, NULL);
-	if (!elf) {
-		pr_warn("elf: could not read elf from %s: %s\n", binary_path, elf_errmsg(-1));
-		close(fd);
-		return -LIBBPF_ERRNO__FORMAT;
-	}
-
-	ret = elf_find_func_offset(elf, binary_path, name);
-	elf_end(elf);
-	close(fd);
+	ret = elf_find_func_offset(elf_fd.elf, binary_path, name);
+	elf_close(&elf_fd);
 	return ret;
 }
-
diff --git a/tools/lib/bpf/libbpf_elf.h b/tools/lib/bpf/libbpf_elf.h
index 1b652220fabf..c763ac35a85e 100644
--- a/tools/lib/bpf/libbpf_elf.h
+++ b/tools/lib/bpf/libbpf_elf.h
@@ -5,6 +5,14 @@ 
 
 #include <libelf.h>
 
+struct elf_fd {
+	Elf *elf;
+	int fd;
+};
+
+int elf_open(const char *binary_path, struct elf_fd *elf_fd);
+void elf_close(struct elf_fd *elf_fd);
+
 long elf_find_func_offset(Elf *elf, const char *binary_path, const char *name);
 long elf_find_func_offset_from_file(const char *binary_path, const char *name);
 
diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c
index f1a141555f08..9fa883ebc0bd 100644
--- a/tools/lib/bpf/usdt.c
+++ b/tools/lib/bpf/usdt.c
@@ -19,6 +19,7 @@ 
 #include "libbpf.h"
 #include "libbpf_common.h"
 #include "libbpf_internal.h"
+#include "libbpf_elf.h"
 #include "hashmap.h"
 
 /* libbpf's USDT support consists of BPF-side state/code and user-space
@@ -943,32 +944,22 @@  struct bpf_link *usdt_manager_attach_usdt(struct usdt_manager *man, const struct
 					  const char *usdt_provider, const char *usdt_name,
 					  __u64 usdt_cookie)
 {
-	int i, fd, err, spec_map_fd, ip_map_fd;
+	int i, err, spec_map_fd, ip_map_fd;
 	LIBBPF_OPTS(bpf_uprobe_opts, opts);
 	struct hashmap *specs_hash = NULL;
 	struct bpf_link_usdt *link = NULL;
 	struct usdt_target *targets = NULL;
+	struct elf_fd elf_fd;
 	size_t target_cnt;
-	Elf *elf;
 
 	spec_map_fd = bpf_map__fd(man->specs_map);
 	ip_map_fd = bpf_map__fd(man->ip_to_spec_id_map);
 
-	fd = open(path, O_RDONLY | O_CLOEXEC);
-	if (fd < 0) {
-		err = -errno;
-		pr_warn("usdt: failed to open ELF binary '%s': %d\n", path, err);
+	err = elf_open(path, &elf_fd);
+	if (err)
 		return libbpf_err_ptr(err);
-	}
 
-	elf = elf_begin(fd, ELF_C_READ_MMAP, NULL);
-	if (!elf) {
-		err = -EBADF;
-		pr_warn("usdt: failed to parse ELF binary '%s': %s\n", path, elf_errmsg(-1));
-		goto err_out;
-	}
-
-	err = sanity_check_usdt_elf(elf, path);
+	err = sanity_check_usdt_elf(elf_fd.elf, path);
 	if (err)
 		goto err_out;
 
@@ -981,7 +972,7 @@  struct bpf_link *usdt_manager_attach_usdt(struct usdt_manager *man, const struct
 	/* discover USDT in given binary, optionally limiting
 	 * activations to a given PID, if pid > 0
 	 */
-	err = collect_usdt_targets(man, elf, path, pid, usdt_provider, usdt_name,
+	err = collect_usdt_targets(man, elf_fd.elf, path, pid, usdt_provider, usdt_name,
 				   usdt_cookie, &targets, &target_cnt);
 	if (err <= 0) {
 		err = (err == 0) ? -ENOENT : err;
@@ -1066,9 +1057,7 @@  struct bpf_link *usdt_manager_attach_usdt(struct usdt_manager *man, const struct
 
 	free(targets);
 	hashmap__free(specs_hash);
-	elf_end(elf);
-	close(fd);
-
+	elf_close(&elf_fd);
 	return &link->link;
 
 err_out:
@@ -1076,9 +1065,7 @@  struct bpf_link *usdt_manager_attach_usdt(struct usdt_manager *man, const struct
 		bpf_link__destroy(&link->link);
 	free(targets);
 	hashmap__free(specs_hash);
-	if (elf)
-		elf_end(elf);
-	close(fd);
+	elf_close(&elf_fd);
 	return libbpf_err_ptr(err);
 }