sort out the lock order in the loop driver

sort out the lock order in the loop driver

[Christoph Hellwig]

Hi Jens,

this series sorts out lock order reversals involving the block layer
open_mutex by drastically reducing the scope of loop_ctl_mutex.  To do
so it first merges the cryptoloop module into the main loop driver
as the unregistrtion of transfers on live loop devices was causing
some nasty locking interactions.

Diffstat:
 b/drivers/block/Kconfig    |    6 
 b/drivers/block/Makefile   |    1 
 b/drivers/block/loop.c     |  327 ++++++++++++++++++++++++---------------------
 b/drivers/block/loop.h     |   30 ----
 drivers/block/cryptoloop.c |  204 ----------------------------
 5 files changed, 188 insertions(+), 380 deletions(-)

[PATCH 1/7] loop: remove the ->ioctl method in loop_func_table

[Christoph Hellwig]

Never set to anything useful.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/cryptoloop.c | 7 -------
 drivers/block/loop.c       | 4 +---
 drivers/block/loop.h       | 3 ---
 3 files changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/block/cryptoloop.c b/drivers/block/cryptoloop.c
index 3cabc335ae74..8be35d37e379 100644
--- a/drivers/block/cryptoloop.c
+++ b/drivers/block/cryptoloop.c
@@ -154,12 +154,6 @@ cryptoloop_transfer(struct loop_device *lo, int cmd,
 	return err;
 }
 
