Re: [Xen-devel] [PATCH] xen/x86: clear per cpu stub page information in cpu_smpboot_free()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/01/2020 12:26, Jürgen Groß wrote:
> On 08.01.20 13:23, Wei Liu wrote:
>> On Wed, Jan 08, 2020 at 01:18:46PM +0100, Jürgen Groß wrote:
>>> On 08.01.20 13:16, Wei Liu wrote:
>>>> On Wed, Jan 08, 2020 at 12:01:48PM +0100, Juergen Gross wrote:
>>>>> cpu_smpboot_free() removes the stubs for the cpu going offline,
>>>>> but it
>>>>> isn't clearing the related percpu variables. This will result in
>>>>> crashes when a stub page is released due to all related cpus gone
>>>>> offline and one of those cpus going online later.
>>>>>
>>>>> Fix that by clearing stubs.addr and stubs.mfn in order to allocate a
>>>>> new stub page when needed.
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
>>>>> ---
>>>>>    xen/arch/x86/smpboot.c | 2 ++
>>>>>    1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
>>>>> index 7e29704080..46c0729214 100644
>>>>> --- a/xen/arch/x86/smpboot.c
>>>>> +++ b/xen/arch/x86/smpboot.c
>>>>> @@ -945,6 +945,8 @@ static void cpu_smpboot_free(unsigned int cpu,
>>>>> bool remove)
>>>>>                                 (per_cpu(stubs.addr, cpu) |
>>>>> ~PAGE_MASK) + 1);
>>>>>            if ( i == STUBS_PER_PAGE )
>>>>>                free_domheap_page(mfn_to_page(mfn));
>>>>> +        per_cpu(stubs.addr, cpu) = 0;
>>>>> +        per_cpu(stubs.mfn, cpu) = 0;
>>>>
>>>> Shouldn't the mfn be set to INVALID_MFN instead?
>>>
>>> This would require modifying alloc_stub_page():
>>>
>>>      if ( *mfn )
>>>          pg = mfn_to_page(_mfn(*mfn));
>>>      else
>>
>> OK. I think the chance of allocating mfn 0 from the allocator is
>> exceedingly low, so I certainly have no objection to reset it to 0.
>
> The chance should be exactly zero. Otherwise we'd have a big problem
> due to L1TF...

Also MFN 0 contains the IVT and BDA.

The IVT at least must remain valid at all times (even on EFI systems),
because AP boot needs to have at least a no-op NMI handler.  A base of 0
and limit of 0xffff is the architectural INIT/RESET state for IDTR. 
(Also for the other system data structures, but they have to be moved
elsewhere before they can be used).

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel