On Wed, Jun 30, 2021, David Matlack wrote:
> On Tue, Jun 29, 2021 at 08:10:37PM -0700, Venkatesh Srinivas wrote:
> > vcpu->cpu contains the last cpu a vcpu run/is running on;
> > kvm_vcpu_kick is used to 'kick' a guest vcpu by sending an IPI
> > to the last CPU; vcpu->cpu is updated on sched_in when a vcpu
> > moves to a new CPU, so it possible for the vcpu->cpu field to
> > be stale.
This fails to document the actual bug being fixed, and why the fix is correct.
The fact that vcpu->cpu may be stale is itself not a bug, e.g. even with this
patch, the IPI can be sent to the "wrong", i.e. smp_send_reschedule() can still
consume a stale vcpu->cpu.
The bug that's being fixed is that grabbing the potentially-stale vcpu->cpu
_before_ kvm_arch_vcpu_should_kick() can cause KVM to send an IPI to the wrong
CPU _and_ let the vCPU run longer than intended. The fix is correct because KVM
doesn't truly care about sending an IPI to the correct pCPU, it only cares about
about kicking the pCPU out of the guest. If the vCPU has exited and been loaded
on a different pCPU after kvm_arch_vcpu_should_kick(), then its mission has been
accomplished and the IPI (to the wrong pCPU) is truly spurious.
> > kvm_vcpu_kick also read vcpu->cpu early with a plain read,
> > while vcpu->cpu could be concurrently written. This caused
> > a data race, found by kcsan:
> >
> > write to 0xffff8880274e8460 of 4 bytes by task 30718 on cpu 0:
> > kvm_arch_vcpu_load arch/x86/kvm/x86.c:4018
> > kvm_sched_in virt/kvm/kvm_main.c:4864
> >
> > vs
> > kvm_vcpu_kick virt/kvm/kvm_main.c:2909
> > kvm_arch_async_page_present_queued arch/x86/kvm/x86.c:11287
> > async_pf_execute virt/kvm/async_pf.c:79
> > ...
> >
> > Use a READ_ONCE to atomically read vcpu->cpu and avoid the
> > data race.
> >
> > Found by kcsan & syzbot.
> >
> > Signed-off-by: Venkatesh Srinivas <venkateshs@xxxxxxxxxxxx>
Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> Reviewed-by: David Matlack <dmatlack@xxxxxxxxxx>
>
> > ---
> > virt/kvm/kvm_main.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 46fb042837d2..0525f42afb7d 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -3058,16 +3058,18 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_wake_up);
> > */
> > void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
> > {
> > - int me;
> > - int cpu = vcpu->cpu;
> > + int me, cpu;
> >
> > if (kvm_vcpu_wake_up(vcpu))
> > return;
> >
> > me = get_cpu();
> > - if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
> > - if (kvm_arch_vcpu_should_kick(vcpu))
> > + if (kvm_arch_vcpu_should_kick(vcpu)) {
> > + cpu = READ_ONCE(vcpu->cpu);
> > + WARN_ON_ONCE(cpu == me);
>
> nit: A comment here may be useful to explain the interaction with
> kvm_arch_vcpu_should_kick(). Took me a minute to understand why you
> added the warning.
Agreed.
> > + if ((unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
> > smp_send_reschedule(cpu);
> > + }
> > put_cpu();
> > }
> > EXPORT_SYMBOL_GPL(kvm_vcpu_kick);
> > --
> > 2.30.2
> >
