Re: [PATCH] LoongArch: KVM: Fix TOCTOU race on pv_features

Hi Bibo,
在 2026/8/10 16:13, Tao Cui 写道:
> From: Tao Cui <[email protected]>
> 
> kvm_loongarch_cpucfg_set_attr() validates and writes the VM-wide
> pv_features with a lockless check-then-set, so two vCPUs racing it can
> both pass the "all-vCPUs-must-match" check and install divergent values.
> Make the check-then-set atomic with a cmpxchg loop; the UPDATED bit
> already packs the configured state into the same word.
> 

I just sent a small patch for the pv_features TOCTOU we discussed. It
turns the check-then-set into a cmpxchg loop on the existing UPDATED
bit, so there's no new field or lock.

While working on it I also looked at two other options and wanted to
mention them here.

One was a spinlock around the check-then-set. It works, but it needs a
new kvm_arch member just for this, which didn't seem worth it given the
UPDATED bit already keeps the value and flag in a single word.

The other is to move pv_features per-vCPU (vcpu->arch). That removes the
shared state altogether: no lock, no latch, no cross-vCPU check, and the
VMM just keeps the vCPUs in sync. It's the cleaner design, but a larger
change, since QEMU would need a matching change too. Today QEMU pushes
pv_features behind a process-wide `static int once` (only the first
vCPU), which relies on the per-VM storage. This is the per-CPU direction
you mentioned earlier [1]; if you're still planning to do it I'm happy to
hold off, otherwise I can put together the kernel + QEMU side.

All three were built and tested locally with a vCPU-attribute test (the
spinlock also came up clean under KCSAN).

Thanks,
Tao

[1] https://lore.kernel.org/all/[email protected]/

> Signed-off-by: Tao Cui <[email protected]>
> ---
>  arch/loongarch/kvm/vcpu.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index 20c207d80e31..55030c37cf06 100644
> --- a/arch/loongarch/kvm/vcpu.c
> +++ b/arch/loongarch/kvm/vcpu.c
> @@ -1164,12 +1164,18 @@ static int kvm_loongarch_cpucfg_set_attr(struct kvm_vcpu *vcpu,
>  		if (val & ~valid)
>  			return -EINVAL;
>  
> -		/* All vCPUs need set the same PV features */
> -		if ((kvm->arch.pv_features & LOONGARCH_PV_FEAT_UPDATED)
> -				&& ((kvm->arch.pv_features & valid) != val))
> -			return -EINVAL;
> -		kvm->arch.pv_features = val | LOONGARCH_PV_FEAT_UPDATED;
> -		return 0;
> +		/* Atomically install val; the cmpxchg serializes concurrent setters. */
> +		for (;;) {
> +			unsigned long old, new;
> +
> +			old = READ_ONCE(kvm->arch.pv_features);
> +			if ((old & LOONGARCH_PV_FEAT_UPDATED) &&
> +			    ((old & valid) != val))
> +				return -EINVAL;
> +			new = val | LOONGARCH_PV_FEAT_UPDATED;
> +			if (cmpxchg(&kvm->arch.pv_features, old, new) == old)
> +				return 0;
> +		}
>  	default:
>  		return -ENXIO;
>  	}