[PATCH v2 0/4] perf file align features, avoid UB

[PATCH v2 0/4] perf file align features, avoid UB

[Ian Rogers]

Features like hostname may not be 8-byte aligned. Add padding to make
the feature aligned to avoid undefined behavior (UB). Modify the
writing of the CPU topology features to always write die_cpus_list
even if empty, as the size could have been aligned and not allow the
missing list to be detected. Avoid UB also in machine where a NULL
may be incremented.

v2: Fix CPU topology as described in replies to v1.
v1: https://lore.kernel.org/lkml/20241212080530.1329601-1-irogers@google.com/

Ian Rogers (4):
  perf header: Write out even empty die_cpus_list
  perf synthetic-events: Ensure features are aligned
  perf machine: Avoid UB by delaying computing branch entries
  perf record: Assert synthesized events are 8-byte aligned

 tools/perf/builtin-record.c        |  5 ++++-
 tools/perf/util/header.c           | 10 ++++------
 tools/perf/util/machine.c          |  2 +-
 tools/perf/util/synthetic-events.c |  2 ++
 4 files changed, 11 insertions(+), 8 deletions(-)

-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v2 1/4] perf header: Write out even empty die_cpus_list

[Ian Rogers]

Reading the CPU topology is tolerant to the dies_cpus_list being
missing by using the feature's size in the data file/pipe. However,
the feature's size is just the header size and may be
unaligned. Making the header size aligned breaks detecting a missing
die_cpus_list. To allow the header size to be aligned always write the
die_cpus_lists even if empty.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/header.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 048e563069bc..03e43a9894d4 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -628,9 +628,6 @@ static int write_cpu_topology(struct feat_fd *ff,
 			return ret;
 	}
 
-	if (!tp->die_cpus_lists)
-		goto done;
-
 	ret = do_write(ff, &tp->die_cpus_lists, sizeof(tp->die_cpus_lists));
 	if (ret < 0)
 		goto done;
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v2 2/4] perf synthetic-events: Ensure features are aligned

[Ian Rogers]

Features like hostname have arbitrary size and break the assumed
8-byte alignment of perf events. Pad all feature events until 8-byte
alignment is restored.

Rather than invent a new mechanism, reuse write_padded but pass an
empty buffer as what to write. As no alignment may be necessary,
update write_padded to be tolerant of 0 as a count and count_aligned
value.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/header.c           | 7 ++++---
 tools/perf/util/synthetic-events.c | 2 ++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 03e43a9894d4..d10703090e83 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -168,11 +168,12 @@ int write_padded(struct feat_fd *ff, const void *bf,
 		 size_t count, size_t count_aligned)
 {
 	static const char zero_buf[NAME_ALIGN];
-	int err = do_write(ff, bf, count);
+	int err = count > 0 ? do_write(ff, bf, count) : 0;
 
-	if (!err)
+	if (!err && count_aligned > count) {
+		assert(count_aligned - count < sizeof(zero_buf));
 		err = do_write(ff, zero_buf, count_aligned - count);
-
+	}
 	return err;
 }
 
diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index f8ac2ac2da45..ee6e9c2fb11b 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -2401,6 +2401,8 @@ int perf_event__synthesize_features(const struct perf_tool *tool, struct perf_se
 			pr_debug("Error writing feature\n");
 			continue;
 		}
+		write_padded(&ff, /*bf=*/NULL, /*count=*/0,
+			     /*count_aligned=*/PERF_ALIGN(ff.offset, sizeof(u64)) - ff.offset);
 		/* ff.buf may have changed due to realloc in do_write() */
 		fe = ff.buf;
 		memset(fe, 0, sizeof(*fe));
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v2 3/4] perf machine: Avoid UB by delaying computing branch entries

[Ian Rogers]

If the branch_stack is NULL then perf_sample__branch_entries may
return NULL+1 which triggers ubsan (undefined behavior
sanitizer). Avoid this by making the computation conditional on branch
existing.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/machine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 84661d950104..7b1e1c254c17 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -2670,7 +2670,6 @@ static int thread__resolve_callchain_sample(struct thread *thread,
 					    bool symbols)
 {
 	struct branch_stack *branch = sample->branch_stack;
-	struct branch_entry *entries = perf_sample__branch_entries(sample);
 	struct ip_callchain *chain = sample->callchain;
 	int chain_nr = 0;
 	u8 cpumode = PERF_RECORD_MISC_USER;
@@ -2712,6 +2711,7 @@ static int thread__resolve_callchain_sample(struct thread *thread,
 	 */
 
 	if (branch && callchain_param.branch_callstack) {
+		struct branch_entry *entries = perf_sample__branch_entries(sample);
 		int nr = min(max_stack, (int)branch->nr);
 		struct branch_entry be[nr];
 		struct iterations iter[nr];
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v2 4/4] perf record: Assert synthesized events are 8-byte aligned

[Ian Rogers]

Capture that events are 8-byte aligned and avoid later misaligned
event problems.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-record.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index adbaf80b398c..a5689d0e93ad 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -624,7 +624,10 @@ static int process_synthesized_event(const struct perf_tool *tool,
 				     struct machine *machine __maybe_unused)
 {
 	struct record *rec = container_of(tool, struct record, tool);
-	return record__write(rec, NULL, event, event->header.size);
+	size_t size = event->header.size;
+
+	assert(PERF_ALIGN(size, sizeof(u64)) == size);
+	return record__write(rec, NULL, event, size);
 }
 
 static struct mutex synth_lock;
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v2 1/4] perf header: Write out even empty die_cpus_list

[Ian Rogers]

On Sun, Dec 15, 2024 at 5:46 PM Ian Rogers <irogers@google.com> wrote:
>
> Reading the CPU topology is tolerant to the dies_cpus_list being
> missing by using the feature's size in the data file/pipe. However,
> the feature's size is just the header size and may be
> unaligned. Making the header size aligned breaks detecting a missing
> die_cpus_list. To allow the header size to be aligned always write the
> die_cpus_lists even if empty.
>
> Signed-off-by: Ian Rogers <irogers@google.com>

This change breaks the "Session topology" test on ARM with "Cpu map -
Die ID doesn't match" . One die id is -1 and the other is 0. I'll look
into addressing this in v3.

Thanks,
Ian