kvm: x86: Fix racy event propagation in kmv timer
PATCH 1/1 kvm: x86: Fix racy event propagation in kmv timer
Minor issues that likely had no practical relevance: The kvm timer function so far incremented the pending counter and then may reset it again to 1 in case reinjection was disabled. This opened a small racy window with the corresponding VCPU loop that may have happened to run on another (real) CPU and already consumed the value.
Moveover, the previous code tried to optimize the setting of KVM_REQ_PENDING_TIMER, but used atomic_inc_and_test - which always returns true unless pending had the invalid value of -1 on entry.
---
arch/x86/kvm/timer.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)
arch/x86/kvm/timer.c +5 -6
@@ -9,12 +9,11 @@ static int __kvm_timer_fn(struct kvm_vcpu *vcpu, struct kvm_timer *ktimer)int restart_timer =0 ;wait_queue_head_t *q = &vcpu->wq; -/* FIXME: this code should not know anything about vcpus */ -if (!atomic_inc_and_test (&ktimer->pending)) -set_bit (KVM_REQ_PENDING_TIMER , &vcpu->requests); - -if (!ktimer->reinject) -atomic_set (&ktimer->pending,1 ); +if (ktimer->reinject || !atomic_read (&ktimer->pending)) { +/* FIXME: this code should not know anything about vcpus */ +if (atomic_inc_return (&ktimer->pending) ==1 ) +set_bit (KVM_REQ_PENDING_TIMER , &vcpu->requests); + }if (waitqueue_active (q))wake_up_interruptible (q);
PATCH v2 1/2 kvm: x86: Fix racy event propagation in kmv timer
v2 as requested in private discussion: Broken into two pieces, and the second one will not change the original semantic.
-------------->
Minor issue that likely had no practical relevance: The kvm timer function so far incremented the pending counter and then may reset it again to 1 in case reinjection was disabled. This opened a small racy window with the corresponding VCPU loop that may have happened to run on another (real) CPU and already consumed the value.
Fix it by skipping the incrementation in case pending is already > 0. This opens a different race windows, but may only rarely cause lost events in case we do not care about them anyway (!reinject).
---
arch/x86/kvm/timer.c | 16 ++++++++++------ 1 files changed, 10 insertions(+), 6 deletions(-)
arch/x86/kvm/timer.c +10 -6
@@ -9,12 +9,16 @@ static int __kvm_timer_fn(struct kvm_vcpu *vcpu, struct kvm_timer *ktimer)int restart_timer =0 ;wait_queue_head_t *q = &vcpu->wq; -/* FIXME: this code should not know anything about vcpus */ -if (!atomic_inc_and_test (&ktimer->pending)) -set_bit (KVM_REQ_PENDING_TIMER , &vcpu->requests); - -if (!ktimer->reinject) -atomic_set (&ktimer->pending,1 ); +/* +* There is a race window between reading and incrementing, but we do +* not care about potentially loosing timer events in the !reinject +* case anyway. +*/ +if (ktimer->reinject || !atomic_read (&ktimer->pending)) { +/* FIXME: this code should not know anything about vcpus */ +if (!atomic_inc_and_test (&ktimer->pending)) +set_bit (KVM_REQ_PENDING_TIMER , &vcpu->requests); + }if (waitqueue_active (q))wake_up_interruptible (q);
Quoted 11 lines · Jan Kiszka
v2 as requested in private discussion: Broken into two pieces, and the second one will not change the original semantic.
-------------->
Minor issue that likely had no practical relevance: The kvm timer function so far incremented the pending counter and then may reset it again to 1 in case reinjection was disabled. This opened a small racy window with the corresponding VCPU loop that may have happened to run on another (real) CPU and already consumed the value.
Quoted 5 lines
Fix it by skipping the incrementation in case pending is already > 0. This opens a different race windows, but may only rarely cause lost events in case we do not care about them anyway (!reinject).
Signed-off-by Jan Kiszka <[email protected]>
ACK both, thanks.
Quoted 13 lines · Marcelo Tosatti
Quoted 11 lines · Jan Kiszka
v2 as requested in private discussion: Broken into two pieces, and the second one will not change the original semantic.
-------------->
Minor issue that likely had no practical relevance: The kvm timer function so far incremented the pending counter and then may reset it again to 1 in case reinjection was disabled. This opened a small racy window with the corresponding VCPU loop that may have happened to run on another (real) CPU and already consumed the value.
Quoted 9 lines
Quoted 6 lines
Fix it by skipping the incrementation in case pending is already > 0. This opens a different race windows, but may only rarely cause lost events in case we do not care about them anyway (!reinject).
Signed-off-by Jan Kiszka <[email protected]>ACK both, thanks.
And applied both, thanks.
Signature · 1 line
error compiling committee.c: too many arguments to function
PATCH v2 2/2 kvm: x86: Drop useless atomic test from timer function
In reply to Jan KiszkaThe current code tries to optimize the setting of KVM_REQ_PENDING_TIMER but used atomic_inc_and_test - which always returns true unless pending had the invalid value of -1 on entry. This patch drops the test part preserving the original semantic but expressing it less confusingly.
---
arch/x86/kvm/timer.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
arch/x86/kvm/timer.c +2 -2
@@ -15,9 +15,9 @@ static int __kvm_timer_fn(struct kvm_vcpu *vcpu, struct kvm_timer *ktimer)* case anyway. */ if (ktimer->reinject || !atomic_read (&ktimer->pending)) { +atomic_inc (&ktimer->pending);/* FIXME: this code should not know anything about vcpus */ -if (!atomic_inc_and_test (&ktimer->pending)) -set_bit (KVM_REQ_PENDING_TIMER , &vcpu->requests); +set_bit (KVM_REQ_PENDING_TIMER , &vcpu->requests); }if (waitqueue_active (q))