[PATCH 5.10.y 0/2] Fixed perf abort when taken branch stack sampling enabled

[PATCH 5.10.y 0/2] Fixed perf abort when taken branch stack sampling enabled

[Shuai Xue]

On x86 platform, kernel v5.10.228, perf-report command aborts due to "free():
invalid pointer" when perf-record command is run with taken branch stack
sampling enabled. This regression can be reproduced with the following steps:

	- sudo perf record -b
	- sudo perf report

The root cause is that bi[i].to.ms.maps does not always point to thread->maps,
which is a buffer dynamically allocated by maps_new(). Instead, it may point to
&machine->kmaps, while kmaps is not a pointer but a variable. The original
upstream commit c1149037f65b ("perf hist: Add missing puts to
hist__account_cycles") worked well because machine->kmaps had been refactored to
a pointer by the previous commit 1a97cee604dc ("perf maps: Use a pointer for
kmaps").

The memory leak issue, which the reverted patch intended to fix, has been solved
by commit cf96b8e45a9b ("perf session: Add missing evlist__delete when deleting
a session"). The root cause is that the evlist is not being deleted on exit in
perf-report, perf-script, and perf-data. Consequently, the reference count of
the thread increased by thread__get() in hist_entry__init() is not decremented
in hist_entry__delete(). As a result, thread->maps is not properly freed.

To this end,

- PATCH 1/2 reverts commit a83fc293acd5c5050a4828eced4a71d2b2fffdd3 to fix the
  abort regression.
- PATCH 2/2 backports cf96b8e45a9b ("perf session: Add missing evlist__delete
  when deleting a session") to fix memory leak issue.

Riccardo Mancini (1):
  perf session: Add missing evlist__delete when deleting a session

Shuai Xue (1):
  Revert "perf hist: Add missing puts to hist__account_cycles"

 tools/perf/util/hist.c    | 10 +++-------
 tools/perf/util/session.c |  5 ++++-
 2 files changed, 7 insertions(+), 8 deletions(-)

-- 
2.39.3

[PATCH 5.10.y 1/2] Revert "perf hist: Add missing puts to hist__account_cycles"

[Shuai Xue]

Revert "perf hist: Add missing puts to hist__account_cycles"

This reverts commit a83fc293acd5c5050a4828eced4a71d2b2fffdd3.

On x86 platform, kernel v5.10.228, perf-report command aborts due to "free():
invalid pointer" when perf-record command is run with taken branch stack
sampling enabled. This regression can be reproduced with the following steps:

	- sudo perf record -b
	- sudo perf report

The root cause is that bi[i].to.ms.maps does not always point to thread->maps,
which is a buffer dynamically allocated by maps_new(). Instead, it may point to
&machine->kmaps, while kmaps is not a pointer but a variable. The original
upstream commit c1149037f65b ("perf hist: Add missing puts to
hist__account_cycles") worked well because machine->kmaps had been refactored to
a pointer by the previous commit 1a97cee604dc ("perf maps: Use a pointer for
kmaps").

To this end, just revert commit a83fc293acd5c5050a4828eced4a71d2b2fffdd3.

It is worth noting that the memory leak issue, which the reverted patch intended
to fix, has been solved by commit cf96b8e45a9b ("perf session: Add missing
evlist__delete when deleting a session"). The root cause is that the evlist is
not being deleted on exit in perf-report, perf-script, and perf-data.
Consequently, the reference count of the thread increased by thread__get() in
hist_entry__init() is not decremented in hist_entry__delete(). As a result,
thread->maps is not properly freed.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: K Prateek Nayak <kprateek.nayak@amd.com>
Cc: Ravi Bangoria <ravi.bangoria@amd.com>
Cc: Sandipan Das <sandipan.das@amd.com>
Cc: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: German Gomez <german.gomez@arm.com>
Cc: James Clark <james.clark@arm.com>
Cc: Nick Terrell <terrelln@fb.com>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Changbin Du <changbin.du@huawei.com>
Cc: liuwenyu <liuwenyu7@huawei.com>
Cc: Yang Jihong <yangjihong1@huawei.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Song Liu <song@kernel.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Kajol Jain <kjain@linux.ibm.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Cc: Yanteng Si <siyanteng@loongson.cn>
Cc: Liam Howlett <liam.howlett@oracle.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: stable@vger.kernel.org # 5.10.228
Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
---
 tools/perf/util/hist.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index c78d8813811c..8a793e4c9400 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -2624,6 +2624,8 @@ void hist__account_cycles(struct branch_stack *bs, struct addr_location *al,
 
 	/* If we have branch cycles always annotate them. */
 	if (bs && bs->nr && entries[0].flags.cycles) {
+		int i;
+
 		bi = sample__resolve_bstack(sample, al);
 		if (bi) {
 			struct addr_map_symbol *prev = NULL;
@@ -2638,7 +2640,7 @@ void hist__account_cycles(struct branch_stack *bs, struct addr_location *al,
 			 * Note that perf stores branches reversed from
 			 * program order!
 			 */
-			for (int i = bs->nr - 1; i >= 0; i--) {
+			for (i = bs->nr - 1; i >= 0; i--) {
 				addr_map_symbol__account_cycles(&bi[i].from,
 					nonany_branch_mode ? NULL : prev,
 					bi[i].flags.cycles);
@@ -2647,12 +2649,6 @@ void hist__account_cycles(struct branch_stack *bs, struct addr_location *al,
 				if (total_cycles)
 					*total_cycles += bi[i].flags.cycles;
 			}
-			for (unsigned int i = 0; i < bs->nr; i++) {
-				map__put(bi[i].to.ms.map);
-				maps__put(bi[i].to.ms.maps);
-				map__put(bi[i].from.ms.map);
-				maps__put(bi[i].from.ms.maps);
-			}
 			free(bi);
 		}
 	}
-- 
2.39.3

[PATCH 5.10.y 2/2] perf session: Add missing evlist__delete when deleting a session

[Shuai Xue]

From: Riccardo Mancini <rickyman7@gmail.com>

commit cf96b8e45a9bf74d2a6f1e1f88a41b10e9357c6b upstream.

ASan reports a memory leak caused by evlist not being deleted on exit in
perf-report, perf-script and perf-data.
The problem is caused by evlist->session not being deleted, which is
allocated in perf_session__read_header, called in perf_session__new if
perf_data is in read mode.
In case of write mode, the session->evlist is filled by the caller.
This patch solves the problem by calling evlist__delete in
perf_session__delete if perf_data is in read mode.

Changes in v2:
 - call evlist__delete from within perf_session__delete

v1: https://lore.kernel.org/lkml/20210621234317.235545-1-rickyman7@gmail.com/

ASan report follows:

$ ./perf script report flamegraph
=================================================================
==227640==ERROR: LeakSanitizer: detected memory leaks

<SNIP unrelated>

Indirect leak of 2704 byte(s) in 1 object(s) allocated from:
    #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
    #1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
    #2 0x7f999e in evlist__new /home/user/linux/tools/perf/util/evlist.c:77:26
    #3 0x8ad938 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3797:20
    #4 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
    #5 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
    #6 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
    #7 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
    #8 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
    #9 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
    #10 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
    #11 0x7f5260654b74  (/lib64/libc.so.6+0x27b74)

Indirect leak of 568 byte(s) in 1 object(s) allocated from:
    #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
    #1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
    #2 0x80ce88 in evsel__new_idx /home/user/linux/tools/perf/util/evsel.c:268:24
    #3 0x8aed93 in evsel__new /home/user/linux/tools/perf/util/evsel.h:210:9
    #4 0x8ae07e in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3853:11
    #5 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
    #6 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
    #7 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
    #8 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
    #9 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
    #10 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
    #11 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
    #12 0x7f5260654b74  (/lib64/libc.so.6+0x27b74)

Indirect leak of 264 byte(s) in 1 object(s) allocated from:
    #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
    #1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
    #2 0xbe3e70 in xyarray__new /home/user/linux/tools/lib/perf/xyarray.c:10:23
    #3 0xbd7754 in perf_evsel__alloc_id /home/user/linux/tools/lib/perf/evsel.c:361:21
    #4 0x8ae201 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3871:7
    #5 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
    #6 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
    #7 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
    #8 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
    #9 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
    #10 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
    #11 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
    #12 0x7f5260654b74  (/lib64/libc.so.6+0x27b74)

Indirect leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
    #1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
    #2 0xbd77e0 in perf_evsel__alloc_id /home/user/linux/tools/lib/perf/evsel.c:365:14
    #3 0x8ae201 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3871:7
    #4 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
    #5 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
    #6 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
    #7 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
    #8 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
    #9 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
    #10 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
    #11 0x7f5260654b74  (/lib64/libc.so.6+0x27b74)

Indirect leak of 7 byte(s) in 1 object(s) allocated from:
    #0 0x4b8207 in strdup (/home/user/linux/tools/perf/perf+0x4b8207)
    #1 0x8b4459 in evlist__set_event_name /home/user/linux/tools/perf/util/header.c:2292:16
    #2 0x89d862 in process_event_desc /home/user/linux/tools/perf/util/header.c:2313:3
    #3 0x8af319 in perf_file_section__process /home/user/linux/tools/perf/util/header.c:3651:9
    #4 0x8aa6e9 in perf_header__process_sections /home/user/linux/tools/perf/util/header.c:3427:9
    #5 0x8ae3e7 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3886:2
    #6 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
    #7 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
    #8 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
    #9 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
    #10 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
    #11 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
    #12 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
    #13 0x7f5260654b74  (/lib64/libc.so.6+0x27b74)

SUMMARY: AddressSanitizer: 3728 byte(s) leaked in 7 allocation(s).

Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
Acked-by: Ian Rogers <irogers@google.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lore.kernel.org/lkml/20210624231926.212208-1-rickyman7@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: stable@vger.kernel.org # 5.10.228
Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
---
 tools/perf/util/session.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 354e1e04a266..81b7ec2ae861 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -299,8 +299,11 @@ void perf_session__delete(struct perf_session *session)
 	perf_session__release_decomp_events(session);
 	perf_env__exit(&session->header.env);
 	machines__exit(&session->machines);
-	if (session->data)
+	if (session->data) {
+		if (perf_data__is_read(session->data))
+			evlist__delete(session->evlist);
 		perf_data__close(session->data);
+	}
 	free(session);
 }
 
-- 
2.39.3

Re: [PATCH 5.10.y 0/2] Fixed perf abort when taken branch stack sampling enabled

[Greg KH]

On Mon, Nov 04, 2024 at 07:27:34PM +0800, Shuai Xue wrote:
> On x86 platform, kernel v5.10.228, perf-report command aborts due to "free():
> invalid pointer" when perf-record command is run with taken branch stack
> sampling enabled. This regression can be reproduced with the following steps:
> 
> 	- sudo perf record -b
> 	- sudo perf report
> 
> The root cause is that bi[i].to.ms.maps does not always point to thread->maps,
> which is a buffer dynamically allocated by maps_new(). Instead, it may point to
> &machine->kmaps, while kmaps is not a pointer but a variable. The original
> upstream commit c1149037f65b ("perf hist: Add missing puts to
> hist__account_cycles") worked well because machine->kmaps had been refactored to
> a pointer by the previous commit 1a97cee604dc ("perf maps: Use a pointer for
> kmaps").
> 
> The memory leak issue, which the reverted patch intended to fix, has been solved
> by commit cf96b8e45a9b ("perf session: Add missing evlist__delete when deleting
> a session"). The root cause is that the evlist is not being deleted on exit in
> perf-report, perf-script, and perf-data. Consequently, the reference count of
> the thread increased by thread__get() in hist_entry__init() is not decremented
> in hist_entry__delete(). As a result, thread->maps is not properly freed.
> 
> To this end,
> 
> - PATCH 1/2 reverts commit a83fc293acd5c5050a4828eced4a71d2b2fffdd3 to fix the
>   abort regression.
> - PATCH 2/2 backports cf96b8e45a9b ("perf session: Add missing evlist__delete
>   when deleting a session") to fix memory leak issue.
> 
> Riccardo Mancini (1):
>   perf session: Add missing evlist__delete when deleting a session
> 
> Shuai Xue (1):
>   Revert "perf hist: Add missing puts to hist__account_cycles"
> 
>  tools/perf/util/hist.c    | 10 +++-------
>  tools/perf/util/session.c |  5 ++++-
>  2 files changed, 7 insertions(+), 8 deletions(-)

perf actually works and builds on this kernel tree?  That's news to me,
but hey, I'll take these now as obviously someone is still trying to run
it.

But why not just use the latest version of perf instead?

thanks,

greg k-h

Re: [PATCH 5.10.y 0/2] Fixed perf abort when taken branch stack sampling enabled

[Shuai Xue]

在 2024/11/10 13:12, Greg KH 写道:
> On Mon, Nov 04, 2024 at 07:27:34PM +0800, Shuai Xue wrote:
>> On x86 platform, kernel v5.10.228, perf-report command aborts due to "free():
>> invalid pointer" when perf-record command is run with taken branch stack
>> sampling enabled. This regression can be reproduced with the following steps:
>>
>> 	- sudo perf record -b
>> 	- sudo perf report
>>
>> The root cause is that bi[i].to.ms.maps does not always point to thread->maps,
>> which is a buffer dynamically allocated by maps_new(). Instead, it may point to
>> &machine->kmaps, while kmaps is not a pointer but a variable. The original
>> upstream commit c1149037f65b ("perf hist: Add missing puts to
>> hist__account_cycles") worked well because machine->kmaps had been refactored to
>> a pointer by the previous commit 1a97cee604dc ("perf maps: Use a pointer for
>> kmaps").
>>
>> The memory leak issue, which the reverted patch intended to fix, has been solved
>> by commit cf96b8e45a9b ("perf session: Add missing evlist__delete when deleting
>> a session"). The root cause is that the evlist is not being deleted on exit in
>> perf-report, perf-script, and perf-data. Consequently, the reference count of
>> the thread increased by thread__get() in hist_entry__init() is not decremented
>> in hist_entry__delete(). As a result, thread->maps is not properly freed.
>>
>> To this end,
>>
>> - PATCH 1/2 reverts commit a83fc293acd5c5050a4828eced4a71d2b2fffdd3 to fix the
>>    abort regression.
>> - PATCH 2/2 backports cf96b8e45a9b ("perf session: Add missing evlist__delete
>>    when deleting a session") to fix memory leak issue.
>>
>> Riccardo Mancini (1):
>>    perf session: Add missing evlist__delete when deleting a session
>>
>> Shuai Xue (1):
>>    Revert "perf hist: Add missing puts to hist__account_cycles"
>>
>>   tools/perf/util/hist.c    | 10 +++-------
>>   tools/perf/util/session.c |  5 ++++-
>>   2 files changed, 7 insertions(+), 8 deletions(-)
> 
> perf actually works and builds on this kernel tree?  That's news to me,
> but hey, I'll take these now as obviously someone is still trying to run
> it.


Yes, it does. Commit cf96b8e45a9b ("perf session: Add missing evlist__delete
when deleting a session") addresses a memory leak issue but is not applicable
for the 5.10-stable tree.


> But why not just use the latest version of perf instead?

Yes, the lastest verison of perf works well. There are many distribution are
based on the upstream 5.10-stable tree, and this issue breaks the perf usage.
So IMHO, it could be fixed in upstream.

Best Regards,
Shuai