[PATCH 0/6] rbd: cleanup and plug leaks

[PATCH 0/6] rbd: cleanup and plug leaks

[Alex Elder]

(This series, and the patch I just posted, are available
in the "review/wip-rbd-cleanup" branch of the ceph-client
git repository.)

This series is one of several I expect to be posting that
are mostly small tweaks and polish to the rbd code.  I'm
doing a review of the whole thing with an eye toward
verifying error paths and finding memory leaks.  Along
the way I'm making small changes to improve overall
consistency and perhaps shore up some coding style issues.

					-Alex

[PATCH 1/6] rbd: fix leak of snapshots during initial probe
[PATCH 2/6] rbd: make snap_size order parameter optional
[PATCH 3/6] rbd: only update values on snap_info success
[PATCH 4/6] rbd: rename __rbd_add_snap_dev()
[PATCH 5/6] rbd: fix leak of format 2 snapshot names
[PATCH 6/6] rbd: use rbd_obj_method_sync() return value

[PATCH 1/6] rbd: fix leak of snapshots during initial probe

[Alex Elder]

When an rbd image is initially mapped, its snapshot context is
collected, and then a list of snapshot entries representing the
snapshots in that context is created.  The list is created using
rbd_dev_snaps_update().  (This function also supports updating an
existing snapshot list based on a new snapshot context.)

If an error occurs, updating the list is aborted, and the list is
currently left as-is, in an inconsistent state.  At that point,
there may be a partially-constructed list, but the calling functions
(rbd_dev_probe_finish() from rbd_dev_probe() from rbd_add()) never
clean them up.  So this constitutes a leak.

A snapshot list that is inconsistent with the current snapshot
context is of no use, and might even be actively bad.  So rather
than just having the caller clean it up, have rbd_dev_snaps_update()
just clear out the entire snapshot list in the event an error
occurs.

The other place rbd_dev_snaps_update() is used is when a refresh is
triggered, either because of a watch callback or via a write to the
/sys/bus/rbd/devices/<id>/refresh interface.  An error while
updating the snapshots has no substantive effect in either of those
cases, but one of them issues a warning.  Move that warning to the
common rbd_dev_refresh() function so it gets issued regardless of
how it got initiated.

This is part of:
    http://tracker.ceph.com/issues/4803

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   50
++++++++++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 515fbf9..28b652c 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2521,7 +2521,6 @@ static void rbd_watch_cb(u64 ver, u64 notify_id,
u8 opcode, void *data)
 {
 	struct rbd_device *rbd_dev = (struct rbd_device *)data;
 	u64 hver;
-	int rc;

 	if (!rbd_dev)
 		return;
@@ -2529,10 +2528,7 @@ static void rbd_watch_cb(u64 ver, u64 notify_id,
u8 opcode, void *data)
 	dout("%s: \"%s\" notify_id %llu opcode %u\n", __func__,
 		rbd_dev->header_name, (unsigned long long) notify_id,
 		(unsigned int) opcode);
-	rc = rbd_dev_refresh(rbd_dev, &hver);
-	if (rc)
-		rbd_warn(rbd_dev, "got notification but failed to "
-			   " update snaps: %d\n", rc);
+	(void)rbd_dev_refresh(rbd_dev, &hver);

 	rbd_obj_notify_ack(rbd_dev, hver, notify_id);
 }
@@ -3085,6 +3081,9 @@ static int rbd_dev_refresh(struct rbd_device
*rbd_dev, u64 *hver)
 		ret = rbd_dev_v2_refresh(rbd_dev, hver);
 	mutex_unlock(&ctl_mutex);
 	revalidate_disk(rbd_dev->disk);
+	if (ret)
+		rbd_warn(rbd_dev, "got notification but failed to "
+			   " update snaps: %d\n", ret);

 	return ret;
 }
@@ -4010,6 +4009,11 @@ out:
  * Assumes the snapshots in the snapshot context are sorted by
  * snapshot id, highest id first.  (Snapshots in the rbd_dev's list
  * are also maintained in that order.)
+ *
+ * Note that any error occurs while updating the snapshot list
+ * aborts the update, and the entire list is cleared.  The snapshot
+ * list becomes inconsistent at that point anyway, so it might as
+ * well be empty.
  */
 static int rbd_dev_snaps_update(struct rbd_device *rbd_dev)
 {
@@ -4018,8 +4022,9 @@ static int rbd_dev_snaps_update(struct rbd_device
*rbd_dev)
 	struct list_head *head = &rbd_dev->snaps;
 	struct list_head *links = head->next;
 	u32 index = 0;
+	int ret = 0;

-	dout("%s: snap count is %u\n", __func__, (unsigned int) snap_count);
+	dout("%s: snap count is %u\n", __func__, (unsigned int)snap_count);
 	while (index < snap_count || links != head) {
 		u64 snap_id;
 		struct rbd_snap *snap;
@@ -4040,17 +4045,17 @@ static int rbd_dev_snaps_update(struct
rbd_device *rbd_dev)
 			 * A previously-existing snapshot is not in
 			 * the new snap context.
 			 *
-			 * If the now missing snapshot is the one the
-			 * image is mapped to, clear its exists flag
-			 * so we can avoid sending any more requests
-			 * to it.
+			 * If the now-missing snapshot is the one
+			 * the image represents, clear its existence
+			 * flag so we can avoid sending any more
+			 * requests to it.
 			 */
 			if (rbd_dev->spec->snap_id == snap->id)
 				clear_bit(RBD_DEV_FLAG_EXISTS, &rbd_dev->flags);
 			dout("removing %ssnap id %llu\n",
 				rbd_dev->spec->snap_id == snap->id ?
 							"mapped " : "",
-				(unsigned long long) snap->id);
+				(unsigned long long)snap->id);
 			rbd_remove_snap_dev(snap);

 			/* Done with this list entry; advance */
@@ -4061,11 +4066,14 @@ static int rbd_dev_snaps_update(struct
rbd_device *rbd_dev)

 		snap_name = rbd_dev_snap_info(rbd_dev, index,
 					&snap_size, &snap_features);
-		if (IS_ERR(snap_name))
-			return PTR_ERR(snap_name);
+		if (IS_ERR(snap_name)) {
+			ret = PTR_ERR(snap_name);
+			dout("failed to get snap info, error %d\n", ret);
+			goto out_err;
+		}

-		dout("entry %u: snap_id = %llu\n", (unsigned int) snap_count,
-			(unsigned long long) snap_id);
+		dout("entry %u: snap_id = %llu\n", (unsigned int)snap_count,
+			(unsigned long long)snap_id);
 		if (!snap || (snap_id != CEPH_NOSNAP && snap->id < snap_id)) {
 			struct rbd_snap *new_snap;

@@ -4074,11 +4082,9 @@ static int rbd_dev_snaps_update(struct rbd_device
*rbd_dev)
 			new_snap = __rbd_add_snap_dev(rbd_dev, snap_name,
 					snap_id, snap_size, snap_features);
 			if (IS_ERR(new_snap)) {
-				int err = PTR_ERR(new_snap);
-
-				dout("  failed to add dev, error %d\n", err);
-
-				return err;
+				ret = PTR_ERR(new_snap);
+				dout("  failed to add dev, error %d\n", ret);
+				goto out_err;
 			}

 			/* New goes before existing, or at end of list */
@@ -4109,6 +4115,10 @@ static int rbd_dev_snaps_update(struct rbd_device
*rbd_dev)
 	dout("%s: done\n", __func__);

 	return 0;
+out_err:
+	rbd_remove_all_snaps(rbd_dev);
+
+	return ret;
 }

 static int rbd_bus_add_dev(struct rbd_device *rbd_dev)
-- 
1.7.9.5

[PATCH 2/6] rbd: make snap_size order parameter optional

[Alex Elder]

Only one of the two callers of _rbd_dev_v2_snap_size() needs the
order value returned.  So make that an optional argument--a null
pointer if the caller doesn't need it.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 28b652c..1e01f0d 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -3494,7 +3494,8 @@ static int _rbd_dev_v2_snap_size(struct rbd_device
*rbd_dev, u64 snap_id,
 	if (ret < sizeof (size_buf))
 		return -ERANGE;

-	*order = size_buf.order;
+	if (order)
+		*order = size_buf.order;
 	*snap_size = le64_to_cpu(size_buf.size);

 	dout("  snap_id 0x%016llx order = %u, snap_size = %llu\n",
@@ -3939,11 +3940,10 @@ static char *rbd_dev_v2_snap_info(struct
rbd_device *rbd_dev, u32 which,
 		u64 *snap_size, u64 *snap_features)
 {
 	u64 snap_id;
-	u8 order;
 	int ret;

 	snap_id = rbd_dev->header.snapc->snaps[which];
-	ret = _rbd_dev_v2_snap_size(rbd_dev, snap_id, &order, snap_size);
+	ret = _rbd_dev_v2_snap_size(rbd_dev, snap_id, NULL, snap_size);
 	if (ret)
 		return ERR_PTR(ret);
 	ret = _rbd_dev_v2_snap_features(rbd_dev, snap_id, snap_features);
-- 
1.7.9.5

[PATCH 3/6] rbd: only update values on snap_info success

[Alex Elder]

Change rbd_dev_v2_snap_info() so it only ever sets values of the
size and features parameters if looking up the snapshot name was
successful.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 1e01f0d..e7d10d3 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -3908,6 +3908,7 @@ static char *rbd_dev_v2_snap_name(struct
rbd_device *rbd_dev, u32 which)
 	if (!reply_buf)
 		return ERR_PTR(-ENOMEM);

+	rbd_assert(which < rbd_dev->header.snapc->num_snaps);
 	snap_id = cpu_to_le64(rbd_dev->header.snapc->snaps[which]);
 	ret = rbd_obj_method_sync(rbd_dev, rbd_dev->header_name,
 				"rbd", "get_snapshot_name",
@@ -3940,17 +3941,30 @@ static char *rbd_dev_v2_snap_info(struct
rbd_device *rbd_dev, u32 which,
 		u64 *snap_size, u64 *snap_features)
 {
 	u64 snap_id;
+	u64 size;
+	u64 features;
+	char *snap_name;
 	int ret;

+	rbd_assert(which < rbd_dev->header.snapc->num_snaps);
 	snap_id = rbd_dev->header.snapc->snaps[which];
-	ret = _rbd_dev_v2_snap_size(rbd_dev, snap_id, NULL, snap_size);
+	ret = _rbd_dev_v2_snap_size(rbd_dev, snap_id, NULL, &size);
 	if (ret)
-		return ERR_PTR(ret);
-	ret = _rbd_dev_v2_snap_features(rbd_dev, snap_id, snap_features);
+		goto out_err;
+
+	ret = _rbd_dev_v2_snap_features(rbd_dev, snap_id, &features);
 	if (ret)
-		return ERR_PTR(ret);
+		goto out_err;
+
+	snap_name = rbd_dev_v2_snap_name(rbd_dev, which);
+	if (!IS_ERR(snap_name)) {
+		*snap_size = size;
+		*snap_features = features;
+	}

-	return rbd_dev_v2_snap_name(rbd_dev, which);
+	return snap_name;
+out_err:
+	return ERR_PTR(ret);
 }

 static char *rbd_dev_snap_info(struct rbd_device *rbd_dev, u32 which,
-- 
1.7.9.5

[PATCH 4/6] rbd: rename __rbd_add_snap_dev()

[Alex Elder]

Rename __rbd_add_snap_dev() to be rbd_snap_create().  We no longer
have devices for non-mapped snapshots, and we're not actually
"adding" it to the list in this function, just creating it.

Rename rbd_remove_snap_dev() to be rbd_snap_destroy() for reasons
similar to the above.  Stop having this function delete the snapshot
from its list (to be symmetrical with its create counterpart) and do
that in the caller instead.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index e7d10d3..916741b 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -359,7 +359,7 @@ static int rbd_img_request_submit(struct
rbd_img_request *img_request);
 static int rbd_dev_snaps_update(struct rbd_device *rbd_dev);

 static void rbd_dev_release(struct device *dev);
-static void rbd_remove_snap_dev(struct rbd_snap *snap);
+static void rbd_snap_destroy(struct rbd_snap *snap);

 static ssize_t rbd_add(struct bus_type *bus, const char *buf,
 		       size_t count);
@@ -3010,8 +3010,10 @@ static void rbd_remove_all_snaps(struct
rbd_device *rbd_dev)
 	struct rbd_snap *snap;
 	struct rbd_snap *next;

-	list_for_each_entry_safe(snap, next, &rbd_dev->snaps, node)
-		rbd_remove_snap_dev(snap);
+	list_for_each_entry_safe(snap, next, &rbd_dev->snaps, node) {
+		list_del(&snap->node);
+		rbd_snap_destroy(snap);
+	}
 }

 static void rbd_update_mapping_size(struct rbd_device *rbd_dev)
@@ -3413,14 +3415,13 @@ static void rbd_dev_destroy(struct rbd_device
*rbd_dev)
 	kfree(rbd_dev);
 }

-static void rbd_remove_snap_dev(struct rbd_snap *snap)
+static void rbd_snap_destroy(struct rbd_snap *snap)
 {
-	list_del(&snap->node);
 	kfree(snap->name);
 	kfree(snap);
 }

-static struct rbd_snap *__rbd_add_snap_dev(struct rbd_device *rbd_dev,
+static struct rbd_snap *rbd_snap_create(struct rbd_device *rbd_dev,
 						const char *snap_name,
 						u64 snap_id, u64 snap_size,
 						u64 snap_features)
@@ -4070,7 +4071,9 @@ static int rbd_dev_snaps_update(struct rbd_device
*rbd_dev)
 				rbd_dev->spec->snap_id == snap->id ?
 							"mapped " : "",
 				(unsigned long long)snap->id);
-			rbd_remove_snap_dev(snap);
+
+			list_del(&snap->node);
+			rbd_snap_destroy(snap);

 			/* Done with this list entry; advance */

@@ -4093,7 +4096,7 @@ static int rbd_dev_snaps_update(struct rbd_device
*rbd_dev)

 			/* We haven't seen this snapshot before */

-			new_snap = __rbd_add_snap_dev(rbd_dev, snap_name,
+			new_snap = rbd_snap_create(rbd_dev, snap_name,
 					snap_id, snap_size, snap_features);
 			if (IS_ERR(new_snap)) {
 				ret = PTR_ERR(new_snap);
-- 
1.7.9.5

[PATCH 5/6] rbd: fix leak of format 2 snapshot names

[Alex Elder]

When the snapshot context for an rbd device gets updated (or the
initial one is recorded) a a list of snapshot structures is created
to represent them, one entry per snapshot.  Each entry includes a
dynamically-allocated copy of the snapshot name.

Currently the name is allocated in rbd_snap_create(), as a duplicate
of the passed-in name.

For format 1 images, the snapshot name provided is just a pointer to
an existing name.  But for format 2 images, the passed-in name is
already dynamically allocated, and in the the process of duplicating
it here we are leaking the passed-in name.

Fix this by dynamically allocating the name for format 1 snapshots
also, and then stop allocating a duplicate in rbd_snap_create().

Change rbd_dev_v1_snap_info() so none of its parameters is
side-effected unless it's going to return success.

This is part of:
    http://tracker.ceph.com/issues/4803

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 916741b..2b5ba50 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -3427,30 +3427,23 @@ static struct rbd_snap *rbd_snap_create(struct
rbd_device *rbd_dev,
 						u64 snap_features)
 {
 	struct rbd_snap *snap;
-	int ret;

 	snap = kzalloc(sizeof (*snap), GFP_KERNEL);
 	if (!snap)
 		return ERR_PTR(-ENOMEM);

-	ret = -ENOMEM;
-	snap->name = kstrdup(snap_name, GFP_KERNEL);
-	if (!snap->name)
-		goto err;
-
+	snap->name = snap_name;
 	snap->id = snap_id;
 	snap->size = snap_size;
 	snap->features = snap_features;

 	return snap;
-
-err:
-	kfree(snap->name);
-	kfree(snap);
-
-	return ERR_PTR(ret);
 }

+/*
+ * Returns a dynamically-allocated snapshot name if successful, or a
+ * pointer-coded error otherwise.
+ */
 static char *rbd_dev_v1_snap_info(struct rbd_device *rbd_dev, u32 which,
 		u64 *snap_size, u64 *snap_features)
 {
@@ -3458,15 +3451,19 @@ static char *rbd_dev_v1_snap_info(struct
rbd_device *rbd_dev, u32 which,

 	rbd_assert(which < rbd_dev->header.snapc->num_snaps);

-	*snap_size = rbd_dev->header.snap_sizes[which];
-	*snap_features = 0;	/* No features for v1 */
-
 	/* Skip over names until we find the one we are looking for */

 	snap_name = rbd_dev->header.snap_names;
 	while (which--)
 		snap_name += strlen(snap_name) + 1;

+	snap_name = kstrdup(snap_name, GFP_KERNEL);
+	if (!snap_name);
+		return ERR_PTR(-ENOMEM);
+
+	*snap_size = rbd_dev->header.snap_sizes[which];
+	*snap_features = 0;	/* No features for v1 */
+
 	return snap_name;
 }

-- 
1.7.9.5

[PATCH 6/6] rbd: use rbd_obj_method_sync() return value

[Alex Elder]

Now that rbd_obj_method_sync() returns the number of bytes
returned by the method call, that value should be used by
callers to ensure we don't overrun the valid portion of the
buffer.

Fix the two spots that remained that weren't doing that,
rbd_dev_image_name() and rbd_dev_v2_snap_name().

Rearrange the error path slightly in rbd_dev_v2_snap_name().

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 2b5ba50..dcd8e58 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2614,7 +2614,8 @@ out_cancel:
 }

 /*
- * Synchronous osd object method call
+ * Synchronous osd object method call.  Returns the number of bytes
+ * returned in the outbound buffer, or a negative error code.
  */
 static int rbd_obj_method_sync(struct rbd_device *rbd_dev,
 			     const char *object_name,
@@ -3740,7 +3741,8 @@ static char *rbd_dev_image_name(struct rbd_device
*rbd_dev)
 	if (ret < 0)
 		goto out;
 	p = reply_buf;
-	end = reply_buf + size;
+	end = reply_buf + ret;
+
 	image_name = ceph_extract_encoded_string(&p, end, &len, GFP_KERNEL);
 	if (IS_ERR(image_name))
 		image_name = NULL;
@@ -3913,26 +3915,23 @@ static char *rbd_dev_v2_snap_name(struct
rbd_device *rbd_dev, u32 which)
 				&snap_id, sizeof (snap_id),
 				reply_buf, size, NULL);
 	dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret);
-	if (ret < 0)
+	if (ret < 0) {
+		snap_name = ERR_PTR(ret);
 		goto out;
+	}

 	p = reply_buf;
-	end = reply_buf + size;
+	end = reply_buf + ret;
 	snap_name = ceph_extract_encoded_string(&p, end, NULL, GFP_KERNEL);
-	if (IS_ERR(snap_name)) {
-		ret = PTR_ERR(snap_name);
+	if (IS_ERR(snap_name))
 		goto out;
-	} else {
-		dout("  snap_id 0x%016llx snap_name = %s\n",
-			(unsigned long long)le64_to_cpu(snap_id), snap_name);
-	}
-	kfree(reply_buf);

-	return snap_name;
+	dout("  snap_id 0x%016llx snap_name = %s\n",
+		(unsigned long long)le64_to_cpu(snap_id), snap_name);
 out:
 	kfree(reply_buf);

-	return ERR_PTR(ret);
+	return snap_name;
 }

 static char *rbd_dev_v2_snap_info(struct rbd_device *rbd_dev, u32 which,
-- 
1.7.9.5

[PATCH 5/6, v2] rbd: fix leak of format 2 snapshot names

[Alex Elder]

(I found a bug in this (which masked another bug).  Here is
version 2.  I have corrected it and updated the various
branches that depended on it.)


When the snapshot context for an rbd device gets updated (or the
initial one is recorded) a a list of snapshot structures is created
to represent them, one entry per snapshot.  Each entry includes a
dynamically-allocated copy of the snapshot name.

Currently the name is allocated in rbd_snap_create(), as a duplicate
of the passed-in name.

For format 1 images, the snapshot name provided is just a pointer to
an existing name.  But for format 2 images, the passed-in name is
already dynamically allocated, and in the the process of duplicating
it here we are leaking the passed-in name.

Fix this by dynamically allocating the name for format 1 snapshots
also, and then stop allocating a duplicate in rbd_snap_create().

Change rbd_dev_v1_snap_info() so none of its parameters is
side-effected unless it's going to return success.

This is part of:
    http://tracker.ceph.com/issues/4803

Signed-off-by: Alex Elder <elder@inktank.com>
---
v2: kill an errant semicolon; use loop iterator rather than "which"

 drivers/block/rbd.c |   30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 916741b..c15bb3f 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -3427,46 +3427,44 @@ static struct rbd_snap *rbd_snap_create(struct
rbd_device *rbd_dev,
 						u64 snap_features)
 {
 	struct rbd_snap *snap;
-	int ret;

 	snap = kzalloc(sizeof (*snap), GFP_KERNEL);
 	if (!snap)
 		return ERR_PTR(-ENOMEM);

-	ret = -ENOMEM;
-	snap->name = kstrdup(snap_name, GFP_KERNEL);
-	if (!snap->name)
-		goto err;
-
+	snap->name = snap_name;
 	snap->id = snap_id;
 	snap->size = snap_size;
 	snap->features = snap_features;

 	return snap;
-
-err:
-	kfree(snap->name);
-	kfree(snap);
-
-	return ERR_PTR(ret);
 }

+/*
+ * Returns a dynamically-allocated snapshot name if successful, or a
+ * pointer-coded error otherwise.
+ */
 static char *rbd_dev_v1_snap_info(struct rbd_device *rbd_dev, u32 which,
 		u64 *snap_size, u64 *snap_features)
 {
 	char *snap_name;
+	int i;

 	rbd_assert(which < rbd_dev->header.snapc->num_snaps);

-	*snap_size = rbd_dev->header.snap_sizes[which];
-	*snap_features = 0;	/* No features for v1 */
-
 	/* Skip over names until we find the one we are looking for */

 	snap_name = rbd_dev->header.snap_names;
-	while (which--)
+	for (i = 0; i < which; i++)
 		snap_name += strlen(snap_name) + 1;

+	snap_name = kstrdup(snap_name, GFP_KERNEL);
+	if (!snap_name)
+		return ERR_PTR(-ENOMEM);
+
+	*snap_size = rbd_dev->header.snap_sizes[which];
+	*snap_features = 0;	/* No features for v1 */
+
 	return snap_name;
 }

-- 
1.7.9.5

Re: [PATCH 1/6] rbd: fix leak of snapshots during initial probe

[Josh Durgin]

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

On 04/26/2013 05:05 AM, Alex Elder wrote:
> When an rbd image is initially mapped, its snapshot context is
> collected, and then a list of snapshot entries representing the
> snapshots in that context is created.  The list is created using
> rbd_dev_snaps_update().  (This function also supports updating an
> existing snapshot list based on a new snapshot context.)
>
> If an error occurs, updating the list is aborted, and the list is
> currently left as-is, in an inconsistent state.  At that point,
> there may be a partially-constructed list, but the calling functions
> (rbd_dev_probe_finish() from rbd_dev_probe() from rbd_add()) never
> clean them up.  So this constitutes a leak.
>
> A snapshot list that is inconsistent with the current snapshot
> context is of no use, and might even be actively bad.  So rather
> than just having the caller clean it up, have rbd_dev_snaps_update()
> just clear out the entire snapshot list in the event an error
> occurs.
>
> The other place rbd_dev_snaps_update() is used is when a refresh is
> triggered, either because of a watch callback or via a write to the
> /sys/bus/rbd/devices/<id>/refresh interface.  An error while
> updating the snapshots has no substantive effect in either of those
> cases, but one of them issues a warning.  Move that warning to the
> common rbd_dev_refresh() function so it gets issued regardless of
> how it got initiated.
>
> This is part of:
>      http://tracker.ceph.com/issues/4803
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   50
> ++++++++++++++++++++++++++++++--------------------
>   1 file changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 515fbf9..28b652c 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2521,7 +2521,6 @@ static void rbd_watch_cb(u64 ver, u64 notify_id,
> u8 opcode, void *data)
>   {
>   	struct rbd_device *rbd_dev = (struct rbd_device *)data;
>   	u64 hver;
> -	int rc;
>
>   	if (!rbd_dev)
>   		return;
> @@ -2529,10 +2528,7 @@ static void rbd_watch_cb(u64 ver, u64 notify_id,
> u8 opcode, void *data)
>   	dout("%s: \"%s\" notify_id %llu opcode %u\n", __func__,
>   		rbd_dev->header_name, (unsigned long long) notify_id,
>   		(unsigned int) opcode);
> -	rc = rbd_dev_refresh(rbd_dev, &hver);
> -	if (rc)
> -		rbd_warn(rbd_dev, "got notification but failed to "
> -			   " update snaps: %d\n", rc);
> +	(void)rbd_dev_refresh(rbd_dev, &hver);
>
>   	rbd_obj_notify_ack(rbd_dev, hver, notify_id);
>   }
> @@ -3085,6 +3081,9 @@ static int rbd_dev_refresh(struct rbd_device
> *rbd_dev, u64 *hver)
>   		ret = rbd_dev_v2_refresh(rbd_dev, hver);
>   	mutex_unlock(&ctl_mutex);
>   	revalidate_disk(rbd_dev->disk);
> +	if (ret)
> +		rbd_warn(rbd_dev, "got notification but failed to "
> +			   " update snaps: %d\n", ret);
>
>   	return ret;
>   }
> @@ -4010,6 +4009,11 @@ out:
>    * Assumes the snapshots in the snapshot context are sorted by
>    * snapshot id, highest id first.  (Snapshots in the rbd_dev's list
>    * are also maintained in that order.)
> + *
> + * Note that any error occurs while updating the snapshot list
> + * aborts the update, and the entire list is cleared.  The snapshot
> + * list becomes inconsistent at that point anyway, so it might as
> + * well be empty.
>    */
>   static int rbd_dev_snaps_update(struct rbd_device *rbd_dev)
>   {
> @@ -4018,8 +4022,9 @@ static int rbd_dev_snaps_update(struct rbd_device
> *rbd_dev)
>   	struct list_head *head = &rbd_dev->snaps;
>   	struct list_head *links = head->next;
>   	u32 index = 0;
> +	int ret = 0;
>
> -	dout("%s: snap count is %u\n", __func__, (unsigned int) snap_count);
> +	dout("%s: snap count is %u\n", __func__, (unsigned int)snap_count);
>   	while (index < snap_count || links != head) {
>   		u64 snap_id;
>   		struct rbd_snap *snap;
> @@ -4040,17 +4045,17 @@ static int rbd_dev_snaps_update(struct
> rbd_device *rbd_dev)
>   			 * A previously-existing snapshot is not in
>   			 * the new snap context.
>   			 *
> -			 * If the now missing snapshot is the one the
> -			 * image is mapped to, clear its exists flag
> -			 * so we can avoid sending any more requests
> -			 * to it.
> +			 * If the now-missing snapshot is the one
> +			 * the image represents, clear its existence
> +			 * flag so we can avoid sending any more
> +			 * requests to it.
>   			 */
>   			if (rbd_dev->spec->snap_id == snap->id)
>   				clear_bit(RBD_DEV_FLAG_EXISTS, &rbd_dev->flags);
>   			dout("removing %ssnap id %llu\n",
>   				rbd_dev->spec->snap_id == snap->id ?
>   							"mapped " : "",
> -				(unsigned long long) snap->id);
> +				(unsigned long long)snap->id);
>   			rbd_remove_snap_dev(snap);
>
>   			/* Done with this list entry; advance */
> @@ -4061,11 +4066,14 @@ static int rbd_dev_snaps_update(struct
> rbd_device *rbd_dev)
>
>   		snap_name = rbd_dev_snap_info(rbd_dev, index,
>   					&snap_size, &snap_features);
> -		if (IS_ERR(snap_name))
> -			return PTR_ERR(snap_name);
> +		if (IS_ERR(snap_name)) {
> +			ret = PTR_ERR(snap_name);
> +			dout("failed to get snap info, error %d\n", ret);
> +			goto out_err;
> +		}
>
> -		dout("entry %u: snap_id = %llu\n", (unsigned int) snap_count,
> -			(unsigned long long) snap_id);
> +		dout("entry %u: snap_id = %llu\n", (unsigned int)snap_count,
> +			(unsigned long long)snap_id);
>   		if (!snap || (snap_id != CEPH_NOSNAP && snap->id < snap_id)) {
>   			struct rbd_snap *new_snap;
>
> @@ -4074,11 +4082,9 @@ static int rbd_dev_snaps_update(struct rbd_device
> *rbd_dev)
>   			new_snap = __rbd_add_snap_dev(rbd_dev, snap_name,
>   					snap_id, snap_size, snap_features);
>   			if (IS_ERR(new_snap)) {
> -				int err = PTR_ERR(new_snap);
> -
> -				dout("  failed to add dev, error %d\n", err);
> -
> -				return err;
> +				ret = PTR_ERR(new_snap);
> +				dout("  failed to add dev, error %d\n", ret);
> +				goto out_err;
>   			}
>
>   			/* New goes before existing, or at end of list */
> @@ -4109,6 +4115,10 @@ static int rbd_dev_snaps_update(struct rbd_device
> *rbd_dev)
>   	dout("%s: done\n", __func__);
>
>   	return 0;
> +out_err:
> +	rbd_remove_all_snaps(rbd_dev);
> +
> +	return ret;
>   }
>
>   static int rbd_bus_add_dev(struct rbd_device *rbd_dev)
>

Re: [PATCH 2/6] rbd: make snap_size order parameter optional

[Josh Durgin]

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

On 04/26/2013 05:05 AM, Alex Elder wrote:
> Only one of the two callers of _rbd_dev_v2_snap_size() needs the
> order value returned.  So make that an optional argument--a null
> pointer if the caller doesn't need it.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |    6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 28b652c..1e01f0d 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -3494,7 +3494,8 @@ static int _rbd_dev_v2_snap_size(struct rbd_device
> *rbd_dev, u64 snap_id,
>   	if (ret < sizeof (size_buf))
>   		return -ERANGE;
>
> -	*order = size_buf.order;
> +	if (order)
> +		*order = size_buf.order;
>   	*snap_size = le64_to_cpu(size_buf.size);
>
>   	dout("  snap_id 0x%016llx order = %u, snap_size = %llu\n",
> @@ -3939,11 +3940,10 @@ static char *rbd_dev_v2_snap_info(struct
> rbd_device *rbd_dev, u32 which,
>   		u64 *snap_size, u64 *snap_features)
>   {
>   	u64 snap_id;
> -	u8 order;
>   	int ret;
>
>   	snap_id = rbd_dev->header.snapc->snaps[which];
> -	ret = _rbd_dev_v2_snap_size(rbd_dev, snap_id, &order, snap_size);
> +	ret = _rbd_dev_v2_snap_size(rbd_dev, snap_id, NULL, snap_size);
>   	if (ret)
>   		return ERR_PTR(ret);
>   	ret = _rbd_dev_v2_snap_features(rbd_dev, snap_id, snap_features);
>

Re: [PATCH 3/6] rbd: only update values on snap_info success

[Josh Durgin]

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

On 04/26/2013 05:06 AM, Alex Elder wrote:
> Change rbd_dev_v2_snap_info() so it only ever sets values of the
> size and features parameters if looking up the snapshot name was
> successful.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   24 +++++++++++++++++++-----
>   1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 1e01f0d..e7d10d3 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -3908,6 +3908,7 @@ static char *rbd_dev_v2_snap_name(struct
> rbd_device *rbd_dev, u32 which)
>   	if (!reply_buf)
>   		return ERR_PTR(-ENOMEM);
>
> +	rbd_assert(which < rbd_dev->header.snapc->num_snaps);
>   	snap_id = cpu_to_le64(rbd_dev->header.snapc->snaps[which]);
>   	ret = rbd_obj_method_sync(rbd_dev, rbd_dev->header_name,
>   				"rbd", "get_snapshot_name",
> @@ -3940,17 +3941,30 @@ static char *rbd_dev_v2_snap_info(struct
> rbd_device *rbd_dev, u32 which,
>   		u64 *snap_size, u64 *snap_features)
>   {
>   	u64 snap_id;
> +	u64 size;
> +	u64 features;
> +	char *snap_name;
>   	int ret;
>
> +	rbd_assert(which < rbd_dev->header.snapc->num_snaps);
>   	snap_id = rbd_dev->header.snapc->snaps[which];
> -	ret = _rbd_dev_v2_snap_size(rbd_dev, snap_id, NULL, snap_size);
> +	ret = _rbd_dev_v2_snap_size(rbd_dev, snap_id, NULL, &size);
>   	if (ret)
> -		return ERR_PTR(ret);
> -	ret = _rbd_dev_v2_snap_features(rbd_dev, snap_id, snap_features);
> +		goto out_err;
> +
> +	ret = _rbd_dev_v2_snap_features(rbd_dev, snap_id, &features);
>   	if (ret)
> -		return ERR_PTR(ret);
> +		goto out_err;
> +
> +	snap_name = rbd_dev_v2_snap_name(rbd_dev, which);
> +	if (!IS_ERR(snap_name)) {
> +		*snap_size = size;
> +		*snap_features = features;
> +	}
>
> -	return rbd_dev_v2_snap_name(rbd_dev, which);
> +	return snap_name;
> +out_err:
> +	return ERR_PTR(ret);
>   }
>
>   static char *rbd_dev_snap_info(struct rbd_device *rbd_dev, u32 which,
>

Re: [PATCH 4/6] rbd: rename __rbd_add_snap_dev()

[Josh Durgin]

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

On 04/26/2013 05:06 AM, Alex Elder wrote:
> Rename __rbd_add_snap_dev() to be rbd_snap_create().  We no longer
> have devices for non-mapped snapshots, and we're not actually
> "adding" it to the list in this function, just creating it.
>
> Rename rbd_remove_snap_dev() to be rbd_snap_destroy() for reasons
> similar to the above.  Stop having this function delete the snapshot
> from its list (to be symmetrical with its create counterpart) and do
> that in the caller instead.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   19 +++++++++++--------
>   1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index e7d10d3..916741b 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -359,7 +359,7 @@ static int rbd_img_request_submit(struct
> rbd_img_request *img_request);
>   static int rbd_dev_snaps_update(struct rbd_device *rbd_dev);
>
>   static void rbd_dev_release(struct device *dev);
> -static void rbd_remove_snap_dev(struct rbd_snap *snap);
> +static void rbd_snap_destroy(struct rbd_snap *snap);
>
>   static ssize_t rbd_add(struct bus_type *bus, const char *buf,
>   		       size_t count);
> @@ -3010,8 +3010,10 @@ static void rbd_remove_all_snaps(struct
> rbd_device *rbd_dev)
>   	struct rbd_snap *snap;
>   	struct rbd_snap *next;
>
> -	list_for_each_entry_safe(snap, next, &rbd_dev->snaps, node)
> -		rbd_remove_snap_dev(snap);
> +	list_for_each_entry_safe(snap, next, &rbd_dev->snaps, node) {
> +		list_del(&snap->node);
> +		rbd_snap_destroy(snap);
> +	}
>   }
>
>   static void rbd_update_mapping_size(struct rbd_device *rbd_dev)
> @@ -3413,14 +3415,13 @@ static void rbd_dev_destroy(struct rbd_device
> *rbd_dev)
>   	kfree(rbd_dev);
>   }
>
> -static void rbd_remove_snap_dev(struct rbd_snap *snap)
> +static void rbd_snap_destroy(struct rbd_snap *snap)
>   {
> -	list_del(&snap->node);
>   	kfree(snap->name);
>   	kfree(snap);
>   }
>
> -static struct rbd_snap *__rbd_add_snap_dev(struct rbd_device *rbd_dev,
> +static struct rbd_snap *rbd_snap_create(struct rbd_device *rbd_dev,
>   						const char *snap_name,
>   						u64 snap_id, u64 snap_size,
>   						u64 snap_features)
> @@ -4070,7 +4071,9 @@ static int rbd_dev_snaps_update(struct rbd_device
> *rbd_dev)
>   				rbd_dev->spec->snap_id == snap->id ?
>   							"mapped " : "",
>   				(unsigned long long)snap->id);
> -			rbd_remove_snap_dev(snap);
> +
> +			list_del(&snap->node);
> +			rbd_snap_destroy(snap);
>
>   			/* Done with this list entry; advance */
>
> @@ -4093,7 +4096,7 @@ static int rbd_dev_snaps_update(struct rbd_device
> *rbd_dev)
>
>   			/* We haven't seen this snapshot before */
>
> -			new_snap = __rbd_add_snap_dev(rbd_dev, snap_name,
> +			new_snap = rbd_snap_create(rbd_dev, snap_name,
>   					snap_id, snap_size, snap_features);
>   			if (IS_ERR(new_snap)) {
>   				ret = PTR_ERR(new_snap);
>

Re: [PATCH 5/6] rbd: fix leak of format 2 snapshot names

[Josh Durgin]

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

On 04/26/2013 05:06 AM, Alex Elder wrote:
> When the snapshot context for an rbd device gets updated (or the
> initial one is recorded) a a list of snapshot structures is created
> to represent them, one entry per snapshot.  Each entry includes a
> dynamically-allocated copy of the snapshot name.
>
> Currently the name is allocated in rbd_snap_create(), as a duplicate
> of the passed-in name.
>
> For format 1 images, the snapshot name provided is just a pointer to
> an existing name.  But for format 2 images, the passed-in name is
> already dynamically allocated, and in the the process of duplicating
> it here we are leaking the passed-in name.
>
> Fix this by dynamically allocating the name for format 1 snapshots
> also, and then stop allocating a duplicate in rbd_snap_create().
>
> Change rbd_dev_v1_snap_info() so none of its parameters is
> side-effected unless it's going to return success.
>
> This is part of:
>      http://tracker.ceph.com/issues/4803
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   27 ++++++++++++---------------
>   1 file changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 916741b..2b5ba50 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -3427,30 +3427,23 @@ static struct rbd_snap *rbd_snap_create(struct
> rbd_device *rbd_dev,
>   						u64 snap_features)
>   {
>   	struct rbd_snap *snap;
> -	int ret;
>
>   	snap = kzalloc(sizeof (*snap), GFP_KERNEL);
>   	if (!snap)
>   		return ERR_PTR(-ENOMEM);
>
> -	ret = -ENOMEM;
> -	snap->name = kstrdup(snap_name, GFP_KERNEL);
> -	if (!snap->name)
> -		goto err;
> -
> +	snap->name = snap_name;
>   	snap->id = snap_id;
>   	snap->size = snap_size;
>   	snap->features = snap_features;
>
>   	return snap;
> -
> -err:
> -	kfree(snap->name);
> -	kfree(snap);
> -
> -	return ERR_PTR(ret);
>   }
>
> +/*
> + * Returns a dynamically-allocated snapshot name if successful, or a
> + * pointer-coded error otherwise.
> + */
>   static char *rbd_dev_v1_snap_info(struct rbd_device *rbd_dev, u32 which,
>   		u64 *snap_size, u64 *snap_features)
>   {
> @@ -3458,15 +3451,19 @@ static char *rbd_dev_v1_snap_info(struct
> rbd_device *rbd_dev, u32 which,
>
>   	rbd_assert(which < rbd_dev->header.snapc->num_snaps);
>
> -	*snap_size = rbd_dev->header.snap_sizes[which];
> -	*snap_features = 0;	/* No features for v1 */
> -
>   	/* Skip over names until we find the one we are looking for */
>
>   	snap_name = rbd_dev->header.snap_names;
>   	while (which--)
>   		snap_name += strlen(snap_name) + 1;
>
> +	snap_name = kstrdup(snap_name, GFP_KERNEL);
> +	if (!snap_name);
> +		return ERR_PTR(-ENOMEM);
> +
> +	*snap_size = rbd_dev->header.snap_sizes[which];
> +	*snap_features = 0;	/* No features for v1 */
> +
>   	return snap_name;
>   }
>

Re: [PATCH 5/6] rbd: fix leak of format 2 snapshot names

[Josh Durgin]

On 04/29/2013 08:16 AM, Josh Durgin wrote:
> Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

That was meant for v2 of this patch.

> On 04/26/2013 05:06 AM, Alex Elder wrote:
>> When the snapshot context for an rbd device gets updated (or the
>> initial one is recorded) a a list of snapshot structures is created
>> to represent them, one entry per snapshot.  Each entry includes a
>> dynamically-allocated copy of the snapshot name.
>>
>> Currently the name is allocated in rbd_snap_create(), as a duplicate
>> of the passed-in name.
>>
>> For format 1 images, the snapshot name provided is just a pointer to
>> an existing name.  But for format 2 images, the passed-in name is
>> already dynamically allocated, and in the the process of duplicating
>> it here we are leaking the passed-in name.
>>
>> Fix this by dynamically allocating the name for format 1 snapshots
>> also, and then stop allocating a duplicate in rbd_snap_create().
>>
>> Change rbd_dev_v1_snap_info() so none of its parameters is
>> side-effected unless it's going to return success.
>>
>> This is part of:
>>      http://tracker.ceph.com/issues/4803
>>
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---

Re: [PATCH 6/6] rbd: use rbd_obj_method_sync() return value

[Josh Durgin]

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

On 04/26/2013 05:06 AM, Alex Elder wrote:
> Now that rbd_obj_method_sync() returns the number of bytes
> returned by the method call, that value should be used by
> callers to ensure we don't overrun the valid portion of the
> buffer.
>
> Fix the two spots that remained that weren't doing that,
> rbd_dev_image_name() and rbd_dev_v2_snap_name().
>
> Rearrange the error path slightly in rbd_dev_v2_snap_name().
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   25 ++++++++++++-------------
>   1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 2b5ba50..dcd8e58 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2614,7 +2614,8 @@ out_cancel:
>   }
>
>   /*
> - * Synchronous osd object method call
> + * Synchronous osd object method call.  Returns the number of bytes
> + * returned in the outbound buffer, or a negative error code.
>    */
>   static int rbd_obj_method_sync(struct rbd_device *rbd_dev,
>   			     const char *object_name,
> @@ -3740,7 +3741,8 @@ static char *rbd_dev_image_name(struct rbd_device
> *rbd_dev)
>   	if (ret < 0)
>   		goto out;
>   	p = reply_buf;
> -	end = reply_buf + size;
> +	end = reply_buf + ret;
> +
>   	image_name = ceph_extract_encoded_string(&p, end, &len, GFP_KERNEL);
>   	if (IS_ERR(image_name))
>   		image_name = NULL;
> @@ -3913,26 +3915,23 @@ static char *rbd_dev_v2_snap_name(struct
> rbd_device *rbd_dev, u32 which)
>   				&snap_id, sizeof (snap_id),
>   				reply_buf, size, NULL);
>   	dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		snap_name = ERR_PTR(ret);
>   		goto out;
> +	}
>
>   	p = reply_buf;
> -	end = reply_buf + size;
> +	end = reply_buf + ret;
>   	snap_name = ceph_extract_encoded_string(&p, end, NULL, GFP_KERNEL);
> -	if (IS_ERR(snap_name)) {
> -		ret = PTR_ERR(snap_name);
> +	if (IS_ERR(snap_name))
>   		goto out;
> -	} else {
> -		dout("  snap_id 0x%016llx snap_name = %s\n",
> -			(unsigned long long)le64_to_cpu(snap_id), snap_name);
> -	}
> -	kfree(reply_buf);
>
> -	return snap_name;
> +	dout("  snap_id 0x%016llx snap_name = %s\n",
> +		(unsigned long long)le64_to_cpu(snap_id), snap_name);
>   out:
>   	kfree(reply_buf);
>
> -	return ERR_PTR(ret);
> +	return snap_name;
>   }
>
>   static char *rbd_dev_v2_snap_info(struct rbd_device *rbd_dev, u32 which,
>