Feedback-ID: i12284293:Fastmail
From:   Alyssa Ross <hi@alyssa.is>
To:     Jens Axboe <axboe@kernel.dk>
Cc:     linux-kernel@vger.kernel.org, Alyssa Ross <hi@alyssa.is>,
        Martijn Coenen <maco@android.com>, linux-block@vger.kernel.org
Subject: [PATCH v2] loop: LOOP_CONFIGURE: send uevents for partitions
Date:   Sun, 12 Mar 2023 19:10:31 +0000
Message-Id: <20230312191031.551204-1-hi@alyssa.is>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

LOOP_CONFIGURE is, as far as I understand it, supposed to be a way to
combine LOOP_SET_FD and LOOP_SET_STATUS64 into a single syscall.  When
using LOOP_SET_FD+LOOP_SET_STATUS64, a single uevent would be sent for
each partition found on the loop device after the second ioctl(), but
when using LOOP_CONFIGURE, no such uevent was being sent.

In the old setup, uevents are disabled for LOOP_SET_FD, but not for
LOOP_SET_STATUS64.  This makes sense, as it prevents uevents being
sent for a partially configured device during LOOP_SET_FD — they're
only sent at the end of LOOP_SET_STATUS64.  But for LOOP_CONFIGURE,
uevents were disabled for the entire operation, so that final
notification was never issued.  To fix this, I've moved the
loop_reread_partitions() call, which causes the uevents to be issued,
to after uevents are re-enabled, matching the behaviour of the
LOOP_SET_FD+LOOP_SET_STATUS64 combination.

I noticed this because Busybox's losetup program recently changed from
using LOOP_SET_FD+LOOP_SET_STATUS64 to LOOP_CONFIGURE, and this broke
my setup, for which I want a notification from the kernel any time a
new partition becomes available.

Signed-off-by: Alyssa Ross <hi@alyssa.is>
Fixes: 3448914e8cc5 ("loop: Add LOOP_CONFIGURE ioctl")
---

v1: https://lore.kernel.org/linux-block/20230221222847.607096-1-hi@alyssa.is/

v1 was an RFC, because I was looking for advice on how to handle
distinguishing between LOOP_SET_FD with non-zero max_part (in which
case partscan will be true, but a uevent should not be emitted), and
LOOP_CONFIGURE (where a uevent should be emitted).  I didn't hear
anything, but I did some experimentation of my own, and adding a
partscan_uevent parameter to distinguish between LOOP_SET_FD and
LOOP_CONFIGURE feels like the least bad solution to me.

 drivers/block/loop.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 839373451c2b..f00a0209b522 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -992,7 +992,8 @@ loop_set_status_from_info(struct loop_device *lo,
 
 static int loop_configure(struct loop_device *lo, fmode_t mode,
 			  struct block_device *bdev,
-			  const struct loop_config *config)
+			  const struct loop_config *config,
+			  bool partscan_uevent)
 {
 	struct file *file = fget(config->fd);
 	struct inode *inode;
@@ -1110,15 +1111,21 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
 		clear_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
 
 	loop_global_unlock(lo, is_loop);
-	if (partscan)
-		loop_reread_partitions(lo);
 	if (!(mode & FMODE_EXCL))
 		bd_abort_claiming(bdev, loop_configure);
 
+	/*
+	 * Now that we are done, reread the partitions with uevent
+	 * re-enabled if appropriate to let userspace know about the
+	 * changes.
+	 */
+	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), !partscan_uevent);
+	if (partscan)
+		loop_reread_partitions(lo);
+	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
+
 	error = 0;
 done:
-	/* enable and uncork uevent now that we are done */
-	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
 	return error;
 
 out_unlock:
@@ -1130,6 +1136,7 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
 	fput(file);
 	/* This is safe: open() is still holding a reference. */
 	module_put(THIS_MODULE);
+	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
 	goto done;
 }
 
@@ -1547,7 +1554,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 		memset(&config, 0, sizeof(config));
 		config.fd = arg;
 
-		return loop_configure(lo, mode, bdev, &config);
+		return loop_configure(lo, mode, bdev, &config, false);
 	}
 	case LOOP_CONFIGURE: {
 		struct loop_config config;
@@ -1555,7 +1562,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 		if (copy_from_user(&config, argp, sizeof(config)))
 			return -EFAULT;
 
-		return loop_configure(lo, mode, bdev, &config);
+		return loop_configure(lo, mode, bdev, &config, true);
 	}
 	case LOOP_CHANGE_FD:
 		return loop_change_fd(lo, bdev, arg);
-- 
2.37.1