fix(uring): harden cancel safety and degradation#12
Merged
Conversation
…backlog#1161) The bounded-drain escape hatch did `sem.close(); mem::forget(state)`, which forgot each pending op's oneshot `done` sender without dropping or sending on it. A ReadHandle still awaited after shutdown then polled its receiver forever: the oneshot neither completed nor errored, so the task pended permanently — the one driver-gone path that became an infinite hang instead of an error, breaking the crate's error-never-hang contract. Fail every stranded sender with a driver-gone error before forgetting the pending table. `oneshot::Sender::send` consumes the sender without touching the kernel-owned buffer, so the leak-over-UAF property is preserved while an awaited handle resolves with an error. Add a fault-injection test that awaits a handle across the bailout and asserts it resolves with an error within a bounded window. Co-Authored-By: heihutu <heihutu@gmail.com>
…r (rustfs/backlog#1162) Every non-EBUSY submit() error hit a silent "retry next turn" arm. That is correct for EINTR, but a persistent failure (e.g. EPERM from a seccomp/LSM policy applied after startup) means the queued SQEs are never accepted: their CQEs never arrive, their callers' oneshots never resolve, and — since the driver is not shutting down — the drain deadline never arms. The loop spins at the 50 ms heartbeat forever with no error, log, counter, or fallback. Classify submit errors: EBUSY (reap backpressure) and EINTR (signal) stay transient and reset the counter; any other errno increments a new submit_errors stat, logs once, and after MAX_CONSECUTIVE_SUBMIT_ERRORS consecutive failures transitions the shard to shutdown so the drain and the bounded-drain bailout fail every pending caller with an error — they fall back to the std backend instead of stalling silently. Co-Authored-By: heihutu <heihutu@gmail.com>
…rn (rustfs/backlog#1163) Two same-root wakeup gaps let work sit idle for a full LOOP_HEARTBEAT (50 ms): - A short-read resubmit only did backlog.push_back(sqe) during reap, which runs after the turn's push+submit, so the remainder was not handed to the kernel until the next heartbeat woke the loop. Flush the backlog and submit once more after reap when it is non-empty (idle turns skip it). - ReadHandle::drop sent Msg::Cancel without signaling wake_efd — the Submitted state carried no wake reference — so on an otherwise idle ring the cancel was not even queued for up to 50 ms. Carry the shard's wake_efd in HandleState::Submitted and signal it after the cancel send. Co-Authored-By: heihutu <heihutu@gmail.com>
…ure (rustfs/backlog#1164) start_shard mapped every fallible step into ProbeFailure so try_new can fall back to the std backend — except the spawn, which used .expect(). Thread creation fails with EAGAIN under a cgroup pids-limit or RLIMIT_NPROC, exactly the constrained environments the degradation design exists for, and the panic unwound async disk init/reconnect instead of quietly selecting std. Map the spawn error to ProbeFailure::Setup and propagate with ?. The spawn runs after the probe read already drained, so on failure the ring/eventfds moved into the closure drop cleanly with no SQE in flight. Add a fault-injection seam and a test asserting a forced spawn failure degrades (no panic) and leaves global state intact for a later normal probe. Co-Authored-By: heihutu <heihutu@gmail.com>
… (rustfs/backlog#1165)
Two probe defects that both bite startup:
- drain_probe_cqe bounded only the EINTR retry count, not wall-clock time.
submit_and_wait(1) parks in io_cqring_wait until a CQE or a signal, so a
probe read that never completes (temp_dir on a hung/D-state or NFS device)
blocked async disk init forever. Bound the wait by time: on EXT_ARG kernels
(>= 5.11) pass a Timespec to the enter and treat ETIME as "re-check the
deadline"; on older kernels fall back to the blocking wait, which the deadline
still re-checks between returns. On expiry return an error so the caller's
leak-over-UAF fallback degrades the disk to the std backend.
- probe_and_start_sharded ran probe_real_read on EVERY shard, contradicting its
own doc ("probing happens on the first shard") and doing shards-1 redundant
O_TMPFILE round-trips per disk on every start and renew. Probe only the first
shard; the rest still create a ring and verify NODROP.
Co-Authored-By: heihutu <heihutu@gmail.com>
…ard (rustfs/backlog#1166) Two gaps versus the driver's own documented three-class errno contract: - The reap loop turned every res < 0 into a final error, including EINTR/EAGAIN, which the contract classifies as "transient -> retry, never surface" — and surfacing them discarded a resubmit's already-read prefix. Retry EINTR/EAGAIN by resubmitting the remaining range, bounded per logical read by MAX_TRANSIENT_RETRIES (reset on progress) so a storm cannot spin the driver thread. Streams still finish; ECANCELED and every other errno terminate. - The submit offset guard only rejected offset > i64::MAX, so a request whose block-aligned end (kernel_offset + region_len) crossed i64::MAX still reached the kernel as a runtime EINVAL/EOVERFLOW — the errno class C7 says to pre-empt at submit. Reject it in submit, which also makes every resubmit offset provably <= i64::MAX. Co-Authored-By: heihutu <heihutu@gmail.com>
… dedup (rustfs/backlog#1167) Three diagnostic-consistency defects: - The bounded-drain bailout leaked the ring (still holding a registered cq_efd and in-flight ops) via mem::forget(state), then returned and dropped the cq_efd parameter, closing the fd out from under the still-mapped ring — violating start_shard's documented "cq_efd outlives the ring" invariant. Leak the eventfd alongside the ring on that path. - Overflow was reported as fatal "CQEs lost", but with NODROP (asserted at probe) overflow CQEs are buffered in the kernel overflow list and flushed on the next enter, not lost. Reword the log and StatsSnapshot doc to a backpressure warning. - AsyncCancel SQEs were not deduplicated: a drop-cancel plus a shutdown could queue two cancels per op, pushing completions past 2*entries and making transient overflow reachable. Track queued cancel ids in a HashSet (bounded by the pending table, entries removed on reap) so each op gets at most one cancel. Co-Authored-By: heihutu <heihutu@gmail.com>
…heuristic (rustfs/backlog#1168)
The short-read resubmit loop treated any non-block-multiple O_DIRECT read as
EOF ("the kernel returns block multiples except at the file tail"). That holds
for local block filesystems but not universally: NFS O_DIRECT partial-RPC
completions, FUSE chunking, and signal-split direct I/O can legally return a
non-block-multiple count mid-file. Assuming EOF there silently truncated the
delivered range, violating the whole-range pread contract.
Confirm EOF with the actual file length (fstat, cheap, driver-thread) instead of
inferring it: deliver only when offset+nread is genuinely at/past EOF; for a
mid-file non-multiple short read — which an O_DIRECT read cannot resubmit from a
non-block-aligned offset — surface an error so the integration falls back to the
std backend, preserving correctness rather than truncating. A failed fstat keeps
the conservative EOF assumption. Buffered reads (align == 1) are unaffected.
Co-Authored-By: heihutu <heihutu@gmail.com>
Each shard thread woke every 50 ms and, even on a fully idle turn, drained two eventfds and called ring.submit(), which on a non-SQPOLL ring always issues an io_uring_enter syscall with zero SQEs. Across disks x shards driver threads this is thousands of pointless timer wakeups and syscalls per second, forever. - Skip submit() when the SQ is empty: nothing to submit means nothing to enter. A non-empty SQ (freshly pushed, or residual after EBUSY) still submits. - Adaptive heartbeat: 50 ms only while shutting down or with in-flight ops to reap; otherwise wait up to IDLE_HEARTBEAT (1 s). New work still wakes the loop instantly via wake_efd and completions via cq_efd, so latency is unaffected. Also cap each ring's io-wq bounded worker pool via register_iowq_max_workers so a cold-read burst cannot materialize thousands of PF_IO_WORKER threads against TasksMax/RLIMIT_NPROC — the kernel default is min(sq_entries, 4*nCPU) per ring, and there is one ring per shard per disk. Best-effort: older kernels keep the default. Co-Authored-By: heihutu <heihutu@gmail.com>
…og#1180) The sharded stress test cannot prove cancel routing: its dropped regular-file reads complete on their own, so a cancel routed to the wrong ring (answered -ENOENT) still conserves. Add a test where every op is a blocked-pipe read that can only finish by being canceled, with more ops than shards so every shard owns one. A mis-routed cancel would leave its read stuck and in_flight would never reach 0; the test asserts every orphan is reclaimed and that a cancel, not a timeout, did it. Co-Authored-By: heihutu <heihutu@gmail.com>
… (rustfs/backlog#1181) The changelog claimed "No unreleased changes" although PR #11 (fault injection) landed after the 0.1.0 tag, and every reference link pointed at a nonexistent `v0.1.0` ref (the only tag is `0.1.0`, no `v`). Record the #11 seams and the backlog#1160 audit fixes under [Unreleased], add the [#11] link, and rewrite the compare/tag/blob links to the existing `0.1.0` ref. Co-Authored-By: heihutu <heihutu@gmail.com>
overtrue
changed the title
…s/backlog#1160) The audit-fix series accreted duplicated logic across the driver loop. Fold it into single, named helpers without changing behavior (both run-docker.sh legs still pass, real io_uring included): - Two submit blocks — the main push+submit with full errno classification and a bare `let _ = submit()` for the reap-flush — become one `submit_ring` helper. The reap-flush now shares the same classification instead of silently dropping submit errors. - Three AsyncCancel-enqueue sites (drop-cancel, shutdown, submit-error shutdown) become `queue_cancel`, keeping the per-op dedup in one place. - Two identical resubmit-SQE builds (short-read and transient-errno retry) become `Pending::resubmit_sqe`. - Two push-to-SQ loops become `flush_backlog`. The loop's submit → reap → conditional re-flush order is kept deliberately: a cache-hit read completes inline inside submit()'s io_uring_enter and is reaped the same turn, so reordering would cost the hot path an extra iteration; the second submit only fires when reap queued a resubmit. Co-Authored-By: heihutu <heihutu@gmail.com>
Bump to 0.2.0 for the backlog#1160 cancel-safety / degradation hardening pass and the driver-loop refactor. Promote the CHANGELOG [Unreleased] section to [0.2.0], and update the version and design-note references in Cargo.toml, the README, and the crate docs. Co-Authored-By: heihutu <heihutu@gmail.com>
Refactor the README to match what the code actually exposes: all three read entry points (read_at / read_at_direct / read_current), ReadHandle and StatsSnapshot, and the degradation contract added in the backlog#1160 hardening (classified probe/submit failures, transient-errno retry, fstat-disambiguated EOF). Correct the acceptance-test counts (16 cancel-safety + 5 fault-injection). Remove docs/DESIGN.md and its now-dangling links from the README and crate docs so there is a single source of truth — the per-invariant rationale lives in the module/function docs and the README. The historical CHANGELOG link into the 0.1.0 tag stays valid. Drop the obsolete `exclude = ["docs/**"]` from Cargo.toml. Co-Authored-By: heihutu <heihutu@gmail.com>
houseme
force-pushed
the
fix/uring-audit-1160
branch
from
c8b1ae3 to
8db27ef
Compare
The Linux-only driver failed `clippy -D warnings`: - the O_DIRECT aligned read buffer was bound `mut`, but it is only read (`as_ptr`/`len`) before being moved into the pending table, which owns and mutates it — so the binding needs no `mut` (unused_mut). - the `ReadHandle::drop` cancel path nested `if !finished && cancel_on_drop` inside the `if let Submitted` guard; collapse into one let-chain (collapsible_if). No behavior change. Verified with `cargo clippy --target x86_64-unknown-linux-gnu --all-targets -- -D warnings` (cross-checks the `cfg(target_os = "linux")` driver module from macOS). Co-Authored-By: heihutu <heihutu@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Hardening pass on the
rustfs-uringread driver from the rustfs/backlog#1160 adversarial audit. Every change is on the read path and preserves the cancel-safety ownership model (the pending table owns each buffer/fd until its CQE; a dropped future abandons only the result; the panic abort-barrier and leak-over-UAF escape hatch are untouched). No public API is removed; one field (StatsSnapshot::submit_errors) is added.Each commit maps to one backlog sub-issue.
Fixes
mem::forget-leaked the pending table including each op's oneshot sender, so aReadHandlestill awaited after shutdown hung forever. It now fails every stranded caller with a driver-gone error first (sendconsumes the sender without touching the kernel-owned buffer, so leak-over-UAF is preserved).ring.submit()error (e.g. EPERM from a seccomp policy applied after startup) was retried silently forever, stalling all in-flight reads. Non-transient submit errors are now counted (submit_errors), logged once, and after a bounded run shut the shard down so callers fall back.ProbeFailureinstead of panicking out of async disk init.i64::MAX.fstatinstead of assumed to be EOF, so a stacked filesystem (NFS/FUSE) cannot cause a silent truncation.io_uring_enteron an empty SQ and uses a longer heartbeat when idle; each ring caps its io-wq bounded workers viaregister_iowq_max_workers.Plus #1180 (a deterministic sharded cancel-routing test) and #1181 (changelog + fixed
v0.1.0reference links).Testing
run-docker.shboth legs pass: leg 1 (io_uring blocked by an explicit seccomp profile) degrades gracefully with every test skipping; leg 2 (seccomp=unconfined) runs real io_uring against the host kernel with no skips — 15 cancel-safety tests and the fault-injection suite (including the two new tests for #1161 and #1164) all pass.Release coupling
rustfs/rustfspinsrustfs-uring = "0.1.0"from crates.io, so these fixes reach production only once this lands and a new version is published; the companion rustfs PR then bumps the dependency. See #1181.🤖 Generated with Claude Code