diff mbox series

[v1,1/2] perf cs-etm: Always allocate memory for cs_etm_queue::prev_packet

Message ID 20190428083228.20246-1-leo.yan@linaro.org
State New
Headers show
Series [v1,1/2] perf cs-etm: Always allocate memory for cs_etm_queue::prev_packet | expand

Commit Message

Leo Yan April 28, 2019, 8:32 a.m. UTC 
Robert Walker reported a segmentation fault is observed when process
CoreSight trace data; this issue can be easily reproduced by the
command 'perf report --itrace=i1000i' for decoding tracing data.

If neither the 'b' flag (synthesize branches events) nor 'l' flag
(synthesize last branch entries) are specified to option '--itrace',
cs_etm_queue::prev_packet will not been initialised.  After merging
the code to support exception packets and sample flags, there
introduced a number of uses of cs_etm_queue::prev_packet without
checking whether it is valid, for these cases any accessing to
uninitialised prev_packet will cause crash.

As cs_etm_queue::prev_packet is used more widely now and it's already
hard to follow which functions have been called in a context where the
validity of cs_etm_queue::prev_packet has been checked, this patch
always allocates memory for cs_etm_queue::prev_packet.

Reported-by: Robert Walker <robert.walker@arm.com>
Suggested-by: Robert Walker <robert.walker@arm.com>
Fixes: 7100b12cf474 ("perf cs-etm: Generate branch sample for exception packet")
Fixes: 24fff5eb2b93 ("perf cs-etm: Avoid stale branch samples when flush packet")
Signed-off-by: Leo Yan <leo.yan@linaro.org>

---
 tools/perf/util/cs-etm.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

-- 
2.17.1

Comments

Robert Walker April 29, 2019, 2:53 p.m. UTC | #1 
Hi,

On 28/04/2019 09:32, Leo Yan wrote:
> Robert Walker reported a segmentation fault is observed when process

> CoreSight trace data; this issue can be easily reproduced by the

> command 'perf report --itrace=i1000i' for decoding tracing data.

>

> If neither the 'b' flag (synthesize branches events) nor 'l' flag

> (synthesize last branch entries) are specified to option '--itrace',

> cs_etm_queue::prev_packet will not been initialised.  After merging

> the code to support exception packets and sample flags, there

> introduced a number of uses of cs_etm_queue::prev_packet without

> checking whether it is valid, for these cases any accessing to

> uninitialised prev_packet will cause crash.

>

> As cs_etm_queue::prev_packet is used more widely now and it's already

> hard to follow which functions have been called in a context where the

> validity of cs_etm_queue::prev_packet has been checked, this patch

> always allocates memory for cs_etm_queue::prev_packet.

>

> Reported-by: Robert Walker <robert.walker@arm.com>

> Suggested-by: Robert Walker <robert.walker@arm.com>

> Fixes: 7100b12cf474 ("perf cs-etm: Generate branch sample for exception packet")

> Fixes: 24fff5eb2b93 ("perf cs-etm: Avoid stale branch samples when flush packet")

> Signed-off-by: Leo Yan <leo.yan@linaro.org>

> ---

>   tools/perf/util/cs-etm.c | 8 +++-----

>   1 file changed, 3 insertions(+), 5 deletions(-)


I've tested these with the trace from the HiKey960 and the segfault no 
longer occurs

Tested-by: Robert Walker <robert.walker@arm.com>


Regards

Rob

>

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c

> index 110804936fc3..054b480aab04 100644

> --- a/tools/perf/util/cs-etm.c

> +++ b/tools/perf/util/cs-etm.c

> @@ -422,11 +422,9 @@ static struct cs_etm_queue *cs_etm__alloc_queue(struct cs_etm_auxtrace *etm)

>   	if (!etmq->packet)

>   		goto out_free;

>   

> -	if (etm->synth_opts.last_branch || etm->sample_branches) {

> -		etmq->prev_packet = zalloc(szp);

> -		if (!etmq->prev_packet)

> -			goto out_free;

> -	}

> +	etmq->prev_packet = zalloc(szp);

> +	if (!etmq->prev_packet)

> +		goto out_free;

>   

>   	if (etm->synth_opts.last_branch) {

>   		size_t sz = sizeof(struct branch_stack);
Arnaldo Carvalho de Melo April 30, 2019, 1:07 a.m. UTC | #2 
Em Sun, Apr 28, 2019 at 04:32:27PM +0800, Leo Yan escreveu:
> Robert Walker reported a segmentation fault is observed when process

> CoreSight trace data; this issue can be easily reproduced by the

> command 'perf report --itrace=i1000i' for decoding tracing data.

> 

> If neither the 'b' flag (synthesize branches events) nor 'l' flag

> (synthesize last branch entries) are specified to option '--itrace',

> cs_etm_queue::prev_packet will not been initialised.  After merging

> the code to support exception packets and sample flags, there

> introduced a number of uses of cs_etm_queue::prev_packet without

> checking whether it is valid, for these cases any accessing to

> uninitialised prev_packet will cause crash.

> 

> As cs_etm_queue::prev_packet is used more widely now and it's already

> hard to follow which functions have been called in a context where the

> validity of cs_etm_queue::prev_packet has been checked, this patch

> always allocates memory for cs_etm_queue::prev_packet.

> 

> Reported-by: Robert Walker <robert.walker@arm.com>

> Suggested-by: Robert Walker <robert.walker@arm.com>

> Fixes: 7100b12cf474 ("perf cs-etm: Generate branch sample for exception packet")


Thanks, applied both to perf/urgent, testing them now in the containers.

- Arnaldo

> Fixes: 24fff5eb2b93 ("perf cs-etm: Avoid stale branch samples when flush packet")

> Signed-off-by: Leo Yan <leo.yan@linaro.org>

> ---

>  tools/perf/util/cs-etm.c | 8 +++-----

>  1 file changed, 3 insertions(+), 5 deletions(-)

> 

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c

> index 110804936fc3..054b480aab04 100644

> --- a/tools/perf/util/cs-etm.c

> +++ b/tools/perf/util/cs-etm.c

> @@ -422,11 +422,9 @@ static struct cs_etm_queue *cs_etm__alloc_queue(struct cs_etm_auxtrace *etm)

>  	if (!etmq->packet)

>  		goto out_free;

>  

> -	if (etm->synth_opts.last_branch || etm->sample_branches) {

> -		etmq->prev_packet = zalloc(szp);

> -		if (!etmq->prev_packet)

> -			goto out_free;

> -	}

> +	etmq->prev_packet = zalloc(szp);

> +	if (!etmq->prev_packet)

> +		goto out_free;

>  

>  	if (etm->synth_opts.last_branch) {

>  		size_t sz = sizeof(struct branch_stack);

> -- 

> 2.17.1


-- 

- Arnaldo
diff mbox series

Patch

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 110804936fc3..054b480aab04 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -422,11 +422,9 @@  static struct cs_etm_queue *cs_etm__alloc_queue(struct cs_etm_auxtrace *etm)
 	if (!etmq->packet)
 		goto out_free;
 
-	if (etm->synth_opts.last_branch || etm->sample_branches) {
-		etmq->prev_packet = zalloc(szp);
-		if (!etmq->prev_packet)
-			goto out_free;
-	}
+	etmq->prev_packet = zalloc(szp);
+	if (!etmq->prev_packet)
+		goto out_free;
 
 	if (etm->synth_opts.last_branch) {
 		size_t sz = sizeof(struct branch_stack);