Skip to contentSkip to conversation

KVM: arm64: Restore the VM's feature bitmap when kvm_setup_vcpu() fails

PATCH Superseded Fuad Tabba 3 participants 7 messages Sep 21 – Sep 22

Subsystems (2)
arm64 KVM arm64

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Fuad Tabba

author reviewer
#0 lore raw

PATCH v2 1/1 KVM: arm64: Restore the VM's feature bitmap when kvm_setup_vcpu() fails

__kvm_vcpu_set_target() copies the requested features into the VM-wide bitmap before kvm_setup_vcpu() runs and doesn't undo it when setup fails, so a rejected KVM_ARM_VCPU_INIT leaves the VM recording features that were never set up. With HAS_EL2 | HAS_EL2_E2H0 on a host without FEAT_NV1, kvm_vcpu_init_nested() returns -EINVAL before it allocates any nested stage-2 MMU, and vcpu_has_nv() is then true with nested_mmus_size == 0; its -ENOMEM paths do the same on a VM's first INIT.

Nothing in the tree loads a vCPU whose init failed, so this is latent. The upcoming series that enables KVM_PRE_FAULT_MEMORY for arm64 exposes
it: the generic kvm_vcpu_pre_fault_memory() calls vcpu_load() whether or not the vCPU has been initialised. After the rejected INIT, the first call's vcpu_load() finds hw_mmu still set to the canonical MMU and leaves it alone, but its vcpu_put() takes the vcpu_has_nv() branch into kvm_vcpu_put_hw_mmu(), which clears hw_mmu. The second call's vcpu_load() finds hw_mmu NULL and takes the nested branch into get_s2_mmu_nested(), whose search over nested_mmus_size == 0 leaves s2_mmu NULL for the BUG_ON(atomic_read(&s2_mmu->refcnt)), under mmu_lock. On kvmarm/next with the series applied:

  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000074
  Call trace:
   kvm_vcpu_load_hw_mmu (arch/arm64/kvm/nested.c:891) (P)
   kvm_arch_vcpu_load (arch/arm64/kvm/arm.c:662)
   kvm_vcpu_pre_fault_memory (virt/kvm/kvm_main.c:170 virt/kvm/kvm_main.c:4349)
   kvm_vcpu_ioctl (virt/kvm/kvm_main.c:4639)

Setup reads the VM-wide bitmap, so the copy can't be deferred; restore the previous value instead when kvm_setup_vcpu() fails.

Fixes 427733579744e ("KVM: arm64: Select default PMU in KVM_ARM_VCPU_INIT handler")
Link https://lore.kernel.org/r/[email protected]/ open here
Reviewed-by Lorenzo Stoakes (ARM) <[email protected]>
Signed-off-by Fuad Tabba <[email protected]>

---
v2:
- Commit message: say what the prefault series enables, bring the two-ioctl walk and the trace up from below the fold, the trace decoded, and drop the Fixes: on 1de10b7d13a97, which had nothing fallible after the copy (Lorenzo).
- Fold Lorenzo's Reviewed-by.

Reproduced on kvmarm/next plus the series under QEMU (-cpu max with an Apple M2 MIDR, which has_nv1() denies; kvm-arm.mode=nested):
KVM_ARM_VCPU_INIT with HAS_EL2 | HAS_EL2_E2H0 returns -EINVAL, then KVM_PRE_FAULT_MEMORY twice. With the fix both calls return -ENOENT and the host is unaffected. Applies unchanged to v7.3-rc3.

v1: https://lore.kernel.org/r/[email protected]/

 arch/arm64/kvm/arm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
arch/arm64/kvm/arm.c +6 -1
index eaf583b771931..b25725f91c925 100644
@@ -1685,6 +1685,7 @@ static int kvm_setup_vcpu(struct kvm_vcpu *vcpu)
 static int __kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
 				 const struct kvm_vcpu_init *init)
 {
+	DECLARE_BITMAP(old_features, KVM_VCPU_MAX_FEATURES);
 	unsigned long features = init->features[0];
 	struct kvm *kvm = vcpu->kvm;
 	int ret = -EINVAL;

@@ -1695,11 +1696,15 @@ static int __kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
 	    kvm_vcpu_init_changed(vcpu, init))
 		goto out_unlock;
 
+	/* Setup reads the VM-wide bitmap, so undo the copy if setup fails. */
+	bitmap_copy(old_features, kvm->arch.vcpu_features, KVM_VCPU_MAX_FEATURES);
 	bitmap_copy(kvm->arch.vcpu_features, &features, KVM_VCPU_MAX_FEATURES);
 
 	ret = kvm_setup_vcpu(vcpu);
-	if (ret)
+	if (ret) {
+		bitmap_copy(kvm->arch.vcpu_features, old_features, KVM_VCPU_MAX_FEATURES);
 		goto out_unlock;
+	}
 
 	/* Now we know what it is, we can reset it. */
 	kvm_reset_vcpu(vcpu);

base-commit: 089e4f3c4862ba3f29dff2361caa8084879194fd

Signature · 1 line
2.39.5
maintainer
#1 lore raw

Hi Fuad,

Quoted 9 lines · Fuad Tabba

__kvm_vcpu_set_target() copies the requested features into the VM-wide bitmap before kvm_setup_vcpu() runs and doesn't undo it when setup fails, so a rejected KVM_ARM_VCPU_INIT leaves the VM recording features that were never set up. With HAS_EL2 | HAS_EL2_E2H0 on a host without FEAT_NV1, kvm_vcpu_init_nested() returns -EINVAL before it allocates any nested stage-2 MMU, and vcpu_has_nv() is then true with nested_mmus_size == 0; its -ENOMEM paths do the same on a VM's first INIT.

Quoted 4 lines

Nothing in the tree loads a vCPU whose init failed, so this is latent. The upcoming series that enables KVM_PRE_FAULT_MEMORY for arm64 exposes
it: the generic kvm_vcpu_pre_fault_memory() calls vcpu_load() whether or not the vCPU has been initialised.

That's a bug, not a feature. We should require an initialized vcpu for the ioctl.

Quoted 16 lines
arch/arm64/kvm/arm.c +1 -0
index eaf583b771931..b25725f91c925 100644
@@ -1685,6 +1685,7 @@ static int kvm_setup_vcpu(struct kvm_vcpu *vcpu)
 static int __kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
 				 const struct kvm_vcpu_init *init)
 {
+	DECLARE_BITMAP(old_features, KVM_VCPU_MAX_FEATURES);
 	unsigned long features = init->features[0];
 	struct kvm *kvm = vcpu->kvm;
 	int ret = -EINVAL;

@@ -1695,11 +1696,15 @@ static int __kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
 	    kvm_vcpu_init_changed(vcpu, init))
 		goto out_unlock;

arch/arm64/kvm/arm.c static int __kvm_vcpu_set_target(struct kvm_vcpu *vcpu, · line 1707

Quoted 10 lines
arch/arm64/kvm/arm.c +5 -1
+	/* Setup reads the VM-wide bitmap, so undo the copy if setup fails. */
+	bitmap_copy(old_features, kvm->arch.vcpu_features, KVM_VCPU_MAX_FEATURES);
 	bitmap_copy(kvm->arch.vcpu_features, &features, KVM_VCPU_MAX_FEATURES);
 
 	ret = kvm_setup_vcpu(vcpu);
-	if (ret)
+	if (ret) {
+		bitmap_copy(kvm->arch.vcpu_features, old_features, KVM_VCPU_MAX_FEATURES);
 		goto out_unlock;
+	}

The bitmap copy is a bit confusing because there's only two possible situations:

- KVM_ARCH_FLAG_VCPU_FEATURES_CONFIGURED is unset and the bitmap was previously zero

- KVM_ARCH_FLAG_VCPU_FEATURES_CONFIGURED is set and @init->features is identical to vcpu_features (see kvm_vcpu_init_changed())

While there's nothing wrong with your diff, I'd prefer if the above detail was represented directly.

	ret = kvm_setup_vcpu(vcpu);
	if (ret) {
		/*
		 * Clear the bitmap if setup fails on the first vCPU to be
		 * initialized.
		 */
		if (!test_bit(KVM_ARCH_FLAG_VCPU_FEATURES_CONFIGURED, &kvm->arch.flags))
			bitmap_zero(kvm->arch.vcpu_features, KVM_VCPU_MAX_FEATURES);

		goto out_unlock;
	}

Thanks,
Oliver

author reviewer
#2 lore raw

Hi Oliver,

On Mon, 21 Sep 2026 00:09:17 -0700, Oliver Upton <[email protected]> wrote:
[...]

Quoted 7 lines
Quoted 4 lines

Nothing in the tree loads a vCPU whose init failed, so this is latent. The upcoming series that enables KVM_PRE_FAULT_MEMORY for arm64 exposes
it: the generic kvm_vcpu_pre_fault_memory() calls vcpu_load() whether or not the vCPU has been initialised.

That's a bug, not a feature. We should require an initialized vcpu for the ioctl.

That one is for Lorenzo then :) I'll reword the message so the stale bitmap comes first and the pre-fault crash is just how it showed up.

[...]

Quoted 12 lines

The bitmap copy is a bit confusing because there's only two possible situations:

- KVM_ARCH_FLAG_VCPU_FEATURES_CONFIGURED is unset and the bitmap was previously zero

- KVM_ARCH_FLAG_VCPU_FEATURES_CONFIGURED is set and @init->features is identical to vcpu_features (see kvm_vcpu_init_changed())

While there's nothing wrong with your diff, I'd prefer if the above detail was represented directly.

Quoted 11 lines
        ret = kvm_setup_vcpu(vcpu);
        if (ret) {
                /*
                 * Clear the bitmap if setup fails on the first vCPU to be
                 * initialized.
                 */
                if (!test_bit(KVM_ARCH_FLAG_VCPU_FEATURES_CONFIGURED, &kvm->arch.flags))
                        bitmap_zero(kvm->arch.vcpu_features, KVM_VCPU_MAX_FEATURES);

                goto out_unlock;
        }

I'd gone for the restore so the failure path didn't depend on the flag, but those two states are all there is and your version makes that explicit. I'll use it in v3.

Thanks,
/fuad

#3 lore raw
Quoted 9 lines · Fuad Tabba

Hi Oliver,

On Mon, 21 Sep 2026 00:09:17 -0700, Oliver Upton <[email protected]> wrote:
[...]

Quoted 5 lines
Quoted 4 lines

Nothing in the tree loads a vCPU whose init failed, so this is latent. The upcoming series that enables KVM_PRE_FAULT_MEMORY for arm64 exposes
it: the generic kvm_vcpu_pre_fault_memory() calls vcpu_load() whether or not the vCPU has been initialised.

Quoted 5 lines
Quoted 2 lines

That's a bug, not a feature. We should require an initialized vcpu for the ioctl.

That one is for Lorenzo then :) I'll reword the message so the stale bitmap comes first and the pre-fault crash is just how it showed up.

Let me dig into this.

--
Cheers, Lorenzo

#4 lore raw
In reply to Fuad Tabba
Quoted 9 lines · Fuad Tabba

Hi Oliver,

On Mon, 21 Sep 2026 00:09:17 -0700, Oliver Upton <[email protected]> wrote:
[...]

Quoted 5 lines
Quoted 4 lines

Nothing in the tree loads a vCPU whose init failed, so this is latent. The upcoming series that enables KVM_PRE_FAULT_MEMORY for arm64 exposes
it: the generic kvm_vcpu_pre_fault_memory() calls vcpu_load() whether or not the vCPU has been initialised.

Quoted 5 lines
Quoted 2 lines

That's a bug, not a feature. We should require an initialized vcpu for the ioctl.

That one is for Lorenzo then :) I'll reword the message so the stale bitmap comes first and the pre-fault crash is just how it showed up.

In general this is the behaviour of _generic_ KVM code (kvm_vcpu_pre_fault_memory()), which itself does not require an initialised vCPU, and no other arch seems to requires that either in the arch-specific pre-fault code.

The actual pre-faulting implementation intentionally only targets the canonical MMU to fault in S2 page tables, which doesn't need initialisation to have taken place AFAICT.

However I could add:

	if (!kvm_vcpu_initialized(vcpu))
		return -ENOEXEC;

To kvm_arch_vcpu_pre_fault_memory(), which would make this a requirement, though that would bring us out of line with other archs.

OTOH, I don't see why anybody would pre-fault BEFORE init, feels like something that should be done later on in VMM bring up right?

So I doubt doing this would really break any sane user.

Let me know if you want that change in the series and I can respin it.

--
Cheers, Lorenzo

maintainer
#5 lore raw

Hey Lorenzo,

Quoted 25 lines · Lorenzo Stoakes (ARM)
Quoted 9 lines · Fuad Tabba

Hi Oliver,

On Mon, 21 Sep 2026 00:09:17 -0700, Oliver Upton <[email protected]> wrote:
[...]

Quoted 5 lines
Quoted 4 lines

Nothing in the tree loads a vCPU whose init failed, so this is latent. The upcoming series that enables KVM_PRE_FAULT_MEMORY for arm64 exposes
it: the generic kvm_vcpu_pre_fault_memory() calls vcpu_load() whether or not the vCPU has been initialised.

Quoted 5 lines
Quoted 2 lines

That's a bug, not a feature. We should require an initialized vcpu for the ioctl.

That one is for Lorenzo then :) I'll reword the message so the stale bitmap comes first and the pre-fault crash is just how it showed up.

In general this is the behaviour of _generic_ KVM code (kvm_vcpu_pre_fault_memory()), which itself does not require an initialised vCPU, and no other arch seems to requires that either in the arch-specific pre-fault code.

The actual pre-faulting implementation intentionally only targets the canonical MMU to fault in S2 page tables, which doesn't need initialisation to have taken place AFAICT.

Quoted 7 lines

However I could add:

	if (!kvm_vcpu_initialized(vcpu))
		return -ENOEXEC;

To kvm_arch_vcpu_pre_fault_memory(), which would make this a requirement, though that would bring us out of line with other archs.

Actually, we need this enforcement to happen before the arch-generic implementation calls vcpu_load(). So that would be some amount of kvm_arch_* boilerplate added to the generic code to enforce this requirement like we do on the other vCPU ioctls.

I'm not too worried about aligning this with other architectures; the idea of a half-baked vCPU getting loaded is worrying.

Quoted 6 lines

OTOH, I don't see why anybody would pre-fault BEFORE init, feels like something that should be done later on in VMM bring up right?

So I doubt doing this would really break any sane user.

Let me know if you want that change in the series and I can respin it.

Please do, thank you for addressing this.

Best,
Oliver

#6 lore raw
Quoted 33 lines · Oliver Upton

Hey Lorenzo,

Quoted 30 lines · Lorenzo Stoakes (ARM)
Quoted 9 lines · Fuad Tabba

Hi Oliver,

On Mon, 21 Sep 2026 00:09:17 -0700, Oliver Upton <[email protected]> wrote:
[...]

Quoted 5 lines
Quoted 4 lines

Nothing in the tree loads a vCPU whose init failed, so this is latent. The upcoming series that enables KVM_PRE_FAULT_MEMORY for arm64 exposes
it: the generic kvm_vcpu_pre_fault_memory() calls vcpu_load() whether or not the vCPU has been initialised.

Quoted 5 lines
Quoted 2 lines

That's a bug, not a feature. We should require an initialized vcpu for the ioctl.

That one is for Lorenzo then :) I'll reword the message so the stale bitmap comes first and the pre-fault crash is just how it showed up.

In general this is the behaviour of _generic_ KVM code (kvm_vcpu_pre_fault_memory()), which itself does not require an initialised vCPU, and no other arch seems to requires that either in the arch-specific pre-fault code.

The actual pre-faulting implementation intentionally only targets the canonical MMU to fault in S2 page tables, which doesn't need initialisation to have taken place AFAICT.

However I could add:

	if (!kvm_vcpu_initialized(vcpu))
		return -ENOEXEC;
Quoted 7 lines
Quoted 2 lines

To kvm_arch_vcpu_pre_fault_memory(), which would make this a requirement, though that would bring us out of line with other archs.

Actually, we need this enforcement to happen before the arch-generic implementation calls vcpu_load(). So that would be some amount of kvm_arch_* boilerplate added to the generic code to enforce this requirement like we do on the other vCPU ioctls.

Ack, will do!

Quoted 3 lines

I'm not too worried about aligning this with other architectures; the idea of a half-baked vCPU getting loaded is worrying.

Yeah and I don't think anybody in their right mind is relying on being able to pre-fault before init anyway.

Quoted 9 lines
Quoted 7 lines

OTOH, I don't see why anybody would pre-fault BEFORE init, feels like something that should be done later on in VMM bring up right?

So I doubt doing this would really break any sane user.

Let me know if you want that change in the series and I can respin it.

Please do, thank you for addressing this.

No worries, will put a commit or 2 at the start of the series to action this and respin.

Quoted 3 lines

Best,
Oliver

--
Cheers, Lorenzo