public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] perf report: Fix a memory corrupton issue when enabling --branch-history
@ 2018-02-13  8:44 Jin Yao
  2018-02-13  9:45 ` Jiri Olsa
  0 siblings, 1 reply; 8+ messages in thread
From: Jin Yao @ 2018-02-13  8:44 UTC (permalink / raw)
  To: acme, jolsa, peterz, mingo, alexander.shishkin
  Cc: Linux-kernel, ak, kan.liang, yao.jin, Jin Yao

Following command lines will cause perf crash.

perf record -j call -g -a <application>
perf report --branch-history

*** Error in `perf': double free or corruption (!prev): 0x00000000104aa040 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x77725)[0x7f6b37254725]
/lib/x86_64-linux-gnu/libc.so.6(+0x7ff4a)[0x7f6b3725cf4a]
/lib/x86_64-linux-gnu/libc.so.6(cfree+0x4c)[0x7f6b37260abc]
perf[0x51b914]
perf(hist_entry_iter__add+0x1e5)[0x51f305]
perf[0x43cf01]
perf[0x4fa3bf]
perf[0x4fa923]
perf[0x4fd396]
perf[0x4f9614]
perf(perf_session__process_events+0x89e)[0x4fc38e]
perf(cmd_report+0x15d2)[0x43f202]
perf[0x4a059f]
perf(main+0x631)[0x427b71]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f6b371fd830]
perf(_start+0x29)[0x427d89]

The memory corruption happens at:

iter_add_next_cumulative_entry()
{
        ...
        for (i = 0; i < iter->curr; i++) {
        ...
}

Whatever in iter_next_cumulative_entry() or in iter_add_next_cumulative_entry(),
they all don't check if iter->curr exceeds the array 'he_cache[]'.

If there are too many nodes in callchain, it's possible that iter->curr >
iter->max_stack, then memory corruption occurs.

This patch will reallocate array 'he_cache[]' in iter_next_cumulative_entry()
if necessary (the case of too many nodes in callchain).

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/util/hist.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index b614095..71f07d2 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -926,11 +926,32 @@ iter_next_cumulative_entry(struct hist_entry_iter *iter,
 			   struct addr_location *al)
 {
 	struct callchain_cursor_node *node;
+	struct hist_entry **tmp;
+	int i;
 
 	node = callchain_cursor_current(&callchain_cursor);
 	if (node == NULL)
 		return 0;
 
+	/*
+	 * If there are too many nodes in callchain,
+	 * increase the size of he_cache[].
+	 */
+	if (iter->curr == iter->max_stack) {
+		i = 2 * iter->max_stack + 1;
+		tmp = realloc(iter->priv, sizeof(struct hist_entry *) * i);
+		if (tmp == NULL) {
+			/*
+			 * No need to free iter->priv here. It will be
+			 * freed in iter_finish_cumulative_entry.
+			 */
+			return 0;
+		}
+
+		iter->priv = tmp;
+		iter->max_stack = i;
+	}
+
 	return fill_callchain_info(al, node, iter->hide_unresolved);
 }
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] perf report: Fix a memory corrupton issue when enabling --branch-history
  2018-02-13  8:44 [PATCH] perf report: Fix a memory corrupton issue when enabling --branch-history Jin Yao
@ 2018-02-13  9:45 ` Jiri Olsa
  2018-02-13 14:00   ` Jin, Yao
  0 siblings, 1 reply; 8+ messages in thread
From: Jiri Olsa @ 2018-02-13  9:45 UTC (permalink / raw)
  To: Jin Yao
  Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
	kan.liang, yao.jin

On Tue, Feb 13, 2018 at 04:44:28PM +0800, Jin Yao wrote:
> Following command lines will cause perf crash.
> 
> perf record -j call -g -a <application>
> perf report --branch-history
> 
> *** Error in `perf': double free or corruption (!prev): 0x00000000104aa040 ***
> ======= Backtrace: =========
> /lib/x86_64-linux-gnu/libc.so.6(+0x77725)[0x7f6b37254725]
> /lib/x86_64-linux-gnu/libc.so.6(+0x7ff4a)[0x7f6b3725cf4a]
> /lib/x86_64-linux-gnu/libc.so.6(cfree+0x4c)[0x7f6b37260abc]
> perf[0x51b914]
> perf(hist_entry_iter__add+0x1e5)[0x51f305]
> perf[0x43cf01]
> perf[0x4fa3bf]
> perf[0x4fa923]
> perf[0x4fd396]
> perf[0x4f9614]
> perf(perf_session__process_events+0x89e)[0x4fc38e]
> perf(cmd_report+0x15d2)[0x43f202]
> perf[0x4a059f]
> perf(main+0x631)[0x427b71]
> /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f6b371fd830]
> perf(_start+0x29)[0x427d89]
> 
> The memory corruption happens at:
> 
> iter_add_next_cumulative_entry()
> {
>         ...
>         for (i = 0; i < iter->curr; i++) {
>         ...
> }
> 
> Whatever in iter_next_cumulative_entry() or in iter_add_next_cumulative_entry(),
> they all don't check if iter->curr exceeds the array 'he_cache[]'.
> 
> If there are too many nodes in callchain, it's possible that iter->curr >
> iter->max_stack, then memory corruption occurs.
> 
> This patch will reallocate array 'he_cache[]' in iter_next_cumulative_entry()
> if necessary (the case of too many nodes in callchain).

right, the max_stack does not say how many nodes end up in
callchain_cursor at the end.. good catch, please mention
that also in the changelog

however we know the final count from callchain_cursor itself,
the attached patch might do the same job, right?

also could we now get rid of iter->max_stack?

thanks,
jirka


---
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index b6140950301e..b50b7b70dcca 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -879,7 +879,7 @@ iter_prepare_cumulative_entry(struct hist_entry_iter *iter,
 	 * cumulated only one time to prevent entries more than 100%
 	 * overhead.
 	 */
-	he_cache = malloc(sizeof(*he_cache) * (iter->max_stack + 1));
+	he_cache = malloc(sizeof(*he_cache) * (callchain_cursor.nr + 1));
 	if (he_cache == NULL)
 		return -ENOMEM;
 

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] perf report: Fix a memory corrupton issue when enabling --branch-history
  2018-02-13  9:45 ` Jiri Olsa
@ 2018-02-13 14:00   ` Jin, Yao
  2018-02-16  2:25     ` Jin, Yao
  0 siblings, 1 reply; 8+ messages in thread
From: Jin, Yao @ 2018-02-13 14:00 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
	kan.liang, yao.jin



On 2/13/2018 5:45 PM, Jiri Olsa wrote:
> On Tue, Feb 13, 2018 at 04:44:28PM +0800, Jin Yao wrote:
>> Following command lines will cause perf crash.
>>
>> perf record -j call -g -a <application>
>> perf report --branch-history
>>
>> *** Error in `perf': double free or corruption (!prev): 0x00000000104aa040 ***
>> ======= Backtrace: =========
>> /lib/x86_64-linux-gnu/libc.so.6(+0x77725)[0x7f6b37254725]
>> /lib/x86_64-linux-gnu/libc.so.6(+0x7ff4a)[0x7f6b3725cf4a]
>> /lib/x86_64-linux-gnu/libc.so.6(cfree+0x4c)[0x7f6b37260abc]
>> perf[0x51b914]
>> perf(hist_entry_iter__add+0x1e5)[0x51f305]
>> perf[0x43cf01]
>> perf[0x4fa3bf]
>> perf[0x4fa923]
>> perf[0x4fd396]
>> perf[0x4f9614]
>> perf(perf_session__process_events+0x89e)[0x4fc38e]
>> perf(cmd_report+0x15d2)[0x43f202]
>> perf[0x4a059f]
>> perf(main+0x631)[0x427b71]
>> /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f6b371fd830]
>> perf(_start+0x29)[0x427d89]
>>
>> The memory corruption happens at:
>>
>> iter_add_next_cumulative_entry()
>> {
>>          ...
>>          for (i = 0; i < iter->curr; i++) {
>>          ...
>> }
>>
>> Whatever in iter_next_cumulative_entry() or in iter_add_next_cumulative_entry(),
>> they all don't check if iter->curr exceeds the array 'he_cache[]'.
>>
>> If there are too many nodes in callchain, it's possible that iter->curr >
>> iter->max_stack, then memory corruption occurs.
>>
>> This patch will reallocate array 'he_cache[]' in iter_next_cumulative_entry()
>> if necessary (the case of too many nodes in callchain).
> 
> right, the max_stack does not say how many nodes end up in
> callchain_cursor at the end.. good catch, please mention
> that also in the changelog
> 

max_stack looks only to limit the number of calls but not for other 
branches.

> however we know the final count from callchain_cursor itself,
> the attached patch might do the same job, right?
> 

I think the attached patch is ok.

> also could we now get rid of iter->max_stack?
> 

 From my opinion, the option '--max-stack' in perf report looks not very 
necessary. While it's just my personal opinion, need to hear from more 
people. :)

Thanks
Jin Yao

> thanks,
> jirka
> 
> 
> ---
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index b6140950301e..b50b7b70dcca 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -879,7 +879,7 @@ iter_prepare_cumulative_entry(struct hist_entry_iter *iter,
>   	 * cumulated only one time to prevent entries more than 100%
>   	 * overhead.
>   	 */
> -	he_cache = malloc(sizeof(*he_cache) * (iter->max_stack + 1));
> +	he_cache = malloc(sizeof(*he_cache) * (callchain_cursor.nr + 1));
>   	if (he_cache == NULL)
>   		return -ENOMEM;
>   
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] perf report: Fix a memory corrupton issue when enabling --branch-history
  2018-02-13 14:00   ` Jin, Yao
