Re: [Xen-devel] [PATCH for-4.12 V2] x86/altp2m: fix HVMOP_altp2m_set_domain_state race

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 2/8/19 5:50 PM, Jan Beulich wrote:
>>>> On 08.02.19 at 15:00, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4525,7 +4525,7 @@ static int do_altp2m_op(
>>      case HVMOP_altp2m_set_domain_state:
>>      {
>>          struct vcpu *v;
>> -        bool_t ostate;
>> +        bool ostate, nstate;
>>  
>>          if ( nestedhvm_enabled(d) )
>>          {
>> @@ -4534,12 +4534,16 @@ static int do_altp2m_op(
>>          }
>>  
>>          ostate = d->arch.altp2m_active;
>> -        d->arch.altp2m_active = !!a.u.domain_state.state;
>> +        nstate = !!a.u.domain_state.state;
> 
> No need for !! here.

I'll remove it.

>>          /* 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.

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

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

There's an if() above (not shown in your reply) that says "if ( idx !=
vcpu_altp2m(v).p2midx )", so, indeed, by the time we end up here we can
reasonably assume that altp2m_active(d) will return true.

I've just put in "altp2m_active(d)" to make sure there's absolutely no
functional change here, but AFAICT it can be safely replaced with "true".


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel