On Fri, Nov 05, 2021, Marc Zyngier wrote:
> All architectures have similar loops iterating over the vcpus,
> freeing one vcpu at a time, and eventually wiping the reference
> off the vcpus array. They are also inconsistently taking
> the kvm->lock mutex when wiping the references from the array.
...
> +void kvm_destroy_vcpus(struct kvm *kvm)
> +{
> + unsigned int i;
> + struct kvm_vcpu *vcpu;
> +
> + kvm_for_each_vcpu(i, vcpu, kvm)
> + kvm_vcpu_destroy(vcpu);
> +
> + mutex_lock(&kvm->lock);
But why is kvm->lock taken here? Unless I'm overlooking an arch, everyone calls
this from kvm_arch_destroy_vm(), in which case this is the only remaining reference
to @kvm. And if there's some magic path for which that's not true, I don't see how
it can possibly be safe to call kvm_vcpu_destroy() without holding kvm->lock, or
how this would guarantee that all vCPUs have actually been destroyed before nullifying
the array.
> + for (i = 0; i < atomic_read(&kvm->online_vcpus); i++)
> + kvm->vcpus[i] = NULL;
> +
> + atomic_set(&kvm->online_vcpus, 0);
> + mutex_unlock(&kvm->lock);
> +}
> +EXPORT_SYMBOL_GPL(kvm_destroy_vcpus);
