Re: [Xen-devel] [PATCH v5 10/15] x86/altp2m: add remaining support routines.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.07.15 at 00:32, <ravi.sahita@xxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>>Sent: Thursday, July 16, 2015 2:34 AM
>>
>>>>> On 16.07.15 at 11:16, <ravi.sahita@xxxxxxxxx> wrote:
>>>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>>>>Sent: Tuesday, July 14, 2015 7:32 AM
>>>>>>> On 14.07.15 at 02:14, <edmund.h.white@xxxxxxxxx> wrote:
>>>>> @@ -2965,9 +3003,15 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>>>>unsigned long gla,
>>>>>          if ( npfec.write_access )
>>>>>          {
>>>>>              paging_mark_dirty(currd, mfn_x(mfn));
>>>>> +            /* If p2m is really an altp2m, unlock here to avoid
>>>>> + lock
>>> ordering
>>>>> +             * violation when the change below is propagated from
>>>>> + host p2m
>>> */
>>>>> +            if ( ap2m_active )
>>>>> +                __put_gfn(p2m, gfn);
>>>>>              p2m_change_type_one(currd, gfn, p2m_ram_logdirty,
>>>>> p2m_ram_rw);
>>>>
>>>>And this won't result in any races?
>>>
>>> No
>>
>>To be honest I expected a little more than just "no" here. Now I have to ask -
>>why?
>>
> 
> Yes, I should have described it more than that :-)  so this part of the code 
> is handling the log dirty transition of the page, and this page permission 
> transition happens always on the hostp2m. Given the way the locking order is 
> setup (hostp2m->altp2m-list-lock->altp2m and there was a separate writeup and 
> discussion with George on that), at this point in this sequence there is a 
> p2m lock (whether it's a hostp2m or altp2m lock depends on the mode of the 
> domain) - the reason we have to drop the lock here first is due to what 
> happens next; the permission changes in hostp2m will be serially propagated 
> to altp2ms and not dropping the lock here would cause a locking order 
> violation. Hope that clarifies. 

Sadly it doesn't at all: You re-explain why you need to drop the lock,
while you fail to say anything on why this won't cause a race.

>>>>> +long p2m_init_altp2m_by_id(struct domain *d, uint16_t idx) {
>>>>> +    long rc = -EINVAL;
>>>>
>>>>Why long (for both variable and function return type)? (More of these
>>>>in functions below.)
>>>
>>> Because the error variable in the code that calls these (in hvm.c) is
>>> a long, and you had given feedback earlier to propagate the returns
>>> from these functions through that calling code.
>>
>>I don't see the connection. The function only returns zero or -E...
>>values, so why would its return type be "long"?
>>
> 
> do_hvm_op declares a rc that is of type "long" and hence this returns a 
> "long"

What type your caller(s) return is of no interest at all here: What
would you do if you had multiple callers with differing return types?
A function's return type should be chosen based on the range of
values it may return, and the result possibly widened to not yield
inefficient code (like in some of the uint16_t cases elsewhere in the
series would be necessary).

>>>>> +void p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
>>>>> +                                 mfn_t mfn, unsigned int page_order,
>>>>> +                                 p2m_type_t p2mt, p2m_access_t
>>>>> +p2ma) {
>>>>> +    struct p2m_domain *p2m;
>>>>> +    p2m_access_t a;
>>>>> +    p2m_type_t t;
>>>>> +    mfn_t m;
>>>>> +    uint16_t i;
>>>>> +    bool_t reset_p2m;
>>>>> +    unsigned int reset_count = 0;
>>>>> +    uint16_t last_reset_idx = ~0;
>>>>> +
>>>>> +    if ( !altp2m_active(d) )
>>>>> +        return;
>>>>> +
>>>>> +    altp2m_list_lock(d);
>>>>> +
>>>>> +    for ( i = 0; i < MAX_ALTP2M; i++ )
>>>>> +    {
>>>>> +        if ( d->arch.altp2m_eptp[i] == INVALID_MFN )
>>>>> +            continue;
>>>>> +
>>>>> +        p2m = d->arch.altp2m_p2m[i];
>>>>> +        m = get_gfn_type_access(p2m, gfn_x(gfn), &t, &a, 0, NULL);
>>>>> +
>>>>> +        reset_p2m = 0;
>>>>> +
>>>>> +        /* Check for a dropped page that may impact this altp2m */
>>>>> +        if ( mfn_x(mfn) == INVALID_MFN &&
>>>>> +             gfn_x(gfn) >= p2m->min_remapped_gfn &&
>>>>> +             gfn_x(gfn) <= p2m->max_remapped_gfn )
>>>>> +            reset_p2m = 1;
>>>>
>>>>Considering that this looks like an optimization, what's the downside
>>>>of possibly having min=0 and max=<end-of-address-space>? I.e.
>>>>can there a long latency operation result that's this way a guest can 
>>>>effect?
>>>>
>>>
>>> ... A p2m is a gfn->mfn map, amongst other things. There is a reverse
>>> mfn->gfn map, but that is only valid for the host p2m. Unless the
>>> remap altp2m hypercall is used, the gfn->mfn map in every altp2m
>>> mirrors the gfn->mfn map in the host p2m (or a subset thereof, due to
>>> lazy-copy), so handling removal of an mfn from a guest is simple: do a
>>> reverse look up for the host p2m and mark the relevant gfn as invalid,
>>> then do the same for every altp2m where that gfn is currently valid.
>>>
>>> Remap changes things: it says take gfn1 and replace ->mfn with the
>>> ->mfn of gfn2. Here is where the optimization is used and the  invalidate
>>logic is:
>>> record the lowest and highest gfn2's that have been used in remap ops;
>>> if an mfn is dropped from the hostp2m, for the purposes of altp2m
>>> invalidation, see if the gfn derived from the host p2m reverse lookup
>>> falls within the range of used gfn2's. If it does, an invalidation is
>>> required. Which is why min and max are inited the way they are - hope
>>> the explanation clarifies this optimization.
>>
>>Sadly it doesn't, it just re-states what I already understood and doesn't
>>answer the question: What happens if min=0 and max=<end-of-address-
>>space>? I.e. can the guest nullify the optimization by careful fiddling 
> issuing
>>some of the new hypercalls, and if so will this have any negative impact on 
> the
>>hypervisor? I'm asking this from a security standpoint ...
>>
> 
> To take that exact case, If min=0 and max=<end of address space> then any 
> hostp2m change where the first mfn is dropped, will cause all altp2ms to be 
> reset even if the mfn dropped doesn't affect altp2ms at all, which wont serve 
> as an optimization at all - Hope that clarifies.

Again - no. I understand the optimization is gone then. But what's the
effect? I.e. will the guest, by extending this range to be arbitrarily
wide, be able to cause a long latency hypervisor operation (i.e. a DoS)?

>>Nor do I find my question answered why max can't be initialized to zero:
>>You don't care whether max is a valid GFN when a certain GFN doesn't fall in
>>the (then empty) [min, max] range. What am I missing?
> 
> Since 0 is a valid GFN so we cannot initialize min or max to 0 - since 
> matching this condition (gfn_x(gfn) >= p2m->min_remapped_gfn && gfn_x(gfn) <= 
> p2m->max_remapped_gfn) will cause a reset (throw away) of the altp2m to 
> rebuild it from the hostp2m. So essentially what is being done here is the 
> range is the non-existent set to start with, unless some altp2m changes 
> occur, 
> and then it is grown to be the smallest set around the gfns affected.

Again you just re-state what was already clear, yet you neglect
answering the actual question. Taking what you wrote above,
when max=0 (and min=INVALID_GFN), then

        gfn_x(gfn) >= p2m->min_remapped_gfn &&
         gfn_x(gfn) <= p2m->max_remapped_gfn

will be false for any value of gfn; in fact the "max" part won't
even be looked at because the "min" part will already be false
for any valid gfn, i.e. only the INVALID_GFN case would make it
to the "max" part.

Jan

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