lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAP-5=fXorDgm-oJS9kC6cxCEvS9-Gz5Uh_5V4MtzCYV3pXAxCA@mail.gmail.com>
Date: Thu, 1 May 2025 13:00:52 -0700
From: Ian Rogers <irogers@...gle.com>
To: Thomas Falcon <thomas.falcon@...el.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>, 
	linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH] perf top: populate PMU capabilities data in perf_env

On Thu, May 1, 2025 at 11:42 AM Thomas Falcon <thomas.falcon@...el.com> wrote:
>
> Calling perf top with brach filters enabled on Intel hybrid CPU's
> with branch counter event logging support results in a segfault.
>
> $ ./perf top -e '{cpu_core/cpu-cycles/,cpu_core/event=0xc6,umask=0x3,\
>         frontend=0x11,name=frontend_retired_dsb_miss/}' -j any,counter
> perf: Segmentation fault
> -------- backtrace --------
> ./perf() [0x55f460]
> /lib64/libc.so.6(+0x1a050) [0x7fd8be227050]
> ./perf() [0x57b4a7]
> ./perf() [0x561e5a]
> ./perf() [0x604a81]
> ./perf() [0x4395b5]
> ./perf() [0x601732]
> ./perf() [0x439bc1]
> ./perf() [0x5d35b3]
> ./perf() [0x43936c]
> /lib64/libc.so.6(+0x70ba8) [0x7fd8be27dba8]
> /lib64/libc.so.6(+0xf4b8c) [0x7fd8be301b8c]

Thanks Thomas. Could you generate this backtrace in GDB? I did write a
patch to symbolize backtraces like this:
https://lore.kernel.org/lkml/20250313052952.871958-2-irogers@google.com/
Sadly without any reviewed tags and unmerged - the code calls routines
that malloc so it isn't strictly sound if say the backtrace was needed
from a SEGV in the malloc implementation, it is nicely
self-referencing the perf APIs, ..

> The cause is that perf_env__find_br_cntr_info tries to access a
> null pointer pmu_caps in the perf_env struct. Presumably this would
> also be an issue when using the cpu_pmu_caps structure available for
> homogeneous core CPU's.

I'm a little confused in the top code, we have the global perf_env
being used and one in the session:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/builtin-top.c?h=perf-tools-next#n649
```
ret = evlist__tui_browse_hists(top->evlist, help, &hbt, top->min_percent,
      &top->session->header.env, !top->record_opts.overwrite);
```
and the global perf_env:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/builtin-top.c?h=perf-tools-next#n1641
```
status = perf_env__read_cpuid(&perf_env);
```

I kind of wish we didn't have the global one as what's the deal with
ownership with it.

> Fix this by populating cpu_pmu_caps and pmu_caps structures with
> values from sysfs when calling perf top with branch stack sampling
> enabled.

I wonder if we could encounter similar problems from say a perf script
handling live data and so some kind of lazy initialization should be
employed. It is hard to say without seeing the backtrace.

Thanks,
Ian

