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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.02.19 at 18:02, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 2/8/19 6:44 PM, Jan Beulich wrote:
>>>>> On 08.02.19 at 17:30, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>> On 2/8/19 5:50 PM, Jan Beulich wrote:
>>>>>>> On 08.02.19 at 15:00, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>>>          /* 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.
>> 
>> Sure - I'm not asking to remove everything above, just the if()
>> (and keep what is now the body). That is because
>> - in the enable case the value is false and writing false into it is
>>   benign,
>> - in the disable case you want to actively change from true to
>>   false.
>> 
>>>>> @@ -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.
>> 
>> Well, as always with mm and altp2m code, I'm not the maintainer, so
>> I don't have the final say. It's just that I think unnecessary conditionals
>> would better be avoided, even if they're not on a performance critical
>> path.
>> 
>[...]
>> 
>> Hmm, according to the change further up in this patch, altp2m_active()
>> returns false before v->p2midx can be set to INVALID_ALTP2M (you
>> don't change this property). So (related to the change further up)
>> there's a period of time during which p2m_get_altp2m() will return
>> non-NULL despite altp2m_active() returning false. Afaict, that is.
> 
> 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;
        }

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

Jan


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