RE: [PATCH] x86/hyperv: Fix kexec panic/hang issues

[Michael Kelley]

From: Dexuan Cui <decui@xxxxxxxxxxxxx>  Sent: Monday, December 21, 2020 5:04 PM
> 
> > From: Michael Kelley
> > Sent: Monday, December 21, 2020 3:33 PM
> > From: Dexuan Cui
> > Sent: Sunday, December 20, 2020 2:30 PM
> > >
> > > Currently the kexec kernel can panic or hang due to 2 causes:
> > >
> > > 1) hv_cpu_die() is not called upon kexec, so the hypervisor corrupts the
> > > VP Assist Pages when the kexec kernel runs. We ever fixed the same issue
> >
> > Spurious word "ever".  And avoid first person "we".  Perhaps:
> >
> > The same issue is fixed for hibernation in commit ..... .  Now fix it for
> > kexec.
> 
> Thanks! Will use this in v2.
> 
> > > for hibernation in
> > > commit 421f090c819d ("x86/hyperv: Suspend/resume the VP assist page for
> > hibernation")
> > > and now let's fix it for kexec.
> >
> > Is the VP Assist Page getting cleared anywhere on the panic path?  We can
> 
> It's not.
> 
> > only do it for the CPU that runs panic(), but I don't think it is getting cleared
> > even for that CPU.   It is cleared only in hv_cpu_die(), and that's not called on
> > the panic path.
> 
> IMO kdump is different from the non-kdump kexec in that the kdump kernel
> runs without depending on the memory used by the first kernel, so it looks
> unnecessary to clear the first kernel's VP Assist Page (and the hypercallpage).
> According to my test, the second kernel can re-enble the VP Asist Page and
> the hypercall page using different GPAs, without disabling the old pages first.

Ah yes.  You are right.  The kdump kernel must be using a disjoint area of
physical memory,  so not clearing these per-CPU overlay pages shouldn't
put the kdump kernel at risk.

> Of course, in the future Hyper-V may require the guest to disable the pages first
> before trying to re-enabling them, so I agree we'd better clear the pages in the
> first kernell like this:
> 
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 4638a52d8eae..8022f51c9c05 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -202,7 +202,7 @@ void clear_hv_tscchange_cb(void)
>  }
>  EXPORT_SYMBOL_GPL(clear_hv_tscchange_cb);
> 
> -static int hv_cpu_die(unsigned int cpu)
> +int hv_cpu_die(unsigned int cpu)
>  {
>         struct hv_reenlightenment_control re_ctrl;
>         unsigned int new_cpu;
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 30f76b966857..d090e781d216 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -76,6 +76,8 @@ static inline void hv_disable_stimer0_percpu_irq(int irq) {}
>  #if IS_ENABLED(CONFIG_HYPERV)
>  extern int hyperv_init_cpuhp;
> 
> +int hv_cpu_die(unsigned int cpu);
> +
>  extern void *hv_hypercall_pg;
>  extern void  __percpu  **hyperv_pcpu_input_arg;
> 
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 43b54bef5448..e54f8262bfe0 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -156,6 +156,9 @@ static void hv_machine_crash_shutdown(struct pt_regs *regs)
>         if (hv_crash_handler)
>                 hv_crash_handler(regs);
> 
> +       /* Only call this on the faulting CPU. */
> +       hv_cpu_die(raw_smp_processor_id());
> +
>         /* The function calls crash_smp_send_stop(). */
>         native_machine_crash_shutdown(regs);

Since we don't *need* to do this, I think it might be less risky to just leave
the code "as is".   But I'm OK either way.

> 
> > > 2) hyperv_cleanup() is called too early. In the kexec path, the other CPUs
> > > are stopped in hv_machine_shutdown() -> native_machine_shutdown(), so
> > > between hv_kexec_handler() and native_machine_shutdown(), the other
> > CPUs
> > > can still try to access the hypercall page and cause panic. The workaround
> > > "hv_hypercall_pg = NULL;" in hyperv_cleanup() can't work reliably.
> >
> > I would note that the comment in hv_suspend() is also incorrect on this
> > point.  Setting hv_hypercall_pg to NULL does not cause subsequent
> > hypercalls to fail safely.  The fast hypercalls don't test for it, and even if they
> > did test like hv_do_hypercall(), the test just creates a race condition.
> 
> The comment in hv_suspend() should be correct because hv_suspend()
> is only called during hibernation from the syscore_ops path where only
> one CPU is active, e.g. for the suspend operation, it's called from
> state_store
>   hibernate
>     hibernation_snapshot
>       create_image
>         suspend_disable_secondary_cpus
>         syscore_suspend
>           hv_suspend
> 
> It's similar for the resume operation:
> resume_store
>   software_resume
>     load_image_and_restore
>       hibernation_restore
>         resume_target_kernel
>           hibernate_resume_nonboot_cpu_disable
>           syscore_suspend
>             hv_suspend

I agree the hv_suspend() code is correct.  I read the second sentence of
the comment as being a more general statement that hypercalls could be
cleanly stopped by setting hv_hypercall_pg to NULL, which isn't true.

> 
> > >  static void hv_machine_crash_shutdown(struct pt_regs *regs)
> > >  {
> > >  	if (hv_crash_handler)
> > >  		hv_crash_handler(regs);
> > > +
> > > +	/* The function calls crash_smp_send_stop(). */
> >
> > Actually, crash_smp_send_stop() or smp_send_stop() has already been
> > called earlier by panic(),
> 
> This is true only when the Hyper-V host supports the feature
> HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE. On an old Hyper-V host
> without the feature, ms_hyperv_init_platform() doesn't set
> crash_kexec_post_notifiers, so crash_kexec_post_notifiers keeps its
> initial value "false", and panic() calls smp_send_stop() *after*
> __crash_kexec() (which calls machine_crash_shutdown() ->
> hv_machine_crash_shutdown()).

OK, I see your point.

> 
> >  so there's already only a single CPU running at
> > this point.  crash_smp_send_stop() is called again in
> > native_machine_crash_shutdown(), but it has a flag to prevent it from
> > running again.
> >
> > >  	native_machine_crash_shutdown(regs);
> > > +
> > > +	/* Disable the hypercall page when there is only 1 active CPU. */
> > > +	hyperv_cleanup();
> >
> > Moving the call to hyperv_cleanup() in the panic path is OK, and it makes
> > the panic and kexec() paths more similar, but I don't think it is necessary.
> > As noted above, the other CPUs have already been stopped, so they shouldn't
> > be executing any hypercalls.
> 
> As I explained above, it's necessary for old Hyper-V hosts. :-)

Agreed.