Async page fault delivered while irq are disabled?

DISCUSSION Frederic Weisbecker 3 participants 8 messages Dec 19, 2019 – Dec 26, 2019 KVM

Share & tools

Frederic Weisbecker

author

2019-12-19 15:28 UTC #0 lore raw

Hi,

While checking the x86 async page fault code, I can't
find anything that prevents KVM_PV_REASON_PAGE_READY to be injected while the guest has interrupts disabled. If that page fault happens to trap in an interrupt disabled section, there may be a deadlock due to the call to wake_up_process() which locks the rq->lock (among others).

Given how long that code is there, I guess such an issue would have been reported for a while already. But I just would like to be sure we are checking that.

Can someone enlighten me?

Thanks.

Sean Christopherson

2019-12-19 15:57 UTC #1 lore raw

Quoted 8 lines · Frederic Weisbecker

Hi,
While checking the x86 async page fault code, I can't
find anything that prevents KVM_PV_REASON_PAGE_READY to be injected while the guest has interrupts disabled. If that page fault happens to trap in an interrupt disabled section, there may be a deadlock due to the call to wake_up_process() which locks the rq->lock (among others).

Quoted 5 lines

Given how long that code is there, I guess such an issue would have been reported for a while already. But I just would like to be sure we are checking that.
Can someone enlighten me?

The check is triggered from the caller of kvm_async_page_present().

kvm_check_async_pf_completion()
|
|-> kvm_arch_can_inject_async_page_present()

    |
    |-> kvm_can_do_async_pf()
        |
        |-> kvm_x86_ops->interrupt_allowed()

Frederic Weisbecker

author

2019-12-19 16:15 UTC #2 lore raw

Quoted 17 lines · Sean Christopherson

Quoted 8 lines · Frederic Weisbecker
Hi,
While checking the x86 async page fault code, I can't
find anything that prevents KVM_PV_REASON_PAGE_READY to be injected while the guest has interrupts disabled. If that page fault happens to trap in an interrupt disabled section, there may be a deadlock due to the call to wake_up_process() which locks the rq->lock (among others).
Quoted 5 lines
Given how long that code is there, I guess such an issue would have been reported for a while already. But I just would like to be sure we are checking that.
Can someone enlighten me?
The check is triggered from the caller of kvm_async_page_present().

Quoted 7 lines

kvm_check_async_pf_completion()
|
|-> kvm_arch_can_inject_async_page_present()
    |
    |-> kvm_can_do_async_pf()
        |
        |-> kvm_x86_ops->interrupt_allowed()

Ah thanks, I missed that one. And what about kvm_async_page_present_sync()? I don't see a similar check there.

And one last silly question, what about that line in kvm_arch_can_inject_async_page_present:

	if (!(vcpu->arch.apf.msr_val & KVM_ASYNC_PF_ENABLED))
		return true;

That looks weird, also it shortcuts the irqs_allowed() check.

Thanks!

Sean Christopherson

2019-12-19 19:00 UTC #3 lore raw

Quoted 26 lines · Frederic Weisbecker

Quoted 17 lines · Sean Christopherson
Quoted 8 lines · Frederic Weisbecker
Hi,
While checking the x86 async page fault code, I can't
find anything that prevents KVM_PV_REASON_PAGE_READY to be injected while the guest has interrupts disabled. If that page fault happens to trap in an interrupt disabled section, there may be a deadlock due to the call to wake_up_process() which locks the rq->lock (among others).
Quoted 5 lines
Given how long that code is there, I guess such an issue would have been reported for a while already. But I just would like to be sure we are checking that.
Can someone enlighten me?
The check is triggered from the caller of kvm_async_page_present().
Quoted 7 lines
kvm_check_async_pf_completion()
|
|-> kvm_arch_can_inject_async_page_present()
    |
    |-> kvm_can_do_async_pf()
        |
        |-> kvm_x86_ops->interrupt_allowed()

Quoted 3 lines

Ah thanks, I missed that one. And what about kvm_async_page_present_sync()? I don't see a similar check there.

CONFIG_KVM_ASYNC_PF_SYNC is selected only by s390, it can't be turned on for x86.

Quoted 7 lines

And one last silly question, what about that line in kvm_arch_can_inject_async_page_present:
	if (!(vcpu->arch.apf.msr_val & KVM_ASYNC_PF_ENABLED))
		return true;
That looks weird, also it shortcuts the irqs_allowed() check.

I wondered about that code as well :-). Definitely odd, but it would require the guest to disable async #PF after an async #PF is queued. Best guess is the idea is that it's the guest's problem if it disables async #PF on the fly.

Paolo Bonzini

maintainer

2019-12-20 09:34 UTC #4 lore raw

Quoted 6 lines · Sean Christopherson

Quoted 6 lines
And one last silly question, what about that line in kvm_arch_can_inject_async_page_present:
	if (!(vcpu->arch.apf.msr_val & KVM_ASYNC_PF_ENABLED))
		return true;

Quoted 7 lines

Quoted 1 line
That looks weird, also it shortcuts the irqs_allowed() check.
I wondered about that code as well :-). Definitely odd, but it would require the guest to disable async #PF after an async #PF is queued. Best guess is the idea is that it's the guest's problem if it disables async #PF on the fly.

When the guest disables async #PF all outstanding page faults are cancelled by kvm_clear_async_pf_completion_queue. However, in case they complete while in cancel_work_sync. you need to inject them even if interrupts are disabled.

Paolo

Frederic Weisbecker

author

2019-12-23 02:17 UTC #5 lore raw

Quoted 15 lines · Paolo Bonzini

Quoted 6 lines · Sean Christopherson
Quoted 6 lines
And one last silly question, what about that line in kvm_arch_can_inject_async_page_present:
	if (!(vcpu->arch.apf.msr_val & KVM_ASYNC_PF_ENABLED))
		return true;
Quoted 7 lines
Quoted 1 line
That looks weird, also it shortcuts the irqs_allowed() check.
I wondered about that code as well :-). Definitely odd, but it would require the guest to disable async #PF after an async #PF is queued. Best guess is the idea is that it's the guest's problem if it disables async #PF on the fly.

Quoted 4 lines

When the guest disables async #PF all outstanding page faults are cancelled by kvm_clear_async_pf_completion_queue. However, in case they complete while in cancel_work_sync. you need to inject them even if interrupts are disabled.

Hmm, shouldn't the guest wait for the whole pending waitqueue in kvm_async_pf_task_wait() to be serviced and woken up before actually allowing to disable async #PF ? Because you can't really afford to inject those #PF while IRQs are disabled, that's a big rq deadlock risk.

Paolo Bonzini

maintainer

2019-12-23 08:38 UTC #6 lore raw

Quoted 21 lines · Frederic Weisbecker

Quoted 15 lines · Paolo Bonzini
Quoted 6 lines · Sean Christopherson
Quoted 6 lines
And one last silly question, what about that line in kvm_arch_can_inject_async_page_present:
	if (!(vcpu->arch.apf.msr_val & KVM_ASYNC_PF_ENABLED))
		return true;
Quoted 7 lines
Quoted 1 line
That looks weird, also it shortcuts the irqs_allowed() check.
I wondered about that code as well :-). Definitely odd, but it would require the guest to disable async #PF after an async #PF is queued. Best guess is the idea is that it's the guest's problem if it disables async #PF on the fly.
Quoted 4 lines
When the guest disables async #PF all outstanding page faults are cancelled by kvm_clear_async_pf_completion_queue. However, in case they complete while in cancel_work_sync. you need to inject them even if interrupts are disabled.

Quoted 4 lines

Hmm, shouldn't the guest wait for the whole pending waitqueue in kvm_async_pf_task_wait() to be serviced and woken up before actually allowing to disable async #PF ? Because you can't really afford to inject those #PF while IRQs are disabled, that's a big rq deadlock risk.

That's just how Linux works, and Linux doesn't ever disable async page faults with disabled IRQ (reboot_notifier_list is a blocking notifier).

Paolo

Frederic Weisbecker

author

2019-12-26 17:28 UTC #7 lore raw

Quoted 22 lines · Paolo Bonzini

Quoted 21 lines · Frederic Weisbecker
Quoted 19 lines · Paolo Bonzini
Quoted 6 lines · Sean Christopherson
Quoted 6 lines
And one last silly question, what about that line in kvm_arch_can_inject_async_page_present:
	if (!(vcpu->arch.apf.msr_val & KVM_ASYNC_PF_ENABLED))
		return true;
Quoted 7 lines
Quoted 1 line
That looks weird, also it shortcuts the irqs_allowed() check.
I wondered about that code as well :-). Definitely odd, but it would require the guest to disable async #PF after an async #PF is queued. Best guess is the idea is that it's the guest's problem if it disables async #PF on the fly.
When the guest disables async #PF all outstanding page faults are cancelled by kvm_clear_async_pf_completion_queue. However, in case they complete while in cancel_work_sync. you need to inject them even if interrupts are disabled.

Quoted 7 lines

Quoted 4 lines
Hmm, shouldn't the guest wait for the whole pending waitqueue in kvm_async_pf_task_wait() to be serviced and woken up before actually allowing to disable async #PF ? Because you can't really afford to inject those #PF while IRQs are disabled, that's a big rq deadlock risk.
That's just how Linux works, and Linux doesn't ever disable async page faults with disabled IRQ (reboot_notifier_list is a blocking notifier).

So when I talk about IRQs enabled requirement, this is to prevent the page fault from interrupting code that may hold a lock.

Now in those case I think we are good, as kvm_pv_guest_cpu_reboot() is called from a generic IPI (rq and others shouldn't be held at that time) and kvm_guest_cpu_offline() is called from a thread with interrupts disabled.

Anyway those semantics and expectations are very obscure. Probably those async page faults should be considered as IRQs from lockdep POV.