daniel/danos
1
0
Fork 0
Code Issues 4 Pull Requests Actions Packages Projects 1 Releases Wiki Activity

volume-manager: drop the medium-event dedup — it only ever misfired (S5 review)

Browse Source 
The adversarial S5 review found a real, unrecoverable defect: onMediumEvent
deduped medium_changed events on a module-global last_medium_change compared by
equality against the event's change_count. But change_count is a PER-DRIVER
counter that restarts at 0 in every usb-storage instance — it is bumped +%=1 and
published only on a real medium transition, so within one instance every count
is unique and monotonic and an equality dedup can never legitimately fire.

The global was carried across a driver restart — S5's OWN crash-rebuild path —
so a fresh instance's first eject (count=1) collided with a stale last==1 and was
dropped. removeDevice never ran; the filesystem kept serving I/O against absent
media forever, and nothing else recovered it: onGeometry answers from a cached
block_count so channelAlive stays true, and isDevicePresent stays true (the
device never left the tree). medium_changed is the sole eject oracle there.

The dedup guarded a re-delivery the driver already makes impossible, and its
only observable effect was this bug. Remove it: react to each present-edge
directly. Both branches are idempotent (a freed device stops matching dev.used)
and the poll reconciles, so acting on every genuine edge is safe — and a fresh
driver instance's counter can no longer be mistaken for the previous one's.

Verified: build + bounds green; volume-removal, volume-medium-change and
volume-driver-restart all pass, so both removal paths survive the change. The
driver-restart-then-eject intersection that triggered the collision cannot be
staged in QEMU — the internal driver-kill cannot be ordered against a QMP eject,
and a usb-storage device_add is not re-presented — so the guarantee rests on the
driver's one-publish-per-transition-with-unique-count contract.
This commit is contained in:
Daniel Samson
2026-08-10 05:56:06 +01:00
parent 5dc966838a
commit 061eb7c004
1 changed files with 10 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files
system/services/volume-manager/volume-manager.zig
+10 -4
View File
@@ -555,8 +555,6 @@ fn onNotification(badge: u64) void {
}
}
var last_medium_change: u32 = 0;
fn anyVolumeOn(device_id: u64) bool {
for (&volumes) |*v| if (v.used and v.device_id == device_id) return true;
return false;
@@ -570,10 +568,18 @@ fn anyVolumeOn(device_id: u64) bool {
/// no device, so `absent` retires every adopted device (its volumes unmount and
/// the poll re-adopts the still-present device with its now-empty medium), and
/// `present` frees any empty adopted device so the poll re-probes and remounts it.
///
/// We act on every edge and do NOT dedup on `change_count`. The driver publishes
/// exactly once per transition, each with a unique monotonic count, so a count is
/// never legitimately re-sent within one subscription — an equality dedup could
/// only ever fire spuriously, and it did: `change_count` restarts at 0 in each
/// driver instance (usb-storage.zig), so a global "last count" carried across a
/// driver restart (S5's own crash-rebuild) mistook the fresh instance's first
/// edge for a re-delivery and dropped a real eject, wedging a mount over absent
/// media. Both branches are idempotent (a freed device stops matching `dev.used`)
/// and the poll reconciles, so reacting to each genuine edge is safe.
fn onMediumEvent(payload: []const u8) void {
const event = block.decodeMediumChanged(payload) orelse return;
if (event.change_count == last_medium_change) return; // a coalesced or re-delivered edge
last_medium_change = event.change_count;
if (event.present == 0) {
std.log.info("medium left a storage device; unmounting its volume(s)", .{});
for (&devices) |*dev| {