[PATCH v1 1/4] perf record: Delete session after stopping sideband thread

[PATCH v1 1/4] perf record: Delete session after stopping sideband thread

[Ian Rogers]

The session has a header in it which contains a perf env with
bpf_progs. The bpf_progs are accessed by the sideband thread and so
the sideband thread must be stopped before the session is deleted, to
avoid a use after free.  This error was detected by AddressSanitizer
in the following:

```
==2054673==ERROR: AddressSanitizer: heap-use-after-free on address 0x61d000161e00 at pc 0x55769289de54 bp 0x7f9df36d4ab0 sp 0x7f9df36d4aa8
READ of size 8 at 0x61d000161e00 thread T1
    #0 0x55769289de53 in __perf_env__insert_bpf_prog_info util/env.c:42
    #1 0x55769289dbb1 in perf_env__insert_bpf_prog_info util/env.c:29
    #2 0x557692bbae29 in perf_env__add_bpf_info util/bpf-event.c:483
    #3 0x557692bbb01a in bpf_event__sb_cb util/bpf-event.c:512
    #4 0x5576928b75f4 in perf_evlist__poll_thread util/sideband_evlist.c:68
    #5 0x7f9df96a63eb in start_thread nptl/pthread_create.c:444
    #6 0x7f9df9726a4b in clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

0x61d000161e00 is located 384 bytes inside of 2136-byte region [0x61d000161c80,0x61d0001624d8)
freed by thread T0 here:
    #0 0x7f9dfa6d7288 in __interceptor_free libsanitizer/asan/asan_malloc_linux.cpp:52
    #1 0x557692978d50 in perf_session__delete util/session.c:319
    #2 0x557692673959 in __cmd_record tools/perf/builtin-record.c:2884
    #3 0x55769267a9f0 in cmd_record tools/perf/builtin-record.c:4259
    #4 0x55769286710c in run_builtin tools/perf/perf.c:349
    #5 0x557692867678 in handle_internal_command tools/perf/perf.c:402
    #6 0x557692867a40 in run_argv tools/perf/perf.c:446
    #7 0x557692867fae in main tools/perf/perf.c:562
    #8 0x7f9df96456c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
```

Fixes: 657ee5531903 ("perf evlist: Introduce side band thread")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-record.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 92ccca9574ca..32df34dda9cd 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -2881,10 +2881,10 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 	}
 #endif
 	zstd_fini(&session->zstd_data);
-	perf_session__delete(session);
-
 	if (!opts->no_bpf_event)
 		evlist__stop_sb_thread(rec->sb_evlist);
+
+	perf_session__delete(session);
 	return status;
 }
 
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v1 2/4] perf test: stat output per thread of just the parent process

[Ian Rogers]

Per-thread mode requires either system-wide (-a), a pid (-p) or a tid
(-t). The stat output tests were using system-wide mode but this is
racy when threads are starting and exiting - something that happens a
lot when running the tests in parallel (perf test -p). Avoid the race
conditions by using pid mode with the pid of the parent process.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/shell/lib/stat_output.sh  | 2 +-
 tools/perf/tests/shell/stat+json_output.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/shell/lib/stat_output.sh b/tools/perf/tests/shell/lib/stat_output.sh
index c81d6a9f7983..9a176ceae4a3 100644
--- a/tools/perf/tests/shell/lib/stat_output.sh
+++ b/tools/perf/tests/shell/lib/stat_output.sh
@@ -79,7 +79,7 @@ check_per_thread()
 		echo "[Skip] paranoid and not root"
 		return
 	fi
-	perf stat --per-thread -a $2 true
+	perf stat --per-thread -p $$ $2 true
 	commachecker --per-thread
 	echo "[Success]"
 }
diff --git a/tools/perf/tests/shell/stat+json_output.sh b/tools/perf/tests/shell/stat+json_output.sh
index 2b9c6212dffc..6b630d33c328 100755
--- a/tools/perf/tests/shell/stat+json_output.sh
+++ b/tools/perf/tests/shell/stat+json_output.sh
@@ -105,7 +105,7 @@ check_per_thread()
 		echo "[Skip] paranoia and not root"
 		return
 	fi
-	perf stat -j --per-thread -a -o "${stat_output}" true
+	perf stat -j --per-thread -p $$ -o "${stat_output}" true
 	$PYTHON $pythonchecker --per-thread --file "${stat_output}"
 	echo "[Success]"
 }
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v1 3/4] perf test: Use a single fd for the child process out/err

[Ian Rogers]

