[PATCHv7 bpf-next 0/3] perf: Add mmap2 build id support

[PATCHv7 bpf-next 0/3] perf: Add mmap2 build id support

[Jiri Olsa]

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.

Note that the build id retrieval code is stolen from bpf
code, where it's been used (together with file offsets)
to replace IPs in user space stack traces. It's now added
under lib directory.

v7 changes:
  - included only missing kernel patches, cc-ed bpf@vger and
    rebased on bpf-next/master [Alexei]

v6 changes:
  - last 4 patches rebased Arnaldo's perf/core

v5 changes:
  - rebased on latest perf/core
  - several patches already pulled in
  - fixed trace+probe_vfs_getname.sh output redirection
  - fixed changelogs [Arnaldo]
  - renamed BUILD_ID_SIZE to BUILD_ID_SIZE_MAX [Song]

v4 changes:
  - fixed typo in changelog [Namhyung]
  - removed force_download bool from struct dso_store_data,
    because it's not used  [Namhyung]

v3 changes:
  - added acks
  - removed forgotten debug code [Arnaldo]
  - fixed readlink termination [Ian]
  - fixed doc for --debuginfod=URLs [Ian]
  - adopted kernel's memchr_inv function and used
    it in build_id__is_defined function [Arnaldo]

On recording server:

  - on the recording server we can run record with --buildid-mmap
    option to store build ids in mmap2 events:

    # perf record --buildid-mmap
    ^C[ perf record: Woken up 2 times to write data ]
    [ perf record: Captured and wrote 0.836 MB perf.data ]

  - it stores nothing to ~/.debug cache:

    # find ~/.debug
    find: ‘/root/.debug’: No such file or directory

  - and still reports properly:

    # perf report --stdio
    ...
    99.82%  swapper          [kernel.kallsyms]  [k] native_safe_halt
     0.03%  swapper          [kernel.kallsyms]  [k] finish_task_switch
     0.02%  swapper          [kernel.kallsyms]  [k] __softirqentry_text_start
     0.01%  kcompactd0       [kernel.kallsyms]  [k] _raw_spin_unlock_irqrestore
     0.01%  ksoftirqd/6      [kernel.kallsyms]  [k] slab_free_freelist_hook
     0.01%  kworker/17:1H-x  [kernel.kallsyms]  [k] slab_free_freelist_hook

  - display used/hit build ids:

    # perf buildid-list | head -5
    5dcec522abf136fcfd3128f47e131f2365834dd7 /proc/kcore
    589e403a34f55486bcac848a45e00bcdeedd1ca8 /usr/lib64/libcrypto.so.1.1.1g
    94569566d4eac7e9c87ba029d43d4e2158f9527e /usr/lib64/libpthread-2.30.so
    559b9702bebe31c6d132c8dc5cc887673d65d5b5 /usr/lib64/libc-2.30.so
    40da7abe89f631f60538a17686a7d65c6a02ed31 /usr/lib64/ld-2.30.so

  - store build id binaries into build id cache:

    # perf buildid-cache -a perf.data
    OK   5dcec522abf136fcfd3128f47e131f2365834dd7 /proc/kcore
    OK   589e403a34f55486bcac848a45e00bcdeedd1ca8 /usr/lib64/libcrypto.so.1.1.1g
    OK   94569566d4eac7e9c87ba029d43d4e2158f9527e /usr/lib64/libpthread-2.30.so
    OK   559b9702bebe31c6d132c8dc5cc887673d65d5b5 /usr/lib64/libc-2.30.so
    OK   40da7abe89f631f60538a17686a7d65c6a02ed31 /usr/lib64/ld-2.30.so
    OK   a674f7a47c78e35a088104647b9640710277b489 /usr/sbin/sshd
    OK   e5cb4ca25f46485bdbc691c3a92e7e111dac3ef2 /usr/bin/bash
    OK   9bc8589108223c944b452f0819298a0c3cba6215 /usr/bin/find

    # find ~/.debug | head -5
    /root/.debug
    /root/.debug/proc
    /root/.debug/proc/kcore
    /root/.debug/proc/kcore/5dcec522abf136fcfd3128f47e131f2365834dd7
    /root/.debug/proc/kcore/5dcec522abf136fcfd3128f47e131f2365834dd7/kallsyms

  - run debuginfod daemon to provide binaries to another server (below)
    (the initialization could take some time)

    # debuginfod -F /


On another server:

  - copy perf.data from 'record' server and run:

    $ find ~/.debug/
    find: ‘/home/jolsa/.debug/’: No such file or directory

    $ perf buildid-list | head -5
    No kallsyms or vmlinux with build-id 5dcec522abf136fcfd3128f47e131f2365834dd7 was found
    5dcec522abf136fcfd3128f47e131f2365834dd7 [kernel.kallsyms]
    5784f813b727a50cfd3363234aef9fcbab685cc4 /lib/modules/5.10.0-rc2speed+/kernel/fs/xfs/xfs.ko
    589e403a34f55486bcac848a45e00bcdeedd1ca8 /usr/lib64/libcrypto.so.1.1.1g
    94569566d4eac7e9c87ba029d43d4e2158f9527e /usr/lib64/libpthread-2.30.so
    559b9702bebe31c6d132c8dc5cc887673d65d5b5 /usr/lib64/libc-2.30.so

  - report does not show anything (kernel build id does not match):

   $ perf report --stdio
   ...
    76.73%  swapper          [kernel.kallsyms]    [k] 0xffffffff81aa8ebe
     1.89%  find             [kernel.kallsyms]    [k] 0xffffffff810f2167
     0.93%  sshd             [kernel.kallsyms]    [k] 0xffffffff8153380c
     0.83%  swapper          [kernel.kallsyms]    [k] 0xffffffff81104b0b
     0.71%  kworker/u40:2-e  [kernel.kallsyms]    [k] 0xffffffff810f3850
     0.70%  kworker/u40:0-e  [kernel.kallsyms]    [k] 0xffffffff810f3850
     0.64%  find             [kernel.kallsyms]    [k] 0xffffffff81a9ba0a
     0.63%  find             [kernel.kallsyms]    [k] 0xffffffff81aa93b0

  - add build ids does not work, because existing binaries (on another server)
    have different build ids:

    $ perf buildid-cache -a perf.data 
    No kallsyms or vmlinux with build-id 5dcec522abf136fcfd3128f47e131f2365834dd7 was found
    FAIL 5dcec522abf136fcfd3128f47e131f2365834dd7 [kernel.kallsyms]
    FAIL 5784f813b727a50cfd3363234aef9fcbab685cc4 /lib/modules/5.10.0-rc2speed+/kernel/fs/xfs/xfs.ko
    FAIL 589e403a34f55486bcac848a45e00bcdeedd1ca8 /usr/lib64/libcrypto.so.1.1.1g
    FAIL 94569566d4eac7e9c87ba029d43d4e2158f9527e /usr/lib64/libpthread-2.30.so
    FAIL 559b9702bebe31c6d132c8dc5cc887673d65d5b5 /usr/lib64/libc-2.30.so
    FAIL 40da7abe89f631f60538a17686a7d65c6a02ed31 /usr/lib64/ld-2.30.so
    FAIL a674f7a47c78e35a088104647b9640710277b489 /usr/sbin/sshd
    FAIL e5cb4ca25f46485bdbc691c3a92e7e111dac3ef2 /usr/bin/bash
    FAIL 9bc8589108223c944b452f0819298a0c3cba6215 /usr/bin/find

  - add build ids with debuginfod setup pointing to record server:

    $ perf buildid-cache -a perf.data --debuginfod http://192.168.122.174:8002
    No kallsyms or vmlinux with build-id 5dcec522abf136fcfd3128f47e131f2365834dd7 was found
    OK   5dcec522abf136fcfd3128f47e131f2365834dd7 [kernel.kallsyms]
    OK   5784f813b727a50cfd3363234aef9fcbab685cc4 /lib/modules/5.10.0-rc2speed+/kernel/fs/xfs/xfs.ko
    OK   589e403a34f55486bcac848a45e00bcdeedd1ca8 /usr/lib64/libcrypto.so.1.1.1g
    OK   94569566d4eac7e9c87ba029d43d4e2158f9527e /usr/lib64/libpthread-2.30.so
    OK   559b9702bebe31c6d132c8dc5cc887673d65d5b5 /usr/lib64/libc-2.30.so
    OK   40da7abe89f631f60538a17686a7d65c6a02ed31 /usr/lib64/ld-2.30.so
    OK   a674f7a47c78e35a088104647b9640710277b489 /usr/sbin/sshd
    OK   e5cb4ca25f46485bdbc691c3a92e7e111dac3ef2 /usr/bin/bash
    OK   9bc8589108223c944b452f0819298a0c3cba6215 /usr/bin/find

  - and report works:

    $ perf report --stdio
    ...
    76.73%  swapper          [kernel.kallsyms]    [k] native_safe_halt
     1.91%  find             [kernel.kallsyms]    [k] queue_work_on
     0.93%  sshd             [kernel.kallsyms]    [k] iowrite16
     0.83%  swapper          [kernel.kallsyms]    [k] finish_task_switch
     0.72%  kworker/u40:2-e  [kernel.kallsyms]    [k] process_one_work
     0.70%  kworker/u40:0-e  [kernel.kallsyms]    [k] process_one_work
     0.64%  find             [kernel.kallsyms]    [k] syscall_enter_from_user_mode
     0.63%  find             [kernel.kallsyms]    [k] _raw_spin_unlock_irqrestore

  - because we have the data in build id cache:

    $ find ~/.debug | head -10
    .../.debug
    .../.debug/home
    .../.debug/home/jolsa
    .../.debug/home/jolsa/.cache
    .../.debug/home/jolsa/.cache/debuginfod_client
    .../.debug/home/jolsa/.cache/debuginfod_client/5dcec522abf136fcfd3128f47e131f2365834dd7
    .../.debug/home/jolsa/.cache/debuginfod_client/5dcec522abf136fcfd3128f47e131f2365834dd7/executable
    .../.debug/home/jolsa/.cache/debuginfod_client/5dcec522abf136fcfd3128f47e131f2365834dd7/executable/5dcec522abf136fcfd3128f47e131f2365834dd7
    .../.debug/home/jolsa/.cache/debuginfod_client/5dcec522abf136fcfd3128f47e131f2365834dd7/executable/5dcec522abf136fcfd3128f47e131f2365834dd7/elf
    .../.debug/home/jolsa/.cache/debuginfod_client/5dcec522abf136fcfd3128f47e131f2365834dd7/executable/5dcec522abf136fcfd3128f47e131f2365834dd7/debug


Available also in:
  git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
  perf/build_id

thanks,
jirka


---
Jiri Olsa (3):
      bpf: Move stack_map_get_build_id into lib
      bpf: Add size arg to build_id_parse function
      perf: Add build id data in mmap2 event

 include/linux/buildid.h         |  12 ++++++++++
 include/uapi/linux/perf_event.h |  42 ++++++++++++++++++++++++++++++----
 kernel/bpf/stackmap.c           | 143 ++++--------------------------------------------------------------------------------------------------------------
 kernel/events/core.c            |  32 ++++++++++++++++++++++----
 lib/Makefile                    |   3 ++-
 lib/buildid.c                   | 149 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 232 insertions(+), 149 deletions(-)
 create mode 100644 include/linux/buildid.h
 create mode 100644 lib/buildid.c

[PATCH bpf-next 1/3] bpf: Move stack_map_get_build_id into lib

[Jiri Olsa]

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

[PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function

[Jiri Olsa]

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

[PATCH bpf-next 3/3] perf: Add build id data in mmap2 event

[Jiri Olsa]

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

Re: [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function

[Yonghong Song]

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,
>   }
>   
[...]

Re: [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function

[Jiri Olsa]

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,
> >   }
> [...]
> 

Re: [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function

[Yonghong Song]

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,
>>>    }
>> [...]
>>
> 

Re: [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function

[Jiri Olsa]

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

Re: [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function

[Yonghong Song]

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
> 

Re: [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function

[Alexei Starovoitov]

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.

Re: [PATCHv7 bpf-next 0/3] perf: Add mmap2 build id support

[patchwork-bot+netdevbpf]

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

Re: [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function

[Jiri Olsa]

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

Re: [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function

[Yonghong Song]

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
>