diff --git a/drivers/cube/cubelinux_store.rs b/drivers/cube/cubelinux_store.rs index 18de5b2d9..4f7ab1274 100644 --- a/drivers/cube/cubelinux_store.rs +++ b/drivers/cube/cubelinux_store.rs @@ -348,6 +348,29 @@ kernel::sync::global_lock! { unsafe(uninit) static STORE_FILE: Mutex> = None; } +kernel::sync::global_lock! { + /// Serialises the store's read-modify-write operations. + /// + /// [`STORE_FILE`]'s mutex guards the file *handle*, not the store: it is taken, the handle is + /// fetched or cached, and it is released before the caller does anything with it. Sharing a + /// `struct file *` is safe, so that is all the handle needs — and it is not enough for the + /// store. An append is a read-modify-write of the control block: read `log_used`, write the + /// entry at that offset, then write a control block counting it. Writers that both read + /// `log_used = 0` write their entries to the same offset and commit the same count, so one + /// entry survives, the rest are discarded, and every caller is told `ok`. + /// + /// Measured with this lock absent: `verify-file-store.sh MODE=race`, twelve concurrent puts, + /// 12 × 68 bytes of log expected — a single 68-byte entry landed and all twelve reported + /// success. The same shape is on the box, where the store has several writers. + /// + /// It is taken by the *entry points* only — [`append`] and [`fold_now`] — and their inner + /// bodies assume it is already held. `append` folds and then calls itself, and `fold_now` is + /// reachable both from inside `append` and on its own, so a lock taken at the top of either + /// body would re-enter itself and deadlock: a kernel mutex is not reentrant. + /// SAFETY: Initialized (to None) before first use. + unsafe(uninit) static STORE_OP: Mutex<()> = (); +} + /// The store device's file, opened on first use and cached for the boot. O_RDWR, because the one /// handle serves both the reads and the log-append / checkpoint writes. fn store_file() -> Result<*mut bindings::file> { @@ -1257,10 +1280,17 @@ fn older_copy(generation: u64) -> usize { /// survives a power cut; one it has not may or may not, which is exactly the boundary an /// acknowledgement is supposed to mark. /// +/// Append a mutation to the log. **Requires [`STORE_OP`] to be held by the caller.** +/// /// On a store device the entry goes after the log bytes already in use, and the control /// block's count is raised afterwards — entry first, count second, so a crash between them /// loses an *unacknowledged* mutation rather than counting one that is not there. A count /// that ran ahead would make replay read past valid data; a count that lags only forgets. +/// +/// That read-then-write pair is what must not interleave — and the lock has to be taken by the +/// caller, *before* it reads the layout this writes against. Taking it here is too late: writers +/// that each read `log_used = 0` before queueing on a lock inside this function still append at +/// the same offset. Measured with the lock here: one 68-byte entry and twelve `ok`s. fn append( image: &[u8], layout: &Layout, @@ -1313,6 +1343,7 @@ fn append( // to, so it keeps writing the log it has. Some(WAL_VERSION) if used > 0 => { if layout.control.is_some() { + // [`STORE_OP`] is held by the op that reached here, so these are plain calls. fold_now(image, layout)?; let (device, layout) = device_and_layout()?; return append(&device, &layout, m, op); @@ -1529,6 +1560,8 @@ fn checkpoint(ctl: &Control, merged: &mut Merged) -> Result { /// Fold the current log into the image — the shared body of `sync` and the append-time /// migration. After it, the image is the pinned v4 shape and the log is empty, so the next /// append starts a fresh v2 log. +/// +/// **Requires [`STORE_OP`] to be held by the caller.** fn fold_now(image: &[u8], layout: &Layout) -> Result<(), Error> { let ctl = match layout.control { Some(c) => c, @@ -1726,6 +1759,9 @@ pub unsafe extern "C" fn cubelinux_kernel_put( len: usize, flags: u16, ) -> i32 { + // The lock spans the whole op — reading the layout and committing the count are one + // read-modify-write, and `ensure_boot_record` below writes too. + let _op = STORE_OP.lock(); ensure_boot_record(); let (sp, key) = unsafe { coord_key(space, x, y, z) }; let mut bytes = KVVec::::new(); @@ -4122,6 +4158,7 @@ pub unsafe extern "C" fn cubelinux_kernel_spaces(cursor: u64, space_out: *mut u8 /// `space` must point to 32 readable bytes. #[unsafe(no_mangle)] pub unsafe extern "C" fn cubelinux_kernel_del(space: *const u8, x: u64, y: u64, z: u64) -> i32 { + let _op = STORE_OP.lock(); ensure_boot_record(); let (sp, key) = unsafe { coord_key(space, x, y, z) }; let mutation = Mutation { @@ -4145,6 +4182,7 @@ pub unsafe extern "C" fn cubelinux_kernel_del(space: *const u8, x: u64, y: u64, /// `CUBE_OP_SYNC`: fold the log into the image. #[unsafe(no_mangle)] pub extern "C" fn cubelinux_kernel_sync() -> i32 { + let _op = STORE_OP.lock(); ensure_boot_record(); let (device, layout) = match device_and_layout() { Ok(pair) => pair, @@ -4185,6 +4223,8 @@ impl kernel::InPlaceModule for CubeStoreModule { // a journal that simply stops. // SAFETY: called exactly once, in the module initializer, before anything can take it. unsafe { STORE_FILE.init() }; + // SAFETY: called exactly once, in the module initializer, before anything can take it. + unsafe { STORE_OP.init() }; try_pin_init!(Self { _miscdev <- MiscDeviceRegistration::register(MiscDeviceOptions { name: c_str!("cubelinux"), @@ -4234,6 +4274,8 @@ impl MiscDevice for CubeStore { } // A write to the store device is the kernel taking the write path, so this is one of the // places a boot gets recorded — the same hook the syscall's write operations call. + // The lock spans the op: everything below reads the layout and commits against it. + let _op = STORE_OP.lock(); ensure_boot_record(); let me = kiocb.file();