LinuxLists.cc - [PATCH] perf parse: Copy string to perf_evsel_config

2020-01-02 15:15:45

Subject: [PATCH] perf parse: Copy string to perf_evsel_config_term

perf with CoreSight fails to record trace data with command:

perf record -e cs_etm/@tmc_etr0/u --per-thread ls
failed to set sink "" on event cs_etm/@tmc_etr0/u with 21 (Is a
directory)/perf/

This failure is root caused with the commit 1dc925568f01 ("perf
parse: Add a deep delete for parse event terms").

The log shows, cs_etm fails to parse the sink attribution; cs_etm event
relies on the event configuration to pass sink name, but the event
specific configuration data cannot be passed properly with flow:

get_config_terms()
ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str);
__t->val.drv_cfg = term->val.str;
`> __t->val.drv_cfg is assigned to term->val.str;

parse_events_terms__purge()
parse_events_term__delete()
zfree(&term->val.str);
`> term->val.str is freed and assigned to NULL pointer;

cs_etm_set_sink_attr()
sink = __t->val.drv_cfg;
`> sink string has been freed.

To fix this issue, in the function get_config_terms(), this patch
changes from directly assignment pointer value for the strings to
use strdup() for allocation a new duplicate string for the cases:

perf_evsel_config_term::val.callgraph
perf_evsel_config_term::val.branch
perf_evsel_config_term::val.drv_cfg.

Fixes: 1dc925568f01 ("perf parse: Add a deep delete for parse event terms")
Signed-off-by: Leo Yan <[email protected]>
---
tools/perf/util/parse-events.c | 49 ++++++++++++++++++++--------------
1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index ed7c008b9c8b..5972acdd40d6 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1220,7 +1220,6 @@ static int get_config_terms(struct list_head *head_config,
struct list_head *head_terms __maybe_unused)
{
#define ADD_CONFIG_TERM(__type, __name, __val) \
-do { \
struct perf_evsel_config_term *__t; \
\
__t = zalloc(sizeof(*__t)); \
@@ -1229,9 +1228,19 @@ do { \
\
INIT_LIST_HEAD(&__t->list); \
__t->type = PERF_EVSEL__CONFIG_TERM_ ## __type; \
- __t->val.__name = __val; \
__t->weak = term->weak; \
- list_add_tail(&__t->list, head_terms); \
+ list_add_tail(&__t->list, head_terms)
+
+#define ADD_CONFIG_TERM_VAL(__type, __name, __val) \
+do { \
+ ADD_CONFIG_TERM(__type, __name, __val); \
+ __t->val.__name = __val; \
+} while (0)
+
+#define ADD_CONFIG_TERM_STR(__type, __name, __val) \
+do { \
+ ADD_CONFIG_TERM(__type, __name, __val); \
+ __t->val.__name = strdup(__val); \
} while (0)

struct parse_events_term *term;
@@ -1239,53 +1248,53 @@ do { \
list_for_each_entry(term, head_config, list) {
switch (term->type_term) {
case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
- ADD_CONFIG_TERM(PERIOD, period, term->val.num);
+ ADD_CONFIG_TERM_VAL(PERIOD, period, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ:
- ADD_CONFIG_TERM(FREQ, freq, term->val.num);
+ ADD_CONFIG_TERM_VAL(FREQ, freq, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_TIME:
- ADD_CONFIG_TERM(TIME, time, term->val.num);
+ ADD_CONFIG_TERM_VAL(TIME, time, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_CALLGRAPH:
- ADD_CONFIG_TERM(CALLGRAPH, callgraph, term->val.str);
+ ADD_CONFIG_TERM_STR(CALLGRAPH, callgraph, term->val.str);
break;
case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE:
- ADD_CONFIG_TERM(BRANCH, branch, term->val.str);
+ ADD_CONFIG_TERM_STR(BRANCH, branch, term->val.str);
break;
case PARSE_EVENTS__TERM_TYPE_STACKSIZE:
- ADD_CONFIG_TERM(STACK_USER, stack_user, term->val.num);
+ ADD_CONFIG_TERM_VAL(STACK_USER, stack_user, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_INHERIT:
- ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 1 : 0);
+ ADD_CONFIG_TERM_VAL(INHERIT, inherit, term->val.num ? 1 : 0);
break;
case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
- ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 0 : 1);
+ ADD_CONFIG_TERM_VAL(INHERIT, inherit, term->val.num ? 0 : 1);
break;
case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
- ADD_CONFIG_TERM(MAX_STACK, max_stack, term->val.num);
+ ADD_CONFIG_TERM_VAL(MAX_STACK, max_stack, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_MAX_EVENTS:
- ADD_CONFIG_TERM(MAX_EVENTS, max_events, term->val.num);
+ ADD_CONFIG_TERM_VAL(MAX_EVENTS, max_events, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
- ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 1 : 0);
+ ADD_CONFIG_TERM_VAL(OVERWRITE, overwrite, term->val.num ? 1 : 0);
break;
case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
- ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 0 : 1);
+ ADD_CONFIG_TERM_VAL(OVERWRITE, overwrite, term->val.num ? 0 : 1);
break;
case PARSE_EVENTS__TERM_TYPE_DRV_CFG:
- ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str);
+ ADD_CONFIG_TERM_STR(DRV_CFG, drv_cfg, term->val.str);
break;
case PARSE_EVENTS__TERM_TYPE_PERCORE:
- ADD_CONFIG_TERM(PERCORE, percore,
+ ADD_CONFIG_TERM_VAL(PERCORE, percore,
term->val.num ? true : false);
break;
case PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT:
- ADD_CONFIG_TERM(AUX_OUTPUT, aux_output, term->val.num ? 1 : 0);
+ ADD_CONFIG_TERM_VAL(AUX_OUTPUT, aux_output, term->val.num ? 1 : 0);
break;
case PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE:
- ADD_CONFIG_TERM(AUX_SAMPLE_SIZE, aux_sample_size, term->val.num);
+ ADD_CONFIG_TERM_VAL(AUX_SAMPLE_SIZE, aux_sample_size, term->val.num);
break;
default:
break;
@@ -1322,7 +1331,7 @@ static int get_config_chgs(struct perf_pmu *pmu, struct list_head *head_config,
}

if (bits)
- ADD_CONFIG_TERM(CFG_CHG, cfg_chg, bits);
+ ADD_CONFIG_TERM_VAL(CFG_CHG, cfg_chg, bits);

#undef ADD_CONFIG_TERM
return 0;
--
2.17.1

2020-01-02 15:27:50

by Leo Yan

[permalink] [raw]

Subject: Re: [PATCH] perf parse: Copy string to perf_evsel_config_term

On Thu, Jan 02, 2020 at 11:13:26PM +0800, Leo Yan wrote:
> perf with CoreSight fails to record trace data with command:
>
> perf record -e cs_etm/@tmc_etr0/u --per-thread ls
> failed to set sink "" on event cs_etm/@tmc_etr0/u with 21 (Is a
> directory)/perf/
>
> This failure is root caused with the commit 1dc925568f01 ("perf
> parse: Add a deep delete for parse event terms").
>
> The log shows, cs_etm fails to parse the sink attribution; cs_etm event
> relies on the event configuration to pass sink name, but the event
> specific configuration data cannot be passed properly with flow:
>
> get_config_terms()
> ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str);
> __t->val.drv_cfg = term->val.str;
> `> __t->val.drv_cfg is assigned to term->val.str;
>
> parse_events_terms__purge()
> parse_events_term__delete()
> zfree(&term->val.str);
> `> term->val.str is freed and assigned to NULL pointer;
>
> cs_etm_set_sink_attr()
> sink = __t->val.drv_cfg;
> `> sink string has been freed.
>
> To fix this issue, in the function get_config_terms(), this patch
> changes from directly assignment pointer value for the strings to
> use strdup() for allocation a new duplicate string for the cases:
>
> perf_evsel_config_term::val.callgraph
> perf_evsel_config_term::val.branch
> perf_evsel_config_term::val.drv_cfg.
>
> Fixes: 1dc925568f01 ("perf parse: Add a deep delete for parse event terms")
> Signed-off-by: Leo Yan <[email protected]>

