From 7a1ad760f05aa4e2ad8875bec50f1f0ba4526b23 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 8 Oct 2026 10:13:20 +0200 Subject: [PATCH 1/3] Record chdir's VFS/process-data lock-order inversion Carried from the kernel audit's branch (pull request 754), which traced it without executing it, so the record lands with the commit that closes it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RvnWQFcMuGqTHYhvSnTe8A --- ...cess-data-lock-open-takes-the-other-way.md | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 issues/chdir-holds-the-vfs-lock-into-the-process-data-lock-open-takes-the-other-way.md diff --git a/issues/chdir-holds-the-vfs-lock-into-the-process-data-lock-open-takes-the-other-way.md b/issues/chdir-holds-the-vfs-lock-into-the-process-data-lock-open-takes-the-other-way.md new file mode 100644 index 00000000000..2dfed975732 --- /dev/null +++ b/issues/chdir-holds-the-vfs-lock-into-the-process-data-lock-open-takes-the-other-way.md @@ -0,0 +1,43 @@ +--- +status: open +kind: defect +opened: 2026-10-08 +--- + +# A successful chdir reverses the open path's VFS/process-data lock order + +`sys_chdir` (`kernel/src/syscall/fs.rs:84`) is written as + +```rust +match vfs::lock().cd(&cwd, path) { + Ok(new_cwd) => { process::with_process_data(|d| d.cwd = new_cwd); 0 } + Err(e) => e.to_u64(), +} +``` + +The `vfs::lock()` guard is a temporary in the match scrutinee. Under the kernel +crate's edition (2021, `kernel/Cargo.toml`), a scrutinee temporary lives to the +end of the whole `match`, so the VFS lock is still held through the `Ok` arm +while `with_process_data` takes the process-data lock +(`kernel/src/process.rs:853`). Order: **VFS then process-data.** + +`sys_open` (`kernel/src/syscall/fs.rs:35`) takes process-data via +`with_process_data` and calls `ops::open`, which takes `crate::vfs::lock()` +inside that closure (`kernel/src/object/ops.rs`). Order: **process-data then +VFS.** + +Two threads of one process (process-data is a per-process `Arc` shared by its +threads; VFS is global) running `SYS_CHDIR` and `SYS_OPEN` on two CPUs can each +hold the lock the other waits for. The ticket spinlock does not break the cycle; +its spin ceiling panics the kernel and unrelated VFS work stalls behind it. This +is a distinct inversion from the one recorded in +`issues/a-user-copy-demand-pages-under-whatever-its-caller-holds.md`. + +**Exit**: `SYS_CHDIR` and `SYS_OPEN` can run concurrently in one process with no +lock-order inversion — `chdir` does not hold the VFS guard while it takes +process-data (the guard is dropped before the `Ok` arm takes the second lock). +A test that stages chdir past a successful lookup with its guard still alive, +then drives open to VFS acquisition under process-data, completes both without +deadlock. **Traced, not executed.** No model compiles these two syscalls, and staging the +window needs an actuator the kernel does not have: one that holds `sys_chdir` +in its `Ok` arm until a sibling releases it. From 96a25fe4bb9d3300bf62b1248d578d8621ef598f Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 8 Oct 2026 10:16:31 +0200 Subject: [PATCH 2/3] chdir drops the VFS guard before taking process-data, matching open's order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sys_chdir` matched on `vfs::lock().cd(...)`, whose guard — a scrutinee temporary — lived to the end of the `match`, so the `Ok` arm took the process-data lock while the VFS lock was still held: order VFS then process-data. `sys_open` takes process-data first and `ops::open` takes VFS inside it: order process-data then VFS. Two threads of one process (process data is a per-process Arc shared by its threads; VFS is global) running chdir and open on two CPUs could each hold the lock the other waited for; the ticket spinlock does not break the cycle and its spin ceiling panics the kernel. chdir now resolves under the VFS guard in a scope, drops it, then takes process-data alone — the order open takes. The guard is a named local, not a scrutinee temporary, so it is dropped where the block closes rather than at the end of the match. Closes the carried issue; deletes its file. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RvnWQFcMuGqTHYhvSnTe8A --- ...cess-data-lock-open-takes-the-other-way.md | 43 ------------------- kernel/src/syscall/fs.rs | 17 +++++--- 2 files changed, 11 insertions(+), 49 deletions(-) delete mode 100644 issues/chdir-holds-the-vfs-lock-into-the-process-data-lock-open-takes-the-other-way.md diff --git a/issues/chdir-holds-the-vfs-lock-into-the-process-data-lock-open-takes-the-other-way.md b/issues/chdir-holds-the-vfs-lock-into-the-process-data-lock-open-takes-the-other-way.md deleted file mode 100644 index 2dfed975732..00000000000 --- a/issues/chdir-holds-the-vfs-lock-into-the-process-data-lock-open-takes-the-other-way.md +++ /dev/null @@ -1,43 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-10-08 ---- - -# A successful chdir reverses the open path's VFS/process-data lock order - -`sys_chdir` (`kernel/src/syscall/fs.rs:84`) is written as - -```rust -match vfs::lock().cd(&cwd, path) { - Ok(new_cwd) => { process::with_process_data(|d| d.cwd = new_cwd); 0 } - Err(e) => e.to_u64(), -} -``` - -The `vfs::lock()` guard is a temporary in the match scrutinee. Under the kernel -crate's edition (2021, `kernel/Cargo.toml`), a scrutinee temporary lives to the -end of the whole `match`, so the VFS lock is still held through the `Ok` arm -while `with_process_data` takes the process-data lock -(`kernel/src/process.rs:853`). Order: **VFS then process-data.** - -`sys_open` (`kernel/src/syscall/fs.rs:35`) takes process-data via -`with_process_data` and calls `ops::open`, which takes `crate::vfs::lock()` -inside that closure (`kernel/src/object/ops.rs`). Order: **process-data then -VFS.** - -Two threads of one process (process-data is a per-process `Arc` shared by its -threads; VFS is global) running `SYS_CHDIR` and `SYS_OPEN` on two CPUs can each -hold the lock the other waits for. The ticket spinlock does not break the cycle; -its spin ceiling panics the kernel and unrelated VFS work stalls behind it. This -is a distinct inversion from the one recorded in -`issues/a-user-copy-demand-pages-under-whatever-its-caller-holds.md`. - -**Exit**: `SYS_CHDIR` and `SYS_OPEN` can run concurrently in one process with no -lock-order inversion — `chdir` does not hold the VFS guard while it takes -process-data (the guard is dropped before the `Ok` arm takes the second lock). -A test that stages chdir past a successful lookup with its guard still alive, -then drives open to VFS acquisition under process-data, completes both without -deadlock. **Traced, not executed.** No model compiles these two syscalls, and staging the -window needs an actuator the kernel does not have: one that holds `sys_chdir` -in its `Ok` arm until a sibling releases it. diff --git a/kernel/src/syscall/fs.rs b/kernel/src/syscall/fs.rs index d2d195d2aae..8482e9f8202 100644 --- a/kernel/src/syscall/fs.rs +++ b/kernel/src/syscall/fs.rs @@ -83,13 +83,18 @@ pub(super) fn sys_delete(path: &str) -> u64 { pub(super) fn sys_chdir(path: &str) -> u64 { let cwd = process::with_process_data(|d| d.cwd.clone()); - match vfs::lock().cd(&cwd, path) { - Ok(new_cwd) => { - process::with_process_data(|d| d.cwd = new_cwd); - 0 + // Resolve under the VFS guard, then drop it before taking process-data: + // `sys_open` nests VFS inside process-data, so chdir must never hold VFS + // into it, or two threads of one process deadlock the ticket spinlock. + let new_cwd = { + let mut vfs = vfs::lock(); + match vfs.cd(&cwd, path) { + Ok(new_cwd) => new_cwd, + Err(e) => return e.to_u64(), } - Err(e) => e.to_u64(), - } + }; + process::with_process_data(|d| d.cwd = new_cwd); + 0 } /// A spawned child's working directory, judged as `SYS_CHDIR` judges a path but From 54e8a0791548f31c0a95d5ba64a1c07a1dd4b233 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 8 Oct 2026 10:45:54 +0200 Subject: [PATCH 3/3] Answer review round 1: chdir takes the tree's form of the fix, and the lint's measurement is recorded `sys_chdir` binds `vfs::lock().cd(..)` to a `let` and matches on the value, the form `sys_open` and the loader already use: the guard ends with the statement, and the original `match` stands. The block, the named guard and the early return were a second idiom for the same thing. The scaffolded `significant_drop_in_scrutinee` run that served as this fix's oracle is recorded in the clippy track, with its counts (6 sites on main, 5 with chdir fixed, x86_64 kernel, default features, `LockGuard` alone marked) and what it does not see. The user-copy finding names the VFS lock beside the three whose order nothing checks. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RvnWQFcMuGqTHYhvSnTe8A --- ...d-pages-under-whatever-its-caller-holds.md | 6 ++++++ ...clippy-stage-two-is-lints-one-at-a-time.md | 6 ++++++ kernel/src/syscall/fs.rs | 19 ++++++++----------- 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/issues/a-user-copy-demand-pages-under-whatever-its-caller-holds.md b/issues/a-user-copy-demand-pages-under-whatever-its-caller-holds.md index dec3e860cbd..89958b50987 100644 --- a/issues/a-user-copy-demand-pages-under-whatever-its-caller-holds.md +++ b/issues/a-user-copy-demand-pages-under-whatever-its-caller-holds.md @@ -16,6 +16,12 @@ lock on the first unmapped window: it spins, and the kernel panics "DEADLOCK at …" (`Lock::lock`, `kernel/src/sync.rs`). Nothing in the tree refuses that order: no lock-order check covers these locks. +The global VFS lock (`vfs::lock()`) is a fourth nothing orders. `ops::open` +takes it under `process_data`, and `sys_chdir` took `process_data` under it +until its guard was made to end before the store; that no holder of the VFS +guard takes `process_data` is held by reading alone. A checked lock level +ranks it with the three. + **Exit:** the order is refused by something that fails (a type or a checked lock level), or every caller is shown to hold none of the three and the invariant is one line in `user_ptr.rs`'s header. diff --git a/issues/clippy-stage-two-is-lints-one-at-a-time.md b/issues/clippy-stage-two-is-lints-one-at-a-time.md index 2469ae0068f..7cd302c5719 100644 --- a/issues/clippy-stage-two-is-lints-one-at-a-time.md +++ b/issues/clippy-stage-two-is-lints-one-at-a-time.md @@ -124,6 +124,12 @@ before adopting: "this device's memory has one owner, enforced by the driver, not by the type." Two of three already carry a `# Safety`/`SAFETY:` comment saying so. +## Measured, not decided + +| lint | count | what was measured, and what it does not see | +|---|---:|---| +| `significant_drop_in_scrutinee` | 6, 1 a defect | Silent in the gate: it reports only a guard marked `#[clippy::has_significant_drop]`, and no kernel guard is. With `sync::LockGuard` alone marked, the kernel under `x86_64-unknown-none` with default features reported 6 sites, and 5 once `sys_chdir` stopped holding the VFS guard into `process_data` — the one defect. The 5 left (`arch/x86_64/idt/mod.rs`'s `for` over `IDT.lock()`, `loader/mod.rs`'s `/system/lib` fallback, `user_ptr::translate_user`, `sys_readdir`, `sys_dlopen`) take no second lock under the guard, and adopting the lint rewrites each. Unmeasured: AArch64, the actuator and instrument shapes, and the guards the measurement did not mark (`SleepGuard`, `block`'s `Locked`, the serial guard). It cannot join `ADOPTED`, which `src/clippy.rs` splices into the host workspace's shapes too, where std's `MutexGuard` is already marked: it would be a per-shape `-W`, as `undocumented_unsafe_blocks` is. **It sees one spelling, not the order**: an `if let Some(x) = L.lock().as_mut()` whose value borrows from the guard is in the tree and in neither report (`file_cache::touch`, `scheduler::remove_vruntime`, `inbox`'s `answer`), and so is any inversion through a named guard, the common shape. | + ## `undocumented_unsafe_blocks`, per area | area | unsafe blocks | status | diff --git a/kernel/src/syscall/fs.rs b/kernel/src/syscall/fs.rs index 8482e9f8202..26854878081 100644 --- a/kernel/src/syscall/fs.rs +++ b/kernel/src/syscall/fs.rs @@ -83,18 +83,15 @@ pub(super) fn sys_delete(path: &str) -> u64 { pub(super) fn sys_chdir(path: &str) -> u64 { let cwd = process::with_process_data(|d| d.cwd.clone()); - // Resolve under the VFS guard, then drop it before taking process-data: - // `sys_open` nests VFS inside process-data, so chdir must never hold VFS - // into it, or two threads of one process deadlock the ticket spinlock. - let new_cwd = { - let mut vfs = vfs::lock(); - match vfs.cd(&cwd, path) { - Ok(new_cwd) => new_cwd, - Err(e) => return e.to_u64(), + // The VFS guard ends with this statement: `sys_open` takes VFS under process-data. + let resolved = vfs::lock().cd(&cwd, path); + match resolved { + Ok(new_cwd) => { + process::with_process_data(|d| d.cwd = new_cwd); + 0 } - }; - process::with_process_data(|d| d.cwd = new_cwd); - 0 + Err(e) => e.to_u64(), + } } /// A spawned child's working directory, judged as `SYS_CHDIR` judges a path but