|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-4.12 V2] x86/altp2m: fix HVMOP_altp2m_set_domain_state race
On 2/8/19 6:44 PM, Jan Beulich wrote:
>>>> On 08.02.19 at 17:30, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> On 2/8/19 5:50 PM, Jan Beulich wrote:
>>>>>> On 08.02.19 at 15:00, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>> /* If the alternate p2m state has changed, handle appropriately */
>>>> - if ( d->arch.altp2m_active != ostate &&
>>>> + if ( nstate != ostate &&
>>>> (ostate || !(rc = p2m_init_altp2m_by_id(d, 0))) )
>>>> {
>>>> + /* First mark altp2m as disabled, then altp2m_vcpu_destroy().
>>>> */
>>>> + if ( ostate )
>>>> + d->arch.altp2m_active = false;
>>>
>>> Why the if()? In the opposite case you'd simply write false into
>>> what already holds false.
>>
>> The value written into d->arch.altp2m_active is not the point here. The
>> point is that if ( ostate ), then we are disabling altp2m (because the
>> if above this one makes sure ostate != nstate).
>>
>> So in the disable case, first I wanted to set d->arch.altp2m_active to
>> false (which immediately causes altp2m_active(d) to return false as
>> well), and then actually perform the work.
>
> Sure - I'm not asking to remove everything above, just the if()
> (and keep what is now the body). That is because
> - in the enable case the value is false and writing false into it is
> benign,
> - in the disable case you want to actively change from true to
> false.
>
>>>> @@ -4550,7 +4554,14 @@ static int do_altp2m_op(
>>>>
>>>> if ( ostate )
>>>> p2m_flush_altp2m(d);
>>>> + else
>>>> + /*
>>>> + * Wait until altp2m_vcpu_initialise() is done before
>>>> marking
>>>> + * altp2m as being enabled for the domain.
>>>> + */
>>>> + d->arch.altp2m_active = true;
>>>
>>> Similarly here you could omit the "else" and simply store "nstate" afaict.
>>
>> As above, in the enable case I wanted to first setup altp2m on all VCPUs
>> with altp2m_vcpu_initialise(), and only after that set
>> d->arch.altp2m_active = true.
>>
>> In summary, if ( ostate ) we want to set d->arch.altp2m_active before
>> the for (we're disabling altp2m), and if ( !ostate ) (which is the else
>> above) we want to set d->arch.altp2m_active after said for.
>>
>> We can indeed store nstate. I just thought things look clearer with
>> "true" and "false", but if you prefer there's no problem assigning nstate.
>
> Well, as always with mm and altp2m code, I'm not the maintainer, so
> I don't have the final say. It's just that I think unnecessary conditionals
> would better be avoided, even if they're not on a performance critical
> path.
>
>>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>>> @@ -2150,13 +2150,13 @@ static bool_t vmx_is_singlestep_supported(void)
>>>> return !!cpu_has_monitor_trap_flag;
>>>> }
>>>>
>>>> -static void vmx_vcpu_update_eptp(struct vcpu *v)
>>>> +static void vmx_vcpu_update_eptp(struct vcpu *v, bool altp2m_enabled)
>>>> {
>>>> struct domain *d = v->domain;
>>>> struct p2m_domain *p2m = NULL;
>>>> struct ept_data *ept;
>>>>
>>>> - if ( altp2m_active(d) )
>>>> + if ( altp2m_enabled )
>>>> p2m = p2m_get_altp2m(v);
>>>> if ( !p2m )
>>>> p2m = p2m_get_hostp2m(d);
>>>
>>> Is this an appropriate transformation? That is, can there not be
>>> any domains for which altp2m_active() returns false despite
>>> altp2m_enabled being true? (Looking at p2m_get_altp2m() I can't
>>> really judge whether index would always be INVALID_ALTP2M in
>>> such a case.)
>>
>> Yes, it should be completely safe (please see below for details).
>>
>>>> --- a/xen/arch/x86/mm/p2m.c
>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>> @@ -2332,7 +2332,7 @@ bool_t p2m_switch_vcpu_altp2m_by_id(struct vcpu *v,
>>>> unsigned int idx)
>>>> atomic_dec(&p2m_get_altp2m(v)->active_vcpus);
>>>> vcpu_altp2m(v).p2midx = idx;
>>>> atomic_inc(&p2m_get_altp2m(v)->active_vcpus);
>>>> - altp2m_vcpu_update_p2m(v);
>>>> + altp2m_vcpu_update_p2m(v, altp2m_active(d));
>>>> }
>>>> rc = 1;
>>>> }
>>>> @@ -2573,7 +2573,7 @@ int p2m_switch_domain_altp2m_by_id(struct domain *d,
>>>> unsigned int idx)
>>>> atomic_dec(&p2m_get_altp2m(v)->active_vcpus);
>>>> vcpu_altp2m(v).p2midx = idx;
>>>> atomic_inc(&p2m_get_altp2m(v)->active_vcpus);
>>>> - altp2m_vcpu_update_p2m(v);
>>>> + altp2m_vcpu_update_p2m(v, altp2m_active(d));
>>>> }
>>>
>>> In both cases, isn't altp2m_active() going to return true anyway
>>> when we get there (related to the question above)?
>>
>> Yes, it will return true. In order for it to return false,
>> altp2m_vcpu_destroy() would have had to run on that VCPU, which (among
>> other things) calls altp2m_vcpu_reset(), which does v->p2midx =
>> INVALID_ALTP2M.
>
> Hmm, according to the change further up in this patch, altp2m_active()
> returns false before v->p2midx can be set to INVALID_ALTP2M (you
> don't change this property). So (related to the change further up)
> there's a period of time during which p2m_get_altp2m() will return
> non-NULL despite altp2m_active() returning false. Afaict, that is.
Right, I now understand what you meant by removing "if ( ostate )" -
you'd like d->arch.altp2m_active to only be set _after_ the for, for the
enable, as well as for the disable case.
However, I wanted to keep both if()s to avoid another problem with this
code:
3655 /*
3656 * If the guest has the ability to switch EPTP without an exit,
3657 * figure out whether it has done so and update the altp2m data.
3658 */
3659 if ( altp2m_active(v->domain) &&
3660 (v->arch.hvm.vmx.secondary_exec_control &
3661 SECONDARY_EXEC_ENABLE_VM_FUNCTIONS) )
3662 {
3663 unsigned long idx;
3664
3665 if ( v->arch.hvm.vmx.secondary_exec_control &
3666 SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS )
3667 __vmread(EPTP_INDEX, &idx);
3668 else
3669 {
3670 unsigned long eptp;
3671
3672 __vmread(EPT_POINTER, &eptp);
3673
3674 if ( (idx = p2m_find_altp2m_by_eptp(v->domain, eptp)) ==
3675 INVALID_ALTP2M )
3676 {
3677 gdprintk(XENLOG_ERR, "EPTP not found in alternate
p2m list\n");
3678 domain_crash(v->domain);
3679
3680 return;
3681 }
3682 }
So for the disable case, I wanted altp2m_active(v->domain) to return
false immediately so that this code won't be triggered. Otherwise,
assuming the following scenario:
1. altp2m_vcpu_destroy() is called for a VCPU _after_ the if (
altp2m_active(v->domain) ) check is performed, but before
__vmread(EPT_POINTER, &eptp).
2. The code now __vmread()s the host EPTP and crashes.
Now, you're right that p2m_get_altp2m() may hand over a pointer to an
altp2m between the moment where d->arch.altp2m_active is false and the
point where v->p2midx actually becomes INVALID_ALTP2M with my code; but
I think it can be argued that I should rather fix p2m_get_altp2m(), to
return NULL if altp2m_active() == false and keep the if()s (which I've
hopefully been more eloquent about now).
Is that OK?
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |