From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] xen/arm: vcpu: Correctly release resource when the VCPU failed to initialized Date: Wed, 30 Apr 2014 19:12:32 +0100 Message-ID: <53613D10.2010901@citrix.com> References: <1398881159-10640-1-git-send-email-julien.grall@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WfYzu-0003Z9-7T for xen-devel@lists.xenproject.org; Wed, 30 Apr 2014 18:12:46 +0000 In-Reply-To: <1398881159-10640-1-git-send-email-julien.grall@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: xen-devel@lists.xenproject.org, stefano.stabellini@citrix.com, ian.campbell@citrix.com, tim@xen.org List-Id: xen-devel@lists.xenproject.org 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 > --- > 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 *);