Re: [PATCH v3 2/5] arch, arm: add consistency checks to REMOVE p2m changes

[Arianna Avanzini <avanzini.arianna@gmail.com>]

On 03/15/2014 11:19 PM, Julien Grall wrote:
> Hello Arianna,
> 
> Thanks for the patch.
> 

Thank you for the feedback.

> On 15/03/14 20:11, Arianna Avanzini wrote:
>> ---
>>   xen/arch/arm/p2m.c | 33 +++++++++++++++++++++++++++++++--
>>   1 file changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index d00c882..47bf154 100644
>> --- a/xen/arcah/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -243,7 +243,8 @@ static int apply_p2m_changes(struct domain *d,
>>       int rc;
>>       struct p2m_domain *p2m = &d->arch.p2m;
>>       lpae_t *first = NULL, *second = NULL, *third = NULL;
>> -    paddr_t addr;
>> +    p2m_type_t _t;
>> +    paddr_t addr, _maddr = INVALID_PADDR;
>>       unsigned long cur_first_page = ~0,
>>                     cur_first_offset = ~0,
>>                     cur_second_offset = ~0;
>> @@ -252,6 +253,20 @@ static int apply_p2m_changes(struct domain *d,
>>       bool_t populate = (op == INSERT || op == ALLOCATE);
>>       lpae_t pte;
>>
>> +    /*
>> +     * As of now, the lookup is needed only in in case
>> +     * of REMOVE operation, as a consistency check on
>> +     * the existence of a mapping between the machine
>> +     * address and the start guest address given as
>> +     * parameters.
>> +     */
>> +    if (op == REMOVE)
>> +        /*
>> +         * Be sure to lookup before grabbing the p2m_lock,
>> +         * as the p2m_lookup() function holds it too.
>> +         */
>> +        _maddr = p2m_lookup(d, start_gpaddr, &_t);
>> +
> 
> Did you try remove path? apply_p2m_changes is taking p2m->lock which is also
> taken by p2m_lookup. With this solution it will end up to a deadlock.
> 

The lookup is performed before grabbing p2m->lock, as stated in the comment.
I'll certainly remove it as it is useless, thank you for the feedback and for
the many suggestions.

> Anyway, you don't need to use p2m_lookup because you already have all the data
> in pte (if pte.p2m.valid == 1):
>    - pte.p2m.type  = p2m type
>    - pte.p2m.base  = MFN
> 
>>       spin_lock(&p2m->lock);
>>
>>       if ( d != current->domain )
>> @@ -367,9 +382,23 @@ static int apply_p2m_changes(struct domain *d,
>>                       maddr += PAGE_SIZE;
>>                   }
>>                   break;
>> -            case RELINQUISH:
>>               case REMOVE:
>>                   {
>> +                    /*
>> +                     * Ensure that, if we are trying to unmap I/O memory
>> +                     * ranges, the given gfn is p2m_mmio_direct.
>> +                     */
> 
>> +                    if ( t == p2m_mmio_direct ? _t != p2m_mmio_direct : 0 ||
>> +                         paddr_to_pfn(_maddr) == INVALID_MFN ||
> 
> Testing pte.p2m.valid instead of paddr_to(_maddr)... is right answer.
> 
> Moreover, why do you need to check t? Every call to guest_physmap_remove_page is
> done with a valid mfn (I guess it can be enhanced by a BUG_ON(mfn !=
> INVALID_MFN) in this function).
> 

I might be wrong, but it seems to me that apply_p2m_changes() is called with op
== REMOVE also from guest_physmap_remove_page(), and in that case t == p2m_invalid.

> 
>> +                         maddr != _maddr )
> 
> maddr is not incremented during where the page is removed. The next iteration
> will likely fail. You need to increment it in various place.
> 

I actually was checking at each iteration the start maddr against the result of
the lookup performed before the loop, which is a mistake. Sorry, and thank you
again for the feedback.

>> +                    {
>> +                        count++;
>> +                        break;
> 
> IHMO, skipping the page is totally wrong. You should return an error here.
> 
> Regards,
> 


-- 
/*
 * Arianna Avanzini
 * avanzini.arianna@gmail.com
 * 73628@studenti.unimore.it
 */