Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
MIME-Version: 1.0
References: <20230914015459.51740-1-sashal@kernel.org>
In-Reply-To: <20230914015459.51740-1-sashal@kernel.org>
From:   Paolo Bonzini <pbonzini@redhat.com>
Date:   Sun, 15 Oct 2023 13:59:06 +0200
Message-ID: <CABgObfbKV3TvVeixtVvp6f9Qtr47aiStUBsDH2_c6jrDGiWR_A@mail.gmail.com>
Subject: Re: [PATCH AUTOSEL 6.5 1/7] x86/reboot: VMCLEAR active VMCSes before
 emergency reboot
To:     Sasha Levin <sashal@kernel.org>
Cc:     linux-kernel@vger.kernel.org, stable@vger.kernel.org,
        Sean Christopherson <seanjc@google.com>,
        Andrew Cooper <Andrew.Cooper3@citrix.com>, tglx@linutronix.de,
        mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
        x86@kernel.org, akpm@linux-foundation.org, bhe@redhat.com,
        eric.devolder@oracle.com, hbathini@linux.ibm.com,
        sourabhjain@linux.ibm.com, bhelgaas@google.com,
        kai.huang@intel.com, peterz@infradead.org, jpoimboe@kernel.org,
        tiwai@suse.de, kvm@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Sep 14, 2023 at 3:55=E2=80=AFAM Sasha Levin <sashal@kernel.org> wro=
te:
>
> From: Sean Christopherson <seanjc@google.com>
>
> [ Upstream commit b23c83ad2c638420ec0608a9de354507c41bec29 ]
>
> VMCLEAR active VMCSes before any emergency reboot, not just if the kernel
> may kexec into a new kernel after a crash.  Per Intel's SDM, the VMX
> architecture doesn't require the CPU to flush the VMCS cache on INIT.  If
> an emergency reboot doesn't RESET CPUs, cached VMCSes could theoretically
> be kept and only be written back to memory after the new kernel is booted=
,
> i.e. could effectively corrupt memory after reboot.
>
> Opportunistically remove the setting of the global pointer to NULL to mak=
e
> checkpatch happy.

Intended as a cleanup but I guess it does not hurt, since it was the first =
patch
in the large series that included it.

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo


> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>
> Link: https://lore.kernel.org/r/20230721201859.2307736-2-seanjc@google.co=
m
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  arch/x86/include/asm/kexec.h  |  2 --
>  arch/x86/include/asm/reboot.h |  2 ++
>  arch/x86/kernel/crash.c       | 31 -------------------------------
>  arch/x86/kernel/reboot.c      | 22 ++++++++++++++++++++++
>  arch/x86/kvm/vmx/vmx.c        | 10 +++-------
>  5 files changed, 27 insertions(+), 40 deletions(-)
>
> diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
> index 5b77bbc28f969..819046974b997 100644
> --- a/arch/x86/include/asm/kexec.h
> +++ b/arch/x86/include/asm/kexec.h
> @@ -205,8 +205,6 @@ int arch_kimage_file_post_load_cleanup(struct kimage =
*image);
>  #endif
>  #endif
>
> -typedef void crash_vmclear_fn(void);
> -extern crash_vmclear_fn __rcu *crash_vmclear_loaded_vmcss;
>  extern void kdump_nmi_shootdown_cpus(void);
>
>  #endif /* __ASSEMBLY__ */
> diff --git a/arch/x86/include/asm/reboot.h b/arch/x86/include/asm/reboot.=
h
> index 9177b4354c3f5..dc201724a6433 100644
> --- a/arch/x86/include/asm/reboot.h
> +++ b/arch/x86/include/asm/reboot.h
> @@ -25,6 +25,8 @@ void __noreturn machine_real_restart(unsigned int type)=
;
>  #define MRR_BIOS       0
>  #define MRR_APM                1
>
> +typedef void crash_vmclear_fn(void);
> +extern crash_vmclear_fn __rcu *crash_vmclear_loaded_vmcss;
>  void cpu_emergency_disable_virtualization(void);
>
>  typedef void (*nmi_shootdown_cb)(int, struct pt_regs*);
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index cdd92ab43cda4..54cd959cb3160 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -48,38 +48,12 @@ struct crash_memmap_data {
>         unsigned int type;
>  };
>
> -/*
> - * This is used to VMCLEAR all VMCSs loaded on the
> - * processor. And when loading kvm_intel module, the
> - * callback function pointer will be assigned.
> - *
> - * protected by rcu.
> - */
> -crash_vmclear_fn __rcu *crash_vmclear_loaded_vmcss =3D NULL;
> -EXPORT_SYMBOL_GPL(crash_vmclear_loaded_vmcss);
> -
> -static inline void cpu_crash_vmclear_loaded_vmcss(void)
> -{
> -       crash_vmclear_fn *do_vmclear_operation =3D NULL;
> -
> -       rcu_read_lock();
> -       do_vmclear_operation =3D rcu_dereference(crash_vmclear_loaded_vmc=
ss);
> -       if (do_vmclear_operation)
> -               do_vmclear_operation();
> -       rcu_read_unlock();
> -}
> -
>  #if defined(CONFIG_SMP) && defined(CONFIG_X86_LOCAL_APIC)
>
>  static void kdump_nmi_callback(int cpu, struct pt_regs *regs)
>  {
>         crash_save_cpu(regs, cpu);
>
> -       /*
> -        * VMCLEAR VMCSs loaded on all cpus if needed.
> -        */
> -       cpu_crash_vmclear_loaded_vmcss();
> -
>         /*
>          * Disable Intel PT to stop its logging
>          */
> @@ -133,11 +107,6 @@ void native_machine_crash_shutdown(struct pt_regs *r=
egs)
>
>         crash_smp_send_stop();
>
> -       /*
> -        * VMCLEAR VMCSs loaded on this cpu if needed.
> -        */
> -       cpu_crash_vmclear_loaded_vmcss();
> -
>         cpu_emergency_disable_virtualization();
>
>         /*
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index 3adbe97015c13..3fa4c6717a1db 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -787,6 +787,26 @@ void machine_crash_shutdown(struct pt_regs *regs)
>  }
>  #endif
>
> +/*
> + * This is used to VMCLEAR all VMCSs loaded on the
> + * processor. And when loading kvm_intel module, the
> + * callback function pointer will be assigned.
> + *
> + * protected by rcu.
> + */
> +crash_vmclear_fn __rcu *crash_vmclear_loaded_vmcss;
> +EXPORT_SYMBOL_GPL(crash_vmclear_loaded_vmcss);
> +
> +static inline void cpu_crash_vmclear_loaded_vmcss(void)
> +{
> +       crash_vmclear_fn *do_vmclear_operation =3D NULL;
> +
> +       rcu_read_lock();
> +       do_vmclear_operation =3D rcu_dereference(crash_vmclear_loaded_vmc=
ss);
> +       if (do_vmclear_operation)
> +               do_vmclear_operation();
> +       rcu_read_unlock();
> +}
>
>  /* This is the CPU performing the emergency shutdown work. */
>  int crashing_cpu =3D -1;
> @@ -798,6 +818,8 @@ int crashing_cpu =3D -1;
>   */
>  void cpu_emergency_disable_virtualization(void)
>  {
> +       cpu_crash_vmclear_loaded_vmcss();
> +
>         cpu_emergency_vmxoff();
>         cpu_emergency_svm_disable();
>  }
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index df461f387e20d..f60fb79fea881 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -41,7 +41,7 @@
>  #include <asm/idtentry.h>
>  #include <asm/io.h>
>  #include <asm/irq_remapping.h>
> -#include <asm/kexec.h>
> +#include <asm/reboot.h>
>  #include <asm/perf_event.h>
>  #include <asm/mmu_context.h>
>  #include <asm/mshyperv.h>
> @@ -754,7 +754,6 @@ static int vmx_set_guest_uret_msr(struct vcpu_vmx *vm=
x,
>         return ret;
>  }
>
> -#ifdef CONFIG_KEXEC_CORE
>  static void crash_vmclear_local_loaded_vmcss(void)
>  {
>         int cpu =3D raw_smp_processor_id();
> @@ -764,7 +763,6 @@ static void crash_vmclear_local_loaded_vmcss(void)
>                             loaded_vmcss_on_cpu_link)
>                 vmcs_clear(v->vmcs);
>  }
> -#endif /* CONFIG_KEXEC_CORE */
>
>  static void __loaded_vmcs_clear(void *arg)
>  {
> @@ -8622,10 +8620,9 @@ static void __vmx_exit(void)
>  {
>         allow_smaller_maxphyaddr =3D false;
>
> -#ifdef CONFIG_KEXEC_CORE
>         RCU_INIT_POINTER(crash_vmclear_loaded_vmcss, NULL);
>         synchronize_rcu();
> -#endif
> +
>         vmx_cleanup_l1d_flush();
>  }
>
> @@ -8674,10 +8671,9 @@ static int __init vmx_init(void)
>                 pi_init_cpu(cpu);
>         }
>
> -#ifdef CONFIG_KEXEC_CORE
>         rcu_assign_pointer(crash_vmclear_loaded_vmcss,
>                            crash_vmclear_local_loaded_vmcss);
> -#endif
> +
>         vmx_check_vmcs12_offsets();
>
>         /*
> --
> 2.40.1
>