linux-kernel - Re: [PATCH] perf tools: session: avoid infinite loop

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:	Fri, 18 Sep 2015 16:18:51 +0100
From:	Mark Rutland <mark.rutland@....com>
To:	Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:	Adrian Hunter <adrian.hunter@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Ingo Molnar <mingo@...hat.com>, Jiri Olsa <jolsa@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH] perf tools: session: avoid infinite loop

On Fri, Sep 18, 2015 at 04:00:18PM +0100, Arnaldo Carvalho de Melo wrote:
> I tried what you suggested, but couldn't reproduce it, I only managed
> when I applied the patch below _and_ executed;
> 
>  # echo 1 > /proc/sys/kernel/kptr_restrict
> 
> To be on the same page as Mark. Then, yes, I get stuck in:
> 
> #0  0x00007f746a1d8aba in mmap64 () at ../sysdeps/unix/syscall-template.S:81
> #1  0x00000000004be66e in __perf_session__process_events (file_size=232, data_size=<optimized out>, data_offset=<optimized out>, session=0x1ba8590)
>     at util/session.c:1604
> #2  perf_session__process_events (session=0x1ba8590) at util/session.c:1685
> #3  0x000000000042d97b in process_buildids (rec=0x869ac0 <record>) at builtin-record.c:364
> #4  __cmd_record (rec=0x869ac0 <record>, argv=<optimized out>, argc=<optimized out>) at builtin-record.c:734
> #5  cmd_record (argc=<optimized out>, argv=<optimized out>, prefix=<optimized out>) at builtin-record.c:1199
> #6  0x0000000000479123 in run_builtin (p=p@...ry=0x874a90 <commands+144>, argc=argc@...ry=3, argv=argv@...ry=0x7ffd92eb5ee0) at perf.c:370
> #7  0x0000000000420aba in handle_internal_command (argv=0x7ffd92eb5ee0, argc=3) at perf.c:429
> #8  run_argv (argv=0x7ffd92eb5c70, argcp=0x7ffd92eb5c7c) at perf.c:473
> #9  main (argc=3, argv=0x7ffd92eb5ee0) at perf.c:588
> (gdb) 
> 
> After applying your patch it works, had to tweak the commit log to avoid
> starting lines with ---, breaks git scripts, also added a commiter log,
> check it, patch is below, after the one I used to not process any
> samples.

Sorry for the '---' problem, I'll bear that in mind in future.

Your commit log looks fine, though I'm slightly confused by the
Reported-by line -- did you mean to add that?

Mark.

> commit dd486ec4aa33cfca2fd912ef501d49909005de79
> Author: Mark Rutland <mark.rutland@....com>
> Date:   Wed Sep 16 18:18:49 2015 +0100
> 
>     perf record: Avoid infinite loop at buildid processing with no samples
>     
>     If a session contains no events, we can get stuck in an infinite loop in
>     __perf_session__process_events, with a non-zero file_size and data_offset, but
>     a zero data_size.
>     
>     In this case, we can mmap the entirety of the file (consisting of the file and
>     attribute headers), and fetch_mmaped_event will correctly refuse to read any
>     (unmapped and non-existent) event headers. This causes
>     __perf_session__process_events to unmap the file and retry with the exact same
>     parameters, getting stuck in an infinite loop.
>     
>     This has been observed to result in an exit-time hang when counting
>     rare/unschedulable events with perf record, and can be triggered artificially
>     with the script below:
>     
>       ----
>       #!/bin/sh
>       printf "REPRO: launching perf\n";
>       ./perf record -e software/config=9/ sleep 1 &
>       PERF_PID=$!;
>       sleep 0.002;
>       kill -2 $PERF_PID;
>       printf "REPRO: waiting for perf (%d) to exit...\n" "$PERF_PID";
>       wait $PERF_PID;
>       printf "REPRO: perf exited\n";
>       ----
>     
>     To avoid this, have __perf_session__process_events bail out early when
>     the file has no data (i.e. it has no events).
>     
>     Commiter note:
>     
>     I only managed to reproduce this when setting
>     /proc/sys/kernel/kptr_restrict to '1' and changing the code to
>     purposefully not process any samples and no synthesized samples, i.e.
>     kptr_restrict prevents 'record' from synthesizing the kernel mmaps for
>     vmlinux + modules and since it is a workload started from perf, we don't
>     synthesize mmap/comm records for existing threads.
>     
>     Adrian Hunter managed to reproduce it in his environment tho.
>     
>     Reported-by: Arnaldo Carvalho de Melo <acme@...nel.org>
>     Signed-off-by: Mark Rutland <mark.rutland@....com>
>     Cc: Adrian Hunter <adrian.hunter@...el.com>
>     Cc: Jiri Olsa <jolsa@...hat.com>
>     Cc: Peter Zijlstra <peterz@...radead.org>
>     Link: http://lkml.kernel.org/r/1442423929-12253-1-git-send-email-mark.rutland@arm.com
>     Signed-off-by: Arnaldo Carvalho de Melo <acme@...hat.com>
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 8a4537ee9bc3..fc3f7c922f99 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1580,7 +1580,10 @@ static int __perf_session__process_events(struct perf_session *session,
>  	file_offset = page_offset;
>  	head = data_offset - page_offset;
>  
> -	if (data_size && (data_offset + data_size < file_size))
> +	if (data_size == 0)
> +		goto out;
> +
> +	if (data_offset + data_size < file_size)
>  		file_size = data_offset + data_size;
>  
>  	ui_progress__init(&prog, file_size, "Processing events...");
> 
--
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/