Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1695220 > unrolled thread
| Started by | Wanpeng Li <kernellwp@gmail.com> |
|---|---|
| First post | 2017-07-25 00:50 +0200 |
| Last post | 2017-07-25 09:50 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH v3] KVM: LAPIC: Fix reentrancy issues with preempt notifiers Wanpeng Li <kernellwp@gmail.com> - 2017-07-25 00:50 +0200
Re: [PATCH v3] KVM: LAPIC: Fix reentrancy issues with preempt notifiers Paolo Bonzini <pbonzini@redhat.com> - 2017-07-25 09:30 +0200
Re: [PATCH v3] KVM: LAPIC: Fix reentrancy issues with preempt notifiers Wanpeng Li <kernellwp@gmail.com> - 2017-07-25 09:50 +0200
| From | Wanpeng Li <kernellwp@gmail.com> |
|---|---|
| Date | 2017-07-25 00:50 +0200 |
| Subject | [PATCH v3] KVM: LAPIC: Fix reentrancy issues with preempt notifiers |
| Message-ID | <u6UkN-2Vc-3@gated-at.bofh.it> |
From: Wanpeng Li <wanpeng.li@hotmail.com>
Preempt can occur in the preemption timer expiration handler:
CPU0 CPU1
preemption timer vmexit
handle_preemption_timer(vCPU0)
kvm_lapic_expired_hv_timer
hv_timer_is_use == true
sched_out
sched_in
kvm_arch_vcpu_load
kvm_lapic_restart_hv_timer
restart_apic_timer
start_hv_timer
already-expired timer or sw timer triggerd in the window
start_sw_timer
cancel_hv_timer
/* back in kvm_lapic_expired_hv_timer */
cancel_hv_timer
WARN_ON(!apic->lapic_timer.hv_timer_in_use); ==> Oops
This can be reproduced if CONFIG_PREEMPT is enabled.
------------[ cut here ]------------
WARNING: CPU: 4 PID: 2972 at /home/kernel/linux/arch/x86/kvm//lapic.c:1563 kvm_lapic_expired_hv_timer+0x9e/0xb0 [kvm]
CPU: 4 PID: 2972 Comm: qemu-system-x86 Tainted: G OE 4.13.0-rc2+ #16
RIP: 0010:kvm_lapic_expired_hv_timer+0x9e/0xb0 [kvm]
Call Trace:
handle_preemption_timer+0xe/0x20 [kvm_intel]
vmx_handle_exit+0xb8/0xd70 [kvm_intel]
kvm_arch_vcpu_ioctl_run+0xdd1/0x1be0 [kvm]
? kvm_arch_vcpu_load+0x47/0x230 [kvm]
? kvm_arch_vcpu_load+0x62/0x230 [kvm]
kvm_vcpu_ioctl+0x340/0x700 [kvm]
? kvm_vcpu_ioctl+0x340/0x700 [kvm]
? __fget+0xfc/0x210
do_vfs_ioctl+0xa4/0x6a0
? __fget+0x11d/0x210
SyS_ioctl+0x79/0x90
do_syscall_64+0x81/0x220
entry_SYSCALL64_slow_path+0x25/0x25
------------[ cut here ]------------
WARNING: CPU: 4 PID: 2972 at /home/kernel/linux/arch/x86/kvm//lapic.c:1498 cancel_hv_timer.isra.40+0x4f/0x60 [kvm]
CPU: 4 PID: 2972 Comm: qemu-system-x86 Tainted: G W OE 4.13.0-rc2+ #16
RIP: 0010:cancel_hv_timer.isra.40+0x4f/0x60 [kvm]
Call Trace:
kvm_lapic_expired_hv_timer+0x3e/0xb0 [kvm]
handle_preemption_timer+0xe/0x20 [kvm_intel]
vmx_handle_exit+0xb8/0xd70 [kvm_intel]
kvm_arch_vcpu_ioctl_run+0xdd1/0x1be0 [kvm]
? kvm_arch_vcpu_load+0x47/0x230 [kvm]
? kvm_arch_vcpu_load+0x62/0x230 [kvm]
kvm_vcpu_ioctl+0x340/0x700 [kvm]
? kvm_vcpu_ioctl+0x340/0x700 [kvm]
? __fget+0xfc/0x210
do_vfs_ioctl+0xa4/0x6a0
? __fget+0x11d/0x210
SyS_ioctl+0x79/0x90
do_syscall_64+0x81/0x220
entry_SYSCALL64_slow_path+0x25/0x25
This patch fixes it by making the caller of start_hv_timer and start_sw_timer
be in preemption-disabled regions, which trivially avoid any reentrancy
issue with preempt notifier.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
v1 -> v2:
* cleanup patch description
v2 -> v3:
* the caller of start_hv_timer and start_sw_timer in a preemption-disabled region
arch/x86/kvm/lapic.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 2819d4c..0d558c5 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1495,11 +1495,10 @@ EXPORT_SYMBOL_GPL(kvm_lapic_hv_timer_in_use);
static void cancel_hv_timer(struct kvm_lapic *apic)
{
+ WARN_ON(preemptible());
WARN_ON(!apic->lapic_timer.hv_timer_in_use);
- preempt_disable();
kvm_x86_ops->cancel_hv_timer(apic->vcpu);
apic->lapic_timer.hv_timer_in_use = false;
- preempt_enable();
}
static bool start_hv_timer(struct kvm_lapic *apic)
@@ -1552,18 +1551,25 @@ static void start_sw_timer(struct kvm_lapic *apic)
static void restart_apic_timer(struct kvm_lapic *apic)
{
+ preempt_disable();
if (!start_hv_timer(apic))
start_sw_timer(apic);
+ preempt_enable();
}
void kvm_lapic_expired_hv_timer(struct kvm_vcpu *vcpu)
{
struct kvm_lapic *apic = vcpu->arch.apic;
- WARN_ON(!apic->lapic_timer.hv_timer_in_use);
+ preempt_disable();
+ /* The preempt notifier has called apic_timer_expired already */
+ if (!apic->lapic_timer.hv_timer_in_use)
+ goto out;
WARN_ON(swait_active(&vcpu->wq));
cancel_hv_timer(apic);
apic_timer_expired(apic);
+out:
+ preempt_enable();
if (apic_lvtt_period(apic) && apic->lapic_timer.period) {
advance_periodic_target_expiration(apic);
@@ -1582,9 +1588,11 @@ void kvm_lapic_switch_to_sw_timer(struct kvm_vcpu *vcpu)
{
struct kvm_lapic *apic = vcpu->arch.apic;
+ preempt_disable();
/* Possibly the TSC deadline timer is not enabled yet */
if (apic->lapic_timer.hv_timer_in_use)
start_sw_timer(apic);
+ preempt_enable();
}
EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_sw_timer);
--
2.7.4
[toc] | [next] | [standalone]
| From | Paolo Bonzini <pbonzini@redhat.com> |
|---|---|
| Date | 2017-07-25 09:30 +0200 |
| Subject | Re: [PATCH v3] KVM: LAPIC: Fix reentrancy issues with preempt notifiers |
| Message-ID | <u72s1-8ic-1@gated-at.bofh.it> |
| In reply to | #1695220 |
On 25/07/2017 00:43, Wanpeng Li wrote:
> - WARN_ON(!apic->lapic_timer.hv_timer_in_use);
> + preempt_disable();
> + /* The preempt notifier has called apic_timer_expired already */
> + if (!apic->lapic_timer.hv_timer_in_use)
> + goto out;
> WARN_ON(swait_active(&vcpu->wq));
> cancel_hv_timer(apic);
> apic_timer_expired(apic);
> +out:
> + preempt_enable();
If apic_timer_expired was called, and the timer is in periodic mode, it
has already set the hv timer. Should the out label be really at the end
of the function, after the timer is restarted? Otherwise you can call
advance_periodic_timer_expiration twice.
Thanks,
Paolo
>
> if (apic_lvtt_period(apic) && apic->lapic_timer.period) {
> advance_periodic_target_expiration(apic);
> @@ -1582,9 +1588,11 @@ void kvm_lapic_switch_to_sw_timer(struct kvm_vcpu *vcpu)
> {
> struct kvm_lapic *apic = vcpu->arch.apic;
>
> + preempt_disable();
> /* Possibly the TSC deadline timer is not enabled yet */
> if (apic->lapic_timer.hv_timer_in_use)
> start_sw_timer(apic);
> + preempt_enable();
> }
> EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_sw_timer);
[toc] | [prev] | [next] | [standalone]
| From | Wanpeng Li <kernellwp@gmail.com> |
|---|---|
| Date | 2017-07-25 09:50 +0200 |
| Message-ID | <u72Lo-8oY-25@gated-at.bofh.it> |
| In reply to | #1695429 |
2017-07-25 15:25 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
> On 25/07/2017 00:43, Wanpeng Li wrote:
>> - WARN_ON(!apic->lapic_timer.hv_timer_in_use);
>> + preempt_disable();
>> + /* The preempt notifier has called apic_timer_expired already */
>> + if (!apic->lapic_timer.hv_timer_in_use)
>> + goto out;
>> WARN_ON(swait_active(&vcpu->wq));
>> cancel_hv_timer(apic);
>> apic_timer_expired(apic);
>> +out:
>> + preempt_enable();
>
> If apic_timer_expired was called, and the timer is in periodic mode, it
> has already set the hv timer. Should the out label be really at the end
> of the function, after the timer is restarted? Otherwise you can call
> advance_periodic_timer_expiration twice.
I just send out v4 to fix it. Thanks for the review.
Regards,
Wanpeng Li
>
> Thanks,
>
> Paolo
>
>>
>> if (apic_lvtt_period(apic) && apic->lapic_timer.period) {
>> advance_periodic_target_expiration(apic);
>> @@ -1582,9 +1588,11 @@ void kvm_lapic_switch_to_sw_timer(struct kvm_vcpu *vcpu)
>> {
>> struct kvm_lapic *apic = vcpu->arch.apic;
>>
>> + preempt_disable();
>> /* Possibly the TSC deadline timer is not enabled yet */
>> if (apic->lapic_timer.hv_timer_in_use)
>> start_sw_timer(apic);
>> + preempt_enable();
>> }
>> EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_sw_timer);
>
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web