mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH v1 0/5] perf tools: Fix jitdump and dso handling
@ 2026-09-03 13:22 Arnaldo Carvalho de Melo
  2026-09-03 13:22 ` [PATCH 1/5] perf jitdump: Byte-swap debug entries via unaligned-safe accessors Arnaldo Carvalho de Melo
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2026-09-03 13:22 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Ingo Molnar, Thomas Gleixner, James Clark, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Clark Williams, linux-kernel, linux-perf-users,
	Arnaldo Carvalho de Melo

Hi,

This series addresses five small fixes in the perf jitdump and dso
code that were found by sashiko-bot during automated review.

Patches 1 and 2 - unaligned-safe debug entries:

  - 1/5 perf jitdump: Byte-swap debug entries via unaligned-safe accessors
    The byte-swap loop in jit_get_next_entry() did 64-bit loads/stores
    through struct member access.  This seems to be UB when entries are
    unaligned after the first variable-length name[].  Use
    get_unaligned()/put_unaligned() for each field, as was done in the
    earlier bounds-check hardening.

  - 2/5 perf genelf: Use unaligned-safe accessors for debug entries
    The same packing issue on the native path.  As far as I can tell,
    jit_process_debug_info(), get_special_opcode() and
    emit_lineno_info() all read u64 addr and int lineno through struct
    access.  Convert them to unaligned-safe accessors, matching the
    layout the jitdump writers (LLVM, JVM agents) emit.

Patch 3 - stale unwinding state:

  - 3/5 perf jitdump: Free unwinding data even when eh_frame_hdr_size is zero
    jit_repipe_code_load() only cleared jd->unwinding_data when both
    unwinding_data and eh_frame_hdr_size were set.  If a record carries
    unwinding_data with eh_frame_hdr_size==0, so the answer would be that
    the check fails and the state is applied to all subsequent records.
    The record is validated upstream so eh_frame_hdr_size <= unwinding_size
    always holds.  Free based on the data pointer alone.

Patch 4 - event sizing:

  - 4/5 perf jitdump: Size code_move event allocation with idr_size
    jit_repipe_code_move() allocated event as sizeof(*event)+16 but
    computed header.size with +idr_size.  When idr_size>16, I believe
    header.size exceeds the allocation and perf_data__write() reads past
    the heap, leaking adjacent heap into perf.data.  Size with idr_size
    like jit_repipe_code_load() does.

Patch 5 - open list deadlock/race:

  - 5/5 perf dso: Defer dropping the open list reference until after the lock
    The reference taken by dso__list_add() cannot be dropped while
    holding dso__data_open_lock: dso__put() may call dso__data_close()
    which takes the same lock, deadlocking.  This seems to be the cause
    of the inconsistent list/counter state under REFCNT_CHECKING.  Fix by
    transferring the reference to a deferred node drained by
    dso__put_deferred() after every unlock.  Since the counter is now
    decremented under the lock, do_open()'s close_first_dso() no longer
    races with a stale count.

Regards,

- Arnaldo

Arnaldo Carvalho de Melo (5):
  perf jitdump: Byte-swap debug entries via unaligned-safe accessors
  perf genelf: Use unaligned-safe accessors for debug entries
  perf jitdump: Free unwinding data even when eh_frame_hdr_size is zero
  perf jitdump: Size code_move event allocation with idr_size
  perf dso: Defer dropping the open list reference until after the lock

 tools/perf/util/dso.c          | 75 ++++++++++++++++++++++++++++++++--
 tools/perf/util/genelf_debug.c | 30 ++++++++------
 tools/perf/util/jitdump.c      | 20 ++++++---
 3 files changed, 103 insertions(+), 22 deletions(-)

-- 
2.55.0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/5] perf jitdump: Byte-swap debug entries via unaligned-safe accessors
  2026-09-03 13:22 [PATCH v1 0/5] perf tools: Fix jitdump and dso handling Arnaldo Carvalho de Melo
@ 2026-09-03 13:22 ` Arnaldo Carvalho de Melo
  2026-09-03 13:22 ` [PATCH 2/5] perf genelf: Use unaligned-safe accessors for debug entries Arnaldo Carvalho de Melo
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2026-09-03 13:22 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Ingo Molnar, Thomas Gleixner, James Clark, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Clark Williams, linux-kernel, linux-perf-users,
	Arnaldo Carvalho de Melo, sashiko-bot, Stephane Eranian

From: Arnaldo Carvalho de Melo <acme@redhat.com>

debug_entry records are packed with a variable-length name[] field, so
entries after the first may start at addresses that are not naturally
aligned for their u64 addr and int lineno/discrim fields.  On strict
alignment architectures the byte-swap loop in jit_get_next_entry()
performed misaligned 64-bit loads and stores through struct member
access, which is undefined behavior.

Use get_unaligned()/put_unaligned() for the byte-swap of each field.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
Assisted-by: LLM
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/jitdump.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
index d25a9fe9b020ce87..e0d5cc9a828189a2 100644
--- a/tools/perf/util/jitdump.c
+++ b/tools/perf/util/jitdump.c
@@ -14,6 +14,8 @@
 #include <sys/stat.h>
 #include <sys/mman.h>
 #include <linux/stringify.h>
+#include <linux/kernel.h>
+#include <linux/unaligned.h>
 
 #include "event.h"
 #include "debug.h"
@@ -343,9 +345,14 @@ jit_get_next_entry(struct jit_buf_desc *jd)
 				/* name must be NUL-terminated within the record */
 				if (!memchr(ent->name, '\0', (char *)end - ent->name))
 					break;
-				ent->addr    = bswap_64(ent->addr);
-				ent->lineno  = bswap_32(ent->lineno);
-				ent->discrim = bswap_32(ent->discrim);
+				/*
+				 * debug entries are packed with a variable-length
+				 * name[], so entries after the first may be
+				 * unaligned: byte-swap via unaligned-safe accessors.
+				 */
+				put_unaligned(bswap_64(get_unaligned(&ent->addr)), &ent->addr);
+				put_unaligned(bswap_32(get_unaligned(&ent->lineno)), &ent->lineno);
+				put_unaligned(bswap_32(get_unaligned(&ent->discrim)), &ent->discrim);
 				ent = debug_entry_next(ent);
 			}
 			/* clamp so downstream consumers don't overrun */
-- 
2.55.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/5] perf genelf: Use unaligned-safe accessors for debug entries
  2026-09-03 13:22 [PATCH v1 0/5] perf tools: Fix jitdump and dso handling Arnaldo Carvalho de Melo
  2026-09-03 13:22 ` [PATCH 1/5] perf jitdump: Byte-swap debug entries via unaligned-safe accessors Arnaldo Carvalho de Melo
