* [PATCH 1/2] perf expr: Force encapsulation on expr_id_data
@ 2020-08-26 4:29 Ian Rogers
2020-08-26 4:29 ` [PATCH 2/2] perf parse-events: Avoid an uninitialized read Ian Rogers
2020-08-26 11:33 ` [PATCH 1/2] perf expr: Force encapsulation on expr_id_data Jiri Olsa
0 siblings, 2 replies; 6+ messages in thread
From: Ian Rogers @ 2020-08-26 4:29 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Kajol Jain, Andi Kleen, Jin Yao, Kan Liang, Adrian Hunter,
Leo Yan, linux-kernel
Cc: Stephane Eranian, Ian Rogers
This patch resolves some undefined behavior where variables in
expr_id_data were accessed (for debugging) without being defined. To
better enforce the tagged union behavior, the struct is moved into
expr.c and accessors provided. Tag values (kinds) are explicitly
identified.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/expr.c | 64 ++++++++++++++++++++++++++++++-----
tools/perf/util/expr.h | 17 +++-------
tools/perf/util/expr.y | 2 +-
tools/perf/util/metricgroup.c | 4 +--
4 files changed, 62 insertions(+), 25 deletions(-)
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index 53482ef53c41..1ca0992db86b 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -17,6 +17,25 @@
extern int expr_debug;
#endif
+struct expr_id_data {
+ union {
+ double val;
+ struct {
+ double val;
+ const char *metric_name;
+ const char *metric_expr;
+ } ref;
+ struct expr_id *parent;
+ };
+
+ enum {
+ EXPR_ID_DATA__VALUE,
+ EXPR_ID_DATA__REF,
+ EXPR_ID_DATA__REF_VALUE,
+ EXPR_ID_DATA__PARENT,
+ } kind;
+};
+
static size_t key_hash(const void *key, void *ctx __maybe_unused)
{
const char *str = (const char *)key;
@@ -48,6 +67,7 @@ int expr__add_id(struct expr_parse_ctx *ctx, const char *id)
return -ENOMEM;
data_ptr->parent = ctx->parent;
+ data_ptr->kind = EXPR_ID_DATA__PARENT;
ret = hashmap__set(&ctx->ids, id, data_ptr,
(const void **)&old_key, (void **)&old_data);
@@ -69,7 +89,7 @@ int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val)
if (!data_ptr)
return -ENOMEM;
data_ptr->val = val;
- data_ptr->is_ref = false;
+ data_ptr->kind = EXPR_ID_DATA__VALUE;
ret = hashmap__set(&ctx->ids, id, data_ptr,
(const void **)&old_key, (void **)&old_data);
@@ -114,8 +134,7 @@ int expr__add_ref(struct expr_parse_ctx *ctx, struct metric_ref *ref)
*/
data_ptr->ref.metric_name = ref->metric_name;
data_ptr->ref.metric_expr = ref->metric_expr;
- data_ptr->ref.counted = false;
- data_ptr->is_ref = true;
+ data_ptr->kind = EXPR_ID_DATA__REF;
ret = hashmap__set(&ctx->ids, name, data_ptr,
(const void **)&old_key, (void **)&old_data);
@@ -148,17 +167,30 @@ int expr__resolve_id(struct expr_parse_ctx *ctx, const char *id,
data = *datap;
- pr_debug2("lookup: is_ref %d, counted %d, val %f: %s\n",
- data->is_ref, data->ref.counted, data->val, id);
-
- if (data->is_ref && !data->ref.counted) {
- data->ref.counted = true;
+ switch (data->kind) {
+ case EXPR_ID_DATA__VALUE:
+ pr_debug2("lookup(%s): val %f\n", id, data->val);
+ break;
+ case EXPR_ID_DATA__PARENT:
+ pr_debug2("lookup(%s): parent %s\n", id, data->parent->id);
+ break;
+ case EXPR_ID_DATA__REF:
+ pr_debug2("lookup(%s): ref metric name %s\n", id,
+ data->ref.metric_name);
pr_debug("processing metric: %s ENTRY\n", id);
- if (expr__parse(&data->val, ctx, data->ref.metric_expr, 1)) {
+ data->kind = EXPR_ID_DATA__REF_VALUE;
+ if (expr__parse(&data->ref.val, ctx, data->ref.metric_expr, 1)) {
pr_debug("%s failed to count\n", id);
return -1;
}
pr_debug("processing metric: %s EXIT: %f\n", id, data->val);
+ break;
+ case EXPR_ID_DATA__REF_VALUE:
+ pr_debug2("lookup(%s): ref val %f metric name %s\n", id,
+ data->ref.val, data->ref.metric_name);
+ break;
+ default:
+ assert(0); /* Unreachable. */
}
return 0;
@@ -241,3 +273,17 @@ int expr__find_other(const char *expr, const char *one,
return ret;
}
+
+double expr_id_data__value(const struct expr_id_data *data)
+{
+ if (data->kind == EXPR_ID_DATA__VALUE)
+ return data->val;
+ assert(data->kind == EXPR_ID_DATA__REF_VALUE);
+ return data->ref.val;
+}
+
+struct expr_id *expr_id_data__parent(struct expr_id_data *data)
+{
+ assert(data->kind == EXPR_ID_DATA__PARENT);
+ return data->parent;
+}
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index fc2b5e824a66..dcf8d19b83c8 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -23,19 +23,7 @@ struct expr_parse_ctx {
struct expr_id *parent;
};
-struct expr_id_data {
- union {
- double val;
- struct {
- const char *metric_name;
- const char *metric_expr;
- bool counted;
- } ref;
- struct expr_id *parent;
- };
-
- bool is_ref;
-};
+struct expr_id_data;
struct expr_scanner_ctx {
int start_token;
@@ -57,4 +45,7 @@ int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
int expr__find_other(const char *expr, const char *one,
struct expr_parse_ctx *ids, int runtime);
+double expr_id_data__value(const struct expr_id_data *data);
+struct expr_id *expr_id_data__parent(struct expr_id_data *data);
+
#endif
diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index d34b370391c6..b2ada8f8309a 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -93,7 +93,7 @@ expr: NUMBER
YYABORT;
}
- $$ = data->val;
+ $$ = expr_id_data__value(data);
free($1);
}
| expr '|' expr { $$ = (long)$1 | (long)$3; }
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 8831b964288f..339bfb19a10b 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -786,7 +786,7 @@ static int recursion_check(struct metric *m, const char *id, struct expr_id **pa
if (ret)
return ret;
- p = data->parent;
+ p = expr_id_data__parent(data);
while (p->parent) {
if (!strcmp(p->id, id)) {
@@ -807,7 +807,7 @@ static int recursion_check(struct metric *m, const char *id, struct expr_id **pa
}
p->id = strdup(id);
- p->parent = data->parent;
+ p->parent = expr_id_data__parent(data);
*parent = p;
return p->id ? 0 : -ENOMEM;
--
2.28.0.297.g1956fa8f8d-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] perf parse-events: Avoid an uninitialized read.
2020-08-26 4:29 [PATCH 1/2] perf expr: Force encapsulation on expr_id_data Ian Rogers
@ 2020-08-26 4:29 ` Ian Rogers
2020-08-26 11:34 ` Jiri Olsa
2020-08-26 11:33 ` [PATCH 1/2] perf expr: Force encapsulation on expr_id_data Jiri Olsa
1 sibling, 1 reply; 6+ messages in thread
From: Ian Rogers @ 2020-08-26 4:29 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Kajol Jain, Andi Kleen, Jin Yao, Kan Liang, Adrian Hunter,
Leo Yan, linux-kernel
Cc: Stephane Eranian, Ian Rogers
With a fake_pmu the pmu_info isn't populated by perf_pmu__check_alias.
In this case, don't try to copy the uninitialized values to the evsel.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/parse-events.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 9f7260e69113..3d7a48b488ed 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1533,19 +1533,23 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
evsel = __add_event(list, &parse_state->idx, &attr, true,
get_config_name(head_config), pmu,
&config_terms, auto_merge_stats, NULL);
- if (evsel) {
- evsel->unit = info.unit;
- evsel->scale = info.scale;
- evsel->per_pkg = info.per_pkg;
- evsel->snapshot = info.snapshot;
- evsel->metric_expr = info.metric_expr;
- evsel->metric_name = info.metric_name;
- evsel->pmu_name = name ? strdup(name) : NULL;
- evsel->use_uncore_alias = use_uncore_alias;
- evsel->percore = config_term_percore(&evsel->config_terms);
- }
-
- return evsel ? 0 : -ENOMEM;
+ if (!evsel)
+ return -ENOMEM;
+
+ evsel->pmu_name = name ? strdup(name) : NULL;
+ evsel->use_uncore_alias = use_uncore_alias;
+ evsel->percore = config_term_percore(&evsel->config_terms);
+
+ if (parse_state->fake_pmu)
+ return 0;
+
+ evsel->unit = info.unit;
+ evsel->scale = info.scale;
+ evsel->per_pkg = info.per_pkg;
+ evsel->snapshot = info.snapshot;
+ evsel->metric_expr = info.metric_expr;
+ evsel->metric_name = info.metric_name;
+ return 0;
}
int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
--
2.28.0.297.g1956fa8f8d-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] perf expr: Force encapsulation on expr_id_data
2020-08-26 4:29 [PATCH 1/2] perf expr: Force encapsulation on expr_id_data Ian Rogers
2020-08-26 4:29 ` [PATCH 2/2] perf parse-events: Avoid an uninitialized read Ian Rogers
@ 2020-08-26 11:33 ` Jiri Olsa
2020-08-26 15:32 ` Ian Rogers
1 sibling, 1 reply; 6+ messages in thread
From: Jiri Olsa @ 2020-08-26 11:33 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Namhyung Kim, Kajol Jain,
Andi Kleen, Jin Yao, Kan Liang, Adrian Hunter, Leo Yan,
linux-kernel, Stephane Eranian
On Tue, Aug 25, 2020 at 09:29:09PM -0700, Ian Rogers wrote:
> This patch resolves some undefined behavior where variables in
> expr_id_data were accessed (for debugging) without being defined. To
> better enforce the tagged union behavior, the struct is moved into
> expr.c and accessors provided. Tag values (kinds) are explicitly
> identified.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> tools/perf/util/expr.c | 64 ++++++++++++++++++++++++++++++-----
> tools/perf/util/expr.h | 17 +++-------
> tools/perf/util/expr.y | 2 +-
> tools/perf/util/metricgroup.c | 4 +--
> 4 files changed, 62 insertions(+), 25 deletions(-)
>
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index 53482ef53c41..1ca0992db86b 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -17,6 +17,25 @@
> extern int expr_debug;
> #endif
>
> +struct expr_id_data {
> + union {
> + double val;
> + struct {
> + double val;
> + const char *metric_name;
> + const char *metric_expr;
> + } ref;
> + struct expr_id *parent;
> + };
> +
> + enum {
> + EXPR_ID_DATA__VALUE,
> + EXPR_ID_DATA__REF,
> + EXPR_ID_DATA__REF_VALUE,
> + EXPR_ID_DATA__PARENT,
> + } kind;
I like that, it's more clear than current state ;-)
could you still put a small comment for each enum above,
as a hint what it's used for?
thanks,
jirka
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] perf parse-events: Avoid an uninitialized read.
2020-08-26 4:29 ` [PATCH 2/2] perf parse-events: Avoid an uninitialized read Ian Rogers
@ 2020-08-26 11:34 ` Jiri Olsa
2020-08-26 13:20 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Olsa @ 2020-08-26 11:34 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Namhyung Kim, Kajol Jain,
Andi Kleen, Jin Yao, Kan Liang, Adrian Hunter, Leo Yan,
linux-kernel, Stephane Eranian
On Tue, Aug 25, 2020 at 09:29:10PM -0700, Ian Rogers wrote:
> With a fake_pmu the pmu_info isn't populated by perf_pmu__check_alias.
> In this case, don't try to copy the uninitialized values to the evsel.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
thanks,
jirka
> ---
> tools/perf/util/parse-events.c | 30 +++++++++++++++++-------------
> 1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 9f7260e69113..3d7a48b488ed 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1533,19 +1533,23 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
> evsel = __add_event(list, &parse_state->idx, &attr, true,
> get_config_name(head_config), pmu,
> &config_terms, auto_merge_stats, NULL);
> - if (evsel) {
> - evsel->unit = info.unit;
> - evsel->scale = info.scale;
> - evsel->per_pkg = info.per_pkg;
> - evsel->snapshot = info.snapshot;
> - evsel->metric_expr = info.metric_expr;
> - evsel->metric_name = info.metric_name;
> - evsel->pmu_name = name ? strdup(name) : NULL;
> - evsel->use_uncore_alias = use_uncore_alias;
> - evsel->percore = config_term_percore(&evsel->config_terms);
> - }
> -
> - return evsel ? 0 : -ENOMEM;
> + if (!evsel)
> + return -ENOMEM;
> +
> + evsel->pmu_name = name ? strdup(name) : NULL;
> + evsel->use_uncore_alias = use_uncore_alias;
> + evsel->percore = config_term_percore(&evsel->config_terms);
> +
> + if (parse_state->fake_pmu)
> + return 0;
> +
> + evsel->unit = info.unit;
> + evsel->scale = info.scale;
> + evsel->per_pkg = info.per_pkg;
> + evsel->snapshot = info.snapshot;
> + evsel->metric_expr = info.metric_expr;
> + evsel->metric_name = info.metric_name;
> + return 0;
> }
>
> int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
> --
> 2.28.0.297.g1956fa8f8d-goog
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] perf parse-events: Avoid an uninitialized read.
2020-08-26 11:34 ` Jiri Olsa
@ 2020-08-26 13:20 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-08-26 13:20 UTC (permalink / raw)
To: Jiri Olsa
Cc: Ian Rogers, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Namhyung Kim, Kajol Jain, Andi Kleen,
Jin Yao, Kan Liang, Adrian Hunter, Leo Yan, linux-kernel,
Stephane Eranian
Em Wed, Aug 26, 2020 at 01:34:18PM +0200, Jiri Olsa escreveu:
> On Tue, Aug 25, 2020 at 09:29:10PM -0700, Ian Rogers wrote:
> > With a fake_pmu the pmu_info isn't populated by perf_pmu__check_alias.
> > In this case, don't try to copy the uninitialized values to the evsel.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
>
> Acked-by: Jiri Olsa <jolsa@redhat.com>
Thanks, applied.
- Arnaldo
> thanks,
> jirka
>
> > ---
> > tools/perf/util/parse-events.c | 30 +++++++++++++++++-------------
> > 1 file changed, 17 insertions(+), 13 deletions(-)
> >
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index 9f7260e69113..3d7a48b488ed 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -1533,19 +1533,23 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
> > evsel = __add_event(list, &parse_state->idx, &attr, true,
> > get_config_name(head_config), pmu,
> > &config_terms, auto_merge_stats, NULL);
> > - if (evsel) {
> > - evsel->unit = info.unit;
> > - evsel->scale = info.scale;
> > - evsel->per_pkg = info.per_pkg;
> > - evsel->snapshot = info.snapshot;
> > - evsel->metric_expr = info.metric_expr;
> > - evsel->metric_name = info.metric_name;
> > - evsel->pmu_name = name ? strdup(name) : NULL;
> > - evsel->use_uncore_alias = use_uncore_alias;
> > - evsel->percore = config_term_percore(&evsel->config_terms);
> > - }
> > -
> > - return evsel ? 0 : -ENOMEM;
> > + if (!evsel)
> > + return -ENOMEM;
> > +
> > + evsel->pmu_name = name ? strdup(name) : NULL;
> > + evsel->use_uncore_alias = use_uncore_alias;
> > + evsel->percore = config_term_percore(&evsel->config_terms);
> > +
> > + if (parse_state->fake_pmu)
> > + return 0;
> > +
> > + evsel->unit = info.unit;
> > + evsel->scale = info.scale;
> > + evsel->per_pkg = info.per_pkg;
> > + evsel->snapshot = info.snapshot;
> > + evsel->metric_expr = info.metric_expr;
> > + evsel->metric_name = info.metric_name;
> > + return 0;
> > }
> >
> > int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
> > --
> > 2.28.0.297.g1956fa8f8d-goog
> >
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] perf expr: Force encapsulation on expr_id_data
2020-08-26 11:33 ` [PATCH 1/2] perf expr: Force encapsulation on expr_id_data Jiri Olsa
@ 2020-08-26 15:32 ` Ian Rogers
0 siblings, 0 replies; 6+ messages in thread
From: Ian Rogers @ 2020-08-26 15:32 UTC (permalink / raw)
To: Jiri Olsa
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Namhyung Kim, Kajol Jain,
Andi Kleen, Jin Yao, Kan Liang, Adrian Hunter, Leo Yan, LKML,
Stephane Eranian
On Wed, Aug 26, 2020 at 4:34 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Tue, Aug 25, 2020 at 09:29:09PM -0700, Ian Rogers wrote:
> > This patch resolves some undefined behavior where variables in
> > expr_id_data were accessed (for debugging) without being defined. To
> > better enforce the tagged union behavior, the struct is moved into
> > expr.c and accessors provided. Tag values (kinds) are explicitly
> > identified.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> > tools/perf/util/expr.c | 64 ++++++++++++++++++++++++++++++-----
> > tools/perf/util/expr.h | 17 +++-------
> > tools/perf/util/expr.y | 2 +-
> > tools/perf/util/metricgroup.c | 4 +--
> > 4 files changed, 62 insertions(+), 25 deletions(-)
> >
> > diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> > index 53482ef53c41..1ca0992db86b 100644
> > --- a/tools/perf/util/expr.c
> > +++ b/tools/perf/util/expr.c
> > @@ -17,6 +17,25 @@
> > extern int expr_debug;
> > #endif
> >
> > +struct expr_id_data {
> > + union {
> > + double val;
> > + struct {
> > + double val;
> > + const char *metric_name;
> > + const char *metric_expr;
> > + } ref;
> > + struct expr_id *parent;
> > + };
> > +
> > + enum {
> > + EXPR_ID_DATA__VALUE,
> > + EXPR_ID_DATA__REF,
> > + EXPR_ID_DATA__REF_VALUE,
> > + EXPR_ID_DATA__PARENT,
> > + } kind;
>
> I like that, it's more clear than current state ;-)
>
> could you still put a small comment for each enum above,
> as a hint what it's used for?
Thanks, I had a go at this in v2:
https://lore.kernel.org/lkml/20200826153055.2067780-1-irogers@google.com/T/#u
Ian
> thanks,
> jirka
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-26 15:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-26 4:29 [PATCH 1/2] perf expr: Force encapsulation on expr_id_data Ian Rogers
2020-08-26 4:29 ` [PATCH 2/2] perf parse-events: Avoid an uninitialized read Ian Rogers
2020-08-26 11:34 ` Jiri Olsa
2020-08-26 13:20 ` Arnaldo Carvalho de Melo
2020-08-26 11:33 ` [PATCH 1/2] perf expr: Force encapsulation on expr_id_data Jiri Olsa
2020-08-26 15:32 ` Ian Rogers
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).