[PATCH v2 0/7] Various asan and test fixes

[PATCH v2 0/7] Various asan and test fixes

[Ian Rogers]

When testing removing perf_env with asan I noticed a number of test
failures either because of asan issues or because of building with
NO_LIBELF=1 and NO_LIBBPF=1 to avoid false memory leaks. Address these
issues so that real test failures stand out.

v2: Fix wrong elf size use in filename__read_build_id fix. Add
    thread__comm_lock fix.

Ian Rogers (7):
  perf symbol: Fix use-after-free in filename__read_build_id
  perf test demangle-java: Don't segv if demangling fails
  perf symbol: Move demangling code out of symbol-elf.c
  perf intel-tpebs: Avoid race when evlist is being deleted
  perf test intel-pt: Skip jitdump test if no libelf
  perf test trace_summary: Skip --bpf-summary tests if no libbpf
  perf thread: Avoid recursively taking thread__comm_lock

 tools/perf/tests/demangle-java-test.c   |   5 +
 tools/perf/tests/shell/test_intel_pt.sh |   5 +
 tools/perf/tests/shell/trace_summary.sh |   6 +
 tools/perf/util/demangle-cxx.h          |   2 +
 tools/perf/util/intel-tpebs.c           |  12 +-
 tools/perf/util/symbol-elf.c            |  86 ------------
 tools/perf/util/symbol-minimal.c        | 175 ++++++++++--------------
 tools/perf/util/symbol.c                |  87 ++++++++++++
 tools/perf/util/thread.c                |  17 ++-
 9 files changed, 198 insertions(+), 197 deletions(-)

-- 
2.49.0.1238.gf8c92423fb-goog

[PATCH v2 1/7] perf symbol: Fix use-after-free in filename__read_build_id

[Ian Rogers]

The same buf is used for the program headers and reading notes. As the
notes memory may be reallocated then this corrupts the memory pointed
to by the phdr. Using the same buffer is in any case a logic
error. Rather than deal with the duplicated code, introduce an elf32
boolean and a union for either the elf32 or elf64 headers that are in
use. Let the program headers have their own memory and grow the buffer
for notes as necessary.

Before `perf list -j` compiled with asan would crash with:
```
==4176189==ERROR: AddressSanitizer: heap-use-after-free on address 0x5160000070b8 at pc 0x555d3b15075b bp 0x7ffebb5a8090 sp 0x7ffebb5a8088
READ of size 8 at 0x5160000070b8 thread T0
    #0 0x555d3b15075a in filename__read_build_id tools/perf/util/symbol-minimal.c:212:25
    #1 0x555d3ae43aff in filename__sprintf_build_id tools/perf/util/build-id.c:110:8
...

0x5160000070b8 is located 312 bytes inside of 560-byte region [0x516000006f80,0x5160000071b0)
freed by thread T0 here:
    #0 0x555d3ab21840 in realloc (perf+0x264840) (BuildId: 12dff2f6629f738e5012abdf0e90055518e70b5e)
    #1 0x555d3b1506e7 in filename__read_build_id tools/perf/util/symbol-minimal.c:206:11
...

previously allocated by thread T0 here:
    #0 0x555d3ab21423 in malloc (perf+0x264423) (BuildId: 12dff2f6629f738e5012abdf0e90055518e70b5e)
    #1 0x555d3b1503a2 in filename__read_build_id tools/perf/util/symbol-minimal.c:182:9
...
```

Note: this bug is long standing and not introduced by the other asan
fix in commit fa9c4977fbfb ("perf symbol-minimal: Fix double free in
filename__read_build_id").

Fixes: b691f64360ecec49 ("perf symbols: Implement poor man's ELF parser")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/symbol-minimal.c | 168 +++++++++++++------------------
 1 file changed, 70 insertions(+), 98 deletions(-)

diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
index d8da3da01fe6..36c1d3090689 100644
--- a/tools/perf/util/symbol-minimal.c
+++ b/tools/perf/util/symbol-minimal.c
@@ -90,11 +90,23 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
 {
 	FILE *fp;
 	int ret = -1;
-	bool need_swap = false;
+	bool need_swap = false, elf32;
 	u8 e_ident[EI_NIDENT];
-	size_t buf_size;
-	void *buf;
 	int i;
+	union {
+		struct {
+			Elf32_Ehdr ehdr32;
+			Elf32_Phdr *phdr32;
+		};
+		struct {
+			Elf64_Ehdr ehdr64;
+			Elf64_Phdr *phdr64;
+		};
+	} hdrs;
+	void *phdr;
+	size_t phdr_size;
+	void *buf = NULL;
+	size_t buf_size = 0;
 
 	fp = fopen(filename, "r");
 	if (fp == NULL)
@@ -108,119 +120,79 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
 		goto out;
 
 	need_swap = check_need_swap(e_ident[EI_DATA]);
+	elf32 = e_ident[EI_CLASS] == ELFCLASS32;
 
-	/* for simplicity */
-	fseek(fp, 0, SEEK_SET);
-
-	if (e_ident[EI_CLASS] == ELFCLASS32) {
-		Elf32_Ehdr ehdr;
-		Elf32_Phdr *phdr;
-
-		if (fread(&ehdr, sizeof(ehdr), 1, fp) != 1)
-			goto out;
+	if (fread(elf32 ? (void *)&hdrs.ehdr32 : (void *)&hdrs.ehdr64,
+		  elf32 ? sizeof(hdrs.ehdr32) : sizeof(hdrs.ehdr64),
+		  1, fp) != 1)
+		goto out;
 
-		if (need_swap) {
-			ehdr.e_phoff = bswap_32(ehdr.e_phoff);
-			ehdr.e_phentsize = bswap_16(ehdr.e_phentsize);
-			ehdr.e_phnum = bswap_16(ehdr.e_phnum);
+	if (need_swap) {
+		if (elf32) {
+			hdrs.ehdr32.e_phoff = bswap_32(hdrs.ehdr32.e_phoff);
+			hdrs.ehdr32.e_phentsize = bswap_16(hdrs.ehdr32.e_phentsize);
+			hdrs.ehdr32.e_phnum = bswap_16(hdrs.ehdr32.e_phnum);
+		} else {
+			hdrs.ehdr64.e_phoff = bswap_64(hdrs.ehdr64.e_phoff);
+			hdrs.ehdr64.e_phentsize = bswap_16(hdrs.ehdr64.e_phentsize);
+			hdrs.ehdr64.e_phnum = bswap_16(hdrs.ehdr64.e_phnum);
 		}
+	}
+	phdr_size = elf32 ? hdrs.ehdr32.e_phentsize * hdrs.ehdr32.e_phnum
+			  : hdrs.ehdr64.e_phentsize * hdrs.ehdr64.e_phnum;
+	phdr = malloc(phdr_size);
+	if (phdr == NULL)
+		goto out;
 
-		buf_size = ehdr.e_phentsize * ehdr.e_phnum;
-		buf = malloc(buf_size);
-		if (buf == NULL)
-			goto out;
-
-		fseek(fp, ehdr.e_phoff, SEEK_SET);
-		if (fread(buf, buf_size, 1, fp) != 1)
-			goto out_free;
-
-		for (i = 0, phdr = buf; i < ehdr.e_phnum; i++, phdr++) {
-			void *tmp;
-			long offset;
-
-			if (need_swap) {
-				phdr->p_type = bswap_32(phdr->p_type);
-				phdr->p_offset = bswap_32(phdr->p_offset);
-				phdr->p_filesz = bswap_32(phdr->p_filesz);
-			}
-
-			if (phdr->p_type != PT_NOTE)
-				continue;
-
-			offset = phdr->p_offset;
-			if (phdr->p_filesz > buf_size) {
-				buf_size = phdr->p_filesz;
-				tmp = realloc(buf, buf_size);
-				if (tmp == NULL)
-					goto out_free;
-				buf = tmp;
-			}
-			fseek(fp, offset, SEEK_SET);
-			if (fread(buf, phdr->p_filesz, 1, fp) != 1)
-				goto out_free;
+	fseek(fp, elf32 ? hdrs.ehdr32.e_phoff : hdrs.ehdr64.e_phoff, SEEK_SET);
+	if (fread(phdr, phdr_size, 1, fp) != 1)
+		goto out_free;
 
-			ret = read_build_id(buf, phdr->p_filesz, bid, need_swap);
-			if (ret == 0) {
-				ret = bid->size;
-				break;
-			}
-		}
-	} else {
-		Elf64_Ehdr ehdr;
-		Elf64_Phdr *phdr;
+	if (elf32)
+		hdrs.phdr32 = phdr;
+	else
+		hdrs.phdr64 = phdr;
 
-		if (fread(&ehdr, sizeof(ehdr), 1, fp) != 1)
-			goto out;
+	for (i = 0; i < elf32 ? hdrs.ehdr32.e_phnum : hdrs.ehdr64.e_phnum; i++) {
+		size_t p_filesz;
 
 		if (need_swap) {
-			ehdr.e_phoff = bswap_64(ehdr.e_phoff);
-			ehdr.e_phentsize = bswap_16(ehdr.e_phentsize);
-			ehdr.e_phnum = bswap_16(ehdr.e_phnum);
+			if (elf32) {
+				hdrs.phdr32[i].p_type = bswap_32(hdrs.phdr32[i].p_type);
+				hdrs.phdr32[i].p_offset = bswap_32(hdrs.phdr32[i].p_offset);
+				hdrs.phdr32[i].p_filesz = bswap_32(hdrs.phdr32[i].p_offset);
+			} else {
+				hdrs.phdr64[i].p_type = bswap_32(hdrs.phdr64[i].p_type);
+				hdrs.phdr64[i].p_offset = bswap_64(hdrs.phdr64[i].p_offset);
+				hdrs.phdr64[i].p_filesz = bswap_64(hdrs.phdr64[i].p_filesz);
+			}
 		}
+		if ((elf32 ? hdrs.phdr32[i].p_type : hdrs.phdr64[i].p_type) != PT_NOTE)
+			continue;
 
-		buf_size = ehdr.e_phentsize * ehdr.e_phnum;
-		buf = malloc(buf_size);
-		if (buf == NULL)
-			goto out;
-
-		fseek(fp, ehdr.e_phoff, SEEK_SET);
-		if (fread(buf, buf_size, 1, fp) != 1)
-			goto out_free;
-
-		for (i = 0, phdr = buf; i < ehdr.e_phnum; i++, phdr++) {
+		p_filesz = elf32 ? hdrs.phdr32[i].p_filesz : hdrs.phdr64[i].p_filesz;
+		if (p_filesz > buf_size) {
 			void *tmp;
-			long offset;
-
-			if (need_swap) {
-				phdr->p_type = bswap_32(phdr->p_type);
-				phdr->p_offset = bswap_64(phdr->p_offset);
-				phdr->p_filesz = bswap_64(phdr->p_filesz);
-			}
-
-			if (phdr->p_type != PT_NOTE)
-				continue;
 
-			offset = phdr->p_offset;
-			if (phdr->p_filesz > buf_size) {
-				buf_size = phdr->p_filesz;
-				tmp = realloc(buf, buf_size);
-				if (tmp == NULL)
-					goto out_free;
-				buf = tmp;
-			}
-			fseek(fp, offset, SEEK_SET);
-			if (fread(buf, phdr->p_filesz, 1, fp) != 1)
+			buf_size = p_filesz;
+			tmp = realloc(buf, buf_size);
+			if (tmp == NULL)
 				goto out_free;
+			buf = tmp;
+		}
+		fseek(fp, elf32 ? hdrs.phdr32[i].p_offset : hdrs.phdr64[i].p_offset, SEEK_SET);
+		if (fread(buf, p_filesz, 1, fp) != 1)
+			goto out_free;
 
-			ret = read_build_id(buf, phdr->p_filesz, bid, need_swap);
-			if (ret == 0) {
-				ret = bid->size;
-				break;
-			}
+		ret = read_build_id(buf, p_filesz, bid, need_swap);
+		if (ret == 0) {
+			ret = bid->size;
+			break;
 		}
 	}
 out_free:
 	free(buf);
+	free(phdr);
 out:
 	fclose(fp);
 	return ret;
-- 
2.49.0.1238.gf8c92423fb-goog

[PATCH v2 2/7] perf test demangle-java: Don't segv if demangling fails

[Ian Rogers]

The buffer returned by dso__demangle_sym may be NULL, don't segv in
strcmp if this happens. Currently this happens for NO_LIBELF=1 builds.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/demangle-java-test.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/perf/tests/demangle-java-test.c b/tools/perf/tests/demangle-java-test.c
index ebaf60cdfa99..0fb3e5a4a0ed 100644
--- a/tools/perf/tests/demangle-java-test.c
+++ b/tools/perf/tests/demangle-java-test.c
@@ -30,6 +30,11 @@ static int test__demangle_java(struct test_suite *test __maybe_unused, int subte
 
 	for (i = 0; i < ARRAY_SIZE(test_cases); i++) {
 		buf = dso__demangle_sym(/*dso=*/NULL, /*kmodule=*/0, test_cases[i].mangled);
+		if (!buf) {
+			pr_debug("FAILED to demangle: \"%s\"\n \"%s\"\n", test_cases[i].mangled,
+				 test_cases[i].demangled);
+			continue;
+		}
 		if (strcmp(buf, test_cases[i].demangled)) {
 			pr_debug("FAILED: %s: %s != %s\n", test_cases[i].mangled,
 				 buf, test_cases[i].demangled);
-- 
2.49.0.1238.gf8c92423fb-goog

[PATCH v2 3/7] perf symbol: Move demangling code out of symbol-elf.c

[Ian Rogers]

symbol-elf.c is used when building with libelf, symbol-minimal is used
otherwise. There is no reason the demangling code with no dependencies
on libelf is part of symbol-elf.c so move to symbol.c. This allows
demangling tests to pass with NO_LIBELF=1.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/demangle-cxx.h   |  2 +
 tools/perf/util/symbol-elf.c     | 86 -------------------------------
 tools/perf/util/symbol-minimal.c |  7 ---
 tools/perf/util/symbol.c         | 87 ++++++++++++++++++++++++++++++++
 4 files changed, 89 insertions(+), 93 deletions(-)

diff --git a/tools/perf/util/demangle-cxx.h b/tools/perf/util/demangle-cxx.h
index 26b5b66c0b4e..9359937a881a 100644
--- a/tools/perf/util/demangle-cxx.h
+++ b/tools/perf/util/demangle-cxx.h
@@ -2,6 +2,8 @@
 #ifndef __PERF_DEMANGLE_CXX
 #define __PERF_DEMANGLE_CXX 1
 
+#include <stdbool.h>
+
 #ifdef __cplusplus
 extern "C" {
 #endif
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 8734e8b6cf84..60f37f149a87 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -13,17 +13,12 @@
 #include "maps.h"
 #include "symbol.h"
 #include "symsrc.h"
-#include "demangle-cxx.h"
-#include "demangle-ocaml.h"
-#include "demangle-java.h"
-#include "demangle-rust-v0.h"
 #include "machine.h"
 #include "vdso.h"
 #include "debug.h"
 #include "util/copyfile.h"
 #include <linux/ctype.h>
 #include <linux/kernel.h>
-#include <linux/log2.h>
 #include <linux/zalloc.h>
 #include <linux/string.h>
 #include <symbol/kallsyms.h>
@@ -280,82 +275,6 @@ static int elf_read_program_header(Elf *elf, u64 vaddr, GElf_Phdr *phdr)
 	return -1;
 }
 
-static bool want_demangle(bool is_kernel_sym)
-{
-	return is_kernel_sym ? symbol_conf.demangle_kernel : symbol_conf.demangle;
-}
-
-/*
- * Demangle C++ function signature, typically replaced by demangle-cxx.cpp
- * version.
- */
-#ifndef HAVE_CXA_DEMANGLE_SUPPORT
-char *cxx_demangle_sym(const char *str __maybe_unused, bool params __maybe_unused,
-		       bool modifiers __maybe_unused)
-{
-#ifdef HAVE_LIBBFD_SUPPORT
-	int flags = (params ? DMGL_PARAMS : 0) | (modifiers ? DMGL_ANSI : 0);
-
-	return bfd_demangle(NULL, str, flags);
-#elif defined(HAVE_CPLUS_DEMANGLE_SUPPORT)
-	int flags = (params ? DMGL_PARAMS : 0) | (modifiers ? DMGL_ANSI : 0);
-
-	return cplus_demangle(str, flags);
-#else
-	return NULL;
-#endif
-}
-#endif /* !HAVE_CXA_DEMANGLE_SUPPORT */
-
-static char *demangle_sym(struct dso *dso, int kmodule, const char *elf_name)
-{
-	struct demangle rust_demangle = {
-		.style = DemangleStyleUnknown,
-	};
-	char *demangled = NULL;
-
-	/*
-	 * We need to figure out if the object was created from C++ sources
-	 * DWARF DW_compile_unit has this, but we don't always have access
-	 * to it...
-	 */
-	if (!want_demangle((dso && dso__kernel(dso)) || kmodule))
-		return demangled;
-
-	rust_demangle_demangle(elf_name, &rust_demangle);
-	if (rust_demangle_is_known(&rust_demangle)) {
-		/* A rust mangled name. */
-		if (rust_demangle.mangled_len == 0)
-			return demangled;
-
-		for (size_t buf_len = roundup_pow_of_two(rust_demangle.mangled_len * 2);
-		     buf_len < 1024 * 1024; buf_len += 32) {
-			char *tmp = realloc(demangled, buf_len);
-
-			if (!tmp) {
-				/* Failure to grow output buffer, return what is there. */
-				return demangled;
-			}
-			demangled = tmp;
-			if (rust_demangle_display_demangle(&rust_demangle, demangled, buf_len,
-							   /*alternate=*/true) == OverflowOk)
-				return demangled;
-		}
-		/* Buffer exceeded sensible bounds, return what is there. */
-		return demangled;
-	}
-
-	demangled = cxx_demangle_sym(elf_name, verbose > 0, verbose > 0);
-	if (demangled)
-		return demangled;
-
-	demangled = ocaml_demangle_sym(elf_name);
-	if (demangled)
-		return demangled;
-
-	return java_demangle_sym(elf_name, JAVA_DEMANGLE_NORET);
-}
-
 struct rel_info {
 	u32		nr_entries;
 	u32		*sorted;
@@ -868,11 +787,6 @@ int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss)
 	return 0;
 }
 
-char *dso__demangle_sym(struct dso *dso, int kmodule, const char *elf_name)
-{
-	return demangle_sym(dso, kmodule, elf_name);
-}
-
 /*
  * Align offset to 4 bytes as needed for note name and descriptor data.
  */
diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
index 36c1d3090689..c73fe2e09fe9 100644
--- a/tools/perf/util/symbol-minimal.c
+++ b/tools/perf/util/symbol-minimal.c
@@ -355,13 +355,6 @@ void symbol__elf_init(void)
 {
 }
 
-char *dso__demangle_sym(struct dso *dso __maybe_unused,
-			int kmodule __maybe_unused,
-			const char *elf_name __maybe_unused)
-{
-	return NULL;
-}
-
 bool filename__has_section(const char *filename __maybe_unused, const char *sec __maybe_unused)
 {
 	return false;
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index fe801880afea..9d712416f0b6 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -19,6 +19,11 @@
 #include "build-id.h"
 #include "cap.h"
 #include "cpumap.h"
+#include "debug.h"
+#include "demangle-cxx.h"
+#include "demangle-java.h"
+#include "demangle-ocaml.h"
+#include "demangle-rust-v0.h"
 #include "dso.h"
 #include "util.h" // lsdir()
 #include "debug.h"
@@ -36,6 +41,7 @@
 #include "header.h"
 #include "path.h"
 #include <linux/ctype.h>
+#include <linux/log2.h>
 #include <linux/zalloc.h>
 
 #include <elf.h>
@@ -2648,3 +2654,84 @@ int symbol__validate_sym_arguments(void)
 	}
 	return 0;
 }
+
+static bool want_demangle(bool is_kernel_sym)
+{
+	return is_kernel_sym ? symbol_conf.demangle_kernel : symbol_conf.demangle;
+}
+
+/*
+ * Demangle C++ function signature, typically replaced by demangle-cxx.cpp
+ * version.
+ */
+#ifndef HAVE_CXA_DEMANGLE_SUPPORT
+char *cxx_demangle_sym(const char *str __maybe_unused, bool params __maybe_unused,
+		       bool modifiers __maybe_unused)
+{
+#ifdef HAVE_LIBBFD_SUPPORT
+	int flags = (params ? DMGL_PARAMS : 0) | (modifiers ? DMGL_ANSI : 0);
+
+	return bfd_demangle(NULL, str, flags);
+#elif defined(HAVE_CPLUS_DEMANGLE_SUPPORT)
+	int flags = (params ? DMGL_PARAMS : 0) | (modifiers ? DMGL_ANSI : 0);
+
+	return cplus_demangle(str, flags);
+#else
+	return NULL;
+#endif
+}
+#endif /* !HAVE_CXA_DEMANGLE_SUPPORT */
+
+static char *demangle_sym(struct dso *dso, int kmodule, const char *elf_name)
+{
+	struct demangle rust_demangle = {
+		.style = DemangleStyleUnknown,
+	};
+	char *demangled = NULL;
+
+	/*
+	 * We need to figure out if the object was created from C++ sources
+	 * DWARF DW_compile_unit has this, but we don't always have access
+	 * to it...
+	 */
+	if (!want_demangle((dso && dso__kernel(dso)) || kmodule))
+		return demangled;
+
+	rust_demangle_demangle(elf_name, &rust_demangle);
+	if (rust_demangle_is_known(&rust_demangle)) {
+		/* A rust mangled name. */
+		if (rust_demangle.mangled_len == 0)
+			return demangled;
+
+		for (size_t buf_len = roundup_pow_of_two(rust_demangle.mangled_len * 2);
+		     buf_len < 1024 * 1024; buf_len += 32) {
+			char *tmp = realloc(demangled, buf_len);
+
+			if (!tmp) {
+				/* Failure to grow output buffer, return what is there. */
+				return demangled;
+			}
+			demangled = tmp;
+			if (rust_demangle_display_demangle(&rust_demangle, demangled, buf_len,
+							   /*alternate=*/true) == OverflowOk)
+				return demangled;
+		}
+		/* Buffer exceeded sensible bounds, return what is there. */
+		return demangled;
+	}
+
+	demangled = cxx_demangle_sym(elf_name, verbose > 0, verbose > 0);
+	if (demangled)
+		return demangled;
+
+	demangled = ocaml_demangle_sym(elf_name);
+	if (demangled)
+		return demangled;
+
+	return java_demangle_sym(elf_name, JAVA_DEMANGLE_NORET);
+}
+
+char *dso__demangle_sym(struct dso *dso, int kmodule, const char *elf_name)
+{
+	return demangle_sym(dso, kmodule, elf_name);
+}
-- 
2.49.0.1238.gf8c92423fb-goog

[PATCH v2 4/7] perf intel-tpebs: Avoid race when evlist is being deleted

[Ian Rogers]

Reading through the evsel->evlist may seg fault if a sample arrives
when the evlist is being deleted. Detect this case and ignore samples
arriving when the evlist is being deleted.

Fixes: bcfab08db7fb ("perf intel-tpebs: Filter non-workload samples")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/intel-tpebs.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
index 4ad4bc118ea5..3b92ebf5c112 100644
--- a/tools/perf/util/intel-tpebs.c
+++ b/tools/perf/util/intel-tpebs.c
@@ -162,9 +162,17 @@ static bool is_child_pid(pid_t parent, pid_t child)
 
 static bool should_ignore_sample(const struct perf_sample *sample, const struct tpebs_retire_lat *t)
 {
-	pid_t workload_pid = t->evsel->evlist->workload.pid;
-	pid_t sample_pid = sample->pid;
+	pid_t workload_pid, sample_pid = sample->pid;
 
+	/*
+	 * During evlist__purge the evlist will be removed prior to the
+	 * evsel__exit calling evsel__tpebs_close and taking the
+	 * tpebs_mtx. Avoid a segfault by ignoring samples in this case.
+	 */
+	if (t->evsel->evlist == NULL)
+		return true;
+
+	workload_pid = t->evsel->evlist->workload.pid;
 	if (workload_pid < 0 || workload_pid == sample_pid)
 		return false;
 
-- 
2.49.0.1238.gf8c92423fb-goog

[PATCH v2 5/7] perf test intel-pt: Skip jitdump test if no libelf

[Ian Rogers]

jitdump support is only present if building with libelf. Skip the
intel-pt jitdump test if perf isn't compiled with libelf support.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/shell/test_intel_pt.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/perf/tests/shell/test_intel_pt.sh b/tools/perf/tests/shell/test_intel_pt.sh
index f3a9a040bacc..32a9b8dcb200 100755
--- a/tools/perf/tests/shell/test_intel_pt.sh
+++ b/tools/perf/tests/shell/test_intel_pt.sh
@@ -288,6 +288,11 @@ test_jitdump()
 	jitdump_incl_dir="${script_dir}/../../util"
 	jitdump_h="${jitdump_incl_dir}/jitdump.h"
 
+        if ! perf check feature -q libelf ; then
+		echo "SKIP: libelf is needed for jitdump"
+		return 2
+	fi
+
 	if [ ! -e "${jitdump_h}" ] ; then
 		echo "SKIP: Include file jitdump.h not found"
 		return 2
-- 
2.49.0.1238.gf8c92423fb-goog

[PATCH v2 6/7] perf test trace_summary: Skip --bpf-summary tests if no libbpf

[Ian Rogers]

If perf is built without libbpf (e.g. NO_LIBBPF=1) then the
--bpf-summary perf trace tests will fail. Skip the tests as this is
expected behavior.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/shell/trace_summary.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/perf/tests/shell/trace_summary.sh b/tools/perf/tests/shell/trace_summary.sh
index 49766524dc21..f9bb7f9388be 100755
--- a/tools/perf/tests/shell/trace_summary.sh
+++ b/tools/perf/tests/shell/trace_summary.sh
@@ -53,6 +53,12 @@ test_perf_trace "-as --summary-mode=thread --no-bpf-summary"
 # summary only for system wide - total summary mode
 test_perf_trace "-as --summary-mode=total --no-bpf-summary"
 
+if ! perf check feature -q bpf; then
+    echo "Skip --bpf-summary tests as perf built without libbpf"
+    rm -f ${OUTPUT}
+    exit 2
+fi
+
 # summary only for system wide - per-thread summary with BPF
 test_perf_trace "-as --summary-mode=thread --bpf-summary"
 
-- 
2.49.0.1238.gf8c92423fb-goog

[PATCH v2 7/7] perf thread: Avoid recursively taking thread__comm_lock

[Ian Rogers]

Fix cases where functions that take thread_comm_lcok were called
holding thread__comm_lock. This created some buggy behavior in perf
top when built with sanitizers. Ensure -Wthread-safety warnings for
clang work.

Fixes: 8f454c95817d ("perf thread: Ensure comm_lock held for comm_list")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/thread.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index c202b98b36c2..f4ad15e1e314 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -201,13 +201,21 @@ int thread__set_namespaces(struct thread *thread, u64 timestamp,
 	return ret;
 }
 
+static struct comm *__thread__comm(struct thread *thread)
+	SHARED_LOCKS_REQUIRED(thread__comm_lock(thread))
+{
+	if (list_empty(thread__comm_list(thread)))
+		return NULL;
+
+	return list_first_entry(thread__comm_list(thread), struct comm, list);
+}
+
 struct comm *thread__comm(struct thread *thread)
 {
 	struct comm *res = NULL;
 
 	down_read(thread__comm_lock(thread));
-	if (!list_empty(thread__comm_list(thread)))
-		res = list_first_entry(thread__comm_list(thread), struct comm, list);
+	res = __thread__comm(thread);
 	up_read(thread__comm_lock(thread));
 	return res;
 }
@@ -243,7 +251,7 @@ static int ____thread__set_comm(struct thread *thread, const char *str,
 				u64 timestamp, bool exec)
 	EXCLUSIVE_LOCKS_REQUIRED(thread__comm_lock(thread))
 {
-	struct comm *new, *curr = thread__comm(thread);
+	struct comm *new, *curr = __thread__comm(thread);
 
 	/* Override the default :tid entry */
 	if (!thread__comm_set(thread)) {
@@ -294,8 +302,9 @@ int thread__set_comm_from_proc(struct thread *thread)
 }
 
 static const char *__thread__comm_str(struct thread *thread)
+	SHARED_LOCKS_REQUIRED(thread__comm_lock(thread))
 {
-	const struct comm *comm = thread__comm(thread);
+	const struct comm *comm = __thread__comm(thread);
 
 	if (!comm)
 		return NULL;
-- 
2.49.0.1238.gf8c92423fb-goog

Re: [PATCH v2 6/7] perf test trace_summary: Skip --bpf-summary tests if no libbpf

[Howard Chu]

Hello Ian,

On Tue, May 27, 2025 at 8:26 PM Ian Rogers <irogers@google.com> wrote:
>
> If perf is built without libbpf (e.g. NO_LIBBPF=1) then the
> --bpf-summary perf trace tests will fail. Skip the tests as this is
> expected behavior.
>
> Signed-off-by: Ian Rogers <irogers@google.com>

Acked-by: Howard Chu <howardchu95@gmail.com>

Thanks,
Howard

Re: [PATCH v2 6/7] perf test trace_summary: Skip --bpf-summary tests if no libbpf

[Arnaldo Carvalho de Melo]

On Tue, May 27, 2025 at 09:17:25PM -0700, Howard Chu wrote:
> Hello Ian,
> 
> On Tue, May 27, 2025 at 8:26 PM Ian Rogers <irogers@google.com> wrote:
> >
> > If perf is built without libbpf (e.g. NO_LIBBPF=1) then the
> > --bpf-summary perf trace tests will fail. Skip the tests as this is
> > expected behavior.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> 
> Acked-by: Howard Chu <howardchu95@gmail.com>

Thanks, applied to perf-tools-next,

BTW, my answers were for v1 but b4 gets v2, for instance, the one that
is failing:

⬢ [acme@toolbx perf-tools-next]$ b4 am -P3 -ctsl --cc-trailers 20250527180703.129336-5-irogers@google.com
Grabbing thread from lore.kernel.org/all/20250527180703.129336-5-irogers@google.com/t.mbox.gz
Breaking thread to remove parents of 20250527180703.129336-1-irogers@google.com
Checking for newer revisions
Grabbing search results from lore.kernel.org
  Added from v2: 8 patches
Analyzing 21 messages in the thread
Analyzing 0 code-review messages
Will use the latest revision: v2
You can pick other revisions using the -vN flag
Unknown range value specified: 3
Checking attestation on all messages, may take a moment...
---
  ✓ [PATCH v2 3/7] perf symbol: Move demangling code out of symbol-elf.c
    + Link: https://lore.kernel.org/r/20250528032637.198960-4-irogers@google.com
    + Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
  ---
  ✓ Signed: DKIM/google.com
---
Total patches: 1 (cherrypicked: 3)
---
Cover: ./v2_20250527_irogers_various_asan_and_test_fixes.cover
 Link: https://lore.kernel.org/r/20250528032637.198960-1-irogers@google.com
 Base: not specified
       git am ./v2_20250527_irogers_various_asan_and_test_fixes.mbx
⬢ [acme@toolbx perf-tools-next]$ 

⬢ [acme@toolbx perf-tools-next]$        git am ./v2_20250527_irogers_various_asan_and_test_fixes.mbx
Applying: perf symbol: Move demangling code out of symbol-elf.c
⬢ [acme@toolbx perf-tools-next]$

⬢ [acme@toolbx perf-tools-next]$ m
rm: cannot remove '/home/acme/libexec/perf-core/scripts/python/Perf-Trace-Util/lib/Perf/Trace/__pycache__/Core.cpython-313.pyc': Permission denied
make: Entering directory '/home/acme/git/perf-tools-next/tools/perf'
  BUILD:   Doing 'make -j32' parallel build
Warning: Kernel ABI header differences:
  diff -u tools/arch/arm64/include/asm/cputype.h arch/arm64/include/asm/cputype.h

Auto-detecting system features:
...                                   libdw: [ on  ]
...                                   glibc: [ on  ]
...                                  libelf: [ on  ]
...                                 libnuma: [ on  ]
...                  numa_num_possible_cpus: [ on  ]
...                                 libperl: [ on  ]
...                               libpython: [ on  ]
...                               libcrypto: [ on  ]
...                             libcapstone: [ on  ]
...                               llvm-perf: [ on  ]
...                                    zlib: [ on  ]
...                                    lzma: [ on  ]
...                               get_cpuid: [ on  ]
...                                     bpf: [ on  ]
...                                  libaio: [ on  ]
...                                 libzstd: [ on  ]

  INSTALL libsubcmd_headers
  INSTALL libperf_headers
  INSTALL libapi_headers
  INSTALL libsymbol_headers
  PERF_VERSION = 6.15.rc7.g50ec05a72bc1
  LINK    /tmp/build/perf-tools-next/libperf-jvmti.so
  INSTALL libbpf_headers
  GEN     perf-archive
  GEN     perf-iostat
  CC      /tmp/build/perf-tools-next/util/symbol.o
  CC      /tmp/build/perf-tools-next/util/header.o
  CC      /tmp/build/perf-tools-next/util/symbol-elf.o
  CXX     /tmp/build/perf-tools-next/util/demangle-cxx.o
util/symbol-elf.c: In function ‘get_plt_got_name’:
util/symbol-elf.c:563:21: error: implicit declaration of function ‘demangle_sym’; did you mean ‘dso__demangle_sym’? [-Wimplicit-function-declaration]
  563 |         demangled = demangle_sym(di->dso, 0, sym_name);
      |                     ^~~~~~~~~~~~
      |                     dso__demangle_sym
util/symbol-elf.c:563:19: error: assignment to ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
  563 |         demangled = demangle_sym(di->dso, 0, sym_name);
      |                   ^
util/symbol-elf.c: In function ‘dso__synthesize_plt_symbols’:
util/symbol-elf.c:761:27: error: assignment to ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
  761 |                 demangled = demangle_sym(dso, 0, elf_name);
      |                           ^
util/symbol-elf.c: In function ‘dso__load_sym_internal’:
util/symbol-elf.c:1778:27: error: assignment to ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
 1778 |                 demangled = demangle_sym(dso, kmodule, elf_name);
      |                           ^
make[4]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:85: /tmp/build/perf-tools-next/util/symbol-elf.o] Error 1
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:142: util] Error 2
make[2]: *** [Makefile.perf:798: /tmp/build/perf-tools-next/perf-util-in.o] Error 2
make[1]: *** [Makefile.perf:290: sub-make] Error 2
make: *** [Makefile:119: install-bin] Error 2
make: Leaving directory '/home/acme/git/perf-tools-next/tools/perf'
⬢ [acme@toolbx perf-tools-next]$

I can try fixing those up, but I'm concentrated now in getting as much
low hanging fruits as possible for this merge, and the patches in this
series were mostly super small and fixed things.

- Arnaldo

Re: [PATCH v2 4/7] perf intel-tpebs: Avoid race when evlist is being deleted

[Namhyung Kim]

Hi Ian,

On Tue, May 27, 2025 at 08:26:34PM -0700, Ian Rogers wrote:
> Reading through the evsel->evlist may seg fault if a sample arrives
> when the evlist is being deleted. Detect this case and ignore samples
> arriving when the evlist is being deleted.
> 
> Fixes: bcfab08db7fb ("perf intel-tpebs: Filter non-workload samples")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/intel-tpebs.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
> index 4ad4bc118ea5..3b92ebf5c112 100644
> --- a/tools/perf/util/intel-tpebs.c
> +++ b/tools/perf/util/intel-tpebs.c
> @@ -162,9 +162,17 @@ static bool is_child_pid(pid_t parent, pid_t child)
>  
>  static bool should_ignore_sample(const struct perf_sample *sample, const struct tpebs_retire_lat *t)
>  {
> -	pid_t workload_pid = t->evsel->evlist->workload.pid;
> -	pid_t sample_pid = sample->pid;
> +	pid_t workload_pid, sample_pid = sample->pid;
>  
> +	/*
> +	 * During evlist__purge the evlist will be removed prior to the
> +	 * evsel__exit calling evsel__tpebs_close and taking the
> +	 * tpebs_mtx. Avoid a segfault by ignoring samples in this case.
> +	 */
> +	if (t->evsel->evlist == NULL)
> +		return true;
> +
> +	workload_pid = t->evsel->evlist->workload.pid;

I'm curious if there's a chance of TOCTOU race.  It'd certainly help
the segfault but would this code prevent it completely?

Thanks,
Namhyung


>  	if (workload_pid < 0 || workload_pid == sample_pid)
>  		return false;
>  
> -- 
> 2.49.0.1238.gf8c92423fb-goog
> 

Re: [PATCH v2 0/7] Various asan and test fixes

[Namhyung Kim]

On Tue, May 27, 2025 at 08:26:30PM -0700, Ian Rogers wrote:
> When testing removing perf_env with asan I noticed a number of test
> failures either because of asan issues or because of building with
> NO_LIBELF=1 and NO_LIBBPF=1 to avoid false memory leaks. Address these
> issues so that real test failures stand out.
> 
> v2: Fix wrong elf size use in filename__read_build_id fix. Add
>     thread__comm_lock fix.
> 
> Ian Rogers (7):
>   perf symbol: Fix use-after-free in filename__read_build_id
>   perf test demangle-java: Don't segv if demangling fails
>   perf symbol: Move demangling code out of symbol-elf.c
>   perf intel-tpebs: Avoid race when evlist is being deleted
>   perf test intel-pt: Skip jitdump test if no libelf
>   perf test trace_summary: Skip --bpf-summary tests if no libbpf
>   perf thread: Avoid recursively taking thread__comm_lock

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

> 
>  tools/perf/tests/demangle-java-test.c   |   5 +
>  tools/perf/tests/shell/test_intel_pt.sh |   5 +
>  tools/perf/tests/shell/trace_summary.sh |   6 +
>  tools/perf/util/demangle-cxx.h          |   2 +
>  tools/perf/util/intel-tpebs.c           |  12 +-
>  tools/perf/util/symbol-elf.c            |  86 ------------
>  tools/perf/util/symbol-minimal.c        | 175 ++++++++++--------------
>  tools/perf/util/symbol.c                |  87 ++++++++++++
>  tools/perf/util/thread.c                |  17 ++-
>  9 files changed, 198 insertions(+), 197 deletions(-)
> 
> -- 
> 2.49.0.1238.gf8c92423fb-goog
> 

Re: [PATCH v2 4/7] perf intel-tpebs: Avoid race when evlist is being deleted

[Ian Rogers]

On Wed, May 28, 2025 at 10:53 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hi Ian,
>
> On Tue, May 27, 2025 at 08:26:34PM -0700, Ian Rogers wrote:
> > Reading through the evsel->evlist may seg fault if a sample arrives
> > when the evlist is being deleted. Detect this case and ignore samples
> > arriving when the evlist is being deleted.
> >
> > Fixes: bcfab08db7fb ("perf intel-tpebs: Filter non-workload samples")
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/intel-tpebs.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
> > index 4ad4bc118ea5..3b92ebf5c112 100644
> > --- a/tools/perf/util/intel-tpebs.c
> > +++ b/tools/perf/util/intel-tpebs.c
> > @@ -162,9 +162,17 @@ static bool is_child_pid(pid_t parent, pid_t child)
> >
> >  static bool should_ignore_sample(const struct perf_sample *sample, const struct tpebs_retire_lat *t)
> >  {
> > -     pid_t workload_pid = t->evsel->evlist->workload.pid;
> > -     pid_t sample_pid = sample->pid;
> > +     pid_t workload_pid, sample_pid = sample->pid;
> >
> > +     /*
> > +      * During evlist__purge the evlist will be removed prior to the
> > +      * evsel__exit calling evsel__tpebs_close and taking the
> > +      * tpebs_mtx. Avoid a segfault by ignoring samples in this case.
> > +      */
> > +     if (t->evsel->evlist == NULL)
> > +             return true;
> > +
> > +     workload_pid = t->evsel->evlist->workload.pid;
>
> I'm curious if there's a chance of TOCTOU race.  It'd certainly help
> the segfault but would this code prevent it completely?

Good point. I think the race is already small as it doesn't happen
without sanitizers for me.
Thinking about the evlist problem. When a destructor (evlist__delete)
it is generally assumed the code is being single threaded and in C++
clang's -Wthread-safety will ignore destructors for this reason
(annoying imo as it hides bugs). I don't see a good way to solve that
for the evlist and evsel for the TPEBS case without using reference
counting. Adding reference counts to evlist and evsel would be do-able
as we could use reference count checking, but it would be a large and
invasive change. Wdyt?

Thanks,
Ian

> Thanks,
> Namhyung
>
>
> >       if (workload_pid < 0 || workload_pid == sample_pid)
> >               return false;
> >
> > --
> > 2.49.0.1238.gf8c92423fb-goog
> >

Re: [PATCH v2 4/7] perf intel-tpebs: Avoid race when evlist is being deleted

[Namhyung Kim]

On Wed, May 28, 2025 at 11:02:44AM -0700, Ian Rogers wrote:
> On Wed, May 28, 2025 at 10:53 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Hi Ian,
> >
> > On Tue, May 27, 2025 at 08:26:34PM -0700, Ian Rogers wrote:
> > > Reading through the evsel->evlist may seg fault if a sample arrives
> > > when the evlist is being deleted. Detect this case and ignore samples
> > > arriving when the evlist is being deleted.
> > >
> > > Fixes: bcfab08db7fb ("perf intel-tpebs: Filter non-workload samples")
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/util/intel-tpebs.c | 12 ++++++++++--
> > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
> > > index 4ad4bc118ea5..3b92ebf5c112 100644
> > > --- a/tools/perf/util/intel-tpebs.c
> > > +++ b/tools/perf/util/intel-tpebs.c
> > > @@ -162,9 +162,17 @@ static bool is_child_pid(pid_t parent, pid_t child)
> > >
> > >  static bool should_ignore_sample(const struct perf_sample *sample, const struct tpebs_retire_lat *t)
> > >  {
> > > -     pid_t workload_pid = t->evsel->evlist->workload.pid;
> > > -     pid_t sample_pid = sample->pid;
> > > +     pid_t workload_pid, sample_pid = sample->pid;
> > >
> > > +     /*
> > > +      * During evlist__purge the evlist will be removed prior to the
> > > +      * evsel__exit calling evsel__tpebs_close and taking the
> > > +      * tpebs_mtx. Avoid a segfault by ignoring samples in this case.
> > > +      */
> > > +     if (t->evsel->evlist == NULL)
> > > +             return true;
> > > +
> > > +     workload_pid = t->evsel->evlist->workload.pid;
> >
> > I'm curious if there's a chance of TOCTOU race.  It'd certainly help
> > the segfault but would this code prevent it completely?
> 
> Good point. I think the race is already small as it doesn't happen
> without sanitizers for me.
> Thinking about the evlist problem. When a destructor (evlist__delete)
> it is generally assumed the code is being single threaded and in C++
> clang's -Wthread-safety will ignore destructors for this reason
> (annoying imo as it hides bugs). I don't see a good way to solve that
> for the evlist and evsel for the TPEBS case without using reference
> counting. Adding reference counts to evlist and evsel would be do-able
> as we could use reference count checking, but it would be a large and
> invasive change. Wdyt?

Would it be possible to kill the TPEBS thread before deleting evlist?

Thanks,
Namhyung

Re: [PATCH v2 4/7] perf intel-tpebs: Avoid race when evlist is being deleted

[Ian Rogers]

On Wed, May 28, 2025 at 1:13 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Wed, May 28, 2025 at 11:02:44AM -0700, Ian Rogers wrote:
> > On Wed, May 28, 2025 at 10:53 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > Hi Ian,
> > >
> > > On Tue, May 27, 2025 at 08:26:34PM -0700, Ian Rogers wrote:
> > > > Reading through the evsel->evlist may seg fault if a sample arrives
> > > > when the evlist is being deleted. Detect this case and ignore samples
> > > > arriving when the evlist is being deleted.
> > > >
> > > > Fixes: bcfab08db7fb ("perf intel-tpebs: Filter non-workload samples")
> > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > ---
> > > >  tools/perf/util/intel-tpebs.c | 12 ++++++++++--
> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
> > > > index 4ad4bc118ea5..3b92ebf5c112 100644
> > > > --- a/tools/perf/util/intel-tpebs.c
> > > > +++ b/tools/perf/util/intel-tpebs.c
> > > > @@ -162,9 +162,17 @@ static bool is_child_pid(pid_t parent, pid_t child)
> > > >
> > > >  static bool should_ignore_sample(const struct perf_sample *sample, const struct tpebs_retire_lat *t)
> > > >  {
> > > > -     pid_t workload_pid = t->evsel->evlist->workload.pid;
> > > > -     pid_t sample_pid = sample->pid;
> > > > +     pid_t workload_pid, sample_pid = sample->pid;
> > > >
> > > > +     /*
> > > > +      * During evlist__purge the evlist will be removed prior to the
> > > > +      * evsel__exit calling evsel__tpebs_close and taking the
> > > > +      * tpebs_mtx. Avoid a segfault by ignoring samples in this case.
> > > > +      */
> > > > +     if (t->evsel->evlist == NULL)
> > > > +             return true;
> > > > +
> > > > +     workload_pid = t->evsel->evlist->workload.pid;
> > >
> > > I'm curious if there's a chance of TOCTOU race.  It'd certainly help
> > > the segfault but would this code prevent it completely?
> >
> > Good point. I think the race is already small as it doesn't happen
> > without sanitizers for me.
> > Thinking about the evlist problem. When a destructor (evlist__delete)
> > it is generally assumed the code is being single threaded and in C++
> > clang's -Wthread-safety will ignore destructors for this reason
> > (annoying imo as it hides bugs). I don't see a good way to solve that
> > for the evlist and evsel for the TPEBS case without using reference
> > counting. Adding reference counts to evlist and evsel would be do-able
> > as we could use reference count checking, but it would be a large and
> > invasive change. Wdyt?
>
> Would it be possible to kill the TPEBS thread before deleting evlist?

The TPEBS thread and other data structures are global and not tied to
the evlist, so there can and are multiple evlists at play. When using
TPEBS there is the evlist for perf stat, there is also the evlist for
the samples. There's sense in having the evlist own the TPEBS data
structures, there's also sense in things being global. I think if I'd
done it I'd have gone with TPEBS within the evlist, but I suspect in
the original changes there was a worry about adding cost on non-x86
builds.

Thanks,
Ian

> Thanks,
> Namhyung
>

Re: [PATCH v2 4/7] perf intel-tpebs: Avoid race when evlist is being deleted

[Namhyung Kim]

On Wed, May 28, 2025 at 01:44:36PM -0700, Ian Rogers wrote:
> On Wed, May 28, 2025 at 1:13 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Wed, May 28, 2025 at 11:02:44AM -0700, Ian Rogers wrote:
> > > On Wed, May 28, 2025 at 10:53 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > > >
> > > > Hi Ian,
> > > >
> > > > On Tue, May 27, 2025 at 08:26:34PM -0700, Ian Rogers wrote:
> > > > > Reading through the evsel->evlist may seg fault if a sample arrives
> > > > > when the evlist is being deleted. Detect this case and ignore samples
> > > > > arriving when the evlist is being deleted.
> > > > >
> > > > > Fixes: bcfab08db7fb ("perf intel-tpebs: Filter non-workload samples")
> > > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > > ---
> > > > >  tools/perf/util/intel-tpebs.c | 12 ++++++++++--
> > > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
> > > > > index 4ad4bc118ea5..3b92ebf5c112 100644
> > > > > --- a/tools/perf/util/intel-tpebs.c
> > > > > +++ b/tools/perf/util/intel-tpebs.c
> > > > > @@ -162,9 +162,17 @@ static bool is_child_pid(pid_t parent, pid_t child)
> > > > >
> > > > >  static bool should_ignore_sample(const struct perf_sample *sample, const struct tpebs_retire_lat *t)
> > > > >  {
> > > > > -     pid_t workload_pid = t->evsel->evlist->workload.pid;
> > > > > -     pid_t sample_pid = sample->pid;
> > > > > +     pid_t workload_pid, sample_pid = sample->pid;
> > > > >
> > > > > +     /*
> > > > > +      * During evlist__purge the evlist will be removed prior to the
> > > > > +      * evsel__exit calling evsel__tpebs_close and taking the
> > > > > +      * tpebs_mtx. Avoid a segfault by ignoring samples in this case.
> > > > > +      */
> > > > > +     if (t->evsel->evlist == NULL)
> > > > > +             return true;
> > > > > +
> > > > > +     workload_pid = t->evsel->evlist->workload.pid;
> > > >
> > > > I'm curious if there's a chance of TOCTOU race.  It'd certainly help
> > > > the segfault but would this code prevent it completely?
> > >
> > > Good point. I think the race is already small as it doesn't happen
> > > without sanitizers for me.
> > > Thinking about the evlist problem. When a destructor (evlist__delete)
> > > it is generally assumed the code is being single threaded and in C++
> > > clang's -Wthread-safety will ignore destructors for this reason
> > > (annoying imo as it hides bugs). I don't see a good way to solve that
> > > for the evlist and evsel for the TPEBS case without using reference
> > > counting. Adding reference counts to evlist and evsel would be do-able
> > > as we could use reference count checking, but it would be a large and
> > > invasive change. Wdyt?
> >
> > Would it be possible to kill the TPEBS thread before deleting evlist?
> 
> The TPEBS thread and other data structures are global and not tied to
> the evlist, so there can and are multiple evlists at play. When using
> TPEBS there is the evlist for perf stat, there is also the evlist for
> the samples. There's sense in having the evlist own the TPEBS data
> structures, there's also sense in things being global. I think if I'd
> done it I'd have gone with TPEBS within the evlist, but I suspect in
> the original changes there was a worry about adding cost on non-x86
> builds.

Ok, I thought deleting evlist is quite late in the execution and it may
be easy to make the change.  If not, let's see how the current fix will
work before going further. :)

Thanks,
Namhyung