[PATCH] KVM: arm64: Disable preemption in kvm_arch_hardware_enable()

[PATCH] KVM: arm64: Disable preemption in kvm_arch_hardware_enable()

[Marc Zyngier]

Since 0bf50497f03b ("KVM: Drop kvm_count_lock and instead protect
kvm_usage_count with kvm_lock"), hotplugging back a CPU whilst
a guest is running results in a number of ugly splats as most
of this code expects to run with preemption disabled, which isn't
the case anymore.

While the context is preemptable, it isn't migratable, which should
be enough. But we have plenty of preemptible() checks all over
the place, and our per-CPU accessors also disable preemption.

Since this affects released versions, let's do the easy fix first,
disabling preemption in kvm_arch_hardware_enable(). We can always
revisit this with a more invasive fix in the future.

Fixes: 0bf50497f03b ("KVM: Drop kvm_count_lock and instead protect kvm_usage_count with kvm_lock")
Reported-by: Kristina Martsenko <kristina.martsenko@arm.com>
Tested-by: Kristina Martsenko <kristina.martsenko@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/aeab7562-2d39-e78e-93b1-4711f8cc3fa5@arm.com
Cc: stable@vger.kernek.org # v6.3, v6.4
---
 arch/arm64/kvm/arm.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index aaeae1145359..a28c4ffe4932 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -1894,8 +1894,17 @@ static void _kvm_arch_hardware_enable(void *discard)
 
 int kvm_arch_hardware_enable(void)
 {
-	int was_enabled = __this_cpu_read(kvm_arm_hardware_enabled);
+	int was_enabled;
 
+	/*
+	 * Most calls to this function are made with migration
+	 * disabled, but not with preemption disabled. The former is
+	 * enough to ensure correctness, but most of the helpers
+	 * expect the later and will throw a tantrum otherwise.
+	 */
+	preempt_disable();
+
+	was_enabled = __this_cpu_read(kvm_arm_hardware_enabled);
 	_kvm_arch_hardware_enable(NULL);
 
 	if (!was_enabled) {
@@ -1903,6 +1912,8 @@ int kvm_arch_hardware_enable(void)
 		kvm_timer_cpu_up();
 	}
 
+	preempt_enable();
+
 	return 0;
 }
 
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Disable preemption in kvm_arch_hardware_enable()

[Kristina Martsenko]

On 03/07/2023 17:35, Marc Zyngier wrote:
> Since 0bf50497f03b ("KVM: Drop kvm_count_lock and instead protect
> kvm_usage_count with kvm_lock"), hotplugging back a CPU whilst
> a guest is running results in a number of ugly splats as most
> of this code expects to run with preemption disabled, which isn't
> the case anymore.
> 
> While the context is preemptable, it isn't migratable, which should
> be enough. But we have plenty of preemptible() checks all over
> the place, and our per-CPU accessors also disable preemption.
> 
> Since this affects released versions, let's do the easy fix first,
> disabling preemption in kvm_arch_hardware_enable(). We can always
> revisit this with a more invasive fix in the future.
> 
> Fixes: 0bf50497f03b ("KVM: Drop kvm_count_lock and instead protect kvm_usage_count with kvm_lock")
> Reported-by: Kristina Martsenko <kristina.martsenko@arm.com>
> Tested-by: Kristina Martsenko <kristina.martsenko@arm.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Link: https://lore.kernel.org/r/aeab7562-2d39-e78e-93b1-4711f8cc3fa5@arm.com
> Cc: stable@vger.kernek.org # v6.3, v6.4

Typo here, didn't make it to the stable list (kernek.org -> kernel.org)

Kristina


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Disable preemption in kvm_arch_hardware_enable()

[Oliver Upton]

On Tue, Jul 04, 2023 at 07:32:09PM +0100, Kristina Martsenko wrote:
> On 03/07/2023 17:35, Marc Zyngier wrote:
> > Since 0bf50497f03b ("KVM: Drop kvm_count_lock and instead protect
> > kvm_usage_count with kvm_lock"), hotplugging back a CPU whilst
> > a guest is running results in a number of ugly splats as most
> > of this code expects to run with preemption disabled, which isn't
> > the case anymore.
> > 
> > While the context is preemptable, it isn't migratable, which should
> > be enough. But we have plenty of preemptible() checks all over
> > the place, and our per-CPU accessors also disable preemption.
> > 
> > Since this affects released versions, let's do the easy fix first,
> > disabling preemption in kvm_arch_hardware_enable(). We can always
> > revisit this with a more invasive fix in the future.
> > 
> > Fixes: 0bf50497f03b ("KVM: Drop kvm_count_lock and instead protect kvm_usage_count with kvm_lock")
> > Reported-by: Kristina Martsenko <kristina.martsenko@arm.com>
> > Tested-by: Kristina Martsenko <kristina.martsenko@arm.com>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > Link: https://lore.kernel.org/r/aeab7562-2d39-e78e-93b1-4711f8cc3fa5@arm.com
> > Cc: stable@vger.kernek.org # v6.3, v6.4
> 
> Typo here, didn't make it to the stable list (kernek.org -> kernel.org)

Thanks for catching this Kristina, I'll fix it up when I apply the
patch. So long as the patch appears in Linus' tree with the correct Cc
it'll find its way to the stable trees.

--
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Disable preemption in kvm_arch_hardware_enable()

[Marc Zyngier]

On Tue, 04 Jul 2023 19:32:09 +0100,
Kristina Martsenko <kristina.martsenko@arm.com> wrote:
> 
> On 03/07/2023 17:35, Marc Zyngier wrote:
> > Since 0bf50497f03b ("KVM: Drop kvm_count_lock and instead protect
> > kvm_usage_count with kvm_lock"), hotplugging back a CPU whilst
> > a guest is running results in a number of ugly splats as most
> > of this code expects to run with preemption disabled, which isn't
> > the case anymore.
> > 
> > While the context is preemptable, it isn't migratable, which should
> > be enough. But we have plenty of preemptible() checks all over
> > the place, and our per-CPU accessors also disable preemption.
> > 
> > Since this affects released versions, let's do the easy fix first,
> > disabling preemption in kvm_arch_hardware_enable(). We can always
> > revisit this with a more invasive fix in the future.
> > 
> > Fixes: 0bf50497f03b ("KVM: Drop kvm_count_lock and instead protect kvm_usage_count with kvm_lock")
> > Reported-by: Kristina Martsenko <kristina.martsenko@arm.com>
> > Tested-by: Kristina Martsenko <kristina.martsenko@arm.com>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > Link: https://lore.kernel.org/r/aeab7562-2d39-e78e-93b1-4711f8cc3fa5@arm.com
> > Cc: stable@vger.kernek.org # v6.3, v6.4
> 
> Typo here, didn't make it to the stable list (kernek.org -> kernel.org)

Yeah, I had a bad day (I also typoed Catalin's address in a separate
email...).

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Disable preemption in kvm_arch_hardware_enable()

[Sean Christopherson]

On Mon, Jul 03, 2023, Marc Zyngier wrote:
> Since 0bf50497f03b ("KVM: Drop kvm_count_lock and instead protect
> kvm_usage_count with kvm_lock"), hotplugging back a CPU whilst
> a guest is running results in a number of ugly splats as most
> of this code expects to run with preemption disabled, which isn't
> the case anymore.
> 
> While the context is preemptable, it isn't migratable, which should
> be enough. But we have plenty of preemptible() checks all over
> the place, and our per-CPU accessors also disable preemption.
> 
> Since this affects released versions, let's do the easy fix first,
> disabling preemption in kvm_arch_hardware_enable(). We can always
> revisit this with a more invasive fix in the future.
> 
> Fixes: 0bf50497f03b ("KVM: Drop kvm_count_lock and instead protect kvm_usage_count with kvm_lock")
> Reported-by: Kristina Martsenko <kristina.martsenko@arm.com>
> Tested-by: Kristina Martsenko <kristina.martsenko@arm.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Link: https://lore.kernel.org/r/aeab7562-2d39-e78e-93b1-4711f8cc3fa5@arm.com
> Cc: stable@vger.kernek.org # v6.3, v6.4
> ---
>  arch/arm64/kvm/arm.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index aaeae1145359..a28c4ffe4932 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1894,8 +1894,17 @@ static void _kvm_arch_hardware_enable(void *discard)
>  
>  int kvm_arch_hardware_enable(void)
>  {
> -	int was_enabled = __this_cpu_read(kvm_arm_hardware_enabled);
> +	int was_enabled;
>  
> +	/*
> +	 * Most calls to this function are made with migration
> +	 * disabled, but not with preemption disabled. The former is
> +	 * enough to ensure correctness, but most of the helpers
> +	 * expect the later and will throw a tantrum otherwise.
> +	 */
> +	preempt_disable();
> +
> +	was_enabled = __this_cpu_read(kvm_arm_hardware_enabled);

IMO, this_cpu_has_cap() is at fault.  E.g. why not do this?

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 7d7128c65161..b862477de2ce 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -3193,7 +3193,9 @@ static void __init setup_boot_cpu_capabilities(void)
 
 bool this_cpu_has_cap(unsigned int n)
 {
-       if (!WARN_ON(preemptible()) && n < ARM64_NCAPS) {
+       __this_cpu_preempt_check("has_cap");
+
+       if (n < ARM64_NCAPS) {
                const struct arm64_cpu_capabilities *cap = cpu_hwcaps_ptrs[n];
 
                if (cap)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Disable preemption in kvm_arch_hardware_enable()

[Marc Zyngier]

On Mon, 10 Jul 2023 19:04:08 +0100,
Sean Christopherson <seanjc@google.com> wrote:
> 
> On Mon, Jul 03, 2023, Marc Zyngier wrote:
> > Since 0bf50497f03b ("KVM: Drop kvm_count_lock and instead protect
> > kvm_usage_count with kvm_lock"), hotplugging back a CPU whilst
> > a guest is running results in a number of ugly splats as most
> > of this code expects to run with preemption disabled, which isn't
> > the case anymore.
> > 
> > While the context is preemptable, it isn't migratable, which should
> > be enough. But we have plenty of preemptible() checks all over
> > the place, and our per-CPU accessors also disable preemption.
> > 
> > Since this affects released versions, let's do the easy fix first,
> > disabling preemption in kvm_arch_hardware_enable(). We can always
> > revisit this with a more invasive fix in the future.
> > 
> > Fixes: 0bf50497f03b ("KVM: Drop kvm_count_lock and instead protect kvm_usage_count with kvm_lock")
> > Reported-by: Kristina Martsenko <kristina.martsenko@arm.com>
> > Tested-by: Kristina Martsenko <kristina.martsenko@arm.com>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > Link: https://lore.kernel.org/r/aeab7562-2d39-e78e-93b1-4711f8cc3fa5@arm.com
> > Cc: stable@vger.kernek.org # v6.3, v6.4
> > ---
> >  arch/arm64/kvm/arm.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index aaeae1145359..a28c4ffe4932 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -1894,8 +1894,17 @@ static void _kvm_arch_hardware_enable(void *discard)
> >  
> >  int kvm_arch_hardware_enable(void)
> >  {
> > -	int was_enabled = __this_cpu_read(kvm_arm_hardware_enabled);
> > +	int was_enabled;
> >  
> > +	/*
> > +	 * Most calls to this function are made with migration
> > +	 * disabled, but not with preemption disabled. The former is
> > +	 * enough to ensure correctness, but most of the helpers
> > +	 * expect the later and will throw a tantrum otherwise.
> > +	 */
> > +	preempt_disable();
> > +
> > +	was_enabled = __this_cpu_read(kvm_arm_hardware_enabled);
> 
> IMO, this_cpu_has_cap() is at fault.  E.g. why not do this?
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 7d7128c65161..b862477de2ce 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -3193,7 +3193,9 @@ static void __init setup_boot_cpu_capabilities(void)
>  
>  bool this_cpu_has_cap(unsigned int n)
>  {
> -       if (!WARN_ON(preemptible()) && n < ARM64_NCAPS) {
> +       __this_cpu_preempt_check("has_cap");
> +
> +       if (n < ARM64_NCAPS) {
>                 const struct arm64_cpu_capabilities *cap = cpu_hwcaps_ptrs[n];
>  
>                 if (cap)
> 

Because this check is not on at all times (it relies on
DEBUG_PREEMPT), and we really want it to be there.

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Disable preemption in kvm_arch_hardware_enable()

[Oliver Upton]

On Mon, Jul 10, 2023 at 11:04:08AM -0700, Sean Christopherson wrote:
> On Mon, Jul 03, 2023, Marc Zyngier wrote:
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index aaeae1145359..a28c4ffe4932 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -1894,8 +1894,17 @@ static void _kvm_arch_hardware_enable(void *discard)
> >  
> >  int kvm_arch_hardware_enable(void)
> >  {
> > -	int was_enabled = __this_cpu_read(kvm_arm_hardware_enabled);
> > +	int was_enabled;
> >  
> > +	/*
> > +	 * Most calls to this function are made with migration
> > +	 * disabled, but not with preemption disabled. The former is
> > +	 * enough to ensure correctness, but most of the helpers
> > +	 * expect the later and will throw a tantrum otherwise.
> > +	 */
> > +	preempt_disable();
> > +
> > +	was_enabled = __this_cpu_read(kvm_arm_hardware_enabled);
> 
> IMO, this_cpu_has_cap() is at fault.

Who ever said otherwise?

> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 7d7128c65161..b862477de2ce 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -3193,7 +3193,9 @@ static void __init setup_boot_cpu_capabilities(void)
>  
>  bool this_cpu_has_cap(unsigned int n)
>  {
> -       if (!WARN_ON(preemptible()) && n < ARM64_NCAPS) {
> +       __this_cpu_preempt_check("has_cap");
> +
> +       if (n < ARM64_NCAPS) {

This is likely sufficient, but to Marc's point we have !preemptible()
checks littered about, it just so happens that this_cpu_has_cap() is the
first to get called. We need to make sure there aren't any other checks
that'd break under hotplug.

While I'd normally like to see the 'right' fix fully fleshed out for
something like this, the bug is ugly enough where I'd rather take a hack
for the time being.

-- 
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Disable preemption in kvm_arch_hardware_enable()

[Sean Christopherson]

On Mon, Jul 10, 2023, Oliver Upton wrote:
> On Mon, Jul 10, 2023 at 11:04:08AM -0700, Sean Christopherson wrote:
> > On Mon, Jul 03, 2023, Marc Zyngier wrote:
> > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > > index aaeae1145359..a28c4ffe4932 100644
> > > --- a/arch/arm64/kvm/arm.c
> > > +++ b/arch/arm64/kvm/arm.c
> > > @@ -1894,8 +1894,17 @@ static void _kvm_arch_hardware_enable(void *discard)
> > >  
> > >  int kvm_arch_hardware_enable(void)
> > >  {
> > > -	int was_enabled = __this_cpu_read(kvm_arm_hardware_enabled);
> > > +	int was_enabled;
> > >  
> > > +	/*
> > > +	 * Most calls to this function are made with migration
> > > +	 * disabled, but not with preemption disabled. The former is
> > > +	 * enough to ensure correctness, but most of the helpers
> > > +	 * expect the later and will throw a tantrum otherwise.
> > > +	 */
> > > +	preempt_disable();
> > > +
> > > +	was_enabled = __this_cpu_read(kvm_arm_hardware_enabled);
> > 
> > IMO, this_cpu_has_cap() is at fault.
> 
> Who ever said otherwise?

Sorry, should have phrased that as "should be fixed".
 
> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > index 7d7128c65161..b862477de2ce 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -3193,7 +3193,9 @@ static void __init setup_boot_cpu_capabilities(void)
> >  
> >  bool this_cpu_has_cap(unsigned int n)
> >  {
> > -       if (!WARN_ON(preemptible()) && n < ARM64_NCAPS) {
> > +       __this_cpu_preempt_check("has_cap");
> > +
> > +       if (n < ARM64_NCAPS) {
> 
> This is likely sufficient, but to Marc's point we have !preemptible()
> checks littered about, it just so happens that this_cpu_has_cap() is the
> first to get called. We need to make sure there aren't any other checks
> that'd break under hotplug.

Ah, "we" is all of arm64.  E.g. this pattern in arch/arm64/kernel/cpu_errata.c
is splattered all over the place.

  WARN_ON(scope != SCOPE_LOCAL_CPU || preemptible());

AFAICT, the only issue in KVM proper is the BUG_ON() in kvm_vgic_init_cpu_hardware().

Sadly, a hack-a-fix for stable probably does make sense :-(

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Disable preemption in kvm_arch_hardware_enable()

[Oliver Upton]

On Mon, 3 Jul 2023 17:35:48 +0100, Marc Zyngier wrote:
> Since 0bf50497f03b ("KVM: Drop kvm_count_lock and instead protect
> kvm_usage_count with kvm_lock"), hotplugging back a CPU whilst
> a guest is running results in a number of ugly splats as most
> of this code expects to run with preemption disabled, which isn't
> the case anymore.
> 
> While the context is preemptable, it isn't migratable, which should
> be enough. But we have plenty of preemptible() checks all over
> the place, and our per-CPU accessors also disable preemption.
> 
> [...]

Applied to kvmarm/fixes, thanks!

[1/1] KVM: arm64: Disable preemption in kvm_arch_hardware_enable()
      https://git.kernel.org/kvmarm/kvmarm/c/970dee09b230

--
Best,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel