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

[Ian Rogers]

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.

> +                       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
>