Re: [PATCH v2 8/9] KVM: arm/arm64: fix race in kvm_psci_vcpu_on

[Christoffer Dall <cdall@linaro.org>]

On Fri, Mar 31, 2017 at 06:06:57PM +0200, Andrew Jones wrote:
> From: Levente Kurusa <lkurusa@redhat.com>
> 
> When two vcpus issue PSCI_CPU_ON on the same core at the same time,
> then it's possible for them to both enter the target vcpu's setup
> at the same time. This results in unexpected behaviors at best,
> and the potential for some nasty bugs at worst.
> 
> Signed-off-by: Levente Kurusa <lkurusa@redhat.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  arch/arm/kvm/psci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
> index f732484abc7a..0204daa899b1 100644
> --- a/arch/arm/kvm/psci.c
> +++ b/arch/arm/kvm/psci.c
> @@ -88,7 +88,8 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  	 */
>  	if (!vcpu)
>  		return PSCI_RET_INVALID_PARAMS;
> -	if (!test_bit(KVM_REQ_POWER_OFF, &vcpu->requests)) {
> +
> +	if (!test_and_clear_bit(KVM_REQ_POWER_OFF, &vcpu->requests)) {
>  		if (kvm_psci_version(source_vcpu) != KVM_ARM_PSCI_0_1)
>  			return PSCI_RET_ALREADY_ON;
>  		else
> @@ -116,7 +117,6 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  	 * the general puspose registers are undefined upon CPU_ON.
>  	 */
>  	vcpu_set_reg(vcpu, 0, context_id);
> -	clear_bit(KVM_REQ_POWER_OFF, &vcpu->requests);
>  
>  	wq = kvm_arch_vcpu_wq(vcpu);
>  	swake_up(wq);
> -- 
> 2.9.3
> 

Depending on what you end up doing with the requests, if you keep the
bool flag you could just use the kvm->lock mutex instead.

Have you considered if there are any potential races between
kvm_psci_system_off() being called on one VCPU while two other VPCUs are
turning on the same CPU that is being turend off as part of system-wide
power down as well?

I'm wondering if this means we should take the kvm->lock at a higher
level when handling PSCI events...

Thanks,
-Christoffer