[PATCH v2 0/5] Various fixes around undefined behavior

[PATCH v2 0/5] Various fixes around undefined behavior

[Ian Rogers]

Fix various undefined behavior issues, improve tests to make them
easier to diagnose and add assertions so that problems don't recur.

v2: Add Namhyung's acked-by. Drop container_of assert that ptr !=
    NULL, to simplify the series. The bsearch UB fix was picked up as
    a patch by CT:
    https://lore.kernel.org/r/20250303183646.327510-2-ctshao@google.com
    It seems this patch series fell-through the cracks as v1 was
    sent/acked 9 months ago.

v1: https://lore.kernel.org/lkml/20241213210425.526512-1-irogers@google.com/

Ian Rogers (5):
  perf disasm: Avoid undefined behavior in incrementing NULL
  perf test trace_btf_enum: Skip if permissions are insufficient
  perf evsel: Avoid container_of on a NULL leader
  perf test shell lock_contention: Extra debug diagnostics
  libperf event: Ensure tracing data is multiple of 8 sized

 tools/lib/perf/include/perf/event.h       |  1 +
 tools/perf/tests/shell/lock_contention.sh |  7 ++++++-
 tools/perf/tests/shell/trace_btf_enum.sh  | 11 +++++++++++
 tools/perf/util/disasm.c                  |  7 +++++--
 tools/perf/util/evsel.c                   |  2 ++
 5 files changed, 25 insertions(+), 3 deletions(-)

-- 
2.51.0.rc1.193.gad69d77794-goog

[PATCH v2 1/5] perf disasm: Avoid undefined behavior in incrementing NULL

[Ian Rogers]

Incrementing NULL is undefined behavior and triggers ubsan during the
perf annotate test. Split a compound statement over two lines to avoid
this.

Fixes: 98f69a573c66 ("perf annotate: Split out util/disasm.c")
Signed-off-by: Ian Rogers <irogers@google.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/disasm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index b1e4919d016f..e257bd918c89 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -390,13 +390,16 @@ static int jump__parse(struct arch *arch, struct ins_operands *ops, struct map_s
 	 * skip over possible up to 2 operands to get to address, e.g.:
 	 * tbnz	 w0, #26, ffff0000083cd190 <security_file_permission+0xd0>
 	 */
-	if (c++ != NULL) {
+	if (c != NULL) {
+		c++;
 		ops->target.addr = strtoull(c, NULL, 16);
 		if (!ops->target.addr) {
 			c = strchr(c, ',');
 			c = validate_comma(c, ops);
-			if (c++ != NULL)
+			if (c != NULL) {
+				c++;
 				ops->target.addr = strtoull(c, NULL, 16);
+			}
 		}
 	} else {
 		ops->target.addr = strtoull(ops->raw, NULL, 16);
-- 
2.51.0.rc1.193.gad69d77794-goog

Re: [PATCH v2 1/5] perf disasm: Avoid undefined behavior in incrementing NULL

[Collin Funk]

Ian Rogers <irogers@google.com> writes:

> Incrementing NULL is undefined behavior and triggers ubsan during the
> perf annotate test. Split a compound statement over two lines to avoid
> this.
>
> Fixes: 98f69a573c66 ("perf annotate: Split out util/disasm.c")
> Signed-off-by: Ian Rogers <irogers@google.com>
> Acked-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/disasm.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index b1e4919d016f..e257bd918c89 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -390,13 +390,16 @@ static int jump__parse(struct arch *arch, struct ins_operands *ops, struct map_s
>  	 * skip over possible up to 2 operands to get to address, e.g.:
>  	 * tbnz	 w0, #26, ffff0000083cd190 <security_file_permission+0xd0>
>  	 */
> -	if (c++ != NULL) {
> +	if (c != NULL) {
> +		c++;
>  		ops->target.addr = strtoull(c, NULL, 16);
>  		if (!ops->target.addr) {
>  			c = strchr(c, ',');
>  			c = validate_comma(c, ops);
> -			if (c++ != NULL)
> +			if (c != NULL) {
> +				c++;
>  				ops->target.addr = strtoull(c, NULL, 16);
> +			}
>  		}
>  	} else {
>  		ops->target.addr = strtoull(ops->raw, NULL, 16);

It is undefined behavior, but works correctly with GCC and Clang. In
Gnulib, we allow it and suggest using -fno-sanitize=pointer-overflow
instead [1].

But I can understand that is not every projects preference. Therefore,
this change looks good to me.

Reviewed-by: Collin Funk <collin.funk1@gmail.com>

Collin

[1] https://www.gnu.org/software/gnulib/manual/html_node/Unsupported-Platforms.html

[PATCH v2 2/5] perf test trace_btf_enum: Skip if permissions are insufficient

[Ian Rogers]

Modify test behavior to skip if BPF calls fail with "Operation not
permitted".

Fixes: d66763fed30f ("perf test trace_btf_enum: Add regression test for the BTF augmentation of enums in 'perf trace'")
Signed-off-by: Ian Rogers <irogers@google.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/shell/trace_btf_enum.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tools/perf/tests/shell/trace_btf_enum.sh b/tools/perf/tests/shell/trace_btf_enum.sh
index 572001d75d78..03e9f680a4a6 100755
--- a/tools/perf/tests/shell/trace_btf_enum.sh
+++ b/tools/perf/tests/shell/trace_btf_enum.sh
@@ -23,6 +23,14 @@ check_vmlinux() {
   fi
 }
 
+check_permissions() {
+  if perf trace -e $syscall $TESTPROG 2>&1 | grep -q "Operation not permitted"
+  then
+    echo "trace+enum test [Skipped permissions]"
+    err=2
+  fi
+}
+
 trace_landlock() {
   echo "Tracing syscall ${syscall}"
 
@@ -56,6 +64,9 @@ trace_non_syscall() {
 }
 
 check_vmlinux
+if [ $err = 0 ]; then
+  check_permissions
+fi
 
 if [ $err = 0 ]; then
   trace_landlock
-- 
2.51.0.rc1.193.gad69d77794-goog

[PATCH v2 3/5] perf evsel: Avoid container_of on a NULL leader

[Ian Rogers]

An evsel should typically have a leader of itself, however, in tests
like 'Sample parsing' a NULL leader may occur and the container_of
will return a corrupt pointer. Avoid this with an explicit NULL test.

Signed-off-by: Ian Rogers <irogers@google.com>
Fixes: fba7c86601e2 ("libperf: Move 'leader' from tools/perf to perf_evsel::leader")
Acked-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/evsel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index d264c143b592..496f42434327 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -3935,6 +3935,8 @@ bool evsel__is_hybrid(const struct evsel *evsel)
 
 struct evsel *evsel__leader(const struct evsel *evsel)
 {
+	if (evsel->core.leader == NULL)
+		return NULL;
 	return container_of(evsel->core.leader, struct evsel, core);
 }
 
-- 
2.51.0.rc1.193.gad69d77794-goog

[PATCH v2 4/5] perf test shell lock_contention: Extra debug diagnostics

[Ian Rogers]

In test_record_concurrent, as stderr is sent to /dev/null, error
messages are hidden. Change this to gather the error messages and dump
them on failure.

Some minor sh->bash changes to add some more diagnostics in
trap_cleanup.

Signed-off-by: Ian Rogers <irogers@google.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/shell/lock_contention.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/shell/lock_contention.sh b/tools/perf/tests/shell/lock_contention.sh
index d33d9e4392b0..7248a74ca2a3 100755
--- a/tools/perf/tests/shell/lock_contention.sh
+++ b/tools/perf/tests/shell/lock_contention.sh
@@ -7,14 +7,17 @@ set -e
 err=0
 perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
 result=$(mktemp /tmp/__perf_test.result.XXXXX)
+errout=$(mktemp /tmp/__perf_test.errout.XXXXX)
 
 cleanup() {
 	rm -f ${perfdata}
 	rm -f ${result}
+	rm -f ${errout}
 	trap - EXIT TERM INT
 }
 
 trap_cleanup() {
+	echo "Unexpected signal in ${FUNCNAME[1]}"
 	cleanup
 	exit ${err}
 }
@@ -75,10 +78,12 @@ test_bpf()
 test_record_concurrent()
 {
 	echo "Testing perf lock record and perf lock contention at the same time"
-	perf lock record -o- -- perf bench sched messaging -p 2> /dev/null | \
+	perf lock record -o- -- perf bench sched messaging -p 2> ${errout} | \
 	perf lock contention -i- -E 1 -q 2> ${result}
 	if [ "$(cat "${result}" | wc -l)" != "1" ]; then
 		echo "[Fail] Recorded result count is not 1:" "$(cat "${result}" | wc -l)"
+		cat ${errout}
+		cat ${result}
 		err=1
 		exit
 	fi
-- 
2.51.0.rc1.193.gad69d77794-goog

[PATCH v2 5/5] libperf event: Ensure tracing data is multiple of 8 sized

[Ian Rogers]

Perf's synthetic-events.c will ensure 8-byte alignment of tracing
data, writing it after a perf_record_header_tracing_data event. Add
padding to struct perf_record_header_tracing_data to make it 16-byte
rather than 12-byte sized.

Fixes: 055c67ed3988 ("perf tools: Move event synthesizing routines to separate .c file")
Signed-off-by: Ian Rogers <irogers@google.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/perf/include/perf/event.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/lib/perf/include/perf/event.h b/tools/lib/perf/include/perf/event.h
index 6608f1e3701b..aa1e91c97a22 100644
--- a/tools/lib/perf/include/perf/event.h
+++ b/tools/lib/perf/include/perf/event.h
@@ -291,6 +291,7 @@ struct perf_record_header_event_type {
 struct perf_record_header_tracing_data {
 	struct perf_event_header header;
 	__u32			 size;
+	__u32			 pad;
 };
 
 #define PERF_RECORD_MISC_BUILD_ID_SIZE (1 << 15)
-- 
2.51.0.rc1.193.gad69d77794-goog

Re: [PATCH v2 0/5] Various fixes around undefined behavior

[James Clark]

On 21/08/2025 5:38 pm, Ian Rogers wrote:
> Fix various undefined behavior issues, improve tests to make them
> easier to diagnose and add assertions so that problems don't recur.
> 
> v2: Add Namhyung's acked-by. Drop container_of assert that ptr !=
>      NULL, to simplify the series. The bsearch UB fix was picked up as
>      a patch by CT:
>      https://lore.kernel.org/r/20250303183646.327510-2-ctshao@google.com
>      It seems this patch series fell-through the cracks as v1 was
>      sent/acked 9 months ago.
> 
> v1: https://lore.kernel.org/lkml/20241213210425.526512-1-irogers@google.com/
> 
> Ian Rogers (5):
>    perf disasm: Avoid undefined behavior in incrementing NULL
>    perf test trace_btf_enum: Skip if permissions are insufficient
>    perf evsel: Avoid container_of on a NULL leader
>    perf test shell lock_contention: Extra debug diagnostics
>    libperf event: Ensure tracing data is multiple of 8 sized
> 
>   tools/lib/perf/include/perf/event.h       |  1 +
>   tools/perf/tests/shell/lock_contention.sh |  7 ++++++-
>   tools/perf/tests/shell/trace_btf_enum.sh  | 11 +++++++++++
>   tools/perf/util/disasm.c                  |  7 +++++--
>   tools/perf/util/evsel.c                   |  2 ++
>   5 files changed, 25 insertions(+), 3 deletions(-)
> 

Reviewed-by: James Clark <james.clark@linaro.org>

Re: [PATCH v2 0/5] Various fixes around undefined behavior

[Arnaldo Carvalho de Melo]

On Tue, Aug 26, 2025 at 09:38:22AM +0100, James Clark wrote:
> On 21/08/2025 5:38 pm, Ian Rogers wrote:
> > Fix various undefined behavior issues, improve tests to make them
> > easier to diagnose and add assertions so that problems don't recur.
 
> Reviewed-by: James Clark <james.clark@linaro.org>

Thanks, applied to perf-tools-next,

- Arnaldo