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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

> > 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.
> No, certainly not. Exactly because of ...
>
> > However, I wanted to keep both if()s to avoid another problem with this
> > code:
> > [...]
> > 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:
> ... this. Apparently "and keep what is now the body" was still not clear
> enough.
>
> Taking your original code, I mean
>
>          if ( nstate != ostate &&
>               (ostate || !(rc = p2m_init_altp2m_by_id(d, 0))) )
>          {
>              /* First mark altp2m as disabled, then altp2m_vcpu_destroy(). */
>              d->arch.altp2m_active = false;
>
>              for_each_vcpu( d, v )
>              [...]
>
>              if ( ostate )
>                  p2m_flush_altp2m(d);
>              /*
>               * Wait until altp2m_vcpu_initialise() is done before marking
>               * altp2m as being enabled for the domain.
>               */
>              d->arch.altp2m_active = nstate;
>          }
Ah! Thanks for the clarification.

> > 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).
> Yes, that looks to be an option, provided it doesn't violate assumptions
> elsewhere.
It doesn't, AFAICT. Additionally, it can be argued that if code relies 
on p2m_get_altp2m() returning not-NULL when !altp2m_active(), said code 
is broken.

Thanks,
Razvan

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