| Message ID | 20230316170149.4106586-6-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/bpf/perf: Store build id in file object | expand |
On Thu, Mar 16, 2023 at 10:03 AM Jiri Olsa <jolsa@kernel.org> wrote: > > Adding read_build_id function that parses out build id from > specified binary. > > It will replace extract_build_id and also be used in following > changes. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > tools/testing/selftests/bpf/trace_helpers.c | 86 +++++++++++++++++++++ > tools/testing/selftests/bpf/trace_helpers.h | 5 ++ > 2 files changed, 91 insertions(+) > > diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c > index 934bf28fc888..72b38a41f574 100644 > --- a/tools/testing/selftests/bpf/trace_helpers.c > +++ b/tools/testing/selftests/bpf/trace_helpers.c > @@ -11,6 +11,9 @@ > #include <linux/perf_event.h> > #include <sys/mman.h> > #include "trace_helpers.h" > +#include <linux/limits.h> > +#include <libelf.h> > +#include <gelf.h> > > #define TRACEFS_PIPE "/sys/kernel/tracing/trace_pipe" > #define DEBUGFS_PIPE "/sys/kernel/debug/tracing/trace_pipe" > @@ -234,3 +237,86 @@ ssize_t get_rel_offset(uintptr_t addr) > fclose(f); > return -EINVAL; > } > + > +static int > +parse_build_id_buf(const void *note_start, Elf32_Word note_size, > + char *build_id) nit: single line should we pass buffer size instead of assuming at least BPF_BUILD_ID_SIZE below? > +{ > + Elf32_Word note_offs = 0, new_offs; > + > + while (note_offs + sizeof(Elf32_Nhdr) < note_size) { > + Elf32_Nhdr *nhdr = (Elf32_Nhdr *)(note_start + note_offs); > + > + if (nhdr->n_type == 3 && nhdr->n_namesz == sizeof("GNU") && > + !strcmp((char *)(nhdr + 1), "GNU") && nhdr->n_descsz > 0 && > + nhdr->n_descsz <= BPF_BUILD_ID_SIZE) { > + memcpy(build_id, note_start + note_offs + > + ALIGN(sizeof("GNU"), 4) + sizeof(Elf32_Nhdr), nhdr->n_descsz); > + memset(build_id + nhdr->n_descsz, 0, BPF_BUILD_ID_SIZE - nhdr->n_descsz); > + return (int) nhdr->n_descsz; > + } > + > + new_offs = note_offs + sizeof(Elf32_Nhdr) + > + ALIGN(nhdr->n_namesz, 4) + ALIGN(nhdr->n_descsz, 4); > + if (new_offs >= note_size) > + break; while condition() above would handle this, so this check appears not necessary? so just assign note_offs directly? > + note_offs = new_offs; > + } > + > + return -EINVAL; nit: -ENOENT or -ESRCH? > +} > + > +/* Reads binary from *path* file and returns it in the *build_id* > + * which is expected to be at least BPF_BUILD_ID_SIZE bytes. > + * Returns size of build id on success. On error the error value > + * is returned. > + */ > +int read_build_id(const char *path, char *build_id) > +{ > + int fd, err = -EINVAL; > + Elf *elf = NULL; > + GElf_Ehdr ehdr; > + size_t max, i; > + > + fd = open(path, O_RDONLY | O_CLOEXEC); > + if (fd < 0) > + return -errno; > + > + (void)elf_version(EV_CURRENT); > + > + elf = elf_begin(fd, ELF_C_READ, NULL); ELF_C_READ_MMAP ? > + if (!elf) > + goto out; > + if (elf_kind(elf) != ELF_K_ELF) > + goto out; > + if (gelf_getehdr(elf, &ehdr) == NULL) nit: !gelf_getehdr() > + goto out; > + if (ehdr.e_ident[EI_CLASS] != ELFCLASS64) > + goto out; does this have to be 64-bit specific?... you are using gelf stuff, you can be bitness-agnostic here > + > + for (i = 0; i < ehdr.e_phnum; i++) { > + GElf_Phdr mem, *phdr; > + char *data; > + > + phdr = gelf_getphdr(elf, i, &mem); > + if (!phdr) > + goto out; > + if (phdr->p_type != PT_NOTE) > + continue; I don't know where ELF + build ID spec is (if at all), but it seems to always be in the ".note.gnu.build-id" section, so should we check the name here? > + data = elf_rawfile(elf, &max); > + if (!data) > + goto out; > + if (phdr->p_offset >= max || (phdr->p_offset + phdr->p_memsz >= max)) `phdr->p_offset + phdr->p_memsz == max` would be fine, no? > + goto out; > + err = parse_build_id_buf(data + phdr->p_offset, phdr->p_memsz, build_id); > + if (err > 0) > + goto out; > + err = -EINVAL; > + } > + > +out: > + if (elf) > + elf_end(elf); > + close(fd); > + return err; > +} > diff --git a/tools/testing/selftests/bpf/trace_helpers.h b/tools/testing/selftests/bpf/trace_helpers.h > index 53efde0e2998..bc3b92057033 100644 > --- a/tools/testing/selftests/bpf/trace_helpers.h > +++ b/tools/testing/selftests/bpf/trace_helpers.h > @@ -4,6 +4,9 @@ > > #include <bpf/libbpf.h> > > +#define __ALIGN_MASK(x, mask) (((x)+(mask))&~(mask)) > +#define ALIGN(x, a) __ALIGN_MASK(x, (typeof(x))(a)-1) > + > struct ksym { > long addr; > char *name; > @@ -23,4 +26,6 @@ void read_trace_pipe(void); > ssize_t get_uprobe_offset(const void *addr); > ssize_t get_rel_offset(uintptr_t addr); > > +int read_build_id(const char *path, char *build_id); > + > #endif > -- > 2.39.2 >
On Thu, Mar 16, 2023 at 03:23:03PM -0700, Andrii Nakryiko wrote: > On Thu, Mar 16, 2023 at 10:03 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > > Adding read_build_id function that parses out build id from > > specified binary. > > > > It will replace extract_build_id and also be used in following > > changes. > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> I'll send this separatelly as bpf/selftests fix so doesn't get lost > > --- > > tools/testing/selftests/bpf/trace_helpers.c | 86 +++++++++++++++++++++ > > tools/testing/selftests/bpf/trace_helpers.h | 5 ++ > > 2 files changed, 91 insertions(+) > > > > diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c > > index 934bf28fc888..72b38a41f574 100644 > > --- a/tools/testing/selftests/bpf/trace_helpers.c > > +++ b/tools/testing/selftests/bpf/trace_helpers.c > > @@ -11,6 +11,9 @@ > > #include <linux/perf_event.h> > > #include <sys/mman.h> > > #include "trace_helpers.h" > > +#include <linux/limits.h> > > +#include <libelf.h> > > +#include <gelf.h> > > > > #define TRACEFS_PIPE "/sys/kernel/tracing/trace_pipe" > > #define DEBUGFS_PIPE "/sys/kernel/debug/tracing/trace_pipe" > > @@ -234,3 +237,86 @@ ssize_t get_rel_offset(uintptr_t addr) > > fclose(f); > > return -EINVAL; > > } > > + > > +static int > > +parse_build_id_buf(const void *note_start, Elf32_Word note_size, > > + char *build_id) > > nit: single line ok > > should we pass buffer size instead of assuming at least BPF_BUILD_ID_SIZE below? ok > > > +{ > > + Elf32_Word note_offs = 0, new_offs; > > + > > + while (note_offs + sizeof(Elf32_Nhdr) < note_size) { > > + Elf32_Nhdr *nhdr = (Elf32_Nhdr *)(note_start + note_offs); > > + > > + if (nhdr->n_type == 3 && nhdr->n_namesz == sizeof("GNU") && > > + !strcmp((char *)(nhdr + 1), "GNU") && nhdr->n_descsz > 0 && > > + nhdr->n_descsz <= BPF_BUILD_ID_SIZE) { > > + memcpy(build_id, note_start + note_offs + > > + ALIGN(sizeof("GNU"), 4) + sizeof(Elf32_Nhdr), nhdr->n_descsz); > > + memset(build_id + nhdr->n_descsz, 0, BPF_BUILD_ID_SIZE - nhdr->n_descsz); > > + return (int) nhdr->n_descsz; > > + } > > + > > + new_offs = note_offs + sizeof(Elf32_Nhdr) + > > + ALIGN(nhdr->n_namesz, 4) + ALIGN(nhdr->n_descsz, 4); > > + if (new_offs >= note_size) > > + break; > > while condition() above would handle this, so this check appears not necessary? > > so just assign note_offs directly? good idea, it will simplify that > > > > + note_offs = new_offs; > > + } > > + > > + return -EINVAL; > > nit: -ENOENT or -ESRCH? I kept the same error as is in kernel, but ENOENT makes more sense > > > +} > > + > > +/* Reads binary from *path* file and returns it in the *build_id* > > + * which is expected to be at least BPF_BUILD_ID_SIZE bytes. > > + * Returns size of build id on success. On error the error value > > + * is returned. > > + */ > > +int read_build_id(const char *path, char *build_id) > > +{ > > + int fd, err = -EINVAL; > > + Elf *elf = NULL; > > + GElf_Ehdr ehdr; > > + size_t max, i; > > + > > + fd = open(path, O_RDONLY | O_CLOEXEC); > > + if (fd < 0) > > + return -errno; > > + > > + (void)elf_version(EV_CURRENT); > > + > > + elf = elf_begin(fd, ELF_C_READ, NULL); > > ELF_C_READ_MMAP ? ok > > > + if (!elf) > > + goto out; > > + if (elf_kind(elf) != ELF_K_ELF) > > + goto out; > > + if (gelf_getehdr(elf, &ehdr) == NULL) > > nit: !gelf_getehdr() ok > > > + goto out; > > + if (ehdr.e_ident[EI_CLASS] != ELFCLASS64) > > + goto out; > > does this have to be 64-bit specific?... you are using gelf stuff, you > can be bitness-agnostic here right, I don't think it's needed, will check > > > + > > + for (i = 0; i < ehdr.e_phnum; i++) { > > + GElf_Phdr mem, *phdr; > > + char *data; > > + > > + phdr = gelf_getphdr(elf, i, &mem); > > + if (!phdr) > > + goto out; > > + if (phdr->p_type != PT_NOTE) > > + continue; > > I don't know where ELF + build ID spec is (if at all), but it seems to > always be in the ".note.gnu.build-id" section, so should we check the > name here? this section name is not manadatory as stated in https://fedoraproject.org/wiki/RolandMcGrath/BuildID The new section is canonically called .note.gnu.build-id, but the name is not normative, and the section can be merged with other SHT_NOTE sections. The ELF note headers give name "GNU" and type 3 (NT_GNU_BUILD_ID) for a build ID note. > > > > + data = elf_rawfile(elf, &max); > > + if (!data) > > + goto out; > > + if (phdr->p_offset >= max || (phdr->p_offset + phdr->p_memsz >= max)) > > `phdr->p_offset + phdr->p_memsz == max` would be fine, no? right, will change thanks, jirka > > > + goto out; > > + err = parse_build_id_buf(data + phdr->p_offset, phdr->p_memsz, build_id); > > + if (err > 0) > > + goto out; > > + err = -EINVAL; > > + } > > + > > +out: > > + if (elf) > > + elf_end(elf); > > + close(fd); > > + return err; > > +} > > diff --git a/tools/testing/selftests/bpf/trace_helpers.h b/tools/testing/selftests/bpf/trace_helpers.h > > index 53efde0e2998..bc3b92057033 100644 > > --- a/tools/testing/selftests/bpf/trace_helpers.h > > +++ b/tools/testing/selftests/bpf/trace_helpers.h > > @@ -4,6 +4,9 @@ > > > > #include <bpf/libbpf.h> > > > > +#define __ALIGN_MASK(x, mask) (((x)+(mask))&~(mask)) > > +#define ALIGN(x, a) __ALIGN_MASK(x, (typeof(x))(a)-1) > > + > > struct ksym { > > long addr; > > char *name; > > @@ -23,4 +26,6 @@ void read_trace_pipe(void); > > ssize_t get_uprobe_offset(const void *addr); > > ssize_t get_rel_offset(uintptr_t addr); > > > > +int read_build_id(const char *path, char *build_id); > > + > > #endif > > -- > > 2.39.2 > >
diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c index 934bf28fc888..72b38a41f574 100644 --- a/tools/testing/selftests/bpf/trace_helpers.c +++ b/tools/testing/selftests/bpf/trace_helpers.c @@ -11,6 +11,9 @@ #include <linux/perf_event.h> #include <sys/mman.h> #include "trace_helpers.h" +#include <linux/limits.h> +#include <libelf.h> +#include <gelf.h> #define TRACEFS_PIPE "/sys/kernel/tracing/trace_pipe" #define DEBUGFS_PIPE "/sys/kernel/debug/tracing/trace_pipe" @@ -234,3 +237,86 @@ ssize_t get_rel_offset(uintptr_t addr) fclose(f); return -EINVAL; } + +static int +parse_build_id_buf(const void *note_start, Elf32_Word note_size, + char *build_id) +{ + Elf32_Word note_offs = 0, new_offs; + + while (note_offs + sizeof(Elf32_Nhdr) < note_size) { + Elf32_Nhdr *nhdr = (Elf32_Nhdr *)(note_start + note_offs); + + if (nhdr->n_type == 3 && nhdr->n_namesz == sizeof("GNU") && + !strcmp((char *)(nhdr + 1), "GNU") && nhdr->n_descsz > 0 && + nhdr->n_descsz <= BPF_BUILD_ID_SIZE) { + memcpy(build_id, note_start + note_offs + + ALIGN(sizeof("GNU"), 4) + sizeof(Elf32_Nhdr), nhdr->n_descsz); + memset(build_id + nhdr->n_descsz, 0, BPF_BUILD_ID_SIZE - nhdr->n_descsz); + return (int) nhdr->n_descsz; + } + + new_offs = note_offs + sizeof(Elf32_Nhdr) + + ALIGN(nhdr->n_namesz, 4) + ALIGN(nhdr->n_descsz, 4); + if (new_offs >= note_size) + break; + note_offs = new_offs; + } + + return -EINVAL; +} + +/* Reads binary from *path* file and returns it in the *build_id* + * which is expected to be at least BPF_BUILD_ID_SIZE bytes. + * Returns size of build id on success. On error the error value + * is returned. + */ +int read_build_id(const char *path, char *build_id) +{ + int fd, err = -EINVAL; + Elf *elf = NULL; + GElf_Ehdr ehdr; + size_t max, i; + + fd = open(path, O_RDONLY | O_CLOEXEC); + if (fd < 0) + return -errno; + + (void)elf_version(EV_CURRENT); + + elf = elf_begin(fd, ELF_C_READ, NULL); + if (!elf) + goto out; + if (elf_kind(elf) != ELF_K_ELF) + goto out; + if (gelf_getehdr(elf, &ehdr) == NULL) + goto out; + if (ehdr.e_ident[EI_CLASS] != ELFCLASS64) + goto out; + + for (i = 0; i < ehdr.e_phnum; i++) { + GElf_Phdr mem, *phdr; + char *data; + + phdr = gelf_getphdr(elf, i, &mem); + if (!phdr) + goto out; + if (phdr->p_type != PT_NOTE) + continue; + data = elf_rawfile(elf, &max); + if (!data) + goto out; + if (phdr->p_offset >= max || (phdr->p_offset + phdr->p_memsz >= max)) + goto out; + err = parse_build_id_buf(data + phdr->p_offset, phdr->p_memsz, build_id); + if (err > 0) + goto out; + err = -EINVAL; + } + +out: + if (elf) + elf_end(elf); + close(fd); + return err; +} diff --git a/tools/testing/selftests/bpf/trace_helpers.h b/tools/testing/selftests/bpf/trace_helpers.h index 53efde0e2998..bc3b92057033 100644 --- a/tools/testing/selftests/bpf/trace_helpers.h +++ b/tools/testing/selftests/bpf/trace_helpers.h @@ -4,6 +4,9 @@ #include <bpf/libbpf.h> +#define __ALIGN_MASK(x, mask) (((x)+(mask))&~(mask)) +#define ALIGN(x, a) __ALIGN_MASK(x, (typeof(x))(a)-1) + struct ksym { long addr; char *name; @@ -23,4 +26,6 @@ void read_trace_pipe(void); ssize_t get_uprobe_offset(const void *addr); ssize_t get_rel_offset(uintptr_t addr); +int read_build_id(const char *path, char *build_id); + #endif
Adding read_build_id function that parses out build id from specified binary. It will replace extract_build_id and also be used in following changes. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- tools/testing/selftests/bpf/trace_helpers.c | 86 +++++++++++++++++++++ tools/testing/selftests/bpf/trace_helpers.h | 5 ++ 2 files changed, 91 insertions(+)