| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANLsYkzs67NXpszueKTM5y05KrOUf-yaphs3Y7yC8P2sNz3YwQ@mail.gmail.com>
Date: Tue, 7 Jan 2020 14:45:27 -0700
From: Mathieu Poirier <mathieu.poirier@...aro.org>
To: Leo Yan <leo.yan@...aro.org>
Cc: Arnaldo Carvalho de Melo <acme@...hat.com>,
Jiri Olsa <jolsa@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Namhyung Kim <namhyung@...nel.org>,
Ian Rogers <irogers@...gle.com>,
Andi Kleen <ak@...ux.intel.com>,
Adrian Hunter <adrian.hunter@...el.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Mike Leach <mike.leach@...aro.org>,
Suzuki K Poulose <suzuki.poulose@....com>
Subject: Re: [PATCH v3] perf parse: Copy string to perf_evsel_config_term
Hi Leo,
On Tue, 7 Jan 2020 at 05:03, Leo Yan <leo.yan@...aro.org> wrote:
>
> perf with CoreSight fails to record trace data with command:
>
> perf record -e cs_etm/@..._etr0/u --per-thread ls
> failed to set sink "" on event cs_etm/@..._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.
>
> In the data structure perf_evsel_config_term, this patch adds
> 'char *str' pointer in the val union and new field 'free_str'. When the
> union is used as a string pointer, 'free_str' will be set to true;
> finally it's flag to tell perf_evsel__free_config_terms() to free the
> string with perf_evsel_config_term::val.str.
Many thanks for digging into this and stepping forward to provide a
solution - it is much appreciated.
>
> Fixes: 1dc925568f01 ("perf parse: Add a deep delete for parse event terms")
> Suggested-by: Jiri Olsa <jolsa@...nel.org>
> Signed-off-by: Leo Yan <leo.yan@...aro.org>
> ---
> tools/perf/util/evsel.c | 2 ++
> tools/perf/util/evsel_config.h | 2 ++
> tools/perf/util/parse-events.c | 56 +++++++++++++++++++++-------------
> 3 files changed, 39 insertions(+), 21 deletions(-)
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index a69e64236120..ab9925cc1aa7 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);
This will do the trick but we can definitely do better.
Part of his comments on V2, Jiri hinted that we should move to a
common perf_evsel_config_term::str to replace {callgraph, drv_cfg,
branch}, something that will work because we have
perf_evsel_config_term::type. That means functions
apply_config_terms() and cs_etm_set_sink_attr() need to be modified
but the changes are quite small and well worth for the benefit they'll
carry.
With that the above becomes neat and clean.
Thanks,
Mathieu
> free(term);
> }
> }
> diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
> index 1f8d2fe0b66e..dfc28738e071 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;
> @@ -48,6 +49,7 @@ struct perf_evsel_config_term {
> bool aux_output;
> u32 aux_sample_size;
> u64 cfg_chg;
> + char *str;
> } val;
> bool weak;
> };
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index ed7c008b9c8b..83fb149b9485 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,24 @@ 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); \
> + if (!__t->val.__name) { \
> + zfree(&__t); \
> + return -ENOMEM; \
> + } \
> + __t->free_str = true; \
> } while (0)
>
> struct parse_events_term *term;
> @@ -1239,53 +1253,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,
> - term->val.num ? true : false);
> + 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 +1336,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
>
Powered by blists - more mailing lists