linux-kernel - Re: [PATCH] perf tools: Refactor precise

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=fUKH8Qw9t173G4HP-UzujUKJrLPPmkDSjBs=2YzV=9LHA@mail.gmail.com>
Date: Thu, 23 Oct 2025 09:14:17 -0700
From: Ian Rogers <irogers@...gle.com>
To: Zide Chen <zide.chen@...el.com>
Cc: linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org, 
	Namhyung Kim <namhyung@...nel.org>, Peter Zijlstra <peterz@...radead.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Ingo Molnar <mingo@...hat.com>, Jiri Olsa <jolsa@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, Arnaldo Carvalho de Melo <acme@...nel.org>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, thomas.falcon@...el.com, 
	dapeng1.mi@...ux.intel.com, xudong.hao@...el.com
Subject: Re: [PATCH] perf tools: Refactor precise_ip fallback logic

On Wed, Oct 22, 2025 at 3:14 PM Zide Chen <zide.chen@...el.com> wrote:
>
> Commit c33aea446bf555ab ("perf tools: Fix precise_ip fallback logic")
> unconditionally called the precise_ip fallback and moved it after the
> missing-feature checks so that it could handle EINVAL as well.
>
> However, this introduced an issue: after disabling missing features,
> the event could fail to open, which makes the subsequent precise_ip
> fallback useless since it will always fail.
>
> For example, run the following command on Intel SPR:
>
> $ perf record -e '{cpu/mem-loads-aux/S,cpu/mem-loads,ldlat=3/PS}' -- ls
>
> Opening the event "cpu/mem-loads,ldlat=3/PS" returns EINVAL when
> precise_ip == 3. It then sets attr.inherit = false, which triggers a
> kernel check failure since it doesn't match the group leader's inherit
> attribute. As a result, it continues to fail even after precise_ip is
> reduced.
>
> By moving the precise_ip fallback earlier, this issue is resolved, as
> well as the kernel test robot report mentioned in commit
> c33aea446bf555ab.
>
> No negative side effects are expected, because the precise_ip level is
> restored by evsel__precise_ip_fallback() if the fallback does not help.
>
> This also aligns with commit 2b70702917337a8d ("perf tools: Remove
> evsel__handle_error_quirks()").
>
> Fixes: af954f76eea56453 ("perf tools: Check fallback error and order")
> Fixes: c33aea446bf555ab ("perf tools: Fix precise_ip fallback logic")
> Reviewed-by: Dapeng Mi <dapeng1.mi@...ux.intel.com>
> Signed-off-by: Zide Chen <zide.chen@...el.com>

Acked-by: Ian Rogers <irogers@...gle.com>

Any chance you could help with a test case that covers this? The
fallback logic is spread out and easy to introduce subtle bugs into.
Just having a test case that does ` perf record -e
'{cpu/mem-loads-aux/S,cpu/mem-loads,ldlat=3/PS}' -- ls` and checks the
output for EINVAL when the events are present would be useful, as then
we can make sure this doesn't regress on SPR and later. Something with
more generic events would of course be better :-)

Thanks,
Ian

> ---
>  tools/perf/util/evsel.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index ca74514c8707..6ce32533a213 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -2714,12 +2714,12 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
>         if (err == -EMFILE && rlimit__increase_nofile(&set_rlimit))
>                 goto retry_open;
>
> +       if (evsel__precise_ip_fallback(evsel))
> +               goto retry_open;
> +
>         if (err == -EINVAL && evsel__detect_missing_features(evsel, cpu))
>                 goto fallback_missing_features;
>
> -       if (evsel__precise_ip_fallback(evsel))
> -               goto retry_open;
> -
>  out_close:
>         if (err)
>                 threads->err_thread = thread;
> --
> 2.51.0
>