Re: [PATCH] tools/perf: Fix the check for parameterized field in event term

[Athira Rajeev <atrajeev@linux.ibm.com>]

> On 24 Mar 2026, at 12:17 AM, Ian Rogers <irogers@google.com> wrote:
> 
> On Mon, Mar 23, 2026 at 5:19 AM Athira Rajeev <atrajeev@linux.ibm.com> wrote:
>> 
>> 
>> 
>>> On 18 Mar 2026, at 12:05 PM, Athira Rajeev <atrajeev@linux.ibm.com> wrote:
>>> 
>>> 
>>> 
>>>> On 17 Mar 2026, at 9:39 PM, Ian Rogers <irogers@google.com> wrote:
>>>> 
>>>> On Tue, Mar 17, 2026 at 1:56 AM Venkat <venkat88@linux.ibm.com> wrote:
>>>>> 
>>>>>> On 14 Mar 2026, at 2:03 PM, Athira Rajeev <atrajeev@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@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@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@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`?


Hi Ian

Thanks for checking and responding with review comments.
Sure I will handle this.
> 
>> +
>>        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.
Yes, will correct this 
> 
>> +               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.

Sure
> 
>>                /* 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.
Ok , 

I will post a V2 patchset with all changes, splitting the fix patch which address the “str” check above
and changes to allow using the builtin-keywords as place holders within PMU event alias

Thanks
Athira
> 
> 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@linux.ibm.com>
>>>>> 
>>>>> Regards,
>>>>> Venkat.