[PATCH] tools/perf: Fix the check for parameterized field in event term
Ian Rogers
irogers at google.com
Tue Mar 24 05:47:58 AEDT 2026
On Mon, Mar 23, 2026 at 5:19 AM Athira Rajeev <atrajeev at linux.ibm.com> wrote:
>
>
>
> > On 18 Mar 2026, at 12:05 PM, Athira Rajeev <atrajeev at linux.ibm.com> wrote:
> >
> >
> >
> >> On 17 Mar 2026, at 9:39 PM, Ian Rogers <irogers at google.com> wrote:
> >>
> >> On Tue, Mar 17, 2026 at 1:56 AM Venkat <venkat88 at linux.ibm.com> wrote:
> >>>
> >>>> On 14 Mar 2026, at 2:03 PM, Athira Rajeev <atrajeev at linux.ibm.com> wrote:
> >>>>
> >>>> The format_alias() function in util/pmu.c has a check to
> >>>> detect whether the event has parameterized field ( =? ).
> >>>> The string alias->terms contains the event and if the event
> >>>> has user configurable parameter, there will be presence of
> >>>> sub string "=?" in the alias->terms.
> >>>>
> >>>> Snippet of code:
> >>>>
> >>>> /* Paramemterized events have the parameters shown. */
> >>>> if (strstr(alias->terms, "=?")) {
> >>>> /* No parameters. */
> >>>> snprintf(buf, len, "%.*s/%s/", (int)pmu_name_len, pmu->name, alias->name);
> >>>>
> >>>> if "strstr" contains the substring, it returns a pointer
> >>>> and hence enters the above check which is not the expected
> >>>> check. And hence "perf list" doesn't have the parameterized
> >>>> fields in the result.
> >>>>
> >>>> Fix this check to use:
> >>>>
> >>>> if (!strstr(alias->terms, "=?")) {
> >>>>
> >>>> With this change, perf list shows the events correctly with
> >>>> the strings showing parameters.
> >>>>
> >>>> Signed-off-by: Athira Rajeev <atrajeev at linux.ibm.com>
> >>
> >> Thanks Athira, Sashiko is noting in its review:
> >> https://sashiko.dev/#/patchset/20260314083304.75321-1-atrajeev%40linux.ibm.com
> >
> > Thanks Ian for pointing this. Its interesting to see this review.
> > I will check through the review.
> >
> > Thanks
> > Athira
> >>
> >> By inverting this check, parameterized events now proceed to
> >> parse_events_terms() and the rest of format_alias().
> >>
> >> If a parameterized event uses a built-in perf keyword for its parameter name
> >> (e.g., config=?), the lexer parses it as a predefined term token, which sets
> >> term->config to NULL.
>
> Hi All,
>
> sashiko brought up a scenario :
>
> “If a parameterized event uses a built-in perf keyword for its parameter name
> (e.g., config=?), the lexer parses it as a predefined term token, which sets
> term->config to NULL."
>
> Existing usage for parameterized events in perf:
> ======================================
>
> In perf, “?” is often used in event aliases to indicate that a parameter needs to be provided
> by the user (e.g., param1=?). This information is exposed via “format” in sysfs too. The current way,
> how this is consumed in different PMU drivers is: example:
>
> # cat /sys/devices/pmu/format/param1
> config:12-15
>
> # cat /sys/devices/pmu/format/param2
> config1:0-17
>
> Here param1 uses bits 12:15 in config and param2 used bits 0:17 in config1
>
> And event will use these parameterized fields:
> #cat /sys/devices/pmu/events/event1 domain=1,param1=?,param2=?
>
> “Perf list” expects "param1=?,param2=?” In the event listing
> # perf list|grep event1
> pmu/event1,param1=?,param2=?/ [Kernel PMU event]
>
> The patch here address fix in “strstr” check in format_alias for “perf list” to display these user expected “?” fields
> in event alias. Traditionally, perf treats config, config1, and config2 etc as hardcoded numeric
> keywords or builtin keyword.
>
> The scneario sashiko brought up is interesting. Where a parameterized event
> uses a built-in perf keyword for its parameter name.
>
> sashiko review : Scenario where parameterized event uses a built-in perf keyword
> ============================================================
> Example, having “config2” in sysfs format-driven logic. Below is an example case.
>
> #cat /sys/devices/pmu/events/event1
> config2=?,domain=1,param1=?,param2=?
>
> # cat /sys/devices/hv_24x7/format/config2
> config2:0-16
>
> Here “perf list” will show as (null) or might crash too as sashiko review pointed out.
>
> # perf list|grep event1
> pmu/event1,(null)=?,param1=?,param2=?/ [Kernel PMU event]
>
> This is because term->config is null for built-in keywords. perf lexer sets this to NULL
> for predefined keywords ( like 'config', 'period'), storing the type in the enum instead.
>
> To fix displaying the “=?” in “perf list” for keywords like config2, format_alias() function needs to handle
> null term->config , ie by using parse_events__term_type_str() to map NULL config
> pointers back to their string keywords
>
> But this only will fix the “perf list” part of it. “Perf stat” or record will still fail since
> currently these keywords are not considered as placeholders for “?” values.
>
> If a PMU wants to use any builtin-keywords in parameters and want to ensure
> it is set by user, this could be useful. Again considering this as a use case, we need other changes
> to allow these built-in keywords to function as placeholders (e.g., config2=?) within a PMU alias.
>
> I am adding changes which I have identified needed for this support.
> If it is better to post this separately as patch set and discuss, definitely will do that.
> I wanted to run the idea of this new scenario with everyone. Apologies for the long text.
>
> Thanks and looking for comments.
>
>
> From 21179a76be97ef81c69a9492f0f432dec5b5361c Mon Sep 17 00:00:00 2001
> From: Athira Rajeev <atrajeev at linux.ibm.com>
> Date: Mon, 23 Mar 2026 08:38:30 -0400
> Subject: [PATCH] tools/perf: Enable parameterized values for keywords like
> config1
>
> PMU defines events that require user-supplied parameters (like chip
> or core) are packed into specific bit-fields of the different config
> registers. Traditionally, perf treats config, config1, and config2
> as hardcoded numeric keywords, causing it to bypass the sysfs
> format-driven bit-masking logic.
>
> This patch allows these built-in keywords to function as placeholders
> (e.g., config2=?) within a PMU alias.
>
> Changes includes:
> - Updated config_term_pmu in parse-events.c to recognize string
> placeholders for event terms. otherwise it will expect those to
> be of type PARSE_EVENTS__TERM_TYPE_NUM
> - Updated pmu_config_term in pmu.c to make sure if keywords are used
> as placeholders , they pass through sysfs based format logic
> (pmu_find_format) and pmu_resolve_param_term() logic
> - Modified format_alias() to ensure "perf list" display the parameterized
> events correctly
> - Replace snprintf with scnprintf in buffer offset calculations to
> ensure the 'used' count will not exceed the "len"
>
> Signed-off-by: Athira Rajeev <atrajeev at linux.ibm.com>
> ---
> tools/perf/util/parse-events.c | 5 +++++
> tools/perf/util/pmu.c | 37 +++++++++++++++++++++++++---------
> 2 files changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index b9efb296bba5..282b0a0a5340 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1051,6 +1051,11 @@ static int config_term_pmu(struct perf_event_attr *attr,
> */
> return 0;
> }
> +
> + if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR && term->val.str)
> + if (strstr(term->val.str, "?"))
> + return 0;
nit: Since the nested if spans more than two lines, could you add
curly braces {} to the outer if statement. Could you also add a
comment explaining why the runtime parameter causes an early return
and why you used `strstr` instead of `strcmp`?
> +
> return config_term_common(attr, term, parse_state);
> }
>
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 23337d2fa281..2c4503e1a11b 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1442,10 +1442,19 @@ static int pmu_resolve_param_term(struct parse_events_term *term,
> __u64 *value)
> {
> struct parse_events_term *t;
> + const char *name = term->config;
> +
> + if (!term->config)
I think you meant "if (!name)" here.
> + name = parse_events__term_type_str(term->type_term);
>
> list_for_each_entry(t, &head_terms->terms, list) {
> + const char *t_name = t->config;
> +
> + if (!t_name)
> + t_name = parse_events__term_type_str(t->type_term);
> +
> if (t->type_val == PARSE_EVENTS__TERM_TYPE_NUM &&
> - t->config && !strcmp(t->config, term->config)) {
> + t_name && name && !strcmp(t_name, name)) {
> t->used = true;
> *value = t->val.num;
> return 0;
> @@ -1453,7 +1462,7 @@ static int pmu_resolve_param_term(struct parse_events_term *term,
> }
>
> if (verbose > 0)
> - printf("Required parameter '%s' not specified\n", term->config);
> + printf("Required parameter '%s' not specified\n", name);
>
> return -1;
> }
> @@ -1494,6 +1503,7 @@ static int pmu_config_term(const struct perf_pmu *pmu,
> struct perf_pmu_format *format;
> __u64 *vp;
> __u64 val, max_val;
> + const char *name;
>
> /*
> * If this is a parameter we've already used for parameterized-eval,
> @@ -1507,7 +1517,8 @@ static int pmu_config_term(const struct perf_pmu *pmu,
> * traditionally have had to handle not having a PMU. An alias may
> * have hard coded config values, optionally apply them below.
> */
> - if (parse_events__is_hardcoded_term(term)) {
> + if (parse_events__is_hardcoded_term(term) &&
> + term->type_val != PARSE_EVENTS__TERM_TYPE_STR) {
What is being guarded against in this case? Could you update or add a comment.
> /* Config terms set all bits in the config. */
> DECLARE_BITMAP(bits, PERF_PMU_FORMAT_BITS);
>
> @@ -1576,7 +1587,11 @@ static int pmu_config_term(const struct perf_pmu *pmu,
> return 0;
> }
>
> - format = pmu_find_format(&pmu->format, term->config);
> + name = term->config;
> + if (!name)
> + name = parse_events__term_type_str(term->type_term);
> +
> + format = pmu_find_format(&pmu->format, name);
> if (!format) {
> char *pmu_term = pmu_formats_string(&pmu->format);
> char *unknown_term;
> @@ -2117,7 +2132,7 @@ static char *format_alias(char *buf, int len, const struct perf_pmu *pmu,
> skip_duplicate_pmus);
>
> /* Paramemterized events have the parameters shown. */
> - if (strstr(alias->terms, "=?")) {
> + if (!strstr(alias->terms, "=?")) {
I wonder if it would be safer if we did a `str_ends_with(alias->terms,
"=?")`. This likely applies to the other strstr usage.
Thanks,
Ian
> /* No parameters. */
> snprintf(buf, len, "%.*s/%s/", (int)pmu_name_len, pmu->name, alias->name);
> return buf;
> @@ -2129,15 +2144,19 @@ static char *format_alias(char *buf, int len, const struct perf_pmu *pmu,
> pr_err("Failure to parse '%s' terms '%s': %d\n",
> alias->name, alias->terms, ret);
> parse_events_terms__exit(&terms);
> - snprintf(buf, len, "%.*s/%s/", (int)pmu_name_len, pmu->name, alias->name);
> + scnprintf(buf, len, "%.*s/%s/", (int)pmu_name_len, pmu->name, alias->name);
> return buf;
> }
> - used = snprintf(buf, len, "%.*s/%s", (int)pmu_name_len, pmu->name, alias->name);
> + used = scnprintf(buf, len, "%.*s/%s", (int)pmu_name_len, pmu->name, alias->name);
>
> list_for_each_entry(term, &terms.terms, list) {
> + const char *name = term->config;
> +
> + if (!name)
> + name = parse_events__term_type_str(term->type_term);
> if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
> - used += snprintf(buf + used, sub_non_neg(len, used),
> - ",%s=%s", term->config,
> + used += scnprintf(buf + used, sub_non_neg(len, used),
> + ",%s=%s", name,
> term->val.str);
> }
> parse_events_terms__exit(&terms);
> -- 2.47.3
>
>
>
> Thanks
> Athira
>
> >>
> >> Does this cause a NULL pointer dereference in the term iteration loop?
> >>
> >> list_for_each_entry(term, &terms.terms, list) {
> >> if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
> >> used += snprintf(buf + used, sub_non_neg(len, used),
> >> ",%s=%s", term->config,
> >> term->val.str);
> >> }
> >>
> >> Passing a NULL pointer to the %s format specifier for term->config might
> >> cause a segmentation fault or formatting corruption.
> >>
> >> Can this also lead to a stack buffer overflow due to how snprintf() return
> >> values are accumulated?
> >>
> >> snprintf() returns the number of characters that would have been written.
> >> If the event names and terms are long enough, the "used" variable can exceed
> >> "len" (which is typically 1024 bytes).
> >>
> >> When evaluating "buf + used" in the same loop:
> >>
> >> used += snprintf(buf + used, sub_non_neg(len, used), ...);
> >>
> >> If "used" exceeds 1024, this creates an out-of-bounds pointer. Modern
> >> compilers can use this undefined behavior to infer that "used" <= 1024
> >> must be true, which optimizes out the safety check inside sub_non_neg():
> >>
> >> static int sub_non_neg(int a, int b)
> >> {
> >> if (b > a)
> >> return 0;
> >> return a - b;
> >> }
> >>
> >> The compiler can reduce this to simply "a - b" (or "len - used").
> >> If "used" is actually greater than 1024, "len - used" evaluates to a
> >> negative integer. This negative value is implicitly cast to size_t for
> >> snprintf(), resulting in a massive size limit.
> >>
> >> Would this cause snprintf() to write past the end of the stack buffer
> >> without bounds checking? Using scnprintf() might prevent "used" from
> >> exceeding "len".
> >>
> >> Thanks,
> >> Ian
> >>
> >>>> ---
> >>>> tools/perf/util/pmu.c | 2 +-
> >>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> >>>> index 23337d2fa281..0b8d58543f17 100644
> >>>> --- a/tools/perf/util/pmu.c
> >>>> +++ b/tools/perf/util/pmu.c
> >>>> @@ -2117,7 +2117,7 @@ static char *format_alias(char *buf, int len, const struct perf_pmu *pmu,
> >>>> skip_duplicate_pmus);
> >>>>
> >>>> /* Paramemterized events have the parameters shown. */
> >>>> - if (strstr(alias->terms, "=?")) {
> >>>> + if (!strstr(alias->terms, "=?")) {
> >>>> /* No parameters. */
> >>>> snprintf(buf, len, "%.*s/%s/", (int)pmu_name_len, pmu->name, alias->name);
> >>>> return buf;
> >>>> --
> >>>> 2.47.3
> >>>>
> >>>
> >>> Tested this patch, and its working as expected.
> >>>
> >>> Before Patch:
> >>>
> >>> ./perf list hv_24x7 | grep -i CPM_EXT_INT_OS
> >>> hv_24x7/CPM_EXT_INT_OS/ [Kernel PMU event]
> >>>
> >>> After Patch:
> >>>
> >>> ./perf list hv_24x7 | grep -i CPM_EXT_INT_OS
> >>> hv_24x7/CPM_EXT_INT_OS,domain=?,core=?/ [Kernel PMU event]
> >>>
> >>>
> >>> ./perf stat -e hv_24x7/PM_PAU_CYC,chip=0/
> >>>
> >>>
> >>> Performance counter stats for 'system wide':
> >>>
> >>> 2018866563 hv_24x7/PM_PAU_CYC,chip=0/
> >>>
> >>> 229.938231314 seconds time elapsed
> >>>
> >>> Tested-by: Venkat Rao Bagalkote <venkat88 at linux.ibm.com>
> >>>
> >>> Regards,
> >>> Venkat.
>
>
More information about the Linuxppc-dev
mailing list