Switch from dumping err then out, to a single file descriptor for both
of them. This allows the err and output to be correctly interleaved in
verbose output.

Fixes: b482f5f8e016 ("perf tests: Add option to run tests in parallel")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/builtin-test.c | 37 ++++++---------------------------
 1 file changed, 6 insertions(+), 31 deletions(-)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index d13ee7683d9d..e05b370b1e2b 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -274,11 +274,8 @@ static int finish_test(struct child_test *child_test, int width)
 	struct test_suite *t = child_test->test;
 	int i = child_test->test_num;
 	int subi = child_test->subtest;
-	int out = child_test->process.out;
 	int err = child_test->process.err;
-	bool out_done = out <= 0;
 	bool err_done = err <= 0;
-	struct strbuf out_output = STRBUF_INIT;
 	struct strbuf err_output = STRBUF_INIT;
 	int ret;
 
@@ -290,11 +287,9 @@ static int finish_test(struct child_test *child_test, int width)
 		pr_info("%3d: %-*s:\n", i + 1, width, test_description(t, -1));
 
 	/*
-	 * Busy loop reading from the child's stdout and stderr that are set to
-	 * be non-blocking until EOF.
+	 * Busy loop reading from the child's stdout/stderr that are set to be
+	 * non-blocking until EOF.
 	 */
-	if (!out_done)
-		fcntl(out, F_SETFL, O_NONBLOCK);
 	if (!err_done)
 		fcntl(err, F_SETFL, O_NONBLOCK);
 	if (verbose > 1) {
@@ -303,11 +298,8 @@ static int finish_test(struct child_test *child_test, int width)
 		else
 			pr_info("%3d: %s:\n", i + 1, test_description(t, -1));
 	}
-	while (!out_done || !err_done) {
-		struct pollfd pfds[2] = {
-			{ .fd = out,
-			  .events = POLLIN | POLLERR | POLLHUP | POLLNVAL,
-			},
+	while (!err_done) {
+		struct pollfd pfds[1] = {
 			{ .fd = err,
 			  .events = POLLIN | POLLERR | POLLHUP | POLLNVAL,
 			},
@@ -317,21 +309,7 @@ static int finish_test(struct child_test *child_test, int width)
 
 		/* Poll to avoid excessive spinning, timeout set for 1000ms. */
 		poll(pfds, ARRAY_SIZE(pfds), /*timeout=*/1000);
-		if (!out_done && pfds[0].revents) {
-			errno = 0;
-			len = read(out, buf, sizeof(buf) - 1);
-
-			if (len <= 0) {
-				out_done = errno != EAGAIN;
-			} else {
-				buf[len] = '\0';
-				if (verbose > 1)
-					fprintf(stdout, "%s", buf);
-				else
-					strbuf_addstr(&out_output, buf);
-			}
-		}
-		if (!err_done && pfds[1].revents) {
+		if (!err_done && pfds[0].revents) {
 			errno = 0;
 			len = read(err, buf, sizeof(buf) - 1);
 
@@ -354,14 +332,10 @@ static int finish_test(struct child_test *child_test, int width)
 			pr_info("%3d.%1d: %s:\n", i + 1, subi + 1, test_description(t, subi));
 		else
 			pr_info("%3d: %s:\n", i + 1, test_description(t, -1));
-		fprintf(stdout, "%s", out_output.buf);
 		fprintf(stderr, "%s", err_output.buf);
 	}
-	strbuf_release(&out_output);
 	strbuf_release(&err_output);
 	print_test_result(t, i, subi, ret, width);
-	if (out > 0)
-		close(out);
 	if (err > 0)
 		close(err);
 	return 0;
@@ -394,6 +368,7 @@ static int start_test(struct test_suite *test, int i, int subi, struct child_tes
 		(*child)->process.no_stdout = 1;
 		(*child)->process.no_stderr = 1;
 	} else {
+		(*child)->process.stdout_to_stderr = 1;
 		(*child)->process.out = -1;
 		(*child)->process.err = -1;
 	}
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v1 4/4] perf test: Read child test 10 times a second rather than 1

[Ian Rogers]

Make the perf test output smoother by timing out the poll of the child
process after 100ms rather than 1s.

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

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index e05b370b1e2b..ddb2f4e38ea5 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -307,8 +307,8 @@ static int finish_test(struct child_test *child_test, int width)
 		char buf[512];
 		ssize_t len;
 
-		/* Poll to avoid excessive spinning, timeout set for 1000ms. */
-		poll(pfds, ARRAY_SIZE(pfds), /*timeout=*/1000);
+		/* Poll to avoid excessive spinning, timeout set for 100ms. */
+		poll(pfds, ARRAY_SIZE(pfds), /*timeout=*/100);
 		if (!err_done && pfds[0].revents) {
 			errno = 0;
 			len = read(err, buf, sizeof(buf) - 1);
-- 
2.44.0.278.ge034bb2e1d-goog

Re: [PATCH v1 1/4] perf record: Delete session after stopping sideband thread

[Ian Rogers]

On Thu, Feb 29, 2024 at 11:47 PM Ian Rogers <irogers@google.com> wrote:
>
> The session has a header in it which contains a perf env with
> bpf_progs. The bpf_progs are accessed by the sideband thread and so
> the sideband thread must be stopped before the session is deleted, to
> avoid a use after free.  This error was detected by AddressSanitizer
> in the following:
>
> ```
> ==2054673==ERROR: AddressSanitizer: heap-use-after-free on address 0x61d000161e00 at pc 0x55769289de54 bp 0x7f9df36d4ab0 sp 0x7f9df36d4aa8
> READ of size 8 at 0x61d000161e00 thread T1
>     #0 0x55769289de53 in __perf_env__insert_bpf_prog_info util/env.c:42
>     #1 0x55769289dbb1 in perf_env__insert_bpf_prog_info util/env.c:29
>     #2 0x557692bbae29 in perf_env__add_bpf_info util/bpf-event.c:483
>     #3 0x557692bbb01a in bpf_event__sb_cb util/bpf-event.c:512
>     #4 0x5576928b75f4 in perf_evlist__poll_thread util/sideband_evlist.c:68
>     #5 0x7f9df96a63eb in start_thread nptl/pthread_create.c:444
>     #6 0x7f9df9726a4b in clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
>
> 0x61d000161e00 is located 384 bytes inside of 2136-byte region [0x61d000161c80,0x61d0001624d8)
> freed by thread T0 here:
>     #0 0x7f9dfa6d7288 in __interceptor_free libsanitizer/asan/asan_malloc_linux.cpp:52
>     #1 0x557692978d50 in perf_session__delete util/session.c:319
>     #2 0x557692673959 in __cmd_record tools/perf/builtin-record.c:2884
>     #3 0x55769267a9f0 in cmd_record tools/perf/builtin-record.c:4259
>     #4 0x55769286710c in run_builtin tools/perf/perf.c:349
>     #5 0x557692867678 in handle_internal_command tools/perf/perf.c:402
>     #6 0x557692867a40 in run_argv tools/perf/perf.c:446
>     #7 0x557692867fae in main tools/perf/perf.c:562
>     #8 0x7f9df96456c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
> ```
>
> Fixes: 657ee5531903 ("perf evlist: Introduce side band thread")
> Signed-off-by: Ian Rogers <irogers@google.com>

Note, after this series I'm seeing parallel perf testing being as
reliable as serial but parallel testing is nearly 3 times faster. I
think after these changes land we can make parallel execution the
default.

Thanks,
Ian

Re: [PATCH v1 1/4] perf record: Delete session after stopping sideband thread

[Ian Rogers]

On Thu, Feb 29, 2024 at 11:50 PM Ian Rogers <irogers@google.com> wrote:
>
> On Thu, Feb 29, 2024 at 11:47 PM Ian Rogers <irogers@google.com> wrote:
> >
> > The session has a header in it which contains a perf env with
> > bpf_progs. The bpf_progs are accessed by the sideband thread and so
> > the sideband thread must be stopped before the session is deleted, to
> > avoid a use after free.  This error was detected by AddressSanitizer
> > in the following:
> >
> > ```
> > ==2054673==ERROR: AddressSanitizer: heap-use-after-free on address 0x61d000161e00 at pc 0x55769289de54 bp 0x7f9df36d4ab0 sp 0x7f9df36d4aa8
> > READ of size 8 at 0x61d000161e00 thread T1
> >     #0 0x55769289de53 in __perf_env__insert_bpf_prog_info util/env.c:42
> >     #1 0x55769289dbb1 in perf_env__insert_bpf_prog_info util/env.c:29
> >     #2 0x557692bbae29 in perf_env__add_bpf_info util/bpf-event.c:483
> >     #3 0x557692bbb01a in bpf_event__sb_cb util/bpf-event.c:512
> >     #4 0x5576928b75f4 in perf_evlist__poll_thread util/sideband_evlist.c:68
> >     #5 0x7f9df96a63eb in start_thread nptl/pthread_create.c:444
> >     #6 0x7f9df9726a4b in clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
> >
> > 0x61d000161e00 is located 384 bytes inside of 2136-byte region [0x61d000161c80,0x61d0001624d8)
> > freed by thread T0 here:
> >     #0 0x7f9dfa6d7288 in __interceptor_free libsanitizer/asan/asan_malloc_linux.cpp:52
> >     #1 0x557692978d50 in perf_session__delete util/session.c:319
> >     #2 0x557692673959 in __cmd_record tools/perf/builtin-record.c:2884
> >     #3 0x55769267a9f0 in cmd_record tools/perf/builtin-record.c:4259
> >     #4 0x55769286710c in run_builtin tools/perf/perf.c:349
> >     #5 0x557692867678 in handle_internal_command tools/perf/perf.c:402
> >     #6 0x557692867a40 in run_argv tools/perf/perf.c:446
> >     #7 0x557692867fae in main tools/perf/perf.c:562
> >     #8 0x7f9df96456c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
> > ```
> >
> > Fixes: 657ee5531903 ("perf evlist: Introduce side band thread")
> > Signed-off-by: Ian Rogers <irogers@google.com>
>
> Note, after this series I'm seeing parallel perf testing being as
> reliable as serial but parallel testing is nearly 3 times faster. I
> think after these changes land we can make parallel execution the
> default.

Ping.

Thanks,
Ian

Re: [PATCH v1 1/4] perf record: Delete session after stopping sideband thread

[Arnaldo Carvalho de Melo]

On Tue, Mar 19, 2024 at 09:42:26PM -0700, Ian Rogers wrote:
> On Thu, Feb 29, 2024 at 11:50 PM Ian Rogers <irogers@google.com> wrote:
> >
> > On Thu, Feb 29, 2024 at 11:47 PM Ian Rogers <irogers@google.com> wrote:
> > >
> > > The session has a header in it which contains a perf env with
> > > bpf_progs. The bpf_progs are accessed by the sideband thread and so
> > > the sideband thread must be stopped before the session is deleted, to
> > > avoid a use after free.  This error was detected by AddressSanitizer
> > > in the following:
> > >
> > > ```
> > > ==2054673==ERROR: AddressSanitizer: heap-use-after-free on address 0x61d000161e00 at pc 0x55769289de54 bp 0x7f9df36d4ab0 sp 0x7f9df36d4aa8
> > > READ of size 8 at 0x61d000161e00 thread T1
> > >     #0 0x55769289de53 in __perf_env__insert_bpf_prog_info util/env.c:42
> > >     #1 0x55769289dbb1 in perf_env__insert_bpf_prog_info util/env.c:29
> > >     #2 0x557692bbae29 in perf_env__add_bpf_info util/bpf-event.c:483
> > >     #3 0x557692bbb01a in bpf_event__sb_cb util/bpf-event.c:512
> > >     #4 0x5576928b75f4 in perf_evlist__poll_thread util/sideband_evlist.c:68
> > >     #5 0x7f9df96a63eb in start_thread nptl/pthread_create.c:444
> > >     #6 0x7f9df9726a4b in clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
> > >
> > > 0x61d000161e00 is located 384 bytes inside of 2136-byte region [0x61d000161c80,0x61d0001624d8)
> > > freed by thread T0 here:
> > >     #0 0x7f9dfa6d7288 in __interceptor_free libsanitizer/asan/asan_malloc_linux.cpp:52
> > >     #1 0x557692978d50 in perf_session__delete util/session.c:319
> > >     #2 0x557692673959 in __cmd_record tools/perf/builtin-record.c:2884
> > >     #3 0x55769267a9f0 in cmd_record tools/perf/builtin-record.c:4259
> > >     #4 0x55769286710c in run_builtin tools/perf/perf.c:349
> > >     #5 0x557692867678 in handle_internal_command tools/perf/perf.c:402
> > >     #6 0x557692867a40 in run_argv tools/perf/perf.c:446
> > >     #7 0x557692867fae in main tools/perf/perf.c:562
> > >     #8 0x7f9df96456c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
> > > ```
> > >
> > > Fixes: 657ee5531903 ("perf evlist: Introduce side band thread")
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> >
> > Note, after this series I'm seeing parallel perf testing being as
> > reliable as serial but parallel testing is nearly 3 times faster. I
> > think after these changes land we can make parallel execution the
> > default.
> 
> Ping.

Thanks, applied to perf-tools-next,

- Arnaldo