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 <[email protected]>
Suggested-by: Robert Walker <[email protected]>
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 <[email protected]>
---
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
Since cs_etm_queue::prev_packet is allocated for all cases, it will
never be NULL pointer; now validity checking prev_packet is pointless,
remove all of them.
Signed-off-by: Leo Yan <[email protected]>
---
tools/perf/util/cs-etm.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 054b480aab04..de488b43f440 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -979,7 +979,6 @@ static int cs_etm__sample(struct cs_etm_queue *etmq)
* PREV_PACKET is a branch.
*/
if (etm->synth_opts.last_branch &&
- etmq->prev_packet &&
etmq->prev_packet->sample_type == CS_ETM_RANGE &&
etmq->prev_packet->last_instr_taken_branch)
cs_etm__update_last_branch_rb(etmq);
@@ -1012,7 +1011,7 @@ static int cs_etm__sample(struct cs_etm_queue *etmq)
etmq->period_instructions = instrs_over;
}
- if (etm->sample_branches && etmq->prev_packet) {
+ if (etm->sample_branches) {
bool generate_sample = false;
/* Generate sample for tracing on packet */
@@ -1069,9 +1068,6 @@ static int cs_etm__flush(struct cs_etm_queue *etmq)
struct cs_etm_auxtrace *etm = etmq->etm;
struct cs_etm_packet *tmp;
- if (!etmq->prev_packet)
- return 0;
-
/* Handle start tracing packet */
if (etmq->prev_packet->sample_type == CS_ETM_EMPTY)
goto swap_packet;
--
2.17.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 <[email protected]>
> Suggested-by: Robert Walker <[email protected]>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
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);
On 28/04/2019 09:32, Leo Yan wrote:
> Since cs_etm_queue::prev_packet is allocated for all cases, it will
> never be NULL pointer; now validity checking prev_packet is pointless,
> remove all of them.
>
> Signed-off-by: Leo Yan <[email protected]>
> ---
> tools/perf/util/cs-etm.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
Tested-by: Robert Walker <[email protected]>
Regards
Rob
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 054b480aab04..de488b43f440 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -979,7 +979,6 @@ static int cs_etm__sample(struct cs_etm_queue *etmq)
> * PREV_PACKET is a branch.
> */
> if (etm->synth_opts.last_branch &&
> - etmq->prev_packet &&
> etmq->prev_packet->sample_type == CS_ETM_RANGE &&
> etmq->prev_packet->last_instr_taken_branch)
> cs_etm__update_last_branch_rb(etmq);
> @@ -1012,7 +1011,7 @@ static int cs_etm__sample(struct cs_etm_queue *etmq)
> etmq->period_instructions = instrs_over;
> }
>
> - if (etm->sample_branches && etmq->prev_packet) {
> + if (etm->sample_branches) {
> bool generate_sample = false;
>
> /* Generate sample for tracing on packet */
> @@ -1069,9 +1068,6 @@ static int cs_etm__flush(struct cs_etm_queue *etmq)
> struct cs_etm_auxtrace *etm = etmq->etm;
> struct cs_etm_packet *tmp;
>
> - if (!etmq->prev_packet)
> - return 0;
> -
> /* Handle start tracing packet */
> if (etmq->prev_packet->sample_type == CS_ETM_EMPTY)
> goto swap_packet;
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 <[email protected]>
> Suggested-by: Robert Walker <[email protected]>
> 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 <[email protected]>
> ---
> 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
Commit-ID: cf0c37b6dbf74fb71bea07b516612d29e00dcbc4
Gitweb: https://git.kernel.org/tip/cf0c37b6dbf74fb71bea07b516612d29e00dcbc4
Author: Leo Yan <[email protected]>
AuthorDate: Sun, 28 Apr 2019 16:32:28 +0800
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 2 May 2019 16:00:20 -0400
perf cs-etm: Don't check cs_etm_queue::prev_packet validity
Since cs_etm_queue::prev_packet is allocated for all cases, it will
never be NULL pointer; now validity checking prev_packet is pointless,
remove all of them.
Signed-off-by: Leo Yan <[email protected]>
Tested-by: Robert Walker <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Mathieu Poirier <[email protected]>
Cc: Mike Leach <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Suzuki K Poulouse <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/cs-etm.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 110804936fc3..7777cfc1ad8c 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -981,7 +981,6 @@ static int cs_etm__sample(struct cs_etm_queue *etmq)
* PREV_PACKET is a branch.
*/
if (etm->synth_opts.last_branch &&
- etmq->prev_packet &&
etmq->prev_packet->sample_type == CS_ETM_RANGE &&
etmq->prev_packet->last_instr_taken_branch)
cs_etm__update_last_branch_rb(etmq);
@@ -1014,7 +1013,7 @@ static int cs_etm__sample(struct cs_etm_queue *etmq)
etmq->period_instructions = instrs_over;
}
- if (etm->sample_branches && etmq->prev_packet) {
+ if (etm->sample_branches) {
bool generate_sample = false;
/* Generate sample for tracing on packet */
@@ -1071,9 +1070,6 @@ static int cs_etm__flush(struct cs_etm_queue *etmq)
struct cs_etm_auxtrace *etm = etmq->etm;
struct cs_etm_packet *tmp;
- if (!etmq->prev_packet)
- return 0;
-
/* Handle start tracing packet */
if (etmq->prev_packet->sample_type == CS_ETM_EMPTY)
goto swap_packet;
Commit-ID: 35bb59c10a6d0578806dd500477dae9cb4be344e
Gitweb: https://git.kernel.org/tip/35bb59c10a6d0578806dd500477dae9cb4be344e
Author: Leo Yan <[email protected]>
AuthorDate: Sun, 28 Apr 2019 16:32:27 +0800
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 2 May 2019 16:00:20 -0400
perf cs-etm: Always allocate memory for cs_etm_queue::prev_packet
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 <[email protected]>
Suggested-by: Robert Walker <[email protected]>
Signed-off-by: Leo Yan <[email protected]>
Tested-by: Robert Walker <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Mathieu Poirier <[email protected]>
Cc: Mike Leach <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Suzuki K Poulouse <[email protected]>
Cc: [email protected]
Fixes: 7100b12cf474 ("perf cs-etm: Generate branch sample for exception packet")
Fixes: 24fff5eb2b93 ("perf cs-etm: Avoid stale branch samples when flush packet")
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
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 7777cfc1ad8c..de488b43f440 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);