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

[Thomas Richter]

On 2/12/26 19:17, Ian Rogers wrote:
> On Thu, Feb 12, 2026 at 4:53 AM Thomas Richter <tmricht@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> Reviewed-by: Jan Polensky <japo@xxxxxxxxxxxxx>
>> Cc: James Clark <james.clark@xxxxxxxxxx>
>> Cc: Ian Rogers <irogers@xxxxxxxxxx>
>> ---
>>  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