Add a new LOOP_SET_FD_AND_STATUS ioctl.

PATCH Superseded Martijn Coenen 3 participants 17 messages Apr 20, 2020 – Apr 27, 2020 Block layer

Share & tools

Martijn Coenen

author

2020-04-20 08:04 UTC #0 lore raw

cover Add a new LOOP_SET_FD_AND_STATUS ioctl.

This series introduces a new ioctl that makes it possible to atomically configure a loop device. Previously, if you wanted to set parameters such as the offset on a loop device, this required calling LOOP_SET_FD to set the backing file, and then LOOP_SET_STATUS to set the offset. However, in between these two calls, the loop device is available and would accept requests, which is generally not desirable.

There are also performance benefits with combining these two ioctls into one, which is described in more detail in the last change in the series.

Note that this series depends on
"loop: Call loop_config_discard() only after new config is applied." [0], which I sent as a separate patch as it fixes an unrelated bug.

[0]: https://lkml.org/lkml/2020/3/31/755

Martijn Coenen (4):
loop: Refactor size calculation.
loop: Factor out configuring loop from status.
loop: Move loop_set_from_status() and friends up.
loop: Add LOOP_SET_FD_AND_STATUS ioctl.

 drivers/block/loop.c      | 309 ++++++++++++++++++++++----------------
 include/uapi/linux/loop.h |   6 +
 2 files changed, 189 insertions(+), 126 deletions(-)

Signature · 1 line

2.26.1.301.g55bc3eb7cb9-goog

Martijn Coenen

author

2020-04-20 08:04 UTC #5 lore raw

PATCH 2/4 loop: Factor out configuring loop from status.

Factor out this code into a separate function, so it can be reused by other code more easily.

Signed-off-by Martijn Coenen <[email protected]>

---
 drivers/block/loop.c | 108 +++++++++++++++++++++++--------------------
 1 file changed, 58 insertions(+), 50 deletions(-)

drivers/block/loop.c +58 ~~-50~~

index d934d65dbe92..e0f9674fe568 100644
@@ -1276,12 +1276,68 @@ static int loop_clr_fd(struct loop_device *lo)
 }
 
 static int
-loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
+loop_set_from_status(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;
+
+	err = loop_release_xfer(lo);
+	if (err)
+		return err;
+
+	if (info->lo_encrypt_type) {
+		unsigned int type = info->lo_encrypt_type;
+
+		if (type >= MAX_LO_CRYPT)
+			return -EINVAL;
+		xfer = xfer_funcs[type];
+		if (xfer == NULL)
+			return -EINVAL;
+	} else
+		xfer = NULL;
+
+	err = loop_init_xfer(lo, xfer, info);
+	if (err)
+		return err;
+
+	lo->lo_offset = info->lo_offset;
+	lo->lo_sizelimit = info->lo_sizelimit;
+	memcpy(lo->lo_file_name, info->lo_file_name, LO_NAME_SIZE);
+	memcpy(lo->lo_crypt_name, info->lo_crypt_name, LO_NAME_SIZE);
+	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->ioctl = xfer->ioctl;
+
+	if ((lo->lo_flags & LO_FLAGS_AUTOCLEAR) !=
+	     (info->lo_flags & LO_FLAGS_AUTOCLEAR))
+		lo->lo_flags ^= LO_FLAGS_AUTOCLEAR;
+
+	lo->lo_encrypt_key_size = info->lo_encrypt_key_size;
+	lo->lo_init[0] = info->lo_init[0];
+	lo->lo_init[1] = info->lo_init[1];
+	if (info->lo_encrypt_key_size) {
+		memcpy(lo->lo_encrypt_key, info->lo_encrypt_key,
+		       info->lo_encrypt_key_size);
+		lo->lo_key_owner = uid;
+	}
+
+	return 0;
+}
+
+static int
+loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
+{
+	int err;
 	struct block_device *bdev;
+	kuid_t uid = current_uid();
 	bool partscan = false;
 	bool size_changed = false;
 	loff_t validated_size;

@@ -1299,10 +1355,6 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 		err = -ENXIO;
 		goto out_unlock;
 	}
-	if ((unsigned int) info->lo_encrypt_key_size > LO_KEY_SIZE) {
-		err = -EINVAL;
-		goto out_unlock;
-	}
 
 	if (lo->lo_offset != info->lo_offset ||
 	    lo->lo_sizelimit != info->lo_sizelimit) {

@@ -1330,54 +1382,10 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 		goto out_unfreeze;
 	}
 
-	err = loop_release_xfer(lo);
-	if (err)
-		goto out_unfreeze;
-
-	if (info->lo_encrypt_type) {
-		unsigned int type = info->lo_encrypt_type;
-
-		if (type >= MAX_LO_CRYPT) {
-			err = -EINVAL;
-			goto out_unfreeze;
-		}
-		xfer = xfer_funcs[type];
-		if (xfer == NULL) {
-			err = -EINVAL;
-			goto out_unfreeze;
-		}
-	} else
-		xfer = NULL;
-
-	err = loop_init_xfer(lo, xfer, info);
+	err = loop_set_from_status(lo, info);
 	if (err)
 		goto out_unfreeze;
 
-	lo->lo_offset = info->lo_offset;
-	lo->lo_sizelimit = info->lo_sizelimit;
-	memcpy(lo->lo_file_name, info->lo_file_name, LO_NAME_SIZE);
-	memcpy(lo->lo_crypt_name, info->lo_crypt_name, LO_NAME_SIZE);
-	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->ioctl = xfer->ioctl;
-
-	if ((lo->lo_flags & LO_FLAGS_AUTOCLEAR) !=
-	     (info->lo_flags & LO_FLAGS_AUTOCLEAR))
-		lo->lo_flags ^= LO_FLAGS_AUTOCLEAR;
-
-	lo->lo_encrypt_key_size = info->lo_encrypt_key_size;
-	lo->lo_init[0] = info->lo_init[0];
-	lo->lo_init[1] = info->lo_init[1];
-	if (info->lo_encrypt_key_size) {
-		memcpy(lo->lo_encrypt_key, info->lo_encrypt_key,
-		       info->lo_encrypt_key_size);
-		lo->lo_key_owner = uid;
-	}
-
 	if (size_changed)
 		loop_set_size(lo, validated_size);

Signature · 1 line

2.26.1.301.g55bc3eb7cb9-goog

Martijn Coenen

author

2020-04-20 08:04 UTC #1 lore raw

PATCH 1/4 loop: Refactor size calculation.

In reply to Martijn Coenen

figure_loop_size() calculates the loop size based on the passed in parameters, but at the same time it updates the offset and sizelimit parameters in the loop device configuration. That is a somewhat unexpected side effect of a function with this name, and it is only only needed by one of the two callers of this function - loop_set_status().

Move the lo_offset and lo_sizelimit assignment back into loop_set_status(), and factor out a new function (loop_set_size()) to apply a newly calculated size that has been validated to fit in sector_t, and notify user-space.

loop_update_size() is now solely used by loop_set_capacity to compute an updated size, and loop_set_status() uses a combination of functions to validate the loop size, and only apply it later.

Signed-off-by Martijn Coenen <[email protected]>

---
 drivers/block/loop.c | 88 ++++++++++++++++++++++++++++----------------
 1 file changed, 56 insertions(+), 32 deletions(-)

drivers/block/loop.c +56 ~~-32~~

index f1754262fc94..d934d65dbe92 100644
@@ -228,23 +228,42 @@ static void __loop_update_dio(struct loop_device *lo, bool dio)
 		blk_mq_unfreeze_queue(lo->lo_queue);
 }
 
+/**
+ * loop_set_size - sets device size and notifies userspace
+ * @lo: struct loop_device to set the size for
+ * @size: new size of the loop device
+ *
+ * Callers must validate that the size passed into this function fits into
+ * a sector_t.
+ */
+static void loop_set_size(struct loop_device *lo, loff_t size)
+{
+	struct block_device *bdev = lo->lo_device;
+
+	set_capacity(lo->lo_disk, size);
+	bd_set_size(bdev, size << 9);
+	/* let user-space know about the new size */
+	kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
+}
+
+/**
+ * loop_update_size - updates device size from the backing file
+ * @lo: struct loop_device to update the size for
+ *
+ * Recomputes the device size from the backing file, and updates the device
+ * with the new size.
+ */
 static int
-figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit)
+loop_update_size(struct loop_device *lo)
 {
-	loff_t size = get_size(offset, sizelimit, lo->lo_backing_file);
+	loff_t size = get_loop_size(lo, lo->lo_backing_file);
 	sector_t x = (sector_t)size;
-	struct block_device *bdev = lo->lo_device;
 
 	if (unlikely((loff_t)x != size))
 		return -EFBIG;
-	if (lo->lo_offset != offset)
-		lo->lo_offset = offset;
-	if (lo->lo_sizelimit != sizelimit)
-		lo->lo_sizelimit = sizelimit;
-	set_capacity(lo->lo_disk, x);
-	bd_set_size(bdev, (loff_t)get_capacity(bdev->bd_disk) << 9);
-	/* let user-space know about the new size */
-	kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
+
+	loop_set_size(lo, size);
+
 	return 0;
 }

@@ -1040,11 +1059,8 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 
 	loop_update_rotational(lo);
 	loop_update_dio(lo);
-	set_capacity(lo->lo_disk, size);
-	bd_set_size(bdev, size << 9);
 	loop_sysfs_init(lo);
-	/* let user-space know about the new size */
-	kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
+	loop_set_size(lo, size);
 
 	set_blocksize(bdev, S_ISBLK(inode->i_mode) ?
 		      block_size(inode->i_bdev) : PAGE_SIZE);

@@ -1267,6 +1283,8 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 	kuid_t uid = current_uid();
 	struct block_device *bdev;
 	bool partscan = false;
+	bool size_changed = false;
+	loff_t validated_size;
 
 	err = mutex_lock_killable(&loop_ctl_mutex);
 	if (err)

@@ -1288,6 +1306,14 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 
 	if (lo->lo_offset != info->lo_offset ||
 	    lo->lo_sizelimit != info->lo_sizelimit) {
+		size_t size = get_size(info->lo_offset, info->lo_sizelimit,
+				       lo->lo_backing_file);
+		if ((loff_t)(sector_t)size != size) {
+			err = -EFBIG;
+			goto out_unlock;
+		}
+		size_changed = true;
+		validated_size = size;
 		sync_blockdev(lo->lo_device);
 		kill_bdev(lo->lo_device);
 	}

@@ -1295,6 +1321,15 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 	/* I/O need to be drained during transfer transition */
 	blk_mq_freeze_queue(lo->lo_queue);
 
+	if (size_changed && lo->lo_device->bd_inode->i_mapping->nrpages) {
+		/* kill_bdev should have truncated all the pages */
+		err = -EAGAIN;
+		pr_warn("%s: loop%d (%s) has still dirty pages (nrpages=%lu)\n",
+			__func__, lo->lo_number, lo->lo_file_name,
+			lo->lo_device->bd_inode->i_mapping->nrpages);
+		goto out_unfreeze;
+	}
+
 	err = loop_release_xfer(lo);
 	if (err)
 		goto out_unfreeze;

@@ -1318,22 +1353,8 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 	if (err)
 		goto out_unfreeze;
 
-	if (lo->lo_offset != info->lo_offset ||
-	    lo->lo_sizelimit != info->lo_sizelimit) {
-		/* kill_bdev should have truncated all the pages */
-		if (lo->lo_device->bd_inode->i_mapping->nrpages) {
-			err = -EAGAIN;
-			pr_warn("%s: loop%d (%s) has still dirty pages (nrpages=%lu)\n",
-				__func__, lo->lo_number, lo->lo_file_name,
-				lo->lo_device->bd_inode->i_mapping->nrpages);
-			goto out_unfreeze;
-		}
-		if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit)) {
-			err = -EFBIG;
-			goto out_unfreeze;
-		}
-	}
-
+	lo->lo_offset = info->lo_offset;
+	lo->lo_sizelimit = info->lo_sizelimit;
 	memcpy(lo->lo_file_name, info->lo_file_name, LO_NAME_SIZE);
 	memcpy(lo->lo_crypt_name, info->lo_crypt_name, LO_NAME_SIZE);
 	lo->lo_file_name[LO_NAME_SIZE-1] = 0;

@@ -1357,6 +1378,9 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 		lo->lo_key_owner = uid;
 	}
 
+	if (size_changed)
+		loop_set_size(lo, validated_size);
+
 	loop_config_discard(lo);
 
 	/* update dio if lo_offset or transfer is changed */

@@ -1534,7 +1558,7 @@ static int loop_set_capacity(struct loop_device *lo)
 	if (unlikely(lo->lo_state != Lo_bound))
 		return -ENXIO;
 
-	return figure_loop_size(lo, lo->lo_offset, lo->lo_sizelimit);
+	return loop_update_size(lo);
 }
 
 static int loop_set_dio(struct loop_device *lo, unsigned long arg)

Signature · 1 line

2.26.1.301.g55bc3eb7cb9-goog

Martijn Coenen

author

2020-04-20 08:04 UTC #9 lore raw

PATCH 3/4 loop: Move loop_set_from_status() and friends up.

In reply to Martijn Coenen

So we can use it without forward declaration. This is a separate commit to make it easier to verify that this is just a move, without functional modifications.

Signed-off-by Martijn Coenen <[email protected]>

---
 drivers/block/loop.c | 188 +++++++++++++++++++++----------------------
 1 file changed, 94 insertions(+), 94 deletions(-)

drivers/block/loop.c +94 ~~-94~~

index e0f9674fe568..6e656390b285 100644
@@ -971,6 +971,100 @@ static void loop_update_rotational(struct loop_device *lo)
 		blk_queue_flag_clear(QUEUE_FLAG_NONROT, q);
 }
 
+static int
+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);
+		lo->transfer = NULL;
+		lo->lo_encryption = NULL;
+		module_put(xfer->owner);
+	}
+	return err;
+}
+
+static int
+loop_init_xfer(struct loop_device *lo, struct loop_func_table *xfer,
+	       const struct loop_info64 *i)
+{
+	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
+			lo->lo_encryption = xfer;
+	}
+	return err;
+}
+
+static int
+loop_set_from_status(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;
+
+	err = loop_release_xfer(lo);
+	if (err)
+		return err;
+
+	if (info->lo_encrypt_type) {
+		unsigned int type = info->lo_encrypt_type;
+
+		if (type >= MAX_LO_CRYPT)
+			return -EINVAL;
+		xfer = xfer_funcs[type];
+		if (xfer == NULL)
+			return -EINVAL;
+	} else
+		xfer = NULL;
+
+	err = loop_init_xfer(lo, xfer, info);
+	if (err)
+		return err;
+
+	lo->lo_offset = info->lo_offset;
+	lo->lo_sizelimit = info->lo_sizelimit;
+	memcpy(lo->lo_file_name, info->lo_file_name, LO_NAME_SIZE);
+	memcpy(lo->lo_crypt_name, info->lo_crypt_name, LO_NAME_SIZE);
+	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->ioctl = xfer->ioctl;
+
+	if ((lo->lo_flags & LO_FLAGS_AUTOCLEAR) !=
+	     (info->lo_flags & LO_FLAGS_AUTOCLEAR))
+		lo->lo_flags ^= LO_FLAGS_AUTOCLEAR;
+
+	lo->lo_encrypt_key_size = info->lo_encrypt_key_size;
+	lo->lo_init[0] = info->lo_init[0];
+	lo->lo_init[1] = info->lo_init[1];
+	if (info->lo_encrypt_key_size) {
+		memcpy(lo->lo_encrypt_key, info->lo_encrypt_key,
+		       info->lo_encrypt_key_size);
+		lo->lo_key_owner = uid;
+	}
+
+	return 0;
+}
+
 static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 		       struct block_device *bdev, unsigned int arg)
 {

@@ -1094,43 +1188,6 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	return error;
 }
 
-static int
-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);
-		lo->transfer = NULL;
-		lo->lo_encryption = NULL;
-		module_put(xfer->owner);
-	}
-	return err;
-}
-
-static int
-loop_init_xfer(struct loop_device *lo, struct loop_func_table *xfer,
-	       const struct loop_info64 *i)
-{
-	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
-			lo->lo_encryption = xfer;
-	}
-	return err;
-}
-
 static int __loop_clr_fd(struct loop_device *lo, bool release)
 {
 	struct file *filp = NULL;

@@ -1275,63 +1332,6 @@ static int loop_clr_fd(struct loop_device *lo)
 	return __loop_clr_fd(lo, false);
 }
 
-static int
-loop_set_from_status(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;
-
-	err = loop_release_xfer(lo);
-	if (err)
-		return err;
-
-	if (info->lo_encrypt_type) {
-		unsigned int type = info->lo_encrypt_type;
-
-		if (type >= MAX_LO_CRYPT)
-			return -EINVAL;
-		xfer = xfer_funcs[type];
-		if (xfer == NULL)
-			return -EINVAL;
-	} else
-		xfer = NULL;
-
-	err = loop_init_xfer(lo, xfer, info);
-	if (err)
-		return err;
-
-	lo->lo_offset = info->lo_offset;
-	lo->lo_sizelimit = info->lo_sizelimit;
-	memcpy(lo->lo_file_name, info->lo_file_name, LO_NAME_SIZE);
-	memcpy(lo->lo_crypt_name, info->lo_crypt_name, LO_NAME_SIZE);
-	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->ioctl = xfer->ioctl;
-
-	if ((lo->lo_flags & LO_FLAGS_AUTOCLEAR) !=
-	     (info->lo_flags & LO_FLAGS_AUTOCLEAR))
-		lo->lo_flags ^= LO_FLAGS_AUTOCLEAR;
-
-	lo->lo_encrypt_key_size = info->lo_encrypt_key_size;
-	lo->lo_init[0] = info->lo_init[0];
-	lo->lo_init[1] = info->lo_init[1];
-	if (info->lo_encrypt_key_size) {
-		memcpy(lo->lo_encrypt_key, info->lo_encrypt_key,
-		       info->lo_encrypt_key_size);
-		lo->lo_key_owner = uid;
-	}
-
-	return 0;
-}
-
 static int
 loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 {

Signature · 1 line

2.26.1.301.g55bc3eb7cb9-goog

Martijn Coenen

author

2020-04-20 08:05 UTC #11 lore raw

PATCH 4/4 loop: Add LOOP_SET_FD_AND_STATUS ioctl.

In reply to Martijn Coenen

This allows userspace to completely setup a loop device with a single ioctl, removing the in-between state where the device can be partially configured - eg the loop device has a backing file associated with it, but is reading from the wrong offset.

Besides removing the intermediate state, another big benefit of this ioctl is that LOOP_SET_STATUS can be slow; the main reason for this slowness is that LOOP_SET_STATUS(64) calls blk_mq_freeze_queue() to freeze the associated queue; this requires waiting for RCU synchronization, which I've measured can take about 15-20ms on this device on average.

Here's setting up ~70 regular loop devices with an offset on an x86 Android device, using LOOP_SET_FD and LOOP_SET_STATUS:

vsoc_x86:/system/apex # time for i in seq 30 100;
do losetup -r -o 4096 /dev/block/loop$i com.android.adbd.apex; done

    0m03.40s real     0m00.02s user     0m00.03s system

Here's configuring ~70 devices in the same way, but using a modified losetup that uses the new LOOP_SET_FD_AND_STATUS ioctl:

vsoc_x86:/system/apex # time for i in seq 30 100;
do losetup -r -o 4096 /dev/block/loop$i com.android.adbd.apex; done

    0m01.94s real     0m00.01s user     0m00.01s system

Signed-off-by Martijn Coenen <[email protected]>

---
 drivers/block/loop.c      | 47 ++++++++++++++++++++++++++++++---------
 include/uapi/linux/loop.h |  6 +++++
 2 files changed, 42 insertions(+), 11 deletions(-)

drivers/block/loop.c +36 ~~-11~~

index 6e656390b285..e1dbd70d6d6e 100644
@@ -1065,8 +1065,9 @@ loop_set_from_status(struct loop_device *lo, const struct loop_info64 *info)
 	return 0;
 }
 
-static int loop_set_fd(struct loop_device *lo, fmode_t mode,
-		       struct block_device *bdev, unsigned int arg)
+static int loop_set_fd_and_status(struct loop_device *lo, fmode_t mode,
+				  struct block_device *bdev, unsigned int fd,
+				  const struct loop_info64 *info)
 {
 	struct file	*file;
 	struct inode	*inode;

@@ -1081,7 +1082,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	__module_get(THIS_MODULE);
 
 	error = -EBADF;
-	file = fget(arg);
+	file = fget(fd);
 	if (!file)
 		goto out;

@@ -1090,7 +1091,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	 * here to avoid changing device under exclusive owner.
 	 */
 	if (!(mode & FMODE_EXCL)) {
-		claimed_bdev = bd_start_claiming(bdev, loop_set_fd);
+		claimed_bdev = bd_start_claiming(bdev, loop_set_fd_and_status);
 		if (IS_ERR(claimed_bdev)) {
 			error = PTR_ERR(claimed_bdev);
 			goto out_putf;

@@ -1117,9 +1118,24 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 		lo_flags |= LO_FLAGS_READ_ONLY;
 
 	error = -EFBIG;
-	size = get_loop_size(lo, file);
+	if (info)
+		size = get_size(info->lo_offset, info->lo_sizelimit,
+				file);
+	else
+		size = get_loop_size(lo, file);
 	if ((loff_t)(sector_t)size != size)
 		goto out_unlock;
+
+	if (info) {
+		error = loop_set_from_status(lo, info);
+		if (error)
+			goto out_unlock;
+	} else {
+		lo->transfer = NULL;
+		lo->ioctl = NULL;
+		lo->lo_sizelimit = 0;
+		lo->lo_offset = 0;
+	}
 	error = loop_prepare_queue(lo);
 	if (error)
 		goto out_unlock;

@@ -1132,9 +1148,6 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	lo->lo_device = bdev;
 	lo->lo_flags = lo_flags;
 	lo->lo_backing_file = file;
-	lo->transfer = NULL;
-	lo->ioctl = NULL;
-	lo->lo_sizelimit = 0;
 	lo->old_gfp_mask = mapping_gfp_mask(mapping);
 	mapping_set_gfp_mask(mapping, lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS));

@@ -1172,14 +1185,14 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	if (partscan)
 		loop_reread_partitions(lo, bdev);
 	if (claimed_bdev)
-		bd_abort_claiming(bdev, claimed_bdev, loop_set_fd);
+		bd_abort_claiming(bdev, claimed_bdev, loop_set_fd_and_status);
 	return 0;
 
 out_unlock:
 	mutex_unlock(&loop_ctl_mutex);
 out_bdev:
 	if (claimed_bdev)
-		bd_abort_claiming(bdev, claimed_bdev, loop_set_fd);
+		bd_abort_claiming(bdev, claimed_bdev, loop_set_fd_and_status);
 out_putf:
 	fput(file);
 out:

@@ -1653,7 +1666,18 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 
 	switch (cmd) {
 	case LOOP_SET_FD:
-		return loop_set_fd(lo, mode, bdev, arg);
+		return loop_set_fd_and_status(lo, mode, bdev, arg, NULL);
+	case LOOP_SET_FD_AND_STATUS: {
+		struct loop_fd_and_status fds;
+
+		if (copy_from_user(&fds,
+				   (struct loop_fd_and_status __user *) arg,
+				   sizeof(fds)))
+			return -EFAULT;
+
+		return loop_set_fd_and_status(lo, mode, bdev, fds.fd,
+					      &fds.info);
+	}
 	case LOOP_CHANGE_FD:
 		return loop_change_fd(lo, bdev, arg);
 	case LOOP_CLR_FD:

@@ -1827,6 +1851,7 @@ static int lo_compat_ioctl(struct block_device *bdev, fmode_t mode,
 	case LOOP_CLR_FD:
 	case LOOP_GET_STATUS64:
 	case LOOP_SET_STATUS64:
+	case LOOP_SET_FD_AND_STATUS:
 		arg = (unsigned long) compat_ptr(arg);
 		/* fall through */
 	case LOOP_SET_FD:

include/uapi/linux/loop.h +6 -0

index 080a8df134ef..fcc9a693b588 100644
@@ -60,6 +60,11 @@ struct loop_info64 {
 	__u64		   lo_init[2];
 };
 
+struct loop_fd_and_status {
+	struct loop_info64	info;
+	__u32			fd;
+};
+
 /*
  * Loop filter types
  */

@@ -90,6 +95,7 @@ struct loop_info64 {
 #define LOOP_SET_CAPACITY	0x4C07
 #define LOOP_SET_DIRECT_IO	0x4C08
 #define LOOP_SET_BLOCK_SIZE	0x4C09
+#define LOOP_SET_FD_AND_STATUS	0x4C0A
 
 /* /dev/loop-control interface */
 #define LOOP_CTL_ADD		0x4C80

Signature · 1 line

2.26.1.301.g55bc3eb7cb9-goog

Bart Van Assche

2020-04-20 13:22 UTC #2 lore raw

Re: [PATCH 1/4] loop: Refactor size calculation.

In reply to Martijn Coenen

Quoted 7 lines · Martijn Coenen

drivers/block/loop.c +7 -0

+/**
+ * loop_set_size - sets device size and notifies userspace
+ * @lo: struct loop_device to set the size for
+ * @size: new size of the loop device
+ *
+ * Callers must validate that the size passed into this function fits into
+ * a sector_t.

drivers/block/loop.c static void __loop_update_dio(struct loop_device *lo, bool dio) · line 247

Quoted 10 lines

drivers/block/loop.c +10 -0

+ */
+static void loop_set_size(struct loop_device *lo, loff_t size)
+{
+	struct block_device *bdev = lo->lo_device;
+
+	set_capacity(lo->lo_disk, size);
+	bd_set_size(bdev, size << 9);
+	/* let user-space know about the new size */
+	kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
+}

How about using the SECTOR_SHIFT constant instead of "9"? I think the following has been added recently in include/linux/blkdev.h:

#define SECTOR_SHIFT 9

Quoted 6 lines

drivers/block/loop.c +2 -0

@@ -1295,6 +1321,15 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
  	/* I/O need to be drained during transfer transition */
  	blk_mq_freeze_queue(lo->lo_queue);
 
+	if (size_changed && lo->lo_device->bd_inode->i_mapping->nrpages) {
+		/* kill_bdev should have truncated all the pages */

drivers/block/loop.c loop_set_status(struct loop_device *lo, const struct loop_info64 *info) · line 1335

Quoted 10 lines

drivers/block/loop.c +7 -0

+		err = -EAGAIN;
+		pr_warn("%s: loop%d (%s) has still dirty pages (nrpages=%lu)\n",
+			__func__, lo->lo_number, lo->lo_file_name,
+			lo->lo_device->bd_inode->i_mapping->nrpages);
+		goto out_unfreeze;
+	}
+
  	err = loop_release_xfer(lo);
  	if (err)
  		goto out_unfreeze;

Please Cc Jaegeuk for any changes in this code (see also https://lore.kernel.org/linux-block/[email protected]/).

Please also change the "kill_bdev should have truncated all the pages" comment into something like "return -EAGAIN if any pages have been dirtied after kill_bdev() returned".

Thanks,

Bart.

Bart Van Assche

2020-04-20 13:34 UTC #6 lore raw

Re: [PATCH 2/4] loop: Factor out configuring loop from status.

In reply to Martijn Coenen

On 4/20/20 1:04 AM, Martijn Coenen wrote:

No trailing dot at the end of a patch subject please.

Quoted 47 lines

static int

drivers/block/loop.c +41 -1

-loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
+loop_set_from_status(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;
+
+	err = loop_release_xfer(lo);
+	if (err)
+		return err;
+
+	if (info->lo_encrypt_type) {
+		unsigned int type = info->lo_encrypt_type;
+
+		if (type >= MAX_LO_CRYPT)
+			return -EINVAL;
+		xfer = xfer_funcs[type];
+		if (xfer == NULL)
+			return -EINVAL;
+	} else
+		xfer = NULL;
+
+	err = loop_init_xfer(lo, xfer, info);
+	if (err)
+		return err;
+
+	lo->lo_offset = info->lo_offset;
+	lo->lo_sizelimit = info->lo_sizelimit;
+	memcpy(lo->lo_file_name, info->lo_file_name, LO_NAME_SIZE);
+	memcpy(lo->lo_crypt_name, info->lo_crypt_name, LO_NAME_SIZE);
+	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->ioctl = xfer->ioctl;
+
+	if ((lo->lo_flags & LO_FLAGS_AUTOCLEAR) !=
+	     (info->lo_flags & LO_FLAGS_AUTOCLEAR))
+		lo->lo_flags ^= LO_FLAGS_AUTOCLEAR;
+
+	lo->lo_encrypt_key_size = info->lo_encrypt_key_size;

drivers/block/loop.c static int loop_clr_fd(struct loop_device *lo) · line 1333

Quoted 10 lines

drivers/block/loop.c +10 -0

+	lo->lo_init[0] = info->lo_init[0];
+	lo->lo_init[1] = info->lo_init[1];
+	if (info->lo_encrypt_key_size) {
+		memcpy(lo->lo_encrypt_key, info->lo_encrypt_key,
+		       info->lo_encrypt_key_size);
+		lo->lo_key_owner = uid;
+	}
+
+	return 0;
+}

Please add a (one line?) comment above this function that explains the purpose of this function. Is the purpose of this function perhaps to initialize loop device parameters based on the information received from user space (the 'info' argument)?

Thanks,

Bart.

Bart Van Assche

Reviewed-by on v1 3/4

2020-04-20 13:43 UTC #10 lore raw

Re: [PATCH 3/4] loop: Move loop_set_from_status() and friends up.

In reply to Martijn Coenen

Quoted 3 lines · Martijn Coenen

So we can use it without forward declaration. This is a separate commit to make it easier to verify that this is just a move, without functional modifications.

Reviewed-by Bart Van Assche <[email protected]>

Bart Van Assche

2020-04-20 13:50 UTC #7 lore raw

Re: [PATCH 2/4] loop: Factor out configuring loop from status.

In reply to Martijn Coenen

drivers/block/loop.c static int loop_clr_fd(struct loop_device *lo) · line 1280

Quoted 4 lines · Martijn Coenen

static int

drivers/block/loop.c +1 -1

-loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
+loop_set_from_status(struct loop_device *lo, const struct loop_info64 *info)
  {

An additional question: since this function sets the status of 'lo' based on the information in 'info', would "loop_set_status" or "loop_set_status_from_info" be a better name for this function?

Thanks,

Bart.

Martijn Coenen

author

2020-04-21 11:46 UTC #8 lore raw

Re: [PATCH 2/4] loop: Factor out configuring loop from status.

Quoted 4 lines · Bart Van Assche

An additional question: since this function sets the status of 'lo' based on the information in 'info', would "loop_set_status" or "loop_set_status_from_info" be a better name for this function?

Yeah, I like the latter. I will rename, and add a comment that explains the purpose more clearly.

Thanks,
Martijn

Quoted 4 lines

Thanks,
Bart.

Martijn Coenen

author

2020-04-21 11:48 UTC #3 lore raw

Re: [PATCH 1/4] loop: Refactor size calculation.

In reply to Bart Van Assche

Quoted 1 line · Bart Van Assche

How about using the SECTOR_SHIFT constant instead of "9"?

Ack, will do.

Quoted 3 lines

Please also change the "kill_bdev should have truncated all the pages" comment into something like "return -EAGAIN if any pages have been dirtied after kill_bdev() returned".

Sure - would you prefer this to be in a separate change?

Thanks,
Martijn

Quoted 4 lines

Thanks,
Bart.

Bart Van Assche

2020-04-21 15:26 UTC #4 lore raw

Re: [PATCH 1/4] loop: Refactor size calculation.

Quoted 6 lines · Martijn Coenen

Quoted 3 lines · Bart Van Assche
Please also change the "kill_bdev should have truncated all the pages" comment into something like "return -EAGAIN if any pages have been dirtied after kill_bdev() returned".
Sure - would you prefer this to be in a separate change?

Since the comment "kill_bdev should have truncated all the pages" has to be moved, I think that it's fine to integrate the fix for that comment in this patch.

Thanks,

Bart.

Christoph Hellwig

2020-04-22 06:19 UTC #12 lore raw

Re: [PATCH 4/4] loop: Add LOOP_SET_FD_AND_STATUS ioctl.

In reply to Martijn Coenen

Quoted 79 lines · Martijn Coenen

This allows userspace to completely setup a loop device with a single ioctl, removing the in-between state where the device can be partially configured - eg the loop device has a backing file associated with it, but is reading from the wrong offset.
Besides removing the intermediate state, another big benefit of this ioctl is that LOOP_SET_STATUS can be slow; the main reason for this slowness is that LOOP_SET_STATUS(64) calls blk_mq_freeze_queue() to freeze the associated queue; this requires waiting for RCU synchronization, which I've measured can take about 15-20ms on this device on average.
Here's setting up ~70 regular loop devices with an offset on an x86 Android device, using LOOP_SET_FD and LOOP_SET_STATUS:
vsoc_x86:/system/apex # time for i in seq 30 100;
do losetup -r -o 4096 /dev/block/loop$i com.android.adbd.apex; done
    0m03.40s real     0m00.02s user     0m00.03s system
Here's configuring ~70 devices in the same way, but using a modified losetup that uses the new LOOP_SET_FD_AND_STATUS ioctl:
vsoc_x86:/system/apex # time for i in seq 30 100;
do losetup -r -o 4096 /dev/block/loop$i com.android.adbd.apex; done
    0m01.94s real     0m00.01s user     0m00.01s system
Signed-off-by Martijn Coenen <[email protected]>
---
 drivers/block/loop.c      | 47 ++++++++++++++++++++++++++++++---------
 include/uapi/linux/loop.h |  6 +++++
 2 files changed, 42 insertions(+), 11 deletions(-)
drivers/block/loop.c +11 -5
index 6e656390b285..e1dbd70d6d6e 100644
@@ -1065,8 +1065,9 @@ loop_set_from_status(struct loop_device *lo, const struct loop_info64 *info)
 	return 0;
 }
 
-static int loop_set_fd(struct loop_device *lo, fmode_t mode,
-		       struct block_device *bdev, unsigned int arg)
+static int loop_set_fd_and_status(struct loop_device *lo, fmode_t mode,
+				  struct block_device *bdev, unsigned int fd,
+				  const struct loop_info64 *info)
 {
 	struct file	*file;
 	struct inode	*inode;
@@ -1081,7 +1082,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	__module_get(THIS_MODULE);
 
 	error = -EBADF;
-	file = fget(arg);
+	file = fget(fd);
 	if (!file)
 		goto out;
 
@@ -1090,7 +1091,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	 * here to avoid changing device under exclusive owner.
 	 */
 	if (!(mode & FMODE_EXCL)) {
-		claimed_bdev = bd_start_claiming(bdev, loop_set_fd);
+		claimed_bdev = bd_start_claiming(bdev, loop_set_fd_and_status);
 		if (IS_ERR(claimed_bdev)) {
 			error = PTR_ERR(claimed_bdev);
 			goto out_putf;
@@ -1117,9 +1118,24 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 		lo_flags |= LO_FLAGS_READ_ONLY;
 
 	error = -EFBIG;
-	size = get_loop_size(lo, file);
+	if (info)
+		size = get_size(info->lo_offset, info->lo_sizelimit,
+				file);
+	else
+		size = get_loop_size(lo, file);
 	if ((loff_t)(sector_t)size != size)
 		goto out_unlock;
+

drivers/block/loop.c static int loop_set_fd(struct loop_device *lo, fmode_t mode, · line 1138

Quoted 10 lines

drivers/block/loop.c +10 -0

+	if (info) {
+		error = loop_set_from_status(lo, info);
+		if (error)
+			goto out_unlock;
+	} else {
+		lo->transfer = NULL;
+		lo->ioctl = NULL;
+		lo->lo_sizelimit = 0;
+		lo->lo_offset = 0;
+	}

Just curious: Can't we just pass in an on-stack info for the legacy case and avoid all these conditionals?

Quoted 8 lines

drivers/block/loop.c +2 -1

 out:

@@ -1653,7 +1666,18 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 
 	switch (cmd) {
 	case LOOP_SET_FD:
-		return loop_set_fd(lo, mode, bdev, arg);
+		return loop_set_fd_and_status(lo, mode, bdev, arg, NULL);
+	case LOOP_SET_FD_AND_STATUS: {

drivers/block/loop.c static int lo_ioctl(struct block_device *bdev, fmode_t mode, · line 1680

Quoted 10 lines

drivers/block/loop.c +10 -0

+		struct loop_fd_and_status fds;
+
+		if (copy_from_user(&fds,
+				   (struct loop_fd_and_status __user *) arg,
+				   sizeof(fds)))
+			return -EFAULT;
+
+		return loop_set_fd_and_status(lo, mode, bdev, fds.fd,
+					      &fds.info);
+	}

Can you throw in another prep patch that adds a:

	void __user *argp = (void __user *)arg;

line at the top of lo_compat_ioctl, and switches the LOOP_SET_STATUS and LOOP_GET_STATUS case to it?

The nhere you can just do:

		if (copy_from_user(&p, argp, sizeof(fds)))
			return -EFAULT;
		return loop_set_fd_and_status(lo, mode, bdev, p.fd, &p.info);

and be done.

drivers/block/loop.c static int lo_ioctl(struct block_device *bdev, fmode_t mode, · line 1683

Quoted 3 lines

drivers/block/loop.c +3 -0

+struct loop_fd_and_status {
+	struct loop_info64	info;
+	__u32			fd;

This should grow a

	__u32	__pad;

to avoid different struct sizes on x86-32 vs x86-64.

Martijn Coenen

author

2020-04-22 08:06 UTC #13 lore raw

Re: [PATCH 4/4] loop: Add LOOP_SET_FD_AND_STATUS ioctl.

Quoted 86 lines · Christoph Hellwig

Quoted 84 lines · Martijn Coenen

Here's setting up ~70 regular loop devices with an offset on an x86 Android device, using LOOP_SET_FD and LOOP_SET_STATUS:

vsoc_x86:/system/apex # time for i in seq 30 100;
do losetup -r -o 4096 /dev/block/loop$i com.android.adbd.apex; done

    0m03.40s real     0m00.02s user     0m00.03s system

Here's configuring ~70 devices in the same way, but using a modified losetup that uses the new LOOP_SET_FD_AND_STATUS ioctl:

vsoc_x86:/system/apex # time for i in seq 30 100;
do losetup -r -o 4096 /dev/block/loop$i com.android.adbd.apex; done

    0m01.94s real     0m00.01s user     0m00.01s system

Signed-off-by Martijn Coenen <[email protected]>

---
 drivers/block/loop.c      | 47 ++++++++++++++++++++++++++++++---------
 include/uapi/linux/loop.h |  6 +++++
 2 files changed, 42 insertions(+), 11 deletions(-)

drivers/block/loop.c +16 -5

index 6e656390b285..e1dbd70d6d6e 100644
@@ -1065,8 +1065,9 @@ loop_set_from_status(struct loop_device *lo, const struct loop_info64 *info)
      return 0;
 }
 
-static int loop_set_fd(struct loop_device *lo, fmode_t mode,
-                    struct block_device *bdev, unsigned int arg)
+static int loop_set_fd_and_status(struct loop_device *lo, fmode_t mode,
+                               struct block_device *bdev, unsigned int fd,
+                               const struct loop_info64 *info)
 {
      struct file     *file;
      struct inode    *inode;

@@ -1081,7 +1082,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
      __module_get(THIS_MODULE);
 
      error = -EBADF;
-     file = fget(arg);
+     file = fget(fd);
      if (!file)
              goto out;

@@ -1090,7 +1091,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
       * here to avoid changing device under exclusive owner.
       */
      if (!(mode & FMODE_EXCL)) {
-             claimed_bdev = bd_start_claiming(bdev, loop_set_fd);
+             claimed_bdev = bd_start_claiming(bdev, loop_set_fd_and_status);
              if (IS_ERR(claimed_bdev)) {
                      error = PTR_ERR(claimed_bdev);
                      goto out_putf;

@@ -1117,9 +1118,24 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
              lo_flags |= LO_FLAGS_READ_ONLY;
 
      error = -EFBIG;
-     size = get_loop_size(lo, file);
+     if (info)
+             size = get_size(info->lo_offset, info->lo_sizelimit,
+                             file);
+     else
+             size = get_loop_size(lo, file);
      if ((loff_t)(sector_t)size != size)
              goto out_unlock;
+
+     if (info) {
+             error = loop_set_from_status(lo, info);
+             if (error)
+                     goto out_unlock;
+     } else {

Quoted 8 lines

drivers/block/loop.c static int loop_set_fd(struct loop_device *lo, fmode_t mode, · line 1138
Quoted 5 lines
drivers/block/loop.c +5 -0
+             lo->transfer = NULL;
+             lo->ioctl = NULL;
+             lo->lo_sizelimit = 0;
+             lo->lo_offset = 0;
+     }
Just curious: Can't we just pass in an on-stack info for the legacy case and avoid all these conditionals?

Yeah, that is actually much nicer. I will rework it to that.

Quoted 6 lines

Can you throw in another prep patch that adds a:
        void __user *argp = (void __user *)arg;
line at the top of lo_compat_ioctl, and switches the LOOP_SET_STATUS and LOOP_GET_STATUS case to it?

Did you mean in regular lo_ioctl()? eg something like this:

@@ -1671,6 +1671,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
unsigned int cmd, unsigned long arg)

 {
        struct loop_device *lo = bdev->bd_disk->private_data;
+       void __user *argp = (void __user *) arg;
        int err;

        switch (cmd) {
@@ -1694,21 +1695,19 @@ static int lo_ioctl(struct block_device *bdev,
fmode_t mode,
        case LOOP_SET_STATUS:
                err = -EPERM;
                if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN)) {
-                       err = loop_set_status_old(lo,
-                                       (struct loop_info __user *)arg);
+                       err = loop_set_status_old(lo, argp);
                }
                break;
        case LOOP_GET_STATUS:
-               return loop_get_status_old(lo, (struct loop_info __user *) arg);
+               return loop_get_status_old(lo, argp);
        case LOOP_SET_STATUS64:
                err = -EPERM;
                if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN)) {
-                       err = loop_set_status64(lo,
-                                       (struct loop_info64 __user *) arg);
+                       err = loop_set_status64(lo, argp);
                }
                break;
        case LOOP_GET_STATUS64:

- return loop_get_status64(lo, (struct loop_info64 __user *) arg);
+ return loop_get_status64(lo, argp);

Quoted 9 lines

drivers/block/loop.c static int loop_set_fd(struct loop_device *lo, fmode_t mode, · line 1141
Quoted 3 lines
drivers/block/loop.c +3 -0
+struct loop_fd_and_status {
+     struct loop_info64      info;
+     __u32                   fd;
This should grow a
        __u32   __pad;
to avoid different struct sizes on x86-32 vs x86-64.

will do, thanks!

Christoph Hellwig

2020-04-22 08:07 UTC #14 lore raw

Re: [PATCH 4/4] loop: Add LOOP_SET_FD_AND_STATUS ioctl.

Quoted 4 lines · Martijn Coenen

Quoted 2 lines
line at the top of lo_compat_ioctl, and switches the LOOP_SET_STATUS and LOOP_GET_STATUS case to it?
Did you mean in regular lo_ioctl()?

Yes, sorry.

Quoted 25 lines

eg something like this:

(quoted diff) +1 -0

@@ -1671,6 +1671,7 @@ static int lo_ioctl(struct block_device *bdev,
 fmode_t mode,
        unsigned int cmd, unsigned long arg)
 {
        struct loop_device *lo = bdev->bd_disk->private_data;
+       void __user *argp = (void __user *) arg;
        int err;

        switch (cmd) {

(quoted diff) +2 -3

@@ -1694,21 +1695,19 @@ static int lo_ioctl(struct block_device *bdev,
 fmode_t mode,
        case LOOP_SET_STATUS:
                err = -EPERM;
                if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN)) {
-                       err = loop_set_status_old(lo,
-                                       (struct loop_info __user *)arg);
+                       err = loop_set_status_old(lo, argp);
                }
                break;
        case LOOP_GET_STATUS:
-               return loop_get_status_old(lo, (struct loop_info __user *) arg);
+               return loop_get_status_old(lo, argp);
        case LOOP_SET_STATUS64:

Review static int lo_ioctl(struct block_device *bdev, · line 1711

Quoted 10 lines

(quoted diff) +2 -3

                err = -EPERM;
                if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN)) {
-                       err = loop_set_status64(lo,
-                                       (struct loop_info64 __user *) arg);
+                       err = loop_set_status64(lo, argp);
                }
                break;
        case LOOP_GET_STATUS64:
-               return loop_get_status64(lo, (struct loop_info64 __user *) arg);
+               return loop_get_status64(lo, argp);

Exactly!

Bart Van Assche

2020-04-22 15:11 UTC #15 lore raw

Re: [PATCH 4/4] loop: Add LOOP_SET_FD_AND_STATUS ioctl.

In reply to Martijn Coenen

Quoted 17 lines · Martijn Coenen

include/uapi/linux/loop.h +5 -0

index 080a8df134ef..fcc9a693b588 100644
@@ -60,6 +60,11 @@ struct loop_info64 {
  	__u64		   lo_init[2];
  };
 
+struct loop_fd_and_status {
+	struct loop_info64	info;
+	__u32			fd;
+};
+
  /*
   * Loop filter types
   */

include/uapi/linux/loop.h struct loop_info64 { · line 101

Quoted 7 lines

include/uapi/linux/loop.h +1 -0

@@ -90,6 +95,7 @@ struct loop_info64 {
  #define LOOP_SET_CAPACITY	0x4C07
  #define LOOP_SET_DIRECT_IO	0x4C08
  #define LOOP_SET_BLOCK_SIZE	0x4C09
+#define LOOP_SET_FD_AND_STATUS	0x4C0A
 
  /* /dev/loop-control interface */
  #define LOOP_CTL_ADD		0x4C80

Should linux-api be Cc'ed for this patch or the entire patch series? From https://www.kernel.org/doc/man-pages/linux-api-ml.html: "Among other things, a primary goal of the list is to help answer the question: How do we even know when an interface has been added or changed?".

Thanks,

Bart.

Martijn Coenen

author

2020-04-27 07:42 UTC #16 lore raw

Re: [PATCH 4/4] loop: Add LOOP_SET_FD_AND_STATUS ioctl.

Quoted 4 lines · Bart Van Assche

Should linux-api be Cc'ed for this patch or the entire patch series?
From https://www.kernel.org/doc/man-pages/linux-api-ml.html: "Among
other things, a primary goal of the list is to help answer the question:
How do we even know when an interface has been added or changed?".

Thanks for the suggestion, will add linux-api in the next series.

Martijn

Quoted 6 lines

Thanks,
Bart.