[PATCH] tools/perf: Fix the check for parameterized field in event term
Athira Rajeev
atrajeev at linux.ibm.com
Tue Mar 24 16:56:54 AEDT 2026
> On 24 Mar 2026, at 12:17 AM, Ian Rogers <irogers at google.com> wrote:
>
> 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`?
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 at linux.ibm.com>
>>>>>
>>>>> Regards,
>>>>> Venkat.
More information about the Linuxppc-dev
mailing list