[PATCH] xen/arm: vcpu: Correctly release resource when the VCPU failed to initialized

[PATCH] xen/arm: vcpu: Correctly release resource when the VCPU failed to initialized

[Julien Grall]

While I was adding new failing code at the end of the function, I've noticed
that the vtimers are not freed which mess all the timers and will crash Xen
quickly when the page will be reused.

Currently none of vcpu_vgic_init and vcpu_vtimer_init doesn't failed, so we
are safe for now. With the new GICv3 code, the former function will be able
to fail. This will result to a memory leak.

Call vcpu_destroy if the initialization has failed. We also need to add a
boolean to know if the vtimers are correctly setup as the timer common code
doesn't have safe guard against removing non-initialized timer.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/domain.c        |    8 ++++++--
 xen/arch/arm/vtimer.c        |    5 +++++
 xen/include/asm-arm/domain.h |    1 +
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index ccccb77..c47db4a 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -468,12 +468,16 @@ int vcpu_initialise(struct vcpu *v)
     processor_vcpu_initialise(v);
 
     if ( (rc = vcpu_vgic_init(v)) != 0 )
-        return rc;
+        goto fail;
 
     if ( (rc = vcpu_vtimer_init(v)) != 0 )
-        return rc;
+        goto fail;
 
     return rc;
+
+fail:
+    vcpu_destroy(v);
+    return rc;
 }
 
 void vcpu_destroy(struct vcpu *v)
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index cb690bb..c515e7e 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -77,11 +77,16 @@ int vcpu_vtimer_init(struct vcpu *v)
         : GUEST_TIMER_VIRT_PPI;
     t->v = v;
 
+    v->arch.vtimer_initialized = 1;
+
     return 0;
 }
 
 void vcpu_timer_destroy(struct vcpu *v)
 {
+    if ( !v->arch.vtimer_initialized )
+        return;
+
     kill_timer(&v->arch.virt_timer.timer);
     kill_timer(&v->arch.phys_timer.timer);
 }
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index ec66a4e..1be3da2 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -285,6 +285,7 @@ struct arch_vcpu
 
     struct vtimer phys_timer;
     struct vtimer virt_timer;
+    bool_t vtimer_initialized;
 }  __cacheline_aligned;
 
 void vcpu_show_execution_state(struct vcpu *);
-- 
1.7.10.4

Re: [PATCH] xen/arm: vcpu: Correctly release resource when the VCPU failed to initialized

[Andrew Cooper]

On 30/04/14 19:05, Julien Grall wrote:
> While I was adding new failing code at the end of the function, I've noticed
> that the vtimers are not freed which mess all the timers and will crash Xen
> quickly when the page will be reused.
>
> Currently none of vcpu_vgic_init and vcpu_vtimer_init doesn't failed, so we

This sentence is confusing.  Do you mean "Currently neither
vcpu_vgic_init nor vcpu_vtimer_init fail, so we..." ?

~Andrew

> are safe for now. With the new GICv3 code, the former function will be able
> to fail. This will result to a memory leak.
>
> Call vcpu_destroy if the initialization has failed. We also need to add a
> boolean to know if the vtimers are correctly setup as the timer common code
> doesn't have safe guard against removing non-initialized timer.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/domain.c        |    8 ++++++--
>  xen/arch/arm/vtimer.c        |    5 +++++
>  xen/include/asm-arm/domain.h |    1 +
>  3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index ccccb77..c47db4a 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -468,12 +468,16 @@ int vcpu_initialise(struct vcpu *v)
>      processor_vcpu_initialise(v);
>  
>      if ( (rc = vcpu_vgic_init(v)) != 0 )
> -        return rc;
> +        goto fail;
>  
>      if ( (rc = vcpu_vtimer_init(v)) != 0 )
> -        return rc;
> +        goto fail;
>  
>      return rc;
> +
> +fail:
> +    vcpu_destroy(v);
> +    return rc;
>  }
>  
>  void vcpu_destroy(struct vcpu *v)
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index cb690bb..c515e7e 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -77,11 +77,16 @@ int vcpu_vtimer_init(struct vcpu *v)
>          : GUEST_TIMER_VIRT_PPI;
>      t->v = v;
>  
> +    v->arch.vtimer_initialized = 1;
> +
>      return 0;
>  }
>  
>  void vcpu_timer_destroy(struct vcpu *v)
>  {
> +    if ( !v->arch.vtimer_initialized )
> +        return;
> +
>      kill_timer(&v->arch.virt_timer.timer);
>      kill_timer(&v->arch.phys_timer.timer);
>  }
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index ec66a4e..1be3da2 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -285,6 +285,7 @@ struct arch_vcpu
>  
>      struct vtimer phys_timer;
>      struct vtimer virt_timer;
> +    bool_t vtimer_initialized;
>  }  __cacheline_aligned;
>  
>  void vcpu_show_execution_state(struct vcpu *);

Re: [PATCH] xen/arm: vcpu: Correctly release resource when the VCPU failed to initialized

[Julien Grall]

Hi Andrew,

On 30/04/14 19:12, Andrew Cooper wrote:
> On 30/04/14 19:05, Julien Grall wrote:
>> While I was adding new failing code at the end of the function, I've noticed
>> that the vtimers are not freed which mess all the timers and will crash Xen
>> quickly when the page will be reused.
>>
>> Currently none of vcpu_vgic_init and vcpu_vtimer_init doesn't failed, so we
>
> This sentence is confusing.  Do you mean "Currently neither
> vcpu_vgic_init nor vcpu_vtimer_init fail, so we..." ?

Yes, I will update the commit message on the next version.

Regards,

-- 
Julien Grall