Re: [PATCH QEMU HCI-8.1 v6 17/34] migration: vmstate_save_state_v: fix double error_setg

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] migration: vmstate_save_state_v() skips error_setg() for positive put() returns, then error_prepend() crashes
--

commit 9a80393fe491fbf7dc54d43181d6301bb98018c6
Author: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

migration: vmstate_save_state_v: fix double error_setg

This patch avoids calling error_setg() twice on the same errp when a nested
vmstate_save_state_v() or vmstate_save_state() fails. It sets the Error
only for leaf put() failures and uses error_prepend() in the common failure
branch.

Link: https://lore.kernel.org/qemu-devel/20260304212303.667141-2-vsementsov@yandex-team.ru

> diff --git a/migration/vmstate.c b/migration/vmstate.c
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c

[ ... ]

> @@ -523,6 +523,9 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                  } else {
>                      ret = inner_field->info->put(f, curr_elem, size,
>                                                   inner_field, vmdesc_loop);
> +                    if (ret < 0) {
> +                        error_setg(errp, "put failed");
> +                    }
>                  }

[Severity: Medium]
Can a positive return from put() leave *errp unset here? The new
error_setg() runs only when ret < 0, but the failure branch below runs for
any non-zero ret.

The old code called error_setg() for every non-zero ret. Nothing in the
VMStateInfo.put contract restricts the return value to zero or a negative
value.

There is an in-tree put() callback that returns a positive value.
put_power() in target/arm/machine.c returns 1 when cpu->power_state is
neither PSCI_ON nor PSCI_OFF:

    if (cpu->power_state == PSCI_ON ||
        cpu->power_state == PSCI_OFF) {
        ...
        return 0;
    } else {
        return 1;
    }

> @@ -535,8 +538,8 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                  }
>  
>                  if (ret) {
> -                    error_setg(errp, "Save of field %s/%s failed",
> -                                vmsd->name, field->name);
> +                    error_prepend(errp, "Save of field %s/%s failed: ",
> +                                  vmsd->name, field->name);

With ret > 0 from put() and a non-NULL errp whose *errp is NULL, this
error_prepend() call reaches error_vprepend() in util/error.c. That
function only checks for errp == NULL, then does:

    g_string_append(newmsg, (*errp)->msg);

Can this dereference a NULL *errp and abort QEMU on the migration or
savevm save path, for example when a save races with an ARM PSCI
power-state transition? Callers such as put_tmp() and virtio_save() pass a
real Error **.

Would it be better to use "if (ret)" instead of "if (ret < 0)" for the
leaf put() error_setg(), so that *errp is set for every non-zero return?

I found no later patch in the series that changes this. The upstream commit
d41ce10d0f5a has the same check.

>                      if (vmsd->post_save) {
>                          vmsd->post_save(opaque);
>                      }

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261030-vfio_v1-v6-0-45de246bc92e@virtuozzo.com?part=17