linux-kernel - RE: [PATCH] perf report: Fix invalid memory accessing

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:	Tue, 8 Sep 2015 13:42:58 +0000
From:	"Liang, Kan" <kan.liang@...el.com>
To:	"Wangnan (F)" <wangnan0@...wei.com>, Jiri Olsa <jolsa@...hat.com>
CC:	"acme@...nel.org" <acme@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"lizefan@...wei.com" <lizefan@...wei.com>,
	"pi3orama@....com" <pi3orama@....com>,
	"Hunter, Adrian" <adrian.hunter@...el.com>,
	Andi Kleen <ak@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
	Namhyung Kim <namhyung@...nel.org>,
	Stephane Eranian <eranian@...gle.com>,
	Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: RE: [PATCH] perf report: Fix invalid memory accessing



> 
> On 2015/9/8 15:37, Jiri Olsa wrote:
> > On Mon, Sep 07, 2015 at 09:27:26PM +0800, Wangnan (F) wrote:
> >
> > SNIP
> >
> >> I found the problem.
> >>
> >> perf relies on build_cpu_topology() to fetch CPU_TOPOLOGY from sysfs.
> >> It depend on the existance of
> >>
> >> /sys/devices/system/cpu/cpu%d/topology/core_siblings_list
> >>
> >> However, CPU can be canceled by hotcpu subsystem. After that the
> >> directory of /sys/devices/system/cpu/cpu%d/topology is gone, which
> >> causes perf's
> >> write_cpu_topology() --> uild_cpu_topology() to fail, result in the
> >> above perf.data.
> >>
> >> So I think my patch is required.
> > no question there.. I just meant it should be placed in
> > perf_event__preprocess_sample function with the rest of the 'al'
> > initialization, like in the patch below?
> >
> > it does not compile, because there're many places calling it and it'd
> > need changing all callers to pass env, which seems to require more
> > changes..
> >
> > also I'm not sure about removing:
> > -	al->socket = cpu_map__get_socket_id(al->cpu);
> >
> >
> > Does any command actually need this initialized from current system?
> >
> > thanks,
> > jirka
> >
> >
> > ---
> > diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c index
> > 0bf8c9889fc0..3339d2579bfc 100644
> > --- a/tools/perf/util/event.c
> > +++ b/tools/perf/util/event.c
> > @@ -990,7 +990,8 @@ void thread__find_addr_location(struct thread
> *thread,
> >   int perf_event__preprocess_sample(const union perf_event *event,
> >   				  struct machine *machine,
> >   				  struct addr_location *al,
> > -				  struct perf_sample *sample)
> > +				  struct perf_sample *sample,
> > +				  struct perf_env *env)
> >   {
> >   	u8 cpumode = event->header.misc &
> PERF_RECORD_MISC_CPUMODE_MASK;
> >   	struct thread *thread = machine__findnew_thread(machine,
> > sample->pid, @@ -1021,7 +1022,10 @@ int
> > perf_event__preprocess_sample(const union perf_event *event,
> >
> >   	al->sym = NULL;
> >   	al->cpu = sample->cpu;
> > -	al->socket = cpu_map__get_socket_id(al->cpu);
> > +
> > +	al.socket = -1;
> > +	if (env->cpu && al->cpu >= 0)
> > +		al.socket = env->cpu[al->cpu].socket_id;
> >
> >   	if (al->map) {
> >   		struct dso *dso = al->map->dso;
> 
> Now I understand your suggestion. You mean we can build env->cpu
> before processing the first sample, then init al.socket using that map
> instead of calling cpu_map__get_socket_id() unconditionally in an ad-hoc
> way.
> 
> And I have another question that, since build_cpu_topo() and
> perf_event__preprocess_sample() are more or less doing similar things,
> why we need both of them?
> 
> Then we need more code for this bug...
> 
> Kan Liang, do you have any suggestion?
> 
> 

I think Jirka's way is better. We should handle al.socket in one place for all tools.

Now we already read env from file in perf_session__new for perf report.
I think we only need to update env in perf_session__new for other tools.
So perf_event__preprocess_sample can use it.

Thanks,
Kan