[PATCH v1 0/5] perf tools: Fix jitdump and dso handling

[PATCH v1 0/5] perf tools: Fix jitdump and dso handling

[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

[PATCH 1/5] perf jitdump: Byte-swap debug entries via unaligned-safe accessors

[Arnaldo Carvalho de Melo]

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

[PATCH 2/5] perf genelf: Use unaligned-safe accessors for debug entries

[Arnaldo Carvalho de Melo]

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

[PATCH 3/5] perf jitdump: Free unwinding data even when eh_frame_hdr_size is zero

[Arnaldo Carvalho de Melo]

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

Re: [PATCH 3/5] perf jitdump: Free unwinding data even when eh_frame_hdr_size is zero

[Ian Rogers]

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
>

[PATCH 4/5] perf jitdump: Size code_move event allocation with idr_size

[Arnaldo Carvalho de Melo]

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

[PATCH 5/5] perf dso: Defer dropping the open list reference until after the lock

[Arnaldo Carvalho de Melo]

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

Re: [PATCH 5/5] perf dso: Defer dropping the open list reference until after the lock

[Ian Rogers]

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
>

[PATCH v2 0/5] perf jitdump: Fix debug entry access, unwinding state and sample id sizing

[Arnaldo Carvalho de Melo]

Hi,

Five fixes for the jitdump/genelf code path, all reported by the
sashiko-bot AI reviewer while reviewing the hardening series that is now
in perf-tools-next:

  - struct debug_entry ends with a variable-length name[], so entries
    after the first start at addresses that are not naturally aligned.

  - jit_repipe_code_load() only released the unwinding state when both
    unwinding_data and eh_frame_hdr_size were set.

  - the sample id area appended to the synthesized mmap2 records was
    cast to a fixed {u32 pid, tid; u64 time;} struct.  That only matches
    what evsel__id_hdr_size() accounts for when PERF_SAMPLE_TID is set
    as well, since the fields are appended in a fixed order skipping the
    ones not requested.  Patch 4 walks the area in that order and patch
    5 sizes the allocation with idr_size instead of a hardcoded +16, so
    that what is written and what is allocated agree.

Patches 1 and 5 are unchanged from v1.

Best regards,

- Arnaldo

What changed from v1 (599fad82626e5eba):

  PATCH 2/5:
  - Kept lineno and last_line signed.  v1 read ent->lineno into an
    unsigned int, so the "lineno - last_line" delta handed to
    emit_advance_lineno() wrapped to a large positive value and, being
    widened to a long, zero-extended instead of sign-extending: a
    backward line jump became a huge forward one, corrupting the DWARF
    line number program [sashiko-bot review of PATCH 2/5].

  PATCH 3/5:
  - Added Reviewed-by: Ian Rogers.

  PATCH 4/5 (new):
  - Walks the sample id area in the order evsel__id_hdr_size() accounts
    for (TID, TIME, ID, STREAM_ID, CPU, IDENTIFIER — 8 bytes each)
    advancing only past the fields whose sample_type bit is set,.

    This is the [Critical] finding from the sashiko-bot review of v1
    PATCH 4/5: with PERF_SAMPLE_TID unset, PERF_SAMPLE_TIME belongs at
    offset 0 and idr_size is 8, so the old code stored it at offset 8 —
    past the end of the allocation, and corrupting what a reader
    expects to find at offset 0 besides.  It is split out from the
    sizing change so that 5/5 no longer removes the slack that was
    masking it, and so that the same latent bug in
    jit_repipe_code_load() — which already sized with idr_size — is
    fixed in the same place.

  PATCH 5/5 (was 4/5):
  - Unchanged, and now safe: 4/5 above keeps the writes inside idr_size.

  DROPPED (was v1 PATCH 5/5):
  - "perf dso: Defer dropping the open list reference until after the
    lock".  Ian Rogers is not a fan of the deferral mechanism and
    suggested instead allocating dso_data separately from the dso, so
    that the lock can be scoped without it.  Dropped from this series
    while that is worked out.

  - The pre-existing issues sashiko-bot raised that are outside the
    scope of this series are recorded in tools/perf/TODO.hardening for
    follow-up work: item 170 (jit_get_next_entry() lacks per-record
    size validation), 171 (jit_process_dump()/jit_inject() swallow
    callback errors), 172 (jit_emit_elf() truncates 64-bit unwinding
    sizes to u32), 173 (jit_emit_elf() opens a predictable filename
    without O_EXCL/O_NOFOLLOW) and 176 (buffer_ext_add() realloc
    failure ignored by all callers).  The two this series does address
    are items 174 and 153, now marked fixed.

  - Rebased onto the current perf-tools-next head (92d50319b4f0c0bb).

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: Write sample id fields in the order used by
    evsel__id_hdr_size()
  perf jitdump: Size code_move event allocation with idr_size

 tools/perf/util/genelf_debug.c | 28 +++++++-----
 tools/perf/util/jitdump.c      | 84 +++++++++++++++++++++++-----------
 2 files changed, 75 insertions(+), 37 deletions(-)

base-commit: 92d50319b4f0c0bbee8a236a09063272cd22faab
v1-head: 599fad82626e5eba485b4d4d188cbd34c0e12cbc

[PATCH 2/5] perf genelf: Use unaligned-safe accessors for debug entries

[Arnaldo Carvalho de Melo]

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.

struct debug_entry.lineno is signed and emit_advance_lineno() takes a
long line delta that relies on sign extension, so the field is read
into an int: reading it into an unsigned int would turn a backward
line jump into a huge forward one and corrupt the line number program.

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 | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/tools/perf/util/genelf_debug.c b/tools/perf/util/genelf_debug.c
index 8588b3e35e008396..7d9ef43aa6ac4ab6 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;
+	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
@@ -362,6 +366,8 @@ static void emit_lineno_info(struct buffer_ext *be,
 	for (i = 0; i < nr_entry; i++, ent = debug_entry_next(ent)) {
 		int need_copy = 0;
 		ubyte special_opcode;
+		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