* [PATCH linux-next] perf parse-events: Fix big-endian 'overwrite' by writing correct union member
@ 2026-02-12 12:53 Thomas Richter
2026-02-12 18:17 ` Ian Rogers
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Richter @ 2026-02-12 12:53 UTC (permalink / raw)
To: linux-kernel, linux-s390, linux-perf-users, acme, namhyung,
james.clark, irogers
Cc: agordeev, gor, sumanthk, hca, japo, Thomas Richter
The "Read backward ring buffer" test crashes on big-endian (e.g. s390x)
due to a NULL dereference when the backward mmap path isn't enabled.
Reproducer:
# ./perf test -F 'Read backward ring buffer'
Segmentation fault (core dumped)
# uname -m
s390x
#
Root cause:
get_config_terms() stores into evsel_config_term::val.val (u64) while later
code reads boolean fields such as evsel_config_term::val.overwrite.
On big-endian the 1-byte boolean is left-aligned, so writing
evsel_config_term::val.val = 1 is read back as
evsel_config_term::val.overwrite = 0,
leaving backward mmap disabled and a NULL map being used.
Store values in the union member that matches the term type, e.g.:
/* for OVERWRITE */
new_term->val.overwrite = 1; /* not new_term->val.val = 1 */
to fix this.
Impact:
Enables backward mmap on big-endian and prevents the crash.
No change on little-endian.
Output after:
# ./perf test -Fv 44
--- start ---
Using CPUID IBM,9175,705,ME1,3.8,002f
mmap size 1052672B
mmap size 8192B
---- end ----
44: Read backward ring buffer : Ok
#
Fixes: 159ca97cd97c ("perf parse-events: Refactor get_config_terms() to remove macros")
Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
Reviewed-by: Jan Polensky <japo@linux.ibm.com>
Cc: James Clark <james.clark@linaro.org>
Cc: Ian Rogers <irogers@google.com>
---
tools/perf/util/parse-events.c | 49 +++++++++++++++++++++++++++++++++-
1 file changed, 48 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index d4647ded340f..12fe5392c832 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1250,7 +1250,54 @@ static int get_config_terms(const struct parse_events_terms *head_config,
}
new_term->free_str = true;
} else {
- new_term->val.val = val;
+ switch (new_type) {
+ case EVSEL__CONFIG_TERM_PERIOD:
+ new_term->val.period = val;
+ break;
+ case EVSEL__CONFIG_TERM_FREQ:
+ new_term->val.freq = val;
+ break;
+ case EVSEL__CONFIG_TERM_TIME:
+ new_term->val.time = val;
+ break;
+ case EVSEL__CONFIG_TERM_STACK_USER:
+ new_term->val.stack_user = val;
+ break;
+ case EVSEL__CONFIG_TERM_INHERIT:
+ new_term->val.inherit = val;
+ break;
+ case EVSEL__CONFIG_TERM_OVERWRITE:
+ new_term->val.overwrite = val;
+ break;
+ case EVSEL__CONFIG_TERM_MAX_STACK:
+ new_term->val.max_stack = val;
+ break;
+ case EVSEL__CONFIG_TERM_MAX_EVENTS:
+ new_term->val.max_events = val;
+ break;
+ case EVSEL__CONFIG_TERM_PERCORE:
+ new_term->val.percore = val;
+ break;
+ case EVSEL__CONFIG_TERM_AUX_OUTPUT:
+ new_term->val.aux_output = val;
+ break;
+ case EVSEL__CONFIG_TERM_AUX_SAMPLE_SIZE:
+ new_term->val.aux_sample_size = val;
+ break;
+ case EVSEL__CONFIG_TERM_CALLGRAPH:
+ case EVSEL__CONFIG_TERM_DRV_CFG:
+ case EVSEL__CONFIG_TERM_BRANCH:
+ case EVSEL__CONFIG_TERM_AUX_ACTION:
+ case EVSEL__CONFIG_TERM_USR_CHG_CONFIG:
+ case EVSEL__CONFIG_TERM_USR_CHG_CONFIG1:
+ case EVSEL__CONFIG_TERM_USR_CHG_CONFIG2:
+ case EVSEL__CONFIG_TERM_USR_CHG_CONFIG3:
+ case EVSEL__CONFIG_TERM_USR_CHG_CONFIG4:
+ case EVSEL__CONFIG_TERM_RATIO_TO_PREV:
+ default:
+ new_term->val.val = val;
+ break;
+ }
}
}
return 0;
--
2.53.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH linux-next] perf parse-events: Fix big-endian 'overwrite' by writing correct union member
2026-02-12 12:53 [PATCH linux-next] perf parse-events: Fix big-endian 'overwrite' by writing correct union member Thomas Richter
@ 2026-02-12 18:17 ` Ian Rogers
2026-02-16 10:04 ` Thomas Richter
0 siblings, 1 reply; 5+ messages in thread
From: Ian Rogers @ 2026-02-12 18:17 UTC (permalink / raw)
To: Thomas Richter
Cc: linux-kernel, linux-s390, linux-perf-users, acme, namhyung,
james.clark, agordeev, gor, sumanthk, hca, japo
On Thu, Feb 12, 2026 at 4:53 AM Thomas Richter <tmricht@linux.ibm.com> wrote:
>
> The "Read backward ring buffer" test crashes on big-endian (e.g. s390x)
> due to a NULL dereference when the backward mmap path isn't enabled.
>
> Reproducer:
> # ./perf test -F 'Read backward ring buffer'
> Segmentation fault (core dumped)
> # uname -m
> s390x
> #
>
> Root cause:
> get_config_terms() stores into evsel_config_term::val.val (u64) while later
> code reads boolean fields such as evsel_config_term::val.overwrite.
> On big-endian the 1-byte boolean is left-aligned, so writing
> evsel_config_term::val.val = 1 is read back as
> evsel_config_term::val.overwrite = 0,
> leaving backward mmap disabled and a NULL map being used.
>
> Store values in the union member that matches the term type, e.g.:
> /* for OVERWRITE */
> new_term->val.overwrite = 1; /* not new_term->val.val = 1 */
> to fix this.
>
> Impact:
> Enables backward mmap on big-endian and prevents the crash.
> No change on little-endian.
>
> Output after:
> # ./perf test -Fv 44
> --- start ---
> Using CPUID IBM,9175,705,ME1,3.8,002f
> mmap size 1052672B
> mmap size 8192B
> ---- end ----
> 44: Read backward ring buffer : Ok
> #
>
> Fixes: 159ca97cd97c ("perf parse-events: Refactor get_config_terms() to remove macros")
> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
> Reviewed-by: Jan Polensky <japo@linux.ibm.com>
> Cc: James Clark <james.clark@linaro.org>
> Cc: Ian Rogers <irogers@google.com>
> ---
> tools/perf/util/parse-events.c | 49 +++++++++++++++++++++++++++++++++-
> 1 file changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index d4647ded340f..12fe5392c832 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1250,7 +1250,54 @@ static int get_config_terms(const struct parse_events_terms *head_config,
> }
> new_term->free_str = true;
> } else {
> - new_term->val.val = val;
> + switch (new_type) {
> + case EVSEL__CONFIG_TERM_PERIOD:
> + new_term->val.period = val;
> + break;
Thanks Thomas and sorry big endian got broken! I'm a little confused
here as period is a u64 so I think this one can be a default case.
> + case EVSEL__CONFIG_TERM_FREQ:
> + new_term->val.freq = val;
> + break;
Also a u64.
> + case EVSEL__CONFIG_TERM_TIME:
> + new_term->val.time = val;
> + break;
> + case EVSEL__CONFIG_TERM_STACK_USER:
> + new_term->val.stack_user = val;
> + break;
Also a u64.
> + case EVSEL__CONFIG_TERM_INHERIT:
> + new_term->val.inherit = val;
> + break;
> + case EVSEL__CONFIG_TERM_OVERWRITE:
> + new_term->val.overwrite = val;
> + break;
> + case EVSEL__CONFIG_TERM_MAX_STACK:
> + new_term->val.max_stack = val;
> + break;
> + case EVSEL__CONFIG_TERM_MAX_EVENTS:
> + new_term->val.max_events = val;
> + break;
> + case EVSEL__CONFIG_TERM_PERCORE:
> + new_term->val.percore = val;
> + break;
> + case EVSEL__CONFIG_TERM_AUX_OUTPUT:
> + new_term->val.aux_output = val;
> + break;
> + case EVSEL__CONFIG_TERM_AUX_SAMPLE_SIZE:
> + new_term->val.aux_sample_size = val;
> + break;
> + case EVSEL__CONFIG_TERM_CALLGRAPH:
> + case EVSEL__CONFIG_TERM_DRV_CFG:
> + case EVSEL__CONFIG_TERM_BRANCH:
> + case EVSEL__CONFIG_TERM_AUX_ACTION:
> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG:
> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG1:
> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG2:
> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG3:
> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG4:
> + case EVSEL__CONFIG_TERM_RATIO_TO_PREV:
I think these cases are all assigning a str so would using str rather
than val be cleaner?
The change looks good but it is a little inconsistent that the default
copying is done for str values but not for u64. It would kind of be
nice to remove the default copying so that if a new config term is
added the switch will fail to compile due to a missing case statement.
Then we can do the right copy for big endian. Given we've broken
big-endian here we should probably add a comment.
Thanks,
Ian
I> + default:
> + new_term->val.val = val;
> + break;
> + }
> }
> }
> return 0;
> --
> 2.53.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH linux-next] perf parse-events: Fix big-endian 'overwrite' by writing correct union member
2026-02-12 18:17 ` Ian Rogers
@ 2026-02-16 10:04 ` Thomas Richter
2026-02-16 11:18 ` James Clark
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Richter @ 2026-02-16 10:04 UTC (permalink / raw)
To: Ian Rogers
Cc: linux-kernel, linux-s390, linux-perf-users, acme, namhyung,
james.clark, agordeev, gor, sumanthk, hca, japo
On 2/12/26 19:17, Ian Rogers wrote:
> On Thu, Feb 12, 2026 at 4:53 AM Thomas Richter <tmricht@linux.ibm.com> wrote:
>>
>> The "Read backward ring buffer" test crashes on big-endian (e.g. s390x)
>> due to a NULL dereference when the backward mmap path isn't enabled.
>>
>> Reproducer:
>> # ./perf test -F 'Read backward ring buffer'
>> Segmentation fault (core dumped)
>> # uname -m
>> s390x
>> #
>>
>> Root cause:
>> get_config_terms() stores into evsel_config_term::val.val (u64) while later
>> code reads boolean fields such as evsel_config_term::val.overwrite.
>> On big-endian the 1-byte boolean is left-aligned, so writing
>> evsel_config_term::val.val = 1 is read back as
>> evsel_config_term::val.overwrite = 0,
>> leaving backward mmap disabled and a NULL map being used.
>>
>> Store values in the union member that matches the term type, e.g.:
>> /* for OVERWRITE */
>> new_term->val.overwrite = 1; /* not new_term->val.val = 1 */
>> to fix this.
>>
>> Impact:
>> Enables backward mmap on big-endian and prevents the crash.
>> No change on little-endian.
>>
>> Output after:
>> # ./perf test -Fv 44
>> --- start ---
>> Using CPUID IBM,9175,705,ME1,3.8,002f
>> mmap size 1052672B
>> mmap size 8192B
>> ---- end ----
>> 44: Read backward ring buffer : Ok
>> #
>>
>> Fixes: 159ca97cd97c ("perf parse-events: Refactor get_config_terms() to remove macros")
>> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
>> Reviewed-by: Jan Polensky <japo@linux.ibm.com>
>> Cc: James Clark <james.clark@linaro.org>
>> Cc: Ian Rogers <irogers@google.com>
>> ---
>> tools/perf/util/parse-events.c | 49 +++++++++++++++++++++++++++++++++-
>> 1 file changed, 48 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index d4647ded340f..12fe5392c832 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -1250,7 +1250,54 @@ static int get_config_terms(const struct parse_events_terms *head_config,
>> }
>> new_term->free_str = true;
>> } else {
>> - new_term->val.val = val;
>> + switch (new_type) {
>> + case EVSEL__CONFIG_TERM_PERIOD:
>> + new_term->val.period = val;
>> + break;
>
> Thanks Thomas and sorry big endian got broken! I'm a little confused
> here as period is a u64 so I think this one can be a default case.
>
>> + case EVSEL__CONFIG_TERM_FREQ:
>> + new_term->val.freq = val;
>> + break;
>
> Also a u64.
>
>> + case EVSEL__CONFIG_TERM_TIME:
>> + new_term->val.time = val;
>> + break;
>> + case EVSEL__CONFIG_TERM_STACK_USER:
>> + new_term->val.stack_user = val;
>> + break;
>
> Also a u64.
Agreed, I really don't mind. However should that type be changed (yep extremely unlikely)
we run into the same issue again.
>
>> + case EVSEL__CONFIG_TERM_INHERIT:
>> + new_term->val.inherit = val;
>> + break;
>> + case EVSEL__CONFIG_TERM_OVERWRITE:
>> + new_term->val.overwrite = val;
>> + break;
>> + case EVSEL__CONFIG_TERM_MAX_STACK:
>> + new_term->val.max_stack = val;
>> + break;
>> + case EVSEL__CONFIG_TERM_MAX_EVENTS:
>> + new_term->val.max_events = val;
>> + break;
>> + case EVSEL__CONFIG_TERM_PERCORE:
>> + new_term->val.percore = val;
>> + break;
>> + case EVSEL__CONFIG_TERM_AUX_OUTPUT:
>> + new_term->val.aux_output = val;
>> + break;
>> + case EVSEL__CONFIG_TERM_AUX_SAMPLE_SIZE:
>> + new_term->val.aux_sample_size = val;
>> + break;
>> + case EVSEL__CONFIG_TERM_CALLGRAPH:
>> + case EVSEL__CONFIG_TERM_DRV_CFG:
>> + case EVSEL__CONFIG_TERM_BRANCH:
>> + case EVSEL__CONFIG_TERM_AUX_ACTION:
>> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG:
>> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG1:
>> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG2:
>> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG3:
>> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG4:
>> + case EVSEL__CONFIG_TERM_RATIO_TO_PREV:
>
> I think these cases are all assigning a str so would using str rather
> than val be cleaner?
>
> The change looks good but it is a little inconsistent that the default
> copying is done for str values but not for u64. It would kind of be
> nice to remove the default copying so that if a new config term is
> added the switch will fail to compile due to a missing case statement.
> Then we can do the right copy for big endian. Given we've broken
> big-endian here we should probably add a comment.
>
> Thanks,
> Ian
Yep I will send v2 later today and hopefully address your remarks completely.
>
> I> + default:
>> + new_term->val.val = val;
>> + break;
>> + }
>> }
>> }
>> return 0;
>> --
>> 2.53.0
>>
Thanks
--
Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH linux-next] perf parse-events: Fix big-endian 'overwrite' by writing correct union member
2026-02-16 10:04 ` Thomas Richter
@ 2026-02-16 11:18 ` James Clark
2026-02-16 11:34 ` James Clark
0 siblings, 1 reply; 5+ messages in thread
From: James Clark @ 2026-02-16 11:18 UTC (permalink / raw)
To: Thomas Richter, Ian Rogers
Cc: linux-kernel, linux-s390, linux-perf-users, acme, namhyung,
agordeev, gor, sumanthk, hca, japo
On 16/02/2026 10:04 am, Thomas Richter wrote:
> On 2/12/26 19:17, Ian Rogers wrote:
>> On Thu, Feb 12, 2026 at 4:53 AM Thomas Richter <tmricht@linux.ibm.com> wrote:
>>>
>>> The "Read backward ring buffer" test crashes on big-endian (e.g. s390x)
>>> due to a NULL dereference when the backward mmap path isn't enabled.
>>>
>>> Reproducer:
>>> # ./perf test -F 'Read backward ring buffer'
>>> Segmentation fault (core dumped)
>>> # uname -m
>>> s390x
>>> #
>>>
>>> Root cause:
>>> get_config_terms() stores into evsel_config_term::val.val (u64) while later
>>> code reads boolean fields such as evsel_config_term::val.overwrite.
>>> On big-endian the 1-byte boolean is left-aligned, so writing
>>> evsel_config_term::val.val = 1 is read back as
>>> evsel_config_term::val.overwrite = 0,
>>> leaving backward mmap disabled and a NULL map being used.
>>>
>>> Store values in the union member that matches the term type, e.g.:
>>> /* for OVERWRITE */
>>> new_term->val.overwrite = 1; /* not new_term->val.val = 1 */
>>> to fix this.
>>>
>>> Impact:
>>> Enables backward mmap on big-endian and prevents the crash.
>>> No change on little-endian.
>>>
>>> Output after:
>>> # ./perf test -Fv 44
>>> --- start ---
>>> Using CPUID IBM,9175,705,ME1,3.8,002f
>>> mmap size 1052672B
>>> mmap size 8192B
>>> ---- end ----
>>> 44: Read backward ring buffer : Ok
>>> #
>>>
>>> Fixes: 159ca97cd97c ("perf parse-events: Refactor get_config_terms() to remove macros")
>>> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
>>> Reviewed-by: Jan Polensky <japo@linux.ibm.com>
>>> Cc: James Clark <james.clark@linaro.org>
>>> Cc: Ian Rogers <irogers@google.com>
>>> ---
>>> tools/perf/util/parse-events.c | 49 +++++++++++++++++++++++++++++++++-
>>> 1 file changed, 48 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>>> index d4647ded340f..12fe5392c832 100644
>>> --- a/tools/perf/util/parse-events.c
>>> +++ b/tools/perf/util/parse-events.c
>>> @@ -1250,7 +1250,54 @@ static int get_config_terms(const struct parse_events_terms *head_config,
>>> }
>>> new_term->free_str = true;
>>> } else {
>>> - new_term->val.val = val;
>>> + switch (new_type) {
>>> + case EVSEL__CONFIG_TERM_PERIOD:
>>> + new_term->val.period = val;
>>> + break;
>>
>> Thanks Thomas and sorry big endian got broken! I'm a little confused
>> here as period is a u64 so I think this one can be a default case.
>>
>>> + case EVSEL__CONFIG_TERM_FREQ:
>>> + new_term->val.freq = val;
>>> + break;
>>
>> Also a u64.
>>
>>> + case EVSEL__CONFIG_TERM_TIME:
>>> + new_term->val.time = val;
>>> + break;
>>> + case EVSEL__CONFIG_TERM_STACK_USER:
>>> + new_term->val.stack_user = val;
>>> + break;
>>
>> Also a u64.
>
> Agreed, I really don't mind. However should that type be changed (yep extremely unlikely)
> we run into the same issue again.
>>
>>> + case EVSEL__CONFIG_TERM_INHERIT:
>>> + new_term->val.inherit = val;
>>> + break;
>>> + case EVSEL__CONFIG_TERM_OVERWRITE:
>>> + new_term->val.overwrite = val;
>>> + break;
>>> + case EVSEL__CONFIG_TERM_MAX_STACK:
>>> + new_term->val.max_stack = val;
>>> + break;
>>> + case EVSEL__CONFIG_TERM_MAX_EVENTS:
>>> + new_term->val.max_events = val;
>>> + break;
>>> + case EVSEL__CONFIG_TERM_PERCORE:
>>> + new_term->val.percore = val;
>>> + break;
>>> + case EVSEL__CONFIG_TERM_AUX_OUTPUT:
>>> + new_term->val.aux_output = val;
>>> + break;
>>> + case EVSEL__CONFIG_TERM_AUX_SAMPLE_SIZE:
>>> + new_term->val.aux_sample_size = val;
>>> + break;
>>> + case EVSEL__CONFIG_TERM_CALLGRAPH:
>>> + case EVSEL__CONFIG_TERM_DRV_CFG:
>>> + case EVSEL__CONFIG_TERM_BRANCH:
>>> + case EVSEL__CONFIG_TERM_AUX_ACTION:
>>> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG:
>>> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG1:
>>> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG2:
>>> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG3:
>>> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG4:
>>> + case EVSEL__CONFIG_TERM_RATIO_TO_PREV:
>>
>> I think these cases are all assigning a str so would using str rather
>> than val be cleaner?
>>
>> The change looks good but it is a little inconsistent that the default
>> copying is done for str values but not for u64. It would kind of be
>> nice to remove the default copying so that if a new config term is
>> added the switch will fail to compile due to a missing case statement.
>> Then we can do the right copy for big endian. Given we've broken
>> big-endian here we should probably add a comment.
>>
>> Thanks,
>> Ian
>
> Yep I will send v2 later today and hopefully address your remarks completely.
>
Oops sorry about the breakage. You could remove the val member entirely
now and set them all individually. I only added it to avoid a second
switch statement after creating the new entry. But if we have to add the
switch back in again and list out each item we might as well go back to
how it was before and assign each one individually.
Or a nuclear option could be to just have a single u64 numeric member
and a string one and not try to rename them and change the types so
much. Ironically half of the usages of the bools look like this:
attr->aux_output = term->val.aux_output ? 1 : 0;
Almost makes all the shuffling around and renaming seem a bit pointless
in the end.
>>
>> I> + default:
>>> + new_term->val.val = val;
>>> + break;
>>> + }
>>> }
>>> }
>>> return 0;
>>> --
>>> 2.53.0
>>>
>
> Thanks
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH linux-next] perf parse-events: Fix big-endian 'overwrite' by writing correct union member
2026-02-16 11:18 ` James Clark
@ 2026-02-16 11:34 ` James Clark
0 siblings, 0 replies; 5+ messages in thread
From: James Clark @ 2026-02-16 11:34 UTC (permalink / raw)
To: Thomas Richter, Ian Rogers
Cc: linux-kernel, linux-s390, linux-perf-users, acme, namhyung,
agordeev, gor, sumanthk, hca, japo
On 16/02/2026 11:18 am, James Clark wrote:
>
>
> On 16/02/2026 10:04 am, Thomas Richter wrote:
>> On 2/12/26 19:17, Ian Rogers wrote:
>>> On Thu, Feb 12, 2026 at 4:53 AM Thomas Richter
>>> <tmricht@linux.ibm.com> wrote:
>>>>
>>>> The "Read backward ring buffer" test crashes on big-endian (e.g. s390x)
>>>> due to a NULL dereference when the backward mmap path isn't enabled.
>>>>
>>>> Reproducer:
>>>> # ./perf test -F 'Read backward ring buffer'
>>>> Segmentation fault (core dumped)
>>>> # uname -m
>>>> s390x
>>>> #
>>>>
>>>> Root cause:
>>>> get_config_terms() stores into evsel_config_term::val.val (u64)
>>>> while later
>>>> code reads boolean fields such as evsel_config_term::val.overwrite.
>>>> On big-endian the 1-byte boolean is left-aligned, so writing
>>>> evsel_config_term::val.val = 1 is read back as
>>>> evsel_config_term::val.overwrite = 0,
>>>> leaving backward mmap disabled and a NULL map being used.
>>>>
>>>> Store values in the union member that matches the term type, e.g.:
>>>> /* for OVERWRITE */
>>>> new_term->val.overwrite = 1; /* not new_term->val.val = 1 */
>>>> to fix this.
>>>>
>>>> Impact:
>>>> Enables backward mmap on big-endian and prevents the crash.
>>>> No change on little-endian.
>>>>
>>>> Output after:
>>>> # ./perf test -Fv 44
>>>> --- start ---
>>>> Using CPUID IBM,9175,705,ME1,3.8,002f
>>>> mmap size 1052672B
>>>> mmap size 8192B
>>>> ---- end ----
>>>> 44: Read backward ring buffer : Ok
>>>> #
>>>>
>>>> Fixes: 159ca97cd97c ("perf parse-events: Refactor get_config_terms()
>>>> to remove macros")
>>>> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
>>>> Reviewed-by: Jan Polensky <japo@linux.ibm.com>
>>>> Cc: James Clark <james.clark@linaro.org>
>>>> Cc: Ian Rogers <irogers@google.com>
>>>> ---
>>>> tools/perf/util/parse-events.c | 49 ++++++++++++++++++++++++++++++
>>>> +++-
>>>> 1 file changed, 48 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-
>>>> events.c
>>>> index d4647ded340f..12fe5392c832 100644
>>>> --- a/tools/perf/util/parse-events.c
>>>> +++ b/tools/perf/util/parse-events.c
>>>> @@ -1250,7 +1250,54 @@ static int get_config_terms(const struct
>>>> parse_events_terms *head_config,
>>>> }
>>>> new_term->free_str = true;
>>>> } else {
>>>> - new_term->val.val = val;
>>>> + switch (new_type) {
>>>> + case EVSEL__CONFIG_TERM_PERIOD:
>>>> + new_term->val.period = val;
>>>> + break;
>>>
>>> Thanks Thomas and sorry big endian got broken! I'm a little confused
>>> here as period is a u64 so I think this one can be a default case.
>>>
>>>> + case EVSEL__CONFIG_TERM_FREQ:
>>>> + new_term->val.freq = val;
>>>> + break;
>>>
>>> Also a u64.
>>>
>>>> + case EVSEL__CONFIG_TERM_TIME:
>>>> + new_term->val.time = val;
>>>> + break;
>>>> + case EVSEL__CONFIG_TERM_STACK_USER:
>>>> + new_term->val.stack_user = val;
>>>> + break;
>>>
>>> Also a u64.
>>
>> Agreed, I really don't mind. However should that type be changed (yep
>> extremely unlikely)
>> we run into the same issue again.
>>>
>>>> + case EVSEL__CONFIG_TERM_INHERIT:
>>>> + new_term->val.inherit = val;
>>>> + break;
>>>> + case EVSEL__CONFIG_TERM_OVERWRITE:
>>>> + new_term->val.overwrite = val;
>>>> + break;
>>>> + case EVSEL__CONFIG_TERM_MAX_STACK:
>>>> + new_term->val.max_stack = val;
>>>> + break;
>>>> + case EVSEL__CONFIG_TERM_MAX_EVENTS:
>>>> + new_term->val.max_events = val;
>>>> + break;
>>>> + case EVSEL__CONFIG_TERM_PERCORE:
>>>> + new_term->val.percore = val;
>>>> + break;
>>>> + case EVSEL__CONFIG_TERM_AUX_OUTPUT:
>>>> + new_term->val.aux_output = val;
>>>> + break;
>>>> + case EVSEL__CONFIG_TERM_AUX_SAMPLE_SIZE:
>>>> + new_term->val.aux_sample_size = val;
>>>> + break;
>>>> + case EVSEL__CONFIG_TERM_CALLGRAPH:
>>>> + case EVSEL__CONFIG_TERM_DRV_CFG:
>>>> + case EVSEL__CONFIG_TERM_BRANCH:
>>>> + case EVSEL__CONFIG_TERM_AUX_ACTION:
>>>> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG:
>>>> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG1:
>>>> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG2:
>>>> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG3:
>>>> + case EVSEL__CONFIG_TERM_USR_CHG_CONFIG4:
>>>> + case EVSEL__CONFIG_TERM_RATIO_TO_PREV:
>>>
>>> I think these cases are all assigning a str so would using str rather
>>> than val be cleaner?
>>>
>>> The change looks good but it is a little inconsistent that the default
>>> copying is done for str values but not for u64. It would kind of be
>>> nice to remove the default copying so that if a new config term is
>>> added the switch will fail to compile due to a missing case statement.
>>> Then we can do the right copy for big endian. Given we've broken
>>> big-endian here we should probably add a comment.
>>>
>>> Thanks,
>>> Ian
>>
>> Yep I will send v2 later today and hopefully address your remarks
>> completely.
>>
>
> Oops sorry about the breakage. You could remove the val member entirely
> now and set them all individually. I only added it to avoid a second
> switch statement after creating the new entry. But if we have to add the
> switch back in again and list out each item we might as well go back to
> how it was before and assign each one individually.
>
> Or a nuclear option could be to just have a single u64 numeric member
> and a string one and not try to rename them and change the types so
> much. Ironically half of the usages of the bools look like this:
>
> attr->aux_output = term->val.aux_output ? 1 : 0;
>
> Almost makes all the shuffling around and renaming seem a bit pointless
> in the end.
>
To further this point: many different string term types share the same
str member, but the numeric ones assign through different names even if
the underlying type is the same. So it's not even that consistent.
>
>>>
>>> I> + default:
>>>> + new_term->val.val = val;
>>>> + break;
>>>> + }
>>>> }
>>>> }
>>>> return 0;
>>>> --
>>>> 2.53.0
>>>>
>>
>> Thanks
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-02-16 11:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-02-12 12:53 [PATCH linux-next] perf parse-events: Fix big-endian 'overwrite' by writing correct union member Thomas Richter
2026-02-12 18:17 ` Ian Rogers
2026-02-16 10:04 ` Thomas Richter
2026-02-16 11:18 ` James Clark
2026-02-16 11:34 ` James Clark
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox