diff --git a/docs/testing.md b/docs/testing.md index ca904714..c25c22c2 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -91,6 +91,11 @@ What they do: - `make check`: fast elfuse-internal gate. Runs, in order: - `scripts/check-syscall-coverage.py` so any new `dispatch.tbl` entry without a direct or aliased test reference fails the build + - `scripts/check-lock-order.py` so a new file-scope `pthread_mutex_t` + or `pthread_rwlock_t` that the lock-ordering block at the top of + `src/syscall/internal.h` does not name fails the build. Membership + only: whether the lock belongs in the ordered list or the leaf list + stays a judgement for review - the unit suite from `tests/manifest.txt` -- deliberately narrow: the elfuse-internal implementation tests with no real Linux counterpart (the EL1 shim fast-path suite, `test-mremap-infra`, `test-mremap-fork-tracking`, diff --git a/mk/config.mk b/mk/config.mk index 72c648ce..4e0f2943 100644 --- a/mk/config.mk +++ b/mk/config.mk @@ -70,6 +70,46 @@ CFLAGS := -O2 -Wall -Wextra -Wpedantic \ -Wnull-dereference -Wno-unused-parameter CFLAGS += $(EXTRA_CFLAGS) +# Warnings are errors, because this tree has none and the gates around it (the +# syscall coverage check, the EINTR contract, the proof targets) all fail the +# build rather than print. A compiler diagnostic that only prints is the one +# signal here that a reader has to notice on their own. +# +# WERROR=0 turns it off, which is what a newer compiler with a new warning +# wants: the flag must not be the reason a fresh clone stops building. CI keeps +# the default so the new warning still gets found. +WERROR ?= 1 +ifeq ($(WERROR),1) +CFLAGS += -Werror +endif + +# Hardening. This process parses input the guest fully controls (its ELF, every +# syscall argument, FUSE frames, netlink messages, sockaddr and cmsg blobs), so +# the cheap compiler-side checks are worth their cost here even though the +# bounds math itself is proved in src/proved/. PIE is already the Darwin +# default; -fstack-protector-strong and _FORTIFY_SOURCE are not. +# +# _FORTIFY_SOURCE is skipped under AddressSanitizer alone, which predefines it +# to 0 on purpose (its interceptors do the same job), so redefining it is a +# -Wmacro-redefined error under the -Werror above. UBSAN and TSAN predefine +# nothing and keep it, which is what makes those lanes exercise the same libc +# entry points (__memcpy_chk and the rest) the shipped binary calls. +# +# Every -fsanitize= argument is split on commas so the test is an exact name +# match. A substring test for -fsanitize=address is order-dependent while +# reading as if it were not: it answers correctly for +# "-fsanitize=address,undefined" and wrongly for "-fsanitize=undefined,address", +# the same request spelled the other way round, which then fails the build on +# the macro redefinition. +sanitize_comma := , +SANITIZERS := $(subst $(sanitize_comma), ,\ + $(patsubst -fsanitize=%,%,$(filter -fsanitize=%,$(CFLAGS)))) + +CFLAGS += -fstack-protector-strong +ifeq ($(filter address,$(SANITIZERS)),) +CFLAGS += -D_FORTIFY_SOURCE=2 +endif + ifneq ($(strip $(ELFUSE_NR_EMBEDDER_HVC6)),) CFLAGS += -DELFUSE_NR_EMBEDDER_HVC6=$(ELFUSE_NR_EMBEDDER_HVC6) endif diff --git a/mk/tests.mk b/mk/tests.mk index cda1a99a..84dfaf2d 100644 --- a/mk/tests.mk +++ b/mk/tests.mk @@ -7,7 +7,7 @@ # src/elfuse-limits.h. ELFUSE_HOST_NOFILE_MIN ?= $(shell bash "$(CURDIR)/tests/test-config.sh" --host-nofile) -.PHONY: test-hello test-all check check-syscall-coverage check-eintr-contract check-skill-refs test-gdbstub test-coreutils test-busybox \ +.PHONY: test-hello test-all check check-syscall-coverage check-eintr-contract check-lock-order check-skill-refs test-gdbstub test-coreutils test-busybox \ test-static-bins \ test-dynamic test-dynamic-coreutils test-glibc-dynamic \ test-glibc-coreutils test-perf \ @@ -72,6 +72,11 @@ check-syscall-coverage: check-eintr-contract: @python3 scripts/check-eintr-contract.py +## Verify every lock in the tree is named by the lock-order document +check-lock-order: + @python3 scripts/check-lock-order.py --self-test + @python3 scripts/check-lock-order.py + ## Verify every path, target, and section the skills name still resolves check-skill-refs: @python3 scripts/check-skill-refs.py --self-test @@ -210,7 +215,7 @@ check-sanitizer: $(ELFUSE_BIN) $(TEST_DEPS) $(CHECK_HOST_UNIT_BINS) $(CHECK_SHARED_LANES) ## Run the unit test suite plus busybox applet validation -check: $(ELFUSE_BIN) $(TEST_DEPS) check-syscall-coverage check-eintr-contract check-skill-refs test-config \ +check: $(ELFUSE_BIN) $(TEST_DEPS) check-syscall-coverage check-eintr-contract check-lock-order check-skill-refs test-config \ $(CHECK_HOST_UNIT_BINS) @bash tests/driver.sh -e $(ELFUSE_BIN) -d $(TEST_DIR) -v $(CHECK_SHARED_LANES) diff --git a/scripts/check-lock-order.py b/scripts/check-lock-order.py new file mode 100644 index 00000000..cd7a5665 --- /dev/null +++ b/scripts/check-lock-order.py @@ -0,0 +1,521 @@ +"""Fail when a lock exists that the lock-order document does not name. + +src/syscall/internal.h opens with the acquisition order, and that block is the +only place the tree records which lock may be taken under which. It is prose, so +nothing stopped it from falling behind the code: at the time this check was +written it named 9 of the tree's 31 file-scope locks (one of those under a +shortened spelling that did not match the symbol), and seven of the twenty-two +it left out genuinely nested (autoreap_lock over pid_lock and pidfd_lock, +elf_path_lock over cwd_lock and sysroot_lock, oom_write_lock over fd_lock, +fuse_lock over the per-session lock, proc_tmpdir_lock over mmap_lock, +pty_keepalive_lock over fd_lock, cwd_lock over fuse_lock). Every one of those +was correct as written. The defect was that a reader adding the next lock had +no rule to consult, and an inversion is not the kind of bug the test suite +finds: it needs the two threads to interleave the one way that deadlocks, on a +machine loaded enough to make that likely, which describes CI and not a +developer's laptop. + +So the document claims to be exhaustive, and this makes the claim true. Two +directions, both a set comparison rather than an analysis: + + 1. Every file-scope pthread_mutex_t and pthread_rwlock_t under src/ appears + in the block. + 2. Every lock the block names still exists in the source. + +What this deliberately does NOT do is decide whether a lock belongs in the +ordered list or the leaf list. That question is "is this lock ever held across +another acquisition", which needs the call graph, and a branch-insensitive +answer to it is wrong in both directions: a linear scan of fuse.c drops +fuse_lock at an early-return unlock and calls it a leaf, while the same scan +reads two comments mentioning sys_close() as an sfd_lock/fd_lock cycle. Both +happened while auditing this. A gate that cries wolf gets an exemption list and +then gets ignored, so the placement stays a human judgement and only the +membership is mechanical. Placing a new lock wrongly is a mistake review can +catch; forgetting it entirely is the one nothing was looking for. +""" + +import pathlib +import re +import sys +import tempfile + +REPO = pathlib.Path(__file__).resolve().parent.parent +HEADER = pathlib.Path("src/syscall/internal.h") +SRC = pathlib.Path("src") + +# A definition, not a declaration: column zero, so internal.h's own "extern +# pthread_mutex_t mmap_lock;" lines and any struct member (which clang-format +# indents) count as neither a definition nor a mention. +# +# The declarator list is captured whole rather than as one name, because every +# form of it has to be seen. A sharded "pthread_mutex_t shard_locks[16];" is the +# most likely next lock in a tree that already contends on mmap_lock, and a +# pattern anchored on "= PTHREAD_MUTEX_INITIALIZER" or on a single identifier +# misses it silently, which is the one way for a gate to be worse than no gate. +# +# pthread_rwlock_t counts as a lock too. The tree has two of them today +# (sysinfo_lock in syscall/sys.c, overlay_lock in syscall/chown-overlay.c), and +# a gate that only knew about pthread_mutex_t would have let the block claim to +# be exhaustive while both sat outside it. +# +# Everything between the type and the semicolon is captured, initializers +# included, and the structure is taken apart afterwards. Splitting it in the +# pattern cannot work: an initializer clause that stops at the first comma drops +# the declarators after it ("first = A, second;" loses second), one that runs to +# the semicolon swallows them, and one that refuses braces skips a brace +# initializer outright. All three are silent misses, which is the one shape +# worse than no gate. A function definition survives this because its declarator +# keeps the parenthesis or the leading star that declared_names rejects. +DEFINITION = re.compile(r"^(?:static\s+)?pthread_(?:mutex|rwlock)_t\s+([^;]+);", re.M) + +# An ordered entry names its owning file and ends in a colon: +# * mmap_lock (syscall/mem.c): mmap/brk allocators +ORDERED = re.compile(r"^ \* (\w+)\s+\([\w/.\-]+\):", re.M) + +# A leaf entry is the same without the colon, two to a line: +# * absock_lock (net-absock.c) nl_lock (netlink.c) +LEAF = re.compile(r"(\w+) \([\w/.\-]+\)(?!:)") + + +def split_declarators(declarators): + """Split a declarator list on the commas that separate declarators. + + A comma inside parentheses, brackets or braces belongs to a parameter list, + an array bound or an initializer, not to the list. Splitting on every comma + tears "f(int a, int b)" into "f(int a" and "int b)", and only the first + carries the parenthesis that marks it a function, so the second reads as a + declarator named int and the gate demands the tree document a lock called + int. Braces matter for the same reason in the other direction: + "shard_locks[2] = {A, B}" is one declarator, not two. + """ + parts, depth, cur = [], 0, [] + for ch in declarators: + if ch in "([{": + depth += 1 + elif ch in ")]}": + depth = max(0, depth - 1) + if ch == "," and depth == 0: + parts.append("".join(cur)) + cur = [] + else: + cur.append(ch) + parts.append("".join(cur)) + return parts + + +def strip_initializer(declarator): + """One declarator without its initializer. + + The "=" has to be the declarator's own, not one inside a brace initializer + or a parameter default, so the scan stops at the first one outside every + bracket. Cutting at the first "=" anywhere would truncate + "shard_locks[2] = {A, B}" the same way but would also mangle a declarator + whose array bound contains one. + """ + depth = 0 + for i, ch in enumerate(declarator): + if ch in "([{": + depth += 1 + elif ch in ")]}": + depth = max(0, depth - 1) + elif ch == "=" and depth == 0: + return declarator[:i] + return declarator + + +def declared_names(declarators): + """The mutex names in one declarator list. + + "a_lock = PTHREAD_MUTEX_INITIALIZER" -> a_lock + "shard_locks[16]" -> shard_locks + "a_lock, b_lock" -> a_lock, b_lock + "first = A, second" -> first, second + + A pointer declarator is not a lock, it refers to one, so "*held" yields + nothing: making the tree document a pointer would be a false positive, and + the mutex it points at is defined somewhere this already sees. A declarator + carrying a parameter list is a function returning the type rather than an + object of it, which is what thread_get_lock in runtime/thread.h is. + """ + names = [] + for part in split_declarators(declarators): + part = strip_initializer(part).strip() + if not part or part.startswith("*") or "(" in part: + continue + name = re.match(r"(\w+)", part) + if name: + names.append(name.group(1)) + return names + + +def lock_definitions(root): + """Every file-scope mutex or rwlock under root, as {name: [paths]}. + + Headers are scanned too. None defines a mutex today, and the exhaustive + claim the block above makes is about the tree rather than about one + extension: a static definition in a header compiles into every translation + unit that includes it, so it is the case least likely to be noticed and the + one that most needs naming. + """ + found = {} + for pattern in ("*.c", "*.h"): + for path in sorted(root.rglob(pattern)): + for declarators in DEFINITION.findall(path.read_text(errors="replace")): + for name in declared_names(declarators): + found.setdefault(name, []).append(path) + return found + + +def documented_locks(header_text): + """The ordered and leaf name sets from the lock-ordering block. + + Returns (ordered, leaves, ok). ok is False when the block itself is + missing or malformed, which is its own failure: a header that lost the + block would otherwise report every lock as undocumented and bury the + real cause under twenty-nine lines of noise. + + The markers decide that, not the size of either list. Either list may + legitimately empty out, one when no lock is held across another and the + other when every lock is, and neither is a malformed header. Parsing + nothing at all from a block whose markers are present is different: it + means the entry spelling drifted out from under the patterns, so it keeps + the one clear error rather than becoming a wall of false ones. + """ + start = header_text.find(" * Lock ordering") + if start < 0: + return set(), set(), False + end = header_text.find("\n */", start) + if end < 0: + return set(), set(), False + block = header_text[start:end] + + split = block.find(" * Leaves.") + if split < 0: + return set(), set(), False + + ordered = set(ORDERED.findall(block[:split])) + leaves = set(LEAF.findall(block[split:])) + return ordered, leaves, bool(ordered or leaves) + + +def check(root, header_path): + """Report every disagreement between the source and the document.""" + problems = [] + defined = lock_definitions(root) + ordered, leaves, ok = documented_locks(header_path.read_text(errors="replace")) + + if not ok: + problems.append( + f"{header_path}: no readable lock-ordering block " + "(expected ' * Lock ordering' and ' * Leaves.' before the close)" + ) + return problems, defined, ordered, leaves + + for name in sorted(defined): + where = ", ".join(str(p) for p in defined[name]) + + # Two mutexes sharing a name are two locks, and one entry cannot + # document both: the reader who looks the name up gets whichever the + # author had in mind. Rejecting the collision keeps the check a + # statement about every mutex rather than about every distinct name, + # without teaching it to match the owning file, which would tie it to + # how each entry happens to spell a path. + if len(defined[name]) > 1: + problems.append( + f"{name} is defined in more than one place ({where}); " + "one document entry cannot name both" + ) + continue + + if name in ordered and name in leaves: + problems.append( + f"{name} ({where}) is in both the ordered list and the leaf " + "list; a lock is one or the other" + ) + elif name not in ordered and name not in leaves: + problems.append( + f"{name} ({where}) is in neither list. Put it in the ordered " + "list if anything is acquired while it is held, in the leaf " + "list if nothing is" + ) + + # Both patterns require the "name (file.c)" shape an entry has and the + # surrounding prose does not, which is what keeps a sentence out of these + # sets. The per-instance locks the block names by role ("epoll inst", + # "futex bucket") are two words and match neither, so they never arrive + # here needing to be filtered back out. + for name in sorted((ordered | leaves) - set(defined)): + problems.append(f"{name} is documented but no longer defined under {root}") + + return problems, defined, ordered, leaves + + +SELF_TEST_HEADER = """/* + * Lock ordering (acquire in ascending order to prevent deadlocks). + * + * outer_lock (a.c): holds inner_lock beneath it + * inner_lock (b.c): inner + * + * Leaves. + * + * quiet_lock (c.c) + */ +#pragma once +""" + + +def self_test(): + """Synthetic trees, so the check is shown to fail before it is trusted.""" + cases = [] + + def case(name, sources, header, expect): + with tempfile.TemporaryDirectory() as tmp: + root = pathlib.Path(tmp) / "src" + root.mkdir() + for fname, text in sources.items(): + (root / fname).write_text(text) + hpath = pathlib.Path(tmp) / "internal.h" + hpath.write_text(header) + problems, *_ = check(root, hpath) + hit = any(expect in p for p in problems) + cases.append((name, hit if expect else not problems, problems)) + + ok_sources = { + "a.c": "pthread_mutex_t outer_lock = PTHREAD_MUTEX_INITIALIZER;\n", + "b.c": "static pthread_mutex_t inner_lock = PTHREAD_MUTEX_INITIALIZER;\n", + "c.c": "static pthread_mutex_t quiet_lock = PTHREAD_MUTEX_INITIALIZER;\n", + } + case("a documented tree passes", ok_sources, SELF_TEST_HEADER, "") + + case( + "an undocumented lock fails", + dict( + ok_sources, + **{ + "d.c": "static pthread_mutex_t new_lock = " + "PTHREAD_MUTEX_INITIALIZER;\n" + }, + ), + SELF_TEST_HEADER, + "new_lock", + ) + + case( + "a zero-initialized undocumented lock fails", + dict( + ok_sources, + **{"d.c": "static pthread_mutex_t new_lock;\n"}, + ), + SELF_TEST_HEADER, + "new_lock", + ) + + case( + "an undocumented lock array fails", + dict(ok_sources, **{"d.c": "static pthread_mutex_t new_lock[16];\n"}), + SELF_TEST_HEADER, + "new_lock", + ) + + # The form the old pattern skipped: the brace initializer ended the match + # before the declarator was ever read, so a whole array of locks went + # undocumented with the gate green. + case( + "a brace-initialized undocumented lock array fails", + dict( + ok_sources, + **{ + "d.c": "static pthread_mutex_t new_lock[2] = {\n" + " PTHREAD_MUTEX_INITIALIZER,\n" + " PTHREAD_MUTEX_INITIALIZER,\n" + "};\n" + }, + ), + SELF_TEST_HEADER, + "new_lock", + ) + + # An rwlock is a lock. The tree has two, and a mutex-only pattern let both + # stay out of a block that claims to name every one. + case( + "an undocumented rwlock fails", + dict( + ok_sources, + **{ + "d.c": "static pthread_rwlock_t new_lock = " + "PTHREAD_RWLOCK_INITIALIZER;\n" + }, + ), + SELF_TEST_HEADER, + "new_lock", + ) + + case( + "the second name in a declarator list is not missed", + dict( + ok_sources, + **{"d.c": "static pthread_mutex_t quiet_lock, new_lock;\n"}, + ), + SELF_TEST_HEADER, + "new_lock", + ) + + # A pointer refers to a lock rather than being one, so demanding it be + # documented would be a false positive. The mutex it aims at is defined + # wherever it is defined, and that definition is what this sees. + case( + "a pointer declarator is not a definition", + dict(ok_sources, **{"e.c": "static pthread_mutex_t *held_lock;\n"}), + SELF_TEST_HEADER, + "", + ) + + case( + "a lock in both lists fails", + ok_sources, + SELF_TEST_HEADER.replace( + " * quiet_lock (c.c)", " * quiet_lock (c.c) inner_lock (b.c)" + ), + "inner_lock", + ) + + case( + "a deleted lock still documented fails", + {k: v for k, v in ok_sources.items() if k != "c.c"}, + SELF_TEST_HEADER, + "quiet_lock", + ) + + case( + "a mutex defined in a header is seen", + dict(ok_sources, **{"d.h": "static pthread_mutex_t new_lock;\n"}), + SELF_TEST_HEADER, + "new_lock", + ) + + # runtime/thread.h declares pthread_mutex_t *thread_get_lock(void) at + # column zero, which is a function returning the type, not a lock. + case( + "a function returning the type is not a definition", + dict( + ok_sources, + **{"d.h": "pthread_mutex_t get_lock(void);\n"}, + ), + SELF_TEST_HEADER, + "", + ) + + # The parameter list carries its own commas. Splitting on all of them + # leaves "int b)" looking like a declarator, so the gate asks the tree to + # document a lock named int. + # An initializer clause that runs to the semicolon swallows the declarators + # behind it, which is a silent miss rather than a false report. + case( + "a declarator after an initializer is not missed", + dict( + ok_sources, + **{ + "d.c": "static pthread_mutex_t quiet_lock = " + "PTHREAD_MUTEX_INITIALIZER, new_lock;\n" + }, + ), + SELF_TEST_HEADER, + "new_lock", + ) + + case( + "a declarator after a brace initializer is not missed", + dict( + ok_sources, + **{"d.c": "static pthread_mutex_t ring[2] = {A, B}, new_lock;\n"}, + ), + SELF_TEST_HEADER, + "new_lock", + ) + + case( + "a parameter list is not a declarator list", + dict( + ok_sources, + **{"d.h": "pthread_mutex_t make_lock(int a, int b);\n"}, + ), + SELF_TEST_HEADER, + "", + ) + + case( + "the same name defined twice is rejected", + dict(ok_sources, **{"d.c": "static pthread_mutex_t quiet_lock;\n"}), + SELF_TEST_HEADER, + "more than one place", + ) + + # Either list may legitimately empty out. A block that still has both + # markers and parses one list is a document, not a malformed header. + case( + "an empty leaf list is not a malformed block", + { + "a.c": "pthread_mutex_t outer_lock = PTHREAD_MUTEX_INITIALIZER;\n", + "b.c": "static pthread_mutex_t inner_lock;\n", + }, + "/*\n * Lock ordering.\n *\n * outer_lock (a.c): holds inner_lock\n" + " * inner_lock (b.c): inner\n *\n * Leaves.\n *\n * None today.\n */\n", + "", + ) + + case( + "a header with no block fails as a header problem", + ok_sources, + "/*\n * Nothing here.\n */\n", + "no readable lock-ordering block", + ) + + # An extern declaration is not a definition, so a header included by the + # scan must not register a lock the .c files do not define. + case( + "an extern declaration is not a definition", + dict(ok_sources, **{"e.c": "extern pthread_mutex_t elsewhere_lock;\n"}), + SELF_TEST_HEADER, + "", + ) + + failures = [(n, p) for n, hit, p in cases if not hit] + if failures: + print( + f" {len(failures)} of {len(cases)} self-test case(s) failed:", + file=sys.stderr, + ) + for name, problems in failures: + print(f" {name}: {problems}", file=sys.stderr) + return 1 + print(f" self-test: {len(cases)} cases, all pass") + return 0 + + +def main(): + if "--self-test" in sys.argv[1:]: + print(" LOCKS self-test", flush=True) + return self_test() + + print(f" LOCKS {HEADER}", flush=True) + problems, defined, ordered, leaves = check(REPO / SRC, REPO / HEADER) + if problems: + print( + f" {len(problems)} lock(s) disagree with the order document:", + file=sys.stderr, + ) + for p in problems: + print(f" {p}", file=sys.stderr) + print(f"\n The block at the top of {HEADER} is the document.", file=sys.stderr) + return 1 + + named = len(ordered & set(defined)) + print( + f" {len(defined)} file-scope lock(s), all named: " + f"{named} ordered, {len(leaves)} leaf" + ) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/src/runtime/thread.c b/src/runtime/thread.c index 5b640ac4..75e5f1a2 100644 --- a/src/runtime/thread.c +++ b/src/runtime/thread.c @@ -1153,25 +1153,29 @@ int thread_exec_de_thread(void) * costs a threaded execve milliseconds instead of a poll quantum per parked * sibling (measured: 38 s to 2.5 s over 200 threaded execs). * - * How long to wait for is measured in departures, not in iterations. A - * fixed count has to be chosen against the slowest re-check quantum a - * parked sibling owes (200 ms, in the io wait) multiplied by how far behind - * schedule the host is running, and that multiplier is not a property this - * code can know: CI runs four sanitizer lanes on one machine, where a - * count wide enough is dead time on every threaded execve elsewhere. - * Waiting while siblings are still leaving separates a slow host from - * siblings nothing wakes, which is what the stall interval and the ceiling - * below are for. + * One wall-clock ceiling bounds the wait, and nothing else does. Departures + * look like a liveness signal and are not one: once a single sibling is + * left there are none left to observe, so a stall interval only ever + * measures that last sibling's own latency, which is the fixed bound it was + * meant to replace. Sized honestly it would have to cover the slowest + * re-check quantum a parked sibling owes (200 ms, in the io wait) + * multiplied by how far behind schedule the host is running, and that + * multiplier is not a property this code can know: CI runs the sanitizer + * lanes of several workflows on one machine, where a 1000 ms stall interval + * gave up on a sibling sitting in mmap and sent an otherwise healthy execve + * to the post-PNR fatal exit. * - * Both are wall-clock, because the sleep is a floor: an oversubscribed host + * Spending the whole budget here rather than splitting it with the join + * below is what the re-poke is for: this loop kicks the wake set every + * iteration, and the join only polls. Time moved from here to there buys a + * parked sibling nothing. + * + * Wall-clock, because the sleep is a floor: an oversubscribed host * stretches each iteration well past its 1 ms, so a bound counted in * iterations would run for minutes and blow the caller's timeout instead of * reaching the diagnosed exit. */ - enum { - DE_THREAD_STALL_MS = 1000, /* no departures before giving up */ - DE_THREAD_CEILING_MS = 10000 /* total, however well it is going */ - }; + enum { DE_THREAD_CEILING_MS = 10000 }; struct timespec started; clock_gettime(CLOCK_MONOTONIC, &started); @@ -1193,8 +1197,7 @@ int thread_exec_de_thread(void) remaining = now; progress_ms = elapsed_ms; } - if (elapsed_ms >= DE_THREAD_CEILING_MS || - elapsed_ms - progress_ms >= DE_THREAD_STALL_MS) + if (elapsed_ms >= DE_THREAD_CEILING_MS) break; } diff --git a/src/syscall/internal.h b/src/syscall/internal.h index 90023200..fa19cd6c 100644 --- a/src/syscall/internal.h +++ b/src/syscall/internal.h @@ -7,16 +7,55 @@ * Cross-domain declarations: shared locks, the FD table, and translation * helpers used by multiple syscall modules. * - * Lock ordering (acquire in ascending order to prevent deadlocks): + * Lock ordering (acquire in ascending order to prevent deadlocks). This is + * every file-scope lock in the tree that is ever held across another + * acquisition, whether the inner one is taken directly or through a call. The + * leaf list below closes it: a lock named there holds nothing beneath it, so + * the pair of lists is exhaustive rather than "the ones that seemed worth + * writing down". Adding a mutex or an rwlock means placing it here or in the + * leaf list, and a leaf that grows a call to another locker moves up here. + * + * Per-instance locks (one per epoll instance, futex bucket, FUSE session) are + * named beside the file-scope lock they nest under. That pairing is the whole + * of their ordering except for the FUSE session lock, which also holds fd_lock + * and sig_lock beneath it: fuse_queue_request_locked runs with the session lock + * held and reaches asyncio_fire, which snapshots an fd and queues a signal. + * fuse_lock is always dropped before the session lock is used that way, so the + * two claims do not meet. + * + * The numbered "Lock order: N" comments at the definitions predate this list + * and are not indices into it. This list is the document; a number there says + * only which locks that one was known to precede when it was written. + * + * proc_tmpdir_lock (runtime/procemu.c): lazy /proc snapshot tree; holds + * mmap_lock beneath it, because + * ensure_proc_tmpdir builds the + * /proc/self/maps and smaps snapshots + * through proc_intercept_open while the + * lock is held. Above mmap_lock, so + * mmap_lock's "1" means first among the + * locks a syscall path takes, not that + * nothing precedes it * mmap_lock (syscall/mem.c): mmap/brk allocators + page tables * pt_lock (core/guest.c): page table pool allocator + * pty_keepalive_lock (runtime/procemu-pty.c): pty master keepalive table; + * holds fd_lock beneath it in + * proc_pty_master_adopt's joint publish + * window and in duplicate_guest_fd, which + * brackets fd_snapshot_and_dup. Both paths + * take it in this direction, which is what + * keeps them from deadlocking each other + * oom_write_lock (runtime/procemu.c): /proc/self/oom_score_adj writer; holds + * fd_lock beneath it through + * proc_oom_refresh_live_fds_locked * fd_lock (syscall/fdtable.c): FD table (alloc/close/dup) * epoll inst (syscall/poll.c): per-epoll-instance regs[]; taken under * fd_lock by the close hook, taken alone * (no fd_lock held) by epoll_ctl/pwait - * exec_handoff (syscall/exec.c): the one execve handoff slot; nested under - * mmap_lock only by exec_handoff_reset, and - * holds sig_lock beneath it through + * exec_handoff_lock (syscall/exec.c): the one execve handoff slot; nested + * under mmap_lock only by + * exec_handoff_reset, and holds sig_lock + * beneath it through * signal_restore_blocked. A requester drops * mmap_lock before taking it: waiting for * the slot under mmap_lock deadlocks @@ -25,9 +64,53 @@ * sig_lock (syscall/signal.c): signal handlers/pending/blocked * thread_lock (runtime/thread.c): thread table * sfd_lock (syscall/fd.c): special fd (never held with thread_lock) + * autoreap_lock (syscall/proc.c): serializes the whole no-zombie reap + * sequence against rt_sigaction; holds + * pid_lock and, through + * proc_pidfd_notify_exit, pidfd_lock + * elf_path_lock (syscall/proc-state.c): cached /proc/self/exe path; holds + * cwd_lock beneath it through + * proc_get_cwd, and sysroot_lock through + * the proc_cwd_refresh that runs with + * cwd_lock dropped * pid_lock (syscall/proc.c): process table / wait state + * pidfd_lock (syscall/proc-pidfd.c): pidfd registry * futex bucket (runtime/futex.c): per-bucket, index-ordered if >1 + * cwd_lock (syscall/proc-state.c): cached guest cwd. A leaf on every + * path but one: proc_acquire_cwd_view + * returns still holding it, and both + * callers that inspect view.path in place + * (sys_faccessat, fuse_resolve_at_path) + * call fuse_path_matches_mount inside the + * view, so fuse_lock nests beneath it + * fuse_lock (syscall/fuse.c): mount/session registry; holds the + * per-session session->lock beneath it, + * which is what pins a session against + * daemon exit while a request is in flight * inotify_lock (syscall/inotify.c): inotify watch table + * + * Leaves. Each of these is the innermost lock on every path that takes it, so + * it has no position in the order above and cannot be half of an inversion: + * + * absock_lock (net-absock.c) rlimit_lock (sys.c) + * log_mutex (debug/log.c) rosettad_path_lock (rosetta.c) + * nl_lock (netlink.c) session_lock (proc-identity.c) + * overlay_lock (chown-overlay.c) shm_dir_lock (procemu.c) + * proc_scratch_lock (procemu.c) shm_lock (sysvipc.c) + * removed_overlay_lock (fs.c) syscpu_dir_lock (procemu.c) + * sysinfo_lock (sys.c) + * sysroot_lock (proc-state.c) + * + * log_mutex is the one leaf every other entry may hold: a lock anywhere in + * either list can log while held. It sits below the whole order rather than + * beside the rest of the leaves, and it acquires nothing itself, so it closes + * no cycle. + * + * pt_lock, thread_lock, sfd_lock, pid_lock and pidfd_lock are leaves today + * too. They stay in the ordered list because each is named as an inner lock + * above, so the order they would be acquired in is the load-bearing fact about + * them. sig_lock is not one of them: signal_queue_thread_common takes + * thread_lock under it. */ #pragma once diff --git a/tests/test-casefold-walk-host.c b/tests/test-casefold-walk-host.c index 7534f39f..c8c95042 100644 --- a/tests/test-casefold-walk-host.c +++ b/tests/test-casefold-walk-host.c @@ -258,12 +258,12 @@ static void section_symlink(void) * link_guest_offset at the link itself, so a relative target can be joined * to the directory holding it. */ - if (casefold_resolve_at(AT_FDCWD, root, "/link.to.lowdir/inner.txt", false, - out, sizeof(out), &walk) == CASEFOLD_SYMLINK && + static const char through_link[] = "/link.to.lowdir/inner.txt"; + if (casefold_resolve_at(AT_FDCWD, root, through_link, false, out, + sizeof(out), &walk) == CASEFOLD_SYMLINK && !strcmp(out + strlen(root), "/link.to.lowdir") && - !strcmp("/link.to.lowdir/inner.txt" + walk.link_rest_offset, - "inner.txt") && - !strcmp("/link.to.lowdir/inner.txt" + walk.link_guest_offset, + !strcmp(through_link + walk.link_rest_offset, "inner.txt") && + !strcmp(through_link + walk.link_guest_offset, "link.to.lowdir/inner.txt")) host_ok(); else