Re: [Xen-devel] [PATCH v3 6/8] p2m: change write_p2m_entry to return an error code

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 2/18/19 11:01 AM, Roger Pau Monné wrote:
> On Fri, Feb 15, 2019 at 06:48:25PM +0100, George Dunlap wrote:
>>
>>> On Feb 15, 2019, at 2:18 PM, Roger Pau Monne <roger.pau@xxxxxxxxxx> wrote:
>>>
>>> This is in preparation for also changing p2m_entry_modify to return an
>>> error code.
>>>
>>> No functional change intended.
>>
>> I think you need to explain wheny/why you’re using BUG_ON() rather than 
>> ASSERT() or passing the caller up the stack.
>>
>>
>> Just in general:
>>
>> * Passing things up the stack should be used when the caller is already 
>> expecting to handle errors, and the state when the error was discovered 
>> isn’t broken, or too hard to fix.
>>
>> * BUG_ON() should be used when you can’t pass things up the stack, and 
>> continuing would certainly cause a vulnerability.
>>
>> * ASSERT() should be used when continuing might work, or might have an 
>> effect later whose badness is equal or less than that of a host crash; OR 
>> whose truth can be clearly observed from the code directly surrounding it.
>>
>> For instance...
>>
>>> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
>>> index 04e9d81cf6..44abd65999 100644
>>> --- a/xen/arch/x86/mm/p2m-pt.c
>>> +++ b/xen/arch/x86/mm/p2m-pt.c
>>> @@ -202,7 +202,7 @@ p2m_next_level(struct p2m_domain *p2m, void **table,
>>>         new_entry = l1e_from_mfn(mfn, P2M_BASE_FLAGS | _PAGE_RW);
>>>
>>>         p2m_add_iommu_flags(&new_entry, level, 
>>> IOMMUF_readable|IOMMUF_writable);
>>> -        p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
>>> +        BUG_ON(p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level 
>>> + 1));
>>
>> In this case, a few lines above we have `return -ENOMEM`; so:
>> 1. The caller is expecting to handle error values, and
>> 2. There’s no unusual state to try to clean up.
> 
> I'm not that familiar with the p2m code, but there's a call to
> p2m_alloc_ptp just further up, and while I think failing here would
> only imply that a page has been added to p2m->pages but has not
> actually been used in the p2m page tables because the addition of the
> entry failed.
> 
> I think adding an entry that expands the page table structure should
> never fail, and hence I think the following:
> 
> rc = p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
> if ( rc )
> {
>     ASSERT_UNREACHABLE();
>     return rc;
> }
> 
> Would be the best way to handle the return code from write_p2m_entry
> in p2m_next_level.

That works for me.

>>>     }
>>>     else
>>>         ASSERT(flags & _PAGE_PRESENT);
>>> @@ -321,7 +321,7 @@ static int p2m_pt_set_recalc_range(struct p2m_domain 
>>> *p2m,
>>>             if ( (l1e_get_flags(e) & _PAGE_PRESENT) && !needs_recalc(l1, e) 
>>> )
>>>             {
>>>                 set_recalc(l1, e);
>>> -                p2m->write_p2m_entry(p2m, first_gfn, pent, e, level);
>>> +                BUG_ON(p2m->write_p2m_entry(p2m, first_gfn, pent, e, 
>>> level));
>>>             }
>>
>> Again here; theoretically, the only change has been that RECALC_FLAGS have 
>> been added.
>>
>> And so on.
>>
>> Thoughts?  (Looking for input from Jan here as well.)
> 
> As you say here and above, the only caller that's expected to fail is
> p2m_pt_set_entry, the rest just do recalc or add intermediate entries
> to the p2m, which must always succeed.
> 
>> If not, it seems like we should also be modifying the places in p2m-ept.c to 
>> do BUG_ON() rather than ASSERT()’ing that the page write succeeded.
> 
> I can apply the same treatment that's done in p2m-ept.c, so that we
> have consistency between both implementations.

Sounds good.

Thanks!
 -George

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