On 10/01/2018 12:11 PM, Razvan Cojocaru wrote:
>
>
> On 10/1/18 1:39 PM, Jan Beulich wrote:
>>>>> On 01.10.18 at 11:58, <rcojoc...@bitdefender.com> wrote:
>>> Changes since V1:
>>> - Removed unnecessary p2m_lock() in p2m_init_altp2m_ept().
>>
>> This was a step in the right direction, but ...
>>
>>> static void ept_enable_pml(struct p2m_domain *p2m)
>>> {
>>> + struct domain *d = p2m->domain;
>>> +
>>> /* Domain must have been paused */
>>> - ASSERT(atomic_read(&p2m->domain->pause_count));
>>> + ASSERT(atomic_read(&d->pause_count));
>>>
>>> /*
>>> * No need to return whether vmx_domain_enable_pml has succeeded, as
>>> * ept_p2m_type_to_flags will do the check, and write protection will
>>> be
>>> * used if PML is not enabled.
>>> */
>>> - if ( vmx_domain_enable_pml(p2m->domain) )
>>> + if ( vmx_domain_enable_pml(d) )
>>> return;
>>>
>>> /* Enable EPT A/D bit for PML */
>>> - p2m->ept.ad = 1;
>>> - vmx_domain_update_eptp(p2m->domain);
>>> + ept_set_ad_sync(p2m, true);
>>> +
>>> + vmx_domain_update_eptp(d);
>>> }
>>>
>>> static void ept_disable_pml(struct p2m_domain *p2m)
>>> {
>>> + struct domain *d = p2m->domain;
>>> +
>>> /* Domain must have been paused */
>>> - ASSERT(atomic_read(&p2m->domain->pause_count));
>>> + ASSERT(atomic_read(&d->pause_count));
>>>
>>> - vmx_domain_disable_pml(p2m->domain);
>>> + vmx_domain_disable_pml(d);
>>>
>>> /* Disable EPT A/D bit */
>>> - p2m->ept.ad = 0;
>>> - vmx_domain_update_eptp(p2m->domain);
>>> + ept_set_ad_sync(p2m, false);
>>> +
>>> + vmx_domain_update_eptp(d);
>>> }
>>
>> These two functions used to be called with the p2m lock held,
>> while now they aren't anymore. Afaict this introduces a race
>> where the opposite ept_set_ad_sync() may be called before
>> an original one was follow by the respective
>> vmx_domain_update_eptp(), resulting in the A/D enable bit
>> being set the wrong way round in the end.
>>
>> I realize that George did already point out that this is sort of
>> ugly a situation, but the fixing of the issue here shouldn't
>> introduce a new race. What's wrong with retaining the
>> host p2m lock in p2m_{en,dis}able_hardware_log_dirty()?
>> ept_set_ad_sync() then simply wouldn't acquire/release that
>> one, but just the altp2m ones.
>
> That is fine with be, in fact the whole change has been prompted by
> George's remark that "there would something a bit funny here about
> grabbing the main p2m lock in p2m.c, and then grabbing altp2m locks
> within the function". If, after these comments, he doesn't mind the
> scenario then I'll do that in V3.
I think I would rather grab the main p2m locks in
ept_{enable,disable}_pml(). Wouldn't hurt to have an
ASSERT(p2m_is_locked_by_me()) in ept_set_ad_sync() as well.
Thanks,
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
