moonchen commented on code in PR #13328: URL: https://github.com/apache/trafficserver/pull/13328#discussion_r3608939907
########## src/iocore/cache/CacheShm.cc: ########## @@ -0,0 +1,776 @@ +/** @file + + Shared-memory-backed cache directory for fast restart. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include "CacheShm.h" +#include "CacheShmLayout.h" +#include "CacheShmPurge.h" + +#include "P_CacheDir.h" +#include "iocore/cache/Store.h" + +#include "records/RecCore.h" +#include "tscore/Diags.h" +#include "tscore/HashFNV.h" +#include "tscore/hugepages.h" +#include "tscore/ink_align.h" +#include "tscore/ink_memory.h" +#include "tscore/ink_string.h" +#include "tsutil/DbgCtl.h" + +#include <fcntl.h> +#include <sys/file.h> +#include <sys/mman.h> +#include <sys/stat.h> +#include <unistd.h> + +#include <csignal> +#include <cstdint> +#include <cstring> +#include <mutex> +#include <string> +#include <unordered_map> + +namespace +{ + +DbgCtl dbg_ctl{"cache_shm"}; + +using cache_shm::CACHE_SHM_MAGIC; +using cache_shm::CACHE_SHM_SCHEMA_VERSION; +using cache_shm::CacheShmControl; +using cache_shm::control_segment_name; +using cache_shm::CONTROL_SIZE; +using cache_shm::LockResult; +using cache_shm::MAX_SHM_NAME_LEN; +using cache_shm::MAX_STRIPES; +using cache_shm::read_shm_name; +using cache_shm::StripeEntry; +using cache_shm::try_lock_control; + +// Sanity bound: the control struct (header + stripe table) must stay small. +constexpr std::size_t MAX_CONTROL_SEGMENT_BYTES = 32 * 1024; +static_assert(sizeof(CacheShmControl) <= MAX_CONTROL_SEGMENT_BYTES, "control segment unexpectedly large"); + +// Configuration loaded at initialize() time. +struct Config { + bool enabled = false; + bool use_hugepages = false; + bool purge_stale_on_start = false; + std::string name_prefix = "/ats-"; // normalized "/<word>-" (see normalize_name_prefix); set in load_config. +}; + +Config g_config; + +// Live state for the open control segment. +CacheShmControl *g_control = nullptr; +std::string g_control_name; + +// Holds the control segment's exclusive flock for the process lifetime; the OS +// releases it on exit. Only set on the path that owns the segment. +ats_scoped_fd g_control_fd; + +// shm pointers we returned (mapped to their length), so the Stripe destructor can +// choose munmap vs ats_free and detach_stripe can munmap the right span. +std::mutex g_pointers_mutex; +std::unordered_map<char *, std::size_t> g_pointers; + +// Guards the control-segment stripe table and the per-run claim bookkeeping below +// (stripes initialize concurrently across disk threads). +std::mutex g_table_mutex; + +// Per-run partial-attach bookkeeping, indexed in lockstep with g_control->stripes[]. +// An entry still unclaimed once init completes is an orphan reclaimed by +// finalize_attach(). Process-local, reset each run. +bool g_entry_claimed[MAX_STRIPES] = {}; +uint32_t g_claims_this_run = 0; + +void +fnv_update(ATSHash64FNV1a &h, uint64_t v) +{ + h.update(&v, sizeof v); +} + +/// Full 64-bit stripe identity used to match a stripe to its prior shm segment. +uint64_t +compute_stripe_key_hash(const char *stripe_key) +{ + ATSHash64FNV1a hash; + hash.update(stripe_key, std::strlen(stripe_key)); + return hash.get(); +} + +/// Build a stripe shm name from its per-host index (unique, so names never +/// collide). Matching to a prior segment uses the key hash, not this name. +std::string +build_stripe_shm_name(const std::string &prefix, uint32_t stripe_index) +{ + std::string name = prefix + "s" + std::to_string(stripe_index); + if (name.size() >= MAX_SHM_NAME_LEN) { + name.resize(MAX_SHM_NAME_LEN - 1); + } + return name; +} + +// Named flags for open_and_map_shm so call sites read `ShmAccess::Create` / +// `HugePages::Off` and the two can't be transposed. +enum class ShmAccess { Open, Create }; +enum class HugePages { Off, On }; + +/// Open or create a shm segment of `size` bytes and mmap it; nullptr on failure. +/// `out_fd` (if set) receives the still-open fd so the caller can flock it, else the +/// fd is closed (the mapping survives). `out_errno` (if set) receives the failing +/// syscall's errno for diagnostics. +void * +open_and_map_shm(const std::string &name, std::size_t size, ShmAccess access, [[maybe_unused]] HugePages hugepages, + int *out_fd = nullptr, int *out_errno = nullptr) +{ + if (out_errno != nullptr) { + *out_errno = 0; + } + int oflags = O_RDWR; + if (access == ShmAccess::Create) { + // O_EXCL so a create never adopts a pre-existing (attacker-planted) object. + oflags |= O_CREAT | O_EXCL; + } + + ats_scoped_fd fd{shm_open(name.c_str(), oflags, 0600)}; + if (fd < 0) { + int e = errno; + Dbg(dbg_ctl, "shm_open(%s, %s) failed: %s", name.c_str(), access == ShmAccess::Create ? "create" : "open", strerror(e)); + if (out_errno != nullptr) { + *out_errno = e; + } + return nullptr; + } + + if (access == ShmAccess::Create) { + if (ftruncate(fd, size) < 0) { + int e = errno; + Warning("ftruncate(%s, %zu) failed: %s", name.c_str(), size, strerror(e)); + shm_unlink(name.c_str()); + if (out_errno != nullptr) { + *out_errno = e; + } + return nullptr; + } + } else { + // The kernel rounds shm size up to a page boundary (16 KiB on macOS / Apple + // Silicon), so accept any size in [requested, page-up]. + struct stat sb { + }; + std::size_t expected_max = INK_ALIGN(size, ats_pagesize()); + if (fstat(fd, &sb) < 0 || sb.st_size < 0 || static_cast<std::size_t>(sb.st_size) < size || + static_cast<std::size_t>(sb.st_size) > expected_max) { Review Comment: This wedges shm off permanently after an upgrade that changes `sizeof(CacheShmControl)` (a MAX_STRIPES bump, a longer shm_name, a new StripeEntry field): the open fails here, `initialize()` never reaches the abi_hash drop path (that needs a successful map), and the O_EXCL create then fails with EEXIST on every restart -- until someone runs `traffic_ctl cache shm clear`. The CacheShmLayout.h comment ("Bumping it changes the ABI hash, so old segments are dropped automatically") only holds for same-size changes. Could `initialize()` treat a wrong-size (or any non-ENOENT) open failure as a stale segment -- take the flock/owner-pid guard, unlink, and fall through to the fresh create? (Alternatively: a fixed-size preamble that is mapped and checked before the full-size map, so the version fields stay readable across layout changes.) ########## src/iocore/cache/Stripe.cc: ########## @@ -170,6 +178,52 @@ Stripe::_init_directory(std::size_t directory_size, int header_size, int footer_ this->directory.footer = reinterpret_cast<StripeHeaderFooter *>(this->directory.raw_dir + footer_offset); } +// Bounds-check the trusted header/freelist fields of an in-shm directory before +// the fast-restart attach (magic/version are already checked by the caller). A +// stale-but-magic-valid segment could present out-of-range offsets that become OOB +// disk I/O. On failure the caller falls through to the disk read + recover_data(). +// +// Trust model: the shm segment is trusted to the same degree as the on-disk +// directory (same-uid, mode 0600). Stripe geometry (segments/buckets) is recomputed +// locally each run and raw_dir_size is exact-matched before attach, so this only +// validates the header cursor fields and per-segment freelist heads; it does not +// re-validate individual Dir entries -- the read path already checks Doc magic + key +// before serving, so a stale entry resolves to a miss, never served corruption. +bool +Stripe::_shm_directory_is_valid() const +{ + // sector_size must be sane geometry (mirrors the hw_sector_size range in Cache.cc). + if (this->directory.header->sector_size == 0 || this->directory.header->sector_size > STORE_BLOCK_SIZE) { + return false; + } + + // phase is a single bit of write-cursor state; only 0/1 are valid. + if (this->directory.header->phase > 1) { + return false; + } + + // write_pos/last_write_pos/agg_pos must point into the data region. + const off_t data_lo = this->start; + const off_t data_hi = this->skip + this->len; + + if (this->directory.header->write_pos < data_lo || this->directory.header->write_pos > data_hi || + this->directory.header->last_write_pos < data_lo || this->directory.header->last_write_pos > data_hi || + this->directory.header->agg_pos < data_lo || this->directory.header->agg_pos > data_hi) { + return false; + } Review Comment: One optional suggestion: a clean shutdown also guarantees the write cursor is quiesced (`flush_aggregate_write_buffer` and `aggWriteDone` both maintain it, and an in-flight AIO invalidates the segment), so the validator can require it -- the runtime asserts that would otherwise catch this are debug-only. ```suggestion } // A clean shutdown leaves the write cursor quiesced; a mismatch means this is // not a clean-shutdown artifact. if (this->directory.header->agg_pos != this->directory.header->write_pos || this->directory.header->last_write_pos > this->directory.header->write_pos) { return false; } ``` ########## src/traffic_ctl/CacheShmCommand.cc: ########## @@ -0,0 +1,266 @@ +/** @file + + traffic_ctl command for inspecting and clearing the cache shared-memory + control segment and its associated stripe segments. + + The status and clear operations work by direct shm_open access rather than + JSONRPC, so they function whether traffic_server is running or not. This + is important for debugging crash-leftover segments when no live process + is available to query. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include "CacheShmCommand.h" +#include "CacheShmLayout.h" +#include "CacheShmPurge.h" +#include "TrafficCtlStatus.h" + +#include <fcntl.h> +#include <sys/mman.h> +#include <sys/stat.h> +#include <unistd.h> + +#include <cerrno> +#include <cstdio> +#include <cstring> +#include <iostream> + +namespace +{ + +// The middle word of the shm name prefix when --prefix is not given. The +// framing "/<word>-" is supplied by cache_shm::normalize_name_prefix, matching +// the server's proxy.config.cache.shm.name_prefix default. +constexpr const char *DEFAULT_PREFIX = "ats"; + +bool +shm_segment_exists(const std::string &name) +{ + int fd = shm_open(name.c_str(), O_RDONLY, 0); + if (fd < 0) { + return false; + } + close(fd); + return true; +} + +std::string +format_size(uint64_t bytes) +{ + char buf[64]; + if (bytes >= (uint64_t{1} << 30)) { + std::snprintf(buf, sizeof(buf), "%.2f GiB", static_cast<double>(bytes) / (uint64_t{1} << 30)); + } else if (bytes >= (uint64_t{1} << 20)) { + std::snprintf(buf, sizeof(buf), "%.2f MiB", static_cast<double>(bytes) / (uint64_t{1} << 20)); + } else if (bytes >= (uint64_t{1} << 10)) { + std::snprintf(buf, sizeof(buf), "%.2f KiB", static_cast<double>(bytes) / (uint64_t{1} << 10)); + } else { + std::snprintf(buf, sizeof(buf), "%llu B", static_cast<unsigned long long>(bytes)); + } + return buf; +} + +// Shared with the cache subsystem (CacheShmPurge.h): read_shm_name bounds a +// possibly-unterminated name field, process_is_alive backs the owner-liveness gate. +using cache_shm::process_is_alive; +using cache_shm::read_shm_name; + +} // namespace + +CacheShmCommand::CacheShmCommand(ts::Arguments *args) : CtrlCommand(args) +{ + if (get_parsed_arguments()->get(STATUS_STR)) { + _invoked_func = [this]() { status(); }; + } else if (get_parsed_arguments()->get(CLEAR_STR)) { + _invoked_func = [this]() { clear(); }; + } +} + +std::string +CacheShmCommand::get_prefix() +{ + // The operator gives only the middle word (e.g. --prefix ats); frame it the + // same way the server does so the two agree on segment names. + std::string configured = DEFAULT_PREFIX; + if (auto arg = get_parsed_arguments()->get(PREFIX_STR); arg && !arg.empty()) { + configured = arg.value(); + } + return cache_shm::normalize_name_prefix(configured); +} + +void +CacheShmCommand::status() +{ + const std::string prefix = get_prefix(); + const std::string control_name = cache_shm::control_segment_name(prefix); + + int fd = shm_open(control_name.c_str(), O_RDONLY, 0); + if (fd < 0) { + std::cerr << "cache shm: control segment '" << control_name << "' not found: " << std::strerror(errno) << '\n'; Review Comment: One optional suggestion: an operator with a custom `proxy.config.cache.shm.name_prefix` gets "not found" against the default `ats` prefix and may conclude shm was never enabled. Worth a hint here (and in `clear()`'s NotPresent message): ```suggestion std::cerr << "cache shm: control segment '" << control_name << "' not found: " << std::strerror(errno) << " (if proxy.config.cache.shm.name_prefix is set, pass --prefix <word>)\n"; ``` ########## src/iocore/cache/Stripe.cc: ########## @@ -170,6 +178,52 @@ Stripe::_init_directory(std::size_t directory_size, int header_size, int footer_ this->directory.footer = reinterpret_cast<StripeHeaderFooter *>(this->directory.raw_dir + footer_offset); } +// Bounds-check the trusted header/freelist fields of an in-shm directory before +// the fast-restart attach (magic/version are already checked by the caller). A +// stale-but-magic-valid segment could present out-of-range offsets that become OOB +// disk I/O. On failure the caller falls through to the disk read + recover_data(). +// +// Trust model: the shm segment is trusted to the same degree as the on-disk +// directory (same-uid, mode 0600). Stripe geometry (segments/buckets) is recomputed +// locally each run and raw_dir_size is exact-matched before attach, so this only +// validates the header cursor fields and per-segment freelist heads; it does not +// re-validate individual Dir entries -- the read path already checks Doc magic + key +// before serving, so a stale entry resolves to a miss, never served corruption. +bool +Stripe::_shm_directory_is_valid() const Review Comment: Since these rejection code branches are critical for preventing cache corruption, I think they should be tested. `shm_poke.py` can poke the stripe segment header in /dev/shm after a clean shutdown (e.g. set `write_pos` past the stripe, or `freelist[0]` out of range); a restart should then log "shm directory invalid ... falling back to disk read" and still serve a HIT after disk recovery. ########## src/iocore/cache/StripeSM.cc: ########## @@ -178,6 +179,26 @@ StripeSM::init(bool clear) return clear_dir_aio(); } + // shm fast restart: a clean shutdown left the in-shm directory authoritative, so + // skip both the disk read and recover_data() (which would re-scan the tail and + // discard the entries the shm copy preserved). After validating the in-shm + // header/footer, jump straight to dir_init_done() in the normal post-recovery + // state. Validation failure falls through to disk read + recover_data(). + if (CacheShm::mode() == CacheShm::Mode::AttachExisting && CacheShm::is_shm_pointer(this->directory.raw_dir)) { + if (this->directory.header->magic == STRIPE_MAGIC && this->directory.footer->magic == STRIPE_MAGIC && Review Comment: `mark_clean_shutdown()` runs while event threads are still live, and the main thread exits without joining them (the wait loop in traffic_server.cc wakes on the same flag and calls exit) -- so a dir mutation torn at process exit can leave a structurally inconsistent directory behind a `clean_shutdown=1` flag. Since there's no cheap way to quiesce writers first, could the fast path also run `Directory::check()` (the CHECK_DIR walk: bucket bounds, chain consistency, loop detection) and fall back to the disk read when it fails? It's an in-memory walk -- small next to the rebuild it replaces -- and it turns a torn dir into a rebuild instead of an attach. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
