Re: [makedumpfile PATCH] Wipe excluded pages that are written into ELF dump file

[Eric DeVolder <eric.devolder@oracle.com>]

On 08/02/2017 08:00 PM, Atsushi Kumagai wrote:
> Hello,
> 
>> On Mon, Jul 31, 2017 at 06:35:30AM -0700, Eric DeVolder wrote:
>>> When a page is excluded by any of the existing dump levels,
>>> that page may still be written to the ELF dump file, depending
>>> upon the PFN_EXCLUDED mechanism.
>>>
>>> The PFN_EXCLUDED mechanism looks for N consecutive "not
>>> dumpable" pages, and if found, the current ELF segment is
>>> closed out and a new ELF segment started, at the next dumpable
>>> page. Otherwise, if the PFN_EXCLUDED criteria is not meet (that
>>> is, there is a mix of dumpable and not dumpable pages, but not
>>> N consecutive not dumpable pages) all pages are written to the
>>> dump file.
>>>
>>> This patch implements a mechanism for those "not dumpable" pages
>>> that are written to the ELF dump file to fill those pages with
>>> constant data, rather than the original data. In other words,
>>> the dump file still contains the page, but its data is wiped.
>>>
>>> The motivation for doing this is to protect real user (customer)
>>> data from "leaking" through to a dump file when that data was
>>> intended to be omitted.
>>>
>>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>>> ---
>>>   makedumpfile.8 |  8 ++++++++
>>>   makedumpfile.c | 32 +++++++++++++++++++++++++-------
>>>   makedumpfile.h |  2 ++
>>>   3 files changed, 35 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/makedumpfile.8 b/makedumpfile.8
>>> index f94f2d7..64af0cf 100644
>>> --- a/makedumpfile.8
>>> +++ b/makedumpfile.8
>>> @@ -621,6 +621,14 @@ order from left to right.  \fIVMCORE\fRs are assembled into a single
>>>   # makedumpfile \-x vmlinux \-\-diskset=vmcore1 \-\-diskset=vmcore2 dumpfile
>>>
>>>   .TP
>>> +\fB\-\-fill-excluded-pages FILL_VALUE\fR
>>
>> I am OK with --fill-excluded-pages to change default value but it is not needed
>> in my opinion. However, I think that we should have --no-fill-excluded-pages
>> variant. Just in case if somebody wish to disable default behavior
> 
> Umm, I can't think of any cases where a user expects "not dumpable pages" to
> remain while he specifies a dump level to exclude those pages.
> That's why I suggested that this feature should be default.

I have changed the patch to do this behavior; I have eliminated the 
option altogether. If I have misunderstood, please indicate as such.

> 
> BTW, could you tell me the benefits of making FILL_VALUE changeable ?

The motivation for this was debugging; perhaps not very useful given 
this is post-mortem debugging...

I've posted v2 of the patch.

Thanks,
eric


> 
>>> +For the ELF dump file format, excluded pages may be written into the dump
>>> +file, but the page contents are wiped. This option allows the wipe value
>>> +to be changed from the default 0x5041474557495045UL "PAGEWIPE", or on
>>> +32-bit systems "WIPE".
>>
>> 0xDEAD9A6E == DEADPAGE where P == 9 (do you have better figure for P?) and G == 6
>>
>> This seems better for me because you do not need to convert it to ASCII
>> to see what it is. And fills exactly 32-bits.
> 
> I'll leave it up to you :-)
> 
> Thanks,
> Atsushi Kumagai
> 
>>> +
>>> +
>>> +.TP
>>>   \fB\-D\fR
>>>   Print debugging message.
>>>
>>> diff --git a/makedumpfile.c b/makedumpfile.c
>>> index f85003a..cee0ab0 100644
>>> --- a/makedumpfile.c
>>> +++ b/makedumpfile.c
>>> @@ -7139,7 +7139,7 @@ out:
>>>
>>>   int
>>>   write_elf_load_segment(struct cache_data *cd_page, unsigned long long paddr,
>>> -		       off_t off_memory, long long size)
>>> +		       off_t off_memory, long long size, struct cycle *cycle)
>>>   {
>>>   	long page_size = info->page_size;
>>>   	long long bufsz_write;
>>> @@ -7163,10 +7163,23 @@ write_elf_load_segment(struct cache_data *cd_page, unsigned long long paddr,
>>>   		else
>>>   			bufsz_write = size;
>>>
>>> -		if (read(info->fd_memory, buf, bufsz_write) != bufsz_write) {
>>> -			ERRMSG("Can't read the dump memory(%s). %s\n",
>>> -			    info->name_memory, strerror(errno));
>>> -			return FALSE;
>>> +		if (!is_dumpable(info->bitmap2, paddr_to_pfn(paddr), cycle)) {
>>> +			unsigned k;
>>> +			unsigned long *p = (unsigned long *)buf;
>>> +			for (k = 0; k < info->page_size; k += sizeof(unsigned long)) {
>>> +				*p++ = info->fill_excluded_pages_value;
>>> +			}
>>> +			if (lseek(info->fd_memory, bufsz_write, SEEK_CUR) < 0) {
>>> +				ERRMSG("Can't seek the dump memory(%s). %s\n",
>>> +				    info->name_memory, strerror(errno));
>>> +				return FALSE;
>>> +			}
>>> +		} else {
>>> +			if (read(info->fd_memory, buf, bufsz_write) != bufsz_write) {
>>> +				ERRMSG("Can't read the dump memory(%s). %s\n",
>>> +				    info->name_memory, strerror(errno));
>>> +				return FALSE;
>>> +			}
>>>   		}
>>>   		filter_data_buffer((unsigned char *)buf, paddr, bufsz_write);
>>>   		paddr += bufsz_write;
>>> @@ -7431,7 +7444,7 @@ write_elf_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_page)
>>>   				 */
>>>   				if (load.p_filesz)
>>>   					if (!write_elf_load_segment(cd_page, paddr,
>>> -								    off_memory, load.p_filesz))
>>> +								    off_memory, load.p_filesz, &cycle))
>>>   						return FALSE;
>>>
>>>   				load.p_paddr += load.p_memsz;
>>> @@ -7473,7 +7486,7 @@ write_elf_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_page)
>>>   		 */
>>>   		if (load.p_filesz)
>>>   			if (!write_elf_load_segment(cd_page, paddr,
>>> -						    off_memory, load.p_filesz))
>>> +						    off_memory, load.p_filesz, &cycle))
>>>   				return FALSE;
>>>
>>>   		off_seg_load += load.p_filesz;
>>> @@ -11057,6 +11070,7 @@ static struct option longopts[] = {
>>>   	{"splitblock-size", required_argument, NULL, OPT_SPLITBLOCK_SIZE},
>>>   	{"work-dir", required_argument, NULL, OPT_WORKING_DIR},
>>>   	{"num-threads", required_argument, NULL, OPT_NUM_THREADS},
>>> +	{"fill-excluded-pages", required_argument, NULL, OPT_FILL_EXCLUDED_PAGES},
>>>   	{0, 0, 0, 0}
>>>   };
>>>
>>> @@ -11083,6 +11097,7 @@ main(int argc, char *argv[])
>>>   	info->fd_dumpfile = -1;
>>>   	info->fd_bitmap = -1;
>>>   	info->kaslr_offset = 0;
>>> +	info->fill_excluded_pages_value = 0x5041474557495045UL;
>>>   	initialize_tables();
>>>
>>>   	/*
>>> @@ -11218,6 +11233,9 @@ main(int argc, char *argv[])
>>>   		case OPT_NUM_THREADS:
>>>   			info->num_threads = MAX(atoi(optarg), 0);
>>>   			break;
>>> +		case OPT_FILL_EXCLUDED_PAGES:
>>> +			info->fill_excluded_pages_value = strtoul(optarg, NULL, 0);
>>> +			break;
>>>   		case '?':
>>>   			MSG("Commandline parameter is invalid.\n");
>>>   			MSG("Try `makedumpfile --help' for more information.\n");
>>> diff --git a/makedumpfile.h b/makedumpfile.h
>>> index 8a05794..9ccd06d 100644
>>> --- a/makedumpfile.h
>>> +++ b/makedumpfile.h
>>> @@ -1266,6 +1266,7 @@ struct DumpInfo {
>>>   	int		vmemmap_cnt;
>>>   	struct ppc64_vmemmap	*vmemmap_list;
>>>   	unsigned long	kaslr_offset;
>>> +	unsigned long	fill_excluded_pages_value; /* fill value for excluded pages */
>>>
>>>   	/*
>>>   	 * page table info for ppc64
>>> @@ -2275,6 +2276,7 @@ struct elf_prstatus {
>>>   #define OPT_WORKING_DIR         OPT_START+15
>>>   #define OPT_NUM_THREADS	OPT_START+16
>>>   #define OPT_PARTIAL_DMESG	OPT_START+17
>>> +#define OPT_FILL_EXCLUDED_PAGES	OPT_START+18
>>
>> Oh, no please fix alignment somehow here. Separate patch?
>> And I think that just in case it should be:
>>
>> #define OPT_FILL_EXCLUDED_PAGES      (OPT_START+18)
>>
>> And probably this applies to others. Next patch?
>>
>> Daniel
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec