* [PATCH bpf-next 1/3] bpf: Move stack_map_get_build_id into lib
2021-01-14 13:40 [PATCHv7 bpf-next 0/3] perf: Add mmap2 build id support Jiri Olsa
@ 2021-01-14 13:40 ` Jiri Olsa
2021-01-14 13:40 ` [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function Jiri Olsa
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Jiri Olsa @ 2021-01-14 13:40 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Alexei Starovoitov, Song Liu, lkml, bpf, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Namhyung Kim, Alexander Shishkin,
Michael Petlan, Ian Rogers, Stephane Eranian, Alexei Budankov,
Andi Kleen, Adrian Hunter
Moving stack_map_get_build_id into lib with
declaration in linux/buildid.h header:
int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id);
This function returns build id for given struct vm_area_struct.
There is no functional change to stack_map_get_build_id function.
Cc: Alexei Starovoitov <ast@kernel.org>
Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
include/linux/buildid.h | 11 ++++
kernel/bpf/stackmap.c | 143 ++--------------------------------------
lib/Makefile | 3 +-
lib/buildid.c | 136 ++++++++++++++++++++++++++++++++++++++
4 files changed, 153 insertions(+), 140 deletions(-)
create mode 100644 include/linux/buildid.h
create mode 100644 lib/buildid.c
diff --git a/include/linux/buildid.h b/include/linux/buildid.h
new file mode 100644
index 000000000000..08028a212589
--- /dev/null
+++ b/include/linux/buildid.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_BUILDID_H
+#define _LINUX_BUILDID_H
+
+#include <linux/mm_types.h>
+
+#define BUILD_ID_SIZE_MAX 20
+
+int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id);
+
+#endif
diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index aea96b638473..55d254a59f07 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -7,10 +7,9 @@
#include <linux/kernel.h>
#include <linux/stacktrace.h>
#include <linux/perf_event.h>
-#include <linux/elf.h>
-#include <linux/pagemap.h>
#include <linux/irq_work.h>
#include <linux/btf_ids.h>
+#include <linux/buildid.h>
#include "percpu_freelist.h"
#define STACK_CREATE_FLAG_MASK \
@@ -143,140 +142,6 @@ static struct bpf_map *stack_map_alloc(union bpf_attr *attr)
return ERR_PTR(err);
}
-#define BPF_BUILD_ID 3
-/*
- * Parse build id from the note segment. This logic can be shared between
- * 32-bit and 64-bit system, because Elf32_Nhdr and Elf64_Nhdr are
- * identical.
- */
-static inline int stack_map_parse_build_id(void *page_addr,
- unsigned char *build_id,
- void *note_start,
- Elf32_Word note_size)
-{
- Elf32_Word note_offs = 0, new_offs;
-
- /* check for overflow */
- if (note_start < page_addr || note_start + note_size < note_start)
- return -EINVAL;
-
- /* only supports note that fits in the first page */
- if (note_start + note_size > page_addr + PAGE_SIZE)
- return -EINVAL;
-
- while (note_offs + sizeof(Elf32_Nhdr) < note_size) {
- Elf32_Nhdr *nhdr = (Elf32_Nhdr *)(note_start + note_offs);
-
- if (nhdr->n_type == BPF_BUILD_ID &&
- nhdr->n_namesz == sizeof("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 0;
- }
- new_offs = note_offs + sizeof(Elf32_Nhdr) +
- ALIGN(nhdr->n_namesz, 4) + ALIGN(nhdr->n_descsz, 4);
- if (new_offs <= note_offs) /* overflow */
- break;
- note_offs = new_offs;
- }
- return -EINVAL;
-}
-
-/* Parse build ID from 32-bit ELF */
-static int stack_map_get_build_id_32(void *page_addr,
- unsigned char *build_id)
-{
- Elf32_Ehdr *ehdr = (Elf32_Ehdr *)page_addr;
- Elf32_Phdr *phdr;
- int i;
-
- /* only supports phdr that fits in one page */
- if (ehdr->e_phnum >
- (PAGE_SIZE - sizeof(Elf32_Ehdr)) / sizeof(Elf32_Phdr))
- return -EINVAL;
-
- phdr = (Elf32_Phdr *)(page_addr + sizeof(Elf32_Ehdr));
-
- for (i = 0; i < ehdr->e_phnum; ++i) {
- if (phdr[i].p_type == PT_NOTE &&
- !stack_map_parse_build_id(page_addr, build_id,
- page_addr + phdr[i].p_offset,
- phdr[i].p_filesz))
- return 0;
- }
- return -EINVAL;
-}
-
-/* Parse build ID from 64-bit ELF */
-static int stack_map_get_build_id_64(void *page_addr,
- unsigned char *build_id)
-{
- Elf64_Ehdr *ehdr = (Elf64_Ehdr *)page_addr;
- Elf64_Phdr *phdr;
- int i;
-
- /* only supports phdr that fits in one page */
- if (ehdr->e_phnum >
- (PAGE_SIZE - sizeof(Elf64_Ehdr)) / sizeof(Elf64_Phdr))
- return -EINVAL;
-
- phdr = (Elf64_Phdr *)(page_addr + sizeof(Elf64_Ehdr));
-
- for (i = 0; i < ehdr->e_phnum; ++i) {
- if (phdr[i].p_type == PT_NOTE &&
- !stack_map_parse_build_id(page_addr, build_id,
- page_addr + phdr[i].p_offset,
- phdr[i].p_filesz))
- return 0;
- }
- return -EINVAL;
-}
-
-/* Parse build ID of ELF file mapped to vma */
-static int stack_map_get_build_id(struct vm_area_struct *vma,
- unsigned char *build_id)
-{
- Elf32_Ehdr *ehdr;
- struct page *page;
- void *page_addr;
- int ret;
-
- /* only works for page backed storage */
- if (!vma->vm_file)
- return -EINVAL;
-
- page = find_get_page(vma->vm_file->f_mapping, 0);
- if (!page)
- return -EFAULT; /* page not mapped */
-
- ret = -EINVAL;
- page_addr = kmap_atomic(page);
- ehdr = (Elf32_Ehdr *)page_addr;
-
- /* compare magic x7f "ELF" */
- if (memcmp(ehdr->e_ident, ELFMAG, SELFMAG) != 0)
- goto out;
-
- /* only support executable file and shared object file */
- if (ehdr->e_type != ET_EXEC && ehdr->e_type != ET_DYN)
- goto out;
-
- if (ehdr->e_ident[EI_CLASS] == ELFCLASS32)
- ret = stack_map_get_build_id_32(page_addr, build_id);
- else if (ehdr->e_ident[EI_CLASS] == ELFCLASS64)
- ret = stack_map_get_build_id_64(page_addr, build_id);
-out:
- kunmap_atomic(page_addr);
- put_page(page);
- return ret;
-}
-
static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
u64 *ips, u32 trace_nr, bool user)
{
@@ -317,18 +182,18 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
for (i = 0; i < trace_nr; i++) {
id_offs[i].status = BPF_STACK_BUILD_ID_IP;
id_offs[i].ip = ips[i];
- memset(id_offs[i].build_id, 0, BPF_BUILD_ID_SIZE);
+ memset(id_offs[i].build_id, 0, BUILD_ID_SIZE_MAX);
}
return;
}
for (i = 0; i < trace_nr; i++) {
vma = find_vma(current->mm, ips[i]);
- if (!vma || stack_map_get_build_id(vma, id_offs[i].build_id)) {
+ if (!vma || build_id_parse(vma, id_offs[i].build_id)) {
/* per entry fall back to ips */
id_offs[i].status = BPF_STACK_BUILD_ID_IP;
id_offs[i].ip = ips[i];
- memset(id_offs[i].build_id, 0, BPF_BUILD_ID_SIZE);
+ memset(id_offs[i].build_id, 0, BUILD_ID_SIZE_MAX);
continue;
}
id_offs[i].offset = (vma->vm_pgoff << PAGE_SHIFT) + ips[i]
diff --git a/lib/Makefile b/lib/Makefile
index afeff05fa8c5..a6b160c3a4fa 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -36,7 +36,8 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \
flex_proportions.o ratelimit.o show_mem.o \
is_single_threaded.o plist.o decompress.o kobject_uevent.o \
earlycpio.o seq_buf.o siphash.o dec_and_lock.o \
- nmi_backtrace.o nodemask.o win_minmax.o memcat_p.o
+ nmi_backtrace.o nodemask.o win_minmax.o memcat_p.o \
+ buildid.o
lib-$(CONFIG_PRINTK) += dump_stack.o
lib-$(CONFIG_SMP) += cpumask.o
diff --git a/lib/buildid.c b/lib/buildid.c
new file mode 100644
index 000000000000..4a4f520c0e29
--- /dev/null
+++ b/lib/buildid.c
@@ -0,0 +1,136 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/buildid.h>
+#include <linux/elf.h>
+#include <linux/pagemap.h>
+
+#define BUILD_ID 3
+/*
+ * Parse build id from the note segment. This logic can be shared between
+ * 32-bit and 64-bit system, because Elf32_Nhdr and Elf64_Nhdr are
+ * identical.
+ */
+static inline int parse_build_id(void *page_addr,
+ unsigned char *build_id,
+ void *note_start,
+ Elf32_Word note_size)
+{
+ Elf32_Word note_offs = 0, new_offs;
+
+ /* check for overflow */
+ if (note_start < page_addr || note_start + note_size < note_start)
+ return -EINVAL;
+
+ /* only supports note that fits in the first page */
+ if (note_start + note_size > page_addr + PAGE_SIZE)
+ return -EINVAL;
+
+ while (note_offs + sizeof(Elf32_Nhdr) < note_size) {
+ Elf32_Nhdr *nhdr = (Elf32_Nhdr *)(note_start + note_offs);
+
+ if (nhdr->n_type == BUILD_ID &&
+ nhdr->n_namesz == sizeof("GNU") &&
+ nhdr->n_descsz > 0 &&
+ nhdr->n_descsz <= BUILD_ID_SIZE_MAX) {
+ memcpy(build_id,
+ note_start + note_offs +
+ ALIGN(sizeof("GNU"), 4) + sizeof(Elf32_Nhdr),
+ nhdr->n_descsz);
+ memset(build_id + nhdr->n_descsz, 0,
+ BUILD_ID_SIZE_MAX - nhdr->n_descsz);
+ return 0;
+ }
+ new_offs = note_offs + sizeof(Elf32_Nhdr) +
+ ALIGN(nhdr->n_namesz, 4) + ALIGN(nhdr->n_descsz, 4);
+ if (new_offs <= note_offs) /* overflow */
+ break;
+ note_offs = new_offs;
+ }
+ return -EINVAL;
+}
+
+/* Parse build ID from 32-bit ELF */
+static int get_build_id_32(void *page_addr, unsigned char *build_id)
+{
+ Elf32_Ehdr *ehdr = (Elf32_Ehdr *)page_addr;
+ Elf32_Phdr *phdr;
+ int i;
+
+ /* only supports phdr that fits in one page */
+ if (ehdr->e_phnum >
+ (PAGE_SIZE - sizeof(Elf32_Ehdr)) / sizeof(Elf32_Phdr))
+ return -EINVAL;
+
+ phdr = (Elf32_Phdr *)(page_addr + sizeof(Elf32_Ehdr));
+
+ for (i = 0; i < ehdr->e_phnum; ++i) {
+ if (phdr[i].p_type == PT_NOTE &&
+ !parse_build_id(page_addr, build_id,
+ page_addr + phdr[i].p_offset,
+ phdr[i].p_filesz))
+ return 0;
+ }
+ return -EINVAL;
+}
+
+/* Parse build ID from 64-bit ELF */
+static int get_build_id_64(void *page_addr, unsigned char *build_id)
+{
+ Elf64_Ehdr *ehdr = (Elf64_Ehdr *)page_addr;
+ Elf64_Phdr *phdr;
+ int i;
+
+ /* only supports phdr that fits in one page */
+ if (ehdr->e_phnum >
+ (PAGE_SIZE - sizeof(Elf64_Ehdr)) / sizeof(Elf64_Phdr))
+ return -EINVAL;
+
+ phdr = (Elf64_Phdr *)(page_addr + sizeof(Elf64_Ehdr));
+
+ for (i = 0; i < ehdr->e_phnum; ++i) {
+ if (phdr[i].p_type == PT_NOTE &&
+ !parse_build_id(page_addr, build_id,
+ page_addr + phdr[i].p_offset,
+ phdr[i].p_filesz))
+ return 0;
+ }
+ return -EINVAL;
+}
+
+/* Parse build ID of ELF file mapped to vma */
+int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id)
+{
+ Elf32_Ehdr *ehdr;
+ struct page *page;
+ void *page_addr;
+ int ret;
+
+ /* only works for page backed storage */
+ if (!vma->vm_file)
+ return -EINVAL;
+
+ page = find_get_page(vma->vm_file->f_mapping, 0);
+ if (!page)
+ return -EFAULT; /* page not mapped */
+
+ ret = -EINVAL;
+ page_addr = kmap_atomic(page);
+ ehdr = (Elf32_Ehdr *)page_addr;
+
+ /* compare magic x7f "ELF" */
+ if (memcmp(ehdr->e_ident, ELFMAG, SELFMAG) != 0)
+ goto out;
+
+ /* only support executable file and shared object file */
+ if (ehdr->e_type != ET_EXEC && ehdr->e_type != ET_DYN)
+ goto out;
+
+ if (ehdr->e_ident[EI_CLASS] == ELFCLASS32)
+ ret = get_build_id_32(page_addr, build_id);
+ else if (ehdr->e_ident[EI_CLASS] == ELFCLASS64)
+ ret = get_build_id_64(page_addr, build_id);
+out:
+ kunmap_atomic(page_addr);
+ put_page(page);
+ return ret;
+}
--
2.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function
2021-01-14 13:40 [PATCHv7 bpf-next 0/3] perf: Add mmap2 build id support Jiri Olsa
2021-01-14 13:40 ` [PATCH bpf-next 1/3] bpf: Move stack_map_get_build_id into lib Jiri Olsa
@ 2021-01-14 13:40 ` Jiri Olsa
2021-01-14 18:56 ` Yonghong Song
2021-01-14 13:40 ` [PATCH bpf-next 3/3] perf: Add build id data in mmap2 event Jiri Olsa
2021-01-15 3:50 ` [PATCHv7 bpf-next 0/3] perf: Add mmap2 build id support patchwork-bot+netdevbpf
3 siblings, 1 reply; 13+ messages in thread
From: Jiri Olsa @ 2021-01-14 13:40 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Alexei Starovoitov, Song Liu, lkml, bpf, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Namhyung Kim, Alexander Shishkin,
Michael Petlan, Ian Rogers, Stephane Eranian, Alexei Budankov,
Andi Kleen, Adrian Hunter
It's possible to have other build id types (other than default SHA1).
Currently there's also ld support for MD5 build id.
Adding size argument to build_id_parse function, that returns (if defined)
size of the parsed build id, so we can recognize the build id type.
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
include/linux/buildid.h | 3 ++-
kernel/bpf/stackmap.c | 2 +-
lib/buildid.c | 29 +++++++++++++++++++++--------
3 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/include/linux/buildid.h b/include/linux/buildid.h
index 08028a212589..40232f90db6e 100644
--- a/include/linux/buildid.h
+++ b/include/linux/buildid.h
@@ -6,6 +6,7 @@
#define BUILD_ID_SIZE_MAX 20
-int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id);
+int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
+ __u32 *size);
#endif
diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index 55d254a59f07..cabaf7db8efc 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -189,7 +189,7 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
for (i = 0; i < trace_nr; i++) {
vma = find_vma(current->mm, ips[i]);
- if (!vma || build_id_parse(vma, id_offs[i].build_id)) {
+ if (!vma || build_id_parse(vma, id_offs[i].build_id, NULL)) {
/* per entry fall back to ips */
id_offs[i].status = BPF_STACK_BUILD_ID_IP;
id_offs[i].ip = ips[i];
diff --git a/lib/buildid.c b/lib/buildid.c
index 4a4f520c0e29..6156997c3895 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -12,6 +12,7 @@
*/
static inline int parse_build_id(void *page_addr,
unsigned char *build_id,
+ __u32 *size,
void *note_start,
Elf32_Word note_size)
{
@@ -38,6 +39,8 @@ static inline int parse_build_id(void *page_addr,
nhdr->n_descsz);
memset(build_id + nhdr->n_descsz, 0,
BUILD_ID_SIZE_MAX - nhdr->n_descsz);
+ if (size)
+ *size = nhdr->n_descsz;
return 0;
}
new_offs = note_offs + sizeof(Elf32_Nhdr) +
@@ -50,7 +53,8 @@ static inline int parse_build_id(void *page_addr,
}
/* Parse build ID from 32-bit ELF */
-static int get_build_id_32(void *page_addr, unsigned char *build_id)
+static int get_build_id_32(void *page_addr, unsigned char *build_id,
+ __u32 *size)
{
Elf32_Ehdr *ehdr = (Elf32_Ehdr *)page_addr;
Elf32_Phdr *phdr;
@@ -65,7 +69,7 @@ static int get_build_id_32(void *page_addr, unsigned char *build_id)
for (i = 0; i < ehdr->e_phnum; ++i) {
if (phdr[i].p_type == PT_NOTE &&
- !parse_build_id(page_addr, build_id,
+ !parse_build_id(page_addr, build_id, size,
page_addr + phdr[i].p_offset,
phdr[i].p_filesz))
return 0;
@@ -74,7 +78,8 @@ static int get_build_id_32(void *page_addr, unsigned char *build_id)
}
/* Parse build ID from 64-bit ELF */
-static int get_build_id_64(void *page_addr, unsigned char *build_id)
+static int get_build_id_64(void *page_addr, unsigned char *build_id,
+ __u32 *size)
{
Elf64_Ehdr *ehdr = (Elf64_Ehdr *)page_addr;
Elf64_Phdr *phdr;
@@ -89,7 +94,7 @@ static int get_build_id_64(void *page_addr, unsigned char *build_id)
for (i = 0; i < ehdr->e_phnum; ++i) {
if (phdr[i].p_type == PT_NOTE &&
- !parse_build_id(page_addr, build_id,
+ !parse_build_id(page_addr, build_id, size,
page_addr + phdr[i].p_offset,
phdr[i].p_filesz))
return 0;
@@ -97,8 +102,16 @@ static int get_build_id_64(void *page_addr, unsigned char *build_id)
return -EINVAL;
}
-/* Parse build ID of ELF file mapped to vma */
-int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id)
+/*
+ * Parse build ID of ELF file mapped to vma
+ * @vma: vma object
+ * @build_id: buffer to store build id, at least BUILD_ID_SIZE long
+ * @size: returns actual build id size in case of success
+ *
+ * Returns 0 on success, otherwise error (< 0).
+ */
+int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
+ __u32 *size)
{
Elf32_Ehdr *ehdr;
struct page *page;
@@ -126,9 +139,9 @@ int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id)
goto out;
if (ehdr->e_ident[EI_CLASS] == ELFCLASS32)
- ret = get_build_id_32(page_addr, build_id);
+ ret = get_build_id_32(page_addr, build_id, size);
else if (ehdr->e_ident[EI_CLASS] == ELFCLASS64)
- ret = get_build_id_64(page_addr, build_id);
+ ret = get_build_id_64(page_addr, build_id, size);
out:
kunmap_atomic(page_addr);
put_page(page);
--
2.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function
2021-01-14 13:40 ` [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function Jiri Olsa
@ 2021-01-14 18:56 ` Yonghong Song
2021-01-14 20:01 ` Jiri Olsa
0 siblings, 1 reply; 13+ messages in thread
From: Yonghong Song @ 2021-01-14 18:56 UTC (permalink / raw)
To: Jiri Olsa, Arnaldo Carvalho de Melo
Cc: Alexei Starovoitov, Song Liu, lkml, bpf, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Namhyung Kim, Alexander Shishkin,
Michael Petlan, Ian Rogers, Stephane Eranian, Alexei Budankov,
Andi Kleen, Adrian Hunter
On 1/14/21 5:40 AM, Jiri Olsa wrote:
> It's possible to have other build id types (other than default SHA1).
> Currently there's also ld support for MD5 build id.
Currently, bpf build_id based stackmap does not returns the size of
the build_id. Did you see an issue here? I guess user space can check
the length of non-zero bits of the build id to decide what kind of
type it is, right?
>
> Adding size argument to build_id_parse function, that returns (if defined)
> size of the parsed build id, so we can recognize the build id type.
>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Song Liu <songliubraving@fb.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> include/linux/buildid.h | 3 ++-
> kernel/bpf/stackmap.c | 2 +-
> lib/buildid.c | 29 +++++++++++++++++++++--------
> 3 files changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/buildid.h b/include/linux/buildid.h
> index 08028a212589..40232f90db6e 100644
> --- a/include/linux/buildid.h
> +++ b/include/linux/buildid.h
> @@ -6,6 +6,7 @@
>
> #define BUILD_ID_SIZE_MAX 20
>
> -int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id);
> +int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
> + __u32 *size);
>
> #endif
> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index 55d254a59f07..cabaf7db8efc 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c
> @@ -189,7 +189,7 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
>
> for (i = 0; i < trace_nr; i++) {
> vma = find_vma(current->mm, ips[i]);
> - if (!vma || build_id_parse(vma, id_offs[i].build_id)) {
> + if (!vma || build_id_parse(vma, id_offs[i].build_id, NULL)) {
> /* per entry fall back to ips */
> id_offs[i].status = BPF_STACK_BUILD_ID_IP;
> id_offs[i].ip = ips[i];
> diff --git a/lib/buildid.c b/lib/buildid.c
> index 4a4f520c0e29..6156997c3895 100644
> --- a/lib/buildid.c
> +++ b/lib/buildid.c
> @@ -12,6 +12,7 @@
> */
> static inline int parse_build_id(void *page_addr,
> unsigned char *build_id,
> + __u32 *size,
> void *note_start,
> Elf32_Word note_size)
> {
> @@ -38,6 +39,8 @@ static inline int parse_build_id(void *page_addr,
> nhdr->n_descsz);
> memset(build_id + nhdr->n_descsz, 0,
> BUILD_ID_SIZE_MAX - nhdr->n_descsz);
> + if (size)
> + *size = nhdr->n_descsz;
> return 0;
> }
> new_offs = note_offs + sizeof(Elf32_Nhdr) +
> @@ -50,7 +53,8 @@ static inline int parse_build_id(void *page_addr,
> }
>
[...]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function
2021-01-14 18:56 ` Yonghong Song
@ 2021-01-14 20:01 ` Jiri Olsa
2021-01-14 21:05 ` Yonghong Song
0 siblings, 1 reply; 13+ messages in thread
From: Jiri Olsa @ 2021-01-14 20:01 UTC (permalink / raw)
To: Yonghong Song
Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Alexei Starovoitov,
Song Liu, lkml, bpf, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Namhyung Kim, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian, Alexei Budankov, Andi Kleen, Adrian Hunter
On Thu, Jan 14, 2021 at 10:56:33AM -0800, Yonghong Song wrote:
>
>
> On 1/14/21 5:40 AM, Jiri Olsa wrote:
> > It's possible to have other build id types (other than default SHA1).
> > Currently there's also ld support for MD5 build id.
>
> Currently, bpf build_id based stackmap does not returns the size of
> the build_id. Did you see an issue here? I guess user space can check
> the length of non-zero bits of the build id to decide what kind of
> type it is, right?
you can have zero bytes in the build id hash, so you need to get the size
I never saw MD5 being used in practise just SHA1, but we added the
size to be complete and make sure we'll fit with build id, because
there's only limited space in mmap2 event
jirka
>
> >
> > Adding size argument to build_id_parse function, that returns (if defined)
> > size of the parsed build id, so we can recognize the build id type.
> >
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Song Liu <songliubraving@fb.com>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > include/linux/buildid.h | 3 ++-
> > kernel/bpf/stackmap.c | 2 +-
> > lib/buildid.c | 29 +++++++++++++++++++++--------
> > 3 files changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/linux/buildid.h b/include/linux/buildid.h
> > index 08028a212589..40232f90db6e 100644
> > --- a/include/linux/buildid.h
> > +++ b/include/linux/buildid.h
> > @@ -6,6 +6,7 @@
> > #define BUILD_ID_SIZE_MAX 20
> > -int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id);
> > +int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
> > + __u32 *size);
> > #endif
> > diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> > index 55d254a59f07..cabaf7db8efc 100644
> > --- a/kernel/bpf/stackmap.c
> > +++ b/kernel/bpf/stackmap.c
> > @@ -189,7 +189,7 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
> > for (i = 0; i < trace_nr; i++) {
> > vma = find_vma(current->mm, ips[i]);
> > - if (!vma || build_id_parse(vma, id_offs[i].build_id)) {
> > + if (!vma || build_id_parse(vma, id_offs[i].build_id, NULL)) {
> > /* per entry fall back to ips */
> > id_offs[i].status = BPF_STACK_BUILD_ID_IP;
> > id_offs[i].ip = ips[i];
> > diff --git a/lib/buildid.c b/lib/buildid.c
> > index 4a4f520c0e29..6156997c3895 100644
> > --- a/lib/buildid.c
> > +++ b/lib/buildid.c
> > @@ -12,6 +12,7 @@
> > */
> > static inline int parse_build_id(void *page_addr,
> > unsigned char *build_id,
> > + __u32 *size,
> > void *note_start,
> > Elf32_Word note_size)
> > {
> > @@ -38,6 +39,8 @@ static inline int parse_build_id(void *page_addr,
> > nhdr->n_descsz);
> > memset(build_id + nhdr->n_descsz, 0,
> > BUILD_ID_SIZE_MAX - nhdr->n_descsz);
> > + if (size)
> > + *size = nhdr->n_descsz;
> > return 0;
> > }
> > new_offs = note_offs + sizeof(Elf32_Nhdr) +
> > @@ -50,7 +53,8 @@ static inline int parse_build_id(void *page_addr,
> > }
> [...]
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function
2021-01-14 20:01 ` Jiri Olsa
@ 2021-01-14 21:05 ` Yonghong Song
2021-01-14 22:02 ` Jiri Olsa
0 siblings, 1 reply; 13+ messages in thread
From: Yonghong Song @ 2021-01-14 21:05 UTC (permalink / raw)
To: Jiri Olsa
Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Alexei Starovoitov,
Song Liu, lkml, bpf, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Namhyung Kim, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian, Alexei Budankov, Andi Kleen, Adrian Hunter
On 1/14/21 12:01 PM, Jiri Olsa wrote:
> On Thu, Jan 14, 2021 at 10:56:33AM -0800, Yonghong Song wrote:
>>
>>
>> On 1/14/21 5:40 AM, Jiri Olsa wrote:
>>> It's possible to have other build id types (other than default SHA1).
>>> Currently there's also ld support for MD5 build id.
>>
>> Currently, bpf build_id based stackmap does not returns the size of
>> the build_id. Did you see an issue here? I guess user space can check
>> the length of non-zero bits of the build id to decide what kind of
>> type it is, right?
>
> you can have zero bytes in the build id hash, so you need to get the size
>
> I never saw MD5 being used in practise just SHA1, but we added the
> size to be complete and make sure we'll fit with build id, because
> there's only limited space in mmap2 event
I am asking to check whether we should extend uapi struct
bpf_stack_build_id to include build_id_size as well. I guess
we can delay this until a real use case.
>
> jirka
>
>>
>>>
>>> Adding size argument to build_id_parse function, that returns (if defined)
>>> size of the parsed build id, so we can recognize the build id type.
>>>
>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>> Cc: Song Liu <songliubraving@fb.com>
>>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>>> ---
>>> include/linux/buildid.h | 3 ++-
>>> kernel/bpf/stackmap.c | 2 +-
>>> lib/buildid.c | 29 +++++++++++++++++++++--------
>>> 3 files changed, 24 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/include/linux/buildid.h b/include/linux/buildid.h
>>> index 08028a212589..40232f90db6e 100644
>>> --- a/include/linux/buildid.h
>>> +++ b/include/linux/buildid.h
>>> @@ -6,6 +6,7 @@
>>> #define BUILD_ID_SIZE_MAX 20
>>> -int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id);
>>> +int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
>>> + __u32 *size);
>>> #endif
>>> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
>>> index 55d254a59f07..cabaf7db8efc 100644
>>> --- a/kernel/bpf/stackmap.c
>>> +++ b/kernel/bpf/stackmap.c
>>> @@ -189,7 +189,7 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
>>> for (i = 0; i < trace_nr; i++) {
>>> vma = find_vma(current->mm, ips[i]);
>>> - if (!vma || build_id_parse(vma, id_offs[i].build_id)) {
>>> + if (!vma || build_id_parse(vma, id_offs[i].build_id, NULL)) {
>>> /* per entry fall back to ips */
>>> id_offs[i].status = BPF_STACK_BUILD_ID_IP;
>>> id_offs[i].ip = ips[i];
>>> diff --git a/lib/buildid.c b/lib/buildid.c
>>> index 4a4f520c0e29..6156997c3895 100644
>>> --- a/lib/buildid.c
>>> +++ b/lib/buildid.c
>>> @@ -12,6 +12,7 @@
>>> */
>>> static inline int parse_build_id(void *page_addr,
>>> unsigned char *build_id,
>>> + __u32 *size,
>>> void *note_start,
>>> Elf32_Word note_size)
>>> {
>>> @@ -38,6 +39,8 @@ static inline int parse_build_id(void *page_addr,
>>> nhdr->n_descsz);
>>> memset(build_id + nhdr->n_descsz, 0,
>>> BUILD_ID_SIZE_MAX - nhdr->n_descsz);
>>> + if (size)
>>> + *size = nhdr->n_descsz;
>>> return 0;
>>> }
>>> new_offs = note_offs + sizeof(Elf32_Nhdr) +
>>> @@ -50,7 +53,8 @@ static inline int parse_build_id(void *page_addr,
>>> }
>> [...]
>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function
2021-01-14 21:05 ` Yonghong Song
@ 2021-01-14 22:02 ` Jiri Olsa
2021-01-14 23:43 ` Yonghong Song
0 siblings, 1 reply; 13+ messages in thread
From: Jiri Olsa @ 2021-01-14 22:02 UTC (permalink / raw)
To: Yonghong Song
Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Alexei Starovoitov,
Song Liu, lkml, bpf, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Namhyung Kim, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian, Alexei Budankov, Andi Kleen, Adrian Hunter
On Thu, Jan 14, 2021 at 01:05:33PM -0800, Yonghong Song wrote:
>
>
> On 1/14/21 12:01 PM, Jiri Olsa wrote:
> > On Thu, Jan 14, 2021 at 10:56:33AM -0800, Yonghong Song wrote:
> > >
> > >
> > > On 1/14/21 5:40 AM, Jiri Olsa wrote:
> > > > It's possible to have other build id types (other than default SHA1).
> > > > Currently there's also ld support for MD5 build id.
> > >
> > > Currently, bpf build_id based stackmap does not returns the size of
> > > the build_id. Did you see an issue here? I guess user space can check
> > > the length of non-zero bits of the build id to decide what kind of
> > > type it is, right?
> >
> > you can have zero bytes in the build id hash, so you need to get the size
> >
> > I never saw MD5 being used in practise just SHA1, but we added the
> > size to be complete and make sure we'll fit with build id, because
> > there's only limited space in mmap2 event
>
> I am asking to check whether we should extend uapi struct
> bpf_stack_build_id to include build_id_size as well. I guess
> we can delay this until a real use case.
right, we can try make some MD5 build id binaries and check if it
explodes with some bcc tools, but I don't expect that.. I'll try
to find some time for that
perf tool uses build ids in .debug cache as file links, and we had
few isues there
jirka
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function
2021-01-14 22:02 ` Jiri Olsa
@ 2021-01-14 23:43 ` Yonghong Song
2021-01-15 3:47 ` Alexei Starovoitov
0 siblings, 1 reply; 13+ messages in thread
From: Yonghong Song @ 2021-01-14 23:43 UTC (permalink / raw)
To: Jiri Olsa
Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Alexei Starovoitov,
Song Liu, lkml, bpf, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Namhyung Kim, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian, Alexei Budankov, Andi Kleen, Adrian Hunter
On 1/14/21 2:02 PM, Jiri Olsa wrote:
> On Thu, Jan 14, 2021 at 01:05:33PM -0800, Yonghong Song wrote:
>>
>>
>> On 1/14/21 12:01 PM, Jiri Olsa wrote:
>>> On Thu, Jan 14, 2021 at 10:56:33AM -0800, Yonghong Song wrote:
>>>>
>>>>
>>>> On 1/14/21 5:40 AM, Jiri Olsa wrote:
>>>>> It's possible to have other build id types (other than default SHA1).
>>>>> Currently there's also ld support for MD5 build id.
>>>>
>>>> Currently, bpf build_id based stackmap does not returns the size of
>>>> the build_id. Did you see an issue here? I guess user space can check
>>>> the length of non-zero bits of the build id to decide what kind of
>>>> type it is, right?
>>>
>>> you can have zero bytes in the build id hash, so you need to get the size
>>>
>>> I never saw MD5 being used in practise just SHA1, but we added the
>>> size to be complete and make sure we'll fit with build id, because
>>> there's only limited space in mmap2 event
>>
>> I am asking to check whether we should extend uapi struct
>> bpf_stack_build_id to include build_id_size as well. I guess
>> we can delay this until a real use case.
>
> right, we can try make some MD5 build id binaries and check if it
> explodes with some bcc tools, but I don't expect that.. I'll try
> to find some time for that
Thanks. We may have issues on bcc side. For build_id collected in
kernel, bcc always generates a length-20 string. But for user
binaries, the build_id string length is equal to actual size of
the build_id. They may not match (MD5 length is 16).
The fix is probably to append '0's (up to length 20) for user
binary build_id's.
I guess MD5 is very seldom used. I will wait if you can reproduce
the issue and then we might fix it.
>
> perf tool uses build ids in .debug cache as file links, and we had
> few isues there
>
> jirka
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function
2021-01-14 23:43 ` Yonghong Song
@ 2021-01-15 3:47 ` Alexei Starovoitov
2021-01-26 20:52 ` Jiri Olsa
0 siblings, 1 reply; 13+ messages in thread
From: Alexei Starovoitov @ 2021-01-15 3:47 UTC (permalink / raw)
To: Yonghong Song
Cc: Jiri Olsa, Jiri Olsa, Arnaldo Carvalho de Melo,
Alexei Starovoitov, Song Liu, lkml, bpf, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Namhyung Kim, Alexander Shishkin,
Michael Petlan, Ian Rogers, Stephane Eranian, Alexei Budankov,
Andi Kleen, Adrian Hunter
On Thu, Jan 14, 2021 at 3:44 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 1/14/21 2:02 PM, Jiri Olsa wrote:
> > On Thu, Jan 14, 2021 at 01:05:33PM -0800, Yonghong Song wrote:
> >>
> >>
> >> On 1/14/21 12:01 PM, Jiri Olsa wrote:
> >>> On Thu, Jan 14, 2021 at 10:56:33AM -0800, Yonghong Song wrote:
> >>>>
> >>>>
> >>>> On 1/14/21 5:40 AM, Jiri Olsa wrote:
> >>>>> It's possible to have other build id types (other than default SHA1).
> >>>>> Currently there's also ld support for MD5 build id.
> >>>>
> >>>> Currently, bpf build_id based stackmap does not returns the size of
> >>>> the build_id. Did you see an issue here? I guess user space can check
> >>>> the length of non-zero bits of the build id to decide what kind of
> >>>> type it is, right?
> >>>
> >>> you can have zero bytes in the build id hash, so you need to get the size
> >>>
> >>> I never saw MD5 being used in practise just SHA1, but we added the
> >>> size to be complete and make sure we'll fit with build id, because
> >>> there's only limited space in mmap2 event
> >>
> >> I am asking to check whether we should extend uapi struct
> >> bpf_stack_build_id to include build_id_size as well. I guess
> >> we can delay this until a real use case.
> >
> > right, we can try make some MD5 build id binaries and check if it
> > explodes with some bcc tools, but I don't expect that.. I'll try
> > to find some time for that
>
> Thanks. We may have issues on bcc side. For build_id collected in
> kernel, bcc always generates a length-20 string. But for user
> binaries, the build_id string length is equal to actual size of
> the build_id. They may not match (MD5 length is 16).
> The fix is probably to append '0's (up to length 20) for user
> binary build_id's.
>
> I guess MD5 is very seldom used. I will wait if you can reproduce
> the issue and then we might fix it.
Indeed.
Jiri, please check whether md5 is really an issue.
Sounds like we have to do something on the kernel side.
Hopefully zero padding will be enough.
I would prefer to avoid extending uapi struct to cover rare case.
I've applied the series, since this issue sounds orthogonal.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function
2021-01-15 3:47 ` Alexei Starovoitov
@ 2021-01-26 20:52 ` Jiri Olsa
2021-01-26 21:00 ` Yonghong Song
0 siblings, 1 reply; 13+ messages in thread
From: Jiri Olsa @ 2021-01-26 20:52 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Yonghong Song, Jiri Olsa, Arnaldo Carvalho de Melo,
Alexei Starovoitov, Song Liu, lkml, bpf, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Namhyung Kim, Alexander Shishkin,
Michael Petlan, Ian Rogers, Stephane Eranian, Alexei Budankov,
Andi Kleen, Adrian Hunter
On Thu, Jan 14, 2021 at 07:47:20PM -0800, Alexei Starovoitov wrote:
> On Thu, Jan 14, 2021 at 3:44 PM Yonghong Song <yhs@fb.com> wrote:
> >
> >
> >
> > On 1/14/21 2:02 PM, Jiri Olsa wrote:
> > > On Thu, Jan 14, 2021 at 01:05:33PM -0800, Yonghong Song wrote:
> > >>
> > >>
> > >> On 1/14/21 12:01 PM, Jiri Olsa wrote:
> > >>> On Thu, Jan 14, 2021 at 10:56:33AM -0800, Yonghong Song wrote:
> > >>>>
> > >>>>
> > >>>> On 1/14/21 5:40 AM, Jiri Olsa wrote:
> > >>>>> It's possible to have other build id types (other than default SHA1).
> > >>>>> Currently there's also ld support for MD5 build id.
> > >>>>
> > >>>> Currently, bpf build_id based stackmap does not returns the size of
> > >>>> the build_id. Did you see an issue here? I guess user space can check
> > >>>> the length of non-zero bits of the build id to decide what kind of
> > >>>> type it is, right?
> > >>>
> > >>> you can have zero bytes in the build id hash, so you need to get the size
> > >>>
> > >>> I never saw MD5 being used in practise just SHA1, but we added the
> > >>> size to be complete and make sure we'll fit with build id, because
> > >>> there's only limited space in mmap2 event
> > >>
> > >> I am asking to check whether we should extend uapi struct
> > >> bpf_stack_build_id to include build_id_size as well. I guess
> > >> we can delay this until a real use case.
> > >
> > > right, we can try make some MD5 build id binaries and check if it
> > > explodes with some bcc tools, but I don't expect that.. I'll try
> > > to find some time for that
> >
> > Thanks. We may have issues on bcc side. For build_id collected in
> > kernel, bcc always generates a length-20 string. But for user
> > binaries, the build_id string length is equal to actual size of
> > the build_id. They may not match (MD5 length is 16).
> > The fix is probably to append '0's (up to length 20) for user
> > binary build_id's.
> >
> > I guess MD5 is very seldom used. I will wait if you can reproduce
> > the issue and then we might fix it.
>
> Indeed.
> Jiri, please check whether md5 is really an issue.
> Sounds like we have to do something on the kernel side.
> Hopefully zero padding will be enough.
> I would prefer to avoid extending uapi struct to cover rare case.
build_id_parse is already doing the zero padding, so we are ok
I tried several bcc tools over perf bench with md5 buildid and
the results looked ok
jirka
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function
2021-01-26 20:52 ` Jiri Olsa
@ 2021-01-26 21:00 ` Yonghong Song
0 siblings, 0 replies; 13+ messages in thread
From: Yonghong Song @ 2021-01-26 21:00 UTC (permalink / raw)
To: Jiri Olsa, Alexei Starovoitov
Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Alexei Starovoitov,
Song Liu, lkml, bpf, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Namhyung Kim, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian, Alexei Budankov, Andi Kleen, Adrian Hunter
On 1/26/21 12:52 PM, Jiri Olsa wrote:
> On Thu, Jan 14, 2021 at 07:47:20PM -0800, Alexei Starovoitov wrote:
>> On Thu, Jan 14, 2021 at 3:44 PM Yonghong Song <yhs@fb.com> wrote:
>>>
>>>
>>>
>>> On 1/14/21 2:02 PM, Jiri Olsa wrote:
>>>> On Thu, Jan 14, 2021 at 01:05:33PM -0800, Yonghong Song wrote:
>>>>>
>>>>>
>>>>> On 1/14/21 12:01 PM, Jiri Olsa wrote:
>>>>>> On Thu, Jan 14, 2021 at 10:56:33AM -0800, Yonghong Song wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 1/14/21 5:40 AM, Jiri Olsa wrote:
>>>>>>>> It's possible to have other build id types (other than default SHA1).
>>>>>>>> Currently there's also ld support for MD5 build id.
>>>>>>>
>>>>>>> Currently, bpf build_id based stackmap does not returns the size of
>>>>>>> the build_id. Did you see an issue here? I guess user space can check
>>>>>>> the length of non-zero bits of the build id to decide what kind of
>>>>>>> type it is, right?
>>>>>>
>>>>>> you can have zero bytes in the build id hash, so you need to get the size
>>>>>>
>>>>>> I never saw MD5 being used in practise just SHA1, but we added the
>>>>>> size to be complete and make sure we'll fit with build id, because
>>>>>> there's only limited space in mmap2 event
>>>>>
>>>>> I am asking to check whether we should extend uapi struct
>>>>> bpf_stack_build_id to include build_id_size as well. I guess
>>>>> we can delay this until a real use case.
>>>>
>>>> right, we can try make some MD5 build id binaries and check if it
>>>> explodes with some bcc tools, but I don't expect that.. I'll try
>>>> to find some time for that
>>>
>>> Thanks. We may have issues on bcc side. For build_id collected in
>>> kernel, bcc always generates a length-20 string. But for user
>>> binaries, the build_id string length is equal to actual size of
>>> the build_id. They may not match (MD5 length is 16).
>>> The fix is probably to append '0's (up to length 20) for user
>>> binary build_id's.
>>>
>>> I guess MD5 is very seldom used. I will wait if you can reproduce
>>> the issue and then we might fix it.
>>
>> Indeed.
>> Jiri, please check whether md5 is really an issue.
>> Sounds like we have to do something on the kernel side.
>> Hopefully zero padding will be enough.
>> I would prefer to avoid extending uapi struct to cover rare case.
>
> build_id_parse is already doing the zero padding, so we are ok
>
> I tried several bcc tools over perf bench with md5 buildid and
> the results looked ok
Great. Thanks for confirmation!
>
> jirka
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH bpf-next 3/3] perf: Add build id data in mmap2 event
2021-01-14 13:40 [PATCHv7 bpf-next 0/3] perf: Add mmap2 build id support Jiri Olsa
2021-01-14 13:40 ` [PATCH bpf-next 1/3] bpf: Move stack_map_get_build_id into lib Jiri Olsa
2021-01-14 13:40 ` [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function Jiri Olsa
@ 2021-01-14 13:40 ` Jiri Olsa
2021-01-15 3:50 ` [PATCHv7 bpf-next 0/3] perf: Add mmap2 build id support patchwork-bot+netdevbpf
3 siblings, 0 replies; 13+ messages in thread
From: Jiri Olsa @ 2021-01-14 13:40 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, lkml, bpf, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Namhyung Kim, Alexander Shishkin, Michael Petlan,
Song Liu, Ian Rogers, Stephane Eranian, Alexei Budankov,
Andi Kleen, Adrian Hunter
Adding support to carry build id data in mmap2 event.
The build id data replaces maj/min/ino/ino_generation
fields, which are also used to identify map's binary,
so it's ok to replace them with build id data:
union {
struct {
u32 maj;
u32 min;
u64 ino;
u64 ino_generation;
};
struct {
u8 build_id_size;
u8 __reserved_1;
u16 __reserved_2;
u8 build_id[20];
};
};
Replaced maj/min/ino/ino_generation fields give us size
of 24 bytes. We use 20 bytes for build id data, 1 byte
for size and rest is unused.
There's new misc bit for mmap2 to signal there's build
id data in it:
#define PERF_RECORD_MISC_MMAP_BUILD_ID (1 << 14)
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
include/uapi/linux/perf_event.h | 42 +++++++++++++++++++++++++++++----
kernel/events/core.c | 32 +++++++++++++++++++++----
2 files changed, 65 insertions(+), 9 deletions(-)
diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index b15e3447cd9f..cb6f84103560 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -386,7 +386,8 @@ struct perf_event_attr {
aux_output : 1, /* generate AUX records instead of events */
cgroup : 1, /* include cgroup events */
text_poke : 1, /* include text poke events */
- __reserved_1 : 30;
+ build_id : 1, /* use build id in mmap2 events */
+ __reserved_1 : 29;
union {
__u32 wakeup_events; /* wakeup every n events */
@@ -659,6 +660,22 @@ struct perf_event_mmap_page {
__u64 aux_size;
};
+/*
+ * The current state of perf_event_header::misc bits usage:
+ * ('|' used bit, '-' unused bit)
+ *
+ * 012 CDEF
+ * |||---------||||
+ *
+ * Where:
+ * 0-2 CPUMODE_MASK
+ *
+ * C PROC_MAP_PARSE_TIMEOUT
+ * D MMAP_DATA / COMM_EXEC / FORK_EXEC / SWITCH_OUT
+ * E MMAP_BUILD_ID / EXACT_IP / SCHED_OUT_PREEMPT
+ * F (reserved)
+ */
+
#define PERF_RECORD_MISC_CPUMODE_MASK (7 << 0)
#define PERF_RECORD_MISC_CPUMODE_UNKNOWN (0 << 0)
#define PERF_RECORD_MISC_KERNEL (1 << 0)
@@ -690,6 +707,7 @@ struct perf_event_mmap_page {
*
* PERF_RECORD_MISC_EXACT_IP - PERF_RECORD_SAMPLE of precise events
* PERF_RECORD_MISC_SWITCH_OUT_PREEMPT - PERF_RECORD_SWITCH* events
+ * PERF_RECORD_MISC_MMAP_BUILD_ID - PERF_RECORD_MMAP2 event
*
*
* PERF_RECORD_MISC_EXACT_IP:
@@ -699,9 +717,13 @@ struct perf_event_mmap_page {
*
* PERF_RECORD_MISC_SWITCH_OUT_PREEMPT:
* Indicates that thread was preempted in TASK_RUNNING state.
+ *
+ * PERF_RECORD_MISC_MMAP_BUILD_ID:
+ * Indicates that mmap2 event carries build id data.
*/
#define PERF_RECORD_MISC_EXACT_IP (1 << 14)
#define PERF_RECORD_MISC_SWITCH_OUT_PREEMPT (1 << 14)
+#define PERF_RECORD_MISC_MMAP_BUILD_ID (1 << 14)
/*
* Reserve the last bit to indicate some extended misc field
*/
@@ -915,10 +937,20 @@ enum perf_event_type {
* u64 addr;
* u64 len;
* u64 pgoff;
- * u32 maj;
- * u32 min;
- * u64 ino;
- * u64 ino_generation;
+ * union {
+ * struct {
+ * u32 maj;
+ * u32 min;
+ * u64 ino;
+ * u64 ino_generation;
+ * };
+ * struct {
+ * u8 build_id_size;
+ * u8 __reserved_1;
+ * u16 __reserved_2;
+ * u8 build_id[20];
+ * };
+ * };
* u32 prot, flags;
* char filename[];
* struct sample_id sample_id;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 55d18791a72d..c37401e3e5f7 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -53,6 +53,7 @@
#include <linux/min_heap.h>
#include <linux/highmem.h>
#include <linux/pgtable.h>
+#include <linux/buildid.h>
#include "internal.h"
@@ -397,6 +398,7 @@ static atomic_t nr_ksymbol_events __read_mostly;
static atomic_t nr_bpf_events __read_mostly;
static atomic_t nr_cgroup_events __read_mostly;
static atomic_t nr_text_poke_events __read_mostly;
+static atomic_t nr_build_id_events __read_mostly;
static LIST_HEAD(pmus);
static DEFINE_MUTEX(pmus_lock);
@@ -4673,6 +4675,8 @@ static void unaccount_event(struct perf_event *event)
dec = true;
if (event->attr.mmap || event->attr.mmap_data)
atomic_dec(&nr_mmap_events);
+ if (event->attr.build_id)
+ atomic_dec(&nr_build_id_events);
if (event->attr.comm)
atomic_dec(&nr_comm_events);
if (event->attr.namespaces)
@@ -8046,6 +8050,8 @@ struct perf_mmap_event {
u64 ino;
u64 ino_generation;
u32 prot, flags;
+ u8 build_id[BUILD_ID_SIZE_MAX];
+ u32 build_id_size;
struct {
struct perf_event_header header;
@@ -8077,6 +8083,7 @@ static void perf_event_mmap_output(struct perf_event *event,
struct perf_sample_data sample;
int size = mmap_event->event_id.header.size;
u32 type = mmap_event->event_id.header.type;
+ bool use_build_id;
int ret;
if (!perf_event_mmap_match(event, data))
@@ -8101,13 +8108,25 @@ static void perf_event_mmap_output(struct perf_event *event,
mmap_event->event_id.pid = perf_event_pid(event, current);
mmap_event->event_id.tid = perf_event_tid(event, current);
+ use_build_id = event->attr.build_id && mmap_event->build_id_size;
+
+ if (event->attr.mmap2 && use_build_id)
+ mmap_event->event_id.header.misc |= PERF_RECORD_MISC_MMAP_BUILD_ID;
+
perf_output_put(&handle, mmap_event->event_id);
if (event->attr.mmap2) {
- perf_output_put(&handle, mmap_event->maj);
- perf_output_put(&handle, mmap_event->min);
- perf_output_put(&handle, mmap_event->ino);
- perf_output_put(&handle, mmap_event->ino_generation);
+ if (use_build_id) {
+ u8 size[4] = { (u8) mmap_event->build_id_size, 0, 0, 0 };
+
+ __output_copy(&handle, size, 4);
+ __output_copy(&handle, mmap_event->build_id, BUILD_ID_SIZE_MAX);
+ } else {
+ perf_output_put(&handle, mmap_event->maj);
+ perf_output_put(&handle, mmap_event->min);
+ perf_output_put(&handle, mmap_event->ino);
+ perf_output_put(&handle, mmap_event->ino_generation);
+ }
perf_output_put(&handle, mmap_event->prot);
perf_output_put(&handle, mmap_event->flags);
}
@@ -8236,6 +8255,9 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
mmap_event->event_id.header.size = sizeof(mmap_event->event_id) + size;
+ if (atomic_read(&nr_build_id_events))
+ build_id_parse(vma, mmap_event->build_id, &mmap_event->build_id_size);
+
perf_iterate_sb(perf_event_mmap_output,
mmap_event,
NULL);
@@ -11172,6 +11194,8 @@ static void account_event(struct perf_event *event)
inc = true;
if (event->attr.mmap || event->attr.mmap_data)
atomic_inc(&nr_mmap_events);
+ if (event->attr.build_id)
+ atomic_inc(&nr_build_id_events);
if (event->attr.comm)
atomic_inc(&nr_comm_events);
if (event->attr.namespaces)
--
2.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCHv7 bpf-next 0/3] perf: Add mmap2 build id support
2021-01-14 13:40 [PATCHv7 bpf-next 0/3] perf: Add mmap2 build id support Jiri Olsa
` (2 preceding siblings ...)
2021-01-14 13:40 ` [PATCH bpf-next 3/3] perf: Add build id data in mmap2 event Jiri Olsa
@ 2021-01-15 3:50 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 13+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-01-15 3:50 UTC (permalink / raw)
To: Jiri Olsa
Cc: acme, linux-kernel, bpf, a.p.zijlstra, mingo, mark.rutland,
namhyung, alexander.shishkin, mpetlan, songliubraving, irogers,
eranian, abudankov, ak, adrian.hunter
Hello:
This series was applied to bpf/bpf-next.git (refs/heads/master):
On Thu, 14 Jan 2021 14:40:41 +0100 you wrote:
> hi,
> adding the support to have buildid stored in mmap2 event,
> so we can bypass the final perf record hunt on build ids.
>
> This patchset allows perf to record build ID in mmap2 event,
> and adds perf tooling to store/download binaries to .debug
> cache based on these build IDs.
>
> [...]
Here is the summary with links:
- [bpf-next,1/3] bpf: Move stack_map_get_build_id into lib
https://git.kernel.org/bpf/bpf-next/c/bd7525dacd7e
- [bpf-next,2/3] bpf: Add size arg to build_id_parse function
https://git.kernel.org/bpf/bpf-next/c/921f88fc8919
- [bpf-next,3/3] perf: Add build id data in mmap2 event
https://git.kernel.org/bpf/bpf-next/c/88a16a130933
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 13+ messages in thread