From: David Woodhouse <[email protected]> Plane twins are published with a plain store to common->vcpus[] plus an smp_wmb() that pairs with online_vcpus, not with twin initialisation, while vcpu_for_each_plane() reads the slots with a bare load. x86-TSO hides it, but the protocol is unannotated generic code: KCSAN reports it, and nothing stops a reordered publication from exposing a half-initialised twin to a lockless iterator.
Publish the slot with smp_store_release(), read it with READ_ONCE() in vcpu_for_each_plane(), and document the pairing at the iterator. The lockless readers here are vcpu_for_each_plane() callers that walk other vCPUs' twins, such as the APICv BLOCKIRQ recompute; the kick paths read common->current_vcpu instead, and the Hyper-V twin lookups that also pair with the release come later, with the VSM code. No functional change on x86. Signed-off-by: David Woodhouse <[email protected]> --- include/linux/kvm_host.h | 7 ++++++- virt/kvm/kvm_main.c | 9 ++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 1f47b3bfa4bb..6922e3063ad8 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -393,9 +393,14 @@ struct kvm_vcpu_common { struct kvm_vcpu_arch_common arch; }; +/* + * The slot read pairs with the smp_store_release() publication in + * kvm_vcpu_finish_common() for iterators running outside the locks that + * serialise twin creation. + */ #define vcpu_for_each_plane(common, i, v) \ for ((i) = 0; (i) < KVM_MAX_PLANES; ++(i)) \ - if (((v) = common->vcpus[(i)]) != NULL) + if (((v) = READ_ONCE(common->vcpus[(i)])) != NULL) /* Tracked per plane-VCPU - used for deciding which plane-vcpu to run */ enum kvm_vcpu_state { diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index b6bdbe5c5133..7f215822e48e 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -548,7 +548,14 @@ static int kvm_vcpu_init_common(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned static void kvm_vcpu_finish_common(struct kvm_vcpu *vcpu) { WARN_ON(vcpu->common->vcpus[vcpu->plane_level] != NULL); - vcpu->common->vcpus[vcpu->plane_level] = vcpu; + /* + * Publish with release semantics so that vcpu_for_each_plane() + * callers running outside the locks that serialise twin creation + * (such as the APICv BLOCKIRQ recompute, which walks every vCPU's + * twins) observe a fully initialised vCPU through their READ_ONCE() + * of the slot; the plain store relied on x86-TSO. + */ + smp_store_release(&vcpu->common->vcpus[vcpu->plane_level], vcpu); smp_wmb(); if (vcpu->plane_level == 0) { /* -- 2.50.1
