linux-kernel - Re: [PART1 RFC 5/9] svm: Add VMEXIT handlers for AVIC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56C5E8E1.9060900@redhat.com>
Date:	Thu, 18 Feb 2016 16:53:05 +0100
From:	Paolo Bonzini <pbonzini@...hat.com>
To:	Radim Krčmář <rkrcmar@...hat.com>
Cc:	Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>,
	joro@...tes.org, alex.williamson@...hat.com, gleb@...nel.org,
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org, wei@...hat.com,
	sherry.hurwitz@....com
Subject: Re: [PART1 RFC 5/9] svm: Add VMEXIT handlers for AVIC



On 18/02/2016 16:43, Radim Krčmář wrote:
> 2016-02-18 15:51+0100, Paolo Bonzini:
>> On 18/02/2016 15:18, Radim Krčmář wrote:
>>> KVM just has to make sure that targeted VCPUs notice the interrupt,
>>> which means to kick (wake up) VCPUs that don't have IsRunning set.
>>> There is no need to do anything with running VCPUs, because they
>>>  - are in guest mode and noticed the doorbell
>>>  - are in host mode, where they will
>>>    1) VMRUN as fast as they can because the VCPU didn't want to halt
>>>       (and IRR is handled on VMRUN)
>>>    2) check IRR after unsetting IsRunning and goto (1) if there are
>>>       pending interrupts.  (RFC doesn't do this, which is another bug)
>>
>> This is not necessary.  IsRunning is only cleared at vcpu_put time.
> 
> It's not necessary if we are being preempted, but it is necessary to
> clear IsRunning earlier when we are going to block (i.e. after a halt).
> 
>>                                                                      The
>> next KVM_RUN will look at IRR (kvm_arch_vcpu_runnable), if necessary set
>> the mp_state to KVM_MP_STATE_RUNNABLE, and do the VMRUN.
> 
> We're not always going to exit to userspace.  I think the following
> order of actions could result in a stuck VM:
> 
> (The main idea is that VCPU targets another VCPU between its last check
>  for IRR and clearing of IsRunning.)
> 
> 1) vcpu0 has set IsRunning and is in guest mode.
> 2) vcpu0 executes HLT and exits guest mode.
> 3) vcpu0 doesn't have any pending interrupts or other stuff that would
>    make kvm_arch_vcpu_runnable() return true.
> 4) vcpu0 runs kvm_vcpu_block() and gets to call schedule().
> 5) *vcpu1* sends IPI to vcpu0.  IsRunning is set on vcpu0, so AVIC
>    doesn't exit.  A doorbell is sent, but it does nothing.
> 6) vcpu0 runs schedule() and clears IsRunning in a callback.

You're completely right.  When the VCPU is halting, the preempt
notifier's sched_out callback is too late to clear IsRunning; you need
to do that before the last time you check for IRR.

Patch 9 is okay, but it is also necessary to clear IsRunning in
kvm_arch_vcpu_blocking and set it in kvm_arch_vcpu_unblocking.  In
addition, vcpu_put/vcpu_load should not modify IsRunning between
kvm_arch_vcpu_blocking and kvm_arch_vcpu_unblocking.  Do you agree?

Paolo