| 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: <99e9614d-a956-40dc-a86b-a4c5aea0fe8a@linaro.org>
Date: Fri, 12 Dec 2025 16:43:57 +0200
From: James Clark <james.clark@...aro.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Linux perf Profiling <linux-perf-users@...r.kernel.org>,
linux-kernel@...r.kernel.org, coresight@...ts.linaro.org,
linux-arm-kernel@...ts.infradead.org, Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>,
Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>,
Suzuki K Poulose <suzuki.poulose@....com>, Mike Leach
<mike.leach@...aro.org>, John Garry <john.g.garry@...cle.com>,
Will Deacon <will@...nel.org>, Leo Yan <leo.yan@...ux.dev>, leo.yan@....com
Subject: Re: [PATCH v2 01/12] perf parse-events: Refactor get_config_terms()
to remove macros
On 09/12/2025 20:09, Ian Rogers wrote:
> On Tue, Dec 9, 2025 at 9:55 AM James Clark <james.clark@...aro.org> wrote:
>>
>> On 09/12/2025 15:58, Ian Rogers wrote:
>>> On Tue, Dec 9, 2025 at 4:48 AM James Clark <james.clark@...aro.org> wrote:
>>>>
>>>> On 08/12/2025 2:22 pm, James Clark wrote:
>>>>> The ADD_CONFIG_TERM() macros build the __type argument out of a partial
>>>>> EVSEL__CONFIG_TERM_x enum name. This means that they can't be called
>>>>> from a function where __type is a variable and it's also impossible to
>>>>> grep the codebase to find usages of these enums as they're never typed
>>>>> in full.
>>>>>
>>>>> Fix this by removing the macros and replacing them with an
>>>>> add_config_term() function. It seems the main reason these existed in
>>>>> the first place was to avoid type punning and to write to a specific
>>>>> field in the union, but the same thing can be achieved with a single
>>>>> write to a u64 'val' field.
>>>>>
>>>>> Signed-off-by: James Clark <james.clark@...aro.org>
>>>>> ---
>>>>> tools/perf/util/evsel_config.h | 1 +
>>>>> tools/perf/util/parse-events.c | 146 ++++++++++++++++++++++++-----------------
>>>>> 2 files changed, 86 insertions(+), 61 deletions(-)
>>>>>
>>>>> diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
>>>>> index bcd3a978f0c4..685fd8d5c4a8 100644
>>>>> --- a/tools/perf/util/evsel_config.h
>>>>> +++ b/tools/perf/util/evsel_config.h
>>>>> @@ -50,6 +50,7 @@ struct evsel_config_term {
>>>>> u64 cfg_chg;
>>>>> char *str;
>>>>> int cpu;
>>>>> + u64 val;
>>>>> } val;
>>>>> bool weak;
>>>>> };
>>>>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>>>>> index 17c1c36a7bf9..d5b009b4ebab 100644
>>>>> --- a/tools/perf/util/parse-events.c
>>>>> +++ b/tools/perf/util/parse-events.c
>>>>> @@ -1116,105 +1116,107 @@ static int config_attr(struct perf_event_attr *attr,
>>>>> return 0;
>>>>> }
>>>>>
>>>>> -static int get_config_terms(const struct parse_events_terms *head_config,
>>>>> - struct list_head *head_terms)
>>>>> +static struct evsel_config_term *add_config_term(enum evsel_term_type type,
>>>>> + struct list_head *head_terms,
>>>>> + bool weak)
>>>>> {
>>>>> -#define ADD_CONFIG_TERM(__type, __weak) \
>>>>> - struct evsel_config_term *__t; \
>>>>> - \
>>>>> - __t = zalloc(sizeof(*__t)); \
>>>>> - if (!__t) \
>>>>> - return -ENOMEM; \
>>>>> - \
>>>>> - INIT_LIST_HEAD(&__t->list); \
>>>>> - __t->type = EVSEL__CONFIG_TERM_ ## __type; \
>>>>> - __t->weak = __weak; \
>>>>> - list_add_tail(&__t->list, head_terms)
>>>>> -
>>>>> -#define ADD_CONFIG_TERM_VAL(__type, __name, __val, __weak) \
>>>>> -do { \
>>>>> - ADD_CONFIG_TERM(__type, __weak); \
>>>>> - __t->val.__name = __val; \
>>>>> -} while (0)
>>>>> + struct evsel_config_term *t;
>>>>>
>>>>> -#define ADD_CONFIG_TERM_STR(__type, __val, __weak) \
>>>>> -do { \
>>>>> - ADD_CONFIG_TERM(__type, __weak); \
>>>>> - __t->val.str = strdup(__val); \
>>>>> - if (!__t->val.str) { \
>>>>> - zfree(&__t); \
>>>>> - return -ENOMEM; \
>>>>> - } \
>>>>> - __t->free_str = true; \
>>>>> -} while (0)
>>>>> + t = zalloc(sizeof(*t));
>>>>> + if (!t)
>>>>> + return NULL;
>>>>> +
>>>>> + INIT_LIST_HEAD(&t->list);
>>>>> + t->type = type;
>>>>> + t->weak = weak;
>>>>> + list_add_tail(&t->list, head_terms);
>>>>>
>>>>> + return t;
>>>>> +}
>>>>> +
>>>>> +static int get_config_terms(const struct parse_events_terms *head_config,
>>>>> + struct list_head *head_terms)
>>>>> +{
>>>>> struct parse_events_term *term;
>>>>>
>>>>> list_for_each_entry(term, &head_config->terms, list) {
>>>>> + struct evsel_config_term *new_term;
>>>>> + enum evsel_term_type new_type;
>>>>> + char *str = NULL;
>>>>> + u64 val;
>>>>> +
>>>>> switch (term->type_term) {
>>>>> case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
>>>>> - ADD_CONFIG_TERM_VAL(PERIOD, period, term->val.num, term->weak);
>>>>> + new_type = EVSEL__CONFIG_TERM_PERIOD;
>>>>> + val = term->val.num;
>>>>> break;
>>>>> case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ:
>>>>> - ADD_CONFIG_TERM_VAL(FREQ, freq, term->val.num, term->weak);
>>>>> + new_type = EVSEL__CONFIG_TERM_FREQ;
>>>>> + val = term->val.num;
>>>>> break;
>>>>> case PARSE_EVENTS__TERM_TYPE_TIME:
>>>>> - ADD_CONFIG_TERM_VAL(TIME, time, term->val.num, term->weak);
>>>>> + new_type = EVSEL__CONFIG_TERM_TIME;
>>>>> + val = term->val.num;
>>>>> break;
>>>>> case PARSE_EVENTS__TERM_TYPE_CALLGRAPH:
>>>>> - ADD_CONFIG_TERM_STR(CALLGRAPH, term->val.str, term->weak);
>>>>> + new_type = EVSEL__CONFIG_TERM_CALLGRAPH;
>>>>> + str = term->val.str;
>>>>> break;
>>>>> case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE:
>>>>> - ADD_CONFIG_TERM_STR(BRANCH, term->val.str, term->weak);
>>>>> + new_type = EVSEL__CONFIG_TERM_BRANCH;
>>>>> + str = term->val.str;
>>>>> break;
>>>>> case PARSE_EVENTS__TERM_TYPE_STACKSIZE:
>>>>> - ADD_CONFIG_TERM_VAL(STACK_USER, stack_user,
>>>>> - term->val.num, term->weak);
>>>>> + new_type = EVSEL__CONFIG_TERM_STACK_USER;
>>>>> + val = term->val.num;
>>>>> break;
>>>>> case PARSE_EVENTS__TERM_TYPE_INHERIT:
>>>>> - ADD_CONFIG_TERM_VAL(INHERIT, inherit,
>>>>> - term->val.num ? 1 : 0, term->weak);
>>>>> + new_type = EVSEL__CONFIG_TERM_INHERIT;
>>>>> + val = term->val.num ? 1 : 0;
>>>>> break;
>>>>> case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
>>>>> - ADD_CONFIG_TERM_VAL(INHERIT, inherit,
>>>>> - term->val.num ? 0 : 1, term->weak);
>>>>> + new_type = EVSEL__CONFIG_TERM_INHERIT;
>>>>> + val = term->val.num ? 0 : 1;
>>>>> break;
>>>>> case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
>>>>> - ADD_CONFIG_TERM_VAL(MAX_STACK, max_stack,
>>>>> - term->val.num, term->weak);
>>>>> + new_type = EVSEL__CONFIG_TERM_MAX_STACK;
>>>>> + val = term->val.num;
>>>>> break;
>>>>> case PARSE_EVENTS__TERM_TYPE_MAX_EVENTS:
>>>>> - ADD_CONFIG_TERM_VAL(MAX_EVENTS, max_events,
>>>>> - term->val.num, term->weak);
>>>>> + new_type = EVSEL__CONFIG_TERM_MAX_EVENTS;
>>>>> + val = term->val.num;
>>>>> break;
>>>>> case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
>>>>> - ADD_CONFIG_TERM_VAL(OVERWRITE, overwrite,
>>>>> - term->val.num ? 1 : 0, term->weak);
>>>>> + new_type = EVSEL__CONFIG_TERM_OVERWRITE;
>>>>> + val = term->val.num ? 1 : 0;
>>>>> break;
>>>>> case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
>>>>> - ADD_CONFIG_TERM_VAL(OVERWRITE, overwrite,
>>>>> - term->val.num ? 0 : 1, term->weak);
>>>>> + new_type = EVSEL__CONFIG_TERM_OVERWRITE;
>>>>> + val = term->val.num ? 0 : 1;
>>>>> break;
>>>>> case PARSE_EVENTS__TERM_TYPE_DRV_CFG:
>>>>> - ADD_CONFIG_TERM_STR(DRV_CFG, term->val.str, term->weak);
>>>>> + new_type = EVSEL__CONFIG_TERM_DRV_CFG;
>>>>> + str = term->val.str;
>>>>> break;
>>>>> case PARSE_EVENTS__TERM_TYPE_PERCORE:
>>>>> - ADD_CONFIG_TERM_VAL(PERCORE, percore,
>>>>> - term->val.num ? true : false, term->weak);
>>>>> + new_type = EVSEL__CONFIG_TERM_PERCORE;
>>>>> + val = term->val.num ? true : false;
>>>>> break;
>>>>> case PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT:
>>>>> - ADD_CONFIG_TERM_VAL(AUX_OUTPUT, aux_output,
>>>>> - term->val.num ? 1 : 0, term->weak);
>>>>> + new_type = EVSEL__CONFIG_TERM_AUX_OUTPUT;
>>>>> + val = term->val.num ? 1 : 0;
>>>>> break;
>>>>> case PARSE_EVENTS__TERM_TYPE_AUX_ACTION:
>>>>> - ADD_CONFIG_TERM_STR(AUX_ACTION, term->val.str, term->weak);
>>>>> + new_type = EVSEL__CONFIG_TERM_AUX_ACTION;
>>>>> + str = term->val.str;
>>>>> break;
>>>>> case PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE:
>>>>> - ADD_CONFIG_TERM_VAL(AUX_SAMPLE_SIZE, aux_sample_size,
>>>>> - term->val.num, term->weak);
>>>>> + new_type = EVSEL__CONFIG_TERM_AUX_SAMPLE_SIZE;
>>>>> + val = term->val.num;
>>>>> break;
>>>>> case PARSE_EVENTS__TERM_TYPE_RATIO_TO_PREV:
>>>>> - ADD_CONFIG_TERM_STR(RATIO_TO_PREV, term->val.str, term->weak);
>>>>> + new_type = EVSEL__CONFIG_TERM_RATIO_TO_PREV;
>>>>> + str = term->val.str;
>>>>> break;
>>>>> case PARSE_EVENTS__TERM_TYPE_USER:
>>>>> case PARSE_EVENTS__TERM_TYPE_CONFIG:
>>>>> @@ -1229,7 +1231,23 @@ do { \
>>>>> case PARSE_EVENTS__TERM_TYPE_RAW:
>>>>> case PARSE_EVENTS__TERM_TYPE_CPU:
>>>>> default:
>>>>> - break;
>>>>> + /* Don't add a new term for these ones */
>>>>> + continue;
>>>>> + }
>>>>> +
>>>>> + new_term = add_config_term(new_type, head_terms, term->weak);
>>>>> + if (!new_term)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + if (str) {
>>>>> + new_term->val.str = strdup(str);
>>>>> + if (!new_term->val.str) {
>>>>> + zfree(&new_term);
>>>>> + return -ENOMEM;
>>>>> + }
>>>>> + new_term->free_str = true;
>>>>> + } else {
>>>>
>>>> This will incorrectly hit the else if term->val.str is NULL. Not sure if
>>>> that can happen but will fix anyway.
>>>>
>>>>> + new_term->val.val = val;
>>>>
>>>> There's an uninitialized variable warning for val here on release
>>>> builds. Will fix too
>>>
>>> I'm not sure how I feel about the change, but did you check that it is
>>> ubsan clean? I worry it is introducing writes to "val" and then reads
>>> from say "period" in the evsel_config_term val union. I believe ubsan
>>> will flag this as:
>>> https://en.cppreference.com/w/cpp/language/union.html
>>> "It is undefined behavior to read from the member of the union that
>>> wasn't most recently written."
>>>
>>> Thanks,
>>> Ian
>>>
>>
>> Maybe for C++, but not for C [1]:
>>
>> Accessing my_union.i after most recently writing to the other member,
>> my_union.d, is an allowed form of type-punning in C, provided that the
>> member read is not larger than the one whose value was set
>>
>> Type punning with unions is widely used in the kernel and there are a
>> couple of instances in Perf. There are tons of discussions about it on
>> the mailing lists too. I can check it with ubsan though.
>
> Thanks. I'm mostly worried about ubsan being clean. One of those
> mailing list discussions, or sadly outside the usual trolls lack of
> discussion, is my trying to remove a prevalent source of type punning:
> https://lore.kernel.org/lkml/20251016205126.2882625-1-irogers@google.com/
>
> Ian
>
No ubsan issues on any Perf tests from this, but I did forget a free in
the new test that I saw with asan so I'll send a new version.
>> [1]: https://en.wikipedia.org/wiki/Type_punning#C_and_C++
>>
>>>>> }
>>>>> }
>>>>> return 0;
>>>>> @@ -1290,10 +1308,16 @@ static int get_config_chgs(struct perf_pmu *pmu, struct parse_events_terms *head
>>>>> }
>>>>> }
>>>>>
>>>>> - if (bits)
>>>>> - ADD_CONFIG_TERM_VAL(CFG_CHG, cfg_chg, bits, false);
>>>>> + if (bits) {
>>>>> + struct evsel_config_term *new_term;
>>>>> +
>>>>> + new_term = add_config_term(EVSEL__CONFIG_TERM_CFG_CHG,
>>>>> + head_terms, false);
>>>>> + if (!new_term)
>>>>> + return -ENOMEM;
>>>>> + new_term->val.cfg_chg = bits;
>>>>> + }
>>>>>
>>>>> -#undef ADD_CONFIG_TERM
>>>>> return 0;
>>>>> }
>>>>>
>>>>>
>>>>
>>
Powered by blists - more mailing lists