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

["Sahita, Ravi" <ravi.sahita@xxxxxxxxx>]

>From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>Sent: Sunday, July 19, 2015 11:53 PM
>
>>>> 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.
>

It only drops the lock on the altp2m, which is no longer required in this 
function anyway. The important aspect is that there is still a lock held on the 
host p2m, and that is dropped after the log-dirty updates, as it would be in 
the non-altp2m case (maybe that was the part I should have explained clearly in 
the para above). Does that clarify or do you see a particular race condition 
here? (We don't ).

>>>>>> +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).
>

What do you suggest the return type be?

>>>>>> +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
>>>space>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)?
>

The extent of the range affects the likelihood of an invalidation. It has no 
impact on the cost of an invalidation (so no its not a DoS issue).
I'm not sure what change you are suggesting here or just clarification (if you 
think this optimization is confusing perhaps some documentation of this 
optimization will help?)


>>>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
>> p2m->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.
>

You are suggesting an alternative which will work, but what we have will also 
produce the same results, and the results are correct for both cases - so can 
we go with the approach we have taken currently?

Ravi



>Jan



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