rbd: fix two leaks

PATCH New Alex Elder 1 participant 3 messages Jan 4, 2013

Subsystems (2)

Block layer RADOS block device (RBD)

lore

Share & tools

lore

Alex Elder

author

2013-01-04 14:57 UTC #0 lore raw

cover rbd: fix two leaks

When certain special osd requests are processed, they have data structures allocated that are not properly freed when the request is completed. This series fixes that.

-Alex

[PATCH REPOST 1/2] rbd: don't leak rbd_req on synchronous requests [PATCH REPOST 2/2] rbd: don't leak rbd_req for rbd_req_sync_notify_ack()

Alex Elder

author

2013-01-04 14:58 UTC #1 lore raw

PATCH 1/2 rbd: don't leak rbd_req on synchronous requests

When rbd_do_request() is called it allocates and populates an rbd_req structure to hold information about the osd request to be sent. This is done for the benefit of the callback function (in particular, rbd_req_cb()), which uses this in processing when the request completes.

Synchronous requests provide no callback function, in which case rbd_do_request() waits for the request to complete before returning. This case is not handling the needed free of the rbd_req structure like it should, so it is getting leaked.

Note however that the synchronous case has no need for the rbd_req structure at all. So rather than simply freeing this structure for synchronous requests, just don't allocate it to begin with.

Signed-off-by Alex Elder <[email protected]>

Reviewed-by Josh Durgin <[email protected]>

---
 drivers/block/rbd.c |   48 ++++++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

drivers/block/rbd.c +24 ~~-24~~

index c1e5f24..17bf0b4 100644
@@ -1113,20 +1113,11 @@ static int rbd_do_request(struct request *rq,
 			  struct ceph_osd_request **linger_req,
 			  u64 *ver)
 {
+	struct ceph_osd_client *osdc;
 	struct ceph_osd_request *osd_req;
-	int ret;
+	struct rbd_request *rbd_req = NULL;
 	struct timespec mtime = CURRENT_TIME;
-	struct rbd_request *rbd_req;
-	struct ceph_osd_client *osdc;
-
-	rbd_req = kzalloc(sizeof(*rbd_req), GFP_NOIO);
-	if (!rbd_req)
-		return -ENOMEM;
-
-	if (coll) {
-		rbd_req->coll = coll;
-		rbd_req->coll_index = coll_index;
-	}
+	int ret;
 
 	dout("rbd_do_request object_name=%s ofs=%llu len=%llu coll=%p[%d]\n",
 		object_name, (unsigned long long) ofs,

@@ -1134,10 +1125,8 @@ static int rbd_do_request(struct request *rq,
 
 	osdc = &rbd_dev->rbd_client->client->osdc;
 	osd_req = ceph_osdc_alloc_request(osdc, snapc, 1, false, GFP_NOIO);
-	if (!osd_req) {
-		ret = -ENOMEM;
-		goto done_pages;
-	}
+	if (!osd_req)
+		return -ENOMEM;
 
 	osd_req->r_flags = flags;
 	osd_req->r_pages = pages;

@@ -1145,13 +1134,22 @@ static int rbd_do_request(struct request *rq,
 		osd_req->r_bio = bio;
 		bio_get(osd_req->r_bio);
 	}
-	osd_req->r_callback = rbd_cb;
 
-	rbd_req->rq = rq;
-	rbd_req->bio = bio;
-	rbd_req->pages = pages;
-	rbd_req->len = len;
+	if (rbd_cb) {
+		ret = -ENOMEM;
+		rbd_req = kmalloc(sizeof(*rbd_req), GFP_NOIO);
+		if (!rbd_req)
+			goto done_osd_req;
+
+		rbd_req->rq = rq;
+		rbd_req->bio = bio;
+		rbd_req->pages = pages;
+		rbd_req->len = len;
+		rbd_req->coll = coll;
+		rbd_req->coll_index = coll ? coll_index : 0;
+	}
 
+	osd_req->r_callback = rbd_cb;
 	osd_req->r_priv = rbd_req;
 
 	strncpy(osd_req->r_oid, object_name, sizeof(osd_req->r_oid));

@@ -1193,10 +1191,12 @@ static int rbd_do_request(struct request *rq,
 	return ret;
 
 done_err:
-	bio_chain_put(rbd_req->bio);
-	ceph_osdc_put_request(osd_req);
-done_pages:
+	if (bio)
+		bio_chain_put(osd_req->r_bio);
 	kfree(rbd_req);
+done_osd_req:
+	ceph_osdc_put_request(osd_req);
+
 	return ret;
 }

Signature · 1 line

1.7.9.5

Alex Elder

author

2013-01-04 14:58 UTC #2 lore raw

PATCH 2/2 rbd: don't leak rbd_req for rbd_req_sync_notify_ack()

In reply to Alex Elder

When rbd_req_sync_notify_ack() calls rbd_do_request() it supplies rbd_simple_req_cb() as its callback function. Because the callback is supplied, an rbd_req structure gets allocated and populated so it can be used by the callback. However rbd_simple_req_cb() is not freeing (or even using) the rbd_req structure, so it's getting leaked.

Since rbd_simple_req_cb() has no need for the rbd_req structure, just avoid allocating one for this case. Of the three calls to rbd_do_request(), only the one from rbd_do_op() needs the rbd_req structure, and that call can be distinguished from the other two because it supplies a non-null rbd_collection pointer.

So fix this leak by only allocating the rbd_req structure if a non-null "coll" value is provided to rbd_do_request().

Signed-off-by Alex Elder <[email protected]>

Reviewed-by Josh Durgin <[email protected]>

---
 drivers/block/rbd.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/block/rbd.c +2 -2

index 17bf0b4..f0b30b2 100644
@@ -1135,7 +1135,7 @@ static int rbd_do_request(struct request *rq,
 		bio_get(osd_req->r_bio);
 	}
 
-	if (rbd_cb) {
+	if (coll) {
 		ret = -ENOMEM;
 		rbd_req = kmalloc(sizeof(*rbd_req), GFP_NOIO);
 		if (!rbd_req)

@@ -1146,7 +1146,7 @@ static int rbd_do_request(struct request *rq,
 		rbd_req->pages = pages;
 		rbd_req->len = len;
 		rbd_req->coll = coll;
-		rbd_req->coll_index = coll ? coll_index : 0;
+		rbd_req->coll_index = coll_index;
 	}
 
 	osd_req->r_callback = rbd_cb;

Signature · 1 line

1.7.9.5