Re: [PATCH v3] KVM: arm64: Clear the VM's feature bitmap when kvm_setup_vcpu() fails

[Lorenzo Stoakes (ARM)]

On Mon, Sep 21, 2026 at 08:08:43PM +0100, Fuad Tabba wrote:
> __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.
>
> Until one INIT has succeeded, every INIT that passes the feature check
> rewrites the bitmap and a failure leaves it there; once
> KVM_ARCH_FLAG_VCPU_FEATURES_CONFIGURED is set, kvm_vcpu_init_changed()
> has already required the requested features to equal the bitmap, so
> the copy is a no-op. Clear the bitmap on failure while the flag is
> clear.
>
> This showed up with the series that enables KVM_PRE_FAULT_MEMORY for
> arm64, since the generic kvm_vcpu_pre_fault_memory() calls vcpu_load()
> without checking that the vCPU has been initialised. After the rejected
> INIT, the first call's vcpu_load() reads 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() reads hw_mmu as 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:

This is no longer accurate at the latest revision of my series ([0])but maybe worth
keeping for historic detail?

[0]: https://lore.kernel.org/kvmarm/20260922-kvm-arm-prefault-v3-0-787bd3bc7e3f@xxxxxxxxxx/

>
>   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)
>
> Fixes: 427733579744e ("KVM: arm64: Select default PMU in KVM_ARM_VCPU_INIT handler")
> Suggested-by: Oliver Upton <oupton@xxxxxxxxxx>
> Link: https://lore.kernel.org/r/20260825-kvm-arm-prefault-v1-0-befe8947702e@xxxxxxxxxx/
> Signed-off-by: Fuad Tabba <fuad.tabba@xxxxxxxxx>

Seems reasonable to me so:

Reviewed-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>

> ---
> v3:
>  - Clear the bitmap on failure instead of saving and restoring it, only
>    while KVM_ARCH_FLAG_VCPU_FEATURES_CONFIGURED is clear, since with the
>    flag set the copy was a no-op (Oliver).
>  - Commit message: drop the "latent, exposed by the series" framing and
>    state the flag invariant behind the clear; the pre-fault crash is
>    how the bug showed up.
>  - Dropped Lorenzo's Reviewed-by since the code changed.
>
> 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. Based on kvmarm/fixes; applies cleanly to
> v7.3-rc4 and to kvmarm/next.
>
> v2: https://lore.kernel.org/r/20260921063718.1604533-1-fuad.tabba@xxxxxxxxx/
> v1: https://lore.kernel.org/r/20260918120553.163139-1-fuad.tabba@xxxxxxxxx/
>
>  arch/arm64/kvm/arm.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index eaf583b771931..bd19f64ae82ef 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1698,8 +1698,16 @@ static int __kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
>  	bitmap_copy(kvm->arch.vcpu_features, &features, KVM_VCPU_MAX_FEATURES);
>
>  	ret = kvm_setup_vcpu(vcpu);
> -	if (ret)
> +	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;
> +	}
>
>  	/* Now we know what it is, we can reset it. */
>  	kvm_reset_vcpu(vcpu);
>
> base-commit: 6b1bca1b1ab77f60a62087337bfe6e2f0efb9e6d
> --
> 2.39.5
>

--
Cheers, Lorenzo