[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

>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
>>>>>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 
>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 
>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>?
>>>>>can there a long latency operation result that's this way a guest can
>>>> ... 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
>>>> 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
>>>space>? I.e. can the guest nullify the optimization by careful
>> 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
>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 
>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?



Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.