[PATCH] makedumpfile: readpage_elf: handle 0-pages not stored in the ELF file

[PATCH] makedumpfile: readpage_elf: handle 0-pages not stored in the ELF file

[Ivan Delalande]

Handle pages filled with zeros that are not stored in the ELF file
directly, but have addresses between p_filesz and p_memsz of a segment.

This allows makedumpfile to dump-dmesg from a previously makedumpfile-ed
vmcore where all 0-pages were excluded (dump_level & 1) for example.

Signed-off-by: Ivan Delalande <colona@arista.com>
---
 elf_info.c     | 22 ++++++++++++++++++++++
 elf_info.h     |  1 +
 makedumpfile.c | 16 ++++++++++++++++
 3 files changed, 39 insertions(+)

diff --git a/elf_info.c b/elf_info.c
index 8bce942..340e885 100644
--- a/elf_info.c
+++ b/elf_info.c
@@ -42,6 +42,7 @@ struct pt_load_segment {
 	unsigned long long	phys_end;
 	unsigned long long	virt_start;
 	unsigned long long	virt_end;
+	size_t			mem_size;
 };
 
 static int			nr_cpus;             /* number of cpu */
@@ -166,6 +167,7 @@ dump_Elf_load(Elf64_Phdr *prog, int num_load)
 	pls->virt_start  = prog->p_vaddr;
 	pls->virt_end    = pls->virt_start + prog->p_filesz;
 	pls->file_offset = prog->p_offset;
+	pls->mem_size    = prog->p_memsz;
 
 	DEBUG_MSG("LOAD (%d)\n", num_load);
 	DEBUG_MSG("  phys_start : %llx\n", pls->phys_start);
@@ -584,6 +586,26 @@ offset_to_pt_load_end(off_t offset)
 }
 
 /*
+ * Return true if a page is in the zone between p_filesz and p_memsz of a
+ * segment, which is a page filled with zero and not stored in the ELF file.
+ */
+int
+paddr_is_null(unsigned long long paddr)
+{
+	int i;
+	struct pt_load_segment *pls;
+
+	for (i = 0; i < num_pt_loads; i++) {
+		pls = &pt_loads[i];
+		if ((paddr >= pls->phys_start)
+		    && (paddr + info->page_size
+			<= pls->phys_start + pls->mem_size))
+			return TRUE;
+	}
+	return FALSE;
+}
+
+/*
  * Judge whether the page is fractional or not.
  */
 int
diff --git a/elf_info.h b/elf_info.h
index e712253..4823311 100644
--- a/elf_info.h
+++ b/elf_info.h
@@ -36,6 +36,7 @@ unsigned long long page_head_to_phys_start(unsigned long long head_paddr);
 unsigned long long page_head_to_phys_end(unsigned long long head_paddr);
 off_t offset_to_pt_load_start(off_t offset);
 off_t offset_to_pt_load_end(off_t offset);
+int paddr_is_null(unsigned long long paddr);
 unsigned long long vaddr_to_paddr_general(unsigned long long vaddr);
 off_t vaddr_to_offset_slow(int fd, char *filename, unsigned long long vaddr);
 unsigned long long get_max_paddr(void);
diff --git a/makedumpfile.c b/makedumpfile.c
index fa0b779..120bfdc 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -654,6 +654,14 @@ readpage_elf(unsigned long long paddr, void *bufptr)
 	phys_end = paddr + info->page_size;
 
 	/*
+	 * Check the 0-filled pages that are not in the ELF file.
+	 */
+	if (!offset1 && !offset2 && paddr_is_null(paddr)) {
+		memset(bufptr, 0, info->page_size);
+		return TRUE;
+	}
+
+	/*
 	 * Check the case phys_start isn't aligned by page size like below:
 	 *
 	 *                           phys_start
@@ -728,6 +736,14 @@ readpage_elf_parallel(int fd_memory, unsigned long long paddr, void *bufptr)
 	phys_end = paddr + info->page_size;
 
 	/*
+	 * Check the 0-filled pages that are not in the ELF file.
+	 */
+	if (!offset1 && !offset2 && paddr_is_null(paddr)) {
+		memset(bufptr, 0, info->page_size);
+		return TRUE;
+	}
+
+	/*
 	 * Check the case phys_start isn't aligned by page size like below:
 	 *
 	 *                           phys_start
-- 
2.7.0

-- 
Ivan "Colona" Delalande
Arista Networks

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

RE: [PATCH] makedumpfile: readpage_elf: handle 0-pages not stored in the ELF file

[Atsushi Kumagai]

Hello Ivan,

>Handle pages filled with zeros that are not stored in the ELF file
>directly, but have addresses between p_filesz and p_memsz of a segment.

This fix looks reasonable, thanks for your work.

>This allows makedumpfile to dump-dmesg from a previously makedumpfile-ed
>vmcore where all 0-pages were excluded (dump_level & 1) for example.

I have some comments below:

>Signed-off-by: Ivan Delalande <colona@arista.com>
>---
> elf_info.c     | 22 ++++++++++++++++++++++
> elf_info.h     |  1 +
> makedumpfile.c | 16 ++++++++++++++++
> 3 files changed, 39 insertions(+)
>
>diff --git a/elf_info.c b/elf_info.c
>index 8bce942..340e885 100644
>--- a/elf_info.c
>+++ b/elf_info.c
>@@ -42,6 +42,7 @@ struct pt_load_segment {
> 	unsigned long long	phys_end;
> 	unsigned long long	virt_start;
> 	unsigned long long	virt_end;
>+	size_t			mem_size;
> };
>
> static int			nr_cpus;             /* number of cpu */
>@@ -166,6 +167,7 @@ dump_Elf_load(Elf64_Phdr *prog, int num_load)
> 	pls->virt_start  = prog->p_vaddr;
> 	pls->virt_end    = pls->virt_start + prog->p_filesz;
> 	pls->file_offset = prog->p_offset;
>+	pls->mem_size    = prog->p_memsz;
>
> 	DEBUG_MSG("LOAD (%d)\n", num_load);
> 	DEBUG_MSG("  phys_start : %llx\n", pls->phys_start);
>@@ -584,6 +586,26 @@ offset_to_pt_load_end(off_t offset)
> }
>
> /*
>+ * Return true if a page is in the zone between p_filesz and p_memsz of a
>+ * segment, which is a page filled with zero and not stored in the ELF file.
>+ */

