Skip to contentSkip to conversation

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

PATCH Superseded Daan De Meyer 2 participants 3 messages Mar 30 – Mar 31 Block layer

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 loop: fix partition scan race between udev and loop_reread_partitions()

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.

---
 drivers/block/loop.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)
drivers/block/loop.c +17 -1
index 0000913f7efc..4e87d65c45e0 100644
@@ -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);
 
Signature · 1 line
2.53.0
#1 lore raw

PATCH v2 1/1 loop: fix partition scan race between udev and loop_reread_partitions()

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")
---
 drivers/block/loop.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)
drivers/block/loop.c +17 -1
index 0000913f7efc..4e87d65c45e0 100644
@@ -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);
 
Signature · 1 line
2.53.0
#2 lore raw
Quoted 6 lines · Daan De Meyer

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.

Setting the flag and instantly clearing them seems like a bad idea. So add a variant that doesn't set it instead, or audit the callers for who even needs the flag set. floppy doesn't because it doesn't support partitions, and the other callers in loop also look suspicious.

Also can you wire up a test for this in blktests?