Skip to contentSkip to conversation

rbd: do not allow remove of mounted-on image

PATCH Superseded Alex Elder 2 participants 4 messages Nov 16, 2012 – Nov 19, 2012

Subsystems (2)
Block layer RADOS block device (RBD)
lore
Share & tools
lore

Alex Elder

author
#0 lore raw

PATCH 1/1 rbd: do not allow remove of mounted-on image

There is no check in rbd_remove() to see if anybody holds ope the image being removed. That's not cool.

Add a simple open count that goes up and down with opens and closes (releases) of the device, and don't allow an rbd image to be removed if the count is non-zero. Both functions are protected by the bd_mutex so there's no need for any other concurrency protection.

Signed-off-by Alex Elder <[email protected]>
---
 drivers/block/rbd.c |    9 +++++++++
 1 file changed, 9 insertions(+)
drivers/block/rbd.c +1 -0
index fba0822..9d9a2f3 100644
@@ -255,6 +255,7 @@ struct rbd_device {
 
 	/* sysfs related */
 	struct device		dev;
+	unsigned long		open_count;
 };
 
 static DEFINE_MUTEX(ctl_mutex);	  /* Serialize open/close/setup/teardown */

@@ -358,6 +359,7 @@ static int rbd_open(struct block_device *bdev,

fmode_t mode)

rbd_get_dev(rbd_dev); set_device_ro(bdev, rbd_dev->mapping.read_only);
+ rbd_dev->open_count++;

return 0;
}
@@ -366,6 +368,8 @@ static int rbd_release(struct gendisk *disk, fmode_t mode)
{
struct rbd_device *rbd_dev = disk->private_data;

+ rbd_assert(rbd_dev->open_count > 0);
+ rbd_dev->open_count--;
rbd_put_dev(rbd_dev);

return 0;
@@ -3764,6 +3768,11 @@ static ssize_t rbd_remove(struct bus_type *bus,
goto done;
}

+ if (rbd_dev->open_count) {
+ ret = -EBUSY;
+ goto done;
+ }
+
rbd_remove_all_snaps(rbd_dev);
rbd_bus_del_dev(rbd_dev);

Signature · 1 line
1.7.9.5
#1 lore raw
Quoted 45 lines · Alex Elder

There is no check in rbd_remove() to see if anybody holds ope the image being removed. That's not cool.

Add a simple open count that goes up and down with opens and closes (releases) of the device, and don't allow an rbd image to be removed if the count is non-zero. Both functions are protected by the bd_mutex so there's no need for any other concurrency protection.

Signed-off-by Alex Elder <[email protected]>
---
  drivers/block/rbd.c |    9 +++++++++
  1 file changed, 9 insertions(+)
drivers/block/rbd.c +2 -0
index fba0822..9d9a2f3 100644
@@ -255,6 +255,7 @@ struct rbd_device {
 
  	/* sysfs related */
  	struct device		dev;
+	unsigned long		open_count;
  };
 
  static DEFINE_MUTEX(ctl_mutex);	  /* Serialize open/close/setup/teardown */

@@ -358,6 +359,7 @@ static int rbd_open(struct block_device *bdev,
 fmode_t mode)
 
  	rbd_get_dev(rbd_dev);
  	set_device_ro(bdev, rbd_dev->mapping.read_only);
+	rbd_dev->open_count++;
 
  	return 0;

  }
drivers/block/rbd.c +2 -0
@@ -366,6 +368,8 @@ static int rbd_release(struct gendisk *disk, fmode_t
 mode)
  {
  	struct rbd_device *rbd_dev = disk->private_data;
 
+	rbd_assert(rbd_dev->open_count > 0);
+	rbd_dev->open_count--;
  	rbd_put_dev(rbd_dev);

return 0;

drivers/block/rbd.c static ssize_t rbd_remove(struct bus_type *bus, · line 3775

Quoted 8 lines
drivers/block/rbd.c +5 -0
@@ -3764,6 +3768,11 @@ static ssize_t rbd_remove(struct bus_type *bus,
  		goto done;
  	}
 
+	if (rbd_dev->open_count) {
+		ret = -EBUSY;
+		goto done;
+	}
+

Is this protected by bd_mutex?

Quoted 3 lines
drivers/block/rbd.c +0 -0
  	rbd_remove_all_snaps(rbd_dev);
  	rbd_bus_del_dev(rbd_dev);
author
#2 lore raw
Quoted 50 lines · Josh Durgin
Quoted 49 lines · Alex Elder

There is no check in rbd_remove() to see if anybody holds ope the image being removed. That's not cool.

Add a simple open count that goes up and down with opens and closes (releases) of the device, and don't allow an rbd image to be removed if the count is non-zero. Both functions are protected by the bd_mutex so there's no need for any other concurrency protection.

Signed-off-by Alex Elder <[email protected]>
---
  drivers/block/rbd.c |    9 +++++++++
  1 file changed, 9 insertions(+)
drivers/block/rbd.c +1 -0
index fba0822..9d9a2f3 100644
@@ -255,6 +255,7 @@ struct rbd_device {
 
      /* sysfs related */
      struct device        dev;
+    unsigned long        open_count;
  };
 
  static DEFINE_MUTEX(ctl_mutex);      /* Serialize

open/close/setup/teardown */

drivers/block/rbd.c +1 -0
@@ -358,6 +359,7 @@ static int rbd_open(struct block_device *bdev,
 fmode_t mode)
 
      rbd_get_dev(rbd_dev);
      set_device_ro(bdev, rbd_dev->mapping.read_only);
+    rbd_dev->open_count++;
 
      return 0;

  }
drivers/block/rbd.c +2 -0
@@ -366,6 +368,8 @@ static int rbd_release(struct gendisk *disk, fmode_t
 mode)
  {
      struct rbd_device *rbd_dev = disk->private_data;
 
+    rbd_assert(rbd_dev->open_count > 0);
+    rbd_dev->open_count--;
      rbd_put_dev(rbd_dev);
 

      return 0;
drivers/block/rbd.c +0 -0
@@ -3764,6 +3768,11 @@ static ssize_t rbd_remove(struct bus_type *bus,
          goto done;
      }
Quoted 7 lines

drivers/block/rbd.c static ssize_t rbd_remove(struct bus_type *bus, · line 3775

Quoted 5 lines
drivers/block/rbd.c +5 -0
+    if (rbd_dev->open_count) {
+        ret = -EBUSY;
+        goto done;
+    }
+

Is this protected by bd_mutex?

No it's not, and that's a very good point.

I'll have to look at that over the weekend.
Thanks.

-Alex

Quoted 4 lines
Quoted 3 lines
drivers/block/rbd.c +0 -0
      rbd_remove_all_snaps(rbd_dev);
      rbd_bus_del_dev(rbd_dev);
author
#3 lore raw

PATCH v2 1/1 rbd: do not allow remove of mounted-on image

In reply to Alex Elder

There is no check in rbd_remove() to see if anybody holds open the image being removed. That's not cool.

Add a simple open count that goes up and down with opens and closes (releases) of the device, and don't allow an rbd image to be removed if the count is non-zero.

Protect the updates of the open count value with ctl_mutex to ensure the underlying rbd device doesn't get removed while concurrently being opened.

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

---
v2: added ctl_mutex locking for rbd_open() and rbd_release()

 drivers/block/rbd.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)
drivers/block/rbd.c +13 -0
Index: b/drivers/block/rbd.c
===================================================================
@@ -255,6 +255,7 @@ struct rbd_device {
 
 	/* sysfs related */
 	struct device		dev;
+	unsigned long		open_count;
 };
 
 static DEFINE_MUTEX(ctl_mutex);	  /* Serialize open/close/setup/teardown */

@@ -356,8 +357,11 @@ static int rbd_open(struct block_device
 	if ((mode & FMODE_WRITE) && rbd_dev->mapping.read_only)
 		return -EROFS;
 
+	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
 	rbd_get_dev(rbd_dev);
 	set_device_ro(bdev, rbd_dev->mapping.read_only);
+	rbd_dev->open_count++;
+	mutex_unlock(&ctl_mutex);
 
 	return 0;
 }

@@ -366,7 +370,11 @@ static int rbd_release(struct gendisk *d
 {
 	struct rbd_device *rbd_dev = disk->private_data;
 
+	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
+	rbd_assert(rbd_dev->open_count > 0);
+	rbd_dev->open_count--;
 	rbd_put_dev(rbd_dev);
+	mutex_unlock(&ctl_mutex);
 
 	return 0;
 }

@@ -3764,6 +3772,11 @@ static ssize_t rbd_remove(struct bus_typ
 		goto done;
 	}
 
+	if (rbd_dev->open_count) {
+		ret = -EBUSY;
+		goto done;
+	}
+
 	rbd_remove_all_snaps(rbd_dev);
 	rbd_bus_del_dev(rbd_dev);