linux-kernel - Re: [PATCH 4/7] perf: cs-etm: Validate options after applying themperf_pmu__format

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: <20230427221047.GA174797@leoy-huanghe>
Date:   Fri, 28 Apr 2023 06:10:47 +0800
From:   Leo Yan <leo.yan@...aro.org>
To:     James Clark <james.clark@....com>
Cc:     linux-perf-users@...r.kernel.org, coresight@...ts.linaro.org,
        shy828301@...il.com, denik@...gle.com,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Mike Leach <mike.leach@...aro.org>,
        John Garry <john.g.garry@...cle.com>,
        Will Deacon <will@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Ian Rogers <irogers@...gle.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/7] perf: cs-etm: Validate options after applying
 themperf_pmu__format_bits

On Thu, Apr 27, 2023 at 04:52:06PM +0100, James Clark wrote:

[...]

> >> -static int cs_etm_set_context_id(struct auxtrace_record *itr,
> >> -				 struct evsel *evsel, int cpu)
> >> +static int cs_etm_validate_context_id(struct auxtrace_record *itr,
> >> +				      struct evsel *evsel, int cpu)
> >>  {
> >> -	struct cs_etm_recording *ptr;
> >> -	struct perf_pmu *cs_etm_pmu;
> >> +	struct cs_etm_recording *ptr =
> >> +		container_of(itr, struct cs_etm_recording, itr);
> >> +	struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;
> >>  	char path[PATH_MAX];
> >> -	int err = -EINVAL;
> >> +	int err;
> >>  	u32 val;
> >> -	u64 contextid;
> >> +	u64 contextid =
> >> +		evsel->core.attr.config &
> >> +		(perf_pmu__format_bits(&cs_etm_pmu->format, "contextid1") |
> >> +		 perf_pmu__format_bits(&cs_etm_pmu->format, "contextid2"));
> > 
> > Seems to me, this would break backward compability.
> > 
> > The old kernel (before 5.11) doesn't provide 'contextid1' and
> > 'contextid2', so we always check the entry 'contextid' rather than
> > 'contextid1' and 'contextid2'.
> > 
> > With this change, if a kernel doesn't contain 'contextid1' and
> > 'contextid2' formats, will perf tool never trace for contexid?
> > 
> 
> No because I changed to to be purely validation, so the format flags
> would still be applied. But yes I think you are right there is a small
> issue.
> 
> Now validation of 'contextid' isn't done on pre 5.11 kernels. But that
> only checks for ETMv3 anyway.

IIUC, 'contextid' is not only used for ETMv3.  Just quotes the comments
from drivers/hwtracing/coresight/coresight-etm-perf.c:

  73 /*
  74  * contextid always traces the "PID".  The PID is in CONTEXTIDR_EL1
  75  * when the kernel is running at EL1; when the kernel is at EL2,
  76  * the PID is in CONTEXTIDR_EL2.
  77  */

ETMv4 uses 'contextid' as well, since the user space needs to know which
exception level's PID should be traced, e.g. when CPU runs in EL2
'contextid' is set as ETM_OPT_CTXTID2, the perf tool will set 'contextid2'
to tell driver to trace CONTEXTIDR_EL2.

We can only verify 'contextid', and set 'contextid1' or 'contextid2' based
on CPU running exception level, finally driver knows how to trace PID.

Thanks,
Leo

> Validation of 'contextid1' and
> 'contextid2' isn't a problem, because if the kernel doesn't support them
> they can't be applied on the command line anyway.
>
> I can fix it by checking for 'contextid' and ETMv3 first and then doing
> 'contextid1' and 'contextid2' after.