[Makedumpfile Patch 1/6] show_mem_usage(): calculate page offset after elf load

Discussion:

Pratyush Anand

2017-02-06 12:01:46 UTC

x86_64 calculated page offset from PT_LOAD headers. Therefore call
get_page_offset() after get_elf_loads()

Signed-off-by: Pratyush Anand <***@redhat.com>
---
makedumpfile.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index e69b6df9a9ee..6942047199de 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -10944,15 +10944,15 @@ int show_mem_usage(void)

info->dump_level = MAX_DUMP_LEVEL;

- if (!get_page_offset())
- return FALSE;
-
if (!open_files_for_creating_dumpfile())
return FALSE;

if (!get_elf_loads(info->fd_memory, info->name_memory))
return FALSE;

+ if (!get_page_offset())
+ return FALSE;
+
if (!get_sys_kernel_vmcoreinfo(&vmcoreinfo_addr, &vmcoreinfo_len))
return FALSE;

--
2.9.3

Pratyush Anand

2017-02-06 12:01:50 UTC

Permalink

From: Baoquan He <***@redhat.com>

In set_kcore_vmcoreinfo, we calculate the virtual address of vmcoreinfo
by OR operation as below:

kvaddr = (ulong)vmcoreinfo_addr | PAGE_OFFSET;

When mm sections kaslr is not enabled, this is correct since the starting
address of direct mapping section is 0xffff880000000000 which 1T aligned.
Usually system with memory below 1T won't cause problem.

However with mm section kaslr enabled, the starting address of direct
mapping is 1G aligned. The above code makes kvaddr unsure.

So change it to adding operation:
kvaddr = (ulong)vmcoreinfo_addr + PAGE_OFFSET;

Signed-off-by: Baoquan He <***@redhat.com>
---
elf_info.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/elf_info.c b/elf_info.c
index c5743b3cab28..100272f83c48 100644
--- a/elf_info.c
+++ b/elf_info.c
@@ -372,7 +372,7 @@ int set_kcore_vmcoreinfo(uint64_t vmcoreinfo_addr, uint64_t vmcoreinfo_len)
off_t offset_desc;

offset = UNINITIALIZED;
- kvaddr = (ulong)vmcoreinfo_addr | PAGE_OFFSET;
+ kvaddr = (ulong)vmcoreinfo_addr + PAGE_OFFSET;

for (i = 0; i < num_pt_loads; ++i) {
struct pt_load_segment *p = &pt_loads[i];

--
2.9.3

Pratyush Anand

2017-02-06 12:01:47 UTC

Permalink

Call cach_init() before get_kcore_dump_loads(), because latter uses
cache_search().

Call path is like this :
get_kcore_dump_loads() -> process_dump_load() -> vaddr_to_paddr() ->
vtop4_x86_64() -> readmem() -> cache_search()

Signed-off-by: Pratyush Anand <***@redhat.com>
---
makedumpfile.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 6942047199de..3b8e9810468d 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -3878,6 +3878,9 @@ initial(void)
if (!get_value_for_old_linux())
return FALSE;

+ if (!is_xen_memory() && !cache_init())
+ return FALSE;
+
if (info->flag_mem_usage && !get_kcore_dump_loads())
return FALSE;

@@ -4000,9 +4003,6 @@ out:
}
}

- if (!is_xen_memory() && !cache_init())
- return FALSE;
-
if (debug_info && !get_machdep_info())
return FALSE;

--
2.9.3

Pratyush Anand

2017-02-06 12:01:51 UTC

Permalink

From: Baoquan He <***@redhat.com>

Pratyush has made a kernel patch [0] to add the physical address of
direct mapping kcore program segments. So no need to calculate it
specifically now. And the old code is not correct since it calls
vaddr_to_paddr() which has been not ready at that time.

[0] http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?h=next-20170206&id=c9d4e5d7b7fd6c74e134ca44df8a5386efbc561c

Signed-off-by: Baoquan He <***@redhat.com>
---
elf_info.c | 17 -----------------
1 file changed, 17 deletions(-)

diff --git a/elf_info.c b/elf_info.c
index 100272f83c48..8e2437622141 100644
--- a/elf_info.c
+++ b/elf_info.c
@@ -857,22 +857,6 @@ static int exclude_segment(struct pt_load_segment **pt_loads,
return 0;
}

-static int
-process_dump_load(struct pt_load_segment *pls)
-{
- unsigned long long paddr;
-
- paddr = vaddr_to_paddr(pls->virt_start);
- pls->phys_start = paddr;
- pls->phys_end = paddr + (pls->virt_end - pls->virt_start);
- DEBUG_MSG("process_dump_load\n");
- DEBUG_MSG(" phys_start : %llx\n", pls->phys_start);
- DEBUG_MSG(" phys_end : %llx\n", pls->phys_end);
- DEBUG_MSG(" virt_start : %llx\n", pls->virt_start);
- DEBUG_MSG(" virt_end : %llx\n", pls->virt_end);
-
- return TRUE;
-}

int get_kcore_dump_loads(void)
{
@@ -917,7 +901,6 @@ int get_kcore_dump_loads(void)
}

pls[j] = *p;
- process_dump_load(&pls[j]);
j++;
}

--
2.9.3

Pratyush Anand

2017-02-06 12:01:49 UTC

Permalink

After a recent kernel changes [0], kcore passes correct phys_start for
direct mapped region and an invalid value (-1) for all other regions.
arch specific function is_phys_addr() accepts only virt_start.
Therefore, check for valid phys_start in get_kcore_dump_loads().

[0] http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?h=next-20170206&id=c9d4e5d7b7fd6c74e134ca44df8a5386efbc561c

Signed-off-by: Pratyush Anand <***@redhat.com>
---
elf_info.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/elf_info.c b/elf_info.c
index 65ff333cf33a..c5743b3cab28 100644
--- a/elf_info.c
+++ b/elf_info.c
@@ -881,7 +881,8 @@ int get_kcore_dump_loads(void)

for (i = 0; i < num_pt_loads; ++i) {
struct pt_load_segment *p = &pt_loads[i];
- if (!is_phys_addr(p->virt_start))
+ if (p->phys_start == NOT_PADDR
+ || !is_phys_addr(p->virt_start))
continue;
loads++;
}
@@ -901,7 +902,8 @@ int get_kcore_dump_loads(void)

for (i = 0, j = 0; i < num_pt_loads; ++i) {
struct pt_load_segment *p = &pt_loads[i];
- if (!is_phys_addr(p->virt_start))
+ if (p->phys_start == NOT_PADDR
+ || !is_phys_addr(p->virt_start))
continue;
if (j >= loads)
return FALSE;

--
2.9.3

Pratyush Anand

2017-02-06 12:01:48 UTC

Permalink

A kcore PT_LOAD can have a section from vmalloc region. However,
physical address in that header would be invalid (-1) after a recent
kernel changes [0]. Therefore, check for valid physical address while
calculating page_offset or phys_offset.

[0] http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?h=next-20170206&id=c9d4e5d7b7fd6c74e134ca44df8a5386efbc561c

Signed-off-by: Pratyush Anand <***@redhat.com>
---
arch/x86_64.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86_64.c b/arch/x86_64.c
index 893cd516fc8b..e978a36f8878 100644
--- a/arch/x86_64.c
+++ b/arch/x86_64.c
@@ -41,7 +41,8 @@ get_page_offset_x86_64(void)
unsigned long long virt_start;

for (i = 0; get_pt_load(i, &phys_start, NULL, &virt_start, NULL); i++) {
- if (virt_start < __START_KERNEL_map) {
+ if (virt_start < __START_KERNEL_map
+ && phys_start != NOT_PADDR) {
info->page_offset = virt_start - phys_start;
return TRUE;
}
@@ -76,7 +77,8 @@ get_phys_base_x86_64(void)
}

for (i = 0; get_pt_load(i, &phys_start, NULL, &virt_start, NULL); i++) {
- if (virt_start >= __START_KERNEL_map) {
+ if (virt_start >= __START_KERNEL_map
+ && phys_start != NOT_PADDR) {

info->phys_base = phys_start -
(virt_start & ~(__START_KERNEL_map));

--
2.9.3

Atsushi Kumagai

2017-02-13 06:26:47 UTC

Permalink

Hello Pratyush,

`makedumpfile --mem-usage /proc/kcore` has been broken after kaslr specific
modifications. I have proposed a kernel patch [0] which has been ACKed by
Andrew Morton and is available in next-2017020 now. Hopefully, it should be
part of upstream in v4.10 release. This kernel patch helps to fix this
issue for both the case of kaslr enabled and disabled.
To seek other's feedback, I am sending makedumpfile patches before kernel
patch hits upstream.

Thanks for taking care of this feature.
I understand that the problem is:

- phys_base is necessary for VtoP, but:
- SYMBOL(phys_base) is unavailable since /proc/kcore doesn't export VMCOREINFO.
- get_phys_base() doesn't work well since current PT_LOAD doesn't have valid p_paddr.

The new makedumpfile with this patches can show unexpected behavior since it
will refer to invalid p_paddr(always 0). Of course, also current makedumpfile cannot
work if phys_base isn't 0.

If we don't have any way to get phys_base in 1st kernel environment,
I think we should disable this feature in older kernel (e.g. by checking kernel version).

Thanks,
Atsushi Kumagai

[0]
http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?h=next-20170206&id=c9d4e5d7b7fd6c74e134ca44d
f8a5386efbc561c
makedumpfile: Correct the calculation of kvaddr in
set_kcore_vmcoreinfo
makedumpfile: Discard process_dump_load
show_mem_usage(): calculate page offset after elf load
initial(): call cach_init() a bit early
x86_64: check physical address in PT_LOAD for none direct mapped
regions
elf_info: kcore: check for invalid physical address
arch/x86_64.c | 6 ++++--
elf_info.c | 25 +++++--------------------
makedumpfile.c | 12 ++++++------
3 files changed, 15 insertions(+), 28 deletions(-)
--
2.9.3
_______________________________________________
kexec mailing list
http://lists.infradead.org/mailman/listinfo/kexec

Pratyush Anand

2017-02-13 07:34:16 UTC

Permalink

Post by Atsushi Kumagai
Hello Pratyush,

Thanks for taking care of this feature.
- SYMBOL(phys_base) is unavailable since /proc/kcore doesn't export VMCOREINFO.
- get_phys_base() doesn't work well since current PT_LOAD doesn't have valid p_paddr.

Yes, this is what the problem is.

Post by Atsushi Kumagai
The new makedumpfile with this patches can show unexpected behavior since it
will refer to invalid p_paddr(always 0). Of course, also current makedumpfile cannot
work if phys_base isn't 0.
If we don't have any way to get phys_base in 1st kernel environment,
I think we should disable this feature in older kernel (e.g. by checking kernel version).

Hummm..I had thought that distros with old kernel will backport kernel
patch and then can use this feature. If we disable it for older kernel,
then they will not be able to do it.

would it be fine if I cc kernel patch to stable mailing list? It seems
that patch can be easily applied all older kernel.

~Pratyush

Atsushi Kumagai

2017-02-13 08:28:32 UTC

Permalink

Post by Pratyush Anand

Post by Atsushi Kumagai
Hello Pratyush,

Yes, this is what the problem is.

Hummm..I had thought that distros with old kernel will backport kernel
patch and then can use this feature. If we disable it for older kernel,
then they will not be able to do it.
would it be fine if I cc kernel patch to stable mailing list? It seems
that patch can be easily applied all older kernel.

Even if there are some "supported" older kernels, there is no guarantee
that all of the kernels provide valid p_paddr. So makedumpfile have to
know whether the target kernel supports --mem-usage or not in older kernel.

Is it possible to add a new stuff into PT_NOTE of /proc/kcore to signify the
kernel supports --mem-usage ? Otherwise, my only idea is --force option for
older kernel, expert user can use it if he is sure that the kernel has valid
p_paddr.

Thanks,
Atsushi Kumagai

Pratyush Anand

2017-02-14 04:43:34 UTC

Permalink

Post by Atsushi Kumagai

Post by Pratyush Anand

Post by Atsushi Kumagai
Hello Pratyush,

Yes, this is what the problem is.

Hummm..I had thought that distros with old kernel will backport kernel
patch and then can use this feature. If we disable it for older kernel,
then they will not be able to do it.
would it be fine if I cc kernel patch to stable mailing list? It seems
that patch can be easily applied all older kernel.

Even if there are some "supported" older kernels, there is no guarantee
that all of the kernels provide valid p_paddr. So makedumpfile have to
know whether the target kernel supports --mem-usage or not in older kernel.
Is it possible to add a new stuff into PT_NOTE of /proc/kcore to signify the
kernel supports --mem-usage ? Otherwise, my only idea is --force option for
older kernel, expert user can use it if he is sure that the kernel has valid
p_paddr.

I think, we can go with --force. I will send next revision after kernel
patch hits Linus's tree.

Thanks for your review.

~Pratyush