* [PATCH v1 1/2] perf cs-etm: Always allocate memory for cs_etm_queue::prev_packet
@ 2019-04-28 8:32 Leo Yan
2019-04-28 8:32 ` [PATCH v1 2/2] perf cs-etm: Don't check cs_etm_queue::prev_packet validity Leo Yan
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Leo Yan @ 2019-04-28 8:32 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Mathieu Poirier, Robert Walker,
Suzuki K Poulose, Mike Leach, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, linux-arm-kernel, linux-kernel
Cc: Leo Yan
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(-)
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v1 2/2] perf cs-etm: Don't check cs_etm_queue::prev_packet validity
2019-04-28 8:32 [PATCH v1 1/2] perf cs-etm: Always allocate memory for cs_etm_queue::prev_packet Leo Yan
@ 2019-04-28 8:32 ` Leo Yan
2019-04-29 14:53 ` Robert Walker
2019-04-29 14:53 ` [PATCH v1 1/2] perf cs-etm: Always allocate memory for cs_etm_queue::prev_packet Robert Walker
2019-04-30 1:07 ` Arnaldo Carvalho de Melo
2 siblings, 1 reply; 5+ messages in thread
From: Leo Yan @ 2019-04-28 8:32 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Mathieu Poirier, Robert Walker,
Suzuki K Poulose, Mike Leach, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, linux-arm-kernel, linux-kernel
Cc: Leo Yan
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 <leo.yan@linaro.org>
---
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v1 1/2] perf cs-etm: Always allocate memory for cs_etm_queue::prev_packet
2019-04-28 8:32 [PATCH v1 1/2] perf cs-etm: Always allocate memory for cs_etm_queue::prev_packet Leo Yan
2019-04-28 8:32 ` [PATCH v1 2/2] perf cs-etm: Don't check cs_etm_queue::prev_packet validity Leo Yan
@ 2019-04-29 14:53 ` Robert Walker
2019-04-30 1:07 ` Arnaldo Carvalho de Melo
2 siblings, 0 replies; 5+ messages in thread
From: Robert Walker @ 2019-04-29 14:53 UTC (permalink / raw)
To: Leo Yan, Arnaldo Carvalho de Melo, Mathieu Poirier,
Suzuki K Poulose, Mike Leach, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, linux-arm-kernel, linux-kernel
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);
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 2/2] perf cs-etm: Don't check cs_etm_queue::prev_packet validity
2019-04-28 8:32 ` [PATCH v1 2/2] perf cs-etm: Don't check cs_etm_queue::prev_packet validity Leo Yan
@ 2019-04-29 14:53 ` Robert Walker
0 siblings, 0 replies; 5+ messages in thread
From: Robert Walker @ 2019-04-29 14:53 UTC (permalink / raw)
To: Leo Yan, Arnaldo Carvalho de Melo, Mathieu Poirier,
Suzuki K Poulose, Mike Leach, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, linux-arm-kernel, linux-kernel
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 <leo.yan@linaro.org>
> ---
> tools/perf/util/cs-etm.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
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 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;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 1/2] perf cs-etm: Always allocate memory for cs_etm_queue::prev_packet
2019-04-28 8:32 [PATCH v1 1/2] perf cs-etm: Always allocate memory for cs_etm_queue::prev_packet Leo Yan
2019-04-28 8:32 ` [PATCH v1 2/2] perf cs-etm: Don't check cs_etm_queue::prev_packet validity Leo Yan
2019-04-29 14:53 ` [PATCH v1 1/2] perf cs-etm: Always allocate memory for cs_etm_queue::prev_packet Robert Walker
@ 2019-04-30 1:07 ` Arnaldo Carvalho de Melo
2 siblings, 0 replies; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-04-30 1:07 UTC (permalink / raw)
To: Leo Yan
Cc: Mathieu Poirier, Suzuki K Poulose, Alexander Shishkin,
linux-kernel, Namhyung Kim, Robert Walker, Jiri Olsa,
linux-arm-kernel, Mike Leach
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-04-30 1:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-28 8:32 [PATCH v1 1/2] perf cs-etm: Always allocate memory for cs_etm_queue::prev_packet Leo Yan
2019-04-28 8:32 ` [PATCH v1 2/2] perf cs-etm: Don't check cs_etm_queue::prev_packet validity Leo Yan
2019-04-29 14:53 ` Robert Walker
2019-04-29 14:53 ` [PATCH v1 1/2] perf cs-etm: Always allocate memory for cs_etm_queue::prev_packet Robert Walker
2019-04-30 1:07 ` Arnaldo Carvalho de Melo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).