Let me to loop Mathieu/Mike/Suzuki, sorry for spamming.

Thanks,
Leo

> ---
> tools/perf/util/parse-events.c | 49 ++++++++++++++++++++--------------
> 1 file changed, 29 insertions(+), 20 deletions(-)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index ed7c008b9c8b..5972acdd40d6 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1220,7 +1220,6 @@ static int get_config_terms(struct list_head *head_config,
> struct list_head *head_terms __maybe_unused)
> {
> #define ADD_CONFIG_TERM(__type, __name, __val) \
> -do { \
> struct perf_evsel_config_term *__t; \
> \
> __t = zalloc(sizeof(*__t)); \
> @@ -1229,9 +1228,19 @@ do { \
> \
> INIT_LIST_HEAD(&__t->list); \
> __t->type = PERF_EVSEL__CONFIG_TERM_ ## __type; \
> - __t->val.__name = __val; \
> __t->weak = term->weak; \
> - list_add_tail(&__t->list, head_terms); \
> + list_add_tail(&__t->list, head_terms)
> +
> +#define ADD_CONFIG_TERM_VAL(__type, __name, __val) \
> +do { \
> + ADD_CONFIG_TERM(__type, __name, __val); \
> + __t->val.__name = __val; \
> +} while (0)
> +
> +#define ADD_CONFIG_TERM_STR(__type, __name, __val) \
> +do { \
> + ADD_CONFIG_TERM(__type, __name, __val); \
> + __t->val.__name = strdup(__val); \
> } while (0)
>
> struct parse_events_term *term;
> @@ -1239,53 +1248,53 @@ do { \
> list_for_each_entry(term, head_config, list) {
> switch (term->type_term) {
> case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
> - ADD_CONFIG_TERM(PERIOD, period, term->val.num);
> + ADD_CONFIG_TERM_VAL(PERIOD, period, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ:
> - ADD_CONFIG_TERM(FREQ, freq, term->val.num);
> + ADD_CONFIG_TERM_VAL(FREQ, freq, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_TIME:
> - ADD_CONFIG_TERM(TIME, time, term->val.num);
> + ADD_CONFIG_TERM_VAL(TIME, time, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_CALLGRAPH:
> - ADD_CONFIG_TERM(CALLGRAPH, callgraph, term->val.str);
> + ADD_CONFIG_TERM_STR(CALLGRAPH, callgraph, term->val.str);
> break;
> case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE:
> - ADD_CONFIG_TERM(BRANCH, branch, term->val.str);
> + ADD_CONFIG_TERM_STR(BRANCH, branch, term->val.str);
> break;
> case PARSE_EVENTS__TERM_TYPE_STACKSIZE:
> - ADD_CONFIG_TERM(STACK_USER, stack_user, term->val.num);
> + ADD_CONFIG_TERM_VAL(STACK_USER, stack_user, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_INHERIT:
> - ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 1 : 0);
> + ADD_CONFIG_TERM_VAL(INHERIT, inherit, term->val.num ? 1 : 0);
> break;
> case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
> - ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 0 : 1);
> + ADD_CONFIG_TERM_VAL(INHERIT, inherit, term->val.num ? 0 : 1);
> break;
> case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
> - ADD_CONFIG_TERM(MAX_STACK, max_stack, term->val.num);
> + ADD_CONFIG_TERM_VAL(MAX_STACK, max_stack, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_MAX_EVENTS:
> - ADD_CONFIG_TERM(MAX_EVENTS, max_events, term->val.num);
> + ADD_CONFIG_TERM_VAL(MAX_EVENTS, max_events, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
> - ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 1 : 0);
> + ADD_CONFIG_TERM_VAL(OVERWRITE, overwrite, term->val.num ? 1 : 0);
> break;
> case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
> - ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 0 : 1);
> + ADD_CONFIG_TERM_VAL(OVERWRITE, overwrite, term->val.num ? 0 : 1);
> break;
> case PARSE_EVENTS__TERM_TYPE_DRV_CFG:
> - ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str);
> + ADD_CONFIG_TERM_STR(DRV_CFG, drv_cfg, term->val.str);
> break;
> case PARSE_EVENTS__TERM_TYPE_PERCORE:
> - ADD_CONFIG_TERM(PERCORE, percore,
> + ADD_CONFIG_TERM_VAL(PERCORE, percore,
> term->val.num ? true : false);
> break;
> case PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT:
> - ADD_CONFIG_TERM(AUX_OUTPUT, aux_output, term->val.num ? 1 : 0);
> + ADD_CONFIG_TERM_VAL(AUX_OUTPUT, aux_output, term->val.num ? 1 : 0);
> break;
> case PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE:
> - ADD_CONFIG_TERM(AUX_SAMPLE_SIZE, aux_sample_size, term->val.num);
> + ADD_CONFIG_TERM_VAL(AUX_SAMPLE_SIZE, aux_sample_size, term->val.num);
> break;
> default:
> break;
> @@ -1322,7 +1331,7 @@ static int get_config_chgs(struct perf_pmu *pmu, struct list_head *head_config,
> }
>
> if (bits)
> - ADD_CONFIG_TERM(CFG_CHG, cfg_chg, bits);
> + ADD_CONFIG_TERM_VAL(CFG_CHG, cfg_chg, bits);
>
> #undef ADD_CONFIG_TERM
> return 0;
> --
> 2.17.1
>

2020-01-06 11:37:24

by Jiri Olsa

[permalink] [raw]

Subject: Re: [PATCH] perf parse: Copy string to perf_evsel_config_term

2020-01-06 12:56:37

by Leo Yan

[permalink] [raw]

Subject: Re: [PATCH] perf parse: Copy string to perf_evsel_config_term

Hi Jiri,

On Mon, Jan 06, 2020 at 12:36:10PM +0100, Jiri Olsa wrote:
> On Thu, Jan 02, 2020 at 11:13:26PM +0800, Leo Yan wrote:
> > perf with CoreSight fails to record trace data with command:
> >
> > perf record -e cs_etm/@tmc_etr0/u --per-thread ls
> > failed to set sink "" on event cs_etm/@tmc_etr0/u with 21 (Is a
> > directory)/perf/
> >
> > This failure is root caused with the commit 1dc925568f01 ("perf
> > parse: Add a deep delete for parse event terms").
> >
> > The log shows, cs_etm fails to parse the sink attribution; cs_etm event
> > relies on the event configuration to pass sink name, but the event
> > specific configuration data cannot be passed properly with flow:
> >
> > get_config_terms()
> > ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str);
> > __t->val.drv_cfg = term->val.str;
> > `> __t->val.drv_cfg is assigned to term->val.str;
> >
> > parse_events_terms__purge()
> > parse_events_term__delete()
> > zfree(&term->val.str);
> > `> term->val.str is freed and assigned to NULL pointer;
> >
> > cs_etm_set_sink_attr()
> > sink = __t->val.drv_cfg;
> > `> sink string has been freed.
>
> so your problem is that the data is freed before you use it?

Yes, exactly.

> >
> > To fix this issue, in the function get_config_terms(), this patch
> > changes from directly assignment pointer value for the strings to
> > use strdup() for allocation a new duplicate string for the cases:
> >
> > perf_evsel_config_term::val.callgraph
> > perf_evsel_config_term::val.branch
> > perf_evsel_config_term::val.drv_cfg.
> >
> > Fixes: 1dc925568f01 ("perf parse: Add a deep delete for parse event terms")
> > Signed-off-by: Leo Yan <[email protected]>
> > ---
> > tools/perf/util/parse-events.c | 49 ++++++++++++++++++++--------------
> > 1 file changed, 29 insertions(+), 20 deletions(-)
> >
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index ed7c008b9c8b..5972acdd40d6 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -1220,7 +1220,6 @@ static int get_config_terms(struct list_head *head_config,
> > struct list_head *head_terms __maybe_unused)
> > {
> > #define ADD_CONFIG_TERM(__type, __name, __val) \
> > -do { \
> > struct perf_evsel_config_term *__t; \
> > \
> > __t = zalloc(sizeof(*__t)); \
> > @@ -1229,9 +1228,19 @@ do { \
> > \
> > INIT_LIST_HEAD(&__t->list); \
> > __t->type = PERF_EVSEL__CONFIG_TERM_ ## __type; \
> > - __t->val.__name = __val; \
> > __t->weak = term->weak; \
> > - list_add_tail(&__t->list, head_terms); \
> > + list_add_tail(&__t->list, head_terms)
> > +
> > +#define ADD_CONFIG_TERM_VAL(__type, __name, __val) \
> > +do { \
> > + ADD_CONFIG_TERM(__type, __name, __val); \
> > + __t->val.__name = __val; \
> > +} while (0)
> > +
> > +#define ADD_CONFIG_TERM_STR(__type, __name, __val) \
> > +do { \
> > + ADD_CONFIG_TERM(__type, __name, __val); \
> > + __t->val.__name = strdup(__val); \
> > } while (0)
>
> the term->val.str is supposed to be already strdup-ed,
> so this seems wrong and leaking mem

Though term->val.str is strdup-ed, after merged the commit 1dc925568f01
("perf parse: Add a deep delete for parse event terms"), term->val.str
is freed in the code [1].

If term->val.str should not be freed, how about to rollback the code as
below:

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 5972acdd40d6..f3211cde0d9f 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2955,7 +2955,12 @@ void parse_events_terms__purge(struct list_head *terms)

list_for_each_entry_safe(term, h, terms, list) {
list_del_init(&term->list);
- parse_events_term__delete(term);
+
+ if (term->array.nr_ranges)
+ zfree(&term->array.ranges);
+
+ zfree(&term->config);
+ free(term);
}
}

Thanks,
Leo

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/parse-events.c?h=v5.5-rc5#n2911

>
> jirka
>

2020-01-06 15:14:00

by Jiri Olsa

[permalink] [raw]

Subject: Re: [PATCH] perf parse: Copy string to perf_evsel_config_term

2020-01-07 00:59:01

by Leo Yan

[permalink] [raw]

Subject: Re: [PATCH] perf parse: Copy string to perf_evsel_config_term

Hi Jiri,

On Mon, Jan 06, 2020 at 04:12:41PM +0100, Jiri Olsa wrote:

[...]

> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index ed7c008b9c8b..5972acdd40d6 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -1220,7 +1220,6 @@ static int get_config_terms(struct list_head *head_config,
> > struct list_head *head_terms __maybe_unused)
> > {
> > #define ADD_CONFIG_TERM(__type, __name, __val) \
> > -do { \
> > struct perf_evsel_config_term *__t; \
> > \
> > __t = zalloc(sizeof(*__t)); \
> > @@ -1229,9 +1228,19 @@ do { \
> > \
> > INIT_LIST_HEAD(&__t->list); \
> > __t->type = PERF_EVSEL__CONFIG_TERM_ ## __type; \
> > - __t->val.__name = __val; \
> > __t->weak = term->weak; \
> > - list_add_tail(&__t->list, head_terms); \
> > + list_add_tail(&__t->list, head_terms)
> > +
> > +#define ADD_CONFIG_TERM_VAL(__type, __name, __val) \
> > +do { \
> > + ADD_CONFIG_TERM(__type, __name, __val); \
> > + __t->val.__name = __val; \
> > +} while (0)
> > +
> > +#define ADD_CONFIG_TERM_STR(__type, __name, __val) \
> > +do { \
> > + ADD_CONFIG_TERM(__type, __name, __val); \
> > + __t->val.__name = strdup(__val); \
>
> ok, I understand now.. we move the pointer to the perf_evsel_config_term,
> but release it after in parse_events_terms__purge
>
> the change seems ok, but please check on the strdup return value
> and cleanup and return -ENOMEM if it fails in here

Thanks for suggestion. Will do it.

Thanks,
Leo