[pve-devel] applied: [PATCH v2 qemu 1/2] savevm-async: avoid segfault when aborting snapshot

[Wolfgang Bumiller]

applied both patches, thanks

One nit for a possible followup though:

On Thu, Aug 18, 2022 at 01:44:16PM +0200, Fiona Ebner wrote:
> Reported in the community forum[0].
> 
> For 6.1.0, there were a few changes to the coroutine-sleep API, but
> the adaptations in f376b2b ("update and rebase to QEMU v6.1.0") made
> a mistake.
> 
> Currently, target_close_wait is NULL when passed to
> qemu_co_sleep_ns_wakeable(), which further passes it to
> qemu_co_sleep(), but there, it is dereferenced when trying to access
> the 'to_wake' member:
> 
> > Thread 1 "kvm" received signal SIGSEGV, Segmentation fault.
> > qemu_co_sleep (w=0x0) at ../util/qemu-coroutine-sleep.c:57
> 
> To fix it, create a proper struct and pass its address instead. Also
> call qemu_co_sleep_wake unconditionally, because the NULL check (for
> the 'to_wake' member) is done inside the function itself.
> 
> This patch is based on what the QEMU commits introducing the changes
> to the coroutine-sleep API did to the callers in QEMU:
> eaee072085 ("coroutine-sleep: allow qemu_co_sleep_wake that wakes nothing")
> 29a6ea24eb ("coroutine-sleep: replace QemuCoSleepState pointer with struct in the API")
> 
> [0]: https://forum.proxmox.com/threads/112130/
> 
> Tested-by: Mira Limbeck <m.limbeck at proxmox.com>
> Signed-off-by: Fiona Ebner <f.ebner at proxmox.com>
> ---
> 
> Changes from v1:
>     * Added Mira's T-b tag.
> 
>  ...async-for-background-state-snapshots.patch | 20 +++++++++----------
>  ...add-optional-buffer-size-to-QEMUFile.patch |  6 +++---
>  ...-register-yank-before-migration_inco.patch |  4 ++--
>  3 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/debian/patches/pve/0016-PVE-add-savevm-async-for-background-state-snapshots.patch b/debian/patches/pve/0016-PVE-add-savevm-async-for-background-state-snapshots.patch
> index 0197289..62a8e98 100644
> --- a/debian/patches/pve/0016-PVE-add-savevm-async-for-background-state-snapshots.patch
> +++ b/debian/patches/pve/0016-PVE-add-savevm-async-for-background-state-snapshots.patch
> @@ -23,19 +23,21 @@ Signed-off-by: Dietmar Maurer <dietmar at proxmox.com>
>  Signed-off-by: Wolfgang Bumiller <w.bumiller at proxmox.com>
>  [improve aborting]
>  Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> +[FE: further improve aborting]
> +Signed-off-by: Fiona Ebner <f.ebner at proxmox.com>
>  ---
>   hmp-commands-info.hx         |  13 +
>   hmp-commands.hx              |  33 ++
>   include/migration/snapshot.h |   2 +
>   include/monitor/hmp.h        |   5 +
>   migration/meson.build        |   1 +
> - migration/savevm-async.c     | 598 +++++++++++++++++++++++++++++++++++
> + migration/savevm-async.c     | 596 +++++++++++++++++++++++++++++++++++
>   monitor/hmp-cmds.c           |  57 ++++
>   qapi/migration.json          |  34 ++
>   qapi/misc.json               |  32 ++
>   qemu-options.hx              |  12 +
>   softmmu/vl.c                 |  10 +
> - 11 files changed, 797 insertions(+)
> + 11 files changed, 795 insertions(+)
>   create mode 100644 migration/savevm-async.c
>  
>  diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> @@ -151,10 +153,10 @@ index 8b5ca5c047..1e2aec8486 100644
>   ), gnutls)
>  diff --git a/migration/savevm-async.c b/migration/savevm-async.c
>  new file mode 100644
> -index 0000000000..79a0cda906
> +index 0000000000..88215cdb70
>  --- /dev/null
>  +++ b/migration/savevm-async.c
> -@@ -0,0 +1,598 @@
> +@@ -0,0 +1,596 @@
>  +#include "qemu/osdep.h"
>  +#include "migration/migration.h"
>  +#include "migration/savevm.h"
> @@ -210,7 +212,7 @@ index 0000000000..79a0cda906
>  +    int64_t total_time;
>  +    QEMUBH *finalize_bh;
>  +    Coroutine *co;
> -+    QemuCoSleep *target_close_wait;
> ++    QemuCoSleep target_close_wait;
>  +} snap_state;
>  +
>  +static bool savevm_aborted(void)
> @@ -285,9 +287,7 @@ index 0000000000..79a0cda906
>  +        blk_unref(snap_state.target);
>  +        snap_state.target = NULL;
>  +
> -+        if (snap_state.target_close_wait) {
> -+            qemu_co_sleep_wake(snap_state.target_close_wait);
> -+        }
> ++        qemu_co_sleep_wake(&snap_state.target_close_wait);
>  +    }
>  +
>  +    return ret;
> @@ -549,6 +549,7 @@ index 0000000000..79a0cda906
>  +    snap_state.bs_pos = 0;
>  +    snap_state.total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>  +    snap_state.blocker = NULL;
> ++    snap_state.target_close_wait.to_wake = NULL;

To be more robust I wonder if this should be a memset on the whole
QemuCoSleep, or use something like:

    snap_state.target_close_wait = (QemuCoSleep){ .to_wake = NULL };