* [PATCH] xen-blkback: fix two memleaks
@ 2015-12-10 1:16 Bob Liu
2015-12-11 17:24 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 4+ messages in thread
From: Bob Liu @ 2015-12-10 1:16 UTC (permalink / raw)
To: xen-devel; +Cc: konrad.wilk, roger.pau, linux-kernel, Bob Liu
This patch fixs two memleaks in konrad/xen.git/for-jens-4.5.
backtrace:
[<ffffffff817ba5e8>] kmemleak_alloc+0x28/0x50
[<ffffffff81205e3b>] kmem_cache_alloc+0xbb/0x1d0
[<ffffffff81534028>] xen_blkbk_probe+0x58/0x230
[<ffffffff8146adb6>] xenbus_dev_probe+0x76/0x130
[<ffffffff81511716>] driver_probe_device+0x166/0x2c0
[<ffffffff815119bc>] __device_attach_driver+0xac/0xb0
[<ffffffff8150fa57>] bus_for_each_drv+0x67/0x90
[<ffffffff81511ab7>] __device_attach+0xc7/0x120
[<ffffffff81511b23>] device_initial_probe+0x13/0x20
[<ffffffff8151059a>] bus_probe_device+0x9a/0xb0
[<ffffffff8150f0a1>] device_add+0x3b1/0x5c0
[<ffffffff8150f47e>] device_register+0x1e/0x30
[<ffffffff8146a9e8>] xenbus_probe_node+0x158/0x170
[<ffffffff8146abaf>] xenbus_dev_changed+0x1af/0x1c0
[<ffffffff8146b1bb>] backend_changed+0x1b/0x20
[<ffffffff81468ca6>] xenwatch_thread+0xb6/0x160
unreferenced object 0xffff880007ba8ef8 (size 224):
backtrace:
[<ffffffff817ba5e8>] kmemleak_alloc+0x28/0x50
[<ffffffff81205c73>] __kmalloc+0xd3/0x1e0
[<ffffffff81534d87>] frontend_changed+0x2c7/0x580
[<ffffffff8146af12>] xenbus_otherend_changed+0xa2/0xb0
[<ffffffff8146b2c0>] frontend_changed+0x10/0x20
[<ffffffff81468ca6>] xenwatch_thread+0xb6/0x160
[<ffffffff810d3e97>] kthread+0xd7/0xf0
[<ffffffff817c4a9f>] ret_from_fork+0x3f/0x70
[<ffffffffffffffff>] 0xffffffffffffffff
unreferenced object 0xffff8800048dcd38 (size 224):
The first leak is caused by not put the be->blkif reference got in
xen_blkif_alloc(), while the second is not free blkif->rings in the right
place.
Signed-off-by: Bob Liu <bob.liu@oracle.com>
---
drivers/block/xen-blkback/xenbus.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
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;
+
for (r = 0; r < blkif->nr_rings; r++) {
struct xen_blkif_ring *ring = &blkif->rings[r];
unsigned int i = 0;
@@ -299,7 +302,14 @@ static int xen_blkif_disconnect(struct xen_blkif *blkif)
WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));
}
blkif->nr_ring_pages = 0;
+ /*
+ * blkif->rings was allocated in connect_ring, so we should free it in
+ * disconnect.
+ */
+ kfree(blkif->rings);
+ blkif->rings = NULL;
+out:
return 0;
}
@@ -310,7 +320,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);
}
@@ -505,6 +514,8 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
xen_blkif_put(be->blkif);
}
+ /* Put the reference got in xen_blkif_alloc(). */
+ xen_blkif_put(be->blkif);
kfree(be->mode);
kfree(be);
return 0;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] xen-blkback: fix two memleaks
2015-12-10 1:16 [PATCH] xen-blkback: fix two memleaks Bob Liu
@ 2015-12-11 17:24 ` Konrad Rzeszutek Wilk
2015-12-11 21:00 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 4+ messages in thread
From: Konrad Rzeszutek Wilk @ 2015-12-11 17:24 UTC (permalink / raw)
To: Bob Liu; +Cc: xen-devel, linux-kernel, roger.pau
> 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.
> for (r = 0; r < blkif->nr_rings; r++) {
> struct xen_blkif_ring *ring = &blkif->rings[r];
> unsigned int i = 0;
> @@ -299,7 +302,14 @@ static int xen_blkif_disconnect(struct xen_blkif *blkif)
> WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));
> }
> blkif->nr_ring_pages = 0;
> + /*
> + * blkif->rings was allocated in connect_ring, so we should free it in
> + * disconnect.
s/disconnect/here/
> + */
> + kfree(blkif->rings);
> + blkif->rings = NULL;
And perhaps also:
blkif->nr_rings = 0?
>
> +out:
> return 0;
> }
>
> @@ -310,7 +320,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);
> }
>
> @@ -505,6 +514,8 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
> xen_blkif_put(be->blkif);
> }
>
> + /* Put the reference got in xen_blkif_alloc(). */
s/got/we set/
Updated this and testing it as such.
> + xen_blkif_put(be->blkif);
> kfree(be->mode);
> kfree(be);
> return 0;
> --
> 1.7.10.4
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] xen-blkback: fix two memleaks
2015-12-11 17:24 ` Konrad Rzeszutek Wilk
@ 2015-12-11 21:00 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 4+ messages in thread
From: Konrad Rzeszutek Wilk @ 2015-12-11 21:00 UTC (permalink / raw)
To: Bob Liu; +Cc: xen-devel, linux-kernel, roger.pau
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 <bob.liu@oracle.com>
Date: Thu, 10 Dec 2015 09:16:48 +0800
Subject: [PATCH] xen/blkback: Fix two memory leaks.
This patch fixs two memleaks:
backtrace:
[<ffffffff817ba5e8>] kmemleak_alloc+0x28/0x50
[<ffffffff81205e3b>] kmem_cache_alloc+0xbb/0x1d0
[<ffffffff81534028>] xen_blkbk_probe+0x58/0x230
[<ffffffff8146adb6>] xenbus_dev_probe+0x76/0x130
[<ffffffff81511716>] driver_probe_device+0x166/0x2c0
[<ffffffff815119bc>] __device_attach_driver+0xac/0xb0
[<ffffffff8150fa57>] bus_for_each_drv+0x67/0x90
[<ffffffff81511ab7>] __device_attach+0xc7/0x120
[<ffffffff81511b23>] device_initial_probe+0x13/0x20
[<ffffffff8151059a>] bus_probe_device+0x9a/0xb0
[<ffffffff8150f0a1>] device_add+0x3b1/0x5c0
[<ffffffff8150f47e>] device_register+0x1e/0x30
[<ffffffff8146a9e8>] xenbus_probe_node+0x158/0x170
[<ffffffff8146abaf>] xenbus_dev_changed+0x1af/0x1c0
[<ffffffff8146b1bb>] backend_changed+0x1b/0x20
[<ffffffff81468ca6>] xenwatch_thread+0xb6/0x160
unreferenced object 0xffff880007ba8ef8 (size 224):
backtrace:
[<ffffffff817ba5e8>] kmemleak_alloc+0x28/0x50
[<ffffffff81205c73>] __kmalloc+0xd3/0x1e0
[<ffffffff81534d87>] frontend_changed+0x2c7/0x580
[<ffffffff8146af12>] xenbus_otherend_changed+0xa2/0xb0
[<ffffffff8146b2c0>] frontend_changed+0x10/0x20
[<ffffffff81468ca6>] xenwatch_thread+0xb6/0x160
[<ffffffff810d3e97>] kthread+0xd7/0xf0
[<ffffffff817c4a9f>] ret_from_fork+0x3f/0x70
[<ffffffffffffffff>] 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 <bob.liu@oracle.com>
Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] xen-blkback: fix two memleaks
@ 2015-12-10 1:16 Bob Liu
0 siblings, 0 replies; 4+ messages in thread
From: Bob Liu @ 2015-12-10 1:16 UTC (permalink / raw)
To: xen-devel; +Cc: Bob Liu, roger.pau, linux-kernel
This patch fixs two memleaks in konrad/xen.git/for-jens-4.5.
backtrace:
[<ffffffff817ba5e8>] kmemleak_alloc+0x28/0x50
[<ffffffff81205e3b>] kmem_cache_alloc+0xbb/0x1d0
[<ffffffff81534028>] xen_blkbk_probe+0x58/0x230
[<ffffffff8146adb6>] xenbus_dev_probe+0x76/0x130
[<ffffffff81511716>] driver_probe_device+0x166/0x2c0
[<ffffffff815119bc>] __device_attach_driver+0xac/0xb0
[<ffffffff8150fa57>] bus_for_each_drv+0x67/0x90
[<ffffffff81511ab7>] __device_attach+0xc7/0x120
[<ffffffff81511b23>] device_initial_probe+0x13/0x20
[<ffffffff8151059a>] bus_probe_device+0x9a/0xb0
[<ffffffff8150f0a1>] device_add+0x3b1/0x5c0
[<ffffffff8150f47e>] device_register+0x1e/0x30
[<ffffffff8146a9e8>] xenbus_probe_node+0x158/0x170
[<ffffffff8146abaf>] xenbus_dev_changed+0x1af/0x1c0
[<ffffffff8146b1bb>] backend_changed+0x1b/0x20
[<ffffffff81468ca6>] xenwatch_thread+0xb6/0x160
unreferenced object 0xffff880007ba8ef8 (size 224):
backtrace:
[<ffffffff817ba5e8>] kmemleak_alloc+0x28/0x50
[<ffffffff81205c73>] __kmalloc+0xd3/0x1e0
[<ffffffff81534d87>] frontend_changed+0x2c7/0x580
[<ffffffff8146af12>] xenbus_otherend_changed+0xa2/0xb0
[<ffffffff8146b2c0>] frontend_changed+0x10/0x20
[<ffffffff81468ca6>] xenwatch_thread+0xb6/0x160
[<ffffffff810d3e97>] kthread+0xd7/0xf0
[<ffffffff817c4a9f>] ret_from_fork+0x3f/0x70
[<ffffffffffffffff>] 0xffffffffffffffff
unreferenced object 0xffff8800048dcd38 (size 224):
The first leak is caused by not put the be->blkif reference got in
xen_blkif_alloc(), while the second is not free blkif->rings in the right
place.
Signed-off-by: Bob Liu <bob.liu@oracle.com>
---
drivers/block/xen-blkback/xenbus.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
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;
+
for (r = 0; r < blkif->nr_rings; r++) {
struct xen_blkif_ring *ring = &blkif->rings[r];
unsigned int i = 0;
@@ -299,7 +302,14 @@ static int xen_blkif_disconnect(struct xen_blkif *blkif)
WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));
}
blkif->nr_ring_pages = 0;
+ /*
+ * blkif->rings was allocated in connect_ring, so we should free it in
+ * disconnect.
+ */
+ kfree(blkif->rings);
+ blkif->rings = NULL;
+out:
return 0;
}
@@ -310,7 +320,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);
}
@@ -505,6 +514,8 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
xen_blkif_put(be->blkif);
}
+ /* Put the reference got in xen_blkif_alloc(). */
+ xen_blkif_put(be->blkif);
kfree(be->mode);
kfree(be);
return 0;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-12-11 21:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-12-10 1:16 [PATCH] xen-blkback: fix two memleaks Bob Liu
2015-12-11 17:24 ` Konrad Rzeszutek Wilk
2015-12-11 21:00 ` Konrad Rzeszutek Wilk
-- strict thread matches above, loose matches on Subject: below --
2015-12-10 1:16 Bob Liu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.