linux-kernel - Re: [PATCH 5/5] perf record: Handle out of space failures writing data with mmap

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, 12 Nov 2013 22:19:26 +0100
From:	Ingo Molnar <mingo@...nel.org>
To:	David Ahern <dsahern@...il.com>
Cc:	acme@...stprotocols.net, linux-kernel@...r.kernel.org,
	jolsa@...hat.com, Frederic Weisbecker <fweisbec@...il.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Namhyung Kim <namhyung@...nel.org>,
	Mike Galbraith <efault@....de>,
	Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH 5/5] perf record: Handle out of space failures writing
 data with mmap


* David Ahern <dsahern@...il.com> wrote:

> If the filesystem where a file is written using mmap fills perf record
> gets a SIGBUS and terminated. Handle the SIGBUS by using longjmp to
> bounce out of the memcpy and fail the write.
> 
> Signed-off-by: David Ahern <dsahern@...il.com>
> Cc: Ingo Molnar <mingo@...nel.org>
> Cc: Frederic Weisbecker <fweisbec@...il.com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Jiri Olsa <jolsa@...hat.com>
> Cc: Namhyung Kim <namhyung@...nel.org>
> Cc: Mike Galbraith <efault@....de>
> Cc: Stephane Eranian <eranian@...gle.com>
> ---
>  tools/perf/builtin-record.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 1a4fa5df215b..48d6535d144f 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -29,9 +29,11 @@
>  #include <unistd.h>
>  #include <sched.h>
>  #include <sys/mman.h>
> +#include <setjmp.h>
>  
>  /* output file mmap'ed N chunks at a time */
>  #define MMAP_OUTPUT_SIZE   (64*1024*1024)
> +sigjmp_buf mmap_jmp;
>  
>  #ifndef HAVE_ON_EXIT_SUPPORT
>  #ifndef ATEXIT_MAX
> @@ -141,6 +143,7 @@ static int do_mmap_output(struct perf_record *rec, void *buf, size_t size)
>  {
>  	u64 remaining;
>  	off_t offset;
> +	volatile size_t total_len = 0;
>  
>  	if (rec->mmap.addr == NULL) {
>  next_segment:
> @@ -157,20 +160,23 @@ next_segment:
>  	 * space write what we can then go back and create the
>  	 * next segment
>  	 */
> -	if (size > remaining) {
> -		memcpy(rec->mmap.addr + rec->mmap.offset, buf, remaining);
> +	if (setjmp(mmap_jmp) != 0) {
> +		pr_err("mmap copy failed.\n");
> +		return -1;
> +	}
> +	if (size-total_len > remaining) {
> +		memcpy(rec->mmap.addr + rec->mmap.offset, buf+total_len, remaining);
>  		rec->bytes_written += remaining;
>  
> -		size -= remaining;
> -		buf  += remaining;
> +		total_len += remaining;
>  
>  		munmap(rec->mmap.addr, rec->mmap.out_size);
>  		goto next_segment;
>  	}
>  
>  	/* more data to copy and it fits in the current segment */
> -	if (size) {
> -		memcpy(rec->mmap.addr + rec->mmap.offset, buf, size);
> +	if (size - total_len) {
> +		memcpy(rec->mmap.addr + rec->mmap.offset, buf+total_len, size-total_len);
>  		rec->bytes_written += size;
>  		rec->mmap.offset += size;
>  	}
> @@ -272,6 +278,9 @@ static void sig_handler(int sig)
>  	if (sig == SIGCHLD)
>  		child_finished = 1;
>  
> +	if (sig == SIGBUS)
> +		longjmp(mmap_jmp, 1);

So this isn't very robust, because it assumes that all sources of SIGBUS 
are due to that memcpy() hitting -ENOSPC...

There are several failure modes:

 - If mmap_jmp is not set yet and we get a SIGBUS is some other place, 
   then the longjmp() result will be undefined.

 - If mmap_jmp environment is set, but we've returned from 
   do_mmap_output() already, then the result will be undefined - likely a 
   non-obvious crash.

So at minimum we need a flag that tells us whether the jump environment is 
valid or not - i.e. whether we are executing inside the protected region 
or not - and only do the longjmp() if that flag is set.

Is there really no other way to handle the -ENOSPC case robustly? I guess 
not because the memcpy() really needs memory to write to, but I thought 
I'd ask ...

Thanks,

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