[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

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

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 )
              * 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

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.


Xen-devel mailing list



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