| Message ID | 20230217191908.1000004-3-deso@posteo.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: Make uprobe attachment APK aware | expand |
On Fri, Feb 17, 2023 at 11:19 AM Daniel Müller <deso@posteo.net> wrote: > > This change splits the elf_find_func_offset() function in two: > elf_find_func_offset(), which now accepts an already opened Elf object > instead of a path to a file that is to be opened, as well as > elf_find_func_offset_from_elf_file(), which opens a binary based on a > path and then invokes elf_find_func_offset() on the Elf object. Having > this split in responsibilities will allow us to call > elf_find_func_offset() from other code paths on Elf objects that did not > necessarily come from a file on disk. > > Signed-off-by: Daniel Müller <deso@posteo.net> > --- > tools/lib/bpf/libbpf.c | 55 +++++++++++++++++++++++++++--------------- > 1 file changed, 35 insertions(+), 20 deletions(-) > Looks good, just few pedantic nits > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 05c4db3..a474f49 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -10531,32 +10531,19 @@ static Elf_Scn *elf_find_next_scn_by_type(Elf *elf, int sh_type, Elf_Scn *scn) > return NULL; > } > > -/* Find offset of function name in object specified by path. "name" matches > - * symbol name or name@@LIB for library functions. > +/* Find offset of function name in the provided ELF object. "binary_path" is > + * the path to the ELF binary represented by "elf", and only used for error > + * reporting matters. "name" matches symbol name or name@@LIB for library > + * functions. > */ > -static long elf_find_func_offset(const char *binary_path, const char *name) > +static long elf_find_func_offset(Elf *elf, const char *binary_path, const char *name) > { > - int fd, i, sh_types[2] = { SHT_DYNSYM, SHT_SYMTAB }; > + int i, sh_types[2] = { SHT_DYNSYM, SHT_SYMTAB }; > bool is_shared_lib, is_name_qualified; > - char errmsg[STRERR_BUFSIZE]; > long ret = -ENOENT; > size_t name_len; > GElf_Ehdr ehdr; > - Elf *elf; > > - 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))); > - 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; > - } > if (!gelf_getehdr(elf, &ehdr)) { > pr_warn("elf: failed to get ehdr from %s: %s\n", binary_path, elf_errmsg(-1)); > ret = -LIBBPF_ERRNO__FORMAT; > @@ -10682,6 +10669,34 @@ static long elf_find_func_offset(const char *binary_path, const char *name) > } > } > out: > + return ret; > +} > + > +/* Find offset of function name in ELF object specified by path. "name" matches nit: seems like it's original spelling, but let's remove these double spaces (same above in elf_find_func_offset comment) > + * symbol name or name@@LIB for library functions. > + */ > +static long elf_find_func_offset_from_elf_file(const char *binary_path, const char *name) "from_file" would be enough, reads a bit tautological otherwise > +{ > + char errmsg[STRERR_BUFSIZE]; > + long ret = -ENOENT; > + Elf *elf; > + int fd; > + > + fd = open(binary_path, O_RDONLY | O_CLOEXEC); btw, this reminded me that in patch #1 we probably want to pass O_CLOEXEC as well? > + if (fd < 0) { > + ret = -errno; > + pr_warn("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; > + } > + > + ret = elf_find_func_offset(elf, binary_path, name); > elf_end(elf); > close(fd); > return ret; > @@ -10805,7 +10820,7 @@ bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid, > if (func_name) { > long sym_off; > > - sym_off = elf_find_func_offset(binary_path, func_name); > + sym_off = elf_find_func_offset_from_elf_file(binary_path, func_name); > if (sym_off < 0) > return libbpf_err_ptr(sym_off); > func_offset += sym_off; > -- > 2.30.2 >
On Fri, Feb 17, 2023 at 04:14:12PM -0800, Andrii Nakryiko wrote: > On Fri, Feb 17, 2023 at 11:19 AM Daniel Müller <deso@posteo.net> wrote: > > > > This change splits the elf_find_func_offset() function in two: > > elf_find_func_offset(), which now accepts an already opened Elf object > > instead of a path to a file that is to be opened, as well as > > elf_find_func_offset_from_elf_file(), which opens a binary based on a > > path and then invokes elf_find_func_offset() on the Elf object. Having > > this split in responsibilities will allow us to call > > elf_find_func_offset() from other code paths on Elf objects that did not > > necessarily come from a file on disk. > > > > Signed-off-by: Daniel Müller <deso@posteo.net> > > --- > > tools/lib/bpf/libbpf.c | 55 +++++++++++++++++++++++++++--------------- > > 1 file changed, 35 insertions(+), 20 deletions(-) > > > > Looks good, just few pedantic nits > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index 05c4db3..a474f49 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -10531,32 +10531,19 @@ static Elf_Scn *elf_find_next_scn_by_type(Elf *elf, int sh_type, Elf_Scn *scn) > > return NULL; > > } > > > > -/* Find offset of function name in object specified by path. "name" matches > > - * symbol name or name@@LIB for library functions. > > +/* Find offset of function name in the provided ELF object. "binary_path" is > > + * the path to the ELF binary represented by "elf", and only used for error > > + * reporting matters. "name" matches symbol name or name@@LIB for library > > + * functions. > > */ > > -static long elf_find_func_offset(const char *binary_path, const char *name) > > +static long elf_find_func_offset(Elf *elf, const char *binary_path, const char *name) > > { > > - int fd, i, sh_types[2] = { SHT_DYNSYM, SHT_SYMTAB }; > > + int i, sh_types[2] = { SHT_DYNSYM, SHT_SYMTAB }; > > bool is_shared_lib, is_name_qualified; > > - char errmsg[STRERR_BUFSIZE]; > > long ret = -ENOENT; > > size_t name_len; > > GElf_Ehdr ehdr; > > - Elf *elf; > > > > - 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))); > > - 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; > > - } > > if (!gelf_getehdr(elf, &ehdr)) { > > pr_warn("elf: failed to get ehdr from %s: %s\n", binary_path, elf_errmsg(-1)); > > ret = -LIBBPF_ERRNO__FORMAT; > > @@ -10682,6 +10669,34 @@ static long elf_find_func_offset(const char *binary_path, const char *name) > > } > > } > > out: > > + return ret; > > +} > > + > > +/* Find offset of function name in ELF object specified by path. "name" matches > > nit: seems like it's original spelling, but let's remove these double > spaces (same above in elf_find_func_offset comment) Done. > > + * symbol name or name@@LIB for library functions. > > + */ > > +static long elf_find_func_offset_from_elf_file(const char *binary_path, const char *name) > > "from_file" would be enough, reads a bit tautological otherwise Sure. > > +{ > > + char errmsg[STRERR_BUFSIZE]; > > + long ret = -ENOENT; > > + Elf *elf; > > + int fd; > > + > > + fd = open(binary_path, O_RDONLY | O_CLOEXEC); > > btw, this reminded me that in patch #1 we probably want to pass > O_CLOEXEC as well? Sounds good. > > + if (fd < 0) { > > + ret = -errno; > > + pr_warn("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; > > + } > > + > > + ret = elf_find_func_offset(elf, binary_path, name); > > elf_end(elf); > > close(fd); > > return ret; > > @@ -10805,7 +10820,7 @@ bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid, > > if (func_name) { > > long sym_off; > > > > - sym_off = elf_find_func_offset(binary_path, func_name); > > + sym_off = elf_find_func_offset_from_elf_file(binary_path, func_name); > > if (sym_off < 0) > > return libbpf_err_ptr(sym_off); > > func_offset += sym_off; > > -- > > 2.30.2 > >
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 05c4db3..a474f49 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -10531,32 +10531,19 @@ static Elf_Scn *elf_find_next_scn_by_type(Elf *elf, int sh_type, Elf_Scn *scn) return NULL; } -/* Find offset of function name in object specified by path. "name" matches - * symbol name or name@@LIB for library functions. +/* Find offset of function name in the provided ELF object. "binary_path" is + * the path to the ELF binary represented by "elf", and only used for error + * reporting matters. "name" matches symbol name or name@@LIB for library + * functions. */ -static long elf_find_func_offset(const char *binary_path, const char *name) +static long elf_find_func_offset(Elf *elf, const char *binary_path, const char *name) { - int fd, i, sh_types[2] = { SHT_DYNSYM, SHT_SYMTAB }; + int i, sh_types[2] = { SHT_DYNSYM, SHT_SYMTAB }; bool is_shared_lib, is_name_qualified; - char errmsg[STRERR_BUFSIZE]; long ret = -ENOENT; size_t name_len; GElf_Ehdr ehdr; - Elf *elf; - 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))); - 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; - } if (!gelf_getehdr(elf, &ehdr)) { pr_warn("elf: failed to get ehdr from %s: %s\n", binary_path, elf_errmsg(-1)); ret = -LIBBPF_ERRNO__FORMAT; @@ -10682,6 +10669,34 @@ static long elf_find_func_offset(const char *binary_path, const char *name) } } out: + return ret; +} + +/* Find offset of function name in ELF object specified by path. "name" matches + * symbol name or name@@LIB for library functions. + */ +static long elf_find_func_offset_from_elf_file(const char *binary_path, const char *name) +{ + char errmsg[STRERR_BUFSIZE]; + 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))); + 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); return ret; @@ -10805,7 +10820,7 @@ bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid, if (func_name) { long sym_off; - sym_off = elf_find_func_offset(binary_path, func_name); + sym_off = elf_find_func_offset_from_elf_file(binary_path, func_name); if (sym_off < 0) return libbpf_err_ptr(sym_off); func_offset += sym_off;
This change splits the elf_find_func_offset() function in two: elf_find_func_offset(), which now accepts an already opened Elf object instead of a path to a file that is to be opened, as well as elf_find_func_offset_from_elf_file(), which opens a binary based on a path and then invokes elf_find_func_offset() on the Elf object. Having this split in responsibilities will allow us to call elf_find_func_offset() from other code paths on Elf objects that did not necessarily come from a file on disk. Signed-off-by: Daniel Müller <deso@posteo.net> --- tools/lib/bpf/libbpf.c | 55 +++++++++++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 20 deletions(-)