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 <bhelgaas@google.com>
Signed-off-by: Alex Elder <elder@inktank.com>
Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 5f64ba77bc7fb8eefde9afafc48cdba0cda072a3..3a897a531e9c82ae1471fe39a8b544ffe825872a 100644 (file)
--- 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);