Re: [PATCH] Revert "x86/mm: drop guest_get_eff_l1e()"

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.12.2020 14:21, Andrew Cooper wrote:
> On 14/12/2020 08:27, Jan Beulich wrote:
>> On 11.12.2020 15:16, Andrew Cooper wrote:
>>> This reverts commit 9ff9705647646aa937b5f5c1426a64c69a62b3bd.
>>>
>>> The change is only correct in the original context of XSA-286, where Xen's 
>>> use
>>> of the linear pagetables were dropped.  However, performance problems
>>> interfered with that plan, and XSA-286 was fixed differently.
>>>
>>> This broke Xen's lazy faulting of the LDT for 64bit PV guests when an access
>>> was first encountered in user context.  Xen would proceed to read the
>>> registered LDT virtual address out of the user pagetables, not the kernel
>>> pagetables.
>>>
>>> Given the nature of the bug, it would have also interfered with the IO
>>> permisison bitmap functionality of userspace, which similarly needs to read
>>> data using the kernel pagetables.
>> This paragraph wants dropping afaict - guest_io_okay() has
>> explicit calls to toggle_guest_pt(), and hence is unaffected by
>> the bug here (and there is in particular page tables reading
>> involved there). Then ...
>>
>>> Reported-by: Manuel Bouyer <bouyer@xxxxxxxxxxxxxxx>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> Tested-by: Manuel Bouyer <bouyer@xxxxxxxxxxxxxxx>
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> I was wondering however whether we really want a full revert. I've
>> locally made the change below as an alternative.
>>
>> Jan
>>
>> x86/PV: guest_get_eff_kern_l1e() may still need to switch page tables
>>
>> While indeed unnecessary for pv_ro_page_fault(), pv_map_ldt_shadow_page()
>> may run when guest user mode is active, and hence may need to switch to
>> the kernel page tables in order to retrieve an LDT page mapping.
>>
>> Fixes: 9ff970564764 ("x86/mm: drop guest_get_eff_l1e()")
>> Reported-by: Manuel Bouyer <bouyer@xxxxxxxxxxxxxxx>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> This is the alternative to fully reverting the offending commit.
> 
> Hmm yes - I think I prefer this, because we don't really want to keep
> the non-kern alternative.
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> however ...

Thanks.

>> I've also been considering to drop the paging-mode-translate ASSERT(),
>> now that we always have translate == external.
> 
> I'd suggest not making that change here in this bugfix.  I think we do
> want to alter how we do asserts like this, and there are other similarly
> impacted code blocks.

Okay, will look forward to learn what exactly you have in mind.

>> --- a/xen/arch/x86/pv/mm.h
>> +++ b/xen/arch/x86/pv/mm.h
>> @@ -11,10 +11,14 @@ int new_guest_cr3(mfn_t mfn);
>>   */
>>  static inline l1_pgentry_t guest_get_eff_kern_l1e(unsigned long linear)
>>  {
>> +    struct vcpu *curr = current;
>>      l1_pgentry_t l1e;
>>  
>> -    ASSERT(!paging_mode_translate(current->domain));
>> -    ASSERT(!paging_mode_external(current->domain));
>> +    ASSERT(!paging_mode_translate(curr->domain));
>> +    ASSERT(!paging_mode_external(curr->domain));
>> +
>> +    if ( !(curr->arch.flags & TF_kernel_mode) )
> 
> ... pull this out into a variable, like the original code used to do.
> 
> bool user_mode = !(curr->arch.flags & TF_kernel_mode);
> 
> I've forgotten which static checker tripped up over this form, but one
> did IIRC.

I've made the change (will send the result in a minute), but I'm curious
not so much which checker might have taken issue here but why.

Jan