Re: [Xen-devel] [PATCH v3 7/8] x86/mm: handle foreign mappings in p2m_entry_modify

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 2/18/19 11:05 AM, Roger Pau Monné wrote:
> On Fri, Feb 15, 2019 at 07:15:53PM +0100, George Dunlap wrote:
>>
>>
>>> On Feb 15, 2019, at 2:18 PM, Roger Pau Monne <roger.pau@xxxxxxxxxx> wrote:
>>>
>>> So that the specific handling can be removed from
>>> atomic_write_ept_entry and be shared with npt and shadow code.
>>>
>>> This commit also removes the check that prevent non-ept PVH dom0 from
>>> mapping foreign pages.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>
>> Mostly looks good; just a few comments...
>>>
>>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>>> index f4ec2becbd..1687b31571 100644
>>> --- a/xen/include/asm-x86/p2m.h
>>> +++ b/xen/include/asm-x86/p2m.h
>>> @@ -932,11 +932,14 @@ int p2m_set_ioreq_server(struct domain *d, unsigned 
>>> int flags,
>>> struct hvm_ioreq_server *p2m_get_ioreq_server(struct domain *d,
>>>                                               unsigned int *flags);
>>>
>>> -static inline void p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt,
>>> -                                    p2m_type_t ot, unsigned int level)
>>> +static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt,
>>> +                                   p2m_type_t ot, mfn_t nfn, mfn_t ofn,
>>> +                                   unsigned int level)
>>> {
>>> -    if ( level != 1 || nt == ot )
>>> -        return;
>>> +    BUG_ON(level > 1 && (nt == p2m_ioreq_server || nt == p2m_map_foreign));
>>> +
>>> +    if ( level != 1 || (nt == ot && mfn_eq(nfn, ofn)) )
>>> +        return 0;
>>>
>>>     switch ( nt )
>>>     {
>>> @@ -948,6 +951,14 @@ static inline void p2m_entry_modify(struct p2m_domain 
>>> *p2m, p2m_type_t nt,
>>>         p2m->ioreq.entry_count++;
>>>         break;
>>>
>>> +    case p2m_map_foreign:
>>> +        BUG_ON(!mfn_valid(nfn));
>>
>> Since we’re going to be returning errors anyway, why not retain the original 
>> functionality of returning -EINVAL in this case, rather than BUG_ON?
> 
> Ack. I added the BUG_ON because trying to add a foreign entry with an
> invalid mfn means something else went wrong in the caller, since it
> should not be possible to perform such action. As you pointed out
> however there's no reason to panic, since an error can be returned to
> the caller.
> 
> Would you be fine with also adding an ASSERT_UNREACHABLE before
> returning the error?
> 
>>> +
>>> +        if ( !page_get_owner_and_reference(mfn_to_page(nfn)) )
>>> +            return -EBUSY;
>>> +
>>> +        break;
>>> +
>>>     default:
>>>         break;
>>>     }
>>> @@ -959,9 +970,16 @@ static inline void p2m_entry_modify(struct p2m_domain 
>>> *p2m, p2m_type_t nt,
>>>         p2m->ioreq.entry_count--;
>>>         break;
>>>
>>> +    case p2m_map_foreign:
>>> +        BUG_ON(!mfn_valid(ofn));
>>
>> If somehow this happened, then the bug isn’t here but somewhere else; 
>> continuing on won’t make things any worse than they would be if this page 
>> weren’t removed.  I think this should probably be an ASSERT() (to help 
>> narrow down where a bug may have come from), followed by a simple return.  
>> Likely the worst that would happen here is an un-killable domain; no need to 
>> crash production servers in this case.
> 
> Ack, I think what I suggested above should also be used here:
> 
> if ( !mfn_valid(ofn) )
> {
>     ASSERT_UNREACHABLE();
>     return -EINVAL;
> }

Yes to both ASSERTs.  If something has broken one of our assumptions,
the sooner we find it out (during development) the better.

Thanks,
 -George

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