Re: [PATCH linux-next] perf parse-events: Fix big-endian 'overwrite' by writing correct union member

[James Clark <james.clark@linaro.org>]

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
>