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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 27.05.15 at 14:03, <ross.lagerwall@citrix.com> wrote:
> On 05/27/2015 12:59 PM, Jan Beulich wrote:
>>>>> On 27.05.15 at 12:23, <ross.lagerwall@citrix.com> wrote:
>>> On 05/18/2015 03:58 PM, Jan Beulich wrote:
>>>>>>> On 15.05.15 at 18:08, <ross.lagerwall@citrix.com> wrote:
>>>>> --- a/xen/arch/x86/domain_page.c
>>>>> +++ b/xen/arch/x86/domain_page.c
>>>>> @@ -32,20 +32,25 @@ static inline struct vcpu *mapcache_current_vcpu(void)
>>>>>            return NULL;
>>>>>
>>>>>        /*
>>>>> +     * When using efi runtime page tables, we have the equivalent of the
>>> idle
>>>>> +     * domain's page tables but current may point at another domain's VCPU.
>>>>> +     * Return NULL as though current is not properly set up yet.
>>>>> +     */
>>>>> +    if ( efi_enabled && read_cr3() == efi_rs_page_table() )
>>>>> +        return NULL;
>>>>
>>>> I'm okay with the patch in principle; what worries me is the CR3 read
>>>> that is now going to be necessary even in non-debug builds. With
>>>> this code being the only user of efi_rs_page_table(), I wonder if it
>>>> wouldn't make sense to alter that function to return non-zero only
>>>> when spin_is_locked(&efi_rs_lock), and then alter the code above
>>>> such that the CR3 read would happen only when we got a non-zero
>>>> value back.
>>>
>>> mapcache_current_vcpu() appears to be called from IRQ-enabled and
>>> IRQ-disabled callers which prevents us from using the spinlock.
>>
>> I didn't suggest to use any spin lock; I merely suggested checking
>> whether that particular one is being held by someone (to avoid the
>> CR3 read if that's not the case).
> 
> spin_is_locked() calls check_lock() which causes a BUG_ON() even though 
> you're not actually using the lock.

Then some other mechanism - spin_is_locked() would have produced
false positives anyway. E.g. storing the CPU which managed to acquire
the lock in efi_rs_enter(), and clearing it in efi_rs_leave() before
dropping the lock. efi_rs_page_table() could then return non-zero on
only this one CPU.

Jan