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

[Namhyung Kim <namhyung@kernel.org>]

Hello,

On Fri, Feb 20, 2026 at 10:31:17AM +0100, Thomas Richter wrote:
> On 2/17/26 15:09, James Clark wrote:
> > 
> > 
> > On 17/02/2026 1:14 pm, Thomas Richter wrote:
> >> v1 -> v2: Add comments from James Clark and Ian Rogers
> >> v2 -> v3: Improve switch statement in add_config_term()
> >> v3 -> v4: Fix clang warnings
> >>
> >> 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. Improve add_config_term() and add two more parameters for
> >> string and value. Function add_config_term() now creates a complete node
> >> element of type evsel_config_term and handles all evsel_config_term::val
> >> union members.
> >>
> >> 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 | 82 +++++++++++++++++++++++++++-------
> >>   1 file changed, 65 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> >> index b9efb296bba5..7b4629625b1e 100644
> >> --- a/tools/perf/util/parse-events.c
> >> +++ b/tools/perf/util/parse-events.c
> >> @@ -1117,7 +1117,7 @@ static int config_attr(struct perf_event_attr *attr,
> >>     static struct evsel_config_term *add_config_term(enum evsel_term_type type,
> >>                            struct list_head *head_terms,
> >> -                         bool weak)
> >> +                         bool weak, char *str, u64 val)
> >>   {
> >>       struct evsel_config_term *t;
> >>   @@ -1128,8 +1128,62 @@ static struct evsel_config_term *add_config_term(enum evsel_term_type type,
> >>       INIT_LIST_HEAD(&t->list);
> >>       t->type = type;
> >>       t->weak    = weak;
> >> +
> >> +    switch (type) {
> >> +    case EVSEL__CONFIG_TERM_PERIOD:
> >> +    case EVSEL__CONFIG_TERM_FREQ:
> >> +    case EVSEL__CONFIG_TERM_STACK_USER:
> >> +    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:
> >> +        t->val.val = val;
> >> +        break;
> >> +    case EVSEL__CONFIG_TERM_TIME:
> >> +        t->val.time = val;
> >> +        break;
> >> +    case EVSEL__CONFIG_TERM_INHERIT:
> >> +        t->val.inherit = val;
> >> +        break;
> >> +    case EVSEL__CONFIG_TERM_OVERWRITE:
> >> +        t->val.overwrite = val;
> >> +        break;
> >> +    case EVSEL__CONFIG_TERM_MAX_STACK:
> >> +        t->val.max_stack = val;
> >> +        break;
> >> +    case EVSEL__CONFIG_TERM_MAX_EVENTS:
> >> +        t->val.max_events = val;
> >> +        break;
> >> +    case EVSEL__CONFIG_TERM_PERCORE:
> >> +        t->val.percore = val;
> >> +        break;
> >> +    case EVSEL__CONFIG_TERM_AUX_OUTPUT:
> >> +        t->val.aux_output = val;
> >> +        break;
> >> +    case EVSEL__CONFIG_TERM_AUX_SAMPLE_SIZE:
> >> +        t->val.aux_sample_size = val;
> >> +        break;
> >> +    case EVSEL__CONFIG_TERM_CALLGRAPH:
> >> +    case EVSEL__CONFIG_TERM_BRANCH:
> >> +    case EVSEL__CONFIG_TERM_DRV_CFG:
> >> +    case EVSEL__CONFIG_TERM_RATIO_TO_PREV:
> >> +    case EVSEL__CONFIG_TERM_AUX_ACTION:
> >> +        if (str) {
> >> +            t->val.str = strdup(str);
> >> +            if (!t->val.str) {
> >> +                zfree(&t);
> >> +                return NULL;
> >> +            }
> >> +            t->free_str = true;
> >> +        }
> >> +        break;
> >> +    default:
> >> +        t->val.val = val;
> >> +        break;
> >> +    }
> >> +
> >>       list_add_tail(&t->list, head_terms);
> >> -
> >>       return t;
> >>   }
> >>   @@ -1142,7 +1196,7 @@ static int get_config_terms(const struct parse_events_terms *head_config,
> >>           struct evsel_config_term *new_term;
> >>           enum evsel_term_type new_type;
> >>           bool str_type = false;
> >> -        u64 val;
> >> +        u64 val = 0;
> >>             switch (term->type_term) {
> >>           case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
> >> @@ -1234,20 +1288,15 @@ static int get_config_terms(const struct parse_events_terms *head_config,
> >>               continue;
> >>           }
> >>   -        new_term = add_config_term(new_type, head_terms, term->weak);
> >> +        /*
> >> +         * Note: Members evsel_config_term::val and
> >> +         * parse_events_term::val are unions and endianness needs
> >> +         * to be taken into account when changing such union members.
> >> +         */
> >> +        new_term = add_config_term(new_type, head_terms, term->weak,
> >> +                       str_type ? term->val.str : NULL, val);
> >>           if (!new_term)
> >>               return -ENOMEM;
> >> -
> >> -        if (str_type) {
> >> -            new_term->val.str = strdup(term->val.str);
> >> -            if (!new_term->val.str) {
> >> -                zfree(&new_term);
> >> -                return -ENOMEM;
> >> -            }
> >> -            new_term->free_str = true;
> >> -        } else {
> >> -            new_term->val.val = val;
> >> -        }
> >>       }
> >>       return 0;
> >>   }
> >> @@ -1277,10 +1326,9 @@ static int add_cfg_chg(const struct perf_pmu *pmu,
> >>       if (bits) {
> >>           struct evsel_config_term *new_term;
> >>   -        new_term = add_config_term(new_term_type, head_terms, false);
> >> +        new_term = add_config_term(new_term_type, head_terms, false, NULL, bits);
> >>           if (!new_term)
> >>               return -ENOMEM;
> >> -        new_term->val.cfg_chg = bits;
> >>       }
> >>         return 0;
> > 
> > Reviewed-by: James Clark <james.clark@linaro.org>
> > 
> 
> Friendly ping, this still fails in linux-next...

Sorry for the delay, I'll queue it up after the merge window.

Thanks,
Namhyung