linux-kernel - Re: [PATCH v3] 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]

Date:   Mon, 8 Mar 2021 20:57:49 +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 v3] perf pmu: Validate raw event with sysfs exported
 format bits

Hi Jiri,

On 3/8/2021 6:35 PM, Jiri Olsa wrote:
> On Mon, Mar 08, 2021 at 11:15:06AM +0800, Jin Yao wrote:
> 
> SNIP
> 
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index 44ef28302fc7..03ab1e6d0418 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -1812,3 +1812,39 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
>>   
>>   	return nr_caps;
>>   }
>> +
>> +void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
>> +				   char *name)
>> +{
>> +	struct perf_pmu_format *format;
>> +	__u64 masks = 0, bits;
>> +	char buf[100];
>> +	unsigned int i;
>> +
>> +	list_for_each_entry(format, &pmu->format, list)	{
>> +		/*
>> +		 * Skip extra configs such as config1/config2.
>> +		 */
>> +		if (format->value > 0)
>> +			continue;
> 
> sorry I did not notice before, but could you please use more direct
> approach like:
> 
> 		if (format->value == PERF_PMU_FORMAT_VALUE_CONFIG) {
> 			break;
> 		}
> 
> this will be more obvious, also no need for the comment.. I spent some
> time looking what's the value for ;-)
> 
> thanks,
> jirka
> 

Oh, yes, using PERF_PMU_FORMAT_VALUE_CONFIG is much more obvious. Sorry about that!

While it can't break the loop, because we need to iterate over the whole list to get the total valid 
bits. So like:

if (format->value != PERF_PMU_FORMAT_VALUE_CONFIG)
	continue;

Is it right?

I will post v4 tomorrow.

Thanks
Jin Yao

>> +
>> +		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;
>> +
>> +	bits = config & ~masks;
>> +	if (bits == 0)
>> +		return;
>> +
>> +	bitmap_scnprintf((unsigned long *)&bits, sizeof(bits) * 8, buf, sizeof(buf));
>> +
>> +	pr_warning("WARNING: event '%s' not valid (bits %s of config "
>> +		   "'%llx' not supported by kernel)!\n",
>> +		   name ?: "N/A", buf, config);
>> +}
>> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
>> index 8164388478c6..160b0f561771 100644
>> --- a/tools/perf/util/pmu.h
>> +++ b/tools/perf/util/pmu.h
>> @@ -123,4 +123,7 @@ int perf_pmu__convert_scale(const char *scale, char **end, double *sval);
>>   
>>   int perf_pmu__caps_parse(struct perf_pmu *pmu);
>>   
>> +void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
>> +				   char *name);
>> +
>>   #endif /* __PMU_H */
>> -- 
>> 2.17.1
>>
>