[PATCH] dm: prevent removal if open

If you misuse the device-mapper interface (or there's a bug in your userspace tools) it's possible to end up with 'unlinked' mapped devices that cannot be removed until you reboot (along with uninterruptible processes). This patch prevents you from removing a device that is still open. It introduces dm_lock_for_deletion() which is called when a device is about to be removed to ensure that nothing has it open and nothing further can open it. It uses a private open_count for this which also lets us remove one of the problematic bdget_disk() calls elsewhere. Signed-off-by: Alasdair G Kergon <agk@redhat.com> Signed-off-by: Andrew Morton <akpm@osdl.org> Signed-off-by: Linus Torvalds <torvalds@osdl.org>

[PATCH] dm: prevent removal if open
If you misuse the device-mapper interface (or there's a bug in your userspace tools) it's possible to end up with 'unlinked' mapped devices that cannot be removed until you reboot (along with uninterruptible processes). This patch prevents you from removing a device that is still open. It introduces dm_lock_for_deletion() which is called when a device is about to be removed to ensure that nothing has it open and nothing further can open it. It uses a private open_count for this which also lets us remove one of the problematic bdget_disk() calls elsewhere. Signed-off-by: Alasdair G Kergon <agk@redhat.com> Signed-off-by: Andrew Morton <akpm@osdl.org> Signed-off-by: Linus Torvalds <torvalds@osdl.org>
Alasdair G Kergon · Linus Torvalds
1 parent c2ade42dd3
Showing 4 changed files with 81 additions and 24 deletions Side-by-side Diff
drivers/md/dm-ioctl.c
drivers/md/dm.c
drivers/md/dm.h
include/linux/dm-ioctl.h
@@ -48,7 +48,7 @@
 static struct list_head _name_buckets[NUM_BUCKETS];
 static struct list_head _uuid_buckets[NUM_BUCKETS];
  
-static void dm_hash_remove_all(void);
+static void dm_hash_remove_all(int keep_open_devices);
  
 /*
  * Guards access to both hash tables.
@@ -73,7 +73,7 @@
  
 static void dm_hash_exit(void)
 {
-	dm_hash_remove_all();
+	dm_hash_remove_all(0);
 	devfs_remove(DM_DIR);
 }
  
  
  
  
  
  
@@ -260,19 +260,41 @@
 	free_cell(hc);
 }
  
-static void dm_hash_remove_all(void)
+static void dm_hash_remove_all(int keep_open_devices)
 {
-	int i;
+	int i, dev_skipped, dev_removed;
 	struct hash_cell *hc;
 	struct list_head *tmp, *n;
  
 	down_write(&_hash_lock);
+
+retry:
+	dev_skipped = dev_removed = 0;
 	for (i = 0; i < NUM_BUCKETS; i++) {
 		list_for_each_safe (tmp, n, _name_buckets + i) {
 			hc = list_entry(tmp, struct hash_cell, name_list);
+
+			if (keep_open_devices &&
+			    dm_lock_for_deletion(hc->md)) {
+				dev_skipped++;
+				continue;
+			}
 			__hash_remove(hc);
+			dev_removed = 1;
 		}
 	}
+
+	/*
+	 * Some mapped devices may be using other mapped devices, so if any
+	 * still exist, repeat until we make no further progress.
+	 */
+	if (dev_skipped) {
+		if (dev_removed)
+			goto retry;
+
+		DMWARN("remove_all left %d open device(s)", dev_skipped);
+	}
+
 	up_write(&_hash_lock);
 }
  
@@ -355,7 +377,7 @@
  
 static int remove_all(struct dm_ioctl *param, size_t param_size)
 {
-	dm_hash_remove_all();
+	dm_hash_remove_all(1);
 	param->data_size = 0;
 	return 0;
 }
@@ -535,7 +557,6 @@
 {
 	struct gendisk *disk = dm_disk(md);
 	struct dm_table *table;
-	struct block_device *bdev;
  
 	param->flags &= ~(DM_SUSPEND_FLAG | DM_READONLY_FLAG |
 			  DM_ACTIVE_PRESENT_FLAG);
  
@@ -545,21 +566,13 @@
  
 	param->dev = huge_encode_dev(MKDEV(disk->major, disk->first_minor));
  
-	if (!(param->flags & DM_SKIP_BDGET_FLAG)) {
-		bdev = bdget_disk(disk, 0);
-		if (!bdev)
-			return -ENXIO;
+	/*
+	 * Yes, this will be out of date by the time it gets back
+	 * to userland, but it is still very useful for
+	 * debugging.
+	 */
+	param->open_count = dm_open_count(md);
  
-		/*
-		 * Yes, this will be out of date by the time it gets back
-		 * to userland, but it is still very useful for
-		 * debugging.
-		 */
-		param->open_count = bdev->bd_openers;
-		bdput(bdev);
-	} else
-		param->open_count = -1;
-
 	if (disk->policy)
 		param->flags |= DM_READONLY_FLAG;
  
@@ -661,6 +674,7 @@
 {
 	struct hash_cell *hc;
 	struct mapped_device *md;
+	int r;
  
 	down_write(&_hash_lock);
 	hc = __find_device_hash_cell(param);
@@ -672,6 +686,17 @@
 	}
  
 	md = hc->md;
+
+	/*
+	 * Ensure the device is not open and nothing further can open it.
+	 */
+	r = dm_lock_for_deletion(md);
+	if (r) {
+		DMWARN("unable to remove open device %s", hc->name);
+		up_write(&_hash_lock);
+		dm_put(md);
+		return r;
+	}
  
 	__hash_remove(hc);
 	up_write(&_hash_lock);
@@ -64,12 +64,14 @@
 #define DMF_SUSPENDED 1
 #define DMF_FROZEN 2
 #define DMF_FREEING 3
+#define DMF_DELETING 4
  
 struct mapped_device {
 	struct rw_semaphore io_lock;
 	struct semaphore suspend_lock;
 	rwlock_t map_lock;
 	atomic_t holders;
+	atomic_t open_count;
  
 	unsigned long flags;
  
  
@@ -228,12 +230,14 @@
 	if (!md)
 		goto out;
  
-	if (test_bit(DMF_FREEING, &md->flags)) {
+	if (test_bit(DMF_FREEING, &md->flags) ||
+	    test_bit(DMF_DELETING, &md->flags)) {
 		md = NULL;
 		goto out;
 	}
  
 	dm_get(md);
+	atomic_inc(&md->open_count);
  
 out:
 	spin_unlock(&_minor_lock);
  
@@ -246,10 +250,35 @@
 	struct mapped_device *md;
  
 	md = inode->i_bdev->bd_disk->private_data;
+	atomic_dec(&md->open_count);
 	dm_put(md);
 	return 0;
 }
  
+int dm_open_count(struct mapped_device *md)
+{
+	return atomic_read(&md->open_count);
+}
+
+/*
+ * Guarantees nothing is using the device before it's deleted.
+ */
+int dm_lock_for_deletion(struct mapped_device *md)
+{
+	int r = 0;
+
+	spin_lock(&_minor_lock);
+
+	if (dm_open_count(md))
+		r = -EBUSY;
+	else
+		set_bit(DMF_DELETING, &md->flags);
+
+	spin_unlock(&_minor_lock);
+
+	return r;
+}
+
 static int dm_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 {
 	struct mapped_device *md = bdev->bd_disk->private_data;
@@ -867,6 +896,7 @@
 	init_MUTEX(&md->suspend_lock);
 	rwlock_init(&md->map_lock);
 	atomic_set(&md->holders, 1);
+	atomic_set(&md->open_count, 0);
 	atomic_set(&md->event_nr, 0);
  
 	md->queue = blk_alloc_queue(GFP_KERNEL);
@@ -123,6 +123,8 @@
  
 void *dm_vcalloc(unsigned long nmemb, unsigned long elem_size);
 union map_info *dm_get_mapinfo(struct bio *bio);
+int dm_open_count(struct mapped_device *md);
+int dm_lock_for_deletion(struct mapped_device *md);
  
 #endif
@@ -285,9 +285,9 @@
 #define DM_DEV_SET_GEOMETRY	_IOWR(DM_IOCTL, DM_DEV_SET_GEOMETRY_CMD, struct dm_ioctl)
  
 #define DM_VERSION_MAJOR	4
-#define DM_VERSION_MINOR	6
+#define DM_VERSION_MINOR	7
 #define DM_VERSION_PATCHLEVEL	0
-#define DM_VERSION_EXTRA	"-ioctl (2006-02-17)"
+#define DM_VERSION_EXTRA	"-ioctl (2006-06-24)"
  
 /* Status bits */
 #define DM_READONLY_FLAG	(1 << 0) /* In/Out */
@@ -314,7 +314,7 @@
 #define DM_BUFFER_FULL_FLAG	(1 << 8) /* Out */
  
 /*
- * Set this to improve performance when you aren't going to use open_count.
+ * This flag is now ignored.
  */
 #define DM_SKIP_BDGET_FLAG	(1 << 9) /* In */
...	...	@@ -48,7 +48,7 @@
48	48	static struct list_head _name_buckets[NUM_BUCKETS];
49	49	static struct list_head _uuid_buckets[NUM_BUCKETS];
50	50
51		-static void dm_hash_remove_all(void);
	51	+static void dm_hash_remove_all(int keep_open_devices);
52	52
53	53	/*
54	54	* Guards access to both hash tables.
...	...	@@ -73,7 +73,7 @@
73	73
74	74	static void dm_hash_exit(void)
75	75	{
76		- dm_hash_remove_all();
	76	+ dm_hash_remove_all(0);
77	77	devfs_remove(DM_DIR);
78	78	}
79	79
80	80
81	81
82	82
83	83
84	84
...	...	@@ -260,19 +260,41 @@
260	260	free_cell(hc);
261	261	}
262	262
263		-static void dm_hash_remove_all(void)
	263	+static void dm_hash_remove_all(int keep_open_devices)
264	264	{
265		- int i;
	265	+ int i, dev_skipped, dev_removed;
266	266	struct hash_cell *hc;
267	267	struct list_head tmp, n;
268	268
269	269	down_write(&_hash_lock);
	270	+
	271	+retry:
	272	+ dev_skipped = dev_removed = 0;
270	273	for (i = 0; i < NUM_BUCKETS; i++) {
271	274	list_for_each_safe (tmp, n, _name_buckets + i) {
272	275	hc = list_entry(tmp, struct hash_cell, name_list);
	276	+
	277	+ if (keep_open_devices &&
	278	+ dm_lock_for_deletion(hc->md)) {
	279	+ dev_skipped++;
	280	+ continue;
	281	+ }
273	282	__hash_remove(hc);
	283	+ dev_removed = 1;
274	284	}
275	285	}
	286	+
	287	+ /*
	288	+ * Some mapped devices may be using other mapped devices, so if any
	289	+ * still exist, repeat until we make no further progress.
	290	+ */
	291	+ if (dev_skipped) {
	292	+ if (dev_removed)
	293	+ goto retry;
	294	+
	295	+ DMWARN("remove_all left %d open device(s)", dev_skipped);
	296	+ }
	297	+
276	298	up_write(&_hash_lock);
277	299	}
278	300
...	...	@@ -355,7 +377,7 @@
355	377
356	378	static int remove_all(struct dm_ioctl *param, size_t param_size)
357	379	{
358		- dm_hash_remove_all();
	380	+ dm_hash_remove_all(1);
359	381	param->data_size = 0;
360	382	return 0;
361	383	}
...	...	@@ -535,7 +557,6 @@
535	557	{
536	558	struct gendisk *disk = dm_disk(md);
537	559	struct dm_table *table;
538		- struct block_device *bdev;
539	560
540	561	param->flags &= ~(DM_SUSPEND_FLAG \| DM_READONLY_FLAG \|
541	562	DM_ACTIVE_PRESENT_FLAG);
542	563
...	...	@@ -545,21 +566,13 @@
545	566
546	567	param->dev = huge_encode_dev(MKDEV(disk->major, disk->first_minor));
547	568
548		- if (!(param->flags & DM_SKIP_BDGET_FLAG)) {
549		- bdev = bdget_disk(disk, 0);
550		- if (!bdev)
551		- return -ENXIO;
	569	+ /*
	570	+ * Yes, this will be out of date by the time it gets back
	571	+ * to userland, but it is still very useful for
	572	+ * debugging.
	573	+ */
	574	+ param->open_count = dm_open_count(md);
552	575
553		- /*
554		- * Yes, this will be out of date by the time it gets back
555		- * to userland, but it is still very useful for
556		- * debugging.
557		- */
558		- param->open_count = bdev->bd_openers;
559		- bdput(bdev);
560		- } else
561		- param->open_count = -1;
562		-
563	576	if (disk->policy)
564	577	param->flags \|= DM_READONLY_FLAG;
565	578
...	...	@@ -661,6 +674,7 @@
661	674	{
662	675	struct hash_cell *hc;
663	676	struct mapped_device *md;
	677	+ int r;
664	678
665	679	down_write(&_hash_lock);
666	680	hc = __find_device_hash_cell(param);
...	...	@@ -672,6 +686,17 @@
672	686	}
673	687
674	688	md = hc->md;
	689	+
	690	+ /*
	691	+ * Ensure the device is not open and nothing further can open it.
	692	+ */
	693	+ r = dm_lock_for_deletion(md);
	694	+ if (r) {
	695	+ DMWARN("unable to remove open device %s", hc->name);
	696	+ up_write(&_hash_lock);
	697	+ dm_put(md);
	698	+ return r;
	699	+ }
675	700
676	701	__hash_remove(hc);
677	702	up_write(&_hash_lock);
...	...	@@ -64,12 +64,14 @@
64	64	#define DMF_SUSPENDED 1
65	65	#define DMF_FROZEN 2
66	66	#define DMF_FREEING 3
	67	+#define DMF_DELETING 4
67	68
68	69	struct mapped_device {
69	70	struct rw_semaphore io_lock;
70	71	struct semaphore suspend_lock;
71	72	rwlock_t map_lock;
72	73	atomic_t holders;
	74	+ atomic_t open_count;
73	75
74	76	unsigned long flags;
75	77
76	78
...	...	@@ -228,12 +230,14 @@
228	230	if (!md)
229	231	goto out;
230	232
231		- if (test_bit(DMF_FREEING, &md->flags)) {
	233	+ if (test_bit(DMF_FREEING, &md->flags) \|\|
	234	+ test_bit(DMF_DELETING, &md->flags)) {
232	235	md = NULL;
233	236	goto out;
234	237	}
235	238
236	239	dm_get(md);
	240	+ atomic_inc(&md->open_count);
237	241
238	242	out:
239	243	spin_unlock(&_minor_lock);
240	244
...	...	@@ -246,10 +250,35 @@
246	250	struct mapped_device *md;
247	251
248	252	md = inode->i_bdev->bd_disk->private_data;
	253	+ atomic_dec(&md->open_count);
249	254	dm_put(md);
250	255	return 0;
251	256	}
252	257
	258	+int dm_open_count(struct mapped_device *md)
	259	+{
	260	+ return atomic_read(&md->open_count);
	261	+}
	262	+
	263	+/*
	264	+ * Guarantees nothing is using the device before it's deleted.
	265	+ */
	266	+int dm_lock_for_deletion(struct mapped_device *md)
	267	+{
	268	+ int r = 0;
	269	+
	270	+ spin_lock(&_minor_lock);
	271	+
	272	+ if (dm_open_count(md))
	273	+ r = -EBUSY;
	274	+ else
	275	+ set_bit(DMF_DELETING, &md->flags);
	276	+
	277	+ spin_unlock(&_minor_lock);
	278	+
	279	+ return r;
	280	+}
	281	+
253	282	static int dm_blk_getgeo(struct block_device bdev, struct hd_geometry geo)
254	283	{
255	284	struct mapped_device *md = bdev->bd_disk->private_data;
...	...	@@ -867,6 +896,7 @@
867	896	init_MUTEX(&md->suspend_lock);
868	897	rwlock_init(&md->map_lock);
869	898	atomic_set(&md->holders, 1);
	899	+ atomic_set(&md->open_count, 0);
870	900	atomic_set(&md->event_nr, 0);
871	901
872	902	md->queue = blk_alloc_queue(GFP_KERNEL);
...	...	@@ -123,6 +123,8 @@
123	123
124	124	void *dm_vcalloc(unsigned long nmemb, unsigned long elem_size);
125	125	union map_info dm_get_mapinfo(struct bio bio);
	126	+int dm_open_count(struct mapped_device *md);
	127	+int dm_lock_for_deletion(struct mapped_device *md);
126	128
127	129	#endif
...	...	@@ -285,9 +285,9 @@
285	285	#define DM_DEV_SET_GEOMETRY _IOWR(DM_IOCTL, DM_DEV_SET_GEOMETRY_CMD, struct dm_ioctl)
286	286
287	287	#define DM_VERSION_MAJOR 4
288		-#define DM_VERSION_MINOR 6
	288	+#define DM_VERSION_MINOR 7
289	289	#define DM_VERSION_PATCHLEVEL 0
290		-#define DM_VERSION_EXTRA "-ioctl (2006-02-17)"
	290	+#define DM_VERSION_EXTRA "-ioctl (2006-06-24)"
291	291
292	292	/* Status bits */
293	293	#define DM_READONLY_FLAG (1 << 0) /* In/Out */
...	...	@@ -314,7 +314,7 @@
314	314	#define DM_BUFFER_FULL_FLAG (1 << 8) /* Out */
315	315
316	316	/*
317		- * Set this to improve performance when you aren't going to use open_count.
	317	+ * This flag is now ignored.
318	318	*/
319	319	#define DM_SKIP_BDGET_FLAG (1 << 9) /* In */
320	320