Commit 3abef3b3 authored by Alex Elder's avatar Alex Elder

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 b536f69a "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: default avatarBjorn Helgaas <bhelgaas@google.com>
Signed-off-by: default avatarAlex Elder <elder@inktank.com>
Reviewed-by: default avatarJosh Durgin <josh.durgin@inktank.com>
parent 7262cfca
...@@ -4700,8 +4700,10 @@ static int rbd_dev_image_id(struct rbd_device *rbd_dev) ...@@ -4700,8 +4700,10 @@ static int rbd_dev_image_id(struct rbd_device *rbd_dev)
return ret; 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) static void rbd_dev_unprobe(struct rbd_device *rbd_dev)
{ {
struct rbd_image_header *header; struct rbd_image_header *header;
...@@ -4905,9 +4907,10 @@ static int rbd_dev_image_probe(struct rbd_device *rbd_dev, bool mapping) ...@@ -4905,9 +4907,10 @@ static int rbd_dev_image_probe(struct rbd_device *rbd_dev, bool mapping)
int tmp; int tmp;
/* /*
* Get the id from the image id object. If it's not a * Get the id from the image id object. Unless there's an
* format 2 image, we'll get ENOENT back, and we'll assume * error, rbd_dev->spec->image_id will be filled in with
* it's a format 1 image. * a dynamically-allocated string, and rbd_dev->image_format
* will be set to either 1 or 2.
*/ */
ret = rbd_dev_image_id(rbd_dev); ret = rbd_dev_image_id(rbd_dev);
if (ret) if (ret)
...@@ -5029,16 +5032,18 @@ static ssize_t rbd_add(struct bus_type *bus, ...@@ -5029,16 +5032,18 @@ static ssize_t rbd_add(struct bus_type *bus,
rbd_dev->mapping.read_only = read_only; rbd_dev->mapping.read_only = read_only;
rc = rbd_dev_device_setup(rbd_dev); rc = rbd_dev_device_setup(rbd_dev);
if (!rc) if (rc) {
return count; rbd_dev_image_release(rbd_dev);
goto err_out_module;
}
return count;
rbd_dev_image_release(rbd_dev);
err_out_rbd_dev: err_out_rbd_dev:
rbd_dev_destroy(rbd_dev); rbd_dev_destroy(rbd_dev);
err_out_client: err_out_client:
rbd_put_client(rbdc); rbd_put_client(rbdc);
err_out_args: err_out_args:
kfree(rbd_opts);
rbd_spec_put(spec); rbd_spec_put(spec);
err_out_module: err_out_module:
module_put(THIS_MODULE); module_put(THIS_MODULE);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment