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: <dac76dd5-d5f0-61dd-fafe-f939f1ebc413@arm.com>
Date:   Thu, 27 Apr 2023 16:52:06 +0100
From:   James Clark <james.clark@....com>
To:     Leo Yan <leo.yan@...aro.org>
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 27/04/2023 16:12, Leo Yan wrote:
> Hi James,
> 
> On Mon, Apr 24, 2023 at 02:47:44PM +0100, James Clark wrote:
>> Currently the cs_etm_set_option() function both validates and applies
>> the config options. Because it's only called when they are added
>> automatically, there are some paths where the user can apply the option
>> on the command line and skip the validation. By moving it to the end it
>> covers both cases.
>>
>> Also, options don't need to be re-applied anyway, Perf handles parsing
>> and applying the config terms automatically.
>>
>> Signed-off-by: James Clark <james.clark@....com>
>> ---
>>  tools/perf/arch/arm/util/cs-etm.c | 152 +++++++++++++-----------------
>>  1 file changed, 68 insertions(+), 84 deletions(-)
>>
>> diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
>> index f9b9ebf7fffc..af0a2400c655 100644
>> --- a/tools/perf/arch/arm/util/cs-etm.c
>> +++ b/tools/perf/arch/arm/util/cs-etm.c
>> @@ -69,21 +69,29 @@ static const char * const metadata_ete_ro[] = {
>>  static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu);
>>  static bool cs_etm_is_ete(struct auxtrace_record *itr, int cpu);
>>  
>> -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. 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.

> Thanks,
> Leo
>