-static int
-cryptoloop_ioctl(struct loop_device *lo, int cmd, unsigned long arg)
-{
-	return -EINVAL;
-}
-
 static int
 cryptoloop_release(struct loop_device *lo)
 {
@@ -176,7 +170,6 @@ cryptoloop_release(struct loop_device *lo)
 static struct loop_func_table cryptoloop_funcs = {
 	.number = LO_CRYPT_CRYPTOAPI,
 	.init = cryptoloop_init,
-	.ioctl = cryptoloop_ioctl,
 	.transfer = cryptoloop_transfer,
 	.release = cryptoloop_release,
 	.owner = THIS_MODULE
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index fa1c298a8cfb..b0dfea6d4371 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1169,7 +1169,6 @@ loop_set_status_from_info(struct loop_device *lo,
 	if (!xfer)
 		xfer = &none_funcs;
 	lo->transfer = xfer->transfer;
-	lo->ioctl = xfer->ioctl;
 
 	lo->lo_flags = info->lo_flags;
 
@@ -1383,7 +1382,6 @@ static int __loop_clr_fd(struct loop_device *lo, bool release)
 
 	loop_release_xfer(lo);
 	lo->transfer = NULL;
-	lo->ioctl = NULL;
 	lo->lo_device = NULL;
 	lo->lo_encryption = NULL;
 	lo->lo_offset = 0;
@@ -1809,7 +1807,7 @@ static int lo_simple_ioctl(struct loop_device *lo, unsigned int cmd,
 		err = loop_set_block_size(lo, arg);
 		break;
 	default:
-		err = lo->ioctl ? lo->ioctl(lo, cmd, arg) : -EINVAL;
+		err = -EINVAL;
 	}
 	mutex_unlock(&lo->lo_mutex);
 	return err;
diff --git a/drivers/block/loop.h b/drivers/block/loop.h
index 1988899db63a..dcde46afc675 100644
--- a/drivers/block/loop.h
+++ b/drivers/block/loop.h
@@ -43,8 +43,6 @@ struct loop_device {
 	struct loop_func_table *lo_encryption;
 	__u32           lo_init[2];
 	kuid_t		lo_key_owner;	/* Who set the key */
-	int		(*ioctl)(struct loop_device *, int cmd, 
-				 unsigned long arg); 
 
 	struct file *	lo_backing_file;
 	struct block_device *lo_device;
@@ -91,7 +89,6 @@ struct loop_func_table {
 	int (*init)(struct loop_device *, const struct loop_info64 *); 
 	/* release is called from loop_unregister_transfer or clr_fd */
 	int (*release)(struct loop_device *); 
-	int (*ioctl)(struct loop_device *, int cmd, unsigned long arg);
 	struct module *owner;
 }; 
 
-- 
2.30.2

[PATCH 2/7] loop: return void from the ->release method in loop_func_table

[Christoph Hellwig]

Returning an error here is no useful.  So remove the cargo culted check
in cryptoloop and remove the return value.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/cryptoloop.c | 12 +++---------
 drivers/block/loop.c       | 10 +++-------
 drivers/block/loop.h       |  2 +-
 3 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/block/cryptoloop.c b/drivers/block/cryptoloop.c
index 8be35d37e379..1d9e56860e56 100644
--- a/drivers/block/cryptoloop.c
+++ b/drivers/block/cryptoloop.c
@@ -154,17 +154,11 @@ cryptoloop_transfer(struct loop_device *lo, int cmd,
 	return err;
 }
 
-static int
+static void
 cryptoloop_release(struct loop_device *lo)
 {
-	struct crypto_sync_skcipher *tfm = lo->key_data;
-	if (tfm != NULL) {
-		crypto_free_sync_skcipher(tfm);
-		lo->key_data = NULL;
-		return 0;
-	}
-	printk(KERN_ERR "cryptoloop_release(): tfm == NULL?\n");
-	return -EINVAL;
+	crypto_free_sync_skcipher(lo->key_data);
+	lo->key_data = NULL;
 }
 
 static struct loop_func_table cryptoloop_funcs = {
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index b0dfea6d4371..d63e9f2c9e9b 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1084,20 +1084,18 @@ static void loop_update_rotational(struct loop_device *lo)
 		blk_queue_flag_clear(QUEUE_FLAG_NONROT, q);
 }
 
-static int
+static void
 loop_release_xfer(struct loop_device *lo)
 {
-	int err = 0;
 	struct loop_func_table *xfer = lo->lo_encryption;
 
 	if (xfer) {
 		if (xfer->release)
-			err = xfer->release(lo);
+			xfer->release(lo);
 		lo->transfer = NULL;
 		lo->lo_encryption = NULL;
 		module_put(xfer->owner);
 	}
-	return err;
 }
 
 static int
@@ -1140,9 +1138,7 @@ loop_set_status_from_info(struct loop_device *lo,
 	if ((unsigned int) info->lo_encrypt_key_size > LO_KEY_SIZE)
 		return -EINVAL;
 
-	err = loop_release_xfer(lo);
-	if (err)
-		return err;
+	loop_release_xfer(lo);
 
 	if (info->lo_encrypt_type) {
 		unsigned int type = info->lo_encrypt_type;
diff --git a/drivers/block/loop.h b/drivers/block/loop.h
index dcde46afc675..7b84ef724de1 100644
--- a/drivers/block/loop.h
+++ b/drivers/block/loop.h
@@ -88,7 +88,7 @@ struct loop_func_table {
 			int size, sector_t real_block);
 	int (*init)(struct loop_device *, const struct loop_info64 *); 
 	/* release is called from loop_unregister_transfer or clr_fd */
-	int (*release)(struct loop_device *); 
+	void (*release)(struct loop_device *); 
 	struct module *owner;
 }; 
 
-- 
2.30.2

[PATCH 3/7] loop: merge the cryptoloop module into the main loop module

[Christoph Hellwig]

No need to keep a separate loadable module infrastructure for a tiny
amount of cryptoapi glue, especially as unloading of the cryptoloop
module leads to nasty interactions with the loop device state machine
through loop_unregister_transfer.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/Kconfig      |   6 +-
 drivers/block/Makefile     |   1 -
 drivers/block/cryptoloop.c | 191 -----------------------------------
 drivers/block/loop.c       | 202 +++++++++++++++++++++++++++----------
 drivers/block/loop.h       |   5 -
 5 files changed, 153 insertions(+), 252 deletions(-)
 delete mode 100644 drivers/block/cryptoloop.c

diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
index 63056cfd4b62..c63386576ef6 100644
--- a/drivers/block/Kconfig
+++ b/drivers/block/Kconfig
@@ -156,6 +156,8 @@ config BLK_DEV_COW_COMMON
 
 config BLK_DEV_LOOP
 	tristate "Loopback device support"
+	select CRYPTO if CRYPTOLOOP
+	select CRYPTO_CBC if CRYPTOLOOP
 	help
 	  Saying Y here will allow you to use a regular file as a block
 	  device; you can then create a file system on that block device and
@@ -213,9 +215,7 @@ config BLK_DEV_LOOP_MIN_COUNT
 	  dynamically allocated with the /dev/loop-control interface.
 
 config BLK_DEV_CRYPTOLOOP
-	tristate "Cryptoloop Support"
-	select CRYPTO
-	select CRYPTO_CBC
+	bool "Cryptoloop Support"
 	depends on BLK_DEV_LOOP
 	help
 	  Say Y here if you want to be able to use the ciphers that are 
diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index bc68817ef496..11a74f17c9ad 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -24,7 +24,6 @@ obj-$(CONFIG_CDROM_PKTCDVD)	+= pktcdvd.o
 obj-$(CONFIG_SUNVDC)		+= sunvdc.o
 
 obj-$(CONFIG_BLK_DEV_NBD)	+= nbd.o
-obj-$(CONFIG_BLK_DEV_CRYPTOLOOP) += cryptoloop.o
 obj-$(CONFIG_VIRTIO_BLK)	+= virtio_blk.o
 
 obj-$(CONFIG_BLK_DEV_SX8)	+= sx8.o
diff --git a/drivers/block/cryptoloop.c b/drivers/block/cryptoloop.c
deleted file mode 100644
index 1d9e56860e56..000000000000
--- a/drivers/block/cryptoloop.c
+++ /dev/null
@@ -1,191 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
-   Linux loop encryption enabling module
-
-   Copyright (C)  2002 Herbert Valerio Riedel <hvr@gnu.org>
-   Copyright (C)  2003 Fruhwirth Clemens <clemens@endorphin.org>
-
- */
-
-#include <linux/module.h>
-
-#include <crypto/skcipher.h>
-#include <linux/init.h>
-#include <linux/string.h>
-#include <linux/blkdev.h>
-#include <linux/scatterlist.h>
-#include <linux/uaccess.h>
-#include "loop.h"
-
-MODULE_LICENSE("GPL");
-MODULE_DESCRIPTION("loop blockdevice transferfunction adaptor / CryptoAPI");
-MODULE_AUTHOR("Herbert Valerio Riedel <hvr@gnu.org>");
-
-#define LOOP_IV_SECTOR_BITS 9
-#define LOOP_IV_SECTOR_SIZE (1 << LOOP_IV_SECTOR_BITS)
-
-static int
-cryptoloop_init(struct loop_device *lo, const struct loop_info64 *info)
-{
-	int err = -EINVAL;
-	int cipher_len;
-	int mode_len;
-	char cms[LO_NAME_SIZE];			/* cipher-mode string */
-	char *mode;
-	char *cmsp = cms;			/* c-m string pointer */
-	struct crypto_sync_skcipher *tfm;
-
-	/* encryption breaks for non sector aligned offsets */
-
-	if (info->lo_offset % LOOP_IV_SECTOR_SIZE)
-		goto out;
-
-	strncpy(cms, info->lo_crypt_name, LO_NAME_SIZE);
-	cms[LO_NAME_SIZE - 1] = 0;
-
-	cipher_len = strcspn(cmsp, "-");
-
-	mode = cmsp + cipher_len;
-	mode_len = 0;
-	if (*mode) {
-		mode++;
-		mode_len = strcspn(mode, "-");
-	}
-
-	if (!mode_len) {
-		mode = "cbc";
-		mode_len = 3;
-	}
-
-	if (cipher_len + mode_len + 3 > LO_NAME_SIZE)
-		return -EINVAL;
-
-	memmove(cms, mode, mode_len);
-	cmsp = cms + mode_len;
-	*cmsp++ = '(';
-	memcpy(cmsp, info->lo_crypt_name, cipher_len);
-	cmsp += cipher_len;
-	*cmsp++ = ')';
-	*cmsp = 0;
-
-	tfm = crypto_alloc_sync_skcipher(cms, 0, 0);
-	if (IS_ERR(tfm))
-		return PTR_ERR(tfm);
-
-	err = crypto_sync_skcipher_setkey(tfm, info->lo_encrypt_key,
-					  info->lo_encrypt_key_size);
-
-	if (err != 0)
-		goto out_free_tfm;
-
-	lo->key_data = tfm;
-	return 0;
-
- out_free_tfm:
-	crypto_free_sync_skcipher(tfm);
-
- out:
-	return err;
-}
-
-
-typedef int (*encdec_cbc_t)(struct skcipher_request *req);
-
-static int
-cryptoloop_transfer(struct loop_device *lo, int cmd,
-		    struct page *raw_page, unsigned raw_off,
-		    struct page *loop_page, unsigned loop_off,
-		    int size, sector_t IV)
-{
-	struct crypto_sync_skcipher *tfm = lo->key_data;
-	SYNC_SKCIPHER_REQUEST_ON_STACK(req, tfm);
-	struct scatterlist sg_out;
-	struct scatterlist sg_in;
-
-	encdec_cbc_t encdecfunc;
-	struct page *in_page, *out_page;
-	unsigned in_offs, out_offs;
-	int err;
-
-	skcipher_request_set_sync_tfm(req, tfm);
-	skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP,
-				      NULL, NULL);
-
-	sg_init_table(&sg_out, 1);
-	sg_init_table(&sg_in, 1);
-
-	if (cmd == READ) {
-		in_page = raw_page;
-		in_offs = raw_off;
-		out_page = loop_page;
-		out_offs = loop_off;
-		encdecfunc = crypto_skcipher_decrypt;
-	} else {
-		in_page = loop_page;
-		in_offs = loop_off;
-		out_page = raw_page;
-		out_offs = raw_off;
-		encdecfunc = crypto_skcipher_encrypt;
-	}
-
-	while (size > 0) {
-		const int sz = min(size, LOOP_IV_SECTOR_SIZE);
-		u32 iv[4] = { 0, };
-		iv[0] = cpu_to_le32(IV & 0xffffffff);
-
-		sg_set_page(&sg_in, in_page, sz, in_offs);
-		sg_set_page(&sg_out, out_page, sz, out_offs);
-
-		skcipher_request_set_crypt(req, &sg_in, &sg_out, sz, iv);
-		err = encdecfunc(req);
-		if (err)
-			goto out;
-
-		IV++;
-		size -= sz;
-		in_offs += sz;
-		out_offs += sz;
-	}
-
-	err = 0;
-
-out:
-	skcipher_request_zero(req);
-	return err;
-}
-
-static void
-cryptoloop_release(struct loop_device *lo)
-{
-	crypto_free_sync_skcipher(lo->key_data);
-	lo->key_data = NULL;
-}
-
-static struct loop_func_table cryptoloop_funcs = {
-	.number = LO_CRYPT_CRYPTOAPI,
-	.init = cryptoloop_init,
-	.transfer = cryptoloop_transfer,
-	.release = cryptoloop_release,
-	.owner = THIS_MODULE
-};
-
-static int __init
-init_cryptoloop(void)
-{
-	int rc = loop_register_transfer(&cryptoloop_funcs);
-
-	if (rc)
-		printk(KERN_ERR "cryptoloop: loop_register_transfer failed\n");
-	return rc;
-}
-
-static void __exit
-cleanup_cryptoloop(void)
-{
-	if (loop_unregister_transfer(LO_CRYPT_CRYPTOAPI))
-		printk(KERN_ERR
-			"cryptoloop: loop_unregister_transfer failed\n");
-}
-
-module_init(init_cryptoloop);
-module_exit(cleanup_cryptoloop);
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index d63e9f2c9e9b..8b187987904e 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -39,6 +39,10 @@
  * Support up to 256 loop devices
  * Heinz Mauelshagen <mge@sistina.com>, Feb 2002
  *
+ * Cryptoloop support:
+ * Copyright (C)  2002 Herbert Valerio Riedel <hvr@gnu.org>
+ * Copyright (C)  2003 Fruhwirth Clemens <clemens@endorphin.org>
+ *
  * Support for falling back on the write file operation when the address space
  * operations write_begin is not available on the backing filesystem.
  * Anton Altaparmakov, 16 Feb 2005
@@ -79,13 +83,16 @@
 #include <linux/ioprio.h>
 #include <linux/blk-cgroup.h>
 #include <linux/sched/mm.h>
-
+#include <crypto/skcipher.h>
 #include "loop.h"
 
 #include <linux/uaccess.h>
 
 #define LOOP_IDLE_WORKER_TIMEOUT (60 * HZ)
 
+#define LOOP_IV_SECTOR_BITS 9
+#define LOOP_IV_SECTOR_SIZE (1 << LOOP_IV_SECTOR_BITS)
+
 static DEFINE_IDR(loop_index_idr);
 static DEFINE_MUTEX(loop_ctl_mutex);
 static DEFINE_MUTEX(loop_validate_mutex);
@@ -179,10 +186,146 @@ static struct loop_func_table xor_funcs = {
 	.init = xor_init
 }; 
 
+#ifdef CONFIG_BLK_DEV_CRYPTOLOOP
+static int cryptoloop_init(struct loop_device *lo,
+		const struct loop_info64 *info)
+{
+	int err = -EINVAL;
+	int cipher_len;
+	int mode_len;
+	char cms[LO_NAME_SIZE];			/* cipher-mode string */
+	char *mode;
+	char *cmsp = cms;			/* c-m string pointer */
+	struct crypto_sync_skcipher *tfm;
+
+	/* encryption breaks for non sector aligned offsets */
+	if (info->lo_offset % LOOP_IV_SECTOR_SIZE)
+		return -EINVAL;
+
+	strncpy(cms, info->lo_crypt_name, LO_NAME_SIZE);
+	cms[LO_NAME_SIZE - 1] = 0;
+
+	cipher_len = strcspn(cmsp, "-");
+	mode = cmsp + cipher_len;
+	mode_len = 0;
+	if (*mode) {
+		mode++;
+		mode_len = strcspn(mode, "-");
+	}
+	if (!mode_len) {
+		mode = "cbc";
+		mode_len = 3;
+	}
+	if (cipher_len + mode_len + 3 > LO_NAME_SIZE)
+		return -EINVAL;
+
+	memmove(cms, mode, mode_len);
+	cmsp = cms + mode_len;
+	*cmsp++ = '(';
+	memcpy(cmsp, info->lo_crypt_name, cipher_len);
+	cmsp += cipher_len;
+	*cmsp++ = ')';
+	*cmsp = 0;
+
+	tfm = crypto_alloc_sync_skcipher(cms, 0, 0);
+	if (IS_ERR(tfm))
+		return PTR_ERR(tfm);
+
+	err = crypto_sync_skcipher_setkey(tfm, info->lo_encrypt_key,
+					  info->lo_encrypt_key_size);
+	if (err != 0)
+		goto out_free_tfm;
+	lo->key_data = tfm;
+	return 0;
+
+ out_free_tfm:
+	crypto_free_sync_skcipher(tfm);
+	return err;
+}
+
+typedef int (*encdec_cbc_t)(struct skcipher_request *req);
+
+static int cryptoloop_transfer(struct loop_device *lo, int cmd,
+		struct page *raw_page, unsigned raw_off, struct page *loop_page,
+		unsigned loop_off, int size, sector_t IV)
+{
+	struct crypto_sync_skcipher *tfm = lo->key_data;
+	SYNC_SKCIPHER_REQUEST_ON_STACK(req, tfm);
+	struct scatterlist sg_out;
+	struct scatterlist sg_in;
+
+	encdec_cbc_t encdecfunc;
+	struct page *in_page, *out_page;
+	unsigned in_offs, out_offs;
+	int err;
+
+	skcipher_request_set_sync_tfm(req, tfm);
+	skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP,
+				      NULL, NULL);
+
+	sg_init_table(&sg_out, 1);
+	sg_init_table(&sg_in, 1);
+
+	if (cmd == READ) {
+		in_page = raw_page;
+		in_offs = raw_off;
+		out_page = loop_page;
+		out_offs = loop_off;
+		encdecfunc = crypto_skcipher_decrypt;
+	} else {
+		in_page = loop_page;
+		in_offs = loop_off;
+		out_page = raw_page;
+		out_offs = raw_off;
+		encdecfunc = crypto_skcipher_encrypt;
+	}
+
+	while (size > 0) {
+		const int sz = min(size, LOOP_IV_SECTOR_SIZE);
+		u32 iv[4] = { 0, };
+		iv[0] = cpu_to_le32(IV & 0xffffffff);
+
+		sg_set_page(&sg_in, in_page, sz, in_offs);
+		sg_set_page(&sg_out, out_page, sz, out_offs);
+
+		skcipher_request_set_crypt(req, &sg_in, &sg_out, sz, iv);
+		err = encdecfunc(req);
+		if (err)
+			goto out;
+
+		IV++;
+		size -= sz;
+		in_offs += sz;
+		out_offs += sz;
+	}
+
+	err = 0;
+out:
+	skcipher_request_zero(req);
+	return err;
+}
+
+static void cryptoloop_release(struct loop_device *lo)
+{
+	crypto_free_sync_skcipher(lo->key_data);
+	lo->key_data = NULL;
+}
+
+static struct loop_func_table cryptoloop_funcs = {
+	.number		= LO_CRYPT_CRYPTOAPI,
+	.init		= cryptoloop_init,
+	.transfer	= cryptoloop_transfer,
+	.release	= cryptoloop_release,
+};
+#endif /* CONFIG_BLK_DEV_CRYPTOLOOP */
+
 /* xfer_funcs[0] is special - its release function is never called */
 static struct loop_func_table *xfer_funcs[MAX_LO_CRYPT] = {
-	&none_funcs,
-	&xor_funcs
+	[LO_CRYPT_NONE]		= &none_funcs,
+	[LO_CRYPT_XOR]		= &xor_funcs,
+#ifdef CONFIG_BLK_DEV_CRYPTOLOOP
+	[LO_CRYPT_CRYPTOAPI]	= &cryptoloop_funcs,
+#endif
 };
 
 static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)
@@ -1094,7 +1237,6 @@ loop_release_xfer(struct loop_device *lo)
 			xfer->release(lo);
 		lo->transfer = NULL;
 		lo->lo_encryption = NULL;
-		module_put(xfer->owner);
 	}
 }
 
@@ -1104,16 +1246,9 @@ loop_init_xfer(struct loop_device *lo, struct loop_func_table *xfer,
 {
 	int err = 0;
 
-	if (xfer) {
-		struct module *owner = xfer->owner;
-
-		if (!try_module_get(owner))
-			return -EINVAL;
-		if (xfer->init)
-			err = xfer->init(lo, i);
-		if (err)
-			module_put(owner);
-		else
+	if (xfer && xfer->init) {
+		err = xfer->init(lo, i);
+		if (!err)
 			lo->lo_encryption = xfer;
 	}
 	return err;
@@ -2094,44 +2229,7 @@ module_param(max_part, int, 0444);
 MODULE_PARM_DESC(max_part, "Maximum number of partitions per loop device");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR);
-
-int loop_register_transfer(struct loop_func_table *funcs)
-{
-	unsigned int n = funcs->number;
-
-	if (n >= MAX_LO_CRYPT || xfer_funcs[n])
-		return -EINVAL;
-	xfer_funcs[n] = funcs;
-	return 0;
-}
-
-static int unregister_transfer_cb(int id, void *ptr, void *data)
-{
-	struct loop_device *lo = ptr;
-	struct loop_func_table *xfer = data;
-
-	mutex_lock(&lo->lo_mutex);
-	if (lo->lo_encryption == xfer)
-		loop_release_xfer(lo);
-	mutex_unlock(&lo->lo_mutex);
-	return 0;
-}
-
-int loop_unregister_transfer(int number)
-{
-	unsigned int n = number;
-	struct loop_func_table *xfer;
-
-	if (n == 0 || n >= MAX_LO_CRYPT || (xfer = xfer_funcs[n]) == NULL)
-		return -EINVAL;
-
-	xfer_funcs[n] = NULL;
-	idr_for_each(&loop_index_idr, &unregister_transfer_cb, xfer);
-	return 0;
-}
-
-EXPORT_SYMBOL(loop_register_transfer);
-EXPORT_SYMBOL(loop_unregister_transfer);
+MODULE_ALIAS("cryptoloop");
 
 static blk_status_t loop_queue_rq(struct blk_mq_hw_ctx *hctx,
 		const struct blk_mq_queue_data *bd)
diff --git a/drivers/block/loop.h b/drivers/block/loop.h
index 7b84ef724de1..dd12d7f1ce30 100644
--- a/drivers/block/loop.h
+++ b/drivers/block/loop.h
@@ -87,12 +87,7 @@ struct loop_func_table {
 			struct page *loop_page, unsigned loop_off,
 			int size, sector_t real_block);
 	int (*init)(struct loop_device *, const struct loop_info64 *); 
-	/* release is called from loop_unregister_transfer or clr_fd */
 	void (*release)(struct loop_device *); 
-	struct module *owner;
 }; 
 
-int loop_register_transfer(struct loop_func_table *funcs);
-int loop_unregister_transfer(int number); 
-
 #endif
-- 
2.30.2

[PATCH 4/7] loop: devirtualize transfer transformations

[Christoph Hellwig]

Besides the already special cased non-transform fastpath there are only
two different transfers.  Hardcode them instead of the maze of indirect
calls and switch the whole cryptoloop code to use IS_ENABLED for dead
code elimination.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/loop.c | 146 +++++++++++--------------------------------
 drivers/block/loop.h |  21 ++-----
 2 files changed, 41 insertions(+), 126 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 8b187987904e..9176784d4fca 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -140,7 +140,7 @@ static void loop_global_unlock(struct loop_device *lo, bool global)
 static int max_part;
 static int part_shift;
 
-static int transfer_xor(struct loop_device *lo, int cmd,
+static void xor_transfer(struct loop_device *lo, int cmd,
 			struct page *raw_page, unsigned raw_off,
 			struct page *loop_page, unsigned loop_off,
 			int size, sector_t real_block)
@@ -166,27 +166,14 @@ static int transfer_xor(struct loop_device *lo, int cmd,
 	kunmap_atomic(loop_buf);
 	kunmap_atomic(raw_buf);
 	cond_resched();
-	return 0;
 }
 
-static int xor_init(struct loop_device *lo, const struct loop_info64 *info)
+static inline bool is_cryptoloop(int encrypt_type)
 {
-	if (unlikely(info->lo_encrypt_key_size <= 0))
-		return -EINVAL;
-	return 0;
+	return IS_ENABLED(CONFIG_BLK_DEV_CRYPTOLOOP) &&
+		encrypt_type == LO_CRYPT_CRYPTOAPI;
 }
 
-static struct loop_func_table none_funcs = {
-	.number = LO_CRYPT_NONE,
-}; 
-
-static struct loop_func_table xor_funcs = {
-	.number = LO_CRYPT_XOR,
-	.transfer = transfer_xor,
-	.init = xor_init
-}; 
-
-#ifdef CONFIG_BLK_DEV_CRYPTOLOOP
 static int cryptoloop_init(struct loop_device *lo,
 		const struct loop_info64 *info)
 {
@@ -235,7 +222,7 @@ static int cryptoloop_init(struct loop_device *lo,
 					  info->lo_encrypt_key_size);
 	if (err != 0)
 		goto out_free_tfm;
-	lo->key_data = tfm;
+	lo->lo_encrypt_tfm = tfm;
 	return 0;
 
  out_free_tfm:
@@ -249,7 +236,7 @@ static int cryptoloop_transfer(struct loop_device *lo, int cmd,
 		struct page *raw_page, unsigned raw_off, struct page *loop_page,
 		unsigned loop_off, int size, sector_t IV)
 {
-	struct crypto_sync_skcipher *tfm = lo->key_data;
+	struct crypto_sync_skcipher *tfm = lo->lo_encrypt_tfm;
 	SYNC_SKCIPHER_REQUEST_ON_STACK(req, tfm);
 	struct scatterlist sg_out;
 	struct scatterlist sg_in;
@@ -302,32 +289,11 @@ static int cryptoloop_transfer(struct loop_device *lo, int cmd,
 	err = 0;
 out:
 	skcipher_request_zero(req);
+	pr_err_ratelimited("loop: Transfer error at byte offset %llu, length %i.\n",
+			IV << 9, size);
 	return err;
 }
 
-static void cryptoloop_release(struct loop_device *lo)
-{
-	crypto_free_sync_skcipher(lo->key_data);
-	lo->key_data = NULL;
-}
-
-static struct loop_func_table cryptoloop_funcs = {
-	.number		= LO_CRYPT_CRYPTOAPI,
-	.init		= cryptoloop_init,
-	.transfer	= cryptoloop_transfer,
-	.release	= cryptoloop_release,
-};
-#endif /* CONFIG_BLK_DEV_CRYPTOLOOP */
-
-/* xfer_funcs[0] is special - its release function is never called */
-static struct loop_func_table *xfer_funcs[MAX_LO_CRYPT] = {
-	[LO_CRYPT_NONE]		= &none_funcs,
-	[LO_CRYPT_XOR]		= &xor_funcs,
-#ifdef CONFIG_BLK_DEV_CRYPTOLOOP
-	[LO_CRYPT_CRYPTOAPI]	= &cryptoloop_funcs,
-#endif
-};
-
 static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)
 {
 	loff_t loopsize;
@@ -382,7 +348,7 @@ static void __loop_update_dio(struct loop_device *lo, bool dio)
 		if (queue_logical_block_size(lo->lo_queue) >= sb_bsize &&
 				!(lo->lo_offset & dio_align) &&
 				mapping->a_ops->direct_IO &&
-				!lo->transfer)
+				!lo->lo_encrypt_type)
 			use_dio = true;
 		else
 			use_dio = false;
@@ -448,16 +414,11 @@ lo_do_transfer(struct loop_device *lo, int cmd,
 	       struct page *lpage, unsigned loffs,
 	       int size, sector_t rblock)
 {
-	int ret;
-
-	ret = lo->transfer(lo, cmd, rpage, roffs, lpage, loffs, size, rblock);
-	if (likely(!ret))
-		return 0;
-
-	printk_ratelimited(KERN_ERR
-		"loop: Transfer error at byte offset %llu, length %i.\n",
-		(unsigned long long)rblock << 9, size);
-	return ret;
+	if (is_cryptoloop(lo->lo_encrypt_type))
+		return cryptoloop_transfer(lo, cmd, rpage, roffs, lpage, loffs,
+					   size, rblock);
+	xor_transfer(lo, cmd, rpage, roffs, lpage, loffs, size, rblock);
+	return 0;
 }
 
 static int lo_write_bvec(struct file *file, struct bio_vec *bvec, loff_t *ppos)
@@ -803,14 +764,14 @@ static int do_req_filebacked(struct loop_device *lo, struct request *rq)
 	case REQ_OP_DISCARD:
 		return lo_fallocate(lo, rq, pos, FALLOC_FL_PUNCH_HOLE);
 	case REQ_OP_WRITE:
-		if (lo->transfer)
+		if (lo->lo_encrypt_type)
 			return lo_write_transfer(lo, rq, pos);
 		else if (cmd->use_aio)
 			return lo_rw_aio(lo, cmd, pos, WRITE);
 		else
 			return lo_write_simple(lo, rq, pos);
 	case REQ_OP_READ:
-		if (lo->transfer)
+		if (lo->lo_encrypt_type)
 			return lo_read_transfer(lo, rq, pos);
 		else if (cmd->use_aio)
 			return lo_rw_aio(lo, cmd, pos, READ);
@@ -1227,33 +1188,6 @@ static void loop_update_rotational(struct loop_device *lo)
 		blk_queue_flag_clear(QUEUE_FLAG_NONROT, q);
 }
 
-static void
-loop_release_xfer(struct loop_device *lo)
-{
-	struct loop_func_table *xfer = lo->lo_encryption;
-
-	if (xfer) {
-		if (xfer->release)
-			xfer->release(lo);
-		lo->transfer = NULL;
-		lo->lo_encryption = NULL;
-	}
-}
-
-static int
-loop_init_xfer(struct loop_device *lo, struct loop_func_table *xfer,
-	       const struct loop_info64 *i)
-{
-	int err = 0;
-
-	if (xfer && xfer->init) {
-		err = xfer->init(lo, i);
-		if (!err)
-			lo->lo_encryption = xfer;
-	}
-	return err;
-}
-
 /**
  * loop_set_status_from_info - configure device from loop_info
  * @lo: struct loop_device to configure
@@ -1267,29 +1201,28 @@ loop_set_status_from_info(struct loop_device *lo,
 			  const struct loop_info64 *info)
 {
 	int err;
-	struct loop_func_table *xfer;
 	kuid_t uid = current_uid();
 
 	if ((unsigned int) info->lo_encrypt_key_size > LO_KEY_SIZE)
 		return -EINVAL;
 
-	loop_release_xfer(lo);
-
-	if (info->lo_encrypt_type) {
-		unsigned int type = info->lo_encrypt_type;
+	if (is_cryptoloop(lo->lo_encrypt_type))
+		crypto_free_sync_skcipher(lo->lo_encrypt_tfm);
+	lo->lo_encrypt_tfm = NULL;
 
-		if (type >= MAX_LO_CRYPT)
+	if (info->lo_encrypt_type == LO_CRYPT_XOR) {
+		if (info->lo_encrypt_key_size <= 0)
 			return -EINVAL;
-		xfer = xfer_funcs[type];
-		if (xfer == NULL)
+	} else if (is_cryptoloop(info->lo_encrypt_type)) {
+		err = cryptoloop_init(lo, info);
+		if (err)
+			return err;
+	} else {
+		if (info->lo_encrypt_type != LO_CRYPT_NONE)
 			return -EINVAL;
-	} else
-		xfer = NULL;
-
-	err = loop_init_xfer(lo, xfer, info);
-	if (err)
-		return err;
+	}
 
+	lo->lo_encrypt_type = info->lo_encrypt_type;
 	lo->lo_offset = info->lo_offset;
 	lo->lo_sizelimit = info->lo_sizelimit;
 	memcpy(lo->lo_file_name, info->lo_file_name, LO_NAME_SIZE);
@@ -1297,10 +1230,6 @@ loop_set_status_from_info(struct loop_device *lo,
 	lo->lo_file_name[LO_NAME_SIZE-1] = 0;
 	lo->lo_crypt_name[LO_NAME_SIZE-1] = 0;
 
-	if (!xfer)
-		xfer = &none_funcs;
-	lo->transfer = xfer->transfer;
-
 	lo->lo_flags = info->lo_flags;
 
 	lo->lo_encrypt_key_size = info->lo_encrypt_key_size;
@@ -1511,10 +1440,10 @@ static int __loop_clr_fd(struct loop_device *lo, bool release)
 	lo->lo_backing_file = NULL;
 	spin_unlock_irq(&lo->lo_lock);
 
-	loop_release_xfer(lo);
-	lo->transfer = NULL;
+	if (is_cryptoloop(lo->lo_encrypt_type))
+		crypto_free_sync_skcipher(lo->lo_encrypt_tfm);
+	lo->lo_encrypt_tfm = NULL;
 	lo->lo_device = NULL;
-	lo->lo_encryption = NULL;
 	lo->lo_offset = 0;
 	lo->lo_sizelimit = 0;
 	lo->lo_encrypt_key_size = 0;
@@ -1727,8 +1656,7 @@ loop_get_status(struct loop_device *lo, struct loop_info64 *info)
 	info->lo_flags = lo->lo_flags;
 	memcpy(info->lo_file_name, lo->lo_file_name, LO_NAME_SIZE);
 	memcpy(info->lo_crypt_name, lo->lo_crypt_name, LO_NAME_SIZE);
-	info->lo_encrypt_type =
-		lo->lo_encryption ? lo->lo_encryption->number : 0;
+	info->lo_encrypt_type = lo->lo_encrypt_type;
 	if (lo->lo_encrypt_key_size && capable(CAP_SYS_ADMIN)) {
 		info->lo_encrypt_key_size = lo->lo_encrypt_key_size;
 		memcpy(info->lo_encrypt_key, lo->lo_encrypt_key,
@@ -1764,7 +1692,7 @@ loop_info64_from_old(const struct loop_info *info, struct loop_info64 *info64)
 	info64->lo_flags = info->lo_flags;
 	info64->lo_init[0] = info->lo_init[0];
 	info64->lo_init[1] = info->lo_init[1];
-	if (info->lo_encrypt_type == LO_CRYPT_CRYPTOAPI)
+	if (is_cryptoloop(info->lo_encrypt_type))
 		memcpy(info64->lo_crypt_name, info->lo_name, LO_NAME_SIZE);
 	else
 		memcpy(info64->lo_file_name, info->lo_name, LO_NAME_SIZE);
@@ -1785,7 +1713,7 @@ loop_info64_to_old(const struct loop_info64 *info64, struct loop_info *info)
 	info->lo_flags = info64->lo_flags;
 	info->lo_init[0] = info64->lo_init[0];
 	info->lo_init[1] = info64->lo_init[1];
-	if (info->lo_encrypt_type == LO_CRYPT_CRYPTOAPI)
+	if (is_cryptoloop(info->lo_encrypt_type))
 		memcpy(info->lo_name, info64->lo_crypt_name, LO_NAME_SIZE);
 	else
 		memcpy(info->lo_name, info64->lo_file_name, LO_NAME_SIZE);
@@ -2048,7 +1976,7 @@ loop_info64_from_compat(const struct compat_loop_info __user *arg,
 	info64->lo_flags = info.lo_flags;
 	info64->lo_init[0] = info.lo_init[0];
 	info64->lo_init[1] = info.lo_init[1];
-	if (info.lo_encrypt_type == LO_CRYPT_CRYPTOAPI)
+	if (is_cryptoloop(info.lo_encrypt_type))
 		memcpy(info64->lo_crypt_name, info.lo_name, LO_NAME_SIZE);
 	else
 		memcpy(info64->lo_file_name, info.lo_name, LO_NAME_SIZE);
@@ -2077,7 +2005,7 @@ loop_info64_to_compat(const struct loop_info64 *info64,
 	info.lo_flags = info64->lo_flags;
 	info.lo_init[0] = info64->lo_init[0];
 	info.lo_init[1] = info64->lo_init[1];
-	if (info.lo_encrypt_type == LO_CRYPT_CRYPTOAPI)
+	if (is_cryptoloop(info.lo_encrypt_type))
 		memcpy(info.lo_name, info64->lo_crypt_name, LO_NAME_SIZE);
 	else
 		memcpy(info.lo_name, info64->lo_file_name, LO_NAME_SIZE);
diff --git a/drivers/block/loop.h b/drivers/block/loop.h
index dd12d7f1ce30..d14ce6bdc014 100644
--- a/drivers/block/loop.h
+++ b/drivers/block/loop.h
@@ -16,6 +16,8 @@
 #include <linux/mutex.h>
 #include <uapi/linux/loop.h>
 
+struct crypto_sync_skcipher;
+
 /* Possible states of device */
 enum {
 	Lo_unbound,
@@ -32,21 +34,17 @@ struct loop_device {
 	loff_t		lo_offset;
 	loff_t		lo_sizelimit;
 	int		lo_flags;
-	int		(*transfer)(struct loop_device *, int cmd,
-				    struct page *raw_page, unsigned raw_off,
-				    struct page *loop_page, unsigned loop_off,
-				    int size, sector_t real_block);
 	char		lo_file_name[LO_NAME_SIZE];
 	char		lo_crypt_name[LO_NAME_SIZE];
 	char		lo_encrypt_key[LO_KEY_SIZE];
 	int		lo_encrypt_key_size;
-	struct loop_func_table *lo_encryption;
+	int		lo_encrypt_type;
+	struct crypto_sync_skcipher *lo_encrypt_tfm; 
 	__u32           lo_init[2];
 	kuid_t		lo_key_owner;	/* Who set the key */
 
 	struct file *	lo_backing_file;
 	struct block_device *lo_device;
-	void		*key_data; 
 
 	gfp_t		old_gfp_mask;
 
@@ -79,15 +77,4 @@ struct loop_cmd {
 	struct cgroup_subsys_state *memcg_css;
 };
 
-/* Support for loadable transfer modules */
-struct loop_func_table {
-	int number;	/* filter type */ 
-	int (*transfer)(struct loop_device *lo, int cmd,
-			struct page *raw_page, unsigned raw_off,
-			struct page *loop_page, unsigned loop_off,
-			int size, sector_t real_block);
-	int (*init)(struct loop_device *, const struct loop_info64 *); 
-	void (*release)(struct loop_device *); 
-}; 
-
 #endif
-- 
2.30.2

[PATCH 5/7] loop: remove the unused idx argument to loop_control_get_free

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/loop.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 9176784d4fca..e93baff664c9 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2486,7 +2486,7 @@ static int loop_control_remove(int idx)
 	return ret;
 }
 
-static int loop_control_get_free(int idx)
+static int loop_control_get_free(void)
 {
 	struct loop_device *lo;
 	int id, ret;
@@ -2514,7 +2514,7 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd,
 	case LOOP_CTL_REMOVE:
 		return loop_control_remove(parm);
 	case LOOP_CTL_GET_FREE:
-		return loop_control_get_free(parm);
+		return loop_control_get_free();
 	default:
 		return -ENOSYS;
 	}
-- 
2.30.2

[PATCH 6/7] loop: move loop device deletion out of loop_ctl_mutex

[Christoph Hellwig]

To avoid complex lock ordering issues always delete loop devices outside
of loop_ctl_mutex.  In loop_control_remove the Lo_deleting state can
be used to prevent further lookups, and given that module unload is
synchronized vs new opens of the control device and thus ioctls there
is no need for locks there at all.

Based on patches from Hillf Danton <hdanton@sina.com> and
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>.

Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/loop.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index e93baff664c9..043673bda8b3 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2480,7 +2480,11 @@ static int loop_control_remove(int idx)
 	mutex_unlock(&lo->lo_mutex);
 
 	idr_remove(&loop_index_idr, lo->lo_number);
+	mutex_unlock(&loop_ctl_mutex);
+
 	loop_remove(lo);
+	return 0;
+
 out_unlock_ctrl:
 	mutex_unlock(&loop_ctl_mutex);
 	return ret;
@@ -2611,11 +2615,12 @@ static void __exit loop_exit(void)
 	unregister_blkdev(LOOP_MAJOR, "loop");
 	misc_deregister(&loop_misc);
 
-	mutex_lock(&loop_ctl_mutex);
+	/*
+	 * No need for loop_ctl_mutex given that no new ioctls and thus
+	 * additions and removals can happen in parallel to module unloading.
+	 */
 	idr_for_each_entry(&loop_index_idr, lo, id)
 		loop_remove(lo);
-	mutex_unlock(&loop_ctl_mutex);
-
 	idr_destroy(&loop_index_idr);
 }
 
-- 
2.30.2

[PATCH 7/7] loop: avoid holding loop_ctl_mutex over add_disk

[Christoph Hellwig]

To avoid complex lock ordering issues loop_ctl_mutex should not
be held over add_disk.  Add a new Lo_new state for a loop device
that has just been created but which is not live yet.

Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/loop.c | 16 ++++++++++++----
 drivers/block/loop.h |  1 +
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 043673bda8b3..80f462edc39f 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2329,7 +2329,7 @@ static int loop_add(int i)
 	lo = kzalloc(sizeof(*lo), GFP_KERNEL);
 	if (!lo)
 		goto out;
-	lo->lo_state = Lo_unbound;
+	lo->lo_state = Lo_new;
 
 	err = mutex_lock_killable(&loop_ctl_mutex);
 	if (err)
@@ -2343,8 +2343,9 @@ static int loop_add(int i)
 	} else {
 		err = idr_alloc(&loop_index_idr, lo, 0, 0, GFP_KERNEL);
 	}
+	mutex_unlock(&loop_ctl_mutex);
 	if (err < 0)
-		goto out_unlock;
+		goto out_free_dev;
 	i = err;
 
 	err = -ENOMEM;
@@ -2414,15 +2415,22 @@ static int loop_add(int i)
 	disk->event_flags	= DISK_EVENT_FLAG_UEVENT;
 	sprintf(disk->disk_name, "loop%d", i);
 	add_disk(disk);
+
+	err = mutex_lock_killable(&loop_ctl_mutex);
+	if (err)
+		goto out_del_gendisk;
+	lo->lo_state = Lo_unbound;
 	mutex_unlock(&loop_ctl_mutex);
+
 	return i;
 
+out_del_gendisk:
+	del_gendisk(lo->lo_disk);
+	blk_cleanup_disk(lo->lo_disk);
 out_cleanup_tags:
 	blk_mq_free_tag_set(&lo->tag_set);
 out_free_idr:
 	idr_remove(&loop_index_idr, i);
-out_unlock:
-	mutex_unlock(&loop_ctl_mutex);
 out_free_dev:
 	kfree(lo);
 out:
diff --git a/drivers/block/loop.h b/drivers/block/loop.h
index d14ce6bdc014..608a20c23c64 100644
--- a/drivers/block/loop.h
+++ b/drivers/block/loop.h
@@ -24,6 +24,7 @@ enum {
 	Lo_bound,
 	Lo_rundown,
 	Lo_deleting,
+	Lo_new,
 };
 
 struct loop_func_table;
-- 
2.30.2

Re: sort out the lock order in the loop driver

[Tetsuo Handa]

On 2021/08/18 15:24, Christoph Hellwig wrote:
> Hi Jens,
> 
> this series sorts out lock order reversals involving the block layer
> open_mutex by drastically reducing the scope of loop_ctl_mutex.  To do
> so it first merges the cryptoloop module into the main loop driver
> as the unregistrtion of transfers on live loop devices was causing
> some nasty locking interactions.
> 
> Diffstat:
>  b/drivers/block/Kconfig    |    6 
>  b/drivers/block/Makefile   |    1 
>  b/drivers/block/loop.c     |  327 ++++++++++++++++++++++++---------------------
>  b/drivers/block/loop.h     |   30 ----
>  drivers/block/cryptoloop.c |  204 ----------------------------
>  5 files changed, 188 insertions(+), 380 deletions(-)
> 

Despite big change, this series introduces at least three bugs by ignoring
what lock protects what resource and/or operation. Locking explanation is very
very wrong. I never want this series merged for v5.14/v5.15 and stable kernels. 

  (1) Will crash at uninitialized mutex_lock_killable().
  (2) Will crash at unprotected idr_remove().
  (3) Will crash syzbot due to hitting WARNING messages (at least two different causes).