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

On 2026/8/10 下午4:17, Tao Cui wrote:
> 
> 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;
>> +		}
The for loop sentence is a little strange, can we use spinlock method 
rather than atomic cmpxchg method? It is not performance sensitive here.

Regards
Bibo Mao
>>   	default:
>>   		return -ENXIO;
>>   	}