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

[Ingo Molnar <mingo@kernel.org>]

* David Ahern <dsahern@gmail.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@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Stephane Eranian <eranian@google.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