From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v2] xen/arm: vcpu: Correctly release resource when the VCPU failed to initialized Date: Wed, 21 May 2014 13:39:29 +0100 Message-ID: <537C9E81.1050109@linaro.org> References: <1398885355-13200-1-git-send-email-julien.grall@linaro.org> <1399033506.32736.84.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Wn5nw-0002as-ME for xen-devel@lists.xenproject.org; Wed, 21 May 2014 12:39:32 +0000 Received: by mail-ee0-f49.google.com with SMTP id e53so1547662eek.36 for ; Wed, 21 May 2014 05:39:30 -0700 (PDT) In-Reply-To: <1399033506.32736.84.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: xen-devel@lists.xenproject.org, tim@xen.org, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org Hi Ian, On 05/02/2014 01:25 PM, Ian Campbell wrote: > 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 > > 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. The commit 6fedf29 "xen/arm: Drop event_mask in arch_vcpu" which makes this patch compiles on both arm32 and arm64. I think you can safely push this patch now :). Thanks, -- Julien Grall