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, term->val.str);
__t->val.str = term->val.str;
`> __t->val.str 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.str;
`> sink string has been freed.
To fix this issue, in the function get_config_terms(), this patch
changes to use strdup() for allocation a new duplicate string rather
than directly assignment string pointer.
This patch addes a new field 'free_str' in the data structure
perf_evsel_config_term; 'free_str' is set to true when the union is used
as a string pointer; thus it can tell perf_evsel__free_config_terms() to
free the string.
Fixes: 1dc925568f01 ("perf parse: Add a deep delete for parse event terms")
Suggested-by: Jiri Olsa <[email protected]>
Signed-off-by: Leo Yan <[email protected]>
Acked-by: Jiri Olsa <[email protected]>
---
tools/perf/util/evsel.c | 2 ++
tools/perf/util/evsel_config.h | 1 +
tools/perf/util/parse-events.c | 7 ++++++-
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 549abd43816f..6fe9e28180e5 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1265,6 +1265,8 @@ static void perf_evsel__free_config_terms(struct evsel *evsel)
list_for_each_entry_safe(term, h, &evsel->config_terms, list) {
list_del_init(&term->list);
+ if (term->free_str)
+ free(term->val.str);
free(term);
}
}
diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
index b4a65201e4f7..e026ab67b008 100644
--- a/tools/perf/util/evsel_config.h
+++ b/tools/perf/util/evsel_config.h
@@ -32,6 +32,7 @@ enum evsel_term_type {
struct perf_evsel_config_term {
struct list_head list;
enum evsel_term_type type;
+ bool free_str;
union {
u64 period;
u64 freq;
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index f59f3c8da473..c01ba6f8fdad 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1240,7 +1240,12 @@ do { \
#define ADD_CONFIG_TERM_STR(__type, __val) \
do { \
ADD_CONFIG_TERM(__type); \
- __t->val.str = __val; \
+ __t->val.str = strdup(__val); \
+ if (!__t->val.str) { \
+ zfree(&__t); \
+ return -ENOMEM; \
+ } \
+ __t->free_str = true; \
} while (0)
struct parse_events_term *term;
--
2.17.1
Em Fri, Jan 17, 2020 at 01:52:51PM +0800, Leo Yan escreveu:
> 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, term->val.str);
> __t->val.str = term->val.str;
> `> __t->val.str 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.str;
> `> sink string has been freed.
>
> To fix this issue, in the function get_config_terms(), this patch
> changes to use strdup() for allocation a new duplicate string rather
> than directly assignment string pointer.
>
> This patch addes a new field 'free_str' in the data structure
> perf_evsel_config_term; 'free_str' is set to true when the union is used
> as a string pointer; thus it can tell perf_evsel__free_config_terms() to
> free the string.
>
> Fixes: 1dc925568f01 ("perf parse: Add a deep delete for parse event terms")
> Suggested-by: Jiri Olsa <[email protected]>
> Signed-off-by: Leo Yan <[email protected]>
> Acked-by: Jiri Olsa <[email protected]>
> ---
> tools/perf/util/evsel.c | 2 ++
> tools/perf/util/evsel_config.h | 1 +
> tools/perf/util/parse-events.c | 7 ++++++-
> 3 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 549abd43816f..6fe9e28180e5 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1265,6 +1265,8 @@ static void perf_evsel__free_config_terms(struct evsel *evsel)
>
> list_for_each_entry_safe(term, h, &evsel->config_terms, list) {
> list_del_init(&term->list);
> + if (term->free_str)
> + free(term->val.str);
I'm replacing this with zfree, so that we can catch possible bugs where
term gets used after freed, just like you do below, in ADD_CONFIG_TERM_STR()
Thanks,
> free(term);
> }
> }
> diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
> index b4a65201e4f7..e026ab67b008 100644
> --- a/tools/perf/util/evsel_config.h
> +++ b/tools/perf/util/evsel_config.h
> @@ -32,6 +32,7 @@ enum evsel_term_type {
> struct perf_evsel_config_term {
> struct list_head list;
> enum evsel_term_type type;
> + bool free_str;
> union {
> u64 period;
> u64 freq;
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index f59f3c8da473..c01ba6f8fdad 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1240,7 +1240,12 @@ do { \
> #define ADD_CONFIG_TERM_STR(__type, __val) \
> do { \
> ADD_CONFIG_TERM(__type); \
> - __t->val.str = __val; \
> + __t->val.str = strdup(__val); \
> + if (!__t->val.str) { \
> + zfree(&__t); \
> + return -ENOMEM; \
> + } \
> + __t->free_str = true; \
> } while (0)
>
> struct parse_events_term *term;
> --
> 2.17.1
On Fri, Jan 17, 2020 at 10:34:09AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jan 17, 2020 at 01:52:51PM +0800, Leo Yan escreveu:
> > 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, term->val.str);
> > __t->val.str = term->val.str;
> > `> __t->val.str 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.str;
> > `> sink string has been freed.
> >
> > To fix this issue, in the function get_config_terms(), this patch
> > changes to use strdup() for allocation a new duplicate string rather
> > than directly assignment string pointer.
> >
> > This patch addes a new field 'free_str' in the data structure
> > perf_evsel_config_term; 'free_str' is set to true when the union is used
> > as a string pointer; thus it can tell perf_evsel__free_config_terms() to
> > free the string.
> >
> > Fixes: 1dc925568f01 ("perf parse: Add a deep delete for parse event terms")
> > Suggested-by: Jiri Olsa <[email protected]>
> > Signed-off-by: Leo Yan <[email protected]>
> > Acked-by: Jiri Olsa <[email protected]>
> > ---
> > tools/perf/util/evsel.c | 2 ++
> > tools/perf/util/evsel_config.h | 1 +
> > tools/perf/util/parse-events.c | 7 ++++++-
> > 3 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index 549abd43816f..6fe9e28180e5 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -1265,6 +1265,8 @@ static void perf_evsel__free_config_terms(struct evsel *evsel)
> >
> > list_for_each_entry_safe(term, h, &evsel->config_terms, list) {
> > list_del_init(&term->list);
> > + if (term->free_str)
> > + free(term->val.str);
>
> I'm replacing this with zfree, so that we can catch possible bugs where
> term gets used after freed, just like you do below, in ADD_CONFIG_TERM_STR()
Thanks a lot, Arnaldo.
> > free(term);
> > }
> > }
> > diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
> > index b4a65201e4f7..e026ab67b008 100644
> > --- a/tools/perf/util/evsel_config.h
> > +++ b/tools/perf/util/evsel_config.h
> > @@ -32,6 +32,7 @@ enum evsel_term_type {
> > struct perf_evsel_config_term {
> > struct list_head list;
> > enum evsel_term_type type;
> > + bool free_str;
> > union {
> > u64 period;
> > u64 freq;
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index f59f3c8da473..c01ba6f8fdad 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -1240,7 +1240,12 @@ do { \
> > #define ADD_CONFIG_TERM_STR(__type, __val) \
> > do { \
> > ADD_CONFIG_TERM(__type); \
> > - __t->val.str = __val; \
> > + __t->val.str = strdup(__val); \
> > + if (!__t->val.str) { \
> > + zfree(&__t); \
> > + return -ENOMEM; \
> > + } \
> > + __t->free_str = true; \
> > } while (0)
> >
> > struct parse_events_term *term;
> > --
> > 2.17.1
>
The following commit has been merged into the perf/urgent branch of tip:
Commit-ID: 3220fb8d5e59d7a9b59d02965d4209aef7691e9f
Gitweb: https://git.kernel.org/tip/3220fb8d5e59d7a9b59d02965d4209aef7691e9f
Author: Leo Yan <[email protected]>
AuthorDate: Fri, 17 Jan 2020 13:52:51 +08:00
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitterDate: Thu, 30 Jan 2020 11:55:02 +01:00
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, term->val.str);
__t->val.str = term->val.str;
`> __t->val.str 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.str;
`> sink string has been freed.
To fix this issue, in the function get_config_terms(), this patch
changes to use strdup() for allocation a new duplicate string rather
than directly assignment string pointer.
This patch addes a new field 'free_str' in the data structure
perf_evsel_config_term; 'free_str' is set to true when the union is used
as a string pointer; thus it can tell perf_evsel__free_config_terms() to
free the string.
Fixes: 1dc925568f01 ("perf parse: Add a deep delete for parse event terms")
Suggested-by: Jiri Olsa <[email protected]>
Signed-off-by: Leo Yan <[email protected]>
Acked-by: Jiri Olsa <[email protected]>
Cc: Adrian Hunter <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Ian Rogers <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Mathieu Poirier <[email protected]>
Cc: Mike Leach <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Suzuki Poulouse <[email protected]>
Cc: [email protected]
Link: http://lore.kernel.org/lkml/[email protected]
[ Use zfree() in perf_evsel__free_config_terms ]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
:# modified: tools/perf/util/evsel_config.h
---
tools/perf/util/evsel.c | 2 ++
tools/perf/util/evsel_config.h | 1 +
tools/perf/util/parse-events.c | 7 ++++++-
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 549abd4..c8dc445 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1265,6 +1265,8 @@ static void perf_evsel__free_config_terms(struct evsel *evsel)
list_for_each_entry_safe(term, h, &evsel->config_terms, list) {
list_del_init(&term->list);
+ if (term->free_str)
+ zfree(&term->val.str);
free(term);
}
}
diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
index b4a6520..e026ab6 100644
--- a/tools/perf/util/evsel_config.h
+++ b/tools/perf/util/evsel_config.h
@@ -32,6 +32,7 @@ enum evsel_term_type {
struct perf_evsel_config_term {
struct list_head list;
enum evsel_term_type type;
+ bool free_str;
union {
u64 period;
u64 freq;
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index f59f3c8..c01ba6f 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1240,7 +1240,12 @@ do { \
#define ADD_CONFIG_TERM_STR(__type, __val) \
do { \
ADD_CONFIG_TERM(__type); \
- __t->val.str = __val; \
+ __t->val.str = strdup(__val); \
+ if (!__t->val.str) { \
+ zfree(&__t); \
+ return -ENOMEM; \
+ } \
+ __t->free_str = true; \
} while (0)
struct parse_events_term *term;