Forwarded: [PATCH] dm: preserve outstanding device references across mode upgrades

[syzbot <syzbot+83827f9a59abe148bff4@syzkaller.appspotmail.com>]

For archival purposes, forwarding an incoming command email to
linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com.

***

Subject: [PATCH] dm: preserve outstanding device references across mode upgrades
Author: diasyzhangqing@gmail.com

dm_get_device() keeps one dm_dev_internal entry per dev_t. When a later
caller requests the same device with additional modes, upgrade_mode()
opens a new table device (table devices are keyed by (dev_t, mode)),
points dd->dm_dev at it and releases the old one.

The pointer returned by the first dm_get_device() call is still held by
the target, e.g. pool_ctr()'s metadata_dev or thin_ctr()'s
tc->origin_dev, so releasing the old table device frees memory that is
still in use and reading e.g. metadata_dev->bdev is a use-after-free.
dm_put_device() can then no longer match the stale pointer either, so
its reference leaks. The old device cannot be widened in place: its
bdev may be accessed concurrently and the dm_dev may be shared with
another table, so it has to stay valid until its holder releases it.

Instead of releasing the replaced dm_dev in upgrade_mode(), keep it on
a list on the dm_dev_internal entry. dm_put_device() matches both the
current and retained pointers, and all associated table devices are
released once the entry's reference count reaches zero, including when
free_devices() tears down a table whose devices were not all put. If
acquiring the upgraded device fails, leave the existing entry unchanged.

This affects any target that opens the same device twice with different
modes, such as the thin-pool metadata/data and thin pool/external-origin
arguments reported by syzbot.

Fixes: 86f1152b117a ("dm: allow active and inactive tables to share dm_devs")
Reported-by: syzbot+83827f9a59abe148bff4@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=83827f9a59abe148bff4
Signed-off-by: Qing Zhang <diasyzhang@tencent.com>
---
#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Fixing this in dm_get_device() itself, as you suggest, also covers the
same-device mode-upgrade instances in the other targets, so a single
change closes them all.  Verified with the syzbot reproducer: the
unpatched modules hit the pool_ctr use-after-free within a few
iterations, the patched modules run the same reproducer clean, with no
KASAN reports and no leaked table-device references.

 drivers/md/dm-table.c | 53 +++++++++++++++++++++++++++++++++++++------
 drivers/md/dm.h       |  1 +
 2 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index a483f49dd4ce..f64db28aa15c 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -163,6 +163,24 @@ int dm_table_create(struct dm_table **result, blk_mode_t mode,
 	return 0;
 }
 
+struct dm_dev_old {
+	struct list_head list;
+	struct dm_dev *dm_dev;
+};
+
+static void put_table_devices(struct dm_dev_internal *dd,
+			      struct mapped_device *md)
+{
+	struct dm_dev_old *old, *tmp;
+
+	list_for_each_entry_safe(old, tmp, &dd->old_devices, list) {
+		dm_put_table_device(md, old->dm_dev);
+		list_del(&old->list);
+		kfree(old);
+	}
+	dm_put_table_device(md, dd->dm_dev);
+}
+
 static void free_devices(struct list_head *devices, struct mapped_device *md)
 {
 	struct list_head *tmp, *next;
@@ -172,7 +190,7 @@ static void free_devices(struct list_head *devices, struct mapped_device *md)
 		    list_entry(tmp, struct dm_dev_internal, list);
 		DMWARN("%s: dm_table_destroy: dm_put_device call missing for %s",
 		       dm_device_name(md), dd->dm_dev->name);
-		dm_put_table_device(md, dd->dm_dev);
+		put_table_devices(dd, md);
 		kfree(dd);
 	}
 }
@@ -313,17 +331,23 @@ static int upgrade_mode(struct dm_dev_internal *dd, blk_mode_t new_mode,
 			struct mapped_device *md)
 {
 	int r;
-	struct dm_dev *old_dev, *new_dev;
+	struct dm_dev *new_dev;
+	struct dm_dev_old *old;
 
-	old_dev = dd->dm_dev;
+	old = kmalloc_obj(*old);
+	if (!old)
+		return -ENOMEM;
 
 	r = dm_get_table_device(md, dd->dm_dev->bdev->bd_dev,
 				dd->dm_dev->mode | new_mode, &new_dev);
-	if (r)
+	if (r) {
+		kfree(old);
 		return r;
+	}
 
+	old->dm_dev = dd->dm_dev;
+	list_add_tail(&old->list, &dd->old_devices);
 	dd->dm_dev = new_dev;
-	dm_put_table_device(md, old_dev);
 
 	return 0;
 }
@@ -392,6 +416,7 @@ int dm_get_device(struct dm_target *ti, const char *path, blk_mode_t mode,
 		}
 
 		refcount_set(&dd->count, 1);
+		INIT_LIST_HEAD(&dd->old_devices);
 		list_add(&dd->list, &t->devices);
 		goto out;
 
@@ -451,6 +476,20 @@ static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev,
 /*
  * Decrement a device's use count and remove it if necessary.
  */
+static bool device_matches(struct dm_dev_internal *dd, struct dm_dev *d)
+{
+	struct dm_dev_old *old;
+
+	if (dd->dm_dev == d)
+		return true;
+
+	list_for_each_entry(old, &dd->old_devices, list)
+		if (old->dm_dev == d)
+			return true;
+
+	return false;
+}
+
 void dm_put_device(struct dm_target *ti, struct dm_dev *d)
 {
 	int found = 0;
@@ -458,7 +497,7 @@ void dm_put_device(struct dm_target *ti, struct dm_dev *d)
 	struct dm_dev_internal *dd;
 
 	list_for_each_entry(dd, devices, list) {
-		if (dd->dm_dev == d) {
+		if (device_matches(dd, d)) {
 			found = 1;
 			break;
 		}
@@ -469,7 +508,7 @@ void dm_put_device(struct dm_target *ti, struct dm_dev *d)
 		return;
 	}
 	if (refcount_dec_and_test(&dd->count)) {
-		dm_put_table_device(ti->table->md, d);
+		put_table_devices(dd, ti->table->md);
 		list_del(&dd->list);
 		kfree(dd);
 	}
diff --git a/drivers/md/dm.h b/drivers/md/dm.h
index 7a795979ec72..037d6cdf2f1c 100644
--- a/drivers/md/dm.h
+++ b/drivers/md/dm.h
@@ -42,6 +42,7 @@ struct dm_dev_internal {
 	struct list_head list;
 	refcount_t count;
 	struct dm_dev *dm_dev;
+	struct list_head old_devices;
 };
 
 struct dm_table;
-- 
2.43.7