linux-kernel - Re: [PATCH] perf tools: Fix use of wrong event when processing exit events

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: <20150902195501.GB11521@kernel.org>
Date:	Wed, 2 Sep 2015 16:55:01 -0300
From:	Arnaldo Carvalho de Melo <acme@...nel.org>
To:	Adrian Hunter <adrian.hunter@...el.com>
Cc:	linux-kernel@...r.kernel.org, Jiri Olsa <jolsa@...hat.com>
Subject: Re: [PATCH] perf tools: Fix use of wrong event when processing exit
 events

Em Wed, Sep 02, 2015 at 04:00:08PM +0300, Adrian Hunter escreveu:
> On 18/08/15 12:07, Adrian Hunter wrote:
> > In a couple of cases the 'comm' member of 'union event' has
> > been used instead of the correct member ('fork') when processing
> > exit events.
> > 
> > In the cases where it has been used incorrectly, only the 'pid'
> > and 'tid' are affected.  The 'pid' value would be correct anyway
> > because it is in the same position in 'comm' and 'fork' events,
> > but the 'tid' would have been incorrectly assigned from 'ppid'.
> > However, for exit events, the kernel puts the current task in
> > the 'ppid' and 'ttid' which is the same as the exiting task.
> > That is 'ppid' == 'pid' and if the task is not multi-threaded,
> > 'pid' == 'tid' i.e. the data goes wrong only when tracing
> > multi-threaded programs.
> > 
> > It is hard to find an example of how this would produce an
> > error in practice.  There are 3 occurences of the fix:
> > 1. perf script is only affected if !sample_id_all which only
> > happens on old kernels
> > 2. intel_pt is only affected when decoding without timestamps
> > and would probably still decode correctly - the exit event is
> > only used to flush out data which anyway gets flushed at the
> > end of the session
> > 3. intel_bts also uses the exit event to flush data which
> > would probably not cause errors as it would get flushed at
> > the end of the session instead
> > 
> > Signed-off-by: Adrian Hunter <adrian.hunter@...el.com>
> 
> I think it would be worth picking this one up for 4.3

I guess I had this picked up, is there anything else from you that is
outstanding?

- Arnaldo
 
> > ---
> >  tools/perf/builtin-script.c | 4 ++--
> >  tools/perf/util/intel-bts.c | 2 +-
> >  tools/perf/util/intel-pt.c  | 2 +-
> >  3 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> > index 105332e950a9..17030c6c64b6 100644
> > --- a/tools/perf/builtin-script.c
> > +++ b/tools/perf/builtin-script.c
> > @@ -739,8 +739,8 @@ static int process_exit_event(struct perf_tool *tool,
> >  	if (!evsel->attr.sample_id_all) {
> >  		sample->cpu = 0;
> >  		sample->time = 0;
> > -		sample->tid = event->comm.tid;
> > -		sample->pid = event->comm.pid;
> > +		sample->tid = event->fork.tid;
> > +		sample->pid = event->fork.pid;
> >  	}
> >  	print_sample_start(sample, thread, evsel);
> >  	perf_event__fprintf(event, stdout);
> > diff --git a/tools/perf/util/intel-bts.c b/tools/perf/util/intel-bts.c
> > index dce99cfb1309..09d142b362fa 100644
> > --- a/tools/perf/util/intel-bts.c
> > +++ b/tools/perf/util/intel-bts.c
> > @@ -623,7 +623,7 @@ static int intel_bts_process_event(struct perf_session *session,
> >  	if (err)
> >  		return err;
> >  	if (event->header.type == PERF_RECORD_EXIT) {
> > -		err = intel_bts_process_tid_exit(bts, event->comm.tid);
> > +		err = intel_bts_process_tid_exit(bts, event->fork.tid);
> >  		if (err)
> >  			return err;
> >  	}
> > diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> > index 2a4a4120473b..ef2fb13f1dfa 100644
> > --- a/tools/perf/util/intel-pt.c
> > +++ b/tools/perf/util/intel-pt.c
> > @@ -1464,7 +1464,7 @@ static int intel_pt_process_event(struct perf_session *session,
> >  	if (pt->timeless_decoding) {
> >  		if (event->header.type == PERF_RECORD_EXIT) {
> >  			err = intel_pt_process_timeless_queues(pt,
> > -							       event->comm.tid,
> > +							       event->fork.tid,
> >  							       sample->time);
> >  		}
> >  	} else if (timestamp) {
> > 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/