All of lore.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.