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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-04-30 at 20:15 +0100, 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 neither vcpu_vgic_init nor vcpu_vtimer_init fail, 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@xxxxxxxxxx>

I was about to acked + apply but it failed to build on arm64 with:

        domain.c: In function 'alloc_vcpu_struct':
        /local/scratch/ianc/devel/committer.git/xen/include/xen/lib.h:19:31: 
error: static assertion failed: "!(sizeof(*v) > PAGE_SIZE)"
         #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond ")"); 
})
                                       ^
        domain.c:415:5: note: in expansion of macro 'BUILD_BUG_ON'
             BUILD_BUG_ON(sizeof(*v) > PAGE_SIZE);
             ^
struct arch_vcpu is apparently now too large.

I had also reworded your commit message somewhat:
    xen/arm: vcpu: Correctly release resources when a VCPU fails to initialize
    
    While I was adding new failing code at the end of the function, I noticed
    that the vtimers are not freed which messes up all the timers and will crash
    Xen quickly when the page s reused.
    
    Currently neither vcpu_vgic_init nor vcpu_vtimer_init fails, so we
    are safe for now. With the new GICv3 code, the former function will be able
    to fail. This will result in 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 any safeguard against removing a non-initialized timer.

Ian.

> 
> ---
>     Changes in v2:
>         - Update commit message
> ---
>  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 *);



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel