mirror of
https://github.com/freemkv/libfreemkv.git
synced 2026-10-07 18:13:10 +00:00
scsi: route raw_command through checked_cdb_len; add the fd hand-off release edge (#16)
raw_command clamped CDBs with cdb.len().min(16) instead of routing through the shared checked_cdb_len guard, so an over-length or empty CDB became a different command rather than a rejected one; it now returns the crate Result so a rejected CDB is distinguishable from an I/O failure. The recovery/Drop fd hand-off lacked a release edge: the teardown swap was not a release op and the recovery CAS was not an acquire load, so a recovery thread publishing its fd after Drop drained the slot could miss dead==true and leak the descriptor. Both sides are now AcqRel, with the execute() drain left at Acquire (sound via the release-sequence rule). A loom model (cfg(loom) dev-dep, its own CI job) provides the regression coverage cargo test cannot. Module docs for fd_handoff and timeline now resolve in their own scope, and item docs stay within comment-guard's caps. Fixes #16.
This commit is contained in:
Rob Ekl
@@ -136,6 +136,49 @@ jobs:
|
||||
- run: cargo test --tests
|
||||
working-directory: libfreemkv
|
||||
|
||||
# The memory orderings in `scsi::fd_handoff` — the fd hand-off between a
|
||||
# recovery thread and `SgIoTransport::drop` — cannot be checked by `cargo
|
||||
# test`: they compile to the same instructions on x86_64, so the WRONG ones
|
||||
# pass every run. loom enumerates the executions the C++20 model permits and
|
||||
# reports the leaked descriptor directly, which makes it the only regression
|
||||
# coverage that part of the module has. If this job stops running, that
|
||||
# coverage disappears silently behind a green build.
|
||||
#
|
||||
# Its own job rather than a step in `test` because `RUSTFLAGS` is part of the
|
||||
# rust-cache key: folding it into `test` costs that job a full cache miss on
|
||||
# every run. It takes the same sibling checkout as the jobs above even though
|
||||
# `--no-default-features` drops freemkv-unlock from the BUILD — `--features`
|
||||
# does not exempt an optional path dependency from manifest RESOLUTION, so
|
||||
# without the sibling this job would fail the moment the branch tip carries
|
||||
# the path-dep form of that dependency rather than a git tag.
|
||||
loom:
|
||||
needs: resolve-msrv
|
||||
runs-on: ubuntu-latest
|
||||
env:
|
||||
RUSTFLAGS: --cfg loom
|
||||
steps:
|
||||
- uses: actions/checkout@v7
|
||||
with:
|
||||
path: libfreemkv
|
||||
- uses: actions/checkout@v7
|
||||
with:
|
||||
repository: freemkv/freemkv-unlock
|
||||
ref: "${{ github.ref_name == 'qa' && 'qa' || 'dev' }}"
|
||||
path: freemkv-unlock
|
||||
- uses: dtolnay/rust-toolchain@master
|
||||
with:
|
||||
toolchain: ${{ needs.resolve-msrv.outputs.v }}
|
||||
components: clippy
|
||||
- uses: Swatinem/rust-cache@v2
|
||||
with:
|
||||
workspaces: libfreemkv
|
||||
- run: cargo test --lib --no-default-features --features scsi fd_handoff
|
||||
working-directory: libfreemkv
|
||||
# `lint` runs clippy without `--cfg loom`, so this is the only place the
|
||||
# model code is linted at all.
|
||||
- run: cargo clippy --lib --no-default-features --features scsi --profile test -- -D warnings
|
||||
working-directory: libfreemkv
|
||||
|
||||
deny:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
|
||||
@@ -2,6 +2,15 @@
|
||||
|
||||
## [1.6.15] — unreleased
|
||||
|
||||
### Fixed
|
||||
|
||||
- Linux: `SgIoTransport::raw_command` clamped an over-length CDB with `cdb.len().min(16)` instead of routing it through the shared `checked_cdb_len` guard that `execute()` uses. Under SPC-4 a CDB's length is fixed by its opcode, so truncating one does not shorten the command — it sends a descriptor the drive will read as something else. An empty CDB was likewise unguarded, and went to the sg driver as a zero-length command descriptor. Unreachable — `raw_command` is private and its one caller passes a fixed 6-byte ALLOW MEDIUM REMOVAL — but it was the one transport path that did the thing the guard exists to prevent.
|
||||
- Linux: the fd hand-off between a background recovery thread and `SgIoTransport::drop` had no release edge, so a recovery thread that published its fd after teardown had drained the slot was not guaranteed to observe `dead == true` and could leave the descriptor unclosed. `Drop`'s claiming `swap` was `Acquire` (a relaxed store, heading no release sequence) and the recovery thread's `compare_exchange` was `Release` (a relaxed load, acquiring nothing) — neither side alone was enough, and upgrading only one still leaks. Both are now `AcqRel`. Worst case was a leaked file descriptor on a transport already being torn down.
|
||||
|
||||
### Changed
|
||||
|
||||
- The Linux recovery-thread fd hand-off moved to `scsi::fd_handoff`, where it is compiled and unit-tested on every platform (`linux.rs` is built on one host only) and model-checked under `loom`. No public API change.
|
||||
|
||||
### Security
|
||||
|
||||
- Fixed a soundness bug (GHSA-j8ww-f5fg-9pmh, low severity): `PrefetchedSectorSource::into_channels` handed out a `Sender<Vec<u8>>` recycle channel whose buffers the producer re-exposed with `unsafe { Vec::set_len }` guarded only by capacity, so a downstream caller recycling a `Vec::with_capacity(n)` (len 0) could drive `set_len` over uninitialized memory — undefined behaviour reachable from safe code. The producer now uses `Vec::resize`, removing the `unsafe`; buffer pooling (the real cross-thread alloc/free win) is unchanged. No in-tree caller triggered it.
|
||||
|
||||
Generated
+182
@@ -13,6 +13,15 @@ dependencies = [
|
||||
"cpufeatures 0.3.1",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "aho-corasick"
|
||||
version = "1.1.5"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "c982642fa9e8606056828ee9a8505737230110bb1099153c79efe865c59d12ba"
|
||||
dependencies = [
|
||||
"memchr",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "autocfg"
|
||||
version = "1.5.1"
|
||||
@@ -55,6 +64,16 @@ version = "3.20.3"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "72f5acc6cb2ba439de613abc23857ec3d78374d8ed5ac84e9d11336e87da8649"
|
||||
|
||||
[[package]]
|
||||
name = "cc"
|
||||
version = "1.4.4"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "0ad534f4357a5264cce5019c989cf66a4f0dc4e0d1b1d15f8aacec0ff7360273"
|
||||
dependencies = [
|
||||
"find-msvc-tools",
|
||||
"shlex",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "cfg-if"
|
||||
version = "1.0.4"
|
||||
@@ -223,6 +242,12 @@ version = "2.5.0"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "da7c62ceae207dd37ea5b845da6a0696c799f85e97da1ab5b7910be3c1c80223"
|
||||
|
||||
[[package]]
|
||||
name = "find-msvc-tools"
|
||||
version = "0.1.11"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "d45db016d36b838f563236e9193d0ee6ce38f3f68b6c94e914b4929c96bbb890"
|
||||
|
||||
[[package]]
|
||||
name = "flate2"
|
||||
version = "1.1.10"
|
||||
@@ -248,6 +273,21 @@ dependencies = [
|
||||
"tracing",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "generator"
|
||||
version = "0.8.9"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "b3b854b0e584ead1a33f18b2fcad7cf7be18b3875c78816b753639aa501513ae"
|
||||
dependencies = [
|
||||
"cc",
|
||||
"cfg-if",
|
||||
"libc",
|
||||
"log",
|
||||
"rustversion",
|
||||
"windows-link",
|
||||
"windows-result",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "generic-array"
|
||||
version = "0.14.7"
|
||||
@@ -310,6 +350,12 @@ version = "1.0.18"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "8f42a60cbdf9a97f5d2305f08a87dc4e09308d1276d28c869c684d7777685682"
|
||||
|
||||
[[package]]
|
||||
name = "lazy_static"
|
||||
version = "1.5.0"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "bbd2bcb4c963f2ddae06a2efc7e9f3591312473c50c6685e1f298068316e66fe"
|
||||
|
||||
[[package]]
|
||||
name = "libc"
|
||||
version = "0.2.189"
|
||||
@@ -325,6 +371,7 @@ dependencies = [
|
||||
"crossbeam-channel",
|
||||
"freemkv-unlock",
|
||||
"libc",
|
||||
"loom",
|
||||
"memchr",
|
||||
"rand",
|
||||
"rayon",
|
||||
@@ -349,12 +396,43 @@ version = "0.4.34"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "f9f8bd3e56ce4dfc153cf470fffbfa98c7620958b312ca5c3a4b8d5181fd13c6"
|
||||
|
||||
[[package]]
|
||||
name = "loom"
|
||||
version = "0.7.2"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "419e0dc8046cb947daa77eb95ae174acfbddb7673b4151f56d1eed8e93fbfaca"
|
||||
dependencies = [
|
||||
"cfg-if",
|
||||
"generator",
|
||||
"scoped-tls",
|
||||
"tracing",
|
||||
"tracing-subscriber",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "matchers"
|
||||
version = "0.2.0"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "d1525a2a28c7f4fa0fc98bb91ae755d1e2d1505079e05539e35bc876b5d65ae9"
|
||||
dependencies = [
|
||||
"regex-automata",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "memchr"
|
||||
version = "2.8.3"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "cf8baf1c55e62ffcace7a9f06f4bd9cd3f0c4beb022d3b367256b91b87513d98"
|
||||
|
||||
[[package]]
|
||||
name = "nu-ansi-term"
|
||||
version = "0.50.3"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5"
|
||||
dependencies = [
|
||||
"windows-sys",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "num-bigint"
|
||||
version = "0.5.1"
|
||||
@@ -456,6 +534,23 @@ dependencies = [
|
||||
"crossbeam-utils",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "regex-automata"
|
||||
version = "0.4.18"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "ad8553b9b26413251cbf30e620595c7a41b3887f03da04579c0e6b0d6a06b4b2"
|
||||
dependencies = [
|
||||
"aho-corasick",
|
||||
"memchr",
|
||||
"regex-syntax",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "regex-syntax"
|
||||
version = "0.8.11"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "d6f6ff9a378485b298a5286656da665ba74413d36db0979633275d2e708145d4"
|
||||
|
||||
[[package]]
|
||||
name = "roxmltree"
|
||||
version = "0.20.0"
|
||||
@@ -475,6 +570,18 @@ dependencies = [
|
||||
"windows-sys",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "rustversion"
|
||||
version = "1.0.23"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "cf54715a573b99ac80df0bc206da022bcd442c974952c7b9720069370852e21f"
|
||||
|
||||
[[package]]
|
||||
name = "scoped-tls"
|
||||
version = "1.0.1"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "e1cf6437eb19a8f4a6cc0f7dca544973b0b78843adbfeb3683d1a94a0024a294"
|
||||
|
||||
[[package]]
|
||||
name = "serde"
|
||||
version = "1.0.229"
|
||||
@@ -540,12 +647,33 @@ dependencies = [
|
||||
"digest 0.11.3",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "sharded-slab"
|
||||
version = "0.1.7"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "f40ca3c46823713e0d4209592e8d6e826aa57e928f09752619fc696c499637f6"
|
||||
dependencies = [
|
||||
"lazy_static",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "shlex"
|
||||
version = "2.0.1"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "f8fadd59c855ef2080decdef8ff161eb6661b86933c9d82e5ba29dc602a55aba"
|
||||
|
||||
[[package]]
|
||||
name = "simd-adler32"
|
||||
version = "0.3.10"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "3a219298ac11a56ea9a6d2120044824d6f01aeb034955e7af7bc16858527deea"
|
||||
|
||||
[[package]]
|
||||
name = "smallvec"
|
||||
version = "1.16.0"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "b9be42f50aa861c555654aa3a37f52f4b1074bacf4e48fe0ef7fa584e80f1f0f"
|
||||
|
||||
[[package]]
|
||||
name = "syn"
|
||||
version = "2.0.119"
|
||||
@@ -581,6 +709,15 @@ dependencies = [
|
||||
"windows-sys",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "thread_local"
|
||||
version = "1.1.10"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "1ad99c4c6d32803332c548b1af0540b357b3f5fc0be8f6c6bfe8b2e6ae784070"
|
||||
dependencies = [
|
||||
"cfg-if",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "tracing"
|
||||
version = "0.1.44"
|
||||
@@ -610,6 +747,36 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "db97caf9d906fbde555dd62fa95ddba9eecfd14cb388e4f491a66d74cd5fb79a"
|
||||
dependencies = [
|
||||
"once_cell",
|
||||
"valuable",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "tracing-log"
|
||||
version = "0.2.0"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "ee855f1f400bd0e5c02d150ae5de3840039a3f54b025156404e34c23c03f47c3"
|
||||
dependencies = [
|
||||
"log",
|
||||
"once_cell",
|
||||
"tracing-core",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "tracing-subscriber"
|
||||
version = "0.3.23"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "cb7f578e5945fb242538965c2d0b04418d38ec25c79d160cd279bf0731c8d319"
|
||||
dependencies = [
|
||||
"matchers",
|
||||
"nu-ansi-term",
|
||||
"once_cell",
|
||||
"regex-automata",
|
||||
"sharded-slab",
|
||||
"smallvec",
|
||||
"thread_local",
|
||||
"tracing",
|
||||
"tracing-core",
|
||||
"tracing-log",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
@@ -630,6 +797,12 @@ version = "1.0.24"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "e6e4313cd5fcd3dad5cafa179702e2b244f760991f45397d14d4ebf38247da75"
|
||||
|
||||
[[package]]
|
||||
name = "valuable"
|
||||
version = "0.1.1"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "ba73ea9cf16a25df0c8caa16c51acb937d5712a8429db78a3ee29d5dcacd3a65"
|
||||
|
||||
[[package]]
|
||||
name = "version_check"
|
||||
version = "0.9.5"
|
||||
@@ -642,6 +815,15 @@ version = "0.2.1"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d0a4fe5"
|
||||
|
||||
[[package]]
|
||||
name = "windows-result"
|
||||
version = "0.4.1"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "7781fa89eaf60850ac3d2da7af8e5242a5ea78d1a11c49bf2910bb5a73853eb5"
|
||||
dependencies = [
|
||||
"windows-link",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "windows-sys"
|
||||
version = "0.61.2"
|
||||
|
||||
+25
@@ -101,10 +101,35 @@ libc = "0.2"
|
||||
[dev-dependencies]
|
||||
tempfile = "3"
|
||||
|
||||
# Permutation model checker for the `scsi::fd_handoff` atomics — the fd
|
||||
# hand-off between a Linux recovery thread and `SgIoTransport::drop`. A normal
|
||||
# test cannot see a missing release edge there: the orderings compile to the
|
||||
# same instructions on x86_64, so the wrong ones pass every time. loom
|
||||
# enumerates the executions the C++20 model permits and reports the leaked
|
||||
# descriptor directly.
|
||||
#
|
||||
# Behind `cfg(loom)`, which nothing sets unless you ask for it, so this is out
|
||||
# of the graph for every ordinary build and for every CI job but the dedicated
|
||||
# `loom` one. Run it with:
|
||||
#
|
||||
# RUSTFLAGS="--cfg loom" cargo test --lib --no-default-features \
|
||||
# --features scsi fd_handoff
|
||||
#
|
||||
# `cargo test --lib` specifically: as a DEV-dependency loom is only in scope for
|
||||
# test targets, which is why `fd_handoff` gates its import on `all(loom, test)`
|
||||
# and falls back to std elsewhere. Without that, `--cfg loom` would break plain
|
||||
# `cargo build`/`clippy` with an unresolved-crate error.
|
||||
[target.'cfg(loom)'.dev-dependencies]
|
||||
loom = "0.7"
|
||||
|
||||
[[bench]]
|
||||
name = "sgio_read"
|
||||
harness = false
|
||||
|
||||
|
||||
[lints.rust]
|
||||
# `loom` is set by RUSTFLAGS for the model checker above, never by a feature.
|
||||
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(loom)'] }
|
||||
|
||||
[lints.clippy]
|
||||
uninlined_format_args = "allow"
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
# libfreemkv — local dev helper.
|
||||
# Mirrors the workspace-wide CI checks but scoped to this single crate.
|
||||
|
||||
.PHONY: test build check ci clean
|
||||
.PHONY: test build check loom ci clean
|
||||
|
||||
test:
|
||||
cargo test --tests
|
||||
@@ -13,7 +13,13 @@ check:
|
||||
cargo fmt --check
|
||||
cargo clippy --all-targets -- -D warnings
|
||||
|
||||
ci: check build test
|
||||
# Model-check the scsi::fd_handoff atomics. `cargo test` cannot fail on those
|
||||
# orderings — they compile to the same instructions on x86_64 — so this is the
|
||||
# only check on them. Mirrors the `loom` CI job.
|
||||
loom:
|
||||
RUSTFLAGS="--cfg loom" cargo test --lib --no-default-features --features scsi fd_handoff
|
||||
|
||||
ci: check build test loom
|
||||
|
||||
clean:
|
||||
cargo clean
|
||||
|
||||
@@ -0,0 +1,510 @@
|
||||
//! The fd hand-off between a Linux recovery thread and the transport that
|
||||
//! spawned it.
|
||||
//!
|
||||
//! When `SgIoTransport::execute` sees a transport-level failure it abandons
|
||||
//! its fd and spawns a detached thread to open a fresh one. That thread has no
|
||||
//! reference back to the transport — the transport may have been dropped
|
||||
//! before the `open()` returns — so the two sides rendezvous through a
|
||||
//! single-slot mailbox: an `AtomicI32` holding an owned fd (`-1` = empty) and
|
||||
//! an `AtomicBool` saying whether the transport is already gone.
|
||||
//!
|
||||
//! The invariant, and the only thing that matters here: **every fd that
|
||||
//! reaches the slot is closed exactly once.** Closing zero times leaks a
|
||||
//! descriptor; closing twice is worse than a leak, because a `close()` of an
|
||||
//! fd number the process has since reused closes an unrelated file.
|
||||
//!
|
||||
//! # Why this lives in `scsi/`, not in `linux.rs`
|
||||
//!
|
||||
//! `linux.rs` is compiled on exactly one host, so nothing in it can be tested
|
||||
//! on macOS or Windows CI — the same reason [`super::checked_cdb_len`] lives
|
||||
//! in the parent module. The protocol below is plain atomics with no syscalls
|
||||
//! in it, so it is compiled and tested on every platform even though Linux is
|
||||
//! its only caller.
|
||||
//!
|
||||
//! # Memory ordering
|
||||
//!
|
||||
//! The subtle case is a `Drop` that races the recovery thread. The two sides
|
||||
//! are ordered only through the slot, so the release edge has to be carried by
|
||||
//! the slot's own operations:
|
||||
//!
|
||||
//! 1. [`claim_for_teardown`] publishes `dead = true` and then claims the slot.
|
||||
//! 2. [`publish_recovered_fd`] fills the slot and then reads `dead`.
|
||||
//!
|
||||
//! Publish-first needs nothing special: teardown's swap is an RMW, so it reads
|
||||
//! the last value in the slot's modification order — the fd — and closes it by
|
||||
//! coherence alone. The dangerous order is teardown-first, where
|
||||
//! `claim_for_teardown` finds the slot empty and returns, leaving the recovery
|
||||
//! thread as the only one that can close the fd — and it will only do so if its
|
||||
//! `dead` load observes the `true`.
|
||||
//!
|
||||
//! That observation is not free. Rust's atomics follow the C++20 rules
|
||||
//! (`std::sync::atomic` module docs: "Rust atomics currently follow the same
|
||||
//! rules as C++20 atomics, specifically the rules from the `intro.races`
|
||||
//! section, without the 'consume' memory ordering"), and under those rules:
|
||||
//!
|
||||
//! - Every operation on the slot is a read-modify-write — `swap` or
|
||||
//! `compare_exchange`, never a plain store after construction. That matters,
|
||||
//! because "a release sequence headed by a release operation A on an atomic
|
||||
//! object M is a maximal contiguous sub-sequence of side effects in the
|
||||
//! modification order of M, where the first operation is A, and every
|
||||
//! subsequent operation is an atomic read-modify-write operation"
|
||||
//! ([intro.races], N4868). So once `claim_for_teardown`'s `swap` executes,
|
||||
//! *every* later slot operation is in its release sequence — including one
|
||||
//! that lands behind an intervening drain or a second recovery thread,
|
||||
//! which is why the argument does not depend on the CAS reading the `swap`'s
|
||||
//! value directly.
|
||||
//! - Landing in that sequence is still not enough to synchronize with its
|
||||
//! head. That needs both halves: "an atomic operation A that performs a
|
||||
//! release operation on an atomic object M synchronizes with an atomic
|
||||
//! operation B that performs an acquire operation on M and takes its value
|
||||
//! from any side effect in the release sequence headed by A"
|
||||
//! ([atomics.order]/2, N4868).
|
||||
//!
|
||||
//! Both halves of that were missing:
|
||||
//!
|
||||
//! - `swap(-1, Acquire)` is not a release store. Per the [`Ordering::Acquire`]
|
||||
//! docs, "using this ordering for an operation that combines loads and
|
||||
//! stores leads to a `Relaxed` store operation" — so it heads no release
|
||||
//! sequence, and there was nothing for the CAS to synchronize with.
|
||||
//! - `compare_exchange(.., Release, Relaxed)` is not an acquire load. Per the
|
||||
//! [`Ordering::Release`] docs, it "leads to a `Relaxed` load operation" — so
|
||||
//! even against a proper release store it would establish no edge.
|
||||
//!
|
||||
//! Upgrading only one side therefore fixes nothing; the loom models in this
|
||||
//! file still report the leak with either half applied alone. Both are
|
||||
//! `AcqRel` below.
|
||||
//!
|
||||
//! Concretely, the CAS in [`publish_recovered_fd`] carries both roles at once:
|
||||
//! its release half publishes the `open()` to [`take_recovered_fd`], and its
|
||||
//! acquire half is what lets the `dead` load that follows observe a teardown
|
||||
//! that has already claimed the slot.
|
||||
//!
|
||||
//! [`take_recovered_fd`] is deliberately left at `Acquire`, which makes its
|
||||
//! store half relaxed. That is harmless precisely because of the release-
|
||||
//! sequence rule above: an RMW extends the sequence whatever its own ordering,
|
||||
//! so a drain landing between `claim_for_teardown` and a later CAS does not
|
||||
//! break the edge. It needs no release half of its own — it is called from
|
||||
//! `execute()`, which cannot run concurrently with `Drop` (both need the
|
||||
//! transport), so it never has anything of its own to publish to a teardown.
|
||||
//! `loom_drain_does_not_break_the_release_sequence` is the check on that.
|
||||
//!
|
||||
//! `dead`'s `Release` store is belt-and-braces: it is sequenced before the
|
||||
//! `AcqRel` swap, and the swap is what carries the edge. Downgrading it to
|
||||
//! `Relaxed` leaves all three models passing.
|
||||
//!
|
||||
//! # Building the model
|
||||
//!
|
||||
//! ```text
|
||||
//! RUSTFLAGS="--cfg loom" cargo test --lib --no-default-features \
|
||||
//! --features scsi fd_handoff
|
||||
//! ```
|
||||
//!
|
||||
//! Under those flags the protocol runs on loom's instrumented atomics, so the
|
||||
//! models check the code the transport actually calls rather than a
|
||||
//! transcription that could drift from it. The alias below is gated on `test`
|
||||
//! as well as `loom`, because `loom` is a DEV-dependency and so is not in the
|
||||
//! graph of a non-test target: `cfg(loom)` alone would break `cargo build`,
|
||||
//! `cargo check` and `cargo clippy --all-targets` under those flags with an
|
||||
//! unresolved-crate error. Non-test targets keep std's atomics.
|
||||
//!
|
||||
//! One consequence to know about: under `all(loom, test)`, `SgIoTransport`'s
|
||||
//! atomics are loom's, and those panic outside a `loom::model`. No unit test
|
||||
//! constructs a transport today — `raw_command` is a free-standing `fn` that
|
||||
//! touches none — but the first one that does must live behind
|
||||
//! `cfg(not(loom))`.
|
||||
|
||||
// See "Building the model" above for why this is gated on `test` too.
|
||||
#[cfg(all(loom, test))]
|
||||
pub(crate) use loom::sync::atomic::{AtomicBool, AtomicI32, Ordering};
|
||||
#[cfg(not(all(loom, test)))]
|
||||
pub(crate) use std::sync::atomic::{AtomicBool, AtomicI32, Ordering};
|
||||
|
||||
/// The empty slot. Not a valid fd; `open()` never returns a negative value.
|
||||
pub(crate) const EMPTY: i32 = -1;
|
||||
|
||||
/// Interpret a value taken out of the slot. Only a non-negative value is an fd
|
||||
/// the caller now owns; `EMPTY` — or any other negative, which nothing can put
|
||||
/// there — is nothing to close. Deliberately `>= 0` rather than `!= EMPTY`, to
|
||||
/// keep the guard the call sites used before this protocol was factored out.
|
||||
fn taken(value: i32) -> Option<i32> {
|
||||
(value >= 0).then_some(value)
|
||||
}
|
||||
|
||||
/// Take whatever fd a recovery thread has published, transferring ownership to
|
||||
/// the caller. `None` if no recovery has completed.
|
||||
///
|
||||
/// Called from `execute()` at the top of a command, which is why it does not
|
||||
/// consult `dead`: a live `execute()` means the transport is not being torn
|
||||
/// down. `Acquire` pairs with the `AcqRel` CAS in [`publish_recovered_fd`] so
|
||||
/// the `open()` that produced the fd happens-before the caller uses it.
|
||||
pub(crate) fn take_recovered_fd(slot: &AtomicI32) -> Option<i32> {
|
||||
taken(slot.swap(EMPTY, Ordering::Acquire))
|
||||
}
|
||||
|
||||
/// Publish a freshly opened fd from a recovery thread. Returns the fd the
|
||||
/// **caller** must now close, or `None` if it was handed off:
|
||||
///
|
||||
/// - The slot was already full — another recovery thread won, so we close
|
||||
/// ours rather than overwrite (and leak) the winner's.
|
||||
/// - The transport was torn down mid-`open()`; nothing will drain the slot,
|
||||
/// so we re-claim through it rather than close `new_fd` directly, which
|
||||
/// would double-close against a teardown swapping at the same moment.
|
||||
///
|
||||
/// Both orderings below are `AcqRel`; the module docs say why.
|
||||
pub(crate) fn publish_recovered_fd(
|
||||
slot: &AtomicI32,
|
||||
dead: &AtomicBool,
|
||||
new_fd: i32,
|
||||
) -> Option<i32> {
|
||||
// A negative `new_fd` would go in as the `EMPTY` sentinel, or as a value no
|
||||
// drain will hand back — silently lost either way. The caller checks
|
||||
// `open()` before getting here; this pins that as the contract.
|
||||
debug_assert!(new_fd >= 0, "only a real fd may enter the slot");
|
||||
if slot
|
||||
.compare_exchange(EMPTY, new_fd, Ordering::AcqRel, Ordering::Relaxed)
|
||||
.is_err()
|
||||
{
|
||||
return Some(new_fd);
|
||||
}
|
||||
if dead.load(Ordering::Acquire) {
|
||||
return taken(slot.swap(EMPTY, Ordering::AcqRel));
|
||||
}
|
||||
None
|
||||
}
|
||||
|
||||
/// Mark the transport dead and claim any fd still sitting in the slot,
|
||||
/// transferring ownership to the caller. Called from `Drop`.
|
||||
///
|
||||
/// The `dead` store must be ordered before the claim: a recovery thread that
|
||||
/// fills the slot after we have drained it has to see `dead == true`, or its
|
||||
/// fd is never closed by anyone. `AcqRel` on the swap is what carries that
|
||||
/// edge — see the module docs for why `Acquire` alone did not.
|
||||
pub(crate) fn claim_for_teardown(slot: &AtomicI32, dead: &AtomicBool) -> Option<i32> {
|
||||
dead.store(true, Ordering::Release);
|
||||
taken(slot.swap(EMPTY, Ordering::AcqRel))
|
||||
}
|
||||
|
||||
/// Ownership tests. These run on every platform's CI (the module has no Linux
|
||||
/// in it) and drive each case directly rather than hoping a thread schedule
|
||||
/// produces it. The assertion is always the same one: an fd that enters the
|
||||
/// slot leaves it through exactly one caller.
|
||||
///
|
||||
/// They cover the ownership protocol, NOT the memory orderings — those compile
|
||||
/// to the same instructions on x86_64, so these pass against the wrong ones.
|
||||
/// [`loom_tests`] is what covers the orderings.
|
||||
#[cfg(all(test, not(loom)))]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
/// Fresh slot + liveness flag, as `SgIoTransport::open` builds them.
|
||||
fn slot() -> (AtomicI32, AtomicBool) {
|
||||
(AtomicI32::new(EMPTY), AtomicBool::new(false))
|
||||
}
|
||||
|
||||
// ── Positive: the ordinary hand-off ────────────────────────────────────
|
||||
|
||||
/// The path taken on every successful recovery: the thread publishes, the
|
||||
/// next `execute()` drains. Neither side is asked to close anything.
|
||||
#[test]
|
||||
fn published_fd_reaches_the_next_execute() {
|
||||
let (s, d) = slot();
|
||||
assert_eq!(
|
||||
publish_recovered_fd(&s, &d, 7),
|
||||
None,
|
||||
"publisher handed off"
|
||||
);
|
||||
assert_eq!(take_recovered_fd(&s), Some(7), "execute() picks it up");
|
||||
assert_eq!(take_recovered_fd(&s), None, "slot is empty afterwards");
|
||||
}
|
||||
|
||||
/// Recovery completes, but the transport is dropped before another
|
||||
/// `execute()` runs (the abort-on-wedge path). Teardown owns the close.
|
||||
#[test]
|
||||
fn teardown_claims_an_undrained_fd() {
|
||||
let (s, d) = slot();
|
||||
assert_eq!(publish_recovered_fd(&s, &d, 7), None);
|
||||
assert_eq!(claim_for_teardown(&s, &d), Some(7), "Drop closes it");
|
||||
}
|
||||
|
||||
/// A small thing that the `EMPTY` sentinel exists to get right: `0` is a
|
||||
/// perfectly legal descriptor and must not read as an empty slot.
|
||||
#[test]
|
||||
fn fd_zero_is_a_real_fd_not_an_empty_slot() {
|
||||
let (s, d) = slot();
|
||||
assert_eq!(publish_recovered_fd(&s, &d, 0), None);
|
||||
assert_eq!(take_recovered_fd(&s), Some(0));
|
||||
}
|
||||
|
||||
// ── Negative: every way the fd could go unclosed or be closed twice ────
|
||||
|
||||
/// Teardown wins the race. It drains an empty slot and will never look
|
||||
/// again, so the late publisher must get its own fd back — this is the
|
||||
/// leak the `Acquire`/`Release` orderings could produce in practice.
|
||||
#[test]
|
||||
fn publish_after_teardown_returns_the_fd_to_the_publisher() {
|
||||
let (s, d) = slot();
|
||||
assert_eq!(claim_for_teardown(&s, &d), None, "nothing published yet");
|
||||
assert_eq!(
|
||||
publish_recovered_fd(&s, &d, 7),
|
||||
Some(7),
|
||||
"transport is gone — publisher must close its own fd"
|
||||
);
|
||||
assert_eq!(take_recovered_fd(&s), None, "and must not leave it behind");
|
||||
}
|
||||
|
||||
/// Two recovery threads for one transport. The loser closes the fd it
|
||||
/// opened; it must not overwrite the winner's, which nothing would then
|
||||
/// close.
|
||||
#[test]
|
||||
fn losing_publisher_closes_its_own_fd_and_leaves_the_winners() {
|
||||
let (s, d) = slot();
|
||||
assert_eq!(publish_recovered_fd(&s, &d, 7), None, "winner");
|
||||
assert_eq!(publish_recovered_fd(&s, &d, 9), Some(9), "loser closes 9");
|
||||
assert_eq!(take_recovered_fd(&s), Some(7), "winner's fd survived");
|
||||
}
|
||||
|
||||
/// Teardown after the fd has already been drained by `execute()`. The slot
|
||||
/// is empty and teardown must claim nothing — returning the stale value
|
||||
/// here would close an fd the transport is still using.
|
||||
#[test]
|
||||
fn teardown_after_drain_claims_nothing() {
|
||||
let (s, d) = slot();
|
||||
assert_eq!(publish_recovered_fd(&s, &d, 7), None);
|
||||
assert_eq!(take_recovered_fd(&s), Some(7));
|
||||
assert_eq!(claim_for_teardown(&s, &d), None, "no double close");
|
||||
}
|
||||
|
||||
/// Teardown twice (belt and braces — `Drop` runs once, but the second
|
||||
/// claim must be inert rather than re-yielding the fd).
|
||||
#[test]
|
||||
fn teardown_is_idempotent() {
|
||||
let (s, d) = slot();
|
||||
assert_eq!(publish_recovered_fd(&s, &d, 7), None);
|
||||
assert_eq!(claim_for_teardown(&s, &d), Some(7));
|
||||
assert_eq!(claim_for_teardown(&s, &d), None);
|
||||
}
|
||||
|
||||
/// Empty slot, nothing published: both drains are no-ops.
|
||||
#[test]
|
||||
fn draining_an_empty_slot_yields_nothing() {
|
||||
let (s, d) = slot();
|
||||
assert_eq!(take_recovered_fd(&s), None);
|
||||
assert_eq!(claim_for_teardown(&s, &d), None);
|
||||
}
|
||||
|
||||
/// Accounting over the orders the three operations can run in, one whole
|
||||
/// operation at a time: for each, the fd is handed to exactly one caller.
|
||||
///
|
||||
/// Sequences, not interleavings — each function is the atomic unit here, so
|
||||
/// this cannot express the case the ordering bug lives in (a teardown swap
|
||||
/// landing between `publish_recovered_fd`'s CAS and its `dead` load). That
|
||||
/// one is only reachable in the loom models below.
|
||||
#[test]
|
||||
fn every_sequence_closes_the_fd_exactly_once() {
|
||||
// Each sequence returns how many callers were handed the fd across the
|
||||
// whole run; the answer is always exactly one.
|
||||
fn exactly_one(order: &str, run: impl Fn() -> usize) {
|
||||
assert_eq!(run(), 1, "`{order}` must close fd 7 exactly once");
|
||||
}
|
||||
|
||||
exactly_one("publish, drain", || {
|
||||
let (s, d) = slot();
|
||||
publish_recovered_fd(&s, &d, 7).is_some() as usize
|
||||
+ take_recovered_fd(&s).is_some() as usize
|
||||
});
|
||||
exactly_one("publish, teardown", || {
|
||||
let (s, d) = slot();
|
||||
publish_recovered_fd(&s, &d, 7).is_some() as usize
|
||||
+ claim_for_teardown(&s, &d).is_some() as usize
|
||||
});
|
||||
exactly_one("teardown, publish", || {
|
||||
let (s, d) = slot();
|
||||
claim_for_teardown(&s, &d).is_some() as usize
|
||||
+ publish_recovered_fd(&s, &d, 7).is_some() as usize
|
||||
});
|
||||
exactly_one("drain, publish, teardown", || {
|
||||
let (s, d) = slot();
|
||||
take_recovered_fd(&s).is_some() as usize
|
||||
+ publish_recovered_fd(&s, &d, 7).is_some() as usize
|
||||
+ claim_for_teardown(&s, &d).is_some() as usize
|
||||
});
|
||||
}
|
||||
|
||||
/// The real race, run for real: a teardown thread against a recovery
|
||||
/// thread, many times over. This exercises the protocol under a genuine
|
||||
/// scheduler — it does NOT prove the memory orderings (x86 and, in
|
||||
/// practice, AArch64 will happily pass the weaker ones). The loom model
|
||||
/// below is what covers those; this covers the ownership logic.
|
||||
#[test]
|
||||
fn concurrent_teardown_and_publish_close_each_fd_exactly_once() {
|
||||
use std::sync::Arc;
|
||||
|
||||
for round in 0..2_000 {
|
||||
let s = Arc::new(AtomicI32::new(EMPTY));
|
||||
let d = Arc::new(AtomicBool::new(false));
|
||||
let fd = 7;
|
||||
|
||||
let (s1, d1) = (Arc::clone(&s), Arc::clone(&d));
|
||||
let teardown = std::thread::spawn(move || claim_for_teardown(&s1, &d1));
|
||||
let (s2, d2) = (Arc::clone(&s), Arc::clone(&d));
|
||||
let recovery = std::thread::spawn(move || publish_recovered_fd(&s2, &d2, fd));
|
||||
|
||||
let claimed = teardown.join().unwrap();
|
||||
let returned = recovery.join().unwrap();
|
||||
|
||||
let closers = claimed.is_some() as usize + returned.is_some() as usize;
|
||||
assert_eq!(
|
||||
closers, 1,
|
||||
"round {round}: fd must be closed exactly once, not {closers} times \
|
||||
(0 = leaked descriptor, 2 = close() of a reused fd number)"
|
||||
);
|
||||
assert_eq!(
|
||||
s.load(Ordering::Acquire),
|
||||
EMPTY,
|
||||
"round {round}: slot must not be left holding an fd nobody owns"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Memory-ordering models. `cargo test` cannot observe a missing release edge:
|
||||
/// the orderings compile to the same instructions on x86_64, and on AArch64 the
|
||||
/// window is far too narrow to hit by chance. loom enumerates the executions
|
||||
/// the C++20 model permits instead, and reports the stale `dead` load directly.
|
||||
#[cfg(all(test, loom))]
|
||||
mod loom_tests {
|
||||
use super::*;
|
||||
use loom::sync::Arc;
|
||||
use loom::sync::atomic::AtomicUsize;
|
||||
|
||||
/// `Drop` against a recovery thread: the execution this file exists for.
|
||||
///
|
||||
/// With `swap(.., Acquire)` in [`claim_for_teardown`], or with
|
||||
/// `compare_exchange(.., Release, ..)` in [`publish_recovered_fd`] — either
|
||||
/// one alone, not just both — loom finds an execution where teardown
|
||||
/// drains the empty slot, the recovery thread's CAS then fills it, and the
|
||||
/// `dead` load still reads `false`. Nobody closes the fd.
|
||||
#[test]
|
||||
fn loom_teardown_racing_publish_closes_the_fd_exactly_once() {
|
||||
loom::model(|| {
|
||||
let slot = Arc::new(AtomicI32::new(EMPTY));
|
||||
let dead = Arc::new(AtomicBool::new(false));
|
||||
let closes = Arc::new(AtomicUsize::new(0));
|
||||
|
||||
let (s, d, c) = (slot.clone(), dead.clone(), closes.clone());
|
||||
let teardown = loom::thread::spawn(move || {
|
||||
if claim_for_teardown(&s, &d).is_some() {
|
||||
c.fetch_add(1, Ordering::Relaxed);
|
||||
}
|
||||
});
|
||||
|
||||
let (s, d, c) = (slot.clone(), dead.clone(), closes.clone());
|
||||
let recovery = loom::thread::spawn(move || {
|
||||
if publish_recovered_fd(&s, &d, 7).is_some() {
|
||||
c.fetch_add(1, Ordering::Relaxed);
|
||||
}
|
||||
});
|
||||
|
||||
teardown.join().unwrap();
|
||||
recovery.join().unwrap();
|
||||
|
||||
assert_eq!(
|
||||
closes.load(Ordering::Relaxed),
|
||||
1,
|
||||
"fd must be closed exactly once (0 = leaked, 2 = double close)"
|
||||
);
|
||||
assert_eq!(slot.load(Ordering::Relaxed), EMPTY, "slot left non-empty");
|
||||
});
|
||||
}
|
||||
|
||||
/// Two recovery threads against a teardown. `execute()` spawns a thread on
|
||||
/// every transport-level failure and an earlier one may still be blocked in
|
||||
/// `open()`, so this is reachable, and it is the case where a CAS lands
|
||||
/// behind an intervening slot operation rather than reading the teardown
|
||||
/// swap's value directly — the release-sequence half of the module docs.
|
||||
///
|
||||
/// Bounded rather than exhaustive: unbounded, this model runs for many
|
||||
/// minutes, which is too slow for the CI step. Three preemptions is enough
|
||||
/// to reach the leaking schedule — the old orderings fail this test.
|
||||
#[test]
|
||||
fn loom_two_publishers_racing_teardown_close_each_fd_exactly_once() {
|
||||
let mut model = loom::model::Builder::new();
|
||||
model.preemption_bound = Some(3);
|
||||
model.check(|| {
|
||||
let slot = Arc::new(AtomicI32::new(EMPTY));
|
||||
let dead = Arc::new(AtomicBool::new(false));
|
||||
let closes = Arc::new(AtomicUsize::new(0));
|
||||
|
||||
let publishers: Vec<_> = [7, 9]
|
||||
.into_iter()
|
||||
.map(|fd| {
|
||||
let (s, d, c) = (slot.clone(), dead.clone(), closes.clone());
|
||||
loom::thread::spawn(move || {
|
||||
if publish_recovered_fd(&s, &d, fd).is_some() {
|
||||
c.fetch_add(1, Ordering::Relaxed);
|
||||
}
|
||||
})
|
||||
})
|
||||
.collect();
|
||||
|
||||
let (s, d, c) = (slot.clone(), dead.clone(), closes.clone());
|
||||
let teardown = loom::thread::spawn(move || {
|
||||
if claim_for_teardown(&s, &d).is_some() {
|
||||
c.fetch_add(1, Ordering::Relaxed);
|
||||
}
|
||||
});
|
||||
|
||||
for p in publishers {
|
||||
p.join().unwrap();
|
||||
}
|
||||
teardown.join().unwrap();
|
||||
|
||||
assert_eq!(
|
||||
closes.load(Ordering::Relaxed),
|
||||
2,
|
||||
"both fds must be closed, exactly once each"
|
||||
);
|
||||
assert_eq!(slot.load(Ordering::Relaxed), EMPTY, "slot left non-empty");
|
||||
});
|
||||
}
|
||||
|
||||
/// The check on leaving [`take_recovered_fd`] at `Acquire` — i.e. with a
|
||||
/// relaxed store half. A drain landing between the teardown swap and a
|
||||
/// recovery CAS must not break the release sequence the edge rides on.
|
||||
#[test]
|
||||
fn loom_drain_does_not_break_the_release_sequence() {
|
||||
loom::model(|| {
|
||||
let slot = Arc::new(AtomicI32::new(EMPTY));
|
||||
let dead = Arc::new(AtomicBool::new(false));
|
||||
let closes = Arc::new(AtomicUsize::new(0));
|
||||
|
||||
// execute() drains, then (later, same transport) Drop tears down.
|
||||
let (s, d, c) = (slot.clone(), dead.clone(), closes.clone());
|
||||
let transport = loom::thread::spawn(move || {
|
||||
if take_recovered_fd(&s).is_some() {
|
||||
c.fetch_add(1, Ordering::Relaxed);
|
||||
}
|
||||
if claim_for_teardown(&s, &d).is_some() {
|
||||
c.fetch_add(1, Ordering::Relaxed);
|
||||
}
|
||||
});
|
||||
|
||||
let (s, d, c) = (slot.clone(), dead.clone(), closes.clone());
|
||||
let recovery = loom::thread::spawn(move || {
|
||||
if publish_recovered_fd(&s, &d, 7).is_some() {
|
||||
c.fetch_add(1, Ordering::Relaxed);
|
||||
}
|
||||
});
|
||||
|
||||
transport.join().unwrap();
|
||||
recovery.join().unwrap();
|
||||
|
||||
assert_eq!(
|
||||
closes.load(Ordering::Relaxed),
|
||||
1,
|
||||
"fd must be closed exactly once (0 = leaked, 2 = double close)"
|
||||
);
|
||||
assert_eq!(slot.load(Ordering::Relaxed), EMPTY, "slot left non-empty");
|
||||
});
|
||||
}
|
||||
}
|
||||
+163
-56
@@ -9,9 +9,13 @@
|
||||
//! See docs/scsi-linux.md for the escalation ladder, the design
|
||||
//! rationale, and the pre-0.13.20 async-poll design it replaced.
|
||||
|
||||
use super::fd_handoff::{
|
||||
AtomicBool, AtomicI32, claim_for_teardown, publish_recovered_fd, take_recovered_fd,
|
||||
};
|
||||
use super::{DataDirection, ScsiResult, ScsiTransport};
|
||||
use crate::error::{Error, Result};
|
||||
use std::path::Path;
|
||||
use std::sync::Arc;
|
||||
|
||||
const SG_IO: u32 = 0x2285;
|
||||
const SG_DXFER_NONE: i32 = -1;
|
||||
@@ -24,6 +28,10 @@ const SG_FLAG_Q_AT_HEAD: u32 = 0x10;
|
||||
/// 16 bytes. Matches `K_MAX_CDB_SIZE` in the macOS and Windows backends.
|
||||
const K_MAX_CDB_SIZE: usize = 16;
|
||||
|
||||
/// SPC-4 PREVENT ALLOW MEDIUM REMOVAL (0x1E) with PREVENT=0. Sent by `Drop` so
|
||||
/// the tray is not left locked; a six-byte group-0 CDB.
|
||||
const ALLOW_MEDIUM_REMOVAL: [u8; 6] = [0x1E, 0, 0, 0, 0, 0];
|
||||
|
||||
#[repr(C)]
|
||||
#[allow(non_camel_case_types)]
|
||||
struct sg_io_hdr {
|
||||
@@ -61,12 +69,15 @@ const _: () = assert!(std::mem::size_of::<sg_io_hdr>() == 64);
|
||||
pub struct SgIoTransport {
|
||||
pub fd: i32,
|
||||
device_path: std::path::PathBuf,
|
||||
pub fd_recovery: std::sync::Arc<std::sync::atomic::AtomicI32>,
|
||||
/// Set to `true` by `Drop` before the transport is torn down. The
|
||||
/// recovery thread checks this after a successful `compare_exchange`
|
||||
/// and closes `new_fd` itself when the transport is already gone,
|
||||
/// preventing an fd leak when Drop races the recovery thread.
|
||||
dead: std::sync::Arc<std::sync::atomic::AtomicBool>,
|
||||
/// Single-slot mailbox a background recovery thread publishes a freshly
|
||||
/// opened fd into. Drained by `execute()`, or claimed by `Drop` if the
|
||||
/// transport dies first. The protocol — and the memory ordering that makes
|
||||
/// it leak-free — lives in `super::fd_handoff`.
|
||||
pub fd_recovery: Arc<AtomicI32>,
|
||||
/// Set to `true` by `Drop` before it claims the slot. A recovery thread
|
||||
/// that publishes after that point sees it and closes its own fd, since
|
||||
/// nothing will ever drain the slot again.
|
||||
dead: Arc<AtomicBool>,
|
||||
}
|
||||
|
||||
impl SgIoTransport {
|
||||
@@ -86,8 +97,8 @@ impl SgIoTransport {
|
||||
Ok(SgIoTransport {
|
||||
fd,
|
||||
device_path: device,
|
||||
fd_recovery: std::sync::Arc::new(std::sync::atomic::AtomicI32::new(-1)),
|
||||
dead: std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)),
|
||||
fd_recovery: Arc::new(AtomicI32::new(super::fd_handoff::EMPTY)),
|
||||
dead: Arc::new(AtomicBool::new(false)),
|
||||
})
|
||||
}
|
||||
|
||||
@@ -107,14 +118,22 @@ impl SgIoTransport {
|
||||
})
|
||||
}
|
||||
|
||||
/// Send a raw SCSI command on an fd. Used by reset() before the
|
||||
/// transport is constructed.
|
||||
fn raw_command(fd: i32, cdb: &[u8], timeout_ms: u32) -> std::result::Result<(), ()> {
|
||||
/// Send a raw SCSI command on a bare fd, for the one caller that has no
|
||||
/// live `&mut self` to route through `execute()`: `Drop`, unlocking the
|
||||
/// tray on its way out.
|
||||
///
|
||||
/// The CDB goes through the shared [`super::checked_cdb_len`] guard rather
|
||||
/// than the old `cdb.len().min(16)` clamp — see docs/scsi-mod.md,
|
||||
/// "checked_cdb_len rationale". Returning a typed [`Error`] rather than
|
||||
/// `()` is what lets `raw_command_cdb_guard_tests` tell a rejected CDB
|
||||
/// from an I/O failure without a real `/dev/sg*` device.
|
||||
fn raw_command(fd: i32, cdb: &[u8], timeout_ms: u32) -> Result<()> {
|
||||
let cmd_len = super::checked_cdb_len(cdb, K_MAX_CDB_SIZE)?;
|
||||
let mut sense = [0u8; 32];
|
||||
let mut hdr: sg_io_hdr = unsafe { std::mem::zeroed() };
|
||||
hdr.interface_id = b'S' as i32;
|
||||
hdr.dxfer_direction = SG_DXFER_NONE;
|
||||
hdr.cmd_len = cdb.len().min(16) as u8;
|
||||
hdr.cmd_len = cmd_len;
|
||||
hdr.mx_sb_len = sense.len() as u8;
|
||||
hdr.dxfer_len = 0;
|
||||
hdr.dxferp = std::ptr::null_mut();
|
||||
@@ -124,15 +143,32 @@ impl SgIoTransport {
|
||||
hdr.flags = SG_FLAG_Q_AT_HEAD;
|
||||
|
||||
let ret = unsafe { libc::ioctl(fd, SG_IO as _, &mut hdr as *mut sg_io_hdr) };
|
||||
if ret < 0 {
|
||||
return Err(Error::IoError {
|
||||
source: std::io::Error::last_os_error(),
|
||||
});
|
||||
}
|
||||
// Mask DRIVER_SENSE (0x08): it only flags "sense data present", not
|
||||
// a failure — matches execute()'s driver_status_real handling so a
|
||||
// benign CHECK CONDITION isn't misread as a transport error.
|
||||
let driver_status_real = hdr.driver_status & !super::DRIVER_SENSE;
|
||||
if ret < 0 || hdr.status != 0 || hdr.host_status != 0 || driver_status_real != 0 {
|
||||
Err(())
|
||||
} else {
|
||||
Ok(())
|
||||
if hdr.host_status != 0 || driver_status_real != 0 {
|
||||
// No SCSI status was delivered: same synthesised sentinel and same
|
||||
// `sense: None` that `execute()` reports for a transport wedge.
|
||||
return Err(Error::ScsiError {
|
||||
opcode: cdb[0],
|
||||
status: super::SCSI_STATUS_TRANSPORT_FAILURE,
|
||||
sense: None,
|
||||
});
|
||||
}
|
||||
if hdr.status != 0 {
|
||||
return Err(Error::ScsiError {
|
||||
opcode: cdb[0],
|
||||
status: hdr.status,
|
||||
sense: Some(super::parse_sense(&sense, hdr.sb_len_wr)),
|
||||
});
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn to_c_path(device: &Path) -> Vec<u8> {
|
||||
@@ -174,20 +210,13 @@ impl Drop for SgIoTransport {
|
||||
fn drop(&mut self) {
|
||||
if self.fd >= 0 {
|
||||
// Unlock tray before closing — don't leave it locked.
|
||||
let _ = Self::raw_command(self.fd, &[0x1E, 0, 0, 0, 0, 0], 3_000);
|
||||
let _ = Self::raw_command(self.fd, &ALLOW_MEDIUM_REMOVAL, 3_000);
|
||||
unsafe { libc::close(self.fd) };
|
||||
}
|
||||
// Signal the recovery thread this transport is gone. Must be set
|
||||
// before the fd_recovery swap so it can't observe dead=false and
|
||||
// store into a slot Drop is no longer going to drain.
|
||||
self.dead.store(true, std::sync::atomic::Ordering::Release);
|
||||
// A failed execute() spawns a thread that opens a fresh fd into
|
||||
// fd_recovery, normally drained at the top of the next execute().
|
||||
// If dropped first (abort-on-wedge), claim and close it here.
|
||||
let recovered = self
|
||||
.fd_recovery
|
||||
.swap(-1, std::sync::atomic::Ordering::Acquire);
|
||||
if recovered >= 0 {
|
||||
// fd_recovery, normally drained at the top of the next execute(). If
|
||||
// dropped first (abort-on-wedge), claim and close it here.
|
||||
if let Some(recovered) = claim_for_teardown(&self.fd_recovery, &self.dead) {
|
||||
unsafe { libc::close(recovered) };
|
||||
}
|
||||
}
|
||||
@@ -221,10 +250,7 @@ impl ScsiTransport for SgIoTransport {
|
||||
);
|
||||
|
||||
// Check if a background recovery has produced a new fd.
|
||||
let recovered = self
|
||||
.fd_recovery
|
||||
.swap(-1, std::sync::atomic::Ordering::Acquire);
|
||||
if recovered >= 0 {
|
||||
if let Some(recovered) = take_recovered_fd(&self.fd_recovery) {
|
||||
// Close the old fd if it's still valid.
|
||||
if self.fd >= 0 {
|
||||
unsafe { libc::close(self.fd) };
|
||||
@@ -329,31 +355,11 @@ impl ScsiTransport for SgIoTransport {
|
||||
if new_fd < 0 {
|
||||
return;
|
||||
}
|
||||
// Publish only into an empty (-1) slot. If two recovery
|
||||
// threads race, the loser closes its own fd rather than
|
||||
// overwriting (and leaking) the winner's.
|
||||
if recovery
|
||||
.compare_exchange(
|
||||
-1,
|
||||
new_fd,
|
||||
std::sync::atomic::Ordering::Release,
|
||||
std::sync::atomic::Ordering::Relaxed,
|
||||
)
|
||||
.is_err()
|
||||
{
|
||||
// Another recovery thread already stored its fd; ours
|
||||
// was not stored so it's our responsibility to close it.
|
||||
unsafe { libc::close(new_fd) };
|
||||
return;
|
||||
}
|
||||
// Check whether Drop raced us: if the transport is already
|
||||
// dead it won't drain fd_recovery, so swap to atomically
|
||||
// claim new_fd and close it; a -1 result means Drop won.
|
||||
if dead.load(std::sync::atomic::Ordering::Acquire) {
|
||||
let claimed = recovery.swap(-1, std::sync::atomic::Ordering::AcqRel);
|
||||
if claimed >= 0 {
|
||||
unsafe { libc::close(claimed) };
|
||||
}
|
||||
// Hand the fd to the transport. Comes back to us only if
|
||||
// nobody there will ever close it: another recovery thread
|
||||
// won the slot, or Drop already tore the transport down.
|
||||
if let Some(orphan) = publish_recovered_fd(&recovery, &dead, new_fd) {
|
||||
unsafe { libc::close(orphan) };
|
||||
}
|
||||
});
|
||||
|
||||
@@ -586,3 +592,104 @@ pub(super) fn drive_has_disc(path: &Path) -> Result<bool> {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// ── CDB guard wiring ───────────────────────────────────────────────────────
|
||||
#[cfg(test)]
|
||||
mod raw_command_cdb_guard_tests {
|
||||
use super::*;
|
||||
|
||||
/// `ioctl()` on this fails with `EBADF` before touching any device, so the
|
||||
/// tests below need no `/dev/sg*` and have no side effects — they run in
|
||||
/// ordinary CI. Any error that is NOT `InvalidCdbLength` therefore means
|
||||
/// the CDB cleared the guard and the syscall was reached.
|
||||
const NO_FD: i32 = -1;
|
||||
|
||||
fn reached_the_ioctl(err: &Error) -> bool {
|
||||
!matches!(err, Error::InvalidCdbLength { .. })
|
||||
}
|
||||
|
||||
// ── Negative: CDBs the guard must reject ──────────────────────────────
|
||||
|
||||
/// The regression this module exists for. `raw_command` used to set
|
||||
/// `cmd_len = cdb.len().min(16)`, silently shortening an over-length CDB
|
||||
/// into a *different* SPC-4 command and issuing it. It must fail the
|
||||
/// caller instead, with the same error `execute()` gives.
|
||||
///
|
||||
/// Before the fix this test fails by reaching the `ioctl` and returning
|
||||
/// `IoError(EBADF)` — the guard never ran.
|
||||
#[test]
|
||||
fn over_length_cdb_is_rejected_not_truncated() {
|
||||
let cdb = [0u8; K_MAX_CDB_SIZE + 1];
|
||||
match SgIoTransport::raw_command(NO_FD, &cdb, 3_000) {
|
||||
Err(Error::InvalidCdbLength { len, max }) => {
|
||||
assert_eq!(len, K_MAX_CDB_SIZE + 1);
|
||||
assert_eq!(max, K_MAX_CDB_SIZE);
|
||||
}
|
||||
other => panic!(
|
||||
"over-length CDB must be rejected with InvalidCdbLength, got {other:?} — \
|
||||
a non-guard error means it was truncated to {K_MAX_CDB_SIZE} bytes and sent"
|
||||
),
|
||||
}
|
||||
}
|
||||
|
||||
/// Well past the field width, to show the guard is a bound and not a
|
||||
/// one-off check at `max + 1`.
|
||||
#[test]
|
||||
fn far_over_length_cdb_is_rejected() {
|
||||
let cdb = [0u8; 260];
|
||||
assert!(matches!(
|
||||
SgIoTransport::raw_command(NO_FD, &cdb, 3_000),
|
||||
Err(Error::InvalidCdbLength {
|
||||
len: 260,
|
||||
max: K_MAX_CDB_SIZE
|
||||
})
|
||||
));
|
||||
}
|
||||
|
||||
/// An empty CDB must be rejected rather than handed to the sg driver as a
|
||||
/// zero-length command descriptor, which under SPC-4 is not a command at
|
||||
/// all. (The pre-fix code did not panic on this — it never read the opcode
|
||||
/// — it just issued `cmd_len = 0`. The two error paths added here DO read
|
||||
/// `cdb[0]`, so the guard is now load-bearing for that too.)
|
||||
#[test]
|
||||
fn empty_cdb_is_rejected_before_the_opcode_is_read() {
|
||||
assert!(matches!(
|
||||
SgIoTransport::raw_command(NO_FD, &[], 3_000),
|
||||
Err(Error::InvalidCdbLength {
|
||||
len: 0,
|
||||
max: K_MAX_CDB_SIZE
|
||||
})
|
||||
));
|
||||
}
|
||||
|
||||
// ── Positive: CDBs the guard must let through ─────────────────────────
|
||||
|
||||
/// The only CDB `raw_command` is called with in the crate. It must still
|
||||
/// reach the syscall — a guard that rejected this would silently stop
|
||||
/// unlocking the tray on `Drop`.
|
||||
#[test]
|
||||
fn drops_allow_medium_removal_still_reaches_the_ioctl() {
|
||||
let err = SgIoTransport::raw_command(NO_FD, &ALLOW_MEDIUM_REMOVAL, 3_000)
|
||||
.expect_err("EBADF on fd -1");
|
||||
assert!(
|
||||
reached_the_ioctl(&err),
|
||||
"the 6-byte CDB Drop sends must pass the guard, got {err:?}"
|
||||
);
|
||||
}
|
||||
|
||||
/// Every real SPC-4 CDB length (groups 0-5: 6, 10, 12, 16), plus the
|
||||
/// shortest a caller could legally construct, plus the boundary case — a
|
||||
/// CDB of exactly `K_MAX_CDB_SIZE` must not be caught by an off-by-one.
|
||||
#[test]
|
||||
fn in_range_cdb_lengths_reach_the_ioctl() {
|
||||
for len in [1usize, 6, 10, 12, K_MAX_CDB_SIZE] {
|
||||
let cdb = vec![0x1Eu8; len];
|
||||
let err = SgIoTransport::raw_command(NO_FD, &cdb, 3_000).expect_err("EBADF on fd -1");
|
||||
assert!(
|
||||
reached_the_ioctl(&err),
|
||||
"a {len}-byte CDB is within the {K_MAX_CDB_SIZE}-byte field and must \
|
||||
pass the guard, got {err:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -12,6 +12,12 @@ mod macos;
|
||||
#[cfg(target_os = "windows")]
|
||||
mod windows;
|
||||
|
||||
// The Linux recovery-thread fd hand-off. Compiled on every platform though
|
||||
// only `linux.rs` uses it, so its tests run everywhere — the same reasoning as
|
||||
// `checked_cdb_len`. Rationale and memory ordering: the module's own docs.
|
||||
#[cfg_attr(not(target_os = "linux"), allow(dead_code))]
|
||||
pub(crate) mod fd_handoff;
|
||||
|
||||
#[allow(unused_imports)]
|
||||
use crate::error::{Error, Result};
|
||||
use std::path::Path;
|
||||
|
||||
Reference in New Issue
Block a user