From 449bb6139d101040f5daa1a3ca2138dd5ddbf9f1 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 24 Jul 2026 13:45:54 -0600 Subject: [PATCH 1/5] fix: suppress tsan false positives tokio's I/O driver runs on its own worker thread(s). When another thread registers an I/O source , tokio allocates and initializes a `ScheduledIo`, then hands the fd to the kernel via `epoll_ctl`. The driver worker only ever sees that `ScheduledIo` after `epoll_wait` returns its token (strictly after the registration syscall). That happens-before edge is real but runs through the kernel, which ThreadSanitizer cannot instrument, so it reports a false positive between RegistrationSet::allocate (init) and Driver::turn -> ScheduledIo::{set_readiness,wake} (use). Matching either tokio frame silences that pattern without hiding races in our own fd handling. Signed-off-by: Daniel Noland --- Cargo.lock | 53 +++++++++++-------- dataplane/build.rs | 45 ++++++++++++++++ dataplane/src/main.rs | 2 + dataplane/src/sanitizer/mod.rs | 26 ++++++++++ dataplane/src/sanitizer/tsan.suppress | 11 ++++ default.nix | 74 +++++++++++++++++++++++++-- 6 files changed, 185 insertions(+), 26 deletions(-) create mode 100644 dataplane/build.rs create mode 100644 dataplane/src/sanitizer/mod.rs create mode 100644 dataplane/src/sanitizer/tsan.suppress diff --git a/Cargo.lock b/Cargo.lock index 23d39ecd15..21f6ff8f95 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -408,7 +408,7 @@ dependencies = [ "bitflags 2.13.1", "cexpr", "clang-sys", - "itertools", + "itertools 0.13.0", "proc-macro2", "quote", "regex", @@ -700,9 +700,9 @@ dependencies = [ [[package]] name = "cc" -version = "1.3.0" +version = "1.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c89588d05638b5b4594a3348a2d6c20277e43a7f5c5202b05cc56888475a47b8" +checksum = "5add81bb678e6cb321aff7fa0dc7689ad82b112dbc032cea19f91d6b8e3582b9" dependencies = [ "find-msvc-tools", "jobserver", @@ -900,7 +900,7 @@ version = "3.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "faf9468729b8cbcea668e36183cb69d317348c2e08e994829fb56ebfdfbaac34" dependencies = [ - "windows-sys 0.59.0", + "windows-sys 0.52.0", ] [[package]] @@ -1016,7 +1016,7 @@ dependencies = [ "ciborium", "clap", "criterion-plot", - "itertools", + "itertools 0.13.0", "num-traits", "oorandom", "page_size", @@ -1034,7 +1034,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d8d80a2f4f5b554395e47b5d8305bc3d27813bacb73493eb1001e8f76dae29ea" dependencies = [ "cast", - "itertools", + "itertools 0.13.0", ] [[package]] @@ -2136,9 +2136,9 @@ dependencies = [ [[package]] name = "either" -version = "1.16.0" +version = "1.17.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "91622ff5e7162018101f2fea40d6ebf4a78bbe5a49736a2020649edf9693679e" +checksum = "9e5e8f6c15a24b9a3ee5efec809ccd006d3b30e8b3bb63c39af737c7f87daa1d" [[package]] name = "encoding_rs" @@ -2211,7 +2211,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.59.0", + "windows-sys 0.52.0", ] [[package]] @@ -2294,7 +2294,7 @@ checksum = "0ce92ff622d6dadf7349484f42c93271a0d49b7cc4d466a936405bacbe10aa78" dependencies = [ "cfg-if", "rustix", - "windows-sys 0.59.0", + "windows-sys 0.52.0", ] [[package]] @@ -3080,6 +3080,15 @@ dependencies = [ "either", ] +[[package]] +name = "itertools" +version = "0.14.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2b192c782037fadd9cfa75548310488aabdbf3d2da73885b31bd0abd03351285" +dependencies = [ + "either", +] + [[package]] name = "itoa" version = "1.0.18" @@ -3206,9 +3215,9 @@ dependencies = [ [[package]] name = "jsonpath-rust" -version = "1.0.5" +version = "1.0.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4222e00941bfe18bf81b79fa23ad933e233bfa18f3ee27254c5dd9b1543b5d5f" +checksum = "a2dbe0623574defe58ba113596c848797f131727e8f54d4b4d10793e1fe14d40" dependencies = [ "pest", "pest_derive", @@ -3972,7 +3981,7 @@ version = "0.50.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" dependencies = [ - "windows-sys 0.59.0", + "windows-sys 0.61.2", ] [[package]] @@ -4543,7 +4552,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b570b25f7617e43d59005d0990ccb79e950a423952cea19671b7a876da390adf" dependencies = [ "anyhow", - "itertools", + "itertools 0.14.0", "proc-macro2", "quote", "syn 2.0.119", @@ -4788,7 +4797,7 @@ dependencies = [ "chrono", "crossterm", "fd-lock", - "itertools", + "itertools 0.13.0", "nu-ansi-term", "serde", "strip-ansi-escapes", @@ -4995,7 +5004,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys", - "windows-sys 0.59.0", + "windows-sys 0.52.0", ] [[package]] @@ -5027,9 +5036,9 @@ dependencies = [ [[package]] name = "rustls-pki-types" -version = "1.15.0" +version = "1.15.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "764899a24af3980067ee14bc143654f297b22eaebfe3c7b6b211920a5a59b046" +checksum = "2f4925028c7eb5d1fcdaf196971378ed9d2c1c4efc7dc5d011256f76c99c0a96" dependencies = [ "zeroize", ] @@ -5052,7 +5061,7 @@ dependencies = [ "security-framework", "security-framework-sys", "webpki-root-certs", - "windows-sys 0.59.0", + "windows-sys 0.52.0", ] [[package]] @@ -5691,7 +5700,7 @@ dependencies = [ "getrandom 0.3.4", "once_cell", "rustix", - "windows-sys 0.59.0", + "windows-sys 0.52.0", ] [[package]] @@ -5701,7 +5710,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "230a1b821ccbd75b185820a1f1ff7b14d21da1e442e22c0863ea5f08771a8874" dependencies = [ "rustix", - "windows-sys 0.59.0", + "windows-sys 0.61.2", ] [[package]] @@ -6404,7 +6413,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.59.0", + "windows-sys 0.52.0", ] [[package]] diff --git a/dataplane/build.rs b/dataplane/build.rs new file mode 100644 index 0000000000..5c068b69fe --- /dev/null +++ b/dataplane/build.rs @@ -0,0 +1,45 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +/// Detects which `-Zsanitizer=` (if any) is active and re-exports it as +/// an ordinary `cfg`. +/// +/// We cannot read the built-in `cfg(sanitize = "...")` from source without the +/// (currently) "unstable" `cfg_sanitize` feature, and gating the whole binary on +/// a nightly feature would break a plain `cargo build` on stable. Sniffing the +/// rustflags here keeps the crate stable-buildable while still letting +/// `src/sanitizer.rs` compile in the matching runtime hooks (e.g. baked-in +/// `ThreadSanitizer` suppressions) for -- and only for -- the relevant sanitizer +/// build. +fn detect_sanitizers() { + const SANITIZERS: [(&str, &str); 2] = [ + ("sanitizer=thread", "sanitize_thread"), + ("sanitizer=address", "sanitize_address"), + ]; + + // Declare every cfg we might set so the crate's `unexpected_cfgs` lint stays + // quiet even in the builds where we don't set it. + for (_, cfg) in SANITIZERS { + println!("cargo::rustc-check-cfg=cfg({cfg})"); + } + + // Build scripts receive the effective rustflags via `CARGO_ENCODED_RUSTFLAGS` + // (unit-separated); fall back to the plain `RUSTFLAGS` string. + let rustflags = std::env::var("CARGO_ENCODED_RUSTFLAGS") + .map(|encoded| encoded.replace('\x1f', " ")) + .or_else(|_| std::env::var("RUSTFLAGS")) + .unwrap_or_default(); + + for (token, cfg) in SANITIZERS { + if rustflags.contains(token) { + println!("cargo::rustc-cfg={cfg}"); + } + } + + println!("cargo::rerun-if-env-changed=CARGO_ENCODED_RUSTFLAGS"); + println!("cargo::rerun-if-env-changed=RUSTFLAGS"); +} + +fn main() { + detect_sanitizers(); +} diff --git a/dataplane/src/main.rs b/dataplane/src/main.rs index 05ac7607e5..05c118d051 100644 --- a/dataplane/src/main.rs +++ b/dataplane/src/main.rs @@ -23,4 +23,6 @@ mod packet_processor; #[cfg(not(feature = "loom"))] mod runtime; #[cfg(not(feature = "loom"))] +mod sanitizer; +#[cfg(not(feature = "loom"))] mod statistics; diff --git a/dataplane/src/sanitizer/mod.rs b/dataplane/src/sanitizer/mod.rs new file mode 100644 index 0000000000..bfcc4a19ee --- /dev/null +++ b/dataplane/src/sanitizer/mod.rs @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +//! Compiled-in sanitizer configuration. +//! +//! Sanitizer runtimes look up a handful of weakly-defined `extern "C"` hooks at +//! startup (e.g. `__tsan_default_suppressions`). By defining strong versions of +//! those hooks we bake our suppression lists straight into the binary: no +//! external suppression file and no `TSAN_OPTIONS=suppressions=...` env var is +//! required to run a sanitized `dataplane`. +//! +//! Each hook is gated on a `cfg` emitted by `build.rs` (which sniffs +//! `-Zsanitizer=` out of the rustflags), so the symbol is only present in +//! the matching sanitizer build. To add suppressions for another sanitizer, +//! register its rustflag token in `build.rs` and add the corresponding hook +//! here. + +#[cfg(sanitize_thread)] +#[unsafe(no_mangle)] +extern "C" fn __tsan_default_suppressions() -> *const core::ffi::c_char { + // Trailing `"\0"` makes this a valid C string: the sanitizer hook returns a raw + // `*const c_char` that the TSan runtime reads until a NUL, and `include_str!` + // alone yields no terminator. `suppress.txt` never contains an interior NUL, so + // the whole file survives to the terminator. + concat!(include_str!("tsan.suppress"), "\0").as_ptr().cast() +} diff --git a/dataplane/src/sanitizer/tsan.suppress b/dataplane/src/sanitizer/tsan.suppress new file mode 100644 index 0000000000..cc1ce328a0 --- /dev/null +++ b/dataplane/src/sanitizer/tsan.suppress @@ -0,0 +1,11 @@ +# tokio's I/O driver runs on its own worker thread(s). When another thread +# registers an I/O source , tokio allocates and initializes a `ScheduledIo`, +# then hands the fd to the kernel via `epoll_ctl`. The driver worker only ever +# sees that `ScheduledIo` after `epoll_wait` returns its token (strictly +# after the registration syscall). That happens-before edge is real but runs +# through the kernel, which ThreadSanitizer cannot instrument, so it reports a +# false positive between RegistrationSet::allocate (init) and +# Driver::turn -> ScheduledIo::{set_readiness,wake} (use). Matching either +# tokio frame silences that pattern without hiding races in our own fd handling. +race:tokio::runtime::io::scheduled_io +race:tokio::runtime::io::registration_set diff --git a/default.nix b/default.nix index 8088e8c04b..ee8abc9d52 100644 --- a/default.nix +++ b/default.nix @@ -32,6 +32,10 @@ let ; }; sanitizers = split-str ",+" sanitize; + # True for any `sanitize=` build (thread/address/leak/cfi/...). Used to bundle + # sanitizer-only tooling (llvm-symbolizer + its debug-info wiring) without + # bloating production images. + is-sanitized = sanitizers != [ ]; cargo-features = split-str ",+" features; profile' = import ./nix/profiles.nix { inherit @@ -215,6 +219,7 @@ let markdownFilter = p: _type: builtins.match ".*\.md$" p != null; jsonFilter = p: _type: builtins.match ".*\.json$" p != null; cHeaderFilter = p: _type: builtins.match ".*\.h$" p != null; + suppressionFilter = p: _type: builtins.match ".*\.suppress$" p != null; outputsFilter = p: _type: (p != "target") && (p != "sysroot") && (p != "devroot") && (p != ".git"); src = pkgs.lib.cleanSourceWith { filter = @@ -226,6 +231,7 @@ let || (markdownFilter p t) || (jsonFilter p t) || (cHeaderFilter p t) + || (suppressionFilter p t) || ((outputsFilter p t) && (craneLib.filterCargoSources full-path t)); src = lib.cleanSource ./.; name = "source"; @@ -637,6 +643,25 @@ let ) package-list; }; + dp-debug = pkgs.buildEnv { + name = "dataplane-debugger-env"; + pathsToLink = [ + "/bin" + "/etc" + "/var" + "/lib" + ]; + paths = [ + pkgs.pkgsHostHost.llvmPackages'.llvm.lib + pkgs.pkgsHostHost.llvmPackages'.llvm + pkgs.pkgsHostHost.libc.debug + pkgs.pkgsHostHost.libxml2 + workspace.cli.debug + workspace.dataplane.debug + workspace.init.debug + ]; + }; + dataplane.tar = pkgs.stdenv'.mkDerivation { pname = "dataplane.tar"; inherit version; @@ -673,10 +698,16 @@ let # busybox applets referencing a `ld-musl-*.so.1` / `libc.so` that # isn't present in the image. libc-tar-input = "${libc-pkg.out}"; + # Compiler-rt sanitizers (TSan/ASan/...) shell out to an external + # `llvm-symbolizer` to turn `binary+0xoffset` frames into + # function/file:line. Bundled only for sanitizer builds (see the + # `is-sanitized` block in buildPhase). Pinned to the LLVM our rustc + # uses via `llvmPackages'`. + llvm-symbolizer-pkg = pkgs.pkgsHostHost.llvmPackages'.llvm; in '' tmp="$(mktemp -d)" - mkdir -p "$tmp/"{bin,lib,var,etc,run/dataplane,run/frr/hh,run/netns,home,tmp} + mkdir -p "$tmp/"{bin,var,etc,run/dataplane,run/frr/hh,run/netns,home,tmp} ln -s /run "$tmp/var/run" for f in "${pkgs.pkgsHostHost.dockerTools.fakeNss}/etc/"* ; do cp --archive "$(readlink -e "$f")" "$tmp/etc/$(basename "$f")" @@ -691,6 +722,31 @@ let ln -s "${workspace.dataplane}/bin/dataplane" "$tmp/dataplane" ln -s "${workspace.init}/bin/dataplane-init" "$tmp/dataplane-init" ln -s "${workspace.cli}/bin/cli" "$tmp/dataplane-cli" + ln -s "${dp-debug}/lib" "$tmp/lib" + ${lib.optionalString is-sanitized '' + # Sanitizer builds rely on an external `llvm-symbolizer` to turn + # `dataplane+0xoffset` frames into function/file:line. compiler-rt + # auto-discovers a binary literally named `llvm-symbolizer` on $PATH, + # so drop a wrapper at /bin/llvm-symbolizer that also points it at the + # debug-info search dirs. No env var / image-config change needed. + # + # `/lib/debug` already carries the per-dependency build-id tree (libc, + # etc.) via dp-debug. The workspace binaries are stripped with a + # gnu_debuglink (see postInstall), so mirror their full-DWARF debug + # files where llvm-symbolizer's debuglink lookup expects them: + # // + # These must be the exact files `objcopy --add-gnu-debuglink` ran + # against, or the embedded CRC check rejects them. + mkdir -p "$tmp/usr/lib/debug${workspace.dataplane}/bin" + ln -s "${workspace.dataplane.debug}/bin/dataplane" "$tmp/usr/lib/debug${workspace.dataplane}/bin/dataplane" + mkdir -p "$tmp/usr/lib/debug${workspace.cli}/bin" + ln -s "${workspace.cli.debug}/bin/cli" "$tmp/usr/lib/debug${workspace.cli}/bin/cli" + mkdir -p "$tmp/usr/lib/debug${workspace.init}/bin" + ln -s "${workspace.init.debug}/bin/dataplane-init" "$tmp/usr/lib/debug${workspace.init}/bin/dataplane-init" + echo '#!/bin/sh' > "$tmp/bin/llvm-symbolizer" + echo 'exec ${llvm-symbolizer-pkg}/bin/llvm-symbolizer --debug-file-directory=/usr/lib/debug --debug-file-directory=/lib/debug "$@"' >> "$tmp/bin/llvm-symbolizer" + chmod +x "$tmp/bin/llvm-symbolizer" + ''} # we take some care to make the tar file reproducible here tar \ --create \ @@ -755,7 +811,10 @@ let ${workspace.dataplane} \ ${workspace.init} \ ${workspace.cli} \ - ${pkgs.pkgsHostHost.busybox} + ${pkgs.pkgsHostHost.busybox} \ + ${pkgs.perf} \ + ${lib.optionalString is-sanitized "${llvm-symbolizer-pkg}"} \ + ${dp-debug} ''; }; @@ -771,15 +830,22 @@ let "/lib" ]; paths = [ - pkgs.pkgsHostHost.dockerTools.fakeNss pkgs.pkgsHostHost.busybox + pkgs.pkgsHostHost.dockerTools.fakeNss pkgs.pkgsHostHost.dockerTools.usrBinEnv + pkgs.pkgsHostHost.libc.debug + pkgs.pkgsHostHost.llvmPackages'.llvm + pkgs.pkgsHostHost.llvmPackages'.llvm.lib workspace.cli + # workspace.cli.debug workspace.dataplane + # workspace.dataplane.debug workspace.init + # workspace.init.debug ]; }; - config.Entrypoint = [ "/bin/dataplane" ]; + # config.Entrypoint = [ "/bin/perf" "-o" "/blah/dataplane-perf.data" "/bin/dataplane" ]; + config.Entrypoint = [ "/dataplane" ]; }; containers.dataplane-debugger = pkgs.dockerTools.buildLayeredImage { From b320bba13d47813d8b024171ebf8b4013647580f Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 24 Jul 2026 13:46:33 -0600 Subject: [PATCH 2/5] build: include frame pointers This can give us better stack traces in sanitized builds. Signed-off-by: Daniel Noland --- nix/profiles.nix | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/nix/profiles.nix b/nix/profiles.nix index a3e24879ec..435d779d5c 100644 --- a/nix/profiles.nix +++ b/nix/profiles.nix @@ -131,6 +131,7 @@ let march.wasm32 = { }; sanitize.address.NIX_CFLAGS_COMPILE = [ "-fsanitize=address,local-bounds" + "-fno-omit-frame-pointer" ]; sanitize.address.NIX_CXXFLAGS_COMPILE = sanitize.address.NIX_CFLAGS_COMPILE; sanitize.address.NIX_CFLAGS_LINK = sanitize.address.NIX_CFLAGS_COMPILE ++ [ @@ -139,6 +140,7 @@ let sanitize.address.RUSTFLAGS = [ "-Zsanitizer=address" "-Zexternal-clangrt" + "-Cforce-frame-pointers=yes" ] ++ (map (flag: "-Clink-arg=${flag}") sanitize.address.NIX_CFLAGS_LINK); sanitize.leak.NIX_CFLAGS_COMPILE = [ @@ -149,10 +151,12 @@ let sanitize.leak.RUSTFLAGS = [ "-Zsanitizer=leak" "-Zexternal-clangrt" + "-Cforce-frame-pointers=yes" ] ++ (map (flag: "-Clink-arg=${flag}") sanitize.leak.NIX_CFLAGS_LINK); sanitize.thread.NIX_CFLAGS_COMPILE = [ "-fsanitize=thread" + "-fno-omit-frame-pointer" ]; sanitize.thread.NIX_CXXFLAGS_COMPILE = sanitize.thread.NIX_CFLAGS_COMPILE; sanitize.thread.NIX_CFLAGS_LINK = sanitize.thread.NIX_CFLAGS_COMPILE ++ [ @@ -163,6 +167,7 @@ let "-Zexternal-clangrt" # gimli doesn't like thread sanitizer, but it shouldn't be an issue since that is all build time logic "-Cunsafe-allow-abi-mismatch=sanitizer" + "-Cforce-frame-pointers=yes" ] ++ (map (flag: "-Clink-arg=${flag}") sanitize.thread.NIX_CFLAGS_LINK); # note: cfi _requires_ LTO and is fundamentally ill suited to debug builds From ae31907dcee61b37e80da7e142d75761ffd82acd Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 24 Jul 2026 13:47:37 -0600 Subject: [PATCH 3/5] ci: build thread sanitized container This is NOT for distribution to customers. Instead, this is intended for use in our labs to help hunt down locking issues. Signed-off-by: Daniel Noland --- .github/workflows/dev.yml | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/.github/workflows/dev.yml b/.github/workflows/dev.yml index 430b650d87..7e008670ae 100644 --- a/.github/workflows/dev.yml +++ b/.github/workflows/dev.yml @@ -133,7 +133,7 @@ jobs: strategy: fail-fast: false matrix: - build: &default-build + build: - &debug-build name: "debug" profile: "debug" @@ -335,7 +335,19 @@ jobs: - frr.dataplane - dataplane - validator - build: *default-build + build: + - name: "debug" + profile: "debug" + sanitize: "" + instrument: "none" + - name: "release" + profile: "release" + sanitize: "" + instrument: "none" + - name: "thread" + profile: "fuzz" + sanitize: "thread" + instrument: "none" exclude: - nix-target: validator build: From 57fcac317c3812d0a649ba0e096e8323c92ed3d1 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 24 Jul 2026 14:16:47 -0600 Subject: [PATCH 4/5] ci: run vlab against the thread sanitized dataplane Adds an opt-in `vlab_tsan` job that points VLAB at the thread-sanitized image the `build` matrix already produces. Gated on the `ci:+tsan` label or a workflow_dispatch input, and deliberately left out of `summary`'s `needs`: this is a lab instrument for hunting locking issues, so a race report should start an investigation rather than block a PR. The only seam into the reusable VLAB workflow is `prebuild`, which runs as an ordinary step in the same job as `hhfab vlab up`. That is enough: it can bump the dataplane to the sanitized tag and raise the gateway VM RAM via `$GITHUB_ENV` (the default 6144 MB is unlikely to survive TSan's shadow memory, and the container has no memory limit, so the VM is the ceiling). `collect_show_tech` is on because a race that TSan reports without killing anything is a *passing* run, and show-tech is what captures the container's stderr. Three things had to change to make that tag reachable: Container tags now carry a sanitizer suffix. The profile alone does not identify a build -- `fuzz` is shared by every sanitizer variant -- so thread and address builds would otherwise collide. This mirrors the `version_san` suffix the justfile already computes locally. Only the plain release build writes the bare `${VERSION}` tag. All three matrix cells were writing it, so its contents were decided by whichever job finished last. The validator is built unsanitized but still published under the variant tag. rustc has no sanitizer support for `wasm32-wasip1`, and `nix/platforms.nix` does not filter sanitizers per platform, so the flags reached rustc and failed the build. Skipping the cell is not an option: fabricator derives the validator ref from the single `DataplaneVersion` field, so pointing that at a sanitized dataplane makes hhfab mirror a matching `dataplane/validator` tag that has to exist. Finally, `__tsan_default_options` sets `exitcode=0`. TSan's default of 66 turns "we found a race" into a DaemonSet restart loop, which destroys the evidence and disguises the finding as a liveness failure. The hook lives next to the existing suppressions hook because the DaemonSet's environment is fixed by the gateway controller, leaving no seam for `TSAN_OPTIONS`. Signed-off-by: Daniel Noland --- .github/workflows/dev.yml | 100 ++++++++++++++++++++++++++++++++- dataplane/src/sanitizer/mod.rs | 45 ++++++++++++++- 2 files changed, 140 insertions(+), 5 deletions(-) diff --git a/.github/workflows/dev.yml b/.github/workflows/dev.yml index 7e008670ae..dbd153ce36 100644 --- a/.github/workflows/dev.yml +++ b/.github/workflows/dev.yml @@ -46,6 +46,11 @@ on: description: "Enable release tests for VLAB/HLAB tests" required: false default: false + run_tsan_vlab: + type: "boolean" + description: "Run VLAB against a thread-sanitized dataplane (slow; lab instrument, not a gate)" + required: false + default: false concurrency: group: "${{ github.workflow }}:${{ github.event.pull_request.number || github.event.after || github.event.merge_group && github.run_id }}" @@ -366,8 +371,22 @@ jobs: DEBUG_JUSTFILE: "${{ github.event_name == 'workflow_dispatch' && github.event.inputs.debug_justfile || false }}" run: | set -euo pipefail + sanitize="${SANITIZE}" if [ "${NIX_TARGET}" = "validator" ]; then platform="wasm32-wasip1" + # rustc has no sanitizer support for `wasm32-wasip1` ("thread + # sanitizer is not supported for this target"), and + # `nix/platforms.nix` does not filter sanitizers per platform, so + # the flags would reach rustc and fail the build. A wasm module has + # nothing to sanitize in any case. + # + # We build it unsanitized rather than skipping the cell, because + # fabricator derives the validator ref from the single + # `DataplaneVersion` field (`comp/gateway.Artifacts`, mirrored via + # `AirgapArtifactsGateway`). Pointing that field at a sanitized + # dataplane tag makes hhfab mirror `dataplane/validator:` + # too -- so the tag has to exist or VLAB dies before it boots. + sanitize="" else platform="x86-64-v3" fi @@ -376,12 +395,27 @@ jobs: debug_justfile="${DEBUG_JUSTFILE}" profile="${PROFILE}" platform="${platform}" - sanitize="${SANITIZE}" + sanitize="${sanitize}" instrument="${INSTRUMENT}" oci_repo=ghcr.io ) - just "${base_args[@]}" push-container "${NIX_TARGET}" - just "${base_args[@]}" "version=${VERSION}-${PROFILE}" push-container "${NIX_TARGET}" + # The profile alone does not identify a build: `fuzz` is shared by + # every sanitizer variant, so a thread- and an address-sanitized + # image would land on the same tag. Mirror the suffix the justfile + # already uses for locally-computed versions (`version_san`). + san_suffix="" + if [ -n "${SANITIZE}" ]; then + san_suffix="-san.${SANITIZE//,/.}" + fi + # The bare `${VERSION}` tag names a single image per commit, so only + # one matrix cell may write it -- otherwise debug, release and thread + # race and the winner is whichever job happens to finish last. Give it + # to the plain release build, which is what every consumer that does + # not ask for a variant means. + if [ -z "${SANITIZE}" ] && [ "${PROFILE}" = "release" ]; then + just "${base_args[@]}" "version=${VERSION}" push-container "${NIX_TARGET}" + fi + just "${base_args[@]}" "version=${VERSION}-${PROFILE}${san_suffix}" push-container "${NIX_TARGET}" - *verify-clean-tree - *tmate @@ -756,6 +790,66 @@ jobs: hybrid: true upgradefrom: "" + # Run VLAB against the thread-sanitized dataplane image built by the `build` + # matrix's "thread" cell. + # + # This is a lab instrument for hunting locking issues, NOT a merge gate, and + # it is deliberately absent from `summary`'s `needs`: a ThreadSanitizer report + # here should start an investigation, not block someone's PR. It is also + # opt-in (label / dispatch) because a sanitized build is slow enough that + # running it on every PR would be a poor trade. + # + # Everything the sanitized run needs beyond the base VLAB job is carried by + # `prebuild`, which the reusable workflow executes as an ordinary step in the + # same job as `hhfab vlab up`. That means it can export settings to later + # steps via `$GITHUB_ENV` -- the only seam available, since a reusable + # workflow cannot inherit the caller's `env`. + vlab_tsan: + if: >- + ${{ + github.event_name == 'pull_request' + && contains(github.event.pull_request.labels.*.name, 'ci:+tsan') + || github.event_name == 'workflow_dispatch' && inputs.run_tsan_vlab + }} + needs: + - version + - build + + name: "v-tsan-gw-iso-l2vni" + + uses: githedgehog/fabricator/.github/workflows/run-vlab.yaml@master + with: + fabricatorref: master + # `HHFAB_VLAB_GW_RAM` is the env var behind `hhfab vlab up --gateway-ram`. + # The default is 6144 MB, which ThreadSanitizer's shadow memory is likely + # to exhaust: the container has no memory limit, so the VM is the ceiling + # and the failure mode is an OOM kill that looks like an unrelated crash. + # 12288 is a starting guess -- tune it from what the first run actually + # uses. Note this applies to each of the two gateway VMs. + # + # Only the dataplane is switched to the sanitized image. FRR is built with + # the same `stdenv'` and so is also instrumented at the `-fuzz-san.thread` + # tag, but sanitizing both at once doubles the slowdown and the noise for + # no extra signal about our own locking. It still has to be bumped to this + # commit's release build: leaving it alone would silently pin FRR to the + # last released version and put a stale frr-agent on the other end of CPI. + prebuild: | + echo "HHFAB_VLAB_GW_RAM=12288" >> "$GITHUB_ENV" + just bump dataplane ${{ needs.version.outputs.version }}-fuzz-san.thread + just bump frr ${{ needs.version.outputs.version }}-release + fabricmode: spine-leaf + gateway: true + includeonie: false + buildmode: iso + vpcmode: l2vni + hybrid: false + upgradefrom: "" + # The whole point of the run is the report, and `show-tech` is what + # captures the dataplane container's stderr (`crictl logs`) into the + # uploaded artifact. Without this it is only collected on failure -- but a + # race that TSan reports without killing anything is a *passing* run. + collect_show_tech: true + summary: name: "Summary" runs-on: "ubuntu-latest" diff --git a/dataplane/src/sanitizer/mod.rs b/dataplane/src/sanitizer/mod.rs index bfcc4a19ee..0aa344b2dc 100644 --- a/dataplane/src/sanitizer/mod.rs +++ b/dataplane/src/sanitizer/mod.rs @@ -5,15 +5,26 @@ //! //! Sanitizer runtimes look up a handful of weakly-defined `extern "C"` hooks at //! startup (e.g. `__tsan_default_suppressions`). By defining strong versions of -//! those hooks we bake our suppression lists straight into the binary: no -//! external suppression file and no `TSAN_OPTIONS=suppressions=...` env var is +//! those hooks we bake our suppression lists and runtime options straight into +//! the binary: no external suppression file and no `TSAN_OPTIONS=...` env var is //! required to run a sanitized `dataplane`. //! +//! That matters because the sanitized image is deployed by machinery we do not +//! own. In VLAB the `dataplane` container is a `DaemonSet` whose environment is +//! fixed by the gateway controller, so there is no seam through which to pass +//! `TSAN_OPTIONS`. Keeping the configuration next to the code it configures +//! also keeps it reviewable in one place rather than spread across three +//! repositories (see `development/code/avoid-global-reasoning.md`). +//! //! Each hook is gated on a `cfg` emitted by `build.rs` (which sniffs //! `-Zsanitizer=` out of the rustflags), so the symbol is only present in //! the matching sanitizer build. To add suppressions for another sanitizer, //! register its rustflag token in `build.rs` and add the corresponding hook //! here. +//! +//! Note that the runtime still reads the `TSAN_OPTIONS` environment variable, +//! and anything set there overrides what `__tsan_default_options` returns. These +//! are defaults, not a policy you have to fight. #[cfg(sanitize_thread)] #[unsafe(no_mangle)] @@ -24,3 +35,33 @@ extern "C" fn __tsan_default_suppressions() -> *const core::ffi::c_char { // the whole file survives to the terminator. concat!(include_str!("tsan.suppress"), "\0").as_ptr().cast() } + +/// Default `TSAN_OPTIONS` for a sanitized `dataplane`. +/// +/// The runtime parses the `TSAN_OPTIONS` environment variable *after* this +/// string, so an operator can still override any of these at run time. +#[cfg(sanitize_thread)] +#[unsafe(no_mangle)] +extern "C" fn __tsan_default_options() -> *const core::ffi::c_char { + // `exitcode=0` is the load-bearing one. TSan's default is to exit 66 once a + // race has been reported, which for a long-lived daemon under a supervisor + // (k8s `DaemonSet`, systemd, ...) turns "we found a race" into a restart + // loop. The restart both destroys the evidence -- `crictl logs` without + // `-p` only shows the current container -- and disguises the finding as an + // unrelated liveness failure. We would rather keep running and leave the + // report on stderr where log collection can find it. + // + // `halt_on_error=0` is already the default; it is spelled out because it is + // the other half of the same intent and we do not want a future runtime + // default flip to change behavior silently. + // + // Deliberately *not* set here: + // - `log_path`: reports must go to stderr so they land in container logs. + // - `history_size`: raising it buys deeper "previous access" stacks at a + // real memory cost, and memory is the binding constraint on the VLAB + // gateway VM. Set it via the env var when you need the depth. + // - `detect_deadlocks=1` (plus `second_deadlock_stack=1`): on-topic for + // lock-order hunting, but the detector is noisy enough that it should + // be an explicit opt-in rather than baked into every sanitized image. + c"exitcode=0 halt_on_error=0".as_ptr().cast() +} From 3d7b8796997e965900b373d89548ef494ae5005d Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 24 Jul 2026 14:57:04 -0600 Subject: [PATCH 5/5] ci: comment out temp setting Need to chat with Sergei before we do any of that Signed-off-by: Daniel Noland --- .github/workflows/dev.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/dev.yml b/.github/workflows/dev.yml index dbd153ce36..734f1fa080 100644 --- a/.github/workflows/dev.yml +++ b/.github/workflows/dev.yml @@ -833,8 +833,10 @@ jobs: # no extra signal about our own locking. It still has to be bumped to this # commit's release build: leaving it alone would silently pin FRR to the # last released version and put a stale frr-agent on the other end of CPI. + # + # temp comment out till I talk to Sergei + # echo "HHFAB_VLAB_GW_RAM=12288" >> "$GITHUB_ENV" prebuild: | - echo "HHFAB_VLAB_GW_RAM=12288" >> "$GITHUB_ENV" just bump dataplane ${{ needs.version.outputs.version }}-fuzz-san.thread just bump frr ${{ needs.version.outputs.version }}-release fabricmode: spine-leaf