Re: [PATCH v3 10/38] arm/p2m: Move hostp2m init/teardown to individual functions

[Sergej Proskurin <proskurin@sec.in.tum.de>]

Hi Julien,


On 09/02/2016 12:51 PM, Julien Grall wrote:
>
>
> On 02/09/16 10:09, Sergej Proskurin wrote:
>> Hi Julien,
>>
>> On 09/01/2016 07:36 PM, Julien Grall wrote:
>>> Hello Sergej,
>>>
>>> On 16/08/16 23:16, Sergej Proskurin wrote:
>>>> ---
>>>>  xen/arch/arm/p2m.c        | 71
>>>> +++++++++++++++++++++++++++++++++++++++++------
>>>>  xen/include/asm-arm/p2m.h | 11 ++++++++
>>>>  2 files changed, 73 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>> index e859fca..9ef19d4 100644
>>>> --- a/xen/arch/arm/p2m.c
>>>> +++ b/xen/arch/arm/p2m.c
>>>> @@ -1245,27 +1245,53 @@ static void p2m_free_vmid(struct domain *d)
>>>>      spin_unlock(&vmid_alloc_lock);
>>>>  }
>>>>
>>>> -void p2m_teardown(struct domain *d)
>>>> +/* Reset this p2m table to be empty. */
>>>> +void p2m_flush_table(struct p2m_domain *p2m)
>>>>  {
>>>> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>>> -    struct page_info *pg;
>>>> +    struct page_info *page, *pg;
>>>> +    unsigned int i;
>>>> +
>>>> +    if ( p2m->root )
>>>> +    {
>>>> +        page = p2m->root;
>>>> +
>>>> +        /* Clear all concatenated first level pages. */
>>>
>>> s/first/root/
>>>
>>
>> Ok.
>>
>>>> +        for ( i = 0; i < P2M_ROOT_PAGES; i++ )
>>>> +            clear_and_clean_page(page + i);
>>>
>>> Clearing live page table like that is not safe. Each entry (64-bit)
>>> should be written atomically to avoid breaking coherency (e.g if the
>>> MMU
>>> is walking the page table at the same time). However you don't know the
>>> implementation of clear_and_clean_page.
>>
>> The function p2m_flush_table gets called by the altp2m subsystem
>> indrectly through the function p2m_teardown_one when the associated view
>> gets destroyed. In this case we should not worry about crashing the
>> domain, as the altp2m views are not active anyway.
>>
>> The function altp2m_reset calls the function p2m_flush_table directly
>> (with active altp2m views), however, locks the p2m before flushing the
>> table. I did not find any locks on page-granularity, so please provide
>> me with further information about the solution you had in mind.
>
> I never mentioned any locking problem. As said in my previous mail,
> the altp2m may still be in use by another vCPU. So the MMU (i.e the
> hardware) may want do a table walk whilst you modify the entry.
>
> The MMU is reading the entry (64-bit) at once so it also expects the
> entry to be modified atomically. However, you don't know the
> implementation of clean_and_clean_page. The function might write
> 32-bit at the time, which means that the MMU will see bogus entry. At
> best it will lead to a crash, at worst open a security issue.
>

I see your point. Not sure how to fix this, though. I believe that the
described issue would remain if we would use free_domheap_pages.
Instead, maybe we should manually set the value in the translation tables?

Or, what if we flush the TLBs immediately before unmapping the root
pages? This would cause the system to load the mappings from memory and
delay a MMU table walk so that it would potentially resolve the
atomicity issue.

Do you have another suggestion?

>>>
>>> Also, this adds a small overhead when tearing down a p2m because the
>>> clear is not necessary.
>>>
>>
>> The p2m views are cleared very rarely so the overhead is really minimal
>> as it affects clearing the root tables.
>
> You seem to forget the p2m teardown is also called during domain
> destruction.
>
>> Besides the function
>> altp2m_reset calls the function p2m_flush_table and assumes that the
>> root tables are cleared as well. If the root tables would not be cleared
>> at this point, stalled entries in the altp2m views that got wiped out in
>> the hostp2m would make the system unstable.
>
> As I already mentioned in the previous version, this code was not
> present before and based of the description of this commit the patch
> is supposed to only move code around. This is not the case here.
>

I will move the part with the clear_and_clean_page invocation into a
separate patch.

>
>>>>
>>>>      radix_tree_destroy(&p2m->mem_access_settings, NULL);
>>>>  }
>>>>
>>>> -int p2m_init(struct domain *d)
>>>> +int p2m_init_one(struct domain *d, struct p2m_domain *p2m)
>>>>  {
>>>> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>>>      int rc = 0;
>>>>
>>>>      rwlock_init(&p2m->lock);
>>>> @@ -1278,11 +1304,14 @@ int p2m_init(struct domain *d)
>>>>          return rc;
>>>>
>>>>      p2m->max_mapped_gfn = _gfn(0);
>>>> -    p2m->lowest_mapped_gfn = _gfn(ULONG_MAX);
>>>> +    p2m->lowest_mapped_gfn = INVALID_GFN;
>>>
>>> Why this change?
>>>
>>
>> Since we compare the gfn's with INVALID_GFN throughout the code it makes
>> sense to use the macro instead of a hardcoded value.
>
> Please don't do unnecessary change. This patch is complex enough to
> review.

Ok.

Cheers,
~Sergej


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel