[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

[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 1/5] perf jitdump: Byte-swap debug entries via unaligned-safe accessors

[sashiko-bot]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903132251.237029-1-acme@kernel.org?part=1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Changing `last_line` and `lineno` to `unsigned int` causes negative line number advances to be incorrectly zero-extended to large positive values on 64-bit architectures, corrupting the DWARF line number program.

Pre-existing issues:
- [High] Unchecked realloc failure in buffer_ext_add() leads to exploitable out-of-bounds heap writes.
--

commit 4a5e45a87437dd59a172031050c168bbb7f8ce78
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf genelf: Use unaligned-safe accessors for debug entries

This patch modifies the perf genelf utility to use unaligned-safe accessors
for debug entry address and line number fields. This prevents undefined
behavior on strict-alignment architectures, as debug_entry records pack
a variable-length string field, leaving subsequent entries unaligned.

> diff --git a/tools/perf/util/genelf_debug.c b/tools/perf/util/genelf_debug.c
> index 8588b3e35e008..8244d3c64103b 100644
> --- a/tools/perf/util/genelf_debug.c
> +++ b/tools/perf/util/genelf_debug.c
> @@ -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;

[Severity: Medium]
Will this zero-extend instead of sign-extend on backward line jumps?

Because lineno and last_line were changed from int to unsigned int,
their subtraction will wrap around to a large 32-bit unsigned value (e.g.,
0xFFFFFFEC for -20).

When this large unsigned value is passed to emit_advance_lineno, which
expects a 64-bit signed long, it implicitly zero-extends on 64-bit
architectures instead of sign-extending. This results in encoding a massive
forward line jump rather than a backward jump, potentially corrupting DWARF
line number mapping.


I also noticed a potential out-of-bounds write during buffer operations here:

tools/perf/util/genelf_debug.c:add_debug_line() {
    ...
	buffer_ext_add(be, (void *)&default_debug_line_header,
		 sizeof(default_debug_line_header));

	buffer_ext_add(be, &standard_opcode_length,  sizeof(standard_opcode_length));

	// empty directory entry
	buffer_ext_add(be, (void *)"", 1);

	// empty filename directory
	buffer_ext_add(be, (void *)"", 1);

	dbg_header = buffer_ext_addr(be) + old_size;
	dbg_header->prolog_length = (buffer_ext_size(be) - old_size) -
		offsetof(struct debug_line_header, minimum_instruction_length);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does this code handle
buffer_ext_add() allocation failures correctly?

If realloc fails inside buffer_ext_add(), it returns -1 without expanding
the buffer. The caller ignores this return value and proceeds to dereference
the computed pointer dbg_header.

If the buffer was completely full when realloc failed, this points exactly
to the end of the heap chunk, leading to unconditionally writing beyond the
allocated bounds.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903132251.237029-1-acme@kernel.org?part=2

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Unsafe open() call in jit_emit_elf allows arbitrary file overwrite via symlink attack.
- [Medium] Stale unwinding and debug data persist on malformed JIT records due to ignored return values and missing cleanup on early returns.
--

commit 57898e6821f20c63014ed2610a5c18278ca4444d
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf jitdump: Free unwinding data even when eh_frame_hdr_size is zero

This commit modifies jit_repipe_code_load() to unconditionally free
the unwinding data if it is present. Previously, it only freed the data
if eh_frame_hdr_size was also non-zero, which caused unwinding sections
to be duplicated and mmap2 lengths to be inflated.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index e0d5cc9a82818..efb40d93e33ae 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;

[Severity: Medium]
This isn't a bug introduced by this patch, but earlier in
jit_repipe_code_load() we return without cleaning up state:

tools/perf/util/jitdump.c:jit_repipe_code_load() {
    ...
	if (jr->load.p.total_size < sizeof(jr->load) ||
	    jr->load.code_size > jr->load.p.total_size - sizeof(jr->load) ||
	    jr->load.code_size > INT_MAX) {
		pr_warning("jitdump: invalid code_size %" PRIu64 " (total_size=%u) in code_load record\n",
			   (uint64_t)jr->load.code_size, jr->load.p.total_size);
		return -1;
	}
    ...
}

If a malformed code load record triggers this early error return, the
pending unwinding and debug data aren't freed or cleared.

Is it expected that jit_process_dump() continues parsing when this happens?

tools/perf/util/jitdump.c:jit_process_dump() {
    ...
	while ((jr = jit_get_next_entry(jd))) {
		switch(jr->prefix.id) {
		case JIT_CODE_LOAD:
			ret = jit_repipe_code_load(jd, jr);
			break;
    ...
}

Because the return value is captured but not evaluated for loop termination,
can this stale unwinding and debug data be erroneously applied to subsequent
valid events?

Could an attacker supply a crafted perf.data file with malformed records
to trigger this state corruption?

[Severity: High]
This is a pre-existing issue, but does this open() call allow an arbitrary
file overwrite?

tools/perf/util/jitdump.c:jit_emit_elf() {
    ...
	nsinfo__mountns_enter(jd->nsi, &nsc);
	fd = open(filename, O_CREAT|O_TRUNC|O_WRONLY, 0644);
	saved_errno = errno;
	nsinfo__mountns_exit(&nsc);
    ...
}

Since the filename path is predictable and based on the attacker-controlled
mmap path, the missing O_EXCL and O_NOFOLLOW flags appear to leave this
vulnerable to a symlink attack.

If a local attacker creates a symlink at the predictable output path in a
shared directory pointing to a sensitive file, will a privileged user running
perf inject silently follow the symlink and overwrite the target file
with an attacker-controlled ELF payload?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903132251.237029-1-acme@kernel.org?part=3

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Replacing the hardcoded +16 padding with +idr_size introduces a heap buffer overflow in jit_repipe_code_move() when PERF_SAMPLE_TID is missing but PERF_SAMPLE_TIME is present, and the filename length approaches PATH_MAX. This shares the same root cause as a pre-existing vulnerability in jit_repipe_code_load().
--

commit 631d6cebecdf5de246fc9c8ce57d27948626c5ac
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Subject: perf jitdump: Size code_move event allocation with idr_size

This commit alters the event buffer allocation in jit_repipe_code_move()
to dynamically scale with idr_size rather than using a static padding.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> --- 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);

[Severity: Critical]
Does this regression introduce a heap buffer overflow when setting the event
time?

Looking later in jit_repipe_code_move(), we calculate the id pointer based on
header.size and idr_size:

    id = (void *)((unsigned long)event + event->mmap.header.size - idr_size);
    if (jd->sample_type & PERF_SAMPLE_TID) {
        id->pid  = pid;
        id->tid  = tid;
    }
    if (jd->sample_type & PERF_SAMPLE_TIME)
        id->time = convert_timestamp(jd, jr->load.p.timestamp);

If PERF_SAMPLE_TID is missing from the configured sample type but
PERF_SAMPLE_TIME is present, idr_size evaluates to 8, making the allocation
size become sizeof(*event) + 8.

Because id->time is located at an 8-byte offset within the id structure
(after pid and tid), writing to id->time accesses memory 8 bytes past the
start of id.

When the filename string approaches PATH_MAX and header.size nears the base
allocation size, this writes exactly past the end of the calloc heap
allocation. Could this memory corruption be triggered by parsing a
maliciously crafted perf.data file?

This is a pre-existing issue, but does this exact same pattern also cause
an out-of-bounds write in jit_repipe_code_load()?

Looking at jit_repipe_code_load(), it similarly uses the idr_size for its
allocation limit and unconditionally performs the same timestamp assignment:

tools/perf/util/jitdump.c:jit_repipe_code_load() {
    ...
    event = calloc(1, sizeof(*event) + idr_size);
    ...
    id = (void *)((unsigned long)event + event->mmap.header.size - idr_size);
    if (jd->sample_type & PERF_SAMPLE_TID) {
        id->pid  = pid;
        id->tid  = tid;
    }
    if (jd->sample_type & PERF_SAMPLE_TIME)
        id->time = convert_timestamp(jd, jr->load.p.timestamp);
    ...
}

Should both load and move operations account for the struct offset of
id->time when sizing the buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903132251.237029-1-acme@kernel.org?part=4

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Failing to allocate a deferred put node leaks the reference handle and causes a memory leak of the DSO object.
--

commit a29267f86fa45a10095cc91a7c0350e522de6c4a
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf dso: Defer dropping the open list reference until after the lock

This patch addresses a deadlock and resource imbalance issue by deferring
the dropping of the DSO open list reference. It transfers the reference to a
deferred node during dso__list_del(), which is later drained by
dso__put_deferred() outside of the dso__data_open_lock.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 42bfe30a3b518..a4b2361bc7420 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;

[Severity: Medium]
Does this early return cause a memory leak of the DSO object if zalloc fails?

During dso__list_del(), returning early without placing the deferred put node
onto dso__data_open_put abandons the reference in dso__data(dso)->dso. If the
DSO is subsequently added to the open list again, dso__list_add() could
overwrite this pointer with a new reference, permanently leaking the prior one
and preventing the DSO reference count from ever reaching 0.

> +
> +	put->dso = dso__data(dso)->dso;
> +	dso__data(dso)->dso = NULL;
> +	list_add_tail(&put->entry, &dso__data_open_put);
> +#endif
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903132251.237029-1-acme@kernel.org?part=5

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
>

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
>