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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Apr 05, 2017 at 10:35:59AM +0200, Andrew Jones wrote:
> On Tue, Apr 04, 2017 at 09:42:08PM +0200, Christoffer Dall wrote:
> > On Fri, Mar 31, 2017 at 06:06:57PM +0200, Andrew Jones wrote:
> > > From: Levente Kurusa <lkurusa@xxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxx>
> > > Signed-off-by: Andrew Jones <drjones@xxxxxxxxxx>
> > > ---
> > >  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?
> 
> Sounds like a nice unit test.  I haven't considered it, but I guess
> the kvm_psci_system_off/reset calling VCPU will ultimately "win", as
> it'll cause an exit to userspace that initiates a shutdown/reset.
> When the VCPUs are restarted then vcpu init should reset the power_off
> state correctly.  As long as the race this patch addresses is fixed, then
> I'm not sure there should be any risk with the actual system_off/reset
> being delayed wrt a vcpu being "on'ed" again, nor with there being more
> than one VCPU trying to "on" it at the same time.
> 
> > 
> > I'm wondering if this means we should take the kvm->lock at a higher
> > level when handling PSCI events...
> 
> That would simplify our analysis of the PSCI emulation, but I'm not
> sure we want to give a guest the power to constantly acquire that
> mutex with a barrage of PSCI calls.  Maybe we should create a PSCI
> mutex?  In order to avoid holding it too long we may want power_off to
> be more than a boolean though, i.e. the PENDING state might also be
> a good idea to represent.
> 

Hmm, the kvm->lock mutex is per-VM, so if a VM wants to use its CPU
resources by taking its own mutex, I don't really see the problem.

-Christoffer



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux