On Wed, Aug 27, 2025 at 04:15:27PM +0300, Nikolai Barybin wrote:
> QMP query-dump-guest-memory-capability reports win dump as available for
> any x86 VM, which is false.
>
> This patch implements proper query of vmcoreinfo and calculation of
> guest note size. Based on that we can surely report whether win dump
> available or not.
>
> To perform this I suggest to split dump_init() into dump_preinit() and
> dump_init_complete() to avoid exausting copypaste in
> win_dump_available().
>
> For further reference one may review this libvirt discussion:
> https://lists.libvirt.org/archives/list/de...@lists.libvirt.org/thread/HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB/#HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB
> [PATCH 0/4] Allow xml-configured coredump format on VM crash
>
> Signed-off-by: Nikolai Barybin <nikolai.bary...@virtuozzo.com>
> ---
> During first series discussion Den mentions that that code will not work
> on 32bit guest with more than 4Gb RAM on i386.
>
> This issue required even more code duplication between dump_init() and
> win_dump_available() which we'd like to avoid as mentioned by Daniel.
>
> Hence I present 2nd version of this fix:
> - split dump_init() into dump_preinit() and dump_init_complete()
> - pass pre-inited dump structure with calculated guest note size to
> win_dump_available()
> - call header check and guest note size check inside
> win_dump_available()
> ---
> dump/dump.c | 129
> ++++++++++++++++++++++++++++++++------------------------
> dump/win_dump.c | 23 ++++++++--
> dump/win_dump.h | 2 +-
> 3 files changed, 95 insertions(+), 59 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index
> 15bbcc0c6192822cf920fcb7d60eb7d2cfad0952..19341fa42feef4d1c50dbb3a892ded59a3468d20
> 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1777,10 +1777,7 @@ static void vmcoreinfo_update_phys_base(DumpState *s)
> g_strfreev(lines);
> }
>
> -static void dump_init(DumpState *s, int fd, bool has_format,
> - DumpGuestMemoryFormat format, bool paging, bool
> has_filter,
> - int64_t begin, int64_t length, bool kdump_raw,
> - Error **errp)
> +static void dump_preinit(DumpState *s, Error **errp)
> {
> ERRP_GUARD();
> VMCoreInfoState *vmci = vmcoreinfo_find();
> @@ -1788,16 +1785,6 @@ static void dump_init(DumpState *s, int fd, bool
> has_format,
> int nr_cpus;
> int ret;
>
> - s->has_format = has_format;
> - s->format = format;
> - s->written_size = 0;
> - s->kdump_raw = kdump_raw;
> -
> - /* kdump-compressed is conflict with paging and filter */
> - if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
> - assert(!paging && !has_filter);
> - }
> -
> if (runstate_is_running()) {
> vm_stop(RUN_STATE_SAVE_VM);
> s->resume = true;
> @@ -1814,41 +1801,10 @@ static void dump_init(DumpState *s, int fd, bool
> has_format,
> nr_cpus++;
> }
>
> - s->fd = fd;
> - if (has_filter && !length) {
> - error_setg(errp, "parameter 'length' expects a non-zero size");
> - goto cleanup;
> - }
> - s->filter_area_begin = begin;
> - s->filter_area_length = length;
> -
> - /* First index is 0, it's the special null name */
> - s->string_table_buf = g_array_new(FALSE, TRUE, 1);
> - /*
> - * Allocate the null name, due to the clearing option set to true
> - * it will be 0.
> - */
> - g_array_set_size(s->string_table_buf, 1);
> -
> memory_mapping_list_init(&s->list);
> -
> guest_phys_blocks_init(&s->guest_phys_blocks);
> guest_phys_blocks_append(&s->guest_phys_blocks);
> - s->total_size = dump_calculate_size(s);
> -#ifdef DEBUG_DUMP_GUEST_MEMORY
> - fprintf(stderr, "DUMP: total memory to dump: %lu\n", s->total_size);
> -#endif
>
> - /* it does not make sense to dump non-existent memory */
> - if (!s->total_size) {
> - error_setg(errp, "dump: no guest memory to dump");
> - goto cleanup;
> - }
> -
> - /* get dump info: endian, class and architecture.
> - * If the target architecture is not supported, cpu_get_dump_info() will
> - * return -1.
> - */
> ret = cpu_get_dump_info(&s->dump_info, &s->guest_phys_blocks);
> if (ret < 0) {
> error_setg(errp,
> @@ -1906,6 +1862,56 @@ static void dump_init(DumpState *s, int fd, bool
> has_format,
> }
> }
>
> + s->nr_cpus = nr_cpus;
> + return;
> +cleanup:
> + dump_cleanup(s);
> +}
The 'dump_cleanup' call is unsafe.
In qmp_query_dump_guest_memory_capability we initialize 's' using
'dump_state_prepare' which just zero's the struct, aside from
the 'status' field.
Meanwhile 'dump_cleanup' will unconditionally do:
close(s->fd);
and 'fd' will be 0, as in stdin, so we break any usage of stdin
that QEMU has. Then some other unlucky part of QEMU will open a
FD and get given FD == 0, making things potentially even worse.
We need 'dump_state_prepare' to set 's->fd = -1', and in
dump_cleanup we should check for s->fd != -1, and after
closing it, must set it back to '-1'.
In fact, I think even the existing dump code is broken in
this respect, and so this should likely be a separate fix
we can send to stable.
I think the 'migrate_del_blocker' call in dump_cleanup
is potentially unsafe too, as it might try to delete a
blocker that is not registered.
> @@ -2215,6 +2224,14 @@ DumpGuestMemoryCapability
> *qmp_query_dump_guest_memory_capability(Error **errp)
> g_new0(DumpGuestMemoryCapability, 1);
> DumpGuestMemoryFormatList **tail = &cap->formats;
>
> + DumpState *s = &dump_state_global;
> + dump_state_prepare(s);
This is unsafe.
Consider we have 2 distinct monitor clients. One client is
in the middle of a dump operation, when another client calls
query-dump-guest-memory-capability - the latter will scribble
over state used by the former. We must use a local stack
allocated DumpState in this query command, instead of
dump_state_global.
> + dump_preinit(s, errp);
> + if (*errp) {
> + qatomic_set(&s->status, DUMP_STATUS_FAILED);
> + return cap;
> + }
> +
> /* elf is always available */
> QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_ELF);
>
> @@ -2234,9 +2251,11 @@ DumpGuestMemoryCapability
> *qmp_query_dump_guest_memory_capability(Error **errp)
> QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_SNAPPY);
> #endif
>
> - if (win_dump_available(NULL)) {
> + if (win_dump_available(s, NULL)) {
> QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_WIN_DMP);
> }
>
> + dump_cleanup(s);
> + qatomic_set(&s->status, DUMP_STATUS_NONE);
> return cap;
> }
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
