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: <55E6F2D8.30306@intel.com>
Date:	Wed, 2 Sep 2015 16:00:08 +0300
From:	Adrian Hunter <adrian.hunter@...el.com>
To:	Arnaldo Carvalho de Melo <acme@...nel.org>
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

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

> ---
>  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/