> Signed-off-by: Thomas Falcon <thomas.falcon@...el.com>
> ---
>  tools/perf/builtin-top.c |   8 +++
>  tools/perf/util/env.c    | 114 +++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/env.h    |   1 +
>  3 files changed, 123 insertions(+)
>
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 1061f4eebc3f..c2688e4ef3c4 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1729,6 +1729,14 @@ int cmd_top(int argc, const char **argv)
>         if (opts->branch_stack && callchain_param.enabled)
>                 symbol_conf.show_branchflag_count = true;
>
> +       if (opts->branch_stack) {
> +               status = perf_env__read_core_pmu_caps(&perf_env);
> +               if (status) {
> +                       pr_err("PMU capability data is not available\n");
> +                       goto out_delete_evlist;
> +               }
> +       }
> +
>         sort__mode = SORT_MODE__TOP;
>         /* display thread wants entries to be collapsed in a different tree */
>         perf_hpp_list.need_collapse = 1;
> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
> index 36411749e007..37ed6dc52cf3 100644
> --- a/tools/perf/util/env.c
> +++ b/tools/perf/util/env.c
> @@ -416,6 +416,120 @@ static int perf_env__read_nr_cpus_avail(struct perf_env *env)
>         return env->nr_cpus_avail ? 0 : -ENOENT;
>  }
>
> +static int __perf_env__read_core_pmu_caps(struct perf_pmu *pmu,
> +                                         int *nr_caps, char ***caps,
> +                                         unsigned int *max_branches,
> +                                         unsigned int *br_cntr_nr,
> +                                         unsigned int *br_cntr_width)
> +{
> +       struct perf_pmu_caps *pcaps = NULL;
> +       char *ptr, **tmp;
> +       int ret = 0;
> +
> +       *nr_caps = 0;
> +       *caps = NULL;
> +
> +       if (!pmu->nr_caps)
> +               return 0;
> +
> +       *caps = zalloc(sizeof(char *) * pmu->nr_caps);
> +       if (!*caps)
> +               return -ENOMEM;
> +
> +       tmp = *caps;
> +       list_for_each_entry(pcaps, &pmu->caps, list) {
> +
> +               if (asprintf(&ptr, "%s=%s", pcaps->name, pcaps->value) < 0) {
> +                       ret = -ENOMEM;
> +                       goto error;
> +               }
> +
> +               *tmp++ = ptr;
> +
> +               if (!strcmp(pcaps->name, "branches"))
> +                       *max_branches = atoi(pcaps->value);
> +
> +               if (!strcmp(pcaps->name, "branch_counter_nr"))
> +                       *br_cntr_nr = atoi(pcaps->value);
> +
> +               if (!strcmp(pcaps->name, "branch_counter_width"))
> +                       *br_cntr_width = atoi(pcaps->value);
> +       }
> +       *nr_caps = pmu->nr_caps;
> +       return 0;
> +error:
> +       while (tmp-- != *caps)
> +               free(*tmp);
> +       free(*caps);
> +       *caps = NULL;
> +       *nr_caps = 0;
> +       return ret;
> +}
> +
> +int perf_env__read_core_pmu_caps(struct perf_env *env)
> +{
> +       struct perf_pmu *pmu = NULL;
> +       struct pmu_caps *pmu_caps;
> +       int nr_pmu = 0, i = 0, j;
> +       int ret;
> +
> +       nr_pmu = perf_pmus__num_core_pmus();
> +
> +       if (!nr_pmu)
> +               return -ENODEV;
> +
> +       if (nr_pmu == 1) {
> +               pmu = perf_pmus__scan_core(NULL);
> +               if (!pmu)
> +                       return -ENODEV;
> +               ret = perf_pmu__caps_parse(pmu);
> +               if (ret < 0)
> +                       return ret;
> +               return __perf_env__read_core_pmu_caps(pmu, &env->nr_cpu_pmu_caps,
> +                                                     &env->cpu_pmu_caps,
> +                                                     &env->max_branches,
> +                                                     &env->br_cntr_nr,
> +                                                     &env->br_cntr_width);
> +       }
> +
> +       pmu_caps = zalloc(sizeof(*pmu_caps) * nr_pmu);
> +       if (!pmu_caps)
> +               return -ENOMEM;
> +
> +       while ((pmu = perf_pmus__scan_core(pmu)) != NULL) {
> +               if (perf_pmu__caps_parse(pmu) <= 0)
> +                       continue;
> +               ret = __perf_env__read_core_pmu_caps(pmu, &pmu_caps[i].nr_caps,
> +                                                    &pmu_caps[i].caps,
> +                                                    &pmu_caps[i].max_branches,
> +                                                    &pmu_caps[i].br_cntr_nr,
> +                                                    &pmu_caps[i].br_cntr_width);
> +               if (ret)
> +                       goto error;
> +
> +               pmu_caps[i].pmu_name = strdup(pmu->name);
> +               if (!pmu_caps[i].pmu_name) {
> +                       ret = -ENOMEM;
> +                       goto error;
> +               }
> +               i++;
> +       }
> +
> +       env->nr_pmus_with_caps = nr_pmu;
> +       env->pmu_caps = pmu_caps;
> +
> +       return 0;
> +error:
> +       for (i = 0; i < nr_pmu; i++) {
> +               for (j = 0; j < pmu_caps[i].nr_caps; j++)
> +                       free(pmu_caps[i].caps[j]);
> +               free(pmu_caps[i].caps);
> +               free(pmu_caps[i].pmu_name);
> +       }
> +       free(pmu_caps);
> +       return ret;
> +}
> +
>  const char *perf_env__raw_arch(struct perf_env *env)
>  {
>         return env && !perf_env__read_arch(env) ? env->arch : "unknown";
> diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
> index d90e343cf1fa..135a1f714905 100644
> --- a/tools/perf/util/env.h
> +++ b/tools/perf/util/env.h
> @@ -152,6 +152,7 @@ struct btf_node;
>
>  extern struct perf_env perf_env;
>
> +int perf_env__read_core_pmu_caps(struct perf_env *env);
>  void perf_env__exit(struct perf_env *env);
>
>  int perf_env__kernel_is_64_bit(struct perf_env *env);
> --
> 2.49.0
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