Re: Concerns regarding e17bebd049 ("dump: Set correct vaddr for ELF dump")

[Jon Doron]

Hi Stephen,

Like you have said the reason is as I wrote in the commit message, 
without "fixing" the vaddr GDB is messing up mapping and working with 
the generated core file.

This patch is almost 4 years old, perhaps some changes to GDB has been 
introduced to resolve this, I have not checked since then.

As I'm no longer using this feature and have not worked and tested it 
in a long while, so I have no obligations to this change, but perhaps
someone else might be using it...

-- Jon.

On 19/09/2023, Stephen Brennan wrote:
> Hello all,
>
> I've started working on better support and documentation around
> hypervisor vmcores in the Drgn debugger[1]. Of course there's quite a
> lot of different implementations out there, but recently I'm looking at
> Qemu kdump and ELF vmcores generated via dump-guest-memory, and one
> thing caught my eye. I generated a ELF vmcore without the paging option
> enabled, and without the guest note loaded, and the resulting core
> dump's program header looked like this:
>
> $ eu-readelf -l dumpfile2
> Program Headers:
>  Type           Offset   VirtAddr           PhysAddr           FileSiz  MemSiz  
>  Flg Align
>  NOTE           0x000168 0x0000000000000000 0x0000000000000000 0x001980 
> 0x001980     0x0
>  LOAD           0x001ae8 0x0000000000000000 0x0000000000000000 0x80000000 
> 0x80000000     0x0
>  LOAD           0x80001ae8 0x00000000fffc0000 0x00000000fffc0000 0x040000 
> 0x040000     0x0
>
> In particular, the "VirtAddr" field for the loadable segment shows a
> confusing address - it appears to reuse the segment's physical address,
> despite the fact that there's no actual corresponding mapping.
>
> By comparison, the /proc/kcore and /proc/vmcore ELF vmcores use the
> VirtAddr in the program header to represent the real virtual memory
> mappings in use by the kernel. Debuggers can directly use these without
> needing to walk page tables. If there is no virtual memory mapping
> information available, I would have expected a placeholder value such as
> 0000... or FFFF... to take the place of VirtAddr here so a debugger can
> detect the lack of virtual mappings and know that it needs to use
> architecture-specific details (and the vmcoreinfo) to find the page
> tables and accurately determine memory mappings. As it is, this program
> header seems to advertise to a debugger, "yes, we have the virtual
> memory mappings" when in fact, that's not the case.
>
> It seems that this behavior was introduced in e17bebd049 ("dump: Set
> correct vaddr for ELF dump")[2], a small commit I'll reproduce below.
> The justification seems to be that it fixes an issue reading the vmcore
> with GDB, but I wonder if that's not a GDB bug which should have been
> fixed with them? If GDB aims to support ELF kernel core dumps,
> presumably it should be handling physical addresses separately from
> virtual addresses. And if GDB doesn't aim for this, but you'd like to
> con it into reading your core dump, presumably the onus is on you to
> edit the ELF VirtAddr field to suit your needs? It should be QEMU's
> primary goal to produce a *correct* vmcore, not work around limitations
> or bugs in GDB.
>
> I'd like to propose reverting this, since it makes it impossible to
> interpret QEMU ELF vmcores, unless you discard all the virtual addresses
> in the program headers, and unconditionally do all the page table walks
> yourself. But I wanted to see if there was some justification for this
> behavior that I missed.
>
> Thanks,
> Stephen
>
> [1]: https://github.com/osandov/drgn
> [2]: https://lore.kernel.org/qemu-devel/20181225125344.4482-1-arilou@gmail.com/
>
> ---
>
> commit e17bebd049d78f489c2cff755e2b66a0536a156e
> Author: Jon Doron <arilou@gmail.com>
> Date:   Wed Jan 9 10:22:03 2019 +0200
>
>    dump: Set correct vaddr for ELF dump
>
>    vaddr needs to be equal to the paddr since the dump file represents the
>    physical memory image.
>
>    Without setting vaddr correctly, GDB would load all the different memory
>    regions on top of each other to vaddr 0, thus making GDB showing the wrong
>    memory data for a given address.
>
>    Signed-off-by: Jon Doron <arilou@gmail.com>
>    Message-Id: <20190109082203.27142-1-arilou@gmail.com>
>    Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>    Tested-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>    Acked-by: Laszlo Ersek <lersek@redhat.com>
>
> diff --git a/dump.c b/dump.c
> index ef1d8025c9..107a67165a 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -192,7 +192,7 @@ static void write_elf64_load(DumpState *s, MemoryMapping 
> *memory_mapping,
>     phdr.p_paddr = cpu_to_dump64(s, memory_mapping->phys_addr);
>     phdr.p_filesz = cpu_to_dump64(s, filesz);
>     phdr.p_memsz = cpu_to_dump64(s, memory_mapping->length);
> -    phdr.p_vaddr = cpu_to_dump64(s, memory_mapping->virt_addr);
> +    phdr.p_vaddr = cpu_to_dump64(s, memory_mapping->virt_addr) ?: phdr.p_paddr;
>
>     assert(memory_mapping->length >= filesz);
>
> @@ -216,7 +216,8 @@ static void write_elf32_load(DumpState *s, MemoryMapping 
> *memory_mapping,
>     phdr.p_paddr = cpu_to_dump32(s, memory_mapping->phys_addr);
>     phdr.p_filesz = cpu_to_dump32(s, filesz);
>     phdr.p_memsz = cpu_to_dump32(s, memory_mapping->length);
> -    phdr.p_vaddr = cpu_to_dump32(s, memory_mapping->virt_addr);
> +    phdr.p_vaddr =
> +        cpu_to_dump32(s, memory_mapping->virt_addr) ?: phdr.p_paddr;
>
>     assert(memory_mapping->length >= filesz);
>
> diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
> index 198cd0fe40..2c587cbefc 100644
> --- a/scripts/dump-guest-memory.py
> +++ b/scripts/dump-guest-memory.py
> @@ -163,6 +163,7 @@ def add_segment(self, p_type, p_paddr, p_size):
>         phdr = get_arch_phdr(self.endianness, self.elfclass)
>         phdr.p_type = p_type
>         phdr.p_paddr = p_paddr
> +        phdr.p_vaddr = p_paddr
>         phdr.p_filesz = p_size
>         phdr.p_memsz = p_size
>         self.segments.append(phdr)