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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

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.
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.
I see no other way out of this (aside from the domain_pause() fix).


Thanks,
Razvan

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