[PATCH v2 for 4.5] x86/VPMU: Clear last_vcpu when destroying VPMU

[PATCH v2 for 4.5] x86/VPMU: Clear last_vcpu when destroying VPMU

[Boris Ostrovsky]

We need to make sure that last_vcpu is not pointing to VCPU whose
VPMU is being destroyed. Otherwise we may try to dereference it in
the future, when VCPU is gone.

We have to do this via IPI since otherwise there is a (somewheat
theoretical) chance that between test and subsequent clearing
of last_vcpu the remote processor (i.e. vpmu->last_pcpu) might do
both load_vpmu() and then save_vpmu() for another VCPU. The former
will clear last_vcpu and the latter will set it to something else.

Performing this operation via IPI will guarantee that nothing can
happen on the remote processor between testing and clearing of
last_vcpu.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/hvm/vpmu.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

Changes in v2:
 * Test last_vcpu locally before IPI
 * Don't handle local pcpu as a special case --- on_selected_cpus
   will take care of that
 * Dont't cast variables unnecessarily

diff --git a/xen/arch/x86/hvm/vpmu.c b/xen/arch/x86/hvm/vpmu.c
index 1df74c2..37f0d9f 100644
--- a/xen/arch/x86/hvm/vpmu.c
+++ b/xen/arch/x86/hvm/vpmu.c
@@ -247,10 +247,30 @@ void vpmu_initialise(struct vcpu *v)
     }
 }
 
+static void vpmu_clear_last(void *arg)
+{
+    if ( this_cpu(last_vcpu) == arg )
+        this_cpu(last_vcpu) = NULL;
+}
+
 void vpmu_destroy(struct vcpu *v)
 {
     struct vpmu_struct *vpmu = vcpu_vpmu(v);
 
+    if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
+        return;
+
+    /*
+     * Need to clear last_vcpu in case it points to v.
+     * We can check here non-atomically whether it is 'v' since
+     * last_vcpu can never become 'v' again at this point.
+     * We will test it again in vpmu_clear_last() with interrupts
+     * disabled to make sure we don't clear someone else.
+     */
+    if ( per_cpu(last_vcpu, vpmu->last_pcpu) == v )
+        on_selected_cpus(cpumask_of(vpmu->last_pcpu),
+                         vpmu_clear_last, v, 1);
+
     if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_destroy )
         vpmu->arch_vpmu_ops->arch_vpmu_destroy(v);
 }
-- 
1.7.1

Re: [PATCH v2 for 4.5] x86/VPMU: Clear last_vcpu when destroying VPMU

[Jan Beulich]

>>> On 15.12.14 at 23:24, <boris.ostrovsky@oracle.com> wrote:
> We need to make sure that last_vcpu is not pointing to VCPU whose
> VPMU is being destroyed. Otherwise we may try to dereference it in
> the future, when VCPU is gone.
> 
> We have to do this via IPI since otherwise there is a (somewheat
> theoretical) chance that between test and subsequent clearing
> of last_vcpu the remote processor (i.e. vpmu->last_pcpu) might do
> both load_vpmu() and then save_vpmu() for another VCPU. The former
> will clear last_vcpu and the latter will set it to something else.

Apart from the question of using cmpxchg instead of the IPI (I can
see with the current model that using an IPI is the only clean way,
i.e. the alternative - if usable - would mean altering existing logic
too), please be sure such descriptions are accurate: There are no
functions with the names you mention, and vpmu_load() alters
last_vcpu only indirectly (via vpmu_save_force()).

>  void vpmu_destroy(struct vcpu *v)
>  {
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
>  
> +    if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
> +        return;

This appears to make unnecessary the respective checks in
amd_vpmu_destroy() and core2_vpmu_destroy().

Jan

Re: [PATCH v2 for 4.5] x86/VPMU: Clear last_vcpu when destroying VPMU

[Boris Ostrovsky]

On 12/16/2014 04:31 AM, Jan Beulich wrote:
>>>> On 15.12.14 at 23:24, <boris.ostrovsky@oracle.com> wrote:
>> We need to make sure that last_vcpu is not pointing to VCPU whose
>> VPMU is being destroyed. Otherwise we may try to dereference it in
>> the future, when VCPU is gone.
>>
>> We have to do this via IPI since otherwise there is a (somewheat
>> theoretical) chance that between test and subsequent clearing
>> of last_vcpu the remote processor (i.e. vpmu->last_pcpu) might do
>> both load_vpmu() and then save_vpmu() for another VCPU. The former
>> will clear last_vcpu and the latter will set it to something else.
> Apart from the question of using cmpxchg instead of the IPI (I can
> see with the current model that using an IPI is the only clean way,
> i.e. the alternative - if usable - would mean altering existing logic
> too),


You mean something like

     struct vcpu **last = &per_cpu(last_vcpu, vpmu->last_pcpu);
     cmpxchg(last, v, NULL);

Yes, that could work.

-boris