[PATCH v1 0/8] Various fixes around undefined behavior

[PATCH v1 0/8] 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.

Ian Rogers (8):
  perf disasm: Avoid undefined behavior in incrementing NULL
  perf test trace_btf_enum: Skip if permissions are insufficient
  tools headers: Update offsetof and container_of
  perf evsel: Avoid container_of on a NULL leader
  tools headers: Add non-NULL assert to container_of
  perf maps: Avoid UB passing NULL to bsearch
  perf test shell lock_contention: Extra debug diagnostics
  libperf event: Ensure tracing data is multiple of 8 sized

 tools/include/linux/kernel.h              | 16 ++++++++++------
 tools/lib/perf/include/perf/event.h       |  1 +
 tools/perf/tests/shell/lock_contention.sh |  9 +++++++--
 tools/perf/tests/shell/trace_btf_enum.sh  | 11 +++++++++++
 tools/perf/util/disasm.c                  |  7 +++++--
 tools/perf/util/evsel.c                   |  2 ++
 tools/perf/util/maps.c                    |  9 ++++++---
 7 files changed, 42 insertions(+), 13 deletions(-)

-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v1 1/8] 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>
---
 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 2c8063660f2e..e8e7a8257007 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -389,13 +389,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.47.1.613.gc27f4b7a9f-goog

[PATCH v1 2/8] 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>
---
 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 5a3b8a5a9b5c..737da3113966 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}"
 
@@ -50,6 +58,9 @@ trace_non_syscall() {
 }
 
 check_vmlinux
+if [ $err = 0 ]; then
+  check_permissions
+fi
 
 if [ $err = 0 ]; then
   trace_landlock
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v1 3/8] tools headers: Update offsetof and container_of

[Ian Rogers]

Update to match kernel definitions in `include/linux/stddef.h` and
`include/linux/container_of.h`.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/include/linux/kernel.h | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/tools/include/linux/kernel.h b/tools/include/linux/kernel.h
index 07cfad817d53..10f74f021d55 100644
--- a/tools/include/linux/kernel.h
+++ b/tools/include/linux/kernel.h
@@ -21,9 +21,8 @@
 #define PERF_ALIGN(x, a)	__PERF_ALIGN_MASK(x, (typeof(x))(a)-1)
 #define __PERF_ALIGN_MASK(x, mask)	(((x)+(mask))&~(mask))
 
-#ifndef offsetof
-#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
-#endif
+#undef offsetof
+#define offsetof(TYPE, MEMBER)	__builtin_offsetof(TYPE, MEMBER)
 
 #ifndef container_of
 /**
@@ -32,10 +31,14 @@
  * @type:	the type of the container struct this is embedded in.
  * @member:	the name of the member within the struct.
  *
+ * WARNING: any const qualifier of @ptr is lost.
  */
-#define container_of(ptr, type, member) ({			\
-	const typeof(((type *)0)->member) * __mptr = (ptr);	\
-	(type *)((char *)__mptr - offsetof(type, member)); })
+#define container_of(ptr, type, member) ({				\
+	void *__mptr = (void *)(ptr);					\
+	static_assert(__same_type(*(ptr), ((type *)0)->member) ||	\
+		      __same_type(*(ptr), void),			\
+		      "pointer type mismatch in container_of()");	\
+	((type *)(__mptr - offsetof(type, member))); })
 #endif
 
 #ifndef max
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v1 4/8] 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")
---
 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 cb6952b6d9d9..dc427eafca6c 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -3327,6 +3327,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.47.1.613.gc27f4b7a9f-goog

[PATCH v1 5/8] tools headers: Add non-NULL assert to container_of

[Ian Rogers]

container_of on a NULL may yield a non-NULL, confusion and errors.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/include/linux/kernel.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/include/linux/kernel.h b/tools/include/linux/kernel.h
index 10f74f021d55..8d617467c5c5 100644
--- a/tools/include/linux/kernel.h
+++ b/tools/include/linux/kernel.h
@@ -38,6 +38,7 @@
 	static_assert(__same_type(*(ptr), ((type *)0)->member) ||	\
 		      __same_type(*(ptr), void),			\
 		      "pointer type mismatch in container_of()");	\
+	assert(ptr != NULL);						\
 	((type *)(__mptr - offsetof(type, member))); })
 #endif
 
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v1 6/8] perf maps: Avoid UB passing NULL to bsearch

[Ian Rogers]

If maps_by_address is NULL it is UB to pass it to bsearch, and will
trigger ubsan, even if the nr_maps is 0.

