Hi,
I've noticed that recent versions of perf will crash when processing CoreSight trace with some combinations of decoding options.
The crash occurs when neither the 'b' (branch events), or 'l' flag (last branch entries) options to --itrace are specified for the perf report/script/inject commands - e.g. 'perf report --itrace=i1000i'. The default flags includes 'b', so the crash doesn't occur if no '--itrace' option is given.
The crash appears to be due to the prev_packet field of cs_etm_queue (tools/perf/util/cs-etm.c) not being initialised unless either 'b' or 'l' is included in the trace flags. There seems to have been a number of uses of this field added without checking whether it's valid - these have been added in 5.0 (e.g. commits 7100b12: perf cs-etm: Generate branch sample for exception packet, 24fff5e: perf cs-etm: Avoid stale branch samples when flush packet) and there have been more added for 5.1-rc1: (06220bf: perf cs-etm: Set sample flags for instruction range packet and related commits)
As this field is used more widely now and it's already hard to follow which functions have been called in a context where the validity of prev_packet has been checked (i.e. "if (etm->synth_opts.last_branch || etm->sample_branches) { ... }", I think it would be better to create prev_packet in all cases.
Regards
Rob
Hi Rob,
On Thu, Apr 25, 2019 at 01:43:02PM +0100, Robert Walker wrote:
Hi,
I've noticed that recent versions of perf will crash when processing CoreSight trace with some combinations of decoding options.
The crash occurs when neither the 'b' (branch events), or 'l' flag (last branch entries) options to --itrace are specified for the perf report/script/inject commands - e.g. 'perf report --itrace=i1000i'. The default flags includes 'b', so the crash doesn't occur if no '--itrace' option is given.
The crash appears to be due to the prev_packet field of cs_etm_queue (tools/perf/util/cs-etm.c) not being initialised unless either 'b' or 'l' is included in the trace flags. There seems to have been a number of uses of this field added without checking whether it's valid - these have been added in 5.0 (e.g. commits 7100b12: perf cs-etm: Generate branch sample for exception packet, 24fff5e: perf cs-etm: Avoid stale branch samples when flush packet) and there have been more added for 5.1-rc1: (06220bf: perf cs-etm: Set sample flags for instruction range packet and related commits)
As this field is used more widely now and it's already hard to follow which functions have been called in a context where the validity of prev_packet has been checked (i.e. "if (etm->synth_opts.last_branch || etm->sample_branches) { ... }", I think it would be better to create prev_packet in all cases.
Sorry for regression ...
I used gdb to quickly narrow this segment fault, and found it's caused by without checking prev_packet in the function cs_etm__exception().
Below patch can fix the issue at my side, could you confirm this? I still need a bit more time to get clear what's exactly flow to introduce this issue.
---8<---
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 27a374ddf661..da17f6528a54 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -1002,6 +1002,9 @@ static int cs_etm__sample(struct cs_etm_queue *etmq)
static int cs_etm__exception(struct cs_etm_queue *etmq) { + if (!etmq->prev_packet) + return 0; + /* * When the exception packet is inserted, whether the last instruction * in previous range packet is taken branch or not, we need to force
Thanks, Leo Yan
Hi Leo,
On 25/04/2019 13:56, Leo Yan wrote:
Hi Rob,
On Thu, Apr 25, 2019 at 01:43:02PM +0100, Robert Walker wrote:
Hi,
I've noticed that recent versions of perf will crash when processing CoreSight trace with some combinations of decoding options.
The crash occurs when neither the 'b' (branch events), or 'l' flag (last branch entries) options to --itrace are specified for the perf report/script/inject commands - e.g. 'perf report --itrace=i1000i'. The default flags includes 'b', so the crash doesn't occur if no '--itrace' option is given.
The crash appears to be due to the prev_packet field of cs_etm_queue (tools/perf/util/cs-etm.c) not being initialised unless either 'b' or 'l' is included in the trace flags. There seems to have been a number of uses of this field added without checking whether it's valid - these have been added in 5.0 (e.g. commits 7100b12: perf cs-etm: Generate branch sample for exception packet, 24fff5e: perf cs-etm: Avoid stale branch samples when flush packet) and there have been more added for 5.1-rc1: (06220bf: perf cs-etm: Set sample flags for instruction range packet and related commits)
As this field is used more widely now and it's already hard to follow which functions have been called in a context where the validity of prev_packet has been checked (i.e. "if (etm->synth_opts.last_branch || etm->sample_branches) { ... }", I think it would be better to create prev_packet in all cases.
Sorry for regression ...
I used gdb to quickly narrow this segment fault, and found it's caused by without checking prev_packet in the function cs_etm__exception().
Below patch can fix the issue at my side, could you confirm this? I still need a bit more time to get clear what's exactly flow to introduce this issue.
I think this should fix the regression in 5.0, but for the later patches in 5.1-rc1, there are other cases where an uninitialised prev_packet can be accessed - e.g. cs_etm__set_sample_flags() and other functions called from there.
Regards
Rob
---8<---
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 27a374ddf661..da17f6528a54 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -1002,6 +1002,9 @@ static int cs_etm__sample(struct cs_etm_queue *etmq) static int cs_etm__exception(struct cs_etm_queue *etmq) {
- if (!etmq->prev_packet)
return 0;- /*
- When the exception packet is inserted, whether the last instruction
- in previous range packet is taken branch or not, we need to force
Thanks, Leo Yan
On Thu, Apr 25, 2019 at 03:10:17PM +0100, Robert Walker wrote:
[...]
I think this should fix the regression in 5.0, but for the later patches in 5.1-rc1, there are other cases where an uninitialised prev_packet can be accessed - e.g. cs_etm__set_sample_flags() and other functions called from there.
Agree and thanks for reminding.
Just want to confirm, could you send patches for this and I can review them? Or you want me to follow up this issue? Please let me know what's your preference.
Thanks, Leo Yan
Hi,
On 25/04/2019 15:28, Leo Yan wrote:
On Thu, Apr 25, 2019 at 03:10:17PM +0100, Robert Walker wrote:
[...]
I think this should fix the regression in 5.0, but for the later patches in 5.1-rc1, there are other cases where an uninitialised prev_packet can be accessed - e.g. cs_etm__set_sample_flags() and other functions called from there.
Agree and thanks for reminding.
Just want to confirm, could you send patches for this and I can review them? Or you want me to follow up this issue? Please let me know what's your preference.
Can you do the patches and I'll review/test?
Thanks
Rob
On Thu, Apr 25, 2019 at 03:48:09PM +0100, Robert Walker wrote:
Hi,
On 25/04/2019 15:28, Leo Yan wrote:
On Thu, Apr 25, 2019 at 03:10:17PM +0100, Robert Walker wrote:
[...]
I think this should fix the regression in 5.0, but for the later patches in 5.1-rc1, there are other cases where an uninitialised prev_packet can be accessed - e.g. cs_etm__set_sample_flags() and other functions called from there.
Agree and thanks for reminding.
Just want to confirm, could you send patches for this and I can review them? Or you want me to follow up this issue? Please let me know what's your preference.
Can you do the patches and I'll review/test?
Sure, will do it.
Thanks, Leo Yan
Hi Rob,
On Thu, Apr 25, 2019 at 01:43:02PM +0100, Robert Walker wrote:
Hi,
I've noticed that recent versions of perf will crash when processing CoreSight trace with some combinations of decoding options.
The crash occurs when neither the 'b' (branch events), or 'l' flag (last branch entries) options to --itrace are specified for the perf report/script/inject commands - e.g. 'perf report --itrace=i1000i'. The default flags includes 'b', so the crash doesn't occur if no '--itrace' option is given.
The crash appears to be due to the prev_packet field of cs_etm_queue (tools/perf/util/cs-etm.c) not being initialised unless either 'b' or 'l' is included in the trace flags. There seems to have been a number of uses of this field added without checking whether it's valid - these have been added in 5.0 (e.g. commits 7100b12: perf cs-etm: Generate branch sample for exception packet, 24fff5e: perf cs-etm: Avoid stale branch samples when flush packet) and there have been more added for 5.1-rc1: (06220bf: perf cs-etm: Set sample flags for instruction range packet and related commits)
As this field is used more widely now and it's already hard to follow which functions have been called in a context where the validity of prev_packet has been checked (i.e. "if (etm->synth_opts.last_branch || etm->sample_branches) { ... }", I think it would be better to create prev_packet in all cases.
After reading the code and added some logs, agree with you.
If we use the command 'perf report --itrace=i1000i', then the both flags 'etm->synth_opts.last_branch' and 'etm->sample_branches' will not be set, thus the function cs_etm__alloc_queue() skips to allocate one temporary data structure for 'etmq->prev_packet' in this case, but it causes the segment fault when invoke function cs_etm__exception().
So we can simply allocate data for 'etmq->prev_packet' from the beginning (in cs_etm__alloc_queue()), thus can avoid segment fault if access 'etmq->prev_packet'; after this change, since 'etmq->prev_packet' will always point to one data structure, I also suggest to remove all redundant code for checking 'etmq->prev_packet' is NULL pointer.
Thanks, Leo Yan
-
Leo Yan -
Robert Walker