linux-kernel - Re: [PATCH v2 1/2] perf evlist: Ensure grouped events with same cpu map

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:   Wed, 27 May 2020 21:49:11 +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 v2 1/2] perf evlist: Ensure grouped events with same cpu
 map

Hi Jiri,

On 5/27/2020 6:28 PM, Jiri Olsa wrote:
> On Wed, May 27, 2020 at 02:31:03PM +0800, Jin, Yao wrote:
> 
> SNIP
> 
>>> Thanks
>>> Jin Yao
>>
>> Issue is found!
>>
>> It looks we can't set "pos->leader = pos" in either for_each_group_member()
>> or in for_each_group_evsel() because it may exit the iteration immediately.
>>
>> 	evlist__for_each_entry(evlist, evsel) {
>> 		if (evsel->leader == evsel)
>> 			continue;
>>
>> 		if (cpu_maps_matched(evsel->leader, evsel))
>> 			continue;
>>
>> 		pr_warning("WARNING: event cpu maps are not fully matched, "
>> 			   "disable group\n");
>>
>> 		for_each_group_member(pos, evsel->leader) {
>> 			pos->leader = pos;
>> 			pos->core.nr_members = 0;
>> 		}
>>
>> Let me use the example of '{cycles,unc_cbo_cache_lookup.any_i}' again.
>>
>> In evlist:
>> cycles,
>> unc_cbo_cache_lookup.any_i,
>> unc_cbo_cache_lookup.any_i,
>> unc_cbo_cache_lookup.any_i,
>> unc_cbo_cache_lookup.any_i,
>>
>> When we reach the for_each_group_member at first time, evsel is the first
>> unc_cbo_cache_lookup.any_i and evsel->leader is cycles. pos is same as the
>> evsel (the first unc_cbo_cache_lookup.any_i).
>>
>> Once we execute "pos->leader = pos;", it's actually "evsel->leader = evsel".
>> So now evsel->leader is changed to the first unc_cbo_cache_lookup.any_i.
>>
>> In next iteration, pos is the second unc_cbo_cache_lookup.any_i. pos->leader
>> is cycles but unfortunately evsel->leader has been changed to the first
>> unc_cbo_cache_lookup.any_i. So iteration stops immediately.
> 
> hum, AFAICS the iteration will not break but continue to next evsel and
> pass the 'continue' for another group member.. what do I miss?
> 
> jirka
> 

Let me use this example again.

cycles,
unc_cbo_cache_lookup.any_i,
unc_cbo_cache_lookup.any_i,
unc_cbo_cache_lookup.any_i,
unc_cbo_cache_lookup.any_i,

Yes, once for_each_group_member breaks (due to the issue in 'pos->leader = pos'), 
evlist__for_each_entry will continue to the second unc_cbo_cache_lookup.any_i. But now evsel->leader 
!= evsel (evsel->leader is "cycles"), so it will go to cpu_maps_matched.

But actually we don't need to go to cpu_maps_matched again.

for_each_group_member(pos, evsel->leader) {
	pos->leader = pos;
	pos->core.nr_members = 0;
}

If we solve the issue in above code, for_each_group_member doesn't break, the leaders of all members 
in this group will be set to themselves.

if (evsel->leader == evsel)
	continue;

So the iteration will continue to the next evsel.

Thanks
Jin Yao