[v2] loop: LOOP_CONFIGURE: send uevents for partitions

  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(-)
  

On Sun, Mar 12, 2023 at 07:10:31PM +0000, Alyssa Ross wrote:
> +	 * 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);

What worries me here is that you move the partition re-read out of
the exclusive claim, which is another potentially user visible
change (and user visible behavior changes are a field of landmines
in loop as you have noticed).

But in the end we only need to suppress the events until Lo_Bound
is set.  So something like the patch below that reduces the no even
critical section might do the job?

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 839373451c2b7d..9d61c027185141 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1010,9 +1010,6 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
 	/* This is safe, since we have a reference from open(). */
 	__module_get(THIS_MODULE);
 
-	/* suppress uevents while reconfiguring the device */
-	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
-
 	/*
 	 * If we don't hold exclusive handle for the device, upgrade to it
 	 * here to avoid changing device under exclusive owner.
@@ -1067,6 +1064,9 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
 		}
 	}
 
+	/* suppress uevents while reconfiguring the device */
+	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
+
 	disk_force_media_change(lo->lo_disk, DISK_EVENT_MEDIA_CHANGE);
 	set_disk_ro(lo->lo_disk, (lo->lo_flags & LO_FLAGS_READ_ONLY) != 0);
 
@@ -1109,17 +1109,17 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
 	if (partscan)
 		clear_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
 
+	/* enable and uncork uevent now that we are done */
+	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
+
 	loop_global_unlock(lo, is_loop);
 	if (partscan)
 		loop_reread_partitions(lo);
+
 	if (!(mode & FMODE_EXCL))
 		bd_abort_claiming(bdev, loop_configure);
 
-	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;
+	return 0;
 
 out_unlock:
 	loop_global_unlock(lo, is_loop);
@@ -1130,7 +1130,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);
-	goto done;
+	return error;
 }
 
 static void __loop_clr_fd(struct loop_device *lo, bool release)
  

On Wed, Mar 15, 2023 at 08:48:40AM -0700, Christoph Hellwig wrote:
> On Sun, Mar 12, 2023 at 07:10:31PM +0000, Alyssa Ross wrote:
> > +	 * 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);
>
> What worries me here is that you move the partition re-read out of
> the exclusive claim, which is another potentially user visible
> change (and user visible behavior changes are a field of landmines
> in loop as you have noticed).

Makes sense.

> But in the end we only need to suppress the events until Lo_Bound
> is set.  So something like the patch below that reduces the no even
> critical section might do the job?

If you say so!  I had trouble understanding which parts of the function
uevents needed to be suppressed for, so I was trying to move as little
as possible out of that section.

What happens next?  I'm still getting up to speed on the kernel
development process — will you submit this as a patch with a patch body
and a S-o-b?  Or am I supposed to do something with it?

I know enough to know that I should give you a:

Tested-by: Alyssa Ross <hi@alyssa.is>

> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 839373451c2b7d..9d61c027185141 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1010,9 +1010,6 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
>  	/* This is safe, since we have a reference from open(). */
>  	__module_get(THIS_MODULE);
>
> -	/* suppress uevents while reconfiguring the device */
> -	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
> -
>  	/*
>  	 * If we don't hold exclusive handle for the device, upgrade to it
>  	 * here to avoid changing device under exclusive owner.
> @@ -1067,6 +1064,9 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
>  		}
>  	}
>
> +	/* suppress uevents while reconfiguring the device */
> +	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
> +
>  	disk_force_media_change(lo->lo_disk, DISK_EVENT_MEDIA_CHANGE);
>  	set_disk_ro(lo->lo_disk, (lo->lo_flags & LO_FLAGS_READ_ONLY) != 0);
>
> @@ -1109,17 +1109,17 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
>  	if (partscan)
>  		clear_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
>
> +	/* enable and uncork uevent now that we are done */
> +	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
> +
>  	loop_global_unlock(lo, is_loop);
>  	if (partscan)
>  		loop_reread_partitions(lo);
> +
>  	if (!(mode & FMODE_EXCL))
>  		bd_abort_claiming(bdev, loop_configure);
>
> -	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;
> +	return 0;
>
>  out_unlock:
>  	loop_global_unlock(lo, is_loop);
> @@ -1130,7 +1130,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);
> -	goto done;
> +	return error;
>  }
>
>  static void __loop_clr_fd(struct loop_device *lo, bool release)
  

On Sat, Mar 18, 2023 at 01:50:05AM +0000, Alyssa Ross wrote:
> If you say so!  I had trouble understanding which parts of the function
> uevents needed to be suppressed for, so I was trying to move as little
> as possible out of that section.
> 
> What happens next?  I'm still getting up to speed on the kernel
> development process — will you submit this as a patch with a patch body
> and a S-o-b?  Or am I supposed to do something with it?

Given that you're done all the hard work, and I've just reduced the
critical section, I'd prefer to give all the credit to you.  If you're
fine with it, I'll send out this version later:

---
From 4648015b4193c81d3de8c1632876314b4a2ab40d Mon Sep 17 00:00:00 2001
Subject: loop: LOOP_CONFIGURE: send uevents for partitions

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, reduce the critical
section to exclude 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>
[hch: reduced the critical section]
Signed-off-by: Christoph Hellwig <hch@lst.de>
Fixes: 3448914e8cc5 ("loop: Add LOOP_CONFIGURE ioctl")
---
 drivers/block/loop.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 839373451c2b7d..9d61c027185141 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1010,9 +1010,6 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
 	/* This is safe, since we have a reference from open(). */
 	__module_get(THIS_MODULE);
 
-	/* suppress uevents while reconfiguring the device */
-	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
-
 	/*
 	 * If we don't hold exclusive handle for the device, upgrade to it
 	 * here to avoid changing device under exclusive owner.
@@ -1067,6 +1064,9 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
 		}
 	}
 
+	/* suppress uevents while reconfiguring the device */
+	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
+
 	disk_force_media_change(lo->lo_disk, DISK_EVENT_MEDIA_CHANGE);
 	set_disk_ro(lo->lo_disk, (lo->lo_flags & LO_FLAGS_READ_ONLY) != 0);
 
@@ -1109,17 +1109,17 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
 	if (partscan)
 		clear_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
 
+	/* enable and uncork uevent now that we are done */
+	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
+
 	loop_global_unlock(lo, is_loop);
 	if (partscan)
 		loop_reread_partitions(lo);
+
 	if (!(mode & FMODE_EXCL))
 		bd_abort_claiming(bdev, loop_configure);
 
-	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;
+	return 0;
 
 out_unlock:
 	loop_global_unlock(lo, is_loop);
@@ -1130,7 +1130,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);
-	goto done;
+	return error;
 }
 
 static void __loop_clr_fd(struct loop_device *lo, bool release)
  

On Sun, Mar 19, 2023 at 11:22:25PM -0700, Christoph Hellwig wrote:
> On Sat, Mar 18, 2023 at 01:50:05AM +0000, Alyssa Ross wrote:
> > What happens next?  I'm still getting up to speed on the kernel
> > development process — will you submit this as a patch with a patch body
> > and a S-o-b?  Or am I supposed to do something with it?
>
> Given that you're done all the hard work, and I've just reduced the
> critical section, I'd prefer to give all the credit to you.  If you're
> fine with it, I'll send out this version later:

LGTM, thanks!

> ---
> From 4648015b4193c81d3de8c1632876314b4a2ab40d Mon Sep 17 00:00:00 2001
> Subject: loop: LOOP_CONFIGURE: send uevents for partitions
>
> 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, reduce the critical
> section to exclude 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>
> [hch: reduced the critical section]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Fixes: 3448914e8cc5 ("loop: Add LOOP_CONFIGURE ioctl")
> ---
>  drivers/block/loop.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 839373451c2b7d..9d61c027185141 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1010,9 +1010,6 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
>  	/* This is safe, since we have a reference from open(). */
>  	__module_get(THIS_MODULE);
>
> -	/* suppress uevents while reconfiguring the device */
> -	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
> -
>  	/*
>  	 * If we don't hold exclusive handle for the device, upgrade to it
>  	 * here to avoid changing device under exclusive owner.
> @@ -1067,6 +1064,9 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
>  		}
>  	}
>
> +	/* suppress uevents while reconfiguring the device */
> +	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
> +
>  	disk_force_media_change(lo->lo_disk, DISK_EVENT_MEDIA_CHANGE);
>  	set_disk_ro(lo->lo_disk, (lo->lo_flags & LO_FLAGS_READ_ONLY) != 0);
>
> @@ -1109,17 +1109,17 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
>  	if (partscan)
>  		clear_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
>
> +	/* enable and uncork uevent now that we are done */
> +	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
> +
>  	loop_global_unlock(lo, is_loop);
>  	if (partscan)
>  		loop_reread_partitions(lo);
> +
>  	if (!(mode & FMODE_EXCL))
>  		bd_abort_claiming(bdev, loop_configure);
>
> -	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;
> +	return 0;
>
>  out_unlock:
>  	loop_global_unlock(lo, is_loop);
> @@ -1130,7 +1130,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);
> -	goto done;
> +	return error;
>  }
>
>  static void __loop_clr_fd(struct loop_device *lo, bool release)
> --
> 2.39.2
>