From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH] xen-blkback: fix two memleaks Date: Fri, 11 Dec 2015 16:00:23 -0500 Message-ID: <20151211210023.GA12951@char.us.oracle.com> References: <1449710208-7806-1-git-send-email-bob.liu@oracle.com> <20151211172401.GA9765@char.us.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1a7Unp-0004TV-S3 for xen-devel@lists.xenproject.org; Fri, 11 Dec 2015 21:00:34 +0000 Content-Disposition: inline In-Reply-To: <20151211172401.GA9765@char.us.oracle.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: Bob Liu Cc: xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org, roger.pau@citrix.com List-Id: xen-devel@lists.xenproject.org On Fri, Dec 11, 2015 at 12:24:01PM -0500, Konrad Rzeszutek Wilk wrote: > > diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c > > index 44396b8..dabdb18 100644 > > --- a/drivers/block/xen-blkback/xenbus.c > > +++ b/drivers/block/xen-blkback/xenbus.c > > @@ -246,6 +246,9 @@ static int xen_blkif_disconnect(struct xen_blkif *blkif) > > struct pending_req *req, *n; > > unsigned int j, r; > > > > + if (!blkif->rings) > > + goto out; > > + > > I dropped this and instead added: blkif->nr_rings = 0. > > Which will mean we can still run through this function many > times but that should not be a problem. And I realized that there is still a latent bug. We take the refcount on the rings whenever we go to connect_ring. But we don't put it in disconnect. So a guest could change our state to go in (say we have 4 rings). Initially our refcount is 1. XenbusStateConnected (we refcount to five). XenbusStateClosed (we don't do a put on the refcount, so we are still at 5). XenbusStateConnected (we refcount to ten now). XenbusStateClosed (refcount still at ten). Guest dies, we go to xen_blkbk_remove, which clears the rings but only refcounts down to five. My first instict was to move the loop from xen_blkbk_remove: for (i = 0; i < be->blkif->nr_rings; i++) xen_blkif_put(be->blkif); in the xen_blkif_disconnect(). That would take care of that, and also of the inadvert bug I added with doign blkif->nr_rings = 0 in here - that is the loop above would never run (oops). But the problem we have is that the xen_blkif_put calls the xen_blkif_deferred_free (when the refcount is zero) which calls xen_blkif_free which calls xen_blkif_disconnect. How about this patch: >>From a95337a898fa452eb417e429ef270283393fd994 Mon Sep 17 00:00:00 2001 From: Bob Liu Date: Thu, 10 Dec 2015 09:16:48 +0800 Subject: [PATCH] xen/blkback: Fix two memory leaks. This patch fixs two memleaks: backtrace: [] kmemleak_alloc+0x28/0x50 [] kmem_cache_alloc+0xbb/0x1d0 [] xen_blkbk_probe+0x58/0x230 [] xenbus_dev_probe+0x76/0x130 [] driver_probe_device+0x166/0x2c0 [] __device_attach_driver+0xac/0xb0 [] bus_for_each_drv+0x67/0x90 [] __device_attach+0xc7/0x120 [] device_initial_probe+0x13/0x20 [] bus_probe_device+0x9a/0xb0 [] device_add+0x3b1/0x5c0 [] device_register+0x1e/0x30 [] xenbus_probe_node+0x158/0x170 [] xenbus_dev_changed+0x1af/0x1c0 [] backend_changed+0x1b/0x20 [] xenwatch_thread+0xb6/0x160 unreferenced object 0xffff880007ba8ef8 (size 224): backtrace: [] kmemleak_alloc+0x28/0x50 [] __kmalloc+0xd3/0x1e0 [] frontend_changed+0x2c7/0x580 [] xenbus_otherend_changed+0xa2/0xb0 [] frontend_changed+0x10/0x20 [] xenwatch_thread+0xb6/0x160 [] kthread+0xd7/0xf0 [] ret_from_fork+0x3f/0x70 [] 0xffffffffffffffff unreferenced object 0xffff8800048dcd38 (size 224): The first leak is caused by not put() the be->blkif reference which we had gotten in xen_blkif_alloc(), while the second is us not freeing blkif->rings in the right place. Signed-off-by: Bob Liu Reported-and-Tested-by: Konrad Rzeszutek Wilk Signed-off-by: Konrad Rzeszutek Wilk --- drivers/block/xen-blkback/xenbus.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c index 44396b8..ab793ac 100644 --- a/drivers/block/xen-blkback/xenbus.c +++ b/drivers/block/xen-blkback/xenbus.c @@ -297,8 +297,16 @@ static int xen_blkif_disconnect(struct xen_blkif *blkif) BUG_ON(ring->free_pages_num != 0); BUG_ON(ring->persistent_gnt_c != 0); WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages)); + xen_blkif_put(be->blkif); } blkif->nr_ring_pages = 0; + /* + * blkif->rings was allocated in connect_ring, so we should free it in + * here. + */ + kfree(blkif->rings); + blkif->rings = NULL; + blkif->nr_rings = 0; return 0; } @@ -310,7 +318,6 @@ static void xen_blkif_free(struct xen_blkif *blkif) xen_vbd_free(&blkif->vbd); /* Make sure everything is drained before shutting down */ - kfree(blkif->rings); kmem_cache_free(xen_blkif_cachep, blkif); } @@ -499,12 +506,11 @@ static int xen_blkbk_remove(struct xenbus_device *dev) dev_set_drvdata(&dev->dev, NULL); - if (be->blkif) { + if (be->blkif) xen_blkif_disconnect(be->blkif); - for (i = 0; i < be->blkif->nr_rings; i++) - xen_blkif_put(be->blkif); - } + /* Put the reference we set in xen_blkif_alloc(). */ + xen_blkif_put(be->blkif); kfree(be->mode); kfree(be); return 0; -- 2.1.0