Re: [Qemu-devel] [PATCH 6/7] scripts/dump-guest-memory.py: add vmcoreinfo

[Laszlo Ersek]

On 07/05/17 11:58, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jul 5, 2017 at 2:22 AM, Laszlo Ersek <address@hidden> wrote:
>> On 06/29/17 15:23, Marc-André Lureau wrote:
>>> Add vmcoreinfo ELF note if vmcoreinfo device is ready.
>>>
>>> To help the python script, add a little global vmcoreinfo_gdb
>>> structure, that is populated with vmcoreinfo_gdb_update().
>>>
>>> Signed-off-by: Marc-André Lureau <address@hidden>
>>> ---
>>>  scripts/dump-guest-memory.py | 32 ++++++++++++++++++++++++++++++++
>>>  include/hw/acpi/vmcoreinfo.h |  1 +
>>>  hw/acpi/vmcoreinfo.c         | 16 ++++++++++++++++
>>>  3 files changed, 49 insertions(+)
>>>
>>> diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
>>> index f7c6635f15..16c3d7cb10 100644
>>> --- a/scripts/dump-guest-memory.py
>>> +++ b/scripts/dump-guest-memory.py
>>> @@ -120,6 +120,20 @@ class ELF(object):
>>>          self.segments[0].p_filesz += ctypes.sizeof(note)
>>>          self.segments[0].p_memsz += ctypes.sizeof(note)
>>>
>>> +
>>> +    def add_vmcoreinfo_note(self, vmcoreinfo):
>>> +        """Adds a vmcoreinfo note to the ELF dump."""
>>> +        chead = type(get_arch_note(self.endianness, 0, 0))
>>> +        header = chead.from_buffer_copy(vmcoreinfo[0:ctypes.sizeof(chead)])
>>> +        note = get_arch_note(self.endianness,
>>> +                             header.n_namesz - 1, header.n_descsz)
>>> +        ctypes.memmove(ctypes.pointer(note), vmcoreinfo, 
>>> ctypes.sizeof(note))
>>> +        header_size = ctypes.sizeof(note) - header.n_descsz
>>> +
>>> +        self.notes.append(note)
>>> +        self.segments[0].p_filesz += ctypes.sizeof(note)
>>> +        self.segments[0].p_memsz += ctypes.sizeof(note)
>>> +
>>>      def add_segment(self, p_type, p_paddr, p_size):
>>>          """Adds a segment to the elf."""
>>>
>>> @@ -505,6 +519,23 @@ shape and this command should mostly work."""
>>>                  cur += chunk_size
>>>                  left -= chunk_size
>>>
>>> +    def add_vmcoreinfo(self):
>>> +        qemu_core = gdb.inferiors()[0]
>>> +
>>> +        gdb.execute("call vmcoreinfo_gdb_update()")
>>
>> I think it's a bad idea to call a function from a process that's just
>> crashed.
> 
> Yeah, if qemu crashed you can't use that script. But we are talking
> about dump of guest kernel, so qemu didn't crash :)

I think we have a misunderstanding here. Extracting the guest kernel
core from the core dump of a crashed QEMU process is the *only* purpose
of the GDB extension implemented in "dump-guest-memory.py".

In other words, if you are loading "dump-guest-memory.py" into gdb, then
QEMU crashed *by definition*. Because otherwise you'd dump the guest
kernel core using the live monitor commands (HMP or QMP).

So, "dump-guest-memory.py" is really a last resort utility, for the case
when the guest kernel does something "interesting" that crashes QEMU
with hopefully localized damage, and most of the data structures
hopefully remain usable. It is not guaranteed at all that
"dump-guest-memory.py" will produce anything useful, dependent on how
corrupt the QEMU process memory is at the time of the SIGSEGV or SIGABRT
(or another fatal signal).

Please see the message on original QEMU commit 3e16d14fd93c
("Python-lang gdb script to extract x86_64 guest vmcore from qemu
coredump", 2013-12-17).

See also the original RFE -- I apologize to non-Red Hatters, the RHBZ is
private because it was filed by a customer --:

https://bugzilla.redhat.com/show_bug.cgi?id=826266

In my opinion, poking at possibly corrupt data structures with the
python script is OK, while executing code directly from the crashed
image is too much. But, again, that's just my opinion.

> 
>>
>> If this feature is so important, maybe we can simply set a global
>> pointer variable at the end of vmcoreinfo_realize(); something like:
>>
>> static void vmcoreinfo_realize(DeviceState *dev, Error **errp)
>> {
>>     static VmcoreinfoState * volatile vmcoreinfo_gdb_helper;
>>     [...]
>>     vmcoreinfo_gdb_helper = VMCOREINFO(dev);
>> }
>>
>> - vmcoreinfo_gdb_helper has function scope, so no other code can abuse
>>   it
>> - it has static storage duration so gdb can access it at any time
>> - the pointer (not the pointed-to object) is qualified volatile, so gcc
>>   cannot optimize out the pointer assignment (which it might be tempted
>>   to do otherwise, due to the pointer never being read within QEMU)
>>
>> Then you can use "vmcoreinfo_gdb_helper->vmcoreinfo_addr_le" to
>> implement all the logic in "dump-guest-memory.py".
> 
> If necessary, I can try that.

Well, I can't claim it is "objectively necessary"; it's just that this
method would satisfy my preferences above (i.e., poking at process data
from python: OK; running code from the process: not OK).

Thanks,
Laszlo