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

[PATCH 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 dereference it in
the future, when VCPU is gone.

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

This needs to be backported to 4.3 and 4.4 as well

diff --git a/xen/arch/x86/hvm/vpmu.c b/xen/arch/x86/hvm/vpmu.c
index 1df74c2..6d39680 100644
--- a/xen/arch/x86/hvm/vpmu.c
+++ b/xen/arch/x86/hvm/vpmu.c
@@ -247,10 +247,32 @@ void vpmu_initialise(struct vcpu *v)
     }
 }
 
+static void vpmu_clear_last(void *arg)
+{
+    struct vcpu *v = (struct vcpu *)arg;
+
+    if ( this_cpu(last_vcpu) == v )
+        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) )
+    {
+        /* Need to clear last_vcpu in case it points to v */
+        if ( vpmu->last_pcpu != smp_processor_id() )
+            on_selected_cpus(cpumask_of(vpmu->last_pcpu),
+                             vpmu_clear_last, (void *)v, 1);
+        else
+        {
+            local_irq_disable();
+            vpmu_clear_last((void *)v);
+            local_irq_enable();
+        }
+    }
+
     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 for 4.5] x86/VPMU: Clear last_vcpu when destroying VPMU

[Konrad Rzeszutek Wilk]

On Fri, Dec 12, 2014 at 04:20:48PM -0500, Boris Ostrovsky wrote:
> We need to make sure that last_vcpu is not pointing to VCPU whose
> VPMU is being destroyed. Otherwise we may try dereference it in
> the future, when VCPU is gone.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  xen/arch/x86/hvm/vpmu.c |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
> 
> This needs to be backported to 4.3 and 4.4 as well
> 
> diff --git a/xen/arch/x86/hvm/vpmu.c b/xen/arch/x86/hvm/vpmu.c
> index 1df74c2..6d39680 100644
> --- a/xen/arch/x86/hvm/vpmu.c
> +++ b/xen/arch/x86/hvm/vpmu.c
> @@ -247,10 +247,32 @@ void vpmu_initialise(struct vcpu *v)
>      }
>  }
>  
> +static void vpmu_clear_last(void *arg)
> +{
> +    struct vcpu *v = (struct vcpu *)arg;
> +
> +    if ( this_cpu(last_vcpu) == v )
> +        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) )
> +    {
> +        /* Need to clear last_vcpu in case it points to v */
> +        if ( vpmu->last_pcpu != smp_processor_id() )
> +            on_selected_cpus(cpumask_of(vpmu->last_pcpu),
> +                             vpmu_clear_last, (void *)v, 1);
> +        else
> +        {
> +            local_irq_disable();
> +            vpmu_clear_last((void *)v);
> +            local_irq_enable();
> +        }
> +    }
> +
>      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 for 4.5] x86/VPMU: Clear last_vcpu when destroying VPMU

[Boris Ostrovsky]

On 12/13/2014 02:08 PM, Konrad Rzeszutek Wilk wrote:
> On Fri, Dec 12, 2014 at 04:20:48PM -0500, Boris Ostrovsky wrote:
>> We need to make sure that last_vcpu is not pointing to VCPU whose
>> VPMU is being destroyed. Otherwise we may try dereference it in
>> the future, when VCPU is gone.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

I would like to send a slightly better patch on Monday (along the same 
lines but trying to avoid unnecessary IPIs if not needed).

-boris


>> ---
>>   xen/arch/x86/hvm/vpmu.c |   22 ++++++++++++++++++++++
>>   1 files changed, 22 insertions(+), 0 deletions(-)
>>
>> This needs to be backported to 4.3 and 4.4 as well
>>
>> diff --git a/xen/arch/x86/hvm/vpmu.c b/xen/arch/x86/hvm/vpmu.c
>> index 1df74c2..6d39680 100644
>> --- a/xen/arch/x86/hvm/vpmu.c
>> +++ b/xen/arch/x86/hvm/vpmu.c
>> @@ -247,10 +247,32 @@ void vpmu_initialise(struct vcpu *v)
>>       }
>>   }
>>   
>> +static void vpmu_clear_last(void *arg)
>> +{
>> +    struct vcpu *v = (struct vcpu *)arg;
>> +
>> +    if ( this_cpu(last_vcpu) == v )
>> +        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) )
>> +    {
>> +        /* Need to clear last_vcpu in case it points to v */
>> +        if ( vpmu->last_pcpu != smp_processor_id() )
>> +            on_selected_cpus(cpumask_of(vpmu->last_pcpu),
>> +                             vpmu_clear_last, (void *)v, 1);
>> +        else
>> +        {
>> +            local_irq_disable();
>> +            vpmu_clear_last((void *)v);
>> +            local_irq_enable();
>> +        }
>> +    }
>> +
>>       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 for 4.5] x86/VPMU: Clear last_vcpu when destroying VPMU

[Jan Beulich]

>>> On 12.12.14 at 22:20, <boris.ostrovsky@oracle.com> wrote:
> --- a/xen/arch/x86/hvm/vpmu.c
> +++ b/xen/arch/x86/hvm/vpmu.c
> @@ -247,10 +247,32 @@ void vpmu_initialise(struct vcpu *v)
>      }
>  }
>  
> +static void vpmu_clear_last(void *arg)
> +{
> +    struct vcpu *v = (struct vcpu *)arg;

Please drop this pointless cast, or perhaps the entire variable, as ...

> +
> +    if ( this_cpu(last_vcpu) == v )

... you don't really need it to be of "struct vcpu *" type - "void *"
is quite fine for a comparison.

> +        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) )
> +    {
> +        /* Need to clear last_vcpu in case it points to v */
> +        if ( vpmu->last_pcpu != smp_processor_id() )
> +            on_selected_cpus(cpumask_of(vpmu->last_pcpu),
> +                             vpmu_clear_last, (void *)v, 1);

The cast here is pointless too. But considering your subsequent
reply this code may go away altogether anyway; if it doesn't,
explaining (in the commit message) why you need to use an IPI
here would seem necessary.

> +        else
> +        {
> +            local_irq_disable();
> +            vpmu_clear_last((void *)v);

And another pointless cast.

Jan

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

[Boris Ostrovsky]

On 12/15/2014 05:07 AM, Jan Beulich wrote:
>>>> On 12.12.14 at 22:20, <boris.ostrovsky@oracle.com> wrote:
>> --- a/xen/arch/x86/hvm/vpmu.c
>> +++ b/xen/arch/x86/hvm/vpmu.c
>> @@ -247,10 +247,32 @@ void vpmu_initialise(struct vcpu *v)
>>       }
>>   }
>>   
>> +static void vpmu_clear_last(void *arg)
>> +{
>> +    struct vcpu *v = (struct vcpu *)arg;
> Please drop this pointless cast, or perhaps the entire variable, as ...
>
>> +
>> +    if ( this_cpu(last_vcpu) == v )
> ... you don't really need it to be of "struct vcpu *" type - "void *"
> is quite fine for a comparison.
>
>> +        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) )
>> +    {
>> +        /* Need to clear last_vcpu in case it points to v */
>> +        if ( vpmu->last_pcpu != smp_processor_id() )
>> +            on_selected_cpus(cpumask_of(vpmu->last_pcpu),
>> +                             vpmu_clear_last, (void *)v, 1);
> The cast here is pointless too. But considering your subsequent
> reply this code may go away altogether anyway; if it doesn't,
> explaining (in the commit message) why you need to use an IPI
> here would seem necessary.

If I do simply
     if (per_cpu(last_vcpu, vpmu->last_pcpu) == v)
         per_cpu(last_vcpu, vpmu->last_pcpu) = NULL

then there is a (rather theoretical) possibility that between the test 
and subsequent clearing the remote cpu (i.e. vpmu->last_pcpu) will do 
load_vpmu() and then save_vpmu() for another VCPU. The former will clear 
last_vcpu and the latter will set last_vcpu to something else. And then 
the destroy_vpmu() will set it again to NULL, which is bad.

Doing it in in IPI will guarantee that nothing can happen between test 
and setting it to NULL.

Again, this very much theoretical, but that's why I have it. (BTW, doing 
this via IPI also preserves assumption that last_vcpu is always updated 
on local CPU.)

My changes for next version would make the need to do the IPIs less 
frequent. But if last_cpu needs to be cleared it would still be via IPI.

-boris

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

[Jan Beulich]

>>> On 15.12.14 at 18:15, <boris.ostrovsky@oracle.com> wrote:
> On 12/15/2014 05:07 AM, Jan Beulich wrote:
>>>>> On 12.12.14 at 22:20, <boris.ostrovsky@oracle.com> wrote:
>>> --- a/xen/arch/x86/hvm/vpmu.c
>>> +++ b/xen/arch/x86/hvm/vpmu.c
>>> @@ -247,10 +247,32 @@ void vpmu_initialise(struct vcpu *v)
>>>       }
>>>   }
>>>   
>>> +static void vpmu_clear_last(void *arg)
>>> +{
>>> +    struct vcpu *v = (struct vcpu *)arg;
>> Please drop this pointless cast, or perhaps the entire variable, as ...
>>
>>> +
>>> +    if ( this_cpu(last_vcpu) == v )
>> ... you don't really need it to be of "struct vcpu *" type - "void *"
>> is quite fine for a comparison.
>>
>>> +        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) )
>>> +    {
>>> +        /* Need to clear last_vcpu in case it points to v */
>>> +        if ( vpmu->last_pcpu != smp_processor_id() )
>>> +            on_selected_cpus(cpumask_of(vpmu->last_pcpu),
>>> +                             vpmu_clear_last, (void *)v, 1);
>> The cast here is pointless too. But considering your subsequent
>> reply this code may go away altogether anyway; if it doesn't,
>> explaining (in the commit message) why you need to use an IPI
>> here would seem necessary.
> 
> If I do simply
>      if (per_cpu(last_vcpu, vpmu->last_pcpu) == v)
>          per_cpu(last_vcpu, vpmu->last_pcpu) = NULL
> 
> then there is a (rather theoretical) possibility that between the test 
> and subsequent clearing the remote cpu (i.e. vpmu->last_pcpu) will do 
> load_vpmu() and then save_vpmu() for another VCPU. The former will clear 
> last_vcpu and the latter will set last_vcpu to something else. And then 
> the destroy_vpmu() will set it again to NULL, which is bad.

So how about using cmpxchg then?

Jan