linux-kernel - Re: [PATCH] perf pmu: Validate raw event with sysfs exported format bits

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: <3c88d257-f923-bc82-f860-ea1f6588d9ed@linux.intel.com>
Date:   Fri, 5 Mar 2021 12:28:38 +0800
From:   "Jin, Yao" <yao.jin@...ux.intel.com>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     acme@...nel.org, jolsa@...nel.org, peterz@...radead.org,
        mingo@...hat.com, alexander.shishkin@...ux.intel.com,
        Linux-kernel@...r.kernel.org, ak@...ux.intel.com,
        kan.liang@...el.com, yao.jin@...el.com
Subject: Re: [PATCH] perf pmu: Validate raw event with sysfs exported format
 bits

Hi Jiri,

On 3/5/2021 4:17 AM, Jiri Olsa wrote:
> On Wed, Mar 03, 2021 at 01:17:36PM +0800, Jin Yao wrote:
> 
> SNIP
> 
>> The set bits in 'bits' indicate the invalid bits used in config.
>> Finally use strbuf to report the invalid bits.
>>
>> Some architectures may not export supported bits through sysfs,
>> so if masks is 0, perf_pmu__config_valid just returns true.
>>
>> After:
>>
>> Single event:
>>
>>    # ./perf stat -e cpu/r031234/ -a -- sleep 1
>>    WARNING: event config '31234' not valid (bits 16 17 not supported by kernel)!
>>
>>     Performance counter stats for 'system wide':
>>
>>                     0      cpu/r031234/
>>
>>           1.001403757 seconds time elapsed
>>
>> Multiple events:
>>
>>    # ./perf stat -e cpu/rf01234/,cpu/r031234/ -a -- sleep 1
>>    WARNING: event config 'f01234' not valid (bits 20 22 not supported by kernel)!
>>    WARNING: event config '31234' not valid (bits 16 17 not supported by kernel)!
> 
> right, seems useful
> 

Thanks :)

>>
>>     Performance counter stats for 'system wide':
>>
>>                     0      cpu/rf01234/
>>                     0      cpu/r031234/
>>
>> The warning is reported for invalid bits.
>>
>> Signed-off-by: Jin Yao <yao.jin@...ux.intel.com>
>> ---
>>   tools/perf/util/parse-events.c | 11 ++++++++++
>>   tools/perf/util/pmu.c          | 38 ++++++++++++++++++++++++++++++++++
>>   tools/perf/util/pmu.h          |  4 ++++
>>   3 files changed, 53 insertions(+)
>>
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index 42c84adeb2fb..1820b2c0a241 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -356,6 +356,17 @@ __add_event(struct list_head *list, int *idx,
>>   	struct perf_cpu_map *cpus = pmu ? perf_cpu_map__get(pmu->cpus) :
>>   			       cpu_list ? perf_cpu_map__new(cpu_list) : NULL;
>>   
> 
> if we want it just for raw/numeric, we could add it earlier on,
> like to parse_events_add_numeric call
> 
> but perhaps it might be helpful to check any pmu event,
> could perhaps reveal some broken format
> 

Yes, I think so. So directly checking the attr->config here may cover more cases.

>> +	if (pmu && attr->type == PERF_TYPE_RAW) {
>> +		struct strbuf buf = STRBUF_INIT;
>> +
>> +		if (!perf_pmu__config_valid(pmu, attr->config, &buf)) {
>> +			pr_warning("WARNING: event config '%llx' not valid ("
>> +				   "bits%s not supported by kernel)!\n",
>> +				   attr->config, buf.buf);
>> +		}
>> +		strbuf_release(&buf);
>> +	}
>> +
>>   	if (init_attr)
>>   		event_attr_init(attr);
>>   
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index 44ef28302fc7..5e361adb2698 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -1812,3 +1812,41 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
>>   
>>   	return nr_caps;
>>   }
>> +
>> +bool perf_pmu__config_valid(struct perf_pmu *pmu, __u64 config,
>> +			    struct strbuf *buf)
>> +{
>> +	struct perf_pmu_format *format;
>> +	__u64 masks = 0, bits;
>> +	int i;
>> +
>> +	list_for_each_entry(format, &pmu->format, list)	{
>> +		/*
>> +		 * Skip extra configs such as config1/config2.
>> +		 */
>> +		if (format->value > 0)
>> +			continue;
>> +
>> +		for_each_set_bit(i, format->bits, PERF_PMU_FORMAT_BITS)
>> +			masks |= 1ULL << i;
>> +	}
>> +
>> +	/*
>> +	 * Kernel doesn't export any valid format bits.
>> +	 */
>> +	if (masks == 0)
>> +		return true;
>> +
>> +	bits = config & ~masks;
>> +	if (bits == 0)
>> +		return true;
> 
> so in here you now that there's something wrong, so why
> bother with the outside strbuf, when we can easily just
> do all the printing in here?
> 

For this patch, yes, I don't need to return the strbuf to caller then print outside.

Andi now comments to print the original event as well, so I need to choose #1 pass the event name to 
perf_pmu__config_valid or #2 return the strbuf to caller.

>> +
>> +	for (i = 0; i < PERF_PMU_FORMAT_BITS; i++) {
>> +		if (bits & 0x1)
>> +			strbuf_addf(buf, " %d", i);
>> +
>> +		bits >>= 1;
> 
> could you use the for_each_set_bit in here?
> 

Yes, maybe I can use the code such as :

for_each_set_bit(i, (unsigned long *) &bits, sizeof(bits) * 8)
	strbuf_addf(buf, " %d", i);

Thanks
Jin Yao

> thanks,
> jirka
>