Re: [PATCH] dump: enhance win_dump_available to report properly

["Daniel P. Berrangé" <berrange@redhat.com>]

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/devel@lists.libvirt.org/thread/HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB/#HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB
> [PATCH 0/4] Allow xml-configured coredump format on VM crash
> 
> Signed-off-by: Nikolai Barybin <nikolai.barybin@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 :|