Fixes: 659ad3492b91 ("perf maps: Switch from rbtree to lazily sorted array for addresses")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/maps.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
index 432399cbe5dd..1830ae776052 100644
--- a/tools/perf/util/maps.c
+++ b/tools/perf/util/maps.c
@@ -1042,10 +1042,13 @@ struct map *maps__find(struct maps *maps, u64 ip)
 	while (!done) {
 		down_read(maps__lock(maps));
 		if (maps__maps_by_address_sorted(maps)) {
-			struct map **mapp =
-				bsearch(&ip, maps__maps_by_address(maps), maps__nr_maps(maps),
-					sizeof(*mapp), map__addr_cmp);
+			struct map **mapp = NULL;
 
+			if (maps__maps_by_address(maps)) {
+				mapp = bsearch(&ip, maps__maps_by_address(maps),
+					       maps__nr_maps(maps), sizeof(*mapp),
+					       map__addr_cmp);
+			}
 			if (mapp)
 				result = map__get(*mapp);
 			done = true;
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v1 7/8] 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>
---
 tools/perf/tests/shell/lock_contention.sh | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/shell/lock_contention.sh b/tools/perf/tests/shell/lock_contention.sh
index c1ec5762215b..43c2438f6153 100755
--- a/tools/perf/tests/shell/lock_contention.sh
+++ b/tools/perf/tests/shell/lock_contention.sh
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash
 # kernel lock contention analysis test
 # SPDX-License-Identifier: GPL-2.0
 
@@ -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 2> /dev/null | \
+	perf lock record -o- -- perf bench sched messaging 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.47.1.613.gc27f4b7a9f-goog

[PATCH v1 8/8] 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>
---
 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 37bb7771d914..32b75c0326c9 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.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v1 0/8] Various fixes around undefined behavior

[Namhyung Kim]

On Fri, Dec 13, 2024 at 01:04:17PM -0800, 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.
> 
> Ian Rogers (8):
>   perf disasm: Avoid undefined behavior in incrementing NULL
>   perf test trace_btf_enum: Skip if permissions are insufficient
>   tools headers: Update offsetof and container_of
>   perf evsel: Avoid container_of on a NULL leader
>   tools headers: Add non-NULL assert to container_of
>   perf maps: Avoid UB passing NULL to bsearch
>   perf test shell lock_contention: Extra debug diagnostics
>   libperf event: Ensure tracing data is multiple of 8 sized

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

> 
>  tools/include/linux/kernel.h              | 16 ++++++++++------
>  tools/lib/perf/include/perf/event.h       |  1 +
>  tools/perf/tests/shell/lock_contention.sh |  9 +++++++--
>  tools/perf/tests/shell/trace_btf_enum.sh  | 11 +++++++++++
>  tools/perf/util/disasm.c                  |  7 +++++--
>  tools/perf/util/evsel.c                   |  2 ++
>  tools/perf/util/maps.c                    |  9 ++++++---
>  7 files changed, 42 insertions(+), 13 deletions(-)
> 
> -- 
> 2.47.1.613.gc27f4b7a9f-goog
> 

Re: [PATCH v1 1/8] perf disasm: Avoid undefined behavior in incrementing NULL

[Kuan-Wei Chiu]

On Fri, Dec 13, 2024 at 01:04:18PM -0800, Ian Rogers wrote:
> 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>

Reviewed-by: Kuan-Wei Chiu <visitorckw@gmail.com>

Regards,
Kuan-Wei

Re: [PATCH v1 3/8] tools headers: Update offsetof and container_of

[Kuan-Wei Chiu]

On Fri, Dec 13, 2024 at 01:04:20PM -0800, Ian Rogers wrote:
> Update to match kernel definitions in `include/linux/stddef.h` and
> `include/linux/container_of.h`.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>

Reviewed-by: Kuan-Wei Chiu <visitorckw@gmail.com>

Regards,
Kuan-Wei

Re: [PATCH v1 5/8] tools headers: Add non-NULL assert to container_of

[Kuan-Wei Chiu]

On Fri, Dec 13, 2024 at 01:04:22PM -0800, Ian Rogers wrote:
> container_of on a NULL may yield a non-NULL, confusion and errors.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>

Reviewed-by: Kuan-Wei Chiu <visitorckw@gmail.com>

Regards,
Kuan-Wei

Re: [PATCH v1 6/8] perf maps: Avoid UB passing NULL to bsearch

[Kuan-Wei Chiu]

On Fri, Dec 13, 2024 at 01:04:23PM -0800, Ian Rogers wrote:
> If maps_by_address is NULL it is UB to pass it to bsearch, and will
> trigger ubsan, even if the nr_maps is 0.
> 
> Fixes: 659ad3492b91 ("perf maps: Switch from rbtree to lazily sorted array for addresses")
> Signed-off-by: Ian Rogers <irogers@google.com>

Reviewed-by: Kuan-Wei Chiu <visitorckw@gmail.com>

Regards,
Kuan-Wei