Re: [PATCH] x86: Don't crash when mapping a page using EFI runtime page tables

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 18/05/15 10:02, Jan Beulich wrote:
>>>> On 15.05.15 at 19:41, <andrew.cooper3@citrix.com> wrote:
>> On 15/05/15 17:08, Ross Lagerwall wrote:
>>> When an interrupt is received during an EFI runtime service call, Xen
>>> may call map_domain_page() while using the EFI runtime page tables.
>>> This fails because, although the EFI runtime page tables are a
>>> copy of the idle domain's page tables, current points at a different
>>> domain's vCPU.
>>>
>>> To fix this, return NULL from mapcache_current_vcpu() when using the EFI
>>> runtime page tables which is treated equivalently to running in an idle
>>> vCPU.
>>>
>>> This issue can be reproduced by repeatedly calling GetVariable() from
>>> dom0 while using VT-d, since VT-d frequently maps a page from interrupt
>>> context.
>>>
>>> Example call trace:
>>> [<ffff82d0801615dc>] __find_next_zero_bit+0x28/0x60
>>> [<ffff82d08016a10e>] map_domain_page+0x4c6/0x4eb
>>> [<ffff82d080156ae6>] map_vtd_domain_page+0xd/0xf
>>> [<ffff82d08015533a>] msi_msg_read_remap_rte+0xe3/0x1d8
>>> [<ffff82d08014e516>] iommu_read_msi_from_ire+0x31/0x34
>>> [<ffff82d08016ff6c>] set_msi_affinity+0x134/0x17a
>>> [<ffff82d0801737b5>] move_masked_irq+0x5c/0x98
>>> [<ffff82d080173816>] move_native_irq+0x25/0x36
>>> [<ffff82d08016ffcb>] ack_nonmaskable_msi_irq+0x19/0x20
>>> [<ffff82d08016ffdb>] ack_maskable_msi_irq+0x9/0x37
>>> [<ffff82d080173e8b>] do_IRQ+0x251/0x635
>>> [<ffff82d080234502>] common_interrupt+0x62/0x70
>>> [<00000000df7ed2be>] 00000000df7ed2be
>>>
>>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>> This is actually a VT-d issue which I have fallen over before, and
>> didn't have time to fix back then.  I had a cascade crash from having to
>> use map_domain_page() to disable the IOMMU following a crash in the
>> middle of a context switch.
>>
>> The VT-d code should use map_domain_page_global() for various of the
>> root structures, which will simply the code and reduce the runtime cost
>> of a lot of the codepaths.
> Yes.
>
>> However, the overall crash proves that map_domain_page() is not safe for
>> use in IRQ or exception context.  In the case that v == current,
>> map_domain_page() will degrade to mfn_to_virt() which is only safe if
>> one can guarantee that the frame being mapped is present in the direct
>> mapped region.  The map_domain_page() used by the VT-d code very
>> certainly isn't.
> I'm not following: There's certainly no special casing of v == current
> in map_domain_page(). And hence I can't see the IRQ/exception
> context issue you try to describe here.

My apologies.  I had confused myself.

>
> I'm wondering whether we shouldn't instead simply disable interrupts
> while doing EFI runtime services calls.

I would be hesitant doing this.  We have no idea how long EFI might
decide to stay in the runtime services.

>
>> We probably want to avoid using map_domain_page() when printing a
>> hypervisor page walk to avoid deadlocks in situations like this.
> That wouldn't be very helpful - such printing happens when we're
> about to halt the system due to a crash, and hence would hamper
> analysis of the cause. Or did you mean to replace the use of
> map_domain_page() there by e.g. a fixmap based mechanism, to
> avoid the lock state dependency?

I indented to find an alternative, not remove the printing.

~Andrew