This function checks just "if a page fits within a PT_LOAD", but this comment
and the function name don't match with it.

>+int
>+paddr_is_null(unsigned long long paddr)
>+{
>+	int i;
>+	struct pt_load_segment *pls;
>+
>+	for (i = 0; i < num_pt_loads; i++) {
>+		pls = &pt_loads[i];
>+		if ((paddr >= pls->phys_start)
>+		    && (paddr + info->page_size
>+			<= pls->phys_start + pls->mem_size))
>+			return TRUE;
>+	}
>+	return FALSE;
>+}

If you keep the function name, the conditional expression should be:

	if ((paddr >= pls->phys_end)
	    && (paddr + info->page_size
		<= pls->phys_start + pls->mem_size))
		return TRUE;

or,

>+
>+/*
>  * Judge whether the page is fractional or not.
>  */
> int
>diff --git a/elf_info.h b/elf_info.h
>index e712253..4823311 100644
>--- a/elf_info.h
>+++ b/elf_info.h
>@@ -36,6 +36,7 @@ unsigned long long page_head_to_phys_start(unsigned long long head_paddr);
> unsigned long long page_head_to_phys_end(unsigned long long head_paddr);
> off_t offset_to_pt_load_start(off_t offset);
> off_t offset_to_pt_load_end(off_t offset);
>+int paddr_is_null(unsigned long long paddr);
> unsigned long long vaddr_to_paddr_general(unsigned long long vaddr);
> off_t vaddr_to_offset_slow(int fd, char *filename, unsigned long long vaddr);
> unsigned long long get_max_paddr(void);
>diff --git a/makedumpfile.c b/makedumpfile.c
>index fa0b779..120bfdc 100644
>--- a/makedumpfile.c
>+++ b/makedumpfile.c
>@@ -654,6 +654,14 @@ readpage_elf(unsigned long long paddr, void *bufptr)
> 	phys_end = paddr + info->page_size;
>
> 	/*
>+	 * Check the 0-filled pages that are not in the ELF file.
>+	 */
>+	if (!offset1 && !offset2 && paddr_is_null(paddr)) {
>+		memset(bufptr, 0, info->page_size);
>+		return TRUE;
>+	}

Since this combination of three conditions is exactly *paddr_is_null()*,
the current paddr_is_null() should be renamed so that the name reflects
its contents.


Thanks,
Atsushi Kumagai

>+
>+	/*
> 	 * Check the case phys_start isn't aligned by page size like below:
> 	 *
> 	 *                           phys_start
>@@ -728,6 +736,14 @@ readpage_elf_parallel(int fd_memory, unsigned long long paddr, void *bufptr)
> 	phys_end = paddr + info->page_size;
>
> 	/*
>+	 * Check the 0-filled pages that are not in the ELF file.
>+	 */
>+	if (!offset1 && !offset2 && paddr_is_null(paddr)) {
>+		memset(bufptr, 0, info->page_size);
>+		return TRUE;
>+	}
>+
>+	/*
> 	 * Check the case phys_start isn't aligned by page size like below:
> 	 *
> 	 *                           phys_start
>--
>2.7.0
>
>--
>Ivan "Colona" Delalande
>Arista Networks

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

Re: [PATCH] makedumpfile: readpage_elf: handle 0-pages not stored in the ELF file

[Petr Tesarik]

Resent to the list.

The list does not like attachments, it seems.

Petr T

On Wed, 27 Jan 2016 08:58:21 +0100
Petr Tesarik <ptesarik@suse.cz> wrote:

> Hello Atsushi-san,
> 
> On Wed, 27 Jan 2016 02:21:57 +0000
> Atsushi Kumagai <ats-kumagai@wm.jp.nec.com> wrote:
> 
> > Hello Ivan,
> > 
> > >Handle pages filled with zeros that are not stored in the ELF file
> > >directly, but have addresses between p_filesz and p_memsz of a segment.
> > 
> > This fix looks reasonable, thanks for your work.
> > 
> > >This allows makedumpfile to dump-dmesg from a previously makedumpfile-ed
> > >vmcore where all 0-pages were excluded (dump_level & 1) for example.
> > 
> > I have some comments below:
> 
> Actually, I have already hit another issue with filtered ELF files.
> When refiltering an ELF file that had already been filtered, the memory
> size vs. file size can cause wrong max_mapnr calculation for a
> compressed file. In turn, if some pages were removed at the end of
> physical memory, those pages are not marked as filtered in the output
> file, but instead the total RAM appears to be smaller by that amount.
> 
> I don't think my patch addresses the same issue that Ivan found, but it
> definitely touches the same area. Please have a look.
> 
> Petr Tesarik

From: Petr Tesarik <ptesarik@suse.cz>
Date: Thu Aug 20 10:43:22 2015 +0200
Subject: Keep segment memory size when re-filtering ELF dumps

Originally, makedumpfile was designed to read from /proc/vmcore, where
each segment's p_memsz is equal to its p_filesz. However, makedumpfile
can also be used to re-filter an already filtered ELF dump file, where
memory size may be larger than file size. In that case the memory size
should be used as the size of the segment. This affects:

1. max_mapnr
   This value is computed as the highest phys_end. If the last segment
   was filtered, max_mapnr may be too small, and the crash utility will
   refuse to read that memory (even with --zero_excluded).

2. p_memsz field in ELF dumps
   The resulting ELF segment p_memsz will be capped to original file's
   p_filesz, ignoring the original p_memsz.

3. memory holes in KDUMP dumps
   Pages excluded in the original ELF dump will be appear as memory
   holes in the resulting KDUMP file's first bitmap.

4. vtop translation
   Virtual addresses that were filtered out in the original ELF file
   cannot be translated to physical addresses using the generic vtop
   translation.

My fix uses p_memsz to set physical and virtual extents of ELF segments,
because this is the actual size. However, file size is important when
accessing page data, so it must be stored separately and checked where
appropriate. Last but not least, pages that were filtered in the original
dump file must also be filtered in the destination file, i.e. pages
between p_filesz and p_memsz must have their corresponding bit cleared in
the 2nd bitmap.

Signed-off-by: Petr Tesarik <ptesarik@suse.com>

---
 elf_info.c     |   37 ++++++++++++++++++++++++++++++-------
 elf_info.h     |    4 ++++
 makedumpfile.c |   26 ++++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 7 deletions(-)

--- a/elf_info.c
+++ b/elf_info.c
@@ -38,6 +38,7 @@
 
 struct pt_load_segment {
 	off_t			file_offset;
+	off_t			file_size;
 	unsigned long long	phys_start;
 	unsigned long long	phys_end;
 	unsigned long long	virt_start;
@@ -162,10 +163,11 @@ dump_Elf_load(Elf64_Phdr *prog, int num_
 
 	pls = &pt_loads[num_load];
 	pls->phys_start  = prog->p_paddr;
-	pls->phys_end    = pls->phys_start + prog->p_filesz;
+	pls->phys_end    = pls->phys_start + prog->p_memsz;
 	pls->virt_start  = prog->p_vaddr;
-	pls->virt_end    = pls->virt_start + prog->p_filesz;
+	pls->virt_end    = pls->virt_start + prog->p_memsz;
 	pls->file_offset = prog->p_offset;
+	pls->file_size   = prog->p_filesz;
 
 	DEBUG_MSG("LOAD (%d)\n", num_load);
 	DEBUG_MSG("  phys_start : %llx\n", pls->phys_start);
@@ -462,7 +464,7 @@ paddr_to_offset(unsigned long long paddr
 	for (i = offset = 0; i < num_pt_loads; i++) {
 		pls = &pt_loads[i];
 		if ((paddr >= pls->phys_start)
-		    && (paddr < pls->phys_end)) {
+		    && (paddr < pls->phys_start + pls->file_size)) {
 			offset = (off_t)(paddr - pls->phys_start) +
 				pls->file_offset;
 			break;
@@ -480,16 +482,14 @@ paddr_to_offset2(unsigned long long padd
 {
 	int i;
 	off_t offset;
-	unsigned long long len;
 	struct pt_load_segment *pls;
 
 	for (i = offset = 0; i < num_pt_loads; i++) {
 		pls = &pt_loads[i];
-		len = pls->phys_end - pls->phys_start;
 		if ((paddr >= pls->phys_start)
-		    && (paddr < pls->phys_end)
+		    && (paddr < pls->phys_start + pls->file_size)
 		    && (hint >= pls->file_offset)
-		    && (hint < pls->file_offset + len)) {
+		    && (hint < pls->file_offset + pls->file_size)) {
 			offset = (off_t)(paddr - pls->phys_start) +
 				pls->file_offset;
 			break;
@@ -1095,6 +1095,29 @@ get_pt_load(int idx,
 
 	return TRUE;
 }
+
+int
+get_pt_extents(int idx,
+	unsigned long long *phys_start,
+	unsigned long long *phys_end,
+	off_t *file_size)
+{
+	struct pt_load_segment *pls;
+
+	if (num_pt_loads <= idx)
+		return FALSE;
+
+	pls = &pt_loads[idx];
+
+	if (phys_start)
+		*phys_start = pls->phys_start;
+	if (phys_end)
+		*phys_end   = pls->phys_end;
+	if (file_size)
+		*file_size = pls->file_size;
+
+	return TRUE;
+}
 
 unsigned int
 get_num_pt_loads(void)
--- a/elf_info.h
+++ b/elf_info.h
@@ -61,6 +61,10 @@ int get_pt_load(int idx,
 	unsigned long long *phys_end,
 	unsigned long long *virt_start,
 	unsigned long long *virt_end);
+int get_pt_extents(int idx,
+	unsigned long long *phys_start,
+	unsigned long long *phys_end,
+	off_t *file_size);
 unsigned int get_num_pt_loads(void);
 
 void set_nr_cpus(int num);
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -4395,6 +4395,26 @@ read_start_flat_header(void)
 	return TRUE;
 }
 
+static void
+exclude_nodata_pages(void)
+{
+	int i;
+	unsigned long long phys_start, phys_end;
+	off_t file_size;
+
+	i = 0;
+	while (get_pt_extents(i, &phys_start, &phys_end, &file_size)) {
+		unsigned long long pfn, pfn_end;
+
+		pfn = paddr_to_pfn(phys_start + file_size);
+		pfn_end = paddr_to_pfn(phys_end);
+		while (pfn <= pfn_end)
+			clear_bit_on_2nd_bitmap(pfn);
+			++pfn;
+		++i;
+	}
+}
+
 int
 read_flat_data_header(struct makedumpfile_data_header *fdh)
 {
@@ -6087,6 +6107,12 @@ create_2nd_bitmap(struct cycle *cycle)
 	}
 
 	/*
+	 * If re-filtering ELF dump, exclude pages that were already
+	 * excluded in the original file.
+	 */
+	exclude_nodata_pages();
+
+	/*
 	 * Exclude cache pages, cache private pages, user data pages,
 	 * and hwpoison pages.
 	 */

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

RE: [PATCH] makedumpfile: readpage_elf: handle 0-pages not stored in the ELF file

[Atsushi Kumagai]

Hello Petr,

> handle 0-pages not stored in the ELF file
>
>Resent to the list.
>
>The list does not like attachments, it seems.
>
>Petr T
>
>On Wed, 27 Jan 2016 08:58:21 +0100
>Petr Tesarik <ptesarik@suse.cz> wrote:
>
>> Hello Atsushi-san,
>>
>> On Wed, 27 Jan 2016 02:21:57 +0000
>> Atsushi Kumagai <ats-kumagai@wm.jp.nec.com> wrote:
>>
>> > Hello Ivan,
>> >
>> > >Handle pages filled with zeros that are not stored in the ELF file
>> > >directly, but have addresses between p_filesz and p_memsz of a segment.
>> >
>> > This fix looks reasonable, thanks for your work.
>> >
>> > >This allows makedumpfile to dump-dmesg from a previously makedumpfile-ed
>> > >vmcore where all 0-pages were excluded (dump_level & 1) for example.
>> >
>> > I have some comments below:
>>
>> Actually, I have already hit another issue with filtered ELF files.
>> When refiltering an ELF file that had already been filtered, the memory
>> size vs. file size can cause wrong max_mapnr calculation for a
>> compressed file. In turn, if some pages were removed at the end of
>> physical memory, those pages are not marked as filtered in the output
>> file, but instead the total RAM appears to be smaller by that amount.
>>
>> I don't think my patch addresses the same issue that Ivan found, but it
>> definitely touches the same area. Please have a look.

Good catch, thanks for your report.

>> Petr Tesarik
>
>From: Petr Tesarik <ptesarik@suse.cz>
>Date: Thu Aug 20 10:43:22 2015 +0200
>Subject: Keep segment memory size when re-filtering ELF dumps
>
>Originally, makedumpfile was designed to read from /proc/vmcore, where
>each segment's p_memsz is equal to its p_filesz. However, makedumpfile
>can also be used to re-filter an already filtered ELF dump file, where
>memory size may be larger than file size. In that case the memory size
>should be used as the size of the segment. This affects:

Does this problem occur only if makedumpfile has done filtering ?
According to the man 5 elf, even the original ELF file can have
"unstored zero pages".

   PT_LOAD     The array element specifies a loadable segment, described  by  p_filesz  and  p_memsz.
               The  bytes  from  the  file are mapped to the beginning of the memory segment.  If the
               segment's memory size p_memsz is larger than the file size p_filesz, the "extra" bytes
               are  defined  to  hold  the value 0 and to follow the segment's initialized area.

If unstored pages will be made only by makedumpfile, what I said
Below has no meaning.

>1. max_mapnr
>   This value is computed as the highest phys_end. If the last segment
>   was filtered, max_mapnr may be too small, and the crash utility will
>   refuse to read that memory (even with --zero_excluded).

Yes, should be computed based on p_memsz.

>2. p_memsz field in ELF dumps
>   The resulting ELF segment p_memsz will be capped to original file's
>   p_filesz, ignoring the original p_memsz.

Yes, should be fixed.

>3. memory holes in KDUMP dumps
>   Pages excluded in the original ELF dump will be appear as memory
>   holes in the resulting KDUMP file's first bitmap.

 a. If an unstored page is a just zero page, it is neither on a memory hole
    nor a filtered page. 
 b. If an unstored page is the result of makedumpfile filtering, it should be
    handled as a filtered page. 

However, I think it's impossible to distinguish whether former or latter
after filtering.

>4. vtop translation
>   Virtual addresses that were filtered out in the original ELF file
>   cannot be translated to physical addresses using the generic vtop
>   translation.

Exactly, should refer to p_memsz for it as you did.

>My fix uses p_memsz to set physical and virtual extents of ELF segments,
>because this is the actual size. However, file size is important when
>accessing page data, so it must be stored separately and checked where
>appropriate. Last but not least, pages that were filtered in the original
>dump file must also be filtered in the destination file, i.e. pages
>between p_filesz and p_memsz must have their corresponding bit cleared in
>the 2nd bitmap.

As I said above, I suspect not all of unstored pages are filtered pages,
I'm not sure exclude_nodata_pages() does right things.
As Ivan's patch does, I guess reading them as zero pages fits ELF's format
specification.


Thanks,
Atsushi Kumagai

>Signed-off-by: Petr Tesarik <ptesarik@suse.com>
>
>---
> elf_info.c     |   37 ++++++++++++++++++++++++++++++-------
> elf_info.h     |    4 ++++
> makedumpfile.c |   26 ++++++++++++++++++++++++++
> 3 files changed, 60 insertions(+), 7 deletions(-)
>
>--- a/elf_info.c
>+++ b/elf_info.c
>@@ -38,6 +38,7 @@
>
> struct pt_load_segment {
> 	off_t			file_offset;
>+	off_t			file_size;
> 	unsigned long long	phys_start;
> 	unsigned long long	phys_end;
> 	unsigned long long	virt_start;
>@@ -162,10 +163,11 @@ dump_Elf_load(Elf64_Phdr *prog, int num_
>
> 	pls = &pt_loads[num_load];
> 	pls->phys_start  = prog->p_paddr;
>-	pls->phys_end    = pls->phys_start + prog->p_filesz;
>+	pls->phys_end    = pls->phys_start + prog->p_memsz;
> 	pls->virt_start  = prog->p_vaddr;
>-	pls->virt_end    = pls->virt_start + prog->p_filesz;
>+	pls->virt_end    = pls->virt_start + prog->p_memsz;
> 	pls->file_offset = prog->p_offset;
>+	pls->file_size   = prog->p_filesz;
>
> 	DEBUG_MSG("LOAD (%d)\n", num_load);
> 	DEBUG_MSG("  phys_start : %llx\n", pls->phys_start);
>@@ -462,7 +464,7 @@ paddr_to_offset(unsigned long long paddr
> 	for (i = offset = 0; i < num_pt_loads; i++) {
> 		pls = &pt_loads[i];
> 		if ((paddr >= pls->phys_start)
>-		    && (paddr < pls->phys_end)) {
>+		    && (paddr < pls->phys_start + pls->file_size)) {
> 			offset = (off_t)(paddr - pls->phys_start) +
> 				pls->file_offset;
> 			break;
>@@ -480,16 +482,14 @@ paddr_to_offset2(unsigned long long padd
> {
> 	int i;
> 	off_t offset;
>-	unsigned long long len;
> 	struct pt_load_segment *pls;
>
> 	for (i = offset = 0; i < num_pt_loads; i++) {
> 		pls = &pt_loads[i];
>-		len = pls->phys_end - pls->phys_start;
> 		if ((paddr >= pls->phys_start)
>-		    && (paddr < pls->phys_end)
>+		    && (paddr < pls->phys_start + pls->file_size)
> 		    && (hint >= pls->file_offset)
>-		    && (hint < pls->file_offset + len)) {
>+		    && (hint < pls->file_offset + pls->file_size)) {
> 			offset = (off_t)(paddr - pls->phys_start) +
> 				pls->file_offset;
> 			break;
>@@ -1095,6 +1095,29 @@ get_pt_load(int idx,
>
> 	return TRUE;
> }
>+
>+int
>+get_pt_extents(int idx,
>+	unsigned long long *phys_start,
>+	unsigned long long *phys_end,
>+	off_t *file_size)
>+{
>+	struct pt_load_segment *pls;
>+
>+	if (num_pt_loads <= idx)
>+		return FALSE;
>+
>+	pls = &pt_loads[idx];
>+
>+	if (phys_start)
>+		*phys_start = pls->phys_start;
>+	if (phys_end)
>+		*phys_end   = pls->phys_end;
>+	if (file_size)
>+		*file_size = pls->file_size;
>+
>+	return TRUE;
>+}
>
> unsigned int
> get_num_pt_loads(void)
>--- a/elf_info.h
>+++ b/elf_info.h
>@@ -61,6 +61,10 @@ int get_pt_load(int idx,
> 	unsigned long long *phys_end,
> 	unsigned long long *virt_start,
> 	unsigned long long *virt_end);
>+int get_pt_extents(int idx,
>+	unsigned long long *phys_start,
>+	unsigned long long *phys_end,
>+	off_t *file_size);
> unsigned int get_num_pt_loads(void);
>
> void set_nr_cpus(int num);
>--- a/makedumpfile.c
>+++ b/makedumpfile.c
>@@ -4395,6 +4395,26 @@ read_start_flat_header(void)
> 	return TRUE;
> }
>
>+static void
>+exclude_nodata_pages(void)
>+{
>+	int i;
>+	unsigned long long phys_start, phys_end;
>+	off_t file_size;
>+
>+	i = 0;
>+	while (get_pt_extents(i, &phys_start, &phys_end, &file_size)) {
>+		unsigned long long pfn, pfn_end;
>+
>+		pfn = paddr_to_pfn(phys_start + file_size);
>+		pfn_end = paddr_to_pfn(phys_end);
>+		while (pfn <= pfn_end)
>+			clear_bit_on_2nd_bitmap(pfn);
>+			++pfn;
>+		++i;
>+	}
>+}
>+
> int
> read_flat_data_header(struct makedumpfile_data_header *fdh)
> {
>@@ -6087,6 +6107,12 @@ create_2nd_bitmap(struct cycle *cycle)
> 	}
>
> 	/*
>+	 * If re-filtering ELF dump, exclude pages that were already
>+	 * excluded in the original file.
>+	 */
>+	exclude_nodata_pages();
>+
>+	/*
> 	 * Exclude cache pages, cache private pages, user data pages,
> 	 * and hwpoison pages.
> 	 */
>
>_______________________________________________
>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

Re: [PATCH] makedumpfile: readpage_elf: handle 0-pages not stored in the ELF file

[Petr Tesarik]

Hello Atsushi,

On Mon, 1 Feb 2016 06:48:13 +0000
Atsushi Kumagai <ats-kumagai@wm.jp.nec.com> wrote:

>[...]
> >Originally, makedumpfile was designed to read from /proc/vmcore, where
> >each segment's p_memsz is equal to its p_filesz. However, makedumpfile
> >can also be used to re-filter an already filtered ELF dump file, where
> >memory size may be larger than file size. In that case the memory size
> >should be used as the size of the segment. This affects:
> 
> Does this problem occur only if makedumpfile has done filtering ?

Indeed, I have only seen it in a previously filtered dump file, but see
below.

> According to the man 5 elf, even the original ELF file can have
> "unstored zero pages".
> 
> [...]

I'm aware of that.

> If unstored pages will be made only by makedumpfile, what I said
> Below has no meaning.

So, I had a look at the kernel code for /proc/vmcore, and it turns out
that the p_memsz and p_filesz fields for PT_LOAD segments are not
changed at all. This means they are prepared by:

  a. kexec(8) or
  b. kexec_file_load(2) in the old kernel, or
  c. elfcorehdr_alloc() in the new kernel (s390).

For option a, kexec/crashdump-elf.c says:

		phdr->p_filesz  = phdr->p_memsz = elf_info->kern_size;

and:
		phdr->p_filesz  = phdr->p_memsz = mend - mstart + 1;

For option b, arch/x86/kernel/crash.c says:

		phdr->p_filesz = phdr->p_memsz = mend - mstart + 1;

and:
		phdr->p_filesz = phdr->p_memsz = _end - _text;

For option c, arch/s390/kernel/crash_dump.c says:

		phdr->p_filesz = end - start;
		phdr->p_memsz = end - start;

To sum it up, both fields (p_filesz and p_memsz) may originate outside
the dump kernel (except s390), hence they cannot be fully trusted.
Theoretically, you can write a custom tool which creates ELF segments
with p_memsz greater than p_filesz and pass it to the secondary kernel.
However, I'm not sure how it should be interpreted in a kernel dump
file: The "file" is in fact physical RAM, so such a segment would in
effect forcibly replace existing RAM content with zeros.

OTOH another tool may post-process /proc/vmcore, translating
zero-filled pages to more segments with a smaller p_filesz (just like
makedumpfile does). If makedumpfile is supposed to interpret the output
of such a (hypothetical) tool correctly, then yes, you must follow the
ELF specification and treat the pages as zero.

>[...]
> >3. memory holes in KDUMP dumps
> >   Pages excluded in the original ELF dump will be appear as memory
> >   holes in the resulting KDUMP file's first bitmap.
> 
>  a. If an unstored page is a just zero page, it is neither on a memory hole
>     nor a filtered page. 
>  b. If an unstored page is the result of makedumpfile filtering, it should be
>     handled as a filtered page. 
> 
> However, I think it's impossible to distinguish whether former or latter
> after filtering.

Correct.

A clean solution would be to store this information in the filtered ELF
file, e.g. with a ELF note or with an OS-specific program header flag.

>[...]
> As I said above, I suspect not all of unstored pages are filtered pages,
> I'm not sure exclude_nodata_pages() does right things.
> As Ivan's patch does, I guess reading them as zero pages fits ELF's format
> specification.

That's right. It follows the ELF specification.

It may replace filtered out pages with zero-filled pages when
converting an already-filtered ELF file to the compressed format; but
they may be filtered again if bit 0 is set in the dump level, so Ivan's
approach is cleaner, in fact. If you ever find a way to mark filtered
out pages in an ELF file, then this behaviour can be improved later.

Anyway, are you going to take the patch by Ivan, or my patch (after I
remove exclude_nodata_pages)?

Regards,
Petr Tesarik

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

RE: [PATCH] makedumpfile: readpage_elf: handle 0-pages not stored in the ELF file

[Atsushi Kumagai]

>> >Originally, makedumpfile was designed to read from /proc/vmcore, where
>> >each segment's p_memsz is equal to its p_filesz. However, makedumpfile
>> >can also be used to re-filter an already filtered ELF dump file, where
>> >memory size may be larger than file size. In that case the memory size
>> >should be used as the size of the segment. This affects:
>>
>> Does this problem occur only if makedumpfile has done filtering ?
>
>Indeed, I have only seen it in a previously filtered dump file, but see
>below.
>
>> According to the man 5 elf, even the original ELF file can have
>> "unstored zero pages".
>>
>> [...]
>
>I'm aware of that.
>
>> If unstored pages will be made only by makedumpfile, what I said
>> Below has no meaning.
>
>So, I had a look at the kernel code for /proc/vmcore, and it turns out
>that the p_memsz and p_filesz fields for PT_LOAD segments are not
>changed at all. This means they are prepared by:
>
>  a. kexec(8) or
>  b. kexec_file_load(2) in the old kernel, or
>  c. elfcorehdr_alloc() in the new kernel (s390).
>
>For option a, kexec/crashdump-elf.c says:
>
>		phdr->p_filesz  = phdr->p_memsz = elf_info->kern_size;
>
>and:
>		phdr->p_filesz  = phdr->p_memsz = mend - mstart + 1;
>
>For option b, arch/x86/kernel/crash.c says:
>
>		phdr->p_filesz = phdr->p_memsz = mend - mstart + 1;
>
>and:
>		phdr->p_filesz = phdr->p_memsz = _end - _text;
>
>For option c, arch/s390/kernel/crash_dump.c says:
>
>		phdr->p_filesz = end - start;
>		phdr->p_memsz = end - start;
>
>To sum it up, both fields (p_filesz and p_memsz) may originate outside
>the dump kernel (except s390), hence they cannot be fully trusted.
>Theoretically, you can write a custom tool which creates ELF segments
>with p_memsz greater than p_filesz and pass it to the secondary kernel.
>However, I'm not sure how it should be interpreted in a kernel dump
>file: The "file" is in fact physical RAM, so such a segment would in
>effect forcibly replace existing RAM content with zeros.
>
>OTOH another tool may post-process /proc/vmcore, translating
>zero-filled pages to more segments with a smaller p_filesz (just like
>makedumpfile does). If makedumpfile is supposed to interpret the output
>of such a (hypothetical) tool correctly, then yes, you must follow the
>ELF specification and treat the pages as zero.

Great, that's helpful investigation.
I'm not going to care about such irregular case, but I reconsidered the policy.

>>[...]
>> >3. memory holes in KDUMP dumps
>> >   Pages excluded in the original ELF dump will be appear as memory
>> >   holes in the resulting KDUMP file's first bitmap.
>>
>>  a. If an unstored page is a just zero page, it is neither on a memory hole
>>     nor a filtered page.
>>  b. If an unstored page is the result of makedumpfile filtering, it should be
>>     handled as a filtered page.
>>
>> However, I think it's impossible to distinguish whether former or latter
>> after filtering.
>
>Correct.
>
>A clean solution would be to store this information in the filtered ELF
>file, e.g. with a ELF note or with an OS-specific program header flag.

You are right, but I think it's too much fix.
I've noticed that we don't need to restore an excluded page in makedumpfile,
I don't think we need to distinguish the two cases.

>>[...]
>> As I said above, I suspect not all of unstored pages are filtered pages,
>> I'm not sure exclude_nodata_pages() does right things.
>> As Ivan's patch does, I guess reading them as zero pages fits ELF's format
>> specification.
>
>That's right. It follows the ELF specification.

Since crash treats excluded pages as zero-filled pages, I thought it's
better way also for makedumpfile, but

>It may replace filtered out pages with zero-filled pages when
>converting an already-filtered ELF file to the compressed format; but
>they may be filtered again if bit 0 is set in the dump level, so Ivan's
>approach is cleaner, in fact. If you ever find a way to mark filtered
>out pages in an ELF file, then this behaviour can be improved later.

in the case of makedumpfile, reproducing zero pages consumes actual disk
space, it sounds silly.
Anyhow we can't restore the excluded pages, there is no point compensating
them with zero pages in writing process. Now I think the unstored pages
should be written as they are to keep the file size. "read as zero pages"
is necessary, but "written as (actual) zero pages" isn't.

>Anyway, are you going to take the patch by Ivan, or my patch (after I
>remove exclude_nodata_pages)?

Ivan's patch is necessary to follow the ELF specification, but also
your patch (with exclude_nodata_pages) should be merged.
The two patches have different approach to expand struct pt_load_segment,
hence could you manage both work ?

Ivan, your v2 patch has no problems, but could I leave this work to Petr
since the two patches touch the same area ?


Thanks,
Atsushi Kumagai

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

Re: [PATCH] makedumpfile: readpage_elf: handle 0-pages not stored in the ELF file

[Ivan Delalande]

On Tue, Feb 02, 2016 at 06:48:17AM +0000, Atsushi Kumagai wrote:
> >Anyway, are you going to take the patch by Ivan, or my patch (after I
> >remove exclude_nodata_pages)?
> 
> Ivan's patch is necessary to follow the ELF specification, but also
> your patch (with exclude_nodata_pages) should be merged.
> The two patches have different approach to expand struct pt_load_segment,
> hence could you manage both work ?
> 
> Ivan, your v2 patch has no problems, but could I leave this work to Petr
> since the two patches touch the same area ?

Sure, this sounds reasonable.

-- 
Ivan "Colona" Delalande
Arista Networks

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

Re: [PATCH] makedumpfile: readpage_elf: handle 0-pages not stored in the ELF file

[Petr Tesarik]

On Tue, 2 Feb 2016 08:00:44 +0100
Ivan Delalande <colona@arista.com> wrote:

> On Tue, Feb 02, 2016 at 06:48:17AM +0000, Atsushi Kumagai wrote:
> > >Anyway, are you going to take the patch by Ivan, or my patch (after I
> > >remove exclude_nodata_pages)?
> > 
> > Ivan's patch is necessary to follow the ELF specification, but also
> > your patch (with exclude_nodata_pages) should be merged.
> > The two patches have different approach to expand struct pt_load_segment,
> > hence could you manage both work ?
> > 
> > Ivan, your v2 patch has no problems, but could I leave this work to Petr
> > since the two patches touch the same area ?
> 
> Sure, this sounds reasonable.

Hello,

just a short status update. I haven't forgotten this patch, but I've
had some troubles reproducing the issue. To make sure I'm not on the
wrong track, what happened when you tried to dump dmesg on the affected
dump file without the patch?

For me, I crafted a dump file which causes a segfault. Did makedumpfile
also terminate on SIGSEGV? Did it report an error? Or did it just
produce wrong output?

Thanks,
Petr T

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

Kexec on ARM?

[Rudici Cazeao]

All,
After compiling the exec tools from my ubuntu machine to run on an ARMv7 target,

1)	I loaded the kernel using : kexec  -f uImage --append="$ ( cat /proc/cmdline )"
2)	Afterwards, I ran: kexec -e 

Note: uImage was successfully loaded using u-boot.

From my serial output, I get the following messages:

Bye!
Uncompressing Linux... done, booting the kernel.   And it just hangs there.

Occasionaly, it goes further and actually prints:

Linux version xxxxxx 
Date: XXX
CPU:XXX
Machine:XXX
Early serial console at MMIO32 0Xxxxxxxxxx

Does anyone have any idea on how to help fix this.

I am also constrained to use kexect-tools 2.0.2




Thanks,


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

Re: [PATCH] makedumpfile: readpage_elf: handle 0-pages not stored in the ELF file

[Ivan Delalande]

Hi,

On Tue, Feb 09, 2016 at 09:31:50AM +0100, Petr Tesarik wrote:
> On Tue, 2 Feb 2016 08:00:44 +0100 Ivan Delalande <colona@arista.com> wrote:
> > On Tue, Feb 02, 2016 at 06:48:17AM +0000, Atsushi Kumagai wrote:
> > > >Anyway, are you going to take the patch by Ivan, or my patch (after I
> > > >remove exclude_nodata_pages)?
> > > 
> > > Ivan's patch is necessary to follow the ELF specification, but also
> > > your patch (with exclude_nodata_pages) should be merged.
> > > The two patches have different approach to expand struct pt_load_segment,
> > > hence could you manage both work ?
> > > 
> > > Ivan, your v2 patch has no problems, but could I leave this work to Petr
> > > since the two patches touch the same area ?
> > 
> > Sure, this sounds reasonable.
> 
> just a short status update. I haven't forgotten this patch, but I've
> had some troubles reproducing the issue. To make sure I'm not on the
> wrong track, what happened when you tried to dump dmesg on the affected
> dump file without the patch?
> 
> For me, I crafted a dump file which causes a segfault. Did makedumpfile
> also terminate on SIGSEGV? Did it report an error? Or did it just
> produce wrong output?

Yeah, just a segfault, without any other message or output. It was
coming from readpage_elf, at the call to memset in the `if (!offset1)`
block, as it thinks it is handling one of this weird overlapping
segments from ia64 and computes bad offsets.

Thank you,
-- 
Ivan "Colona" Delalande
Arista Networks

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

Re: [PATCH] makedumpfile: readpage_elf: handle 0-pages not stored in the ELF file

[Petr Tesarik]

On Wed, 10 Feb 2016 04:21:18 +0100
Ivan Delalande <colona@arista.com> wrote:

> Hi,
> 
> On Tue, Feb 09, 2016 at 09:31:50AM +0100, Petr Tesarik wrote:
>[...]
> > just a short status update. I haven't forgotten this patch, but I've
> > had some troubles reproducing the issue. To make sure I'm not on the
> > wrong track, what happened when you tried to dump dmesg on the affected
> > dump file without the patch?
> > 
> > For me, I crafted a dump file which causes a segfault. Did makedumpfile
> > also terminate on SIGSEGV? Did it report an error? Or did it just
> > produce wrong output?
> 
> Yeah, just a segfault, without any other message or output. It was
> coming from readpage_elf, at the call to memset in the `if (!offset1)`
> block, as it thinks it is handling one of this weird overlapping
> segments from ia64 and computes bad offsets.

Good. Then I was looking at the same bug, but fixed it differently.
Anyway, thank you very much for the original fix! It set me in the
right direction.

Petr T

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

Kexec on ARM?

[Rudici Cazeao]

All,
After compiling the exec tools from my ubuntu machine to run on an ARMv7 target,

1)	I loaded the kernel using : kexec  -f uImage --append="$ ( cat /proc/cmdline )"
2)	Afterwards, I ran: kexec -e 

Note: uImage was successfully loaded using u-boot.

From my serial output, I get the following messages:

Bye!
Uncompressing Linux... done, booting the kernel.   And it just hangs there.

Occasionaly, it goes further and actually prints:

Linux version xxxxxx 
Date: XXX
CPU:XXX
Machine:XXX
Early serial console at MMIO32 0Xxxxxxxxxx

Does anyone have any idea on how to help fix this.

I am also constrained to use kexect-tools 2.0.2




Thanks,


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

[PATCH v2] makedumpfile: readpage_elf: handle 0-pages not stored in the ELF file

[Ivan Delalande]

Handle pages filled with zero that are not stored in the ELF file
directly, but have addresses between p_filesz and p_memsz of a segment.

This allows makedumpfile to dump-dmesg from a previously makedumpfile-ed
vmcore where all 0-page were excluded (dump_level & 1) for example.

Signed-off-by: Ivan Delalande <colona@arista.com>
---
 elf_info.c     | 22 ++++++++++++++++++++++
 elf_info.h     |  1 +
 makedumpfile.c | 17 +++++++++++++++++
 3 files changed, 40 insertions(+)

diff --git a/elf_info.c b/elf_info.c
index 8bce942..d9d1f42 100644
--- a/elf_info.c
+++ b/elf_info.c
@@ -42,6 +42,7 @@ struct pt_load_segment {
 	unsigned long long	phys_end;
 	unsigned long long	virt_start;
 	unsigned long long	virt_end;
+	size_t			mem_size;
 };
 
 static int			nr_cpus;             /* number of cpu */
@@ -166,6 +167,7 @@ dump_Elf_load(Elf64_Phdr *prog, int num_load)
 	pls->virt_start  = prog->p_vaddr;
 	pls->virt_end    = pls->virt_start + prog->p_filesz;
 	pls->file_offset = prog->p_offset;
+	pls->mem_size    = prog->p_memsz;
 
 	DEBUG_MSG("LOAD (%d)\n", num_load);
 	DEBUG_MSG("  phys_start : %llx\n", pls->phys_start);
@@ -584,6 +586,26 @@ offset_to_pt_load_end(off_t offset)
 }
 
 /*
+ * Return true if a page is in the zone between p_filesz and p_memsz of a
+ * segment, which is a page filled with zero and not stored in the ELF file.
+ */
+int
+paddr_is_null(unsigned long long paddr)
+{
+	int i;
+	struct pt_load_segment *pls;
+
+	for (i = 0; i < num_pt_loads; i++) {
+		pls = &pt_loads[i];
+		if ((paddr >= pls->phys_end)
+		    && (paddr + info->page_size
+			<= pls->phys_start + pls->mem_size))
+			return TRUE;
+	}
+	return FALSE;
+}
+
+/*
  * Judge whether the page is fractional or not.
  */
 int
diff --git a/elf_info.h b/elf_info.h
index e712253..4823311 100644
--- a/elf_info.h
+++ b/elf_info.h
@@ -36,6 +36,7 @@ unsigned long long page_head_to_phys_start(unsigned long long head_paddr);
 unsigned long long page_head_to_phys_end(unsigned long long head_paddr);
 off_t offset_to_pt_load_start(off_t offset);
 off_t offset_to_pt_load_end(off_t offset);
+int paddr_is_null(unsigned long long paddr);
 unsigned long long vaddr_to_paddr_general(unsigned long long vaddr);
 off_t vaddr_to_offset_slow(int fd, char *filename, unsigned long long vaddr);
 unsigned long long get_max_paddr(void);
diff --git a/makedumpfile.c b/makedumpfile.c
index b802446..491f278 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -652,11 +652,20 @@ readpage_elf(unsigned long long paddr, void *bufptr)
 	size_t size1, size2;
 	unsigned long long phys_start, phys_end, frac_head = 0;
 
+	/*
+	 * Check the 0-filled pages that are not in the ELF file.
+	 */
+	if (paddr_is_null(paddr)) {
+		memset(bufptr, 0, info->page_size);
+		return TRUE;
+	}
+
 	offset1 = paddr_to_offset(paddr);
 	offset2 = paddr_to_offset(paddr + info->page_size);
 	phys_start = paddr;
 	phys_end = paddr + info->page_size;
 
+
 	/*
 	 * Check the case phys_start isn't aligned by page size like below:
 	 *
@@ -726,6 +735,14 @@ readpage_elf_parallel(int fd_memory, unsigned long long paddr, void *bufptr)
 	size_t size1, size2;
 	unsigned long long phys_start, phys_end, frac_head = 0;
 
+	/*
+	 * Check the 0-filled pages that are not in the ELF file.
+	 */
+	if (paddr_is_null(paddr)) {
+		memset(bufptr, 0, info->page_size);
+		return TRUE;
+	}
+
 	offset1 = paddr_to_offset(paddr);
 	offset2 = paddr_to_offset(paddr + info->page_size);
 	phys_start = paddr;
-- 
2.7.0


-- 
Ivan "Colona" Delalande
Arista Networks

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