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

[Bibo Mao]

On 2026/8/10 下午4:17, Tao Cui wrote:
> Hi Bibo,
> 在 2026/8/10 16:13, Tao Cui 写道:
> > From: Tao Cui <cuitao@xxxxxxxxxx>
> >
> > 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/9696e097-7399-96b2-e0ae-bbb0d2f8c0d7@xxxxxxxxxxx/
>
> > Signed-off-by: Tao Cui <cuitao@xxxxxxxxxx>
> > ---
> >   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;
> >   	}