spapr: Don't hijack current_machine->boot_order

[PATCH] spapr: Don't hijack current_machine->boot_order

Posted by Greg Kurz 2 years, 11 months ago

QEMU 6.0 moved all the -boot variables to the machine. Especially, the
removal of the boot_order static changed the handling of '-boot once'
from:

    if (boot_once) {
        qemu_boot_set(boot_once, &error_fatal);
        qemu_register_reset(restore_boot_order, g_strdup(boot_order));
    }

to

    if (current_machine->boot_once) {
        qemu_boot_set(current_machine->boot_once, &error_fatal);
        qemu_register_reset(restore_boot_order,
                            g_strdup(current_machine->boot_order));
    }

This means that we now register as subsequent boot order a copy
of current_machine->boot_once that was just set with the previous
call to qemu_boot_set(), i.e. we never transition away from the
once boot order.

It is certainly fragile^Wwrong for the spapr code to hijack a
field of the base machine type object like that. The boot order
rework simply turned this software boundary violation into an
actual bug.

Have the spapr code to handle that with its own field in
SpaprMachineState. Also kfree() the initial boot device
string when "once" was used.

Fixes: 4b7acd2ac821 ("vl: clean up -boot variables")
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1960119
Cc: pbonzini@redhat.com
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 include/hw/ppc/spapr.h | 3 +++
 hw/ppc/spapr.c         | 8 +++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index bbf817af4647..f05219f75ef6 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -223,6 +223,9 @@ struct SpaprMachineState {
     int fwnmi_machine_check_interlock;
     QemuCond fwnmi_machine_check_interlock_cond;
 
+    /* Set by -boot */
+    char *boot_device;
+
     /*< public >*/
     char *kvm_type;
     char *host_model;
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index c23bcc449071..4dd90b75cc52 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1005,7 +1005,7 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt, bool reset)
     _FDT(chosen = fdt_add_subnode(fdt, 0, "chosen"));
 
     if (reset) {
-        const char *boot_device = machine->boot_order;
+        const char *boot_device = spapr->boot_device;
         char *stdout_path = spapr_vio_stdout_path(spapr->vio_bus);
         size_t cb = 0;
         char *bootlist = get_boot_devices_list(&cb);
@@ -2376,8 +2376,10 @@ static SaveVMHandlers savevm_htab_handlers = {
 static void spapr_boot_set(void *opaque, const char *boot_device,
                            Error **errp)
 {
-    MachineState *machine = MACHINE(opaque);
-    machine->boot_order = g_strdup(boot_device);
+    SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
+
+    g_free(spapr->boot_device);
+    spapr->boot_device = g_strdup(boot_device);
 }
 
 static void spapr_create_lmb_dr_connectors(SpaprMachineState *spapr)
-- 
2.26.3

Re: [PATCH] spapr: Don't hijack current_machine->boot_order

Posted by David Gibson 2 years, 11 months ago

On Fri, May 21, 2021 at 06:07:35PM +0200, Greg Kurz wrote:
> QEMU 6.0 moved all the -boot variables to the machine. Especially, the
> removal of the boot_order static changed the handling of '-boot once'
> from:
> 
>     if (boot_once) {
>         qemu_boot_set(boot_once, &error_fatal);
>         qemu_register_reset(restore_boot_order, g_strdup(boot_order));
>     }
> 
> to
> 
>     if (current_machine->boot_once) {
>         qemu_boot_set(current_machine->boot_once, &error_fatal);
>         qemu_register_reset(restore_boot_order,
>                             g_strdup(current_machine->boot_order));
>     }
> 
> This means that we now register as subsequent boot order a copy
> of current_machine->boot_once that was just set with the previous
> call to qemu_boot_set(), i.e. we never transition away from the
> once boot order.
> 
> It is certainly fragile^Wwrong for the spapr code to hijack a
> field of the base machine type object like that. The boot order
> rework simply turned this software boundary violation into an
> actual bug.
> 
> Have the spapr code to handle that with its own field in
> SpaprMachineState. Also kfree() the initial boot device
> string when "once" was used.
> 
> Fixes: 4b7acd2ac821 ("vl: clean up -boot variables")
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1960119
> Cc: pbonzini@redhat.com
> Signed-off-by: Greg Kurz <groug@kaod.org>


Applied to ppc-for-6.1, thanks.

> ---
>  include/hw/ppc/spapr.h | 3 +++
>  hw/ppc/spapr.c         | 8 +++++---
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index bbf817af4647..f05219f75ef6 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -223,6 +223,9 @@ struct SpaprMachineState {
>      int fwnmi_machine_check_interlock;
>      QemuCond fwnmi_machine_check_interlock_cond;
>  
> +    /* Set by -boot */
> +    char *boot_device;
> +
>      /*< public >*/
>      char *kvm_type;
>      char *host_model;
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c23bcc449071..4dd90b75cc52 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1005,7 +1005,7 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt, bool reset)
>      _FDT(chosen = fdt_add_subnode(fdt, 0, "chosen"));
>  
>      if (reset) {
> -        const char *boot_device = machine->boot_order;
> +        const char *boot_device = spapr->boot_device;
>          char *stdout_path = spapr_vio_stdout_path(spapr->vio_bus);
>          size_t cb = 0;
>          char *bootlist = get_boot_devices_list(&cb);
> @@ -2376,8 +2376,10 @@ static SaveVMHandlers savevm_htab_handlers = {
>  static void spapr_boot_set(void *opaque, const char *boot_device,
>                             Error **errp)
>  {
> -    MachineState *machine = MACHINE(opaque);
> -    machine->boot_order = g_strdup(boot_device);
> +    SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
> +
> +    g_free(spapr->boot_device);
> +    spapr->boot_device = g_strdup(boot_device);
>  }
>  
>  static void spapr_create_lmb_dr_connectors(SpaprMachineState *spapr)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson