From 7262cfca430a1a0e0707149af29ae86bc0ded230 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Thu, 16 May 2013 15:04:20 -0500 Subject: rbd: don't destroy ceph_opts in rbd_add() Whether rbd_client_create() successfully creates a new client or not, it takes responsibility for getting the ceph_opts structure it's passed destroyed. If successful, the structure becomes associated with the created client; if not, rbd_client_create() will destroy it. Previously, rbd_get_client() would call ceph_destroy_options() if rbd_get_client() failed, and that meant it got called twice. That led freeing various pointers more than once, which is never a good idea. This resolves: http://tracker.ceph.com/issues/4559 Cc: stable@vger.kernel.org # 3.8+ Reported-by: Dan van der Ster Signed-off-by: Alex Elder Reviewed-by: Josh Durgin --- drivers/block/rbd.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) (limited to 'drivers/block/rbd.c') diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 6b872f21977..5f64ba77bc7 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -521,8 +521,8 @@ static const struct block_device_operations rbd_bd_ops = { }; /* - * Initialize an rbd client instance. - * We own *ceph_opts. + * Initialize an rbd client instance. Success or not, this function + * consumes ceph_opts. */ static struct rbd_client *rbd_client_create(struct ceph_options *ceph_opts) { @@ -677,7 +677,8 @@ static int parse_rbd_opts_token(char *c, void *private) /* * Get a ceph client with specific addr and configuration, if one does - * not exist create it. + * not exist create it. Either way, ceph_opts is consumed by this + * function. */ static struct rbd_client *rbd_get_client(struct ceph_options *ceph_opts) { @@ -4994,7 +4995,6 @@ static ssize_t rbd_add(struct bus_type *bus, rc = PTR_ERR(rbdc); goto err_out_args; } - ceph_opts = NULL; /* rbd_dev client now owns this */ /* pick the pool */ osdc = &rbdc->client->osdc; @@ -5038,8 +5038,6 @@ err_out_rbd_dev: err_out_client: rbd_put_client(rbdc); err_out_args: - if (ceph_opts) - ceph_destroy_options(ceph_opts); kfree(rbd_opts); rbd_spec_put(spec); err_out_module: -- cgit v1.2.3-70-g09d2 From 3abef3b3585bbc67d56fdc9c67761a900fb4b69d Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Mon, 13 May 2013 20:35:37 -0500 Subject: rbd: fix cleanup in rbd_add() Bjorn Helgaas pointed out that a recent commit introduced a use-after-free condition in an error path for rbd_add(). He correctly stated: I think b536f69a3a5 "rbd: set up devices only for mapped images" introduced a use-after-free error in rbd_add(): ... If rbd_dev_device_setup() returns an error, we call rbd_dev_image_release(), which ultimately kfrees rbd_dev. Then we call rbd_dev_destroy(), which references fields in the already-freed rbd_dev struct before kfreeing it again. The simple fix is to return the error code after the call to rbd_dev_image_release(). Closer examination revealed that there's no need to clean up rbd_opts in that function, so fix that too. Update some other comments that have also become out of date. Reported-by: Bjorn Helgaas Signed-off-by: Alex Elder Reviewed-by: Josh Durgin --- drivers/block/rbd.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) (limited to 'drivers/block/rbd.c') diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 5f64ba77bc7..3a897a531e9 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -4700,8 +4700,10 @@ out: return ret; } -/* Undo whatever state changes are made by v1 or v2 image probe */ - +/* + * Undo whatever state changes are made by v1 or v2 header info + * call. + */ static void rbd_dev_unprobe(struct rbd_device *rbd_dev) { struct rbd_image_header *header; @@ -4905,9 +4907,10 @@ static int rbd_dev_image_probe(struct rbd_device *rbd_dev, bool mapping) int tmp; /* - * Get the id from the image id object. If it's not a - * format 2 image, we'll get ENOENT back, and we'll assume - * it's a format 1 image. + * Get the id from the image id object. Unless there's an + * error, rbd_dev->spec->image_id will be filled in with + * a dynamically-allocated string, and rbd_dev->image_format + * will be set to either 1 or 2. */ ret = rbd_dev_image_id(rbd_dev); if (ret) @@ -5029,16 +5032,18 @@ static ssize_t rbd_add(struct bus_type *bus, rbd_dev->mapping.read_only = read_only; rc = rbd_dev_device_setup(rbd_dev); - if (!rc) - return count; + if (rc) { + rbd_dev_image_release(rbd_dev); + goto err_out_module; + } + + return count; - rbd_dev_image_release(rbd_dev); err_out_rbd_dev: rbd_dev_destroy(rbd_dev); err_out_client: rbd_put_client(rbdc); err_out_args: - kfree(rbd_opts); rbd_spec_put(spec); err_out_module: module_put(THIS_MODULE); -- cgit v1.2.3-70-g09d2 From 3a96d5cd7bdce45d5dded75c3a62d4fb98050280 Mon Sep 17 00:00:00 2001 From: Josh Durgin Date: Wed, 12 Jun 2013 19:15:06 -0700 Subject: rbd: use the correct length for format 2 object names Format 2 objects use 16 characters for the object name suffix to be able to express the full 64-bit range of object numbers. Format 1 images only use 12 characters for this. Using 12-character names for format 2 caused userspace and kernel rbd clients to read differently named objects, which made an image written by one client look empty to the other client. CC: stable@vger.kernel.org # 3.9+ Reported-by: Chris Dunlop Signed-off-by: Josh Durgin Reviewed-by: Sage Weil --- drivers/block/rbd.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'drivers/block/rbd.c') diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 3a897a531e9..cc7c60e8f27 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -1038,12 +1038,16 @@ static const char *rbd_segment_name(struct rbd_device *rbd_dev, u64 offset) char *name; u64 segment; int ret; + char *name_format; name = kmem_cache_alloc(rbd_segment_name_cache, GFP_NOIO); if (!name) return NULL; segment = offset >> rbd_dev->header.obj_order; - ret = snprintf(name, MAX_OBJ_NAME_SIZE + 1, "%s.%012llx", + name_format = "%s.%012llx"; + if (rbd_dev->image_format == 2) + name_format = "%s.%016llx"; + ret = snprintf(name, MAX_OBJ_NAME_SIZE + 1, name_format, rbd_dev->header.object_prefix, segment); if (ret < 0 || ret > MAX_OBJ_NAME_SIZE) { pr_err("error formatting segment name for #%llu (%d)\n", -- cgit v1.2.3-70-g09d2 From 1617e40c1eeeeb857ff4b66acee20ed2acc1b5e7 Mon Sep 17 00:00:00 2001 From: Josh Durgin Date: Wed, 12 Jun 2013 14:43:10 -0700 Subject: rbd: fetch object order before using it rbd_dev_v2_header_onetime() fetches striping information, and checks whether the image can be read by compariing the stripe unit to the object size. It determines the object size by shifting the object order, which is 0 at this point since it has not been read yet. Move the call to get the image size and object order before rbd_dev_v2_header_onetime() so it is set before use. Signed-off-by: Josh Durgin Reviewed-by: Sage Weil --- drivers/block/rbd.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'drivers/block/rbd.c') diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index cc7c60e8f27..7c66173e2d8 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -4245,6 +4245,10 @@ static int rbd_dev_v2_header_info(struct rbd_device *rbd_dev) down_write(&rbd_dev->header_rwsem); + ret = rbd_dev_v2_image_size(rbd_dev); + if (ret) + goto out; + if (first_time) { ret = rbd_dev_v2_header_onetime(rbd_dev); if (ret) @@ -4278,10 +4282,6 @@ static int rbd_dev_v2_header_info(struct rbd_device *rbd_dev) "is EXPERIMENTAL!"); } - ret = rbd_dev_v2_image_size(rbd_dev); - if (ret) - goto out; - if (rbd_dev->spec->snap_id == CEPH_NOSNAP) if (rbd_dev->mapping.size != rbd_dev->header.image_size) rbd_dev->mapping.size = rbd_dev->header.image_size; -- cgit v1.2.3-70-g09d2 From d2d1f17a0dad823a4cb71583433d26cd7f734e08 Mon Sep 17 00:00:00 2001 From: Josh Durgin Date: Wed, 26 Jun 2013 12:56:17 -0700 Subject: rbd: send snapshot context with writes Sending the right snapshot context with each write is required for snapshots to work. Due to the ordering of calls, the snapshot context is never set for any requests. This causes writes to the current version of the image to be reflected in all snapshots, which are supposed to be read-only. This happens because rbd_osd_req_format_write() sets the snapshot context based on obj_request->img_request. At this point, however, obj_request->img_request has not been set yet, to the snapshot context is set to NULL. Fix this by moving rbd_img_obj_request_add(), which sets obj_request->img_request, before the osd request formatting calls. This resolves: http://tracker.ceph.com/issues/5465 Reported-by: Karol Jurak Signed-off-by: Josh Durgin Reviewed-by: Sage Weil Reviewed-by: Alex Elder --- drivers/block/rbd.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'drivers/block/rbd.c') diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 7c66173e2d8..c8eb9cb77d3 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -2254,13 +2254,17 @@ static int rbd_img_request_fill(struct rbd_img_request *img_request, obj_request->pages, length, offset & ~PAGE_MASK, false, false); + /* + * set obj_request->img_request before formatting + * the osd_request so that it gets the right snapc + */ + rbd_img_obj_request_add(img_request, obj_request); if (write_request) rbd_osd_req_format_write(obj_request); else rbd_osd_req_format_read(obj_request); obj_request->img_offset = img_offset; - rbd_img_obj_request_add(img_request, obj_request); img_offset += length; resid -= length; -- cgit v1.2.3-70-g09d2