@ 2026-09-03 13:22 ` Arnaldo Carvalho de Melo
  2026-09-03 13:22 ` [PATCH 3/5] perf jitdump: Free unwinding data even when eh_frame_hdr_size is zero Arnaldo Carvalho de Melo
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2026-09-03 13:22 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Ingo Molnar, Thomas Gleixner, James Clark, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Clark Williams, linux-kernel, linux-perf-users,
	Arnaldo Carvalho de Melo, sashiko-bot, Stephane Eranian

From: Arnaldo Carvalho de Melo <acme@redhat.com>

debug_entry records are packed with a variable-length name[] field, so
entries after the first may start at addresses that are not naturally
aligned.  jit_process_debug_info(), get_special_opcode() and
emit_lineno_info() read and write the u64 addr and int lineno fields
through struct member access, which is undefined behavior on
strict-alignment architectures.

Use get_unaligned()/put_unaligned() to read and update each field,
matching the layout the jitdump writers (LLVM, JVM agents) emit, which
packs entries without padding.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
Assisted-by: LLM
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/genelf_debug.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/genelf_debug.c b/tools/perf/util/genelf_debug.c
index 8588b3e35e008396..8244d3c64103b6be 100644
--- a/tools/perf/util/genelf_debug.c
+++ b/tools/perf/util/genelf_debug.c
@@ -12,6 +12,8 @@
  */
 #include <linux/compiler.h>
 #include <linux/zalloc.h>
+#include <linux/kernel.h>
+#include <linux/unaligned.h>
 #include <sys/types.h>
 #include <stdio.h>
 #include <getopt.h>
@@ -303,11 +305,13 @@ static ubyte get_special_opcode(struct debug_entry *ent,
 {
 	unsigned int temp;
 	unsigned long delta_addr;
+	unsigned int lineno = get_unaligned(&ent->lineno);
+	uint64_t addr = get_unaligned(&ent->addr);
 
 	/*
 	 * delta from line_base
 	 */
-	temp = (ent->lineno - last_line) - default_debug_line_header.line_base;
+	temp = (lineno - last_line) - default_debug_line_header.line_base;
 
 	if (temp >= default_debug_line_header.line_range)
 		return 0;
@@ -315,7 +319,7 @@ static ubyte get_special_opcode(struct debug_entry *ent,
 	/*
 	 * delta of addresses
 	 */
-	delta_addr = (ent->addr - last_vma) / default_debug_line_header.minimum_instruction_length;
+	delta_addr = (addr - last_vma) / default_debug_line_header.minimum_instruction_length;
 
 	/* This is not sufficient to ensure opcode will be in [0-256] but
 	 * sufficient to ensure when summing with the delta lineno we will
@@ -355,13 +359,15 @@ static void emit_lineno_info(struct buffer_ext *be,
 	unsigned long last_vma = 0;
 	char const  *cur_filename = NULL;
 	unsigned long cur_file_idx = 0;
-	int last_line = 1;
+	unsigned int last_line = 1;
 
 	emit_lne_set_address(be, (void *)code_addr);
 
 	for (i = 0; i < nr_entry; i++, ent = debug_entry_next(ent)) {
 		int need_copy = 0;
 		ubyte special_opcode;
+		unsigned int lineno = get_unaligned(&ent->lineno);
+		uint64_t addr = get_unaligned(&ent->addr);
 
 		/*
 		 * check if filename changed, if so add it
@@ -376,24 +382,24 @@ static void emit_lineno_info(struct buffer_ext *be,
 
 		special_opcode = get_special_opcode(ent, last_line, last_vma);
 		if (special_opcode != 0) {
-			last_line = ent->lineno;
-			last_vma  = ent->addr;
+			last_line = lineno;
+			last_vma  = addr;
 			emit_opcode(be, special_opcode);
 		} else {
 			/*
 			 * lines differ, emit line delta
 			 */
-			if (last_line != ent->lineno) {
-				emit_advance_lineno(be, ent->lineno - last_line);
-				last_line = ent->lineno;
+			if (last_line != lineno) {
+				emit_advance_lineno(be, lineno - last_line);
+				last_line = lineno;
 				need_copy = 1;
 			}
 			/*
 			 * addresses differ, emit address delta
 			 */
-			if (last_vma != ent->addr) {
-				emit_advance_pc(be, ent->addr - last_vma);
-				last_vma = ent->addr;
+			if (last_vma != addr) {
+				emit_advance_pc(be, addr - last_vma);
+				last_vma = addr;
 				need_copy = 1;
 			}
 			/*
@@ -480,7 +486,7 @@ jit_process_debug_info(uint64_t code_addr,
 	int i;
 
 	for (i = 0; i < nr_debug_entries; i++) {
-		ent->addr = ent->addr - code_addr;
+		put_unaligned(get_unaligned(&ent->addr) - code_addr, &ent->addr);
 		ent = debug_entry_next(ent);
 	}
 	add_compilation_unit(di, buffer_ext_size(dl));
-- 
2.55.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 3/5] perf jitdump: Free unwinding data even when eh_frame_hdr_size is zero
  2026-09-03 13:22 [PATCH v1 0/5] perf tools: Fix jitdump and dso handling Arnaldo Carvalho de Melo
  2026-09-03 13:22 ` [PATCH 1/5] perf jitdump: Byte-swap debug entries via unaligned-safe accessors Arnaldo Carvalho de Melo
  2026-09-03 13:22 ` [PATCH 2/5] perf genelf: Use unaligned-safe accessors for debug entries Arnaldo Carvalho de Melo
@ 2026-09-03 13:22 ` Arnaldo Carvalho de Melo
  2026-09-03 17:05   ` Ian Rogers
  2026-09-03 13:22 ` [PATCH 4/5] perf jitdump: Size code_move event allocation with idr_size Arnaldo Carvalho de Melo
  2026-09-03 13:22 ` [PATCH 5/5] perf dso: Defer dropping the open list reference until after the lock Arnaldo Carvalho de Melo
  4 siblings, 1 reply; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2026-09-03 13:22 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Ingo Molnar, Thomas Gleixner, James Clark, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Clark Williams, linux-kernel, linux-perf-users,
	Arnaldo Carvalho de Melo, sashiko-bot, Stephane Eranian

From: Arnaldo Carvalho de Melo <acme@redhat.com>

jit_repipe_code_load() only cleared the unwinding state when both
unwinding_data and eh_frame_hdr_size were set.  When a record carries
unwinding data but eh_frame_hdr_size is 0, the cleanup condition fails
and the unwinding state persists in jd, being applied to all subsequent
JIT_CODE_LOAD and JIT_CODE_MOVE records, duplicating unwinding sections
in the generated ELF files and inflating their event->mmap2.len.

The record is validated upstream so eh_frame_hdr_size <= unwinding_size
always holds.  Free the unwinding data based on the data pointer alone.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
Assisted-by: LLM
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/jitdump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
index e0d5cc9a828189a2..efb40d93e33ae664 100644
--- a/tools/perf/util/jitdump.c
+++ b/tools/perf/util/jitdump.c
@@ -530,7 +530,7 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd, union jr_entry *jr)
 		jd->nr_debug_entries = 0;
 	}
 
-	if (jd->unwinding_data && jd->eh_frame_hdr_size) {
+	if (jd->unwinding_data) {
 		zfree(&jd->unwinding_data);
 		jd->eh_frame_hdr_size = 0;
 		jd->unwinding_mapped_size = 0;
-- 
2.55.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 4/5] perf jitdump: Size code_move event allocation with idr_size
  2026-09-03 13:22 [PATCH v1 0/5] perf tools: Fix jitdump and dso handling Arnaldo Carvalho de Melo
                   ` (2 preceding siblings ...)
  2026-09-03 13:22 ` [PATCH 3/5] perf jitdump: Free unwinding data even when eh_frame_hdr_size is zero Arnaldo Carvalho de Melo
@ 2026-09-03 13:22 ` Arnaldo Carvalho de Melo
  2026-09-03 13:22 ` [PATCH 5/5] perf dso: Defer dropping the open list reference until after the lock Arnaldo Carvalho de Melo
  4 siblings, 0 replies; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2026-09-03 13:22 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Ingo Molnar, Thomas Gleixner, James Clark, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Clark Williams, linux-kernel, linux-perf-users,
	Arnaldo Carvalho de Melo, sashiko-bot, Stephane Eranian

From: Arnaldo Carvalho de Melo <acme@redhat.com>

jit_repipe_code_move() allocated the mmap2 event with a hardcoded
+16, but computes event->mmap2.header.size as sizeof(event->mmap2)
minus unused filename bytes plus idr_size.  When idr_size is larger
than 16, header.size exceeds the allocation, so perf_data__write()
reads past the heap allocation, leaking adjacent heap memory into the
generated perf.data file.

Size the allocation with idr_size like jit_repipe_code_load() does.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
Assisted-by: LLM
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/jitdump.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
index efb40d93e33ae664..689aa0a8c60bcd8b 100644
--- a/tools/perf/util/jitdump.c
+++ b/tools/perf/util/jitdump.c
@@ -636,9 +636,10 @@ static int jit_repipe_code_move(struct jit_buf_desc *jd, union jr_entry *jr)
 	idr_size = jd->machine->id_hdr_size;
 
 	/*
-	 * +16 to account for sample_id_all (hack)
+	 * Sample ID is written past the end of the mmap2 record; size
+	 * the allocation to account for it instead of a hardcoded +16.
 	 */
-	event = calloc(1, sizeof(*event) + 16);
+	event = calloc(1, sizeof(*event) + idr_size);
 	if (!event)
 		return -1;
 
-- 
2.55.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 5/5] perf dso: Defer dropping the open list reference until after the lock
  2026-09-03 13:22 [PATCH v1 0/5] perf tools: Fix jitdump and dso handling Arnaldo Carvalho de Melo
                   ` (3 preceding siblings ...)
  2026-09-03 13:22 ` [PATCH 4/5] perf jitdump: Size code_move event allocation with idr_size Arnaldo Carvalho de Melo
@ 2026-09-03 13:22 ` Arnaldo Carvalho de Melo
  2026-09-03 16:40   ` Ian Rogers
  4 siblings, 1 reply; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2026-09-03 13:22 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Ingo Molnar, Thomas Gleixner, James Clark, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Clark Williams, linux-kernel, linux-perf-users,
	Arnaldo Carvalho de Melo, sashiko-bot, Stephane Eranian

From: Arnaldo Carvalho de Melo <acme@redhat.com>

There was a problem in the code with some resources potentially being
left unbalanced, and the logic on dso__data_close() becoming confused
if the fd had been closed already.

The reference taken by dso__list_add() on the open list cannot be
dropped while holding the open lock: dso__put() may call
dso__data_close(), which takes dso__data_open_lock() itself,
deadlocking and leaving the list and its counter inconsistent for
concurrent threads.

Fix it by changing dso__list_del() to transfer the reference to a
deferred node, drained by dso__put_deferred() right after every
unlock of dso__data_open_lock().  Since the counter is now decremented
under the open lock, do_open()'s close_first_dso() no longer races
with a stale count.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
Assisted-by: LLM
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/dso.c | 75 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 71 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 42bfe30a3b518e80..a4b2361bc7420084 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -531,18 +531,79 @@ static void dso__list_add(struct dso *dso) EXCLUSIVE_LOCKS_REQUIRED(_dso__data_o
 	dso__data_open_cnt++;
 }
 
+#ifdef REFCNT_CHECKING
+/*
+ * A deferred put: carries the reference taken by dso__list_add() for an
+ * entry removed from dso__data_open.  Dedicated nodes are used so that
+ * the dso_data's own open_entry node can be relinked by a concurrent
+ * dso__list_add() without corrupting this list or its reference.
+ */
+struct dso_data_put {
+	struct list_head entry;
+	struct dso *dso;
+};
+static LIST_HEAD(dso__data_open_put);
+#endif
+
 static void dso__list_del(struct dso *dso) EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)
 {
-	list_del_init(&dso__data(dso)->open_entry);
 #ifdef REFCNT_CHECKING
-	mutex_unlock(dso__data_open_lock());
-	dso__put(dso__data(dso)->dso);
-	mutex_lock(dso__data_open_lock());
+	struct dso_data_put *put;
 #endif
+
+	list_del_init(&dso__data(dso)->open_entry);
 	WARN_ONCE(dso__data_open_cnt <= 0,
 		  "DSO data fd counter out of bounds.");
 	dso__data_open_cnt--;
+#ifdef REFCNT_CHECKING
+	/*
+	 * The reference taken in dso__list_add() cannot be dropped while
+	 * holding the open lock: dso__put() may call dso__data_close(),
+	 * which takes dso__data_open_lock itself, deadlocking and leaving
+	 * the list/counter state inconsistent for concurrent threads.
+	 * Transfer the reference to a deferred node drained by
+	 * dso__put_deferred() once the lock is released.
+	 */
+	put = zalloc(sizeof(*put));
+
+	if (put == NULL)
+		return;
+
+	put->dso = dso__data(dso)->dso;
+	dso__data(dso)->dso = NULL;
+	list_add_tail(&put->entry, &dso__data_open_put);
+#endif
+}
+
+#ifdef REFCNT_CHECKING
+/*
+ * Drop the references deferred by dso__list_del().  Must be called
+ * without holding dso__data_open_lock: dso__put() may re-enter it via
+ * dso__data_close().
+ */
+static void dso__put_deferred(void) LOCKS_EXCLUDED(_dso__data_open_lock)
+{
+	for (;;) {
+		struct dso_data_put *put;
+		struct dso *dso;
+
+		mutex_lock(dso__data_open_lock());
+		put = list_first_entry_or_null(&dso__data_open_put, struct dso_data_put, entry);
+		if (put == NULL) {
+			mutex_unlock(dso__data_open_lock());
+			return;
+		}
+		list_del_init(&put->entry);
+		dso = put->dso;
+		mutex_unlock(dso__data_open_lock());
+
+		free(put);
+		dso__put(dso);
+	}
 }
+#else
+static void dso__put_deferred(void) {}
+#endif
 
 static void close_first_dso(void);
 
@@ -805,6 +866,7 @@ void dso__data_close(struct dso *dso)
 	mutex_lock(dso__data_open_lock());
 	close_dso(dso);
 	mutex_unlock(dso__data_open_lock());
+	dso__put_deferred();
 }
 
 static void try_to_open_dso(struct dso *dso, struct machine *machine)
@@ -865,12 +927,14 @@ bool dso__data_get_fd(struct dso *dso, struct machine *machine, int *fd)
 		return true;
 
 	mutex_unlock(dso__data_open_lock());
+	dso__put_deferred();
 	return false;
 }
 
 void dso__data_put_fd(struct dso *dso __maybe_unused)
 {
 	mutex_unlock(dso__data_open_lock());
+	dso__put_deferred();
 }
 
 bool dso__data_status_seen(struct dso *dso, enum dso_data_status_seen by)
@@ -1058,6 +1122,7 @@ static ssize_t file_read(struct dso *dso, struct machine *machine,
 	ret = pread(dso__data(dso)->fd, data, DSO__DATA_CACHE_SIZE, offset);
 out:
 	mutex_unlock(dso__data_open_lock());
+	dso__put_deferred();
 	return ret;
 }
 
@@ -1188,6 +1253,7 @@ static int file_size(struct dso *dso, struct machine *machine)
 
 out:
 	mutex_unlock(dso__data_open_lock());
+	dso__put_deferred();
 	return ret;
 }
 
@@ -1405,6 +1471,7 @@ uint16_t dso__e_machine_endian(struct dso *dso, struct machine *machine, uint32_
 		*e_flags = 0;
 
 	mutex_unlock(dso__data_open_lock());
+	dso__put_deferred();
 	return e_machine;
 }
 
-- 
2.55.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 5/5] perf dso: Defer dropping the open list reference until after the lock
  2026-09-03 13:22 ` [PATCH 5/5] perf dso: Defer dropping the open list reference until after the lock Arnaldo Carvalho de Melo
@ 2026-09-03 16:40   ` Ian Rogers
  0 siblings, 0 replies; 9+ messages in thread
From: Ian Rogers @ 2026-09-03 16:40 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Namhyung Kim, Ingo Molnar, Thomas Gleixner, James Clark,
	Jiri Olsa, Adrian Hunter, Clark Williams, linux-kernel,
	linux-perf-users, Arnaldo Carvalho de Melo, sashiko-bot,
	Stephane Eranian

On Thu, Sep 3, 2026 at 6:23 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> There was a problem in the code with some resources potentially being
> left unbalanced, and the logic on dso__data_close() becoming confused
> if the fd had been closed already.
>
> The reference taken by dso__list_add() on the open list cannot be
> dropped while holding the open lock: dso__put() may call
> dso__data_close(), which takes dso__data_open_lock() itself,
> deadlocking and leaving the list and its counter inconsistent for
> concurrent threads.
>
> Fix it by changing dso__list_del() to transfer the reference to a
> deferred node, drained by dso__put_deferred() right after every
> unlock of dso__data_open_lock().  Since the counter is now decremented
> under the open lock, do_open()'s close_first_dso() no longer races
> with a stale count.

So I'm not a fan of this change due to its complexity. There reference
counting with dso_data is funny, see:
https://lore.kernel.org/r/20240506180104.485674-5-irogers@google.com
Basically a dso has a dso_data embedded within it. Perhaps the cleaner
fix is to allocate the dso_data, separate from the dso, and have a
reference to the dso from the dso_data. We should be able to scope our
lock usage and avoid deadlock without resorting to a deferral
mechanism.

Thanks,
Ian

> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
> Cc: Stephane Eranian <eranian@google.com>
> Assisted-by: LLM
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/util/dso.c | 75 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 71 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 42bfe30a3b518e80..a4b2361bc7420084 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -531,18 +531,79 @@ static void dso__list_add(struct dso *dso) EXCLUSIVE_LOCKS_REQUIRED(_dso__data_o
>         dso__data_open_cnt++;
>  }
>
> +#ifdef REFCNT_CHECKING
> +/*
> + * A deferred put: carries the reference taken by dso__list_add() for an
> + * entry removed from dso__data_open.  Dedicated nodes are used so that
> + * the dso_data's own open_entry node can be relinked by a concurrent
> + * dso__list_add() without corrupting this list or its reference.
> + */
> +struct dso_data_put {
> +       struct list_head entry;
> +       struct dso *dso;
> +};
> +static LIST_HEAD(dso__data_open_put);
> +#endif
> +
>  static void dso__list_del(struct dso *dso) EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)
>  {
> -       list_del_init(&dso__data(dso)->open_entry);
>  #ifdef REFCNT_CHECKING
> -       mutex_unlock(dso__data_open_lock());
> -       dso__put(dso__data(dso)->dso);
> -       mutex_lock(dso__data_open_lock());
> +       struct dso_data_put *put;
>  #endif
> +
> +       list_del_init(&dso__data(dso)->open_entry);
>         WARN_ONCE(dso__data_open_cnt <= 0,
>                   "DSO data fd counter out of bounds.");
>         dso__data_open_cnt--;
> +#ifdef REFCNT_CHECKING
> +       /*
> +        * The reference taken in dso__list_add() cannot be dropped while
> +        * holding the open lock: dso__put() may call dso__data_close(),
> +        * which takes dso__data_open_lock itself, deadlocking and leaving
> +        * the list/counter state inconsistent for concurrent threads.
> +        * Transfer the reference to a deferred node drained by
> +        * dso__put_deferred() once the lock is released.
> +        */
> +       put = zalloc(sizeof(*put));
> +
> +       if (put == NULL)
> +               return;
> +
> +       put->dso = dso__data(dso)->dso;
> +       dso__data(dso)->dso = NULL;
> +       list_add_tail(&put->entry, &dso__data_open_put);
> +#endif
> +}
> +
> +#ifdef REFCNT_CHECKING
> +/*
> + * Drop the references deferred by dso__list_del().  Must be called
> + * without holding dso__data_open_lock: dso__put() may re-enter it via
> + * dso__data_close().
> + */
> +static void dso__put_deferred(void) LOCKS_EXCLUDED(_dso__data_open_lock)
> +{
> +       for (;;) {
> +               struct dso_data_put *put;
> +               struct dso *dso;
> +
> +               mutex_lock(dso__data_open_lock());
> +               put = list_first_entry_or_null(&dso__data_open_put, struct dso_data_put, entry);
> +               if (put == NULL) {
> +                       mutex_unlock(dso__data_open_lock());
> +                       return;
> +               }
> +               list_del_init(&put->entry);
> +               dso = put->dso;
> +               mutex_unlock(dso__data_open_lock());
> +
> +               free(put);
> +               dso__put(dso);
> +       }
>  }
> +#else
> +static void dso__put_deferred(void) {}
> +#endif
>
>  static void close_first_dso(void);
>
> @@ -805,6 +866,7 @@ void dso__data_close(struct dso *dso)
>         mutex_lock(dso__data_open_lock());
>         close_dso(dso);
>         mutex_unlock(dso__data_open_lock());
> +       dso__put_deferred();
>  }
>
>  static void try_to_open_dso(struct dso *dso, struct machine *machine)
> @@ -865,12 +927,14 @@ bool dso__data_get_fd(struct dso *dso, struct machine *machine, int *fd)
>                 return true;
>
>         mutex_unlock(dso__data_open_lock());
> +       dso__put_deferred();
>         return false;
>  }
>
>  void dso__data_put_fd(struct dso *dso __maybe_unused)
>  {
>         mutex_unlock(dso__data_open_lock());
> +       dso__put_deferred();
>  }
>
>  bool dso__data_status_seen(struct dso *dso, enum dso_data_status_seen by)
> @@ -1058,6 +1122,7 @@ static ssize_t file_read(struct dso *dso, struct machine *machine,
>         ret = pread(dso__data(dso)->fd, data, DSO__DATA_CACHE_SIZE, offset);
>  out:
>         mutex_unlock(dso__data_open_lock());
> +       dso__put_deferred();
>         return ret;
>  }
>
> @@ -1188,6 +1253,7 @@ static int file_size(struct dso *dso, struct machine *machine)
>
>  out:
>         mutex_unlock(dso__data_open_lock());
> +       dso__put_deferred();
>         return ret;
>  }
>
> @@ -1405,6 +1471,7 @@ uint16_t dso__e_machine_endian(struct dso *dso, struct machine *machine, uint32_
>                 *e_flags = 0;
>
>         mutex_unlock(dso__data_open_lock());
> +       dso__put_deferred();
>         return e_machine;
>  }
>
> --
> 2.55.0
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/5] perf jitdump: Free unwinding data even when eh_frame_hdr_size is zero
  2026-09-03 13:22 ` [PATCH 3/5] perf jitdump: Free unwinding data even when eh_frame_hdr_size is zero Arnaldo Carvalho de Melo
@ 2026-09-03 17:05   ` Ian Rogers
  0 siblings, 0 replies; 9+ messages in thread
From: Ian Rogers @ 2026-09-03 17:05 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Namhyung Kim, Ingo Molnar, Thomas Gleixner, James Clark,
	Jiri Olsa, Adrian Hunter, Clark Williams, linux-kernel,
	linux-perf-users, Arnaldo Carvalho de Melo, sashiko-bot,
	Stephane Eranian

On Thu, Sep 3, 2026 at 6:23 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> jit_repipe_code_load() only cleared the unwinding state when both
> unwinding_data and eh_frame_hdr_size were set.  When a record carries
> unwinding data but eh_frame_hdr_size is 0, the cleanup condition fails
> and the unwinding state persists in jd, being applied to all subsequent
> JIT_CODE_LOAD and JIT_CODE_MOVE records, duplicating unwinding sections
> in the generated ELF files and inflating their event->mmap2.len.
>
> The record is validated upstream so eh_frame_hdr_size <= unwinding_size
> always holds.  Free the unwinding data based on the data pointer alone.
>
> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
> Cc: Stephane Eranian <eranian@google.com>
> Assisted-by: LLM
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/jitdump.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index e0d5cc9a828189a2..efb40d93e33ae664 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -530,7 +530,7 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd, union jr_entry *jr)
>                 jd->nr_debug_entries = 0;
>         }
>
> -       if (jd->unwinding_data && jd->eh_frame_hdr_size) {
> +       if (jd->unwinding_data) {
>                 zfree(&jd->unwinding_data);
>                 jd->eh_frame_hdr_size = 0;
>                 jd->unwinding_mapped_size = 0;
> --
> 2.55.0
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/5] perf jitdump: Byte-swap debug entries via unaligned-safe accessors
  2026-09-04 14:40 [PATCH v2 0/5] perf jitdump: Fix debug entry access, unwinding state and sample id sizing Arnaldo Carvalho de Melo
@ 2026-09-04 14:40 ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2026-09-04 14:40 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Ingo Molnar, Thomas Gleixner, James Clark, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Clark Williams, linux-kernel, linux-perf-users,
	Arnaldo Carvalho de Melo, sashiko-bot, Stephane Eranian

From: Arnaldo Carvalho de Melo <acme@redhat.com>

debug_entry records are packed with a variable-length name[] field, so
entries after the first may start at addresses that are not naturally
aligned for their u64 addr and int lineno/discrim fields.  On strict
alignment architectures the byte-swap loop in jit_get_next_entry()
performed misaligned 64-bit loads and stores through struct member
access, which is undefined behavior.

Use get_unaligned()/put_unaligned() for the byte-swap of each field.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
Assisted-by: LLM
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/jitdump.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
index d25a9fe9b020ce87..e0d5cc9a828189a2 100644
--- a/tools/perf/util/jitdump.c
+++ b/tools/perf/util/jitdump.c
@@ -14,6 +14,8 @@
 #include <sys/stat.h>
 #include <sys/mman.h>
 #include <linux/stringify.h>
+#include <linux/kernel.h>
+#include <linux/unaligned.h>
 
 #include "event.h"
 #include "debug.h"
@@ -343,9 +345,14 @@ jit_get_next_entry(struct jit_buf_desc *jd)
 				/* name must be NUL-terminated within the record */
 				if (!memchr(ent->name, '\0', (char *)end - ent->name))
 					break;
-				ent->addr    = bswap_64(ent->addr);
-				ent->lineno  = bswap_32(ent->lineno);
-				ent->discrim = bswap_32(ent->discrim);
+				/*
+				 * debug entries are packed with a variable-length
+				 * name[], so entries after the first may be
+				 * unaligned: byte-swap via unaligned-safe accessors.
+				 */
+				put_unaligned(bswap_64(get_unaligned(&ent->addr)), &ent->addr);
+				put_unaligned(bswap_32(get_unaligned(&ent->lineno)), &ent->lineno);
+				put_unaligned(bswap_32(get_unaligned(&ent->discrim)), &ent->discrim);
 				ent = debug_entry_next(ent);
 			}
 			/* clamp so downstream consumers don't overrun */
-- 
2.55.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2026-09-04 14:41 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-03 13:22 [PATCH v1 0/5] perf tools: Fix jitdump and dso handling Arnaldo Carvalho de Melo
2026-09-03 13:22 ` [PATCH 1/5] perf jitdump: Byte-swap debug entries via unaligned-safe accessors Arnaldo Carvalho de Melo
2026-09-03 13:22 ` [PATCH 2/5] perf genelf: Use unaligned-safe accessors for debug entries Arnaldo Carvalho de Melo
2026-09-03 13:22 ` [PATCH 3/5] perf jitdump: Free unwinding data even when eh_frame_hdr_size is zero Arnaldo Carvalho de Melo
2026-09-03 17:05   ` Ian Rogers
2026-09-03 13:22 ` [PATCH 4/5] perf jitdump: Size code_move event allocation with idr_size Arnaldo Carvalho de Melo
2026-09-03 13:22 ` [PATCH 5/5] perf dso: Defer dropping the open list reference until after the lock Arnaldo Carvalho de Melo
2026-09-03 16:40   ` Ian Rogers
2026-09-04 14:40 [PATCH v2 0/5] perf jitdump: Fix debug entry access, unwinding state and sample id sizing Arnaldo Carvalho de Melo
2026-09-04 14:40 ` [PATCH 1/5] perf jitdump: Byte-swap debug entries via unaligned-safe accessors Arnaldo Carvalho de Melo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®