Re: [PATCH] x86/vpmu: fix race-condition in vpmu_load

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.09.2022 16:01, Tamas K Lengyel wrote:
> While experimenting with the vPMU subsystem an ASSERT failure was
> observed in vmx_find_msr because the vcpu_runnable state was true.
> 
> The root cause of the bug appears to be the fact that the vPMU subsystem
> doesn't save its state on context_switch. The vpmu_load function will attempt
> to gather the PMU state if its still loaded two different ways:
>     1. if the current pcpu is not where the vcpu ran before doing a remote 
> save
>     2. if the current pcpu had another vcpu active before doing a local save
> 
> However, in case the prev vcpu is being rescheduled on another pcpu its state
> has already changed and vcpu_runnable is returning true, thus #2 will trip the
> ASSERT. The only way to avoid this race condition is to make sure the
> prev vcpu is paused while being checked and its context saved. Once the prev
> vcpu is resumed and does #1 it will find its state already saved.

While I consider this explanation plausible, I'm worried:

> --- a/xen/arch/x86/cpu/vpmu.c
> +++ b/xen/arch/x86/cpu/vpmu.c
> @@ -419,8 +419,10 @@ int vpmu_load(struct vcpu *v, bool_t from_guest)
>          vpmu = vcpu_vpmu(prev);
>  
>          /* Someone ran here before us */
> +        vcpu_pause(prev);
>          vpmu_save_force(prev);
>          vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
> +        vcpu_unpause(prev);
>  
>          vpmu = vcpu_vpmu(v);
>      }

We're running with IRQs off here, yet vcpu_pause() waits for the vcpu
to actually be de-scheduled. Even with IRQs on this is already a
relatively heavy operation (also including its impact on the remote
side). Additionally the function is called from context_switch(), and
I'm unsure of the usability of vcpu_pause() on such a path. In
particular: Is there a risk of two CPUs doing this mutually to one
another? If so, is deadlocking excluded?

Hence at the very least I think the description wants extending, to
discuss the safety of the change.

Boris - any chance you could comment here? Iirc that's code you did
introduce.

Jan