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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.02.19 at 14:46, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 2/11/19 12:57 PM, Razvan Cojocaru wrote:
>> On 2/11/19 12:10 PM, Jan Beulich wrote:
>>>>>> On 11.02.19 at 10:13, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>> --- 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);
>>>
>>> With the change you now make to p2m_get_altp2m(), this looks to be
>>> a benign change. Which to me would suggest to either leave the code
>>> alone, or to drop the if() (but - again - not its body) altogether. At
>>> which point the code could be further streamlined, as then the NULL
>>> initializer can go away and the assignment (or then perhaps initializer)
>>> could become "p2m = p2m_get_altp2m(v) ?: p2m_get_hostp2m(d)".
>>> (Generally I'd recommend to leave out the change here, and do the
>>> transformation in a follow-on patch.)
>> 
>> Thanks for noticing, actually this appears to invalidate the whole 
>> purpose of the patch (I should have tested this more before sumbitting 
>> V3, sorry).
>> 
>> The whole point of the new boolean is to have p2m assigned an altp2m 
>> regardless of altp2m_active() (hence the change) - which now no longer 
>> happens. I got carried away with this change.
>> 
>> The fact that this is so easy to get wrong is the reason why I've 
>> preferred the domain_pause() solution. There appears to be no clean way 
>> to fix this otherwise, and if this is so easy to misunderstand it'll 
>> break just as easily with further changes.
>> 
>> I suppose I could just pass the bool along to p2m_get_altp2m() (and 
>> indeed remove the if())...
> 
> I think the best that can be done here is to check if altp2m_active() 
> early in p2m_switch_vcpu_altp2m_by_id() and 
> p2m_switch_domain_altp2m_by_id(), then bail if it's not active. Since 
> these are only called by HVMOP_altp2m_* (and thus serialized by the 
> domain lock), it should be enough WRT HVMOP_altp2m_set_domain_state.

I'm confused: Where do you see the domain lock used there?
Plus I can't see p2m_switch_vcpu_altp2m_by_id() called for
any HVMOP_altp2m_* at all. One of the actual callers is guarded
by altp2m_active(), but the other isn't.

> This of course means reverting p2m_get_altp2m() to its original 
> non-intuitive state of returning a valid altp2m pointer even when 
> altp2m_active() is false.

Yeah, this looks to be unavoidable.

> I see no other way out of this (aside from the domain_pause() fix).

If only that one would have been a complete fix, rather than just a
partial one.

Jan



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