[Crash-utility] [PATCH v3 0/4] Generalize KASLR calculation and use it for KDUMPs

[Dave Anderson]

Hi Sergio,  (and Takao and Daisuke)

This patch to map_cpus_to_prstatus_kdump_cmprs():

@@ -153,8 +154,13 @@ resize_note_pointers:
                            dd->num_qemu_notes * sizeof(void *))) == NULL)
                                error(FATAL,
                                    "compressed kdump: cannot realloc QEMU note pointers\n");
+                       if  ((dd->nt_qemucs_percpu = realloc(dd->nt_qemucs_percpu,
+                           dd->num_qemu_notes * sizeof(void *))) == NULL)
+                               error(FATAL,
+                                   "compressed kdump: cannot realloc QEMU note pointers\n");
                } else
                        free(dd->nt_qemu_percpu);
+                       free(dd->nt_qemucs_percpu);
        }
 }

is missing brackets around the else clause, resulting in:

        if (machine_type("X86_64") || machine_type("X86") ||
            machine_type("ARM64")) {
                if ((dd->nt_prstatus_percpu = realloc(dd->nt_prstatus_percpu,
                    dd->num_prstatus_notes * sizeof(void *))) == NULL)
                        error(FATAL,
                            "compressed kdump: cannot realloc NT_PRSTATUS note pointers\n");
                if (dd->num_qemu_notes) {
                        if  ((dd->nt_qemu_percpu = realloc(dd->nt_qemu_percpu,
                            dd->num_qemu_notes * sizeof(void *))) == NULL)
                                error(FATAL,
                                    "compressed kdump: cannot realloc QEMU note pointers\n");
                        if  ((dd->nt_qemucs_percpu = realloc(dd->nt_qemucs_percpu,
                            dd->num_qemu_notes * sizeof(void *))) == NULL)
                                error(FATAL,
                                    "compressed kdump: cannot realloc QEMU note pointers\n");
                } else
                        free(dd->nt_qemu_percpu);
                        free(dd->nt_qemucs_percpu);
        }

So as soon as the realloc(dd->nt_qemucs_percpu, ...) is done, it gets free()'d.
So I'm not sure how your testing worked, but presumably, the freed data 
remained untouched at least long enough to get the job done during 
initialization.

One minor nit -- in order to be able to search for functions in a file 
by entering "^function_name", I prefer that function names always be 
placed on their own line, with the return type or void on the line above.  

Other that, the patch looks OK.  

As I mentioned before, I will need ACK's from Takao Indou and Daisuke
Hatayama.  They can fix the bracket bug above themselves for their
sadump testing, so it's probably not necessary to repost the patch.
I've added d.hatayama at jp.fujitsu.com to the cc: list. 

Thanks,
  Dave



----- Original Message -----
> Commit 45b74b89530d611b3fa95a1041e158fbb865fa84 added support for
> calculating phys_base and kernel offset for KASLR-enabled kernels on
> SADUMPs by using a technique developed by Takao Indoh. Originally, the
> patchset included support for KDUMPs, but this was dropped in v2, as it
> was deemed unnecessary due to the implementation of the vmcoreinfo
> device in QEMU.
> 
> Sadly, there are many reasons for which the vmcoreinfo device may not be
> present in the moment of taking the memory dump from a VM, ranging from
> a Host running older QEMU/libvirt versions, to misconfigured VMs or
> environments running Hypervisors that doesn't support this device.
> 
> This patchset generalizes the kaslr related functions from sadump.c
> moving them to kaslr_helper.c, and makes KDUMP analysis fallback to
> KASLR offset calculation if vmcoreinfo data is missing.
> 
> These changes have been successfully tested with a 3.10.0-830.el7.x86_64
> under the following conditions:
> 
>  - kdump with KASLR and vmcoreinfo
> 
>  - kdump with KASLR but no vmcoreinfo
> 
>  - kdump without KASLR ("nokaslr" kernel command line option)
> 
> It was also tested that a "crash" patched with these changes still
> builds and runs (live and kdump debugging) on an aarch64 machine.
> 
> changelog:
> 
> v3:
>  - Merge *get_cr3 and *get_idtr functions and move them to
>    kaslr_helper.c
>  - diskdump: drop kaslr_phys_base addition and use
>    sub_header_kdump->phys_base instead.
>  - Unconditionally call x86_64_virt_phys_base after grabbing phys_base
> 
> v2:
>  - Limit application to QEMU ELF and QEMU COMPRESSED dumps (thanks Dave)
>  - Add support for QEMU COMPRESSED dumps (diskdump)
> 
> Sergio Lopez (4):
>   Move kaslr related functions from sadump.c to kaslr_helper.c
>   Move QEMUCPU* structs from netdump.h to defs.h
>   netdump: infer kaslr offset for QEMU ELF dumps without vmcoreinfo
>   diskdump: infer kaslr offset for QEMU COMPRESSED dumps without
>     vmcoreinfo
> 
>  Makefile       |   7 +-
>  defs.h         |  39 +++++
>  diskdump.c     |  61 ++++++++
>  kaslr_helper.c | 488
>  +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  netdump.c      |  54 +++++++
>  netdump.h      |  24 +--
>  sadump.c       | 486
>  ++++----------------------------------------------------
>  symbols.c      |  26 ++-
>  x86_64.c       |  18 ++-
>  9 files changed, 719 insertions(+), 484 deletions(-)
>  create mode 100644 kaslr_helper.c
> 
> --
> 2.14.3
> 
>