Re: [PATCH] coredump: allow interrupting dumps of large anonymous regions

[Jan Kara <jack@xxxxxxx>]

On Wed 15-01-25 23:05:38, Tavian Barnes wrote:
> dump_user_range() supports sparse core dumps by skipping anonymous pages
> which have not been modified.  If get_dump_page() returns NULL, the page
> is skipped rather than written to the core dump with dump_emit_page().
> 
> Sadly, dump_emit_page() contains the only check for dump_interrupted(),
> so when dumping a very large sparse region, the core dump becomes
> effectively uninterruptible.  This can be observed with the following
> test program:
> 
>     #include <stdlib.h>
>     #include <stdio.h>
>     #include <sys/mman.h>
> 
>     int main(void) {
>         char *mem = mmap(NULL, 1ULL << 40, PROT_READ | PROT_WRITE,
>                 MAP_ANONYMOUS | MAP_NORESERVE | MAP_PRIVATE, -1, 0);
>         printf("%p %m\n", mem);
>         if (mem != MAP_FAILED) {
>                 mem[0] = 1;
>         }
>         abort();
>     }
> 
> The program allocates 1 TiB of anonymous memory, touches one page of it,
> and aborts.  During the core dump, SIGKILL has no effect.  It takes
> about 30 seconds to finish the dump, burning 100% CPU.
> 
> This issue naturally arises with things like Address Sanitizer, which
> allocate a large sparse region of virtual address space for their shadow
> memory.
> 
> Fix it by checking dump_interrupted() explicitly in dump_user_pages().
> 
> Signed-off-by: Tavian Barnes <tavianator@xxxxxxxxxxxxxx>

Thanks for the patch! The idea looks good to me as a quick fix, one
suggestion for improvement below:

> diff --git a/fs/coredump.c b/fs/coredump.c
> index d48edb37bc35..fd29d3f15f1e 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -950,6 +950,10 @@ int dump_user_range(struct coredump_params *cprm, unsigned long start,
>  			}
>  		} else {
>  			dump_skip(cprm, PAGE_SIZE);
> +			if (dump_interrupted()) {
> +				dump_page_free(dump_page);
> +				return 0;
> +			}

So rather than doing the check here, I'd do it before cond_resched() below
and remove the check from dump_emit_page(). That way we have the
interruption handling all in one place.

>  		}
>  		cond_resched();
>  	}

Bonus points for unifying the exit paths from the loop (perhaps as a
separate cleanup patch) like:

		if (page)
			ret = dump_emit_page(...)
		else
			dump_skip(...)
		if (dump_interrupted())
			ret = 0;
		if (!ret)
			break;
		cond_resched();
	}
	dump_page_free(dump_page);
	return ret;

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR