[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

 


Rackspace

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