[PATCH] loop: fix partition scan race between udev and loop_reread_partitions()

[PATCH] loop: fix partition scan race between udev and loop_reread_partitions()

[Daan De Meyer]

When LOOP_CONFIGURE is called with LO_FLAGS_PARTSCAN, the following
sequence occurs:

  1. disk_force_media_change() sets GD_NEED_PART_SCAN
  2. Uevent suppression is lifted and a KOBJ_CHANGE uevent is sent
  3. loop_global_unlock() releases the lock
  4. loop_reread_partitions() calls bdev_disk_changed() to scan

There is a race between steps 2 and 4: when udev receives the uevent
and opens the device before loop_reread_partitions() runs,
blkdev_get_whole() in bdev.c sees GD_NEED_PART_SCAN set and calls
bdev_disk_changed() for a first scan. Then loop_reread_partitions()
does a second scan. The open_mutex serializes these two scans, but
does not prevent both from running.

The second scan in bdev_disk_changed() drops all partition devices
from the first scan (via blk_drop_partitions()) before re-adding
them, causing partition block devices to briefly disappear. This
breaks any systemd unit with BindsTo= on the partition device: systemd
observes the device going dead, fails the dependent units, and does
not retry them when the device reappears.

Fix this by clearing GD_NEED_PART_SCAN before sending the uevent when
LO_FLAGS_PARTSCAN is set. Since loop_reread_partitions() will perform
the authoritative partition scan, the lazy on-open scan triggered by
GD_NEED_PART_SCAN is redundant. With the flag cleared, udev opening
the device no longer triggers a scan in blkdev_get_whole(), and only
the single explicit scan from loop_reread_partitions() runs.

When LO_FLAGS_PARTSCAN is not set, GD_NEED_PART_SCAN is left as-is
since loop_reread_partitions() will not be called and GD_SUPPRESS_PART_SCAN
remains set, preventing the lazy scan path from triggering anyway.

Signed-off-by: Daan De Meyer <daan@amutable.com>
---
 drivers/block/loop.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 0000913f7efc..4e87d65c45e0 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1085,9 +1085,25 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 	if (part_shift)
 		lo->lo_flags |= LO_FLAGS_PARTSCAN;
 	partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
-	if (partscan)
+	if (partscan) {
 		clear_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
 
+		/*
+		 * disk_force_media_change() above sets GD_NEED_PART_SCAN to
+		 * trigger a partition scan on the next device open. However,
+		 * when LO_FLAGS_PARTSCAN is set, loop_reread_partitions() below
+		 * will do the scan explicitly. If we leave GD_NEED_PART_SCAN
+		 * set, the uevent we're about to send causes udev to open the
+		 * device, which triggers blkdev_get_whole() to scan first, and
+		 * then loop_reread_partitions() scans again. The second scan
+		 * drops all partitions from the first scan before re-adding
+		 * them, causing partition devices to briefly disappear. Clear
+		 * the flag to prevent this since loop_reread_partitions() will
+		 * handle the scan.
+		 */
+		clear_bit(GD_NEED_PART_SCAN, &lo->lo_disk->state);
+	}
+
 	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
 	kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
 
-- 
2.53.0

[PATCH v2] loop: fix partition scan race between udev and loop_reread_partitions()

[Daan De Meyer]

When LOOP_CONFIGURE is called with LO_FLAGS_PARTSCAN, the following
sequence occurs:

  1. disk_force_media_change() sets GD_NEED_PART_SCAN
  2. Uevent suppression is lifted and a KOBJ_CHANGE uevent is sent
  3. loop_global_unlock() releases the lock
  4. loop_reread_partitions() calls bdev_disk_changed() to scan

There is a race between steps 2 and 4: when udev receives the uevent
and opens the device before loop_reread_partitions() runs,
blkdev_get_whole() in bdev.c sees GD_NEED_PART_SCAN set and calls
bdev_disk_changed() for a first scan. Then loop_reread_partitions()
does a second scan. The open_mutex serializes these two scans, but
does not prevent both from running.

The second scan in bdev_disk_changed() drops all partition devices
from the first scan (via blk_drop_partitions()) before re-adding
them, causing partition block devices to briefly disappear. This
breaks any systemd unit with BindsTo= on the partition device: systemd
observes the device going dead, fails the dependent units, and does
not retry them when the device reappears.

Fix this by clearing GD_NEED_PART_SCAN before sending the uevent when
LO_FLAGS_PARTSCAN is set. Since loop_reread_partitions() will perform
the authoritative partition scan, the lazy on-open scan triggered by
GD_NEED_PART_SCAN is redundant. With the flag cleared, udev opening
the device no longer triggers a scan in blkdev_get_whole(), and only
the single explicit scan from loop_reread_partitions() runs.

When LO_FLAGS_PARTSCAN is not set, GD_NEED_PART_SCAN is left as-is
since loop_reread_partitions() will not be called and GD_SUPPRESS_PART_SCAN
remains set, preventing the lazy scan path from triggering anyway.

Fixes: 9f65c489b68d ("loop: raise media_change event")
Signed-off-by: Daan De Meyer <daan@amutable.com>
---
 drivers/block/loop.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 0000913f7efc..4e87d65c45e0 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1085,9 +1085,25 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 	if (part_shift)
 		lo->lo_flags |= LO_FLAGS_PARTSCAN;
 	partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
-	if (partscan)
+	if (partscan) {
 		clear_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
 
+		/*
+		 * disk_force_media_change() above sets GD_NEED_PART_SCAN to
+		 * trigger a partition scan on the next device open. However,
+		 * when LO_FLAGS_PARTSCAN is set, loop_reread_partitions() below
+		 * will do the scan explicitly. If we leave GD_NEED_PART_SCAN
+		 * set, the uevent we're about to send causes udev to open the
+		 * device, which triggers blkdev_get_whole() to scan first, and
+		 * then loop_reread_partitions() scans again. The second scan
+		 * drops all partitions from the first scan before re-adding
+		 * them, causing partition devices to briefly disappear. Clear
+		 * the flag to prevent this since loop_reread_partitions() will
+		 * handle the scan.
+		 */
+		clear_bit(GD_NEED_PART_SCAN, &lo->lo_disk->state);
+	}
+
 	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
 	kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
 
-- 
2.53.0