* [PATCH] KVM: kvm_vcpu_kick: Do not read potentially-stale vcpu->cpu @ 2021-06-30 3:10 Venkatesh Srinivas 2021-06-30 16:24 ` David Matlack 0 siblings, 1 reply; 4+ messages in thread From: Venkatesh Srinivas @ 2021-06-30 3:10 UTC (permalink / raw) To: kvm, jmattson, seanjc, elver, dvyukov; +Cc: Venkatesh Srinivas 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. 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@chromium.org> --- 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); + if ((unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) smp_send_reschedule(cpu); + } put_cpu(); } EXPORT_SYMBOL_GPL(kvm_vcpu_kick); -- 2.30.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: kvm_vcpu_kick: Do not read potentially-stale vcpu->cpu 2021-06-30 3:10 [PATCH] KVM: kvm_vcpu_kick: Do not read potentially-stale vcpu->cpu Venkatesh Srinivas @ 2021-06-30 16:24 ` David Matlack 2021-07-09 17:47 ` Sean Christopherson 0 siblings, 1 reply; 4+ messages in thread From: David Matlack @ 2021-06-30 16:24 UTC (permalink / raw) To: Venkatesh Srinivas; +Cc: kvm, jmattson, seanjc, elver, dvyukov 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. > > 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@chromium.org> Reviewed-by: David Matlack <dmatlack@google.com> > --- > 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. > + if ((unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) > smp_send_reschedule(cpu); > + } > put_cpu(); > } > EXPORT_SYMBOL_GPL(kvm_vcpu_kick); > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: kvm_vcpu_kick: Do not read potentially-stale vcpu->cpu 2021-06-30 16:24 ` David Matlack @ 2021-07-09 17:47 ` Sean Christopherson 2021-07-13 1:45 ` Venkatesh Srinivas 0 siblings, 1 reply; 4+ messages in thread From: Sean Christopherson @ 2021-07-09 17:47 UTC (permalink / raw) To: David Matlack; +Cc: Venkatesh Srinivas, kvm, jmattson, elver, dvyukov 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@chromium.org> Suggested-by: Sean Christopherson <seanjc@google.com> > Reviewed-by: David Matlack <dmatlack@google.com> > > > --- > > 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 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: kvm_vcpu_kick: Do not read potentially-stale vcpu->cpu 2021-07-09 17:47 ` Sean Christopherson @ 2021-07-13 1:45 ` Venkatesh Srinivas 0 siblings, 0 replies; 4+ messages in thread From: Venkatesh Srinivas @ 2021-07-13 1:45 UTC (permalink / raw) To: Sean Christopherson; +Cc: David Matlack, kvm, Jim Mattson, elver, dvyukov On Fri, Jul 9, 2021 at 10:47 AM Sean Christopherson <seanjc@google.com> wrote: > > 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. Will improve the comment for V2. > > > 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@chromium.org> > > Suggested-by: Sean Christopherson <seanjc@google.com> > > > Reviewed-by: David Matlack <dmatlack@google.com> > > > > > --- > > > 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. Will do for V2 > > > + if ((unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) > > > smp_send_reschedule(cpu); > > > + } > > > put_cpu(); > > > } > > > EXPORT_SYMBOL_GPL(kvm_vcpu_kick); > > > -- > > > 2.30.2 > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-07-13 1:45 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-06-30 3:10 [PATCH] KVM: kvm_vcpu_kick: Do not read potentially-stale vcpu->cpu Venkatesh Srinivas 2021-06-30 16:24 ` David Matlack 2021-07-09 17:47 ` Sean Christopherson 2021-07-13 1:45 ` Venkatesh Srinivas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).