@ 2018-02-16  2:25     ` Jin, Yao
  2018-02-16  7:53       ` Jiri Olsa
  0 siblings, 1 reply; 8+ messages in thread
From: Jin, Yao @ 2018-02-16  2:25 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
	kan.liang, yao.jin



On 2/13/2018 10:00 PM, Jin, Yao wrote:
> 
> 
> On 2/13/2018 5:45 PM, Jiri Olsa wrote:
>> On Tue, Feb 13, 2018 at 04:44:28PM +0800, Jin Yao wrote:
>>> Following command lines will cause perf crash.
>>>
>>> perf record -j call -g -a <application>
>>> perf report --branch-history
>>>
>>> *** Error in `perf': double free or corruption (!prev): 
>>> 0x00000000104aa040 ***
>>> ======= Backtrace: =========
>>> /lib/x86_64-linux-gnu/libc.so.6(+0x77725)[0x7f6b37254725]
>>> /lib/x86_64-linux-gnu/libc.so.6(+0x7ff4a)[0x7f6b3725cf4a]
>>> /lib/x86_64-linux-gnu/libc.so.6(cfree+0x4c)[0x7f6b37260abc]
>>> perf[0x51b914]
>>> perf(hist_entry_iter__add+0x1e5)[0x51f305]
>>> perf[0x43cf01]
>>> perf[0x4fa3bf]
>>> perf[0x4fa923]
>>> perf[0x4fd396]
>>> perf[0x4f9614]
>>> perf(perf_session__process_events+0x89e)[0x4fc38e]
>>> perf(cmd_report+0x15d2)[0x43f202]
>>> perf[0x4a059f]
>>> perf(main+0x631)[0x427b71]
>>> /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f6b371fd830]
>>> perf(_start+0x29)[0x427d89]
>>>
>>> The memory corruption happens at:
>>>
>>> iter_add_next_cumulative_entry()
>>> {
>>>          ...
>>>          for (i = 0; i < iter->curr; i++) {
>>>          ...
>>> }
>>>
>>> Whatever in iter_next_cumulative_entry() or in 
>>> iter_add_next_cumulative_entry(),
>>> they all don't check if iter->curr exceeds the array 'he_cache[]'.
>>>
>>> If there are too many nodes in callchain, it's possible that 
>>> iter->curr >
>>> iter->max_stack, then memory corruption occurs.
>>>
>>> This patch will reallocate array 'he_cache[]' in 
>>> iter_next_cumulative_entry()
>>> if necessary (the case of too many nodes in callchain).
>>
>> right, the max_stack does not say how many nodes end up in
>> callchain_cursor at the end.. good catch, please mention
>> that also in the changelog
>>
> 
> max_stack looks only to limit the number of calls but not for other 
> branches.
> 
>> however we know the final count from callchain_cursor itself,
>> the attached patch might do the same job, right?
>>
> 
> I think the attached patch is ok.
> 
>> also could we now get rid of iter->max_stack?
>>
> 
>  From my opinion, the option '--max-stack' in perf report looks not very 
> necessary. While it's just my personal opinion, need to hear from more 
> people. :)
> 
> Thanks
> Jin Yao
> 
>> thanks,
>> jirka
>>
>>
>> ---
>> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
>> index b6140950301e..b50b7b70dcca 100644
>> --- a/tools/perf/util/hist.c
>> +++ b/tools/perf/util/hist.c
>> @@ -879,7 +879,7 @@ iter_prepare_cumulative_entry(struct 
>> hist_entry_iter *iter,
>>        * cumulated only one time to prevent entries more than 100%
>>        * overhead.
>>        */
>> -    he_cache = malloc(sizeof(*he_cache) * (iter->max_stack + 1));
>> +    he_cache = malloc(sizeof(*he_cache) * (callchain_cursor.nr + 1));
>>       if (he_cache == NULL)
>>           return -ENOMEM;
>>

Hi Jiri,

I guess you will post this patch, right?

Thanks
Jin Yao

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] perf report: Fix a memory corrupton issue when enabling --branch-history
  2018-02-16  2:25     ` Jin, Yao
@ 2018-02-16  7:53       ` Jiri Olsa
  2018-02-16 12:36         ` [PATCH] perf report: Fix memory corruption in --branch-history mode --branch-history Jiri Olsa
  0 siblings, 1 reply; 8+ messages in thread
From: Jiri Olsa @ 2018-02-16  7:53 UTC (permalink / raw)
  To: Jin, Yao
  Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
	kan.liang, yao.jin

On Fri, Feb 16, 2018 at 10:25:31AM +0800, Jin, Yao wrote:

SNIP

> >  From my opinion, the option '--max-stack' in perf report looks not very
> > necessary. While it's just my personal opinion, need to hear from more
> > people. :)
> > 
> > Thanks
> > Jin Yao
> > 
> > > thanks,
> > > jirka
> > > 
> > > 
> > > ---
> > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > > index b6140950301e..b50b7b70dcca 100644
> > > --- a/tools/perf/util/hist.c
> > > +++ b/tools/perf/util/hist.c
> > > @@ -879,7 +879,7 @@ iter_prepare_cumulative_entry(struct
> > > hist_entry_iter *iter,
> > >        * cumulated only one time to prevent entries more than 100%
> > >        * overhead.
> > >        */
> > > -    he_cache = malloc(sizeof(*he_cache) * (iter->max_stack + 1));
> > > +    he_cache = malloc(sizeof(*he_cache) * (callchain_cursor.nr + 1));
> > >       if (he_cache == NULL)
> > >           return -ENOMEM;
> > > 
> 
> Hi Jiri,
> 
> I guess you will post this patch, right?

yep, later today

jirka

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] perf report: Fix memory corruption in --branch-history mode --branch-history
  2018-02-16  7:53       ` Jiri Olsa
@ 2018-02-16 12:36         ` Jiri Olsa
  2018-02-16 13:02           ` Arnaldo Carvalho de Melo
  2018-02-17 11:34           ` [tip:perf/core] " tip-bot for Jiri Olsa
  0 siblings, 2 replies; 8+ messages in thread
From: Jiri Olsa @ 2018-02-16 12:36 UTC (permalink / raw)
  To: Jin, Yao, acme
  Cc: jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
	kan.liang, yao.jin

On Fri, Feb 16, 2018 at 08:53:04AM +0100, Jiri Olsa wrote:
> On Fri, Feb 16, 2018 at 10:25:31AM +0800, Jin, Yao wrote:
> 
> SNIP
> 
> > >  From my opinion, the option '--max-stack' in perf report looks not very
> > > necessary. While it's just my personal opinion, need to hear from more
> > > people. :)
> > > 
> > > Thanks
> > > Jin Yao
> > > 
> > > > thanks,
> > > > jirka
> > > > 
> > > > 
> > > > ---
> > > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > > > index b6140950301e..b50b7b70dcca 100644
> > > > --- a/tools/perf/util/hist.c
> > > > +++ b/tools/perf/util/hist.c
> > > > @@ -879,7 +879,7 @@ iter_prepare_cumulative_entry(struct
> > > > hist_entry_iter *iter,
> > > >        * cumulated only one time to prevent entries more than 100%
> > > >        * overhead.
> > > >        */
> > > > -    he_cache = malloc(sizeof(*he_cache) * (iter->max_stack + 1));
> > > > +    he_cache = malloc(sizeof(*he_cache) * (callchain_cursor.nr + 1));
> > > >       if (he_cache == NULL)
> > > >           return -ENOMEM;
> > > > 
> > 
> > Hi Jiri,
> > 
> > I guess you will post this patch, right?
> 
> yep, later today

here it is.. I think we want this change now to fix the crash, and
some more fixes later to ensure that the branch stack code follows
properly the logic of --max-stack, which is not the case now

thanks,
jirka


---
Jin Yao reported memory corrupton in perf report with
branch info used for stack trace:

> Following command lines will cause perf crash.

> perf record -j call -g -a <application>
> perf report --branch-history
>
> *** Error in `perf': double free or corruption (!prev): 0x00000000104aa040 ***
> ======= Backtrace: =========
> /lib/x86_64-linux-gnu/libc.so.6(+0x77725)[0x7f6b37254725]
> /lib/x86_64-linux-gnu/libc.so.6(+0x7ff4a)[0x7f6b3725cf4a]
> /lib/x86_64-linux-gnu/libc.so.6(cfree+0x4c)[0x7f6b37260abc]
> perf[0x51b914]
> perf(hist_entry_iter__add+0x1e5)[0x51f305]
> perf[0x43cf01]
> perf[0x4fa3bf]
> perf[0x4fa923]
> perf[0x4fd396]
> perf[0x4f9614]
> perf(perf_session__process_events+0x89e)[0x4fc38e]
> perf(cmd_report+0x15d2)[0x43f202]
> perf[0x4a059f]
> perf(main+0x631)[0x427b71]
> /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f6b371fd830]
> perf(_start+0x29)[0x427d89]

For the cumulative output, we allocate he_cache array based
on the --max-stack option value and populate it with data
from callchain_cursor.

The --max-stack option value does not ensure now the limit
for number of callchain_cursor nodes, so the cumulative
iter code will allocate smaller array than it's actually
needed and cause above corruption.

I think the --max-stack limit does not apply here anyway,
because we add callchain data as normal hist entries,
while the --max-stack control the limit of single entry
callchain depth.

Using the callchain_cursor.nr as he_cache array count
to fix this. Also removing struct hist_entry_iter::max_stack,
because there's no longer any use for it.

We need more fixes to ensure that the branch stack code
follows properly the logic of --max-stack, which is not
the case at the moment.

Reported-by: Jin Yao <yao.jin@linux.intel.com>
Original-patch-by: Jin Yao <yao.jin@linux.intel.com>
Link: http://lkml.kernel.org/n/tip-qj1kdpvyu25ac6w22lhmy7m2@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/hist.c | 4 +---
 tools/perf/util/hist.h | 1 -
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index b6140950301e..44a8456cea10 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -879,7 +879,7 @@ iter_prepare_cumulative_entry(struct hist_entry_iter *iter,
 	 * cumulated only one time to prevent entries more than 100%
 	 * overhead.
 	 */
-	he_cache = malloc(sizeof(*he_cache) * (iter->max_stack + 1));
+	he_cache = malloc(sizeof(*he_cache) * (callchain_cursor.nr + 1));
 	if (he_cache == NULL)
 		return -ENOMEM;
 
@@ -1045,8 +1045,6 @@ int hist_entry_iter__add(struct hist_entry_iter *iter, struct addr_location *al,
 	if (err)
 		return err;
 
-	iter->max_stack = max_stack_depth;
-
 	err = iter->ops->prepare_entry(iter, al);
 	if (err)
 		goto out;
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 02721b579746..e869cad4d89f 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -107,7 +107,6 @@ struct hist_entry_iter {
 	int curr;
 
 	bool hide_unresolved;
-	int max_stack;
 
 	struct perf_evsel *evsel;
 	struct perf_sample *sample;
-- 
2.13.6

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] perf report: Fix memory corruption in --branch-history mode --branch-history
  2018-02-16 12:36         ` [PATCH] perf report: Fix memory corruption in --branch-history mode --branch-history Jiri Olsa
@ 2018-02-16 13:02           ` Arnaldo Carvalho de Melo
  2018-02-17 11:34           ` [tip:perf/core] " tip-bot for Jiri Olsa
  1 sibling, 0 replies; 8+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-02-16 13:02 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Jin, Yao, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel,
	ak, kan.liang, yao.jin

Em Fri, Feb 16, 2018 at 01:36:19PM +0100, Jiri Olsa escreveu:
> 
> here it is.. I think we want this change now to fix the crash, and
> some more fixes later to ensure that the branch stack code follows
> properly the logic of --max-stack, which is not the case now

Thanks, applied.

- Arnaldo

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [tip:perf/core] perf report: Fix memory corruption in --branch-history mode --branch-history
  2018-02-16 12:36         ` [PATCH] perf report: Fix memory corruption in --branch-history mode --branch-history Jiri Olsa
  2018-02-16 13:02           ` Arnaldo Carvalho de Melo
@ 2018-02-17 11:34           ` tip-bot for Jiri Olsa
  1 sibling, 0 replies; 8+ messages in thread
From: tip-bot for Jiri Olsa @ 2018-02-17 11:34 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: acme, peterz, hpa, mingo, jolsa, alexander.shishkin, kan.liang,
	tglx, ak, jolsa, yao.jin, linux-kernel

Commit-ID:  e3ebaa465136ecfedf9c6f4671df02bf625f8125
Gitweb:     https://git.kernel.org/tip/e3ebaa465136ecfedf9c6f4671df02bf625f8125
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Fri, 16 Feb 2018 13:36:19 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 16 Feb 2018 14:55:47 -0300

perf report: Fix memory corruption in --branch-history mode --branch-history

Jin Yao reported memory corrupton in perf report with
branch info used for stack trace:

  > Following command lines will cause perf crash.

  > perf record -j call -g -a <application>
  > perf report --branch-history
  >
  > *** Error in `perf': double free or corruption (!prev): 0x00000000104aa040 ***
  > ======= Backtrace: =========
  > /lib/x86_64-linux-gnu/libc.so.6(+0x77725)[0x7f6b37254725]
  > /lib/x86_64-linux-gnu/libc.so.6(+0x7ff4a)[0x7f6b3725cf4a]
  > /lib/x86_64-linux-gnu/libc.so.6(cfree+0x4c)[0x7f6b37260abc]
  > perf[0x51b914]
  > perf(hist_entry_iter__add+0x1e5)[0x51f305]
  > perf[0x43cf01]
  > perf[0x4fa3bf]
  > perf[0x4fa923]
  > perf[0x4fd396]
  > perf[0x4f9614]
  > perf(perf_session__process_events+0x89e)[0x4fc38e]
  > perf(cmd_report+0x15d2)[0x43f202]
  > perf[0x4a059f]
  > perf(main+0x631)[0x427b71]
  > /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f6b371fd830]
  > perf(_start+0x29)[0x427d89]

For the cumulative output, we allocate the he_cache array based on the
--max-stack option value and populate it with data from 'callchain_cursor'.

The --max-stack option value does not ensure now the limit for number of
callchain_cursor nodes, so the cumulative iter code will allocate smaller array
than it's actually needed and cause above corruption.

I think the --max-stack limit does not apply here anyway, because we add
callchain data as normal hist entries, while the --max-stack control the limit
of single entry callchain depth.

Using the callchain_cursor.nr as he_cache array count to fix this. Also
removing struct hist_entry_iter::max_stack, because there's no longer any use
for it.

We need more fixes to ensure that the branch stack code follows properly the
logic of --max-stack, which is not the case at the moment.

Original-patch-by: Jin Yao <yao.jin@linux.intel.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Reported-by: Jin Yao <yao.jin@linux.intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20180216123619.GA9945@krava
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/hist.c | 4 +---
 tools/perf/util/hist.h | 1 -
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index b614095..44a8456 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -879,7 +879,7 @@ iter_prepare_cumulative_entry(struct hist_entry_iter *iter,
 	 * cumulated only one time to prevent entries more than 100%
 	 * overhead.
 	 */
-	he_cache = malloc(sizeof(*he_cache) * (iter->max_stack + 1));
+	he_cache = malloc(sizeof(*he_cache) * (callchain_cursor.nr + 1));
 	if (he_cache == NULL)
 		return -ENOMEM;
 
@@ -1045,8 +1045,6 @@ int hist_entry_iter__add(struct hist_entry_iter *iter, struct addr_location *al,
 	if (err)
 		return err;
 
-	iter->max_stack = max_stack_depth;
-
 	err = iter->ops->prepare_entry(iter, al);
 	if (err)
 		goto out;
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 02721b57..e869cad 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -107,7 +107,6 @@ struct hist_entry_iter {
 	int curr;
 
 	bool hide_unresolved;
-	int max_stack;
 
 	struct perf_evsel *evsel;
 	struct perf_sample *sample;

^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-02-17 11:45 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-13  8:44 [PATCH] perf report: Fix a memory corrupton issue when enabling --branch-history Jin Yao
2018-02-13  9:45 ` Jiri Olsa
2018-02-13 14:00   ` Jin, Yao
2018-02-16  2:25     ` Jin, Yao
2018-02-16  7:53       ` Jiri Olsa
2018-02-16 12:36         ` [PATCH] perf report: Fix memory corruption in --branch-history mode --branch-history Jiri Olsa
2018-02-16 13:02           ` Arnaldo Carvalho de Melo
2018-02-17 11:34           ` [tip:perf/core] " tip-bot for Jiri Olsa

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox