From f01bf33a0b6b2a6ab373258f24c023f9bda5a6e4 Mon Sep 17 00:00:00 2001 From: Pooya Parsa Date: Fri, 31 Jul 2026 18:50:45 +0000 Subject: [PATCH 1/7] feat(nfs): bound the file handle table with an LRU MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Neither NFS version has a FORGET, so the table grew to one entry per path a client had ever named and stayed there for the life of the server. `maxHandles` caps it, least recently used first; the default is still no cap, which is what this server has always done. Recency is stamped where a handle is used — decode(), at(), pathOf() and #attachPath() — so an entry a client is working through is never the victim. The order is #byId's own insertion order, so nothing can fall out of step with #byPath/#byKey. Eviction runs the same teardown #detachPath does, and the root stays exempt: PUTROOTFH and MNT encode it from a field rather than from a lookup. An eviction costs the holder an ESTALE and a re-lookup, and an NFSv4.1 client that had the file open an OPEN as well, since v4 keys its state by entry id. That is why there is no default cap. Co-Authored-By: Claude Opus 5 --- src/nfs/handles.ts | 195 +++++++++++++++++++++++++++++++++++++-- src/nfs/session.ts | 1 + src/nfs/util.ts | 9 ++ test/nfs/handles.test.ts | 155 +++++++++++++++++++++++++++++++ test/nfs/server.test.ts | 61 ++++++++++++ 5 files changed, 411 insertions(+), 10 deletions(-) diff --git a/src/nfs/handles.ts b/src/nfs/handles.ts index 67bcc32..c13bcea 100644 --- a/src/nfs/handles.ts +++ b/src/nfs/handles.ts @@ -16,9 +16,9 @@ * earlier. That matters because ids are minted from a monotonic counter and * never reused, so create/delete churn under a mount (a build, a `tar -x` * followed by a `rm -rf`) would otherwise grow `#byId` without any bound at - * all. What is left is the honest v1 tradeoff — one live entry per path the - * client currently has a name for — and a generation-stamped LRU is the fix - * if a workload ever needs more. + * all. What is left is one live entry per path the client currently has a + * name for, plus the root — held for the life of the server however long ago + * the client looked, unless the table is given a cap. See "The bound" below. * - **Handles are identity-keyed**, on the driver's `(dev, ino)`, exactly as * nodeids are. That is what makes a handle survive `rename` (the file is the * same file, whatever it is called now) and what makes two hardlinks one @@ -32,6 +32,38 @@ * The handle itself is 20 bytes: a magic word, the server's boot verifier, and * a 64-bit entry id. The verifier is what makes a handle from a previous * process answer `NFS3ERR_STALE` rather than colliding with a live entry. + * + * ## The bound ({@link FileHandleTableOptions.maxHandles}) + * + * Unset — the default — nothing is evicted, and the table grows to one entry + * per path the client currently has a name for: a client that walks a + * million-file tree leaves a million entries, and nothing on the wire will ever + * say they can go. Set, the table keeps at most that many entries (the root + * counted) and drops the least recently *used* one past the cap. Recency is + * stamped by every path that reaches an entry — `decode`, `at`, `pathOf`, and + * every `bind`/`remap` attach — not only by the call that created it, so the + * entries a client is working with are the last ones considered, and the one it + * looked up an hour ago and forgot is the first. + * + * Evicting is not free the way dropping a path-less entry was: the entry it + * takes is one the client may still have a live name for, so this *is* visible + * to it. It is however still **safe**, for the same reason — `#nextId` only + * ever counts up, so the id in the handle the client kept can never be handed + * to a different file, and that handle answers `ESTALE` ("unknown file handle") + * rather than quietly naming somebody else's. What it costs an NFSv3 client is + * one round trip: `ESTALE` means "drop the dentry and look the name up again", + * which re-binds the path and mints a fresh handle. What it costs an NFSv4.1 + * client that had the file **open** is more — `v4/session.ts` keys open and + * lock state by entry id, so the re-lookup is a different file as far as share + * reservations are concerned and the client has to re-`OPEN`. That asymmetry is + * why the cap is off unless asked for, rather than defaulted to some number. + * + * The root is never a victim: `PUTROOTFH`, `PUTPUBFH` and `MNT` hand out its + * handle from the `root` field rather than from a lookup, so an id that stopped + * decoding would be a handle this server minted and immediately refused. + * Neither is the entry currently being bound — the call that overflowed the cap + * is about to answer with it — so the smallest table a cap can produce is those + * two, whatever number it is given. */ import { randomFillSync } from "node:crypto"; @@ -87,6 +119,18 @@ export interface FileHandleTableOptions { useDriverIno?: boolean; /** Boot verifier. Default: eight random bytes, so handles do not survive a restart. */ verifier?: Uint8Array; + /** + * Most entries to keep, the least recently used going first past it. Default: + * no cap at all, which is what every version of this table did before the + * option existed. + * + * The number counts everything {@link FileHandleTable.size} does, the root + * included. The root and the entry being bound are never evicted, so a cap + * below two is honoured as far as it can be rather than refused. See the + * module docs for what an eviction costs a client still holding the handle — + * it is a real cost, which is why there is no default. + */ + maxHandles?: number; } /** fh ↔ path ↔ `(dev, ino)`, for the lifetime of the server. */ @@ -96,6 +140,9 @@ export class FileHandleTable { readonly verifier: Uint8Array; readonly #useDriverIno: boolean; + /** Entries to keep, or `0` for "no cap" — see {@link FileHandleTableOptions.maxHandles}. */ + readonly #maxHandles: number; + /** Doubles as the LRU order: insertion order, youngest last. See {@link FileHandleTable.touch}. */ readonly #byId = new Map(); readonly #byPath = new Map(); /** Only holds entries with at least one path — see {@link FileHandleTable.detachPath}. */ @@ -113,6 +160,11 @@ export class FileHandleTable { constructor(options: FileHandleTableOptions = {}) { this.#useDriverIno = options.useDriverIno ?? true; + // Anything that is not a usable positive count — unset, zero, `Infinity`, + // `NaN` — is the uncapped default rather than a refusal, because "grow + // forever" is exactly what this table did before the option existed. + const cap = Math.trunc(options.maxHandles ?? 0); + this.#maxHandles = Number.isFinite(cap) && cap > 0 ? cap : 0; this.verifier = options.verifier ?? randomFillSync(new Uint8Array(8)); this.root = { id: ROOT_HANDLE_ID, @@ -126,7 +178,9 @@ export class FileHandleTable { /** * Entries currently known — one per file that still has at least one name, - * plus the root. See the module docs for what is and is not dropped. + * plus the root, and never more than + * {@link FileHandleTableOptions.maxHandles} when one was given. See the + * module docs for what is and is not dropped. */ get size(): number { return this.#byId.size; @@ -153,7 +207,8 @@ export class FileHandleTable { * Every rejection is `ESTALE` rather than `EINVAL`: from a client's point of * view a handle it cannot use any more and a handle that was never ours are * the same event, and `ESTALE` is the one it knows how to recover from - * (drop the dentry, look the name up again). + * (drop the dentry, look the name up again). Under a cap, an entry that was + * evicted arrives here as the second of those and leaves as the first. */ decode(fh: Uint8Array): HandleEntry { if (fh.byteLength !== FH_SIZE) { @@ -176,16 +231,26 @@ export class FileHandleTable { if (entry === undefined) { throw stale("unknown file handle"); } + this.#touch(entry); return entry; } - /** The entry an id names, if it is still live. Nothing in `src/` needs this; the tests do. */ + /** + * The entry an id names, if it is still live. Nothing in `src/` needs this; + * the tests do — which is also why it is the one lookup that does *not* + * restamp recency: a test has to be able to look at the table without + * changing what the next eviction picks. + */ get(id: bigint): HandleEntry | undefined { return this.#byId.get(id); } at(path: string): HandleEntry | undefined { - return this.#byPath.get(path); + const entry = this.#byPath.get(path); + if (entry !== undefined) { + this.#touch(entry); + } + return entry; } /** @@ -202,6 +267,7 @@ export class FileHandleTable { // single asymmetry between the two maps from turning into a handle that // silently operates on somebody else's file. if (this.#byPath.get(path) === entry) { + this.#touch(entry); return path; } entry.paths.delete(path); @@ -244,6 +310,10 @@ export class FileHandleTable { } this.#attachPath(entry, path); this.#pruneToNlink(entry, stats); + // Last, and only here: `bind` is the one thing that adds an entry, so it is + // the one thing that can push the table over its cap. `entry` is exempt — + // the caller is about to encode it into a reply. + this.#enforceLimit(entry); return entry; } @@ -326,15 +396,36 @@ export class FileHandleTable { if (entry.key !== undefined) { this.#byKey.set(entry.key, entry); } + // Being given a name is a use, which is what makes `remap` — whose every + // rewritten path lands here — restamp a whole moved subtree rather than + // leaving it looking untouched since whenever it was first looked up. + this.#touch(entry); } #detachPath(entry: HandleEntry, path: string): void { entry.paths.delete(path); - this.#byPath.delete(path); + // Only while the path map still agrees this is the entry's name. The two + // can disagree — `pathOf` exists because of it — and an eviction detaches + // every name an entry remembers in one go, so a name that has since been + // re-bound to a different entry must not be taken out from under it. + if (this.#byPath.get(path) === entry) { + this.#byPath.delete(path); + } if (entry.paths.size > 0) { return; } - // A path-less entry must not be found by identity again: a real filesystem + this.#dropEntry(entry); + } + + /** + * Forget an entry outright: the identity map, then the id map. + * + * Both callers mean the same thing by it — a last name went away, or the LRU + * chose this one — and both need every map to agree afterwards, which is why + * there is one copy of it rather than one per caller. + */ + #dropEntry(entry: HandleEntry): void { + // A dropped entry must not be found by identity again: a real filesystem // reuses the `ino` of a deleted file for the next one created. if (entry.key !== undefined && this.#byKey.get(entry.key) === entry) { this.#byKey.delete(entry.key); @@ -344,7 +435,9 @@ export class FileHandleTable { // thing that happens to it is `pathOf()` throwing `ESTALE`; dropping it // makes `decode()` say the same thing one sentence earlier ("unknown file // handle"), which is safe precisely because `#nextId` only ever counts up, - // so no later file can be handed this id and alias the dead handle. + // so no later file can be handed this id and alias the dead handle. That + // last sentence is the whole licence for evicting a *live* entry too: the + // client is told `ESTALE`, never handed somebody else's file. // // The root is the exception: `PUTROOTFH` and `MNT` hand out its handle from // the `root` field rather than from a lookup, so an id that no longer @@ -353,6 +446,88 @@ export class FileHandleTable { this.#byId.delete(entry.id); } } + + /** + * Restamp an entry as the most recently used one. + * + * The LRU order *is* `#byId`'s insertion order — re-inserting moves an entry + * to the young end, the same trick {@link DirectorySnapshots} plays — so + * there is no second structure that could fall out of step with the three + * maps, and no allocation per lookup. The `delete` result is the guard: an + * entry that has already been dropped must not be resurrected by whatever is + * still holding a reference to it. + * + * Uncapped, this is a no-op rather than bookkeeping nobody will read: with no + * eviction there is no order to maintain, and a delete-and-set on every + * `decode` is not free. The root is a no-op too, and for the opposite reason: + * it can never be a victim, so its place in the order means nothing, and + * leaving it where it was inserted — the old end, for the life of the table — + * is what keeps {@link FileHandleTable.lruVictim}'s scan to a step or two. + */ + #touch(entry: HandleEntry): void { + if (this.#maxHandles <= 0 || entry.id === ROOT_HANDLE_ID) { + return; + } + if (this.#byId.delete(entry.id)) { + this.#byId.set(entry.id, entry); + } + } + + /** Drop least-recently-used entries until the table is inside its cap. */ + #enforceLimit(keep: HandleEntry): void { + if (this.#maxHandles <= 0) { + return; + } + while (this.#byId.size > this.#maxHandles) { + const victim = this.#lruVictim(keep); + // Root plus the entry just bound, and a cap of one or two asking for + // less than that. Honour what can be honoured and stop. + if (victim === undefined) { + return; + } + this.#evict(victim); + } + } + + /** + * The oldest entry that may go — anything but the root and the entry the + * caller is in the middle of binding. + * + * The scan is not the linear cost it looks like: the root is inserted first + * and never touched, so it sits at the old end for the life of the table and + * this walks past it and one more entry at most. + */ + #lruVictim(keep: HandleEntry): HandleEntry | undefined { + for (const entry of this.#byId.values()) { + if (entry.id !== ROOT_HANDLE_ID && entry !== keep) { + return entry; + } + } + return undefined; + } + + /** + * Drop an entry the client may still be naming. + * + * Name by name through {@link FileHandleTable.detachPath}, so this is the + * same teardown as a file whose last link went away rather than a second one + * beside it — the maps cannot end up disagreeing about an entry that is gone, + * because only one piece of code decides what "gone" does. The trailing + * {@link FileHandleTable.dropEntry} covers the case the loop cannot reach: + * `pathOf` forgets a name the path map no longer agrees with without going + * through `#detachPath` at all, so an entry can be down to no names and still + * be here. It is idempotent, so paying for it unconditionally is cheaper than + * asking. + * + * Deleting the name being visited is what {@link FileHandleTable.pruneToNlink} + * already does to the same set, and is well defined for a `Set` iterator. + */ + #evict(entry: HandleEntry): void { + for (const path of entry.paths) { + this.#detachPath(entry, path); + } + this.#dropEntry(entry); + } } // --------------------------------------------------------------------------- diff --git a/src/nfs/session.ts b/src/nfs/session.ts index 017a9d0..d92fdd4 100644 --- a/src/nfs/session.ts +++ b/src/nfs/session.ts @@ -156,6 +156,7 @@ export class NfsSession { handles: new FileHandleTable({ useDriverIno: options.useDriverIno, verifier: options.verifier, + maxHandles: options.maxHandles, }), lock: new PathLock(), stats: newSessionStats(), diff --git a/src/nfs/util.ts b/src/nfs/util.ts index f6914c9..43c3d93 100644 --- a/src/nfs/util.ts +++ b/src/nfs/util.ts @@ -136,6 +136,15 @@ export interface NfsSessionOptions { useDriverIno?: boolean; /** Boot verifier for file handles. Default: random, so restarts invalidate handles. */ verifier?: Uint8Array; + /** + * Most file handle table entries to keep, the least recently used going + * first past it. Default: no cap, which is what this server has always done. + * + * An eviction costs the client holding that handle an `ESTALE` and a + * re-lookup — and an NFSv4.1 client that had the file open rather more than + * that. See `./handles.ts` before setting it. + */ + maxHandles?: number; /** Largest `READ` answered. */ rtmax?: number; /** Largest `WRITE` accepted. */ diff --git a/test/nfs/handles.test.ts b/test/nfs/handles.test.ts index 227fdee..b4e5f82 100644 --- a/test/nfs/handles.test.ts +++ b/test/nfs/handles.test.ts @@ -223,6 +223,161 @@ describe("path-less entries", () => { }); }); +/** + * The other half of the growth story: an entry whose names are all still live. + * + * Nothing on the wire ever says a client is done with one, so a cap is the only + * bound there can be — and evicting one is visible to the client, unlike + * dropping a path-less entry. What these check is that it is visible as + * `ESTALE` and never as somebody else's file. + */ +describe("the handle-table bound", () => { + it("grows without limit unless a cap is asked for", () => { + const table = new FileHandleTable(); + for (let index = 0; index < 200; index++) { + table.bind(`/f${index}`, stats(100 + index)); + } + expect(table.size).toBe(201); + expect(table.pathCount).toBe(201); + }); + + it("evicts the least recently used, and only past the cap", () => { + // Root plus three. + const table = new FileHandleTable({ maxHandles: 4 }); + const a = table.bind("/a", stats(2)); + const b = table.bind("/b", stats(3)); + const c = table.bind("/c", stats(4)); + const gone = table.encode(b); + expect(table.size).toBe(4); + + // Using `/a` makes `/b` the oldest, which is the whole point of stamping + // recency on `decode` rather than only on `bind`. + expect(table.decode(table.encode(a))).toBe(a); + table.bind("/d", stats(5)); + + expect(table.size).toBe(4); + expect(table.get(b.id)).toBeUndefined(); + expect(table.at("/b")).toBeUndefined(); + expect(table.get(a.id)).toBe(a); + expect(table.get(c.id)).toBe(c); + // What the client that still holds `/b`'s handle is told. + expect(() => table.decode(gone)).toThrow( + expect.objectContaining({ code: "ESTALE", errno: -116 }), + ); + expect(() => table.decode(gone)).toThrow(/unknown file handle/); + }); + + it("leaves an evicted handle stale however the identities line up afterwards", () => { + const table = new FileHandleTable({ maxHandles: 2 }); + const evicted = table.bind("/a", stats(5)); + const gone = table.encode(evicted); + table.bind("/b", stats(6)); + expect(table.get(evicted.id)).toBeUndefined(); + + // The same `(dev, ino)`, straight back from the driver, at the same path: + // the evicted entry must not be found by identity, and the id it had must + // not come round again. + const fresh = table.bind("/a", stats(5)); + expect(fresh).not.toBe(evicted); + expect(fresh.id).not.toBe(evicted.id); + expect(() => table.decode(gone)).toThrow(/unknown file handle/); + expect(table.pathOf(fresh)).toBe("/a"); + }); + + it("never evicts an entry the client is working with", () => { + const table = new FileHandleTable({ maxHandles: 3 }); + const keep = table.bind("/keep", stats(2)); + const kept = table.encode(keep); + // Churn far past the cap, touching `/keep` the way a client holding its + // handle would. + for (let index = 0; index < 50; index++) { + table.bind(`/tmp${index}`, stats(100 + index)); + expect(table.resolve(kept)).toBe("/keep"); + } + expect(table.size).toBe(3); + expect(table.get(keep.id)).toBe(keep); + }); + + it("counts reaching an entry by path as a use", () => { + const table = new FileHandleTable({ maxHandles: 3 }); + const dir = table.bind("/dir", stats(2, 2, S_IFDIR | 0o755)); + for (let index = 0; index < 20; index++) { + table.bind(`/dir/f${index}`, stats(100 + index)); + // What a READDIR paging through a directory does with it: reach the entry + // by path rather than by the handle the client sent. + expect(table.at("/dir")).toBe(dir); + } + expect(table.get(dir.id)).toBe(dir); + }); + + it("never evicts the root, whose handle is handed out without a lookup", () => { + const table = new FileHandleTable({ maxHandles: 2 }); + for (let index = 0; index < 20; index++) { + table.bind(`/f${index}`, stats(100 + index)); + } + expect(table.size).toBe(2); + expect(table.decode(table.encode(table.root))).toBe(table.root); + expect(table.pathOf(table.root)).toBe("/"); + }); + + it("keeps the root and the entry being bound however small the cap is", () => { + const table = new FileHandleTable({ maxHandles: 1 }); + const entry = table.bind("/a", stats(2)); + // Two is the floor: the caller is about to encode `entry` into a reply, and + // the root is never evictable at all. + expect(table.size).toBe(2); + expect(table.resolve(table.encode(entry))).toBe("/a"); + }); + + it("takes a cap that is not a usable count as no cap", () => { + for (const maxHandles of [0, -1, Number.NaN, Number.POSITIVE_INFINITY]) { + const table = new FileHandleTable({ maxHandles }); + for (let index = 0; index < 10; index++) { + table.bind(`/f${index}`, stats(100 + index)); + } + expect(table.size, `maxHandles: ${maxHandles}`).toBe(11); + } + }); + + it("leaves the path map agreeing with the id map after an eviction", () => { + const table = new FileHandleTable({ maxHandles: 4 }); + // A hardlinked pair, so the evicted entry has two names to lose at once. + const linked = table.bind("/a", stats(5, 2)); + table.bind("/b", stats(5, 2)); + expect(table.size).toBe(2); + table.bind("/c", stats(6)); + table.bind("/d", stats(7)); + table.bind("/e", stats(8)); + + expect(table.get(linked.id)).toBeUndefined(); + expect(table.at("/a")).toBeUndefined(); + expect(table.at("/b")).toBeUndefined(); + // One path per surviving entry, plus the root's `/`. + expect(table.size).toBe(4); + expect(table.pathCount).toBe(4); + }); + + it("restamps a renamed subtree, and moves it whole", () => { + const table = new FileHandleTable({ maxHandles: 5 }); + const stale = table.bind("/old", stats(2)); + const dir = table.bind("/dir", stats(3, 2, S_IFDIR | 0o755)); + const file = table.bind("/dir/f", stats(4)); + const deep = table.bind("/dir/sub/g", stats(5)); + + table.remap("/dir", "/moved"); + expect(table.pathOf(dir)).toBe("/moved"); + expect(table.pathOf(file)).toBe("/moved/f"); + expect(table.pathOf(deep)).toBe("/moved/sub/g"); + + // The rename touched all three, so the entry nobody has named since it was + // created is the one that goes. + table.bind("/new", stats(6)); + expect(table.get(stale.id)).toBeUndefined(); + expect(table.size).toBe(5); + expect(table.resolve(table.encode(deep))).toBe("/moved/sub/g"); + }); +}); + describe("rename", () => { it("moves a path and everything under it", () => { const table = new FileHandleTable(); diff --git a/test/nfs/server.test.ts b/test/nfs/server.test.ts index ee94dd8..4101661 100644 --- a/test/nfs/server.test.ts +++ b/test/nfs/server.test.ts @@ -43,6 +43,8 @@ import { type NfsServerOptions, } from "../../src/nfs/server.ts"; import { + NFS3_OK, + NFS3ERR_STALE, NFS_PROGRAM, NFS_V3, NFSPROC3_GETATTR, @@ -297,6 +299,65 @@ describe("reply ordering", () => { // --------------------------------------------------------------------------- +describe("the handle table bound", () => { + it("evicts a handle the client stopped using, and says `ESTALE` about it", async () => { + // The third bound, and the only one that is off by default: `maxRecord` + // bounds one record and `maxInFlight` bounds the answers in flight, but the + // handle table grows for the *life of the server*, one entry per path a + // client has a name for, and no NFS message ever says a name is finished + // with. What a cap costs is here rather than in `handles.test.ts`, because + // it is a cost paid by a client rather than by the table: a handle it kept + // stops working, and the recovery is the ordinary `ESTALE` one. + const driver = createMemoryDriver(); + const loopback = createLoopback(driver); + for (let index = 0; index < 8; index++) { + await loopback.writeFile(`/f${index}.txt`, "x"); + } + // The root plus two files, which the walk below overruns immediately. + const server = await serve(driver, { maxHandles: 3 }); + const client = await connect(server); + + const root = check(await client.mnt("/"), "mnt").fh!; + const kept = check(await client.lookup(root, "f0.txt"), "lookup").object!; + expect((await client.getattr(kept)).status).toBe(NFS3_OK); + + for (let index = 1; index < 8; index++) { + check(await client.lookup(root, `f${index}.txt`), "lookup"); + } + + // The client still holds the handle; the server does not. + expect((await client.getattr(kept)).status).toBe(NFS3ERR_STALE); + // And the recovery a Linux client already performs for `ESTALE` — drop the + // dentry, look the name up again — gets it a working handle back. + const again = check(await client.lookup(root, "f0.txt"), "lookup").object!; + expect((await client.getattr(again)).status).toBe(NFS3_OK); + // The root is in that three, and is never the one evicted. + expect(server.session.handles.size).toBe(3); + expect((await client.getattr(root)).status).toBe(NFS3_OK); + }, 30_000); + + it("keeps every handle it ever handed out when no cap is asked for", async () => { + const driver = createMemoryDriver(); + const loopback = createLoopback(driver); + for (let index = 0; index < 8; index++) { + await loopback.writeFile(`/f${index}.txt`, "x"); + } + const server = await serve(driver); + const client = await connect(server); + + const root = check(await client.mnt("/"), "mnt").fh!; + const kept = check(await client.lookup(root, "f0.txt"), "lookup").object!; + for (let index = 1; index < 8; index++) { + check(await client.lookup(root, `f${index}.txt`), "lookup"); + } + + expect((await client.getattr(kept)).status).toBe(NFS3_OK); + expect(server.session.handles.size).toBe(9); + }, 30_000); +}); + +// --------------------------------------------------------------------------- + describe("flow control", () => { /** One framed `GETATTR` call. */ function getattrCall(xid: number, fh: Uint8Array): Uint8Array { From 6615c3ca57e56252dc15a67716b5705faabe7576 Mon Sep 17 00:00:00 2001 From: Pooya Parsa Date: Fri, 31 Jul 2026 18:50:54 +0000 Subject: [PATCH 2/7] feat(nfs): exclusive create on v3 CREATE and v4.1 OPEN MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CREATE/EXCLUSIVE answered NFS3ERR_NOTSUPP and OPEN/EXCLUSIVE4{,_1} answered NFS4ERR_INVAL, because there is nowhere in FsDriver to commit a verifier to. There does not have to be: what EXCLUSIVE asks for is that a *retransmission* recognise its own creation, so the verifier lives in an `ExclusiveCreates` table on the shared state — one table, so a create over v3 and a retry over v4 are one server answering once (RFC 1813 §3.3.8, RFC 5661 §18.16.3). Bounded twice: a 120 s window, since that is how long the original request can still be in flight, and 256 entries, oldest insertion first. Both edges fail the same safe way — the retry stops being recognised and gets EXIST, never a wrong file. A restart still loses it; the class doc says so rather than implying otherwise. Same verifier returns the existing object, a different one is EXIST, and the entry is dropped by REMOVE, by both sides of a RENAME, and by the SETATTR §3.3.8 treats as the client's commit. v3's arm carries no sattr3 so it applies no attributes; EXCLUSIVE4_1's cva_attrs go through the same #settableOrRefuse/#applyAttrs/attrset path UNCHECKED4 does. Co-Authored-By: Claude Opus 5 --- src/nfs/session.ts | 2 + src/nfs/util.ts | 172 ++++++++++++++++++++++++++- src/nfs/v3/session.ts | 92 +++++++++++++-- src/nfs/v4/session.ts | 138 +++++++++++++++++++--- test/nfs/session.test.ts | 116 +++++++++++++++++- test/nfs/v3/session.test.ts | 91 ++++++++++++-- test/nfs/v4/client.ts | 36 ++++-- test/nfs/v4/session.test.ts | 229 ++++++++++++++++++++++++++++++++---- 8 files changed, 800 insertions(+), 76 deletions(-) diff --git a/src/nfs/session.ts b/src/nfs/session.ts index d92fdd4..d35fe83 100644 --- a/src/nfs/session.ts +++ b/src/nfs/session.ts @@ -68,6 +68,7 @@ import { type RpcCall, } from "./rpc.ts"; import { + ExclusiveCreates, newSessionStats, type NfsRequestContext, type NfsSessionOptions, @@ -160,6 +161,7 @@ export class NfsSession { }), lock: new PathLock(), stats: newSessionStats(), + exclusiveCreates: new ExclusiveCreates(), }; this.#v3 = new Nfs3Session(driver, options, this.#shared); this.#v4 = new Nfs4Session(driver, options, this.#shared); diff --git a/src/nfs/util.ts b/src/nfs/util.ts index 43c3d93..a604240 100644 --- a/src/nfs/util.ts +++ b/src/nfs/util.ts @@ -20,7 +20,7 @@ import type { PathLock } from "../lock.ts"; import type { StatsLike } from "../types.ts"; import { S_IFDIR, S_IFMT } from "../types.ts"; -import type { FileHandleTable } from "./handles.ts"; +import { sameVerifier, type FileHandleTable } from "./handles.ts"; import type { RpcCall, RpcCredentials } from "./rpc.ts"; /** @@ -187,6 +187,167 @@ export function newSessionStats(): NfsSessionStats { return { requests: 0, replies: 0, errors: 0, dropped: 0, procedures: new Map() }; } +/** + * How long one exclusive-create verifier is remembered: two minutes. + * + * The thing being survived is a **retransmission**, not an outage: the client + * resends because it never saw the reply, and it resends on an RPC timeout — + * seconds, doubling, a handful of times. Two minutes is generous against that + * and short enough that the table empties itself on any idle server. + */ +export const EXCLUSIVE_CREATE_WINDOW_MS = 120_000; + +/** How many exclusive-create verifiers are remembered at once. */ +export const DEFAULT_EXCLUSIVE_CREATES = 256; + +/** One remembered exclusive create. */ +export interface ExclusiveCreate { + /** The verifier the client sent, copied out of its request. */ + readonly verifier: Uint8Array; + /** + * The attribute bits the original request actually applied, so a replay + * answers with the same `attrset` the lost reply carried. Always empty for + * v3, whose `EXCLUSIVE` carries no `sattr3` at all. + */ + readonly attrset: readonly number[]; + /** When it was recorded, for the window. */ + readonly at: number; +} + +/** Knobs for {@link ExclusiveCreates}; the defaults are the two constants above. */ +export interface ExclusiveCreatesOptions { + /** Entries kept at once. Default {@link DEFAULT_EXCLUSIVE_CREATES}. */ + limit?: number | undefined; + /** How long one is remembered. Default {@link EXCLUSIVE_CREATE_WINDOW_MS}. */ + windowMs?: number | undefined; + /** Milliseconds since an arbitrary epoch. Default `Date.now`. */ + now?: (() => number) | undefined; +} + +/** + * The verifiers of exclusive creates, for exactly as long as a retry can take. + * + * `CREATE` with `EXCLUSIVE` (RFC 1813 §3.3.8) and OPEN with `EXCLUSIVE4` / + * `EXCLUSIVE4_1` (RFC 8881 §18.16.3) ask a server for the same one thing: keep + * the client's verifier beside the file it created, so that the *duplicate* of + * a request whose reply was lost recognises its own creation and is answered + * with it instead of `EXIST`. That is the whole feature — an exclusive create + * is the one operation a client cannot make idempotent by itself. + * + * **This is a table in memory, and it says so.** Both RFCs would rather it were + * not: §3.3.8 wants the verifier in stable storage, and §18.16.4 names "the + * requirement to commit the verifier to stable storage" as the reason a server + * may refuse the mode outright. There is nowhere in `FsDriver` to commit one to + * — it would want an xattr — so what is kept here survives a lost reply and a + * retransmission, **and nothing else**. A restarted server has forgotten every + * verifier, exactly as it has forgotten every file handle (`../handles.ts`) and + * for the same reason; a client retrying across a restart is told `EXIST` by + * the create that follows, which is what it would have had from `GUARDED`. + * + * **Bounded twice**, in the spirit `../handles.ts` states its own growth: + * + * - **By age.** An entry older than the window is not matched, and is dropped + * when it is next looked at. A lookup does *not* refresh it — the window is + * about how long the original request can still be in flight, and re-reading + * the entry does not make that longer. + * - **By count.** `limit` entries, oldest insertion evicted first, so a client + * creating exclusively in a loop cannot grow this without bound. + * + * Both edges fail the same safe way: the retry stops being recognised and gets + * `EXIST`, which is a worse answer than the RFC's but never a wrong file. + * + * An entry is dropped as soon as it cannot mean anything: the file was removed + * or renamed away (a later file at that path is a different file, and must not + * inherit the promise), or the client sent the SETATTR that §3.3.8 makes the + * commit — the follow-up that carries the attributes an exclusive create had + * nowhere to put, after which there is nothing left for the verifier to + * protect. + */ +export class ExclusiveCreates { + readonly #entries = new Map(); + readonly #limit: number; + readonly #windowMs: number; + readonly #now: () => number; + + constructor(options: ExclusiveCreatesOptions = {}) { + this.#limit = Math.max(1, options.limit ?? DEFAULT_EXCLUSIVE_CREATES); + this.#windowMs = Math.max(0, options.windowMs ?? EXCLUSIVE_CREATE_WINDOW_MS); + this.#now = options.now ?? Date.now; + } + + /** Entries held, expired ones included until something looks at them. */ + get size(): number { + return this.#entries.size; + } + + /** Remember `verifier` as the one that created `path`. */ + set(path: string, verifier: Uint8Array, attrset: readonly number[] = []): void { + // Copied, both of them: this outlives the request they were decoded from, + // and `attrset` is the caller's own array, still being pushed to. + const entry: ExclusiveCreate = { + verifier: verifier.slice(), + attrset: [...attrset], + at: this.#now(), + }; + // Delete first, so the Map's insertion order stays the eviction order. + this.#entries.delete(path); + this.#entries.set(path, entry); + while (this.#entries.size > this.#limit) { + const oldest = this.#entries.keys().next().value!; + this.#entries.delete(oldest); + } + } + + /** + * The record at `path` when `verifier` is the one that created it. + * + * `undefined` for every other case — nothing remembered, the window passed, + * or a *different* verifier, which is a second client racing for the same + * name and is the one case that genuinely is `EXIST`. + */ + match(path: string, verifier: Uint8Array): ExclusiveCreate | undefined { + const entry = this.#entries.get(path); + if (entry === undefined) { + return undefined; + } + if (this.#now() - entry.at >= this.#windowMs) { + this.#entries.delete(path); + return undefined; + } + return sameVerifier(entry.verifier, verifier) ? entry : undefined; + } + + /** + * Forget `path`, and everything under it. + * + * The subtree sweep is for the one caller that needs it — a renamed or + * removed *directory* takes every remembered name below it with it — and is + * a walk of a table capped at `limit`, which is why it does not need the + * index `../subtree.ts` builds for the handle table. + * + * The root is the one path this cannot sweep, its prefix being `//`, and + * that is the wanted answer rather than an edge case: `/` is never removed + * or renamed, and a SETATTR on it is about the directory, not about the + * files inside it. + */ + forget(path: string): void { + if (this.#entries.size === 0) { + return; + } + this.#entries.delete(path); + const prefix = `${path}/`; + for (const key of this.#entries.keys()) { + if (key.startsWith(prefix)) { + this.#entries.delete(key); + } + } + } + + clear(): void { + this.#entries.clear(); + } +} + /** What the caller of a request is, as far as `AUTH_SYS` can say. */ export interface NfsRequestContext { /** Remote address, for `DUMP` and for logging. */ @@ -209,6 +370,15 @@ export interface NfsSharedState { lock?: PathLock | undefined; /** The counters both versions increment. */ stats?: NfsSessionStats | undefined; + /** + * The verifiers of exclusive creates still worth remembering. + * + * Shared for the same reason the handle table is: a client that created a + * file exclusively over v3 and retransmits over v4 (or the reverse — the two + * are one server on one port) must be answered from one table, or the same + * request gets two different answers depending on which version carried it. + */ + exclusiveCreates?: ExclusiveCreates | undefined; } // --------------------------------------------------------------------------- diff --git a/src/nfs/v3/session.ts b/src/nfs/v3/session.ts index e2917f8..14244a4 100644 --- a/src/nfs/v3/session.ts +++ b/src/nfs/v3/session.ts @@ -73,9 +73,11 @@ import { MOUNTPROC3_UMNT, MOUNTPROC3_UMNTALL, NFS3_COOKIEVERFSIZE, + NFS3_CREATEVERFSIZE, NFS3_OK, NFS3_WRITEVERFSIZE, NFS3ERR_BAD_COOKIE, + NFS3ERR_EXIST, NFS3ERR_NOTSUPP, NFS3ERR_NOT_SYNC, NFS3ERR_TOOSMALL, @@ -181,6 +183,7 @@ import { import { type AccessRights, allowedAccess, + ExclusiveCreates, MAX_OFFSET, modeBitsOfFtype, NAME_MAX, @@ -320,12 +323,15 @@ export class Nfs3Session { readonly #snapshots: DirectorySnapshots; readonly #lock: PathLock; readonly #mounts: MountRecord[] = []; + /** The verifiers of `CREATE` with `EXCLUSIVE` — see `../util.ts`. */ + readonly #exclusives: ExclusiveCreates; #destroyed = false; /** - * `shared` is the router's: one handle table, one path lock and one stats - * object across every version this server speaks. Left out — which is what - * this session's own tests do — each is constructed here instead. + * `shared` is the router's: one handle table, one path lock, one stats object + * and one exclusive-create table across every version this server speaks. + * Left out — which is what this session's own tests do — each is constructed + * here instead. */ constructor(driver: FsDriver, options: NfsSessionOptions = {}, shared: NfsSharedState = {}) { this.driver = createLoopback(driver); @@ -341,6 +347,7 @@ export class Nfs3Session { // Derived from the handle table's boot verifier, so both change together. this.writeVerifier = this.handles.verifier.slice(0, NFS3_WRITEVERFSIZE); this.#snapshots = new DirectorySnapshots(options.snapshotCache); + this.#exclusives = shared.exclusiveCreates ?? new ExclusiveCreates(); } /** MOUNT registrations currently outstanding, in order. */ @@ -401,6 +408,7 @@ export class Nfs3Session { this.#destroyed = true; this.handles.clear(); this.#snapshots.clear(); + this.#exclusives.clear(); this.#mounts.length = 0; await Promise.resolve(); } @@ -724,6 +732,10 @@ export class Nfs3Session { throw new NfsStatusError(NFS3ERR_NOT_SYNC, "SETATTR guard does not match the ctime"); } await this.#applySattr(path, request.attributes, { current: stats }); + // §3.3.8's commit: this is the SETATTR an `EXCLUSIVE` create sends the + // client back for, and once it has landed the client is not going to + // retry the create. See `ExclusiveCreates` in `../util.ts`. + this.#exclusives.forget(path); writeWccRes(writer, { status: NFS3_OK, wcc: await this.#wcc(before, path) }); } catch (error) { writeWccRes(writer, { @@ -1106,13 +1118,9 @@ export class Nfs3Session { const path = joinPath(dir, this.#checkName(request.where.name, "open")); before = await this.#preOp(dir); if (request.mode === CREATE_EXCLUSIVE) { - // `EXCLUSIVE` asks the server to *store* the client's verifier - // somewhere it survives a retry, so a duplicated request recognises its - // own creation instead of failing `EEXIST`. There is nowhere in the - // driver interface to keep it (it would want an xattr), and inventing a - // side table that a restart loses would be a worse lie than saying no. - // Linux falls back to `GUARDED` on this status. - throw new NfsStatusError(NFS3ERR_NOTSUPP, "EXCLUSIVE create is not supported"); + await this.#createExclusive(dir, path, request.verf, creds); + await this.#created(writer, dir, before, path); + return; } const mode = request.attributes?.mode; const flags = @@ -1133,6 +1141,62 @@ export class Nfs3Session { return; } + /** + * `CREATE` with `EXCLUSIVE` (§3.3.8): create it, or recognise the retry. + * + * The mode exists because a client cannot make a create idempotent by itself: + * if the reply is lost, the resend cannot tell "I created it" from "someone + * else did". So the client sends a verifier it made up, and the server + * remembers which verifier created which file — a resend carrying the same + * one is answered with the file it already made, and a *different* verifier + * on the same name is the second client, which is the case that really is + * `NFS3ERR_EXIST`. + * + * What "remembers" means here — a bounded table in memory, so a retry is + * covered and a restart is not — is stated in full on `ExclusiveCreates` in + * `../util.ts`. §3.3.8 would rather that were stable storage and offers + * `NFS3ERR_NOTSUPP` to a server that cannot manage it, which is what this + * answered before the table existed. + * + * There are **no attributes**: `createhow3`'s `EXCLUSIVE` arm carries the + * verifier *instead of* an `sattr3`, and §3.3.8 has the client follow up with + * a SETATTR carrying the ones it wanted — the same SETATTR that retires the + * entry, since after it the client is no longer relying on the retry. The + * file is therefore created with the mode `UNCHECKED` uses when the client + * names none, and the client's SETATTR narrows it. + */ + async #createExclusive( + dir: string, + path: string, + verf: Uint8Array | undefined, + creds: RpcCredentials, + ): Promise { + const verifier = verf ?? new Uint8Array(NFS3_CREATEVERFSIZE); + let handle: FileHandleLike; + try { + handle = await this.driver.open( + path, + constants.O_WRONLY | constants.O_CREAT | constants.O_EXCL, + 0o666, + ); + } catch (error) { + if ((error as { code?: string }).code !== "EEXIST") { + throw error; + } + if (this.#exclusives.match(path, verifier) === undefined) { + throw new NfsStatusError(NFS3ERR_EXIST, "a different file already has that name"); + } + // The duplicate of a request whose reply never arrived. Nothing to + // create, nothing to invalidate, nothing to chown a second time — the + // caller answers with the file this client already made. + return; + } + await handle.close(); + this.#exclusives.set(path, verifier); + this.#invalidate(dir); + await this.#claim(path, creds); + } + async #mkdir(args: XdrReader, creds: RpcCredentials, writer: XdrWriter): Promise { const request = readMkdirArgs(args); args.end("MKDIR arguments"); @@ -1236,6 +1300,9 @@ export class Nfs3Session { if (entry !== undefined && directory) { this.#snapshots.delete(entry.id); } + // Whatever appears at this name next is a different file, and must not + // inherit the promise made about this one. + this.#exclusives.forget(path); writeWccRes(writer, { status: NFS3_OK, wcc: await this.#wcc(before, dir) }); } catch (error) { writeWccRes(writer, { @@ -1266,6 +1333,11 @@ export class Nfs3Session { // client goes on using the handle it already has, for the file and for // everything below it if this was a directory. this.handles.remap(from, to); + // The verifiers are keyed by *path*, and both of these paths now name + // something else: `from` nothing, `to` the file that moved onto it (over + // whatever was there). Neither promise survives the move. + this.#exclusives.forget(from); + this.#exclusives.forget(to); // Exactly two directories changed their names: the one that lost `from` // and the one that gained `to`. A directory that *moved* keeps its own // snapshot — same contents, and `remap` kept it the same entry — and no diff --git a/src/nfs/v4/session.ts b/src/nfs/v4/session.ts index f31d02c..d853ac9 100644 --- a/src/nfs/v4/session.ts +++ b/src/nfs/v4/session.ts @@ -105,6 +105,7 @@ import { import { type AccessRights, allowedAccess, + ExclusiveCreates, MAX_OFFSET, modeBitsOfFtype, NAME_MAX, @@ -297,6 +298,7 @@ import { type Compound4res, type Create4args, type Create4res, + type CreateHow4, type CreateSession4args, type CreateSession4res, type DestroyClientid4args, @@ -911,11 +913,14 @@ export class Nfs4Session { * encoded. */ readonly #released: string[] = []; + /** The verifiers of OPEN with `EXCLUSIVE4`/`EXCLUSIVE4_1` — see `../util.ts`. */ + readonly #exclusives: ExclusiveCreates; #destroyed = false; /** * `shared` is the router's — see `../util.ts`. Left out, this session makes - * its own handle table, lock and counters, which is what its own tests do. + * its own handle table, lock, counters and exclusive-create table, which is + * what its own tests do. */ constructor(driver: FsDriver, options: NfsSessionOptions = {}, shared: NfsSharedState = {}) { this.driver = createLoopback(driver); @@ -930,6 +935,7 @@ export class Nfs4Session { }); this.writeVerifier = this.handles.verifier.slice(0, NFS4_VERIFIER_SIZE); this.#snapshots = new DirectorySnapshots(options.snapshotCache); + this.#exclusives = shared.exclusiveCreates ?? new ExclusiveCreates(); // The ID map is this file's, not the state table's — see `../util.ts`. const { idmap, ...knobs } = options.nfs4 ?? {}; this.#idmap = idmap; @@ -1002,6 +1008,7 @@ export class Nfs4Session { async destroy(): Promise { this.#destroyed = true; this.#snapshots.clear(); + this.#exclusives.clear(); this.#maxOpsBySession.clear(); const open = [...this.#openHandles.keys(), ...this.#released]; this.#released.length = 0; @@ -1993,6 +2000,12 @@ export class Nfs4Session { return { status: statusOf(error), attrsset: [] }; } const applied = await this.#applyAttrs(path, args.objAttributes); + if (applied.status === NFS4_OK) { + // §18.16.4's commit: this is the SETATTR an exclusive create sends the + // client back for, and once it has landed the client is not going to + // retry the OPEN. See `ExclusiveCreates` in `../util.ts`. + this.#exclusives.forget(path); + } return { status: applied.status, attrsset: bitmapOf(applied.bits) }; } @@ -2457,6 +2470,9 @@ export class Nfs4Session { if (entry !== undefined && directory) { this.#snapshots.delete(entry.id); } + // Whatever appears at this name next is a different object, and must not + // inherit the promise made about this one. + this.#exclusives.forget(path); return { status: NFS4_OK, cinfo: this.#cinfo(before, await this.#changeOf(dir)) }; } @@ -2478,6 +2494,11 @@ export class Nfs4Session { // client goes on using the handle it already has, for the object and for // everything below it if this was a directory. this.handles.remap(from, to); + // The verifiers are keyed by *path*, and both of these paths now name + // something else: `from` nothing, `to` the object that moved onto it (over + // whatever was there). Neither promise survives the move. + this.#exclusives.forget(from); + this.#exclusives.forget(to); // Exactly two directories changed their names: the one that lost `from` and // the one that gained `to`. A directory that *moved* keeps its own snapshot // — same contents, and `remap` kept it the same entry — and no unrelated @@ -2944,20 +2965,15 @@ export class Nfs4Session { * "valid in general but ... not appropriate to the context in which the * stateid is used". * - * Exclusive create is the one place this file knowingly departs from a - * sentence of the RFC, and it is a contradiction inside the RFC: §18.16.4 - * says "if the server cannot support exclusive create semantics, possibly - * because of the requirement to commit the verifier to stable storage, it - * should fail the OPEN request with the error NFS4ERR_NOTSUPP", while - * §15.2's OPEN row and §15.4's `NFS4ERR_NOTSUPP` row — the two normative - * error tables, agreeing from both directions — do not admit that status for - * OPEN. `NFS4ERR_INVAL` is what they do admit, and it is the status §18.16.3 - * itself gives to the two neighbouring cases of "this exclusive-create form - * must not be used here" (a named attribute directory: "the server will - * return EINVAL"; a persistent session or a pNFS client ID: "the server MUST - * return NFS4ERR_INVAL"). There is nowhere in the driver interface to commit - * a verifier to, and a side table a restart loses would be a worse answer - * than a refusal — the same call `../v3/session.ts` makes about `EXCLUSIVE`. + * Exclusive create is served, out of a table in memory that covers a retry + * and not a restart — {@link Nfs4Session.#openCreateExclusive} below, and + * `ExclusiveCreates` in `../util.ts` for what that does and does not promise. + * §18.16.4 offers the other answer ("if the server cannot support exclusive + * create semantics, possibly because of the requirement to commit the + * verifier to stable storage, it should fail the OPEN request with the error + * NFS4ERR_NOTSUPP") and this file would not have been able to give it: §15.2's + * OPEN row and §15.4's `NFS4ERR_NOTSUPP` row — the two normative error + * tables, agreeing from both directions — do not admit that status for OPEN. * * On success the opened file becomes the current filehandle ("upon success * ... the current filehandle is replaced by that of the created or existing @@ -3101,7 +3117,9 @@ export class Nfs4Session { } /** - * The create half of an OPEN: `UNCHECKED4` and `GUARDED4` (§18.16.3). + * The create half of an OPEN: `UNCHECKED4` and `GUARDED4` (§18.16.3), with + * the two exclusive modes handed on to + * {@link Nfs4Session.#openCreateExclusive}. * * Answers `undefined` when the file is there to be opened, or the status that * refuses the whole OPEN. `attrset` is filled with what was actually applied, @@ -3124,7 +3142,7 @@ export class Nfs4Session { ): Promise { const how = args.openhow.how!; if (how.mode === EXCLUSIVE4 || how.mode === EXCLUSIVE4_1) { - return NFS4ERR_INVAL; + return this.#openCreateExclusive(how, path, cursor, attrset); } const attrs = how.createattrs!; this.#settableOrRefuse(attrs); @@ -3163,6 +3181,92 @@ export class Nfs4Session { return undefined; } + /** + * The other create half: `EXCLUSIVE4` and `EXCLUSIVE4_1` (§18.16.3). + * + * Both exist for one reason — a client cannot make a create idempotent by + * itself, because a lost reply leaves it unable to tell "I created it" from + * "someone else did". So it sends a verifier it made up, the server remembers + * which verifier created which file, and the resend carrying that same + * verifier is answered with the file it already made. A *different* verifier + * on the same name is a second client, and that is the case that really is + * `NFS4ERR_EXIST`. + * + * What "remembers" means — a bounded table in memory, so a retransmission is + * covered and a restart is not — is stated in full on `ExclusiveCreates` in + * `../util.ts`. + * + * The two modes differ only in what travels beside the verifier. `EXCLUSIVE4` + * carries nothing else, so the file is created with the mode `UNCHECKED4` + * uses when the client names none and §18.16.4's follow-up SETATTR sets the + * rest. `EXCLUSIVE4_1` carries `creatverfattr`, whose `cva_attrs` are applied + * and reported in `attrset` exactly as `createattrs` are on the other path — + * and are checked against the same supported/settable sets, which is the + * answer this server would give for `suppattr_exclcreat` (75) if it + * advertised it: the verifier lives beside the file rather than *in* one of + * its attributes, so no attribute is reserved and every settable one may be + * named here. That attribute stays out of `SUPPORTED_ATTRS` (`./attr.ts` + * says why), so a client sees the omission and sends what it would have sent + * anyway. + * + * The `attrset` is remembered with the verifier, so the reply to a resend + * carries the same set the lost one did rather than an empty one. + * + * §18.16.3's one refusal of `EXCLUSIVE4` — "if the client ID is + * pNFS-enabled, or the session is persistent, the server MUST return + * NFS4ERR_INVAL" — cannot arise: this server grants no `csa_flags` at all + * (`./state.ts` on CREATE_SESSION), so no session is persistent, and there + * is no pNFS here to enable. A check for it would be unreachable code + * describing a state the server cannot enter. + */ + async #openCreateExclusive( + how: CreateHow4, + path: string, + cursor: Cursor, + attrset: number[], + ): Promise { + const exclusive4 = how.mode === EXCLUSIVE4; + const verifier = exclusive4 ? how.createverf! : how.createboth!.verf; + const attrs = exclusive4 ? undefined : how.createboth!.attrs; + if (attrs !== undefined) { + this.#settableOrRefuse(attrs); + } + const created = await this.#createFile(path, (attrs?.values.mode ?? 0o666) & 0o7777); + if (!created) { + const remembered = this.#exclusives.match(path, verifier); + if (remembered === undefined) { + return NFS4ERR_EXIST; + } + // The duplicate of a request whose reply never arrived: same file, same + // answer. The OPEN below it proceeds as it did the first time. + attrset.push(...remembered.attrset); + return undefined; + } + await this.#claim(path, cursor.creds); + const bits: number[] = []; + let status = NFS4_OK; + if (attrs !== undefined) { + // The mode went in as `open`'s third argument, so `#applyAttrs` must not + // see it — and must see everything else, size included. + const applied = await this.#applyAttrs(path, { + ...attrs, + values: { ...attrs.values, mode: undefined }, + }); + if (attrs.values.mode !== undefined) { + bits.push(FATTR4_MODE); + } + bits.push(...applied.bits); + status = applied.status; + } + // Recorded even when an attribute failed to land: the file exists and this + // verifier is what made it, so a resend must still be recognised — and it + // is answered with the bits that *did* land, which is the partial `attrset` + // §18.16.4 tells client implementors to check for. + this.#exclusives.set(path, verifier, bits); + attrset.push(...bits); + return status === NFS4_OK ? undefined : status; + } + /** Create a file, answering whether it was this call that created it. */ async #createFile(path: string, mode: number): Promise { try { diff --git a/test/nfs/session.test.ts b/test/nfs/session.test.ts index 1b40a54..c388348 100644 --- a/test/nfs/session.test.ts +++ b/test/nfs/session.test.ts @@ -16,9 +16,11 @@ * directly; `server.ts` adds nothing this file is about. * * What the router *owns* rather than routes — the one handle table, the one - * counters object shared by both versions — is asserted in - * `test/nfs/v4/session.test.ts`, where there is a v4 client to obtain a handle - * with. This file is the switch itself. + * counters object, the one exclusive-create table shared by both versions — is + * asserted in `test/nfs/v4/session.test.ts`, where there is a v4 client to + * obtain a handle with and a v3 one beside it. This file is the switch itself, + * plus the one owned object with edges no client can reach: the block at the + * bottom is `ExclusiveCreates`' window and size cap. */ import { describe, expect, it } from "vitest"; @@ -33,6 +35,11 @@ import { RPC_SUCCESS, } from "../../src/nfs/rpc.ts"; import { NfsSession } from "../../src/nfs/session.ts"; +import { + DEFAULT_EXCLUSIVE_CREATES, + EXCLUSIVE_CREATE_WINDOW_MS, + ExclusiveCreates, +} from "../../src/nfs/util.ts"; import { MOUNT_PROGRAM, MOUNT_V3, NFS_PROGRAM, NFS_V3 } from "../../src/nfs/v3/constants.ts"; import { NFS4_OK, @@ -199,3 +206,106 @@ describe("the version router: which session gets the record", () => { expect(results.u32("status")).toBe(NFS4_OK); }); }); + +// --------------------------------------------------------------------------- +// the exclusive-create table +// --------------------------------------------------------------------------- + +/** + * The other thing the router owns: one table of exclusive-create verifiers + * across both versions. + * + * What it *means* — a v3 `CREATE EXCLUSIVE` and a v4 `OPEN EXCLUSIVE4` are the + * same promise — is asserted through the wire in the two versioned suites. + * This block is about the promise's edges, which are deliberately not + * reachable from a client: the window and the size cap. Both are bounds rather + * than protocol, so they are tested where they are written. + */ +describe("ExclusiveCreates", () => { + const VERIFIER = Uint8Array.from([1, 2, 3, 4, 5, 6, 7, 8]); + const OTHER = Uint8Array.from([9, 9, 9, 9, 9, 9, 9, 9]); + + it("matches the verifier that created a path, and nothing else", () => { + const table = new ExclusiveCreates(); + table.set("/dir/file", VERIFIER, [33]); + expect(table.match("/dir/file", VERIFIER)?.attrset).toEqual([33]); + // A second client on the same name, and the same client on another name. + expect(table.match("/dir/file", OTHER)).toBeUndefined(); + expect(table.match("/dir/other", VERIFIER)).toBeUndefined(); + }); + + it("copies the verifier and the attrset it is handed", () => { + const table = new ExclusiveCreates(); + const verifier = Uint8Array.from(VERIFIER); + const attrset = [33]; + table.set("/f", verifier, attrset); + // The request buffer is reused and the caller goes on pushing to its + // array; neither may reach back into the table. + verifier.fill(0); + attrset.push(4); + expect(table.match("/f", VERIFIER)?.attrset).toEqual([33]); + }); + + it("forgets an entry past the window, and a lookup does not extend it", () => { + let now = 1000; + const table = new ExclusiveCreates({ windowMs: 100, now: () => now }); + table.set("/f", VERIFIER); + now += 60; + expect(table.match("/f", VERIFIER)).toBeDefined(); + // Still measured from the *set*, not from that match. + now += 60; + expect(table.match("/f", VERIFIER)).toBeUndefined(); + // And the expired entry is gone rather than merely unmatched. + expect(table.size).toBe(0); + }); + + it("keeps at most `limit` entries, oldest insertion out first", () => { + const table = new ExclusiveCreates({ limit: 2 }); + table.set("/a", VERIFIER); + table.set("/b", VERIFIER); + table.set("/c", VERIFIER); + expect(table.size).toBe(2); + // Evicting is the safe failure: the retry stops being recognised, which + // the session answers `EXIST`. + expect(table.match("/a", VERIFIER)).toBeUndefined(); + expect(table.match("/b", VERIFIER)).toBeDefined(); + expect(table.match("/c", VERIFIER)).toBeDefined(); + }); + + it("holds the documented default, and no more", () => { + const table = new ExclusiveCreates(); + for (let index = 0; index <= DEFAULT_EXCLUSIVE_CREATES; index++) { + table.set(`/f${index}`, VERIFIER); + } + expect(table.size).toBe(DEFAULT_EXCLUSIVE_CREATES); + expect(table.match("/f0", VERIFIER)).toBeUndefined(); + // Two minutes: a handful of RPC retransmits, not a lease. + expect(EXCLUSIVE_CREATE_WINDOW_MS).toBe(120_000); + }); + + it("forgets a path, and everything under it", () => { + const table = new ExclusiveCreates(); + table.set("/dir/file", VERIFIER); + table.set("/dir/sub/file", VERIFIER); + table.set("/dirty", VERIFIER); + // A removed or renamed *directory* takes the names below it with it — and + // takes nothing that merely shares a prefix. + table.forget("/dir"); + expect(table.match("/dir/file", VERIFIER)).toBeUndefined(); + expect(table.match("/dir/sub/file", VERIFIER)).toBeUndefined(); + expect(table.match("/dirty", VERIFIER)).toBeDefined(); + + // ...and the root sweeps nothing, which is what a SETATTR on `/` should + // say about the files inside it. + table.set("/dirty/file", VERIFIER); + table.forget("/"); + expect(table.match("/dirty", VERIFIER)).toBeDefined(); + expect(table.match("/dirty/file", VERIFIER)).toBeDefined(); + + table.clear(); + expect(table.size).toBe(0); + // Forgetting into an empty table is the common case, and does nothing. + table.forget("/dirty"); + expect(table.size).toBe(0); + }); +}); diff --git a/test/nfs/v3/session.test.ts b/test/nfs/v3/session.test.ts index d1e9b20..0f9ffe3 100644 --- a/test/nfs/v3/session.test.ts +++ b/test/nfs/v3/session.test.ts @@ -67,6 +67,9 @@ import { createLoopback, type Loopback } from "../../../src/harness.ts"; const encoder = new TextEncoder(); +/** One client's `createverf3`: eight bytes it made up, and keeps resending. */ +const VERIFIER = new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8]); + interface Harness { server: NfsServer; client: NfsClient; @@ -732,24 +735,90 @@ describe("individual procedures", () => { ); }); - it("implements all three CREATE modes, EXCLUSIVE as NOTSUPP", async () => { + it("implements all three CREATE modes", async () => { const { client, root } = await serve(); expect(check(await client.create(root, "f", CREATE_UNCHECKED), "create").obj).toBeDefined(); // UNCHECKED over an existing file succeeds and applies the attributes. const again = check(await client.create(root, "f", CREATE_UNCHECKED, { size: 0n }), "create"); expect(again.objAttributes!.size).toBe(0n); expect((await client.create(root, "f", CREATE_GUARDED)).status).toBe(NFS3ERR_EXIST); - // EXCLUSIVE wants the verifier stored somewhere that survives a retry, and - // there is nowhere in the driver interface to put it. Linux falls back. - const exclusive = await client.create( - root, - "excl", - CREATE_EXCLUSIVE, - {}, - new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8]), + // EXCLUSIVE creates like GUARDED, and carries a verifier instead of an + // `sattr3` — so there are no attributes to apply and none are. + const created = check( + await client.create(root, "excl", CREATE_EXCLUSIVE, {}, VERIFIER), + "create", + ); + expect(created.obj).toBeDefined(); + expect(created.objAttributes!.type).toBe(NF3REG); + expect(check(await client.lookup(root, "excl"), "lookup").object).toEqual(created.obj); + }); + + /** + * §3.3.8's whole point: a client cannot make a create idempotent by itself, + * because a lost reply leaves it unable to tell "I created it" from "someone + * else did". The verifier is how it tells the difference, and the table + * behind it is `ExclusiveCreates` in `src/nfs/util.ts` — a retry, not a + * restart. + */ + it("answers the retry of an EXCLUSIVE create with the file it already made", async () => { + const { client, root, fs } = await serve(); + const first = check( + await client.create(root, "excl", CREATE_EXCLUSIVE, {}, VERIFIER), + "create", + ); + await fs.writeFile("/excl", "written after the create"); + + // The duplicate of a request whose reply never arrived: the same file + // handle back, `NFS3_OK` rather than `EXIST`, and nothing truncated. + const retry = check( + await client.create(root, "excl", CREATE_EXCLUSIVE, {}, VERIFIER), + "create", + ); + expect([...retry.obj!]).toEqual([...first.obj!]); + expect(retry.objAttributes!.size).toBe(24n); + expect(new TextDecoder().decode(await fs.readFile("/excl"))).toBe("written after the create"); + + // A different verifier on the same name is a *second client*, which is the + // one case that really is `NFS3ERR_EXIST`. + const other = new Uint8Array([9, 9, 9, 9, 9, 9, 9, 9]); + expect((await client.create(root, "excl", CREATE_EXCLUSIVE, {}, other)).status).toBe( + NFS3ERR_EXIST, + ); + }); + + it("forgets the verifier once the client commits with SETATTR", async () => { + const { client, root } = await serve(); + const created = check( + await client.create(root, "excl", CREATE_EXCLUSIVE, {}, VERIFIER), + "create", + ); + // The follow-up §3.3.8 sends the client back for, since EXCLUSIVE had + // nowhere to carry the mode. After it, the client is not retrying. + expect((await client.setattr(created.obj!, { mode: 0o600 })).status).toBe(NFS3_OK); + expect((await client.create(root, "excl", CREATE_EXCLUSIVE, {}, VERIFIER)).status).toBe( + NFS3ERR_EXIST, + ); + }); + + it("forgets the verifier when the name stops meaning that file", async () => { + const { client, root } = await serve(); + // Removed, then re-created by somebody else: the name is a different file + // now, and the promise made about the first one must not cover it. + check(await client.create(root, "gone", CREATE_EXCLUSIVE, {}, VERIFIER), "create"); + expect((await client.remove(root, "gone")).status).toBe(NFS3_OK); + check(await client.create(root, "gone", CREATE_UNCHECKED), "create"); + expect((await client.create(root, "gone", CREATE_EXCLUSIVE, {}, VERIFIER)).status).toBe( + NFS3ERR_EXIST, + ); + + // Same again through a rename, which moves the file out from under the + // name rather than deleting it. + check(await client.create(root, "moved", CREATE_EXCLUSIVE, {}, VERIFIER), "create"); + expect((await client.rename(root, "moved", root, "elsewhere")).status).toBe(NFS3_OK); + check(await client.create(root, "moved", CREATE_UNCHECKED), "create"); + expect((await client.create(root, "moved", CREATE_EXCLUSIVE, {}, VERIFIER)).status).toBe( + NFS3ERR_EXIST, ); - expect(exclusive.status).toBe(NFS3ERR_NOTSUPP); - expect((await client.lookup(root, "excl")).status).toBe(NFS3ERR_NOENT); }); it("answers MKNOD with NOTSUPP when the driver has no mknod extension", async () => { diff --git a/test/nfs/v4/client.ts b/test/nfs/v4/client.ts index cba48bb..2ae85f1 100644 --- a/test/nfs/v4/client.ts +++ b/test/nfs/v4/client.ts @@ -53,6 +53,7 @@ import { ACCESS4_ALL, CLAIM_NULL, errnoCodeOfStatus4, + EXCLUSIVE4_1, FATTR4_CHANGE, FATTR4_FILEID, FATTR4_FILES_AVAIL, @@ -1248,6 +1249,16 @@ export class Nfs4Client { mode?: number; owner?: Uint8Array; path?: string; + /** + * A `createverf4`, which makes the create *exclusive* — the retry of a + * create whose reply was lost is answered with the file it made rather + * than `NFS4ERR_EXIST` (§18.16.3). + * + * Sent as `EXCLUSIVE4_1`, carrying the same attributes the other create + * modes send: a 4.1 client has no reason to use `EXCLUSIVE4` and lose + * them. Outranks `exclusive`, which is only `O_EXCL`'s `GUARDED4`. + */ + verifier?: Uint8Array; } = {}, ): Promise { const access = options.access ?? OPEN4_SHARE_ACCESS_READ; @@ -1259,15 +1270,24 @@ export class Nfs4Client { } } const openhow = - options.create === true - ? { + options.create !== true + ? { opentype: OPEN4_NOCREATE } + : { opentype: OPEN4_CREATE, - how: { - mode: options.exclusive === true ? GUARDED4 : UNCHECKED4, - createattrs: fattr(settableMask(values), values), - }, - } - : { opentype: OPEN4_NOCREATE }; + how: + options.verifier === undefined + ? { + mode: options.exclusive === true ? GUARDED4 : UNCHECKED4, + createattrs: fattr(settableMask(values), values), + } + : { + mode: EXCLUSIVE4_1, + createboth: { + verf: options.verifier, + attrs: fattr(settableMask(values), values), + }, + }, + }; const owner = options.owner ?? encoder.encode(`open-owner-${this.#owners++}`); const reply = checkCompound( await this.compound( diff --git a/test/nfs/v4/session.test.ts b/test/nfs/v4/session.test.ts index a503202..54a04e4 100644 --- a/test/nfs/v4/session.test.ts +++ b/test/nfs/v4/session.test.ts @@ -209,6 +209,7 @@ import { XdrWriter } from "../../../src/nfs/xdr.ts"; import { createLoopback } from "../../../src/harness.ts"; import { createNfsServer, type NfsServer } from "../../../src/nfs/server.ts"; import { withoutExtensions } from "../../no-extensions.ts"; +import { CREATE_EXCLUSIVE, NFS3ERR_EXIST } from "../../../src/nfs/v3/constants.ts"; import { check as check3, NfsClient as Nfs3Client, nfsDriver } from "../v3/client.ts"; import { type Compound4reply, @@ -2014,6 +2015,40 @@ function createHow(mode: number, attrs: { bits?: number[]; values?: Fattr4Values }; } +/** + * `createhow4` for the two exclusive modes: `EXCLUSIVE4` carries the verifier + * alone, `EXCLUSIVE4_1` carries it beside a `cva_attrs` the other modes would + * have sent as `createattrs`. + */ +function exclusiveHow( + mode: number, + verf: Uint8Array, + attrs?: { bits?: number[]; values?: Fattr4Values }, +): unknown { + return { + opentype: OPEN4_CREATE, + how: + mode === EXCLUSIVE4 + ? { mode, createverf: verf } + : { + mode, + createboth: { + verf, + attrs: { + attrmask: bitmapOf(attrs?.bits ?? []), + values: attrs?.values ?? {}, + unsupported: [], + }, + }, + }, + }; +} + +/** One client's verifier: eight bytes it made up, and keeps resending. */ +const VERIFIER = Uint8Array.from([1, 2, 3, 4, 5, 6, 7, 8]); +/** A second client's, for the same name. */ +const OTHER_VERIFIER = Uint8Array.from([9, 9, 9, 9, 9, 9, 9, 9]); + /** Open `/dir/file` and hand back the stateid, with the file as the current FH. */ async function opened(client: Client, options: OpenOptions = {}): Promise { const reply = await client.run([...TO_DIR, OPEN(options)]); @@ -2204,36 +2239,142 @@ describe("OPEN", () => { expect(reply.compound.status).toBe(NFS4ERR_INVAL); }); - it("refuses both exclusive create modes with NFS4ERR_INVAL", async () => { - const session = new Nfs4Session(await populated()); - const client = await ready(session); + /** + * §18.16.3's exclusive create, in both its shapes. The point of the mode is + * that a client cannot make a create idempotent by itself: a lost reply + * leaves it unable to tell "I created it" from "someone else did", so it + * sends a verifier and the server remembers which verifier made which file. + * `ExclusiveCreates` in `src/nfs/util.ts` states what that memory does and + * does not cover — a retransmission, not a restart. + */ + it("creates with either exclusive mode and answers the retry with the same file", async () => { for (const mode of [EXCLUSIVE4, EXCLUSIVE4_1]) { - const reply = await client.run([ + const session = new Nfs4Session(await populated()); + const client = await ready(session); + const first = await client.run([ ...TO_DIR, - OPEN({ - name: "exclusive", - openhow: { - opentype: OPEN4_CREATE, - how: - mode === EXCLUSIVE4 - ? { mode, createverf: Uint8Array.from([1, 2, 3, 4, 5, 6, 7, 8]) } - : { - mode, - createboth: { - verf: Uint8Array.from([1, 2, 3, 4, 5, 6, 7, 8]), - attrs: { attrmask: [], values: {}, unsupported: [] }, - }, - }, - }, - }), + OPEN({ name: "exclusive", openhow: exclusiveHow(mode, VERIFIER) }), + { op: OP_GETFH }, ]); - // There is nowhere to commit a verifier to. §18.16.4 would have this be - // NFS4ERR_NOTSUPP, but §15.2's OPEN row and §15.4's NFS4ERR_NOTSUPP row - // both exclude that status for OPEN, and NFS4ERR_INVAL is what §18.16.3 - // itself gives the neighbouring "this form must not be used here" cases. - expect(reply.compound.status).toBe(NFS4ERR_INVAL); + expect(first.compound.status).toBe(NFS4_OK); + const created = resFor(first, OP_GETFH).object!; + expect(session.handles.resolve(created)).toBe("/dir/exclusive"); + + // The duplicate of a request whose reply never arrived: `NFS4_OK` and + // the same file, where `GUARDED4` would now say NFS4ERR_EXIST. + const retry = await client.run([ + ...TO_DIR, + OPEN({ name: "exclusive", owner: 2, openhow: exclusiveHow(mode, VERIFIER) }), + { op: OP_GETFH }, + ]); + expect(retry.compound.status).toBe(NFS4_OK); + expect([...resFor(retry, OP_GETFH).object!]).toEqual([...created]); + + // A different verifier on the same name is a *second client*, which is + // the one case that really is NFS4ERR_EXIST. + const rival = await client.run([ + ...TO_DIR, + OPEN({ name: "exclusive", owner: 3, openhow: exclusiveHow(mode, OTHER_VERIFIER) }), + ]); + expect(rival.compound.status).toBe(NFS4ERR_EXIST); } - await expect(session.driver.stat("/dir/exclusive")).rejects.toThrow(); + }); + + it("applies EXCLUSIVE4_1's cva_attrs and replays the same attrset", async () => { + const session = new Nfs4Session(await populated()); + const client = await ready(session); + const how = exclusiveHow(EXCLUSIVE4_1, VERIFIER, { + bits: [FATTR4_MODE, FATTR4_SIZE], + values: { mode: 0o640, size: 0n }, + }); + const reply = await client.run([ + ...TO_DIR, + OPEN({ name: "both", access: OPEN4_SHARE_ACCESS_BOTH, openhow: how }), + GETATTR([FATTR4_MODE]), + ]); + expect(reply.compound.status).toBe(NFS4_OK); + expect(attrsFor(reply).mode).toBe(0o640); + // The same "which attributes were successfully set" answer the other two + // create modes give — `creatverfattr` is the only difference on the wire. + const attrset = resFor(reply, OP_OPEN).attrset; + expect(attrset).toEqual(bitmapOf([FATTR4_MODE, FATTR4_SIZE])); + + // And the replay carries the set the lost reply carried, not an empty one. + const retry = await client.run([ + ...TO_DIR, + OPEN({ name: "both", owner: 2, access: OPEN4_SHARE_ACCESS_BOTH, openhow: how }), + ]); + expect(retry.compound.status).toBe(NFS4_OK); + expect(resFor(retry, OP_OPEN).attrset).toEqual(attrset); + + // `EXCLUSIVE4` has no attributes to report, and reports none. + const bare = await client.run([ + ...TO_DIR, + OPEN({ name: "bare", openhow: exclusiveHow(EXCLUSIVE4, VERIFIER) }), + ]); + expect(bare.compound.status).toBe(NFS4_OK); + expect(resFor(bare, OP_OPEN).attrset).toEqual(bitmapOf([])); + }); + + it("refuses an unsettable cva_attrs the way createattrs is refused", async () => { + const session = new Nfs4Session(await populated()); + const client = await ready(session); + // Nothing is reserved to hold the verifier, so what may be named here is + // exactly what SETATTR may name — and `fileid` is read-only in both. + const reply = await client.run([ + ...TO_DIR, + OPEN({ + name: "readonly-attr", + openhow: exclusiveHow(EXCLUSIVE4_1, VERIFIER, { + bits: [FATTR4_FILEID], + values: { fileid: 7n }, + }), + }), + ]); + expect(reply.compound.status).toBe(NFS4ERR_INVAL); + await expect(session.driver.stat("/dir/readonly-attr")).rejects.toThrow(); + }); + + it("forgets the verifier once SETATTR commits it, or the name stops meaning that file", async () => { + const session = new Nfs4Session(await populated()); + const client = await ready(session); + const create = (name: string, owner: number): Op[] => [ + ...TO_DIR, + OPEN({ name, owner, openhow: exclusiveHow(EXCLUSIVE4, VERIFIER) }), + ]; + + // The follow-up §18.16.4 sends the client back for. After it, the client + // is not retrying the OPEN, and the entry has nothing left to protect. + expect((await client.run(create("committed", 1))).compound.status).toBe(NFS4_OK); + const set = await client.run([ + ...TO_DIR, + { op: OP_LOOKUP, args: { objname: "committed" } }, + { + op: OP_SETATTR, + args: { + stateid: ANONYMOUS, + objAttributes: { + attrmask: bitmapOf([FATTR4_MODE]), + values: { mode: 0o600 }, + unsupported: [], + }, + }, + }, + ]); + expect(set.compound.status).toBe(NFS4_OK); + expect((await client.run(create("committed", 2))).compound.status).toBe(NFS4ERR_EXIST); + + // Removed and re-created by somebody else: the name is a different file + // now, and the promise made about the first one must not cover it. + expect((await client.run(create("recycled", 3))).compound.status).toBe(NFS4_OK); + const removed = await client.run([...TO_DIR, { op: OP_REMOVE, args: { target: "recycled" } }]); + expect(removed.compound.status).toBe(NFS4_OK); + const replaced = await client.run([ + ...TO_DIR, + OPEN({ name: "recycled", owner: 4, openhow: createHow(UNCHECKED4) }), + ]); + expect(replaced.compound.status).toBe(NFS4_OK); + expect((await client.run(create("recycled", 5))).compound.status).toBe(NFS4ERR_EXIST); }); it("answers a reclaim with NFS4ERR_NO_GRACE, whichever claim asks", async () => { @@ -3695,6 +3836,42 @@ describe("the v4 client over a real socket", () => { expect(server.session.stats.procedures.get("MOUNT:MNT")).toBe(1); }); + /** + * The exclusive-create verifiers are the router's, like the handle table: + * one server on one port, so the same request must get the same answer + * whichever version carried it. A client that created over v3 and retries + * over v4 has not changed its mind about anything. + */ + it("recognises a v3 exclusive create retried over v4, from one table", async () => { + const server = await serve(); + const v3 = await connect3(server); + const v4 = await connect(server); + const root = await v4.rootFh(); + + const created = check3( + await v3.client.create(v3.root, "excl", CREATE_EXCLUSIVE, {}, VERIFIER), + "create", + ); + const retried = await v4.openAt(root, "excl", { + create: true, + verifier: VERIFIER, + access: OPEN4_SHARE_ACCESS_BOTH, + }); + expect([...retried.fh]).toEqual([...created.obj!]); + // v3's `EXCLUSIVE` carried no attributes, so none were applied and the + // replay says so rather than claiming this OPEN's `cva_attrs` landed. + await retried.close(); + + // And a second client's verifier is refused on the same name, again + // whichever version asks. + await expect( + v4.openAt(root, "excl", { create: true, verifier: OTHER_VERIFIER }), + ).rejects.toThrow(); + expect( + (await v3.client.create(v3.root, "excl", CREATE_EXCLUSIVE, {}, OTHER_VERIFIER)).status, + ).toBe(NFS3ERR_EXIST); + }); + it("destroys its session and leaves the next request without one", async () => { const server = await serve(); const client = await connect(server); From dfc88efe8a8ddceafa9925890accb3464cd0953c Mon Sep 17 00:00:00 2001 From: Pooya Parsa Date: Fri, 31 Jul 2026 19:13:01 +0000 Subject: [PATCH 3/7] feat(nfs): set-group-ID inheritance for new entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A driver creates everything as the server process, so each session hands the result to its caller afterwards. The group half of that was wrong: it gave every new entry the caller's gid, where a set-group-ID parent means the entry takes the *parent's* group and a new directory takes the bit as well. `src/ownership.ts` is that rule, once, transcribed from `inode_init_owner()` (fs/inode.c) — the place the kernel makes this decision for every local filesystem and makes for no userspace one — with POSIX mkdir(2)/open(2) declining to pick between the two arms and inode(7) stating the choice. It also clears S_ISGID on a new group-executable whose creator is in neither the effective nor the supplementary group AUTH_SYS carried, reading CAP_FSETID as uid 0, which is all a userspace server can see of a remote caller's capabilities. No new stat: v3 already read the parent for wcc_data (#preOp is now split so the stat can be kept), v4 already read it for change_info4 and the NOTDIR check. The only added driver call is the chmod a new directory needs, and it runs after the lchown because chown(2) clears set-group-ID in the other order. Rename is untouched — the rule is about creation. 9P needs nothing: its client computes both halves (v9fs_get_fsgid_for_create()) and they arrive on the wire; its #claim comment said it matched NFS character for character and now says that instead. FUSE is left as a named gap — nothing on that path reads the parent, and the membership half wants FUSE_CREATE_SUPP_GROUP (7.38), which init.ts does not request. Per-call caller credentials in FsDriver were evaluated and declined; the roadmap entry now carries the reasoning and what would reopen it. Co-Authored-By: Claude Opus 5 --- .agents/architecture.md | 26 ++-- .agents/pjdfstest-results.md | 8 +- .agents/roadmap.md | 38 +++++- docs/2.transports/4.nfs.md | 4 +- src/9p/session.ts | 19 ++- src/fuse/session.ts | 24 +++- src/nfs/util.ts | 9 +- src/nfs/v3/session.ts | 79 ++++++++---- src/nfs/v4/session.ts | 57 ++++++--- src/ownership.ts | 152 ++++++++++++++++++++++ src/types.ts | 6 + test/index.test.ts | 116 ++++++++++++++++- test/nfs/v3/session.test.ts | 162 ++++++++++++++++++++++- test/nfs/v4/session.test.ts | 242 +++++++++++++++++++++++++++++++++-- 14 files changed, 850 insertions(+), 92 deletions(-) create mode 100644 src/ownership.ts diff --git a/.agents/architecture.md b/.agents/architecture.md index cb59182..ec39763 100644 --- a/.agents/architecture.md +++ b/.agents/architecture.md @@ -27,15 +27,16 @@ Deviations are noted per area below. ## Core (`src/`) -| File | What | -| ------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `types.ts` | `FsDriver`, `FsCapabilities`, `StatsLike`/`DirentLike`/`FileHandleLike`, and the `mountx.*` namespace — **two live members**, `mknod` and `utimens`, no xattr | -| `errors.ts` | `ERRNO_CODES` (Linux), `fsError()` (byte-identical to `node:fs`'s), `errnoOf()` — the one errno table in the repo | -| `path.ts` | absolute POSIX helpers, `..` clamps at root; canonical paths early-return, `resolvePath()` returns `{ path, segments }` | -| `harness.ts` | `createLoopback(driver)` — normalize, fill gaps with `ENOSYS`, resolve capabilities. The method table is fixed **at construction** | -| `lock.ts` | `PathLock` — `RENAME` takes it, `READ`/`WRITE` run outside it | -| `subtree.ts` | `remapSubtree()` — the rename rewrite; internal, deliberately not in the public `path.ts` | -| `auto.ts` | `mountx/auto` — probe, then FUSE → 9P → NFS, each behind `await import()` | +| File | What | +| -------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `types.ts` | `FsDriver`, `FsCapabilities`, `StatsLike`/`DirentLike`/`FileHandleLike`, and the `mountx.*` namespace — **two live members**, `mknod` and `utimens`, no xattr | +| `errors.ts` | `ERRNO_CODES` (Linux), `fsError()` (byte-identical to `node:fs`'s), `errnoOf()` — the one errno table in the repo | +| `path.ts` | absolute POSIX helpers, `..` clamps at root; canonical paths early-return, `resolvePath()` returns `{ path, segments }` | +| `harness.ts` | `createLoopback(driver)` — normalize, fill gaps with `ENOSYS`, resolve capabilities. The method table is fixed **at construction** | +| `lock.ts` | `PathLock` — `RENAME` takes it, `READ`/`WRITE` run outside it | +| `subtree.ts` | `remapSubtree()` — the rename rewrite; internal, deliberately not in the public `path.ts` | +| `ownership.ts` | who a new entry belongs to: `inode_init_owner()`'s set-gid rule, plus the `lchown`/`chmod` that applies it. Internal; used by the two NFS sessions' `#claim` | +| `auto.ts` | `mountx/auto` — probe, then FUSE → 9P → NFS, each behind `await import()` | ### Drivers (`src/drivers/`) @@ -177,6 +178,13 @@ The facts no single file's header can own. transport (FUSE keeps the orphan for a later `FORGET`, NFS drops the key, 9P releases the qid identity). Cost is O(tracked paths) per rename; a trie is the real fix and waits on a benchmark that wants it. +- **`src/ownership.ts` is one rule for four sessions, and two of them use it.** The + NFS sessions apply it from the parent `stat` they already took for `wcc_data` / + `change_info4`; 9P's client computes the same rule and puts the answer on the wire + (`v9fs_get_fsgid_for_create()`), so the session would be disagreeing with the + kernel if it applied it again; FUSE's would need an `lstat` per create that nothing + there does today, and `FUSE_CREATE_SUPP_GROUP` for the membership half. Each of the + three states its own reason at its `#claim`. - **`v3/` and `v4/` never import from each other.** `nfs/session.ts` and `nfs/util.ts` are the only seam, and the router hands both versions one shared `FileHandleTable`/`PathLock`. diff --git a/.agents/pjdfstest-results.md b/.agents/pjdfstest-results.md index 720485d..96d94b3 100644 --- a/.agents/pjdfstest-results.md +++ b/.agents/pjdfstest-results.md @@ -164,8 +164,12 @@ without a line of rename code changing. reason `#claim` sets only `uid`/`gid`. - **Known gaps that are ours but not pjdfstest's.** Set-gid directory inheritance (a new entry in a set-gid directory should take its parent's - group) is not implemented; pjdfstest does not cover it, and the honest fix is - caller credentials in the driver interface rather than more work in `#claim`. + group; a new subdirectory takes the bit too) is not implemented **on this + transport**. The rule shipped as `src/ownership.ts` and both NFS sessions + apply it, but the FUSE session does not — `#claim` there states the two + reasons, and `.agents/roadmap.md` carries the item. pjdfstest does not cover + it, so none of the numbers above move either way: they were not re-measured + for that change and did not need to be. ## Running it diff --git a/.agents/roadmap.md b/.agents/roadmap.md index 27dcd5d..72cd974 100644 --- a/.agents/roadmap.md +++ b/.agents/roadmap.md @@ -130,8 +130,36 @@ rather than by accident. A generation-stamped LRU is the fix if a workload needs a bound on _that_ — it is the only remaining growth, and the one an LRU can serve without breaking a live client. -- **Set-gid inheritance and caller credentials in the driver interface.** A new - entry in a set-gid directory should take its parent's group; more generally, - supplementary groups and per-call caller credentials want to be first-class in - `FsDriver` rather than patched on after the fact (the way new-inode ownership - is today, via a post-hoc `lchown` in each session's `#claim`). +- **Set-gid inheritance on the FUSE session.** The rule itself now exists — + `src/ownership.ts`, a transcription of `inode_init_owner()` — and both NFS + sessions apply it: a new entry in a set-gid directory takes the parent's + group, a new directory takes the bit, and a new group-executable loses + `S_ISGID` when its creator is in neither the effective nor the supplementary + group `AUTH_SYS` carried. 9P needs nothing: its client computes both halves + (`v9fs_get_fsgid_for_create()`) and they arrive on the wire. **FUSE is the + gap**, and `#claim` in `src/fuse/session.ts` says why it was not simply + mirrored: the NFS sessions read the parent anyway (for `wcc_data` and + `change_info4`) while nothing on the FUSE path does, so the rule would add an + `lstat` to every create for every caller — including the daemon-is-the-caller + case that currently costs no driver call at all — and the membership half + needs `FUSE_CREATE_SUPP_GROUP` (7.38), which `src/fuse/init.ts` does not ask + for. Both are decisions to take deliberately, not omissions. +- **Per-call caller credentials in `FsDriver`: considered, and not the + answer.** The shape was a `mountx.*` extension carrying the caller's + uid/gid/groups into each creating call, so ownership would be the driver's + from the start instead of a post-hoc `lchown` in `#claim`. Three things + against it, and they compound: `node:fs/promises` has no credential parameter + and the POSIX way to get one — `setfsuid(2)`/`setfsgid(2)` — is per-thread + state Node does not expose and the threadpool would scramble, so `node-fs` + could never implement it and `unstorage`/S3 have no ownership to implement it + with; the memory driver, the only one that could, enforces no permissions at + all, so the sole observable effect of a credential inside it is the initial + owner and group, which `src/ownership.ts` now gets right; and invariant 5 + means every session must keep the `lchown` path anyway for drivers without + the extension, so the whole surface buys one driver's atomicity at the price + of two permanent code paths. What would reopen it is a driver that enforces + permissions itself — at that point the credential decides whether a call + _succeeds_, not just who owns the result, and `#claim` cannot express that at + all. Note that the credential is already first-class where the **server** + decides: `allowedAccess()` in `src/nfs/util.ts` answers ACCESS from the + effective and supplementary groups both. diff --git a/docs/2.transports/4.nfs.md b/docs/2.transports/4.nfs.md index 7cd1671..6148fea 100644 --- a/docs/2.transports/4.nfs.md +++ b/docs/2.transports/4.nfs.md @@ -286,12 +286,14 @@ session.v4; // the Nfs4Session underneath, for tests | `verifier` | random | boot verifier for file handles, so restarts invalidate them | | `rtmax` / `wtmax` | — | largest `READ` answered / `WRITE` accepted | | `snapshotCache` | `64` | directory snapshots kept for readdir cookies | -| `claimOwnership` | `true` | `chown` new entries to the request's `AUTH_SYS` uid/gid | +| `claimOwnership` | `true` | give new entries to the request's `AUTH_SYS` caller | | `onError` | none | called for every request that ends in an error status | | `nfs4` | — | `Nfs4StateKnobs`: NFSv4.1-only knobs, ignored by the v3 session | `claimOwnership` solves the same problem the FUSE session solves the same way: the driver creates everything as the server process, while requests arrive from whoever mounted the share. It is quiet when the driver has no `lchown`, or when the server is not privileged enough to hand ownership away — a driver with no concept of ownership is not thereby broken. +The group is the one POSIX gives, not simply the caller's. A **set-group-ID parent directory hands its own group down**, and a new directory inherits the bit as well, so a shared directory stays shared all the way down — the rule Linux applies in `inode_init_owner()` for a local filesystem, applied here because nothing else in the path will. A new group-executable file loses `S_ISGID` when its creator is in neither the effective nor the supplementary groups the `AUTH_SYS` credential carried. Turning `claimOwnership` off leaves every new entry exactly as the driver made it, group and mode included. + `nfs4` carries the lease length (90 s default), the id-mapping hooks (`idmap: Nfs4IdMap`, see [Owner strings](#owner-strings)), and the session/slot/operation ceilings a real client negotiates against (`maxSessions`, `maxForeSlots`, `maxOperations`, and the rest) — everything a v3 mount never asks for. ## The macOS consent gate diff --git a/src/9p/session.ts b/src/9p/session.ts index 0f286a2..5c0cf6f 100644 --- a/src/9p/session.ts +++ b/src/9p/session.ts @@ -1469,11 +1469,20 @@ export class P9Session { /** * Give a newly created entry to whoever asked for it. * - * Character-for-character `NfsSession.#claim`'s stance, with the credentials - * gathered from the two places 9P keeps them: the uid from the `Tattach` this - * fid descends from (`p9_client_attach()` sends it once per user, and every - * walk inherits it), the gid from the create message itself, which is the one - * credential 9P repeats per request. + * `NfsSession.#claim`'s stance, with the credentials gathered from the two + * places 9P keeps them: the uid from the `Tattach` this fid descends from + * (`p9_client_attach()` sends it once per user, and every walk inherits it), + * the gid from the create message itself, which is the one credential 9P + * repeats per request. + * + * **Set-gid inheritance is deliberately absent here, and is not a gap.** The + * rule the NFS sessions apply (`../ownership.ts`) is applied by the *client* + * on this wire: `v9fs_get_fsgid_for_create()` hands `Tmkdir`/`Tcreate`/ + * `Tsymlink`/`Tmknod` the parent's gid when the parent is set-gid, and + * `v9fs_vfs_mkdir_dotl()` sets `S_ISGID` in the mode it sends. So the `gid` + * below already *is* the inherited group, and the mode `#mkdir` passes + * through already carries the bit — applying the rule a second time here + * would be the server disagreeing with the kernel that computed it. * * Best effort, deliberately. A driver with no `lchown` (`ENOSYS`), or one not * privileged enough to hand ownership away (`EPERM`/`ENOTSUP`), is not diff --git a/src/fuse/session.ts b/src/fuse/session.ts index a904ac2..84bbb72 100644 --- a/src/fuse/session.ts +++ b/src/fuse/session.ts @@ -867,10 +867,26 @@ export class FuseSession { * not privileged enough to hand ownership away (`EPERM`): a driver with no * concept of ownership is not thereby broken. * - * Two things this does **not** do, both of which want the credentials to - * reach the driver properly rather than more patching here: supplementary - * groups (only `gid` is on the wire before `FUSE_EXT_GROUPS`), and the - * set-gid directory rule that gives a new entry its parent's group. + * **Set-gid inheritance is missing here, and unlike on 9P that is a gap.** + * The rule — a new entry in a set-gid directory takes the parent's group, a + * new directory takes the bit too — is in `../ownership.ts`, and the two NFS + * sessions apply it. Nothing applies it on this wire: the kernel calls + * `inode_init_owner()` for local filesystems and leaves it to the server for + * a FUSE one. Two things stand between here and it, which is why it is not + * simply mirrored: + * + * - **A `stat` of the parent per create.** The NFS sessions read the parent + * anyway — for `wcc_data` on v3, for `change_info4` on v4.1 — so the rule + * is free there. Nothing here reads it: `#childOf` resolves a nodeid to a + * path without touching the driver, and the skip above means the common + * case currently costs *no* driver call at all. Applying the rule would + * make every create cost one, for every caller. + * - **The membership half cannot be answered.** Linux clears `S_ISGID` on a + * new group-executable file when the creator is not in the target group, + * and the only thing that puts that group on this wire is + * `FUSE_CREATE_SUPP_GROUP` — 7.38, riding in a `FUSE_EXT_GROUPS` extension + * block, and listed in `./init.ts` among the flags this server does not + * ask for because nothing consumed them. */ async #claim(path: string, header: FuseInHeader): Promise { const uid = process.getuid?.() ?? -1; diff --git a/src/nfs/util.ts b/src/nfs/util.ts index a604240..c91632b 100644 --- a/src/nfs/util.ts +++ b/src/nfs/util.ts @@ -152,14 +152,19 @@ export interface NfsSessionOptions { /** Directory snapshots kept for readdir cookies. Default `64`. */ snapshotCache?: number; /** - * `chown` a newly created entry to the `AUTH_SYS` uid/gid the request - * carried. Default `true`. + * `chown` a newly created entry to the `AUTH_SYS` uid the request carried, + * and to the group POSIX gives it. Default `true`. * * The same problem the FUSE session solves the same way: the driver creates * everything as the server process, while the requests arriving on it come * from whoever mounted the share. Quiet when the driver has no `lchown`, or * when the server is not privileged enough to hand ownership away — a driver * with no concept of ownership is not thereby broken. + * + * The group is not always the caller's: a set-gid parent directory hands its + * own down and a new directory takes the bit with it, which is `chmod`'s job + * and so is gated by this option too (`src/ownership.ts`). Turning it off + * leaves every new entry exactly as the driver made it. */ claimOwnership?: boolean; /** Called for every request that ends in an error status. */ diff --git a/src/nfs/v3/session.ts b/src/nfs/v3/session.ts index 14244a4..5df9c44 100644 --- a/src/nfs/v3/session.ts +++ b/src/nfs/v3/session.ts @@ -35,6 +35,7 @@ import { constants } from "node:fs"; import { fsError } from "../../errors.ts"; import { createLoopback, type Loopback } from "../../harness.ts"; import { PathLock } from "../../lock.ts"; +import { claimNewEntry, type NewEntry, newEntryOwnership } from "../../ownership.ts"; import { dirname, joinPath, normalizePath } from "../../path.ts"; import type { FileHandleLike, FsDriver, StatsLike, TimeLike } from "../../types.ts"; import { S_IFDIR, S_IFLNK, S_IFMT } from "../../types.ts"; @@ -637,8 +638,23 @@ export class Nfs3Session { /** The `before` half of a `wcc_data`, taken before the operation runs. */ async #preOp(path: string): Promise { + const stats = await this.#preOpStats(path); + return stats === undefined ? undefined : wccAttrOf(stats); + } + + /** + * The whole `lstat` behind {@link Nfs3Session.#preOp}, for the creating + * operations: they need the parent's mode and gid for set-gid inheritance + * (`#claim`) as well as the three fields a `wcc_attr` keeps, and this is the + * `lstat` they were already paying for. + * + * Swallows the failure for the same reason `#preOp` does — a `wcc_data` is a + * hint — and an unreadable parent then inherits nothing, which is what a + * parent with the bit clear does too. + */ + async #preOpStats(path: string): Promise { try { - return wccAttrOf(await this.#statOf(path)); + return await this.#statOf(path); } catch { return undefined; } @@ -1089,23 +1105,22 @@ export class Nfs3Session { * server process, and without this a file created by uid 1000 comes back * owned by the server and then fails every permission check its own creator * makes. Skipped when the caller *is* the server, and quiet when the driver - * cannot express ownership. + * cannot express ownership — both in `claimNewEntry`. + * + * The group is not simply the caller's: a set-gid parent hands its own down, + * and a new directory takes the bit with it (`../../ownership.ts` has the + * rule and where it comes from). That is why `parent` is threaded in from the + * `lstat` `#preOpStats` already did for the reply's `wcc_data` rather than + * stat'ed here — every creating operation in this file reads the parent + * anyway, so the rule costs no extra round trip. It does cost a `chmod` when + * a new directory has to take the bit, which is the one case that cannot be + * folded into the create. */ - async #claim(path: string, creds: RpcCredentials): Promise { - if (this.options.claimOwnership === false || creds.uid === undefined) { - return; - } - if (creds.uid === (process.getuid?.() ?? -1) && creds.gid === (process.getgid?.() ?? -1)) { + async #claim(path: string, creds: RpcCredentials, entry: NewEntry): Promise { + if (this.options.claimOwnership === false) { return; } - try { - await this.driver.lchown(path, creds.uid, creds.gid ?? -1); - } catch (error) { - const code = (error as { code?: string }).code; - if (code !== "ENOSYS" && code !== "EPERM" && code !== "ENOTSUP") { - throw error; - } - } + await claimNewEntry(this.driver, path, newEntryOwnership(creds, entry)); } async #create(args: XdrReader, creds: RpcCredentials, writer: XdrWriter): Promise { @@ -1116,21 +1131,22 @@ export class Nfs3Session { try { dir = this.#pathOf(request.where.dir); const path = joinPath(dir, this.#checkName(request.where.name, "open")); - before = await this.#preOp(dir); + const parent = await this.#preOpStats(dir); + before = parent === undefined ? undefined : wccAttrOf(parent); if (request.mode === CREATE_EXCLUSIVE) { - await this.#createExclusive(dir, path, request.verf, creds); + await this.#createExclusive(dir, path, request.verf, creds, parent); await this.#created(writer, dir, before, path); return; } - const mode = request.attributes?.mode; + const mode = (request.attributes?.mode ?? 0o666) & 0o7777; const flags = constants.O_WRONLY | constants.O_CREAT | (request.mode === CREATE_GUARDED ? constants.O_EXCL : 0); - const handle = await this.driver.open(path, flags, (mode ?? 0o666) & 0o7777); + const handle = await this.driver.open(path, flags, mode); await handle.close(); this.#invalidate(dir); - await this.#claim(path, creds); + await this.#claim(path, creds, { parent, directory: false, mode }); // `UNCHECKED` over an existing file still applies the attributes, which // is how a client asks for `open(…, O_CREAT|O_TRUNC)` in one round trip. await this.#applySattr(path, { ...request.attributes, mode: undefined }); @@ -1170,6 +1186,7 @@ export class Nfs3Session { path: string, verf: Uint8Array | undefined, creds: RpcCredentials, + parent: StatsLike | undefined, ): Promise { const verifier = verf ?? new Uint8Array(NFS3_CREATEVERFSIZE); let handle: FileHandleLike; @@ -1194,7 +1211,7 @@ export class Nfs3Session { await handle.close(); this.#exclusives.set(path, verifier); this.#invalidate(dir); - await this.#claim(path, creds); + await this.#claim(path, creds, { parent, directory: false, mode: 0o666 }); } async #mkdir(args: XdrReader, creds: RpcCredentials, writer: XdrWriter): Promise { @@ -1205,10 +1222,12 @@ export class Nfs3Session { try { dir = this.#pathOf(request.where.dir); const path = joinPath(dir, this.#checkName(request.where.name, "mkdir")); - before = await this.#preOp(dir); - await this.driver.mkdir(path, { mode: (request.attributes.mode ?? 0o777) & 0o7777 }); + const parent = await this.#preOpStats(dir); + before = parent === undefined ? undefined : wccAttrOf(parent); + const mode = (request.attributes.mode ?? 0o777) & 0o7777; + await this.driver.mkdir(path, { mode }); this.#invalidate(dir); - await this.#claim(path, creds); + await this.#claim(path, creds, { parent, directory: true, mode }); await this.#applySattr(path, { ...request.attributes, mode: undefined }); await this.#created(writer, dir, before, path); } catch (error) { @@ -1225,10 +1244,13 @@ export class Nfs3Session { try { dir = this.#pathOf(request.where.dir); const path = joinPath(dir, this.#checkName(request.where.name, "symlink")); - before = await this.#preOp(dir); + const parent = await this.#preOpStats(dir); + before = parent === undefined ? undefined : wccAttrOf(parent); await this.driver.symlink(request.target, path); this.#invalidate(dir); - await this.#claim(path, creds); + // A symlink's own mode is 0o777 and not the client's to choose, so the + // only half of the rule that can apply to one is the group. + await this.#claim(path, creds, { parent, directory: false, mode: 0o777 }); // A symlink has no mode of its own to set; times and ownership still do. await this.#applySattr( path, @@ -1259,7 +1281,8 @@ export class Nfs3Session { try { dir = this.#pathOf(request.where.dir); const path = joinPath(dir, this.#checkName(request.where.name, "mknod")); - before = await this.#preOp(dir); + const parent = await this.#preOpStats(dir); + before = parent === undefined ? undefined : wccAttrOf(parent); const mknod = this.driver.mountx?.mknod; if (mknod === undefined) { throw new NfsStatusError(NFS3ERR_NOTSUPP, "MKNOD needs the mountx.mknod extension"); @@ -1268,7 +1291,7 @@ export class Nfs3Session { const rdev = ((request.spec?.major ?? 0) << 8) | (request.spec?.minor ?? 0); await mknod.call(this.driver.mountx, path, mode | modeBitsOfFtype(request.type), rdev); this.#invalidate(dir); - await this.#claim(path, creds); + await this.#claim(path, creds, { parent, directory: false, mode }); await this.#created(writer, dir, before, path); } catch (error) { await this.#createFailed(writer, dir, before, error); diff --git a/src/nfs/v4/session.ts b/src/nfs/v4/session.ts index d853ac9..58a206b 100644 --- a/src/nfs/v4/session.ts +++ b/src/nfs/v4/session.ts @@ -71,6 +71,7 @@ import { constants } from "node:fs"; import { ERRNO_CODES, fsError } from "../../errors.ts"; import { createLoopback, type Loopback } from "../../harness.ts"; import { PathLock } from "../../lock.ts"; +import { claimNewEntry, type NewEntry, newEntryOwnership } from "../../ownership.ts"; import { dirname, joinPath } from "../../path.ts"; import type { FileHandleLike, FsDriver, StatsLike, TimeLike } from "../../types.ts"; import { S_IFDIR, S_IFLNK, S_IFMT, S_IFREG } from "../../types.ts"; @@ -2326,23 +2327,23 @@ export class Nfs4Session { * indicated in the RPC credentials of the call". Without it a file created by * uid 1000 comes back owned by the server and then fails every permission * check its own creator makes. Quiet when the driver cannot express - * ownership, which is not the same as broken. + * ownership, which is not the same as broken — that, and the skip when the + * caller *is* the server, are in `claimNewEntry`. + * + * The *group* is not derived from the principal alone: §18.4.3's "MUST + * derive" is about the owner, and the group a set-gid parent hands down is + * the parent's (`../../ownership.ts` has the rule and where it comes from). + * `parent` is therefore threaded in from the `stat` the caller already took — + * CREATE reads the parent to check it is a directory, OPEN to build its + * `change_info4` — so the rule adds no round trip of its own. A new directory + * taking the set-gid bit does cost a `chmod`, which is the one part of it + * that cannot be folded into the create. */ - async #claim(path: string, creds: RpcCredentials): Promise { - if (this.options.claimOwnership === false || creds.uid === undefined) { + async #claim(path: string, creds: RpcCredentials, entry: NewEntry): Promise { + if (this.options.claimOwnership === false) { return; } - if (creds.uid === (process.getuid?.() ?? -1) && creds.gid === (process.getgid?.() ?? -1)) { - return; - } - try { - await this.driver.lchown(path, creds.uid, creds.gid ?? -1); - } catch (error) { - const code = (error as { code?: string }).code; - if (code !== "ENOSYS" && code !== "EPERM" && code !== "ENOTSUP") { - throw error; - } - } + await claimNewEntry(this.driver, path, newEntryOwnership(creds, entry)); } /** @@ -2420,7 +2421,14 @@ export class Nfs4Session { } this.#invalidate(dir); - await this.#claim(path, cursor.creds); + // A symlink's mode is 0o777 and not the client's to choose — `createattrs` + // never reached `symlink` — so the mode named here is the one the object + // really has. + await this.#claim(path, cursor.creds, { + parent, + directory: type === NF4DIR, + mode: type === NF4LNK ? 0o777 : mode, + }); // `mkdir` already took the mode; a symlink has none to take. `size` is not // a writable attribute of any of these types, so it is neither applied nor // reported (§18.4.3, "any writable attribute valid for the object type"). @@ -3034,11 +3042,15 @@ export class Nfs4Session { let dir: string | undefined; let before = 0n; let path: string; + // Kept for `#openCreate`: a creating OPEN always names its file inside a + // directory (the CLAIM_FH arm above is refused for one), and set-gid + // inheritance is decided from the parent this already read. + let parent: StatsLike | undefined; if (claim === CLAIM_FH) { path = this.#pathOfCurrent(cursor); } else { dir = this.#pathOfCurrent(cursor); - const parent = await this.#statOf(dir); + parent = await this.#statOf(dir); if ((parent.mode & S_IFMT) !== S_IFDIR) { return refused(NFS4ERR_NOTDIR); } @@ -3048,7 +3060,7 @@ export class Nfs4Session { const attrset: number[] = []; if (create) { - const refusal = await this.#openCreate(args, path, cursor, attrset); + const refusal = await this.#openCreate(args, path, cursor, attrset, parent); if (refusal !== undefined) { return refused(refusal); } @@ -3139,10 +3151,11 @@ export class Nfs4Session { path: string, cursor: Cursor, attrset: number[], + parent: StatsLike | undefined, ): Promise { const how = args.openhow.how!; if (how.mode === EXCLUSIVE4 || how.mode === EXCLUSIVE4_1) { - return this.#openCreateExclusive(how, path, cursor, attrset); + return this.#openCreateExclusive(how, path, cursor, attrset, parent); } const attrs = how.createattrs!; this.#settableOrRefuse(attrs); @@ -3159,7 +3172,7 @@ export class Nfs4Session { } return undefined; } - await this.#claim(path, cursor.creds); + await this.#claim(path, cursor.creds, { parent, directory: false, mode }); // The mode went in as `open`'s third argument, so `#applyAttrs` must not // see it — and must see everything else, size included: a create names the // initial state of a file that did not exist a moment ago. @@ -3224,6 +3237,7 @@ export class Nfs4Session { path: string, cursor: Cursor, attrset: number[], + parent: StatsLike | undefined, ): Promise { const exclusive4 = how.mode === EXCLUSIVE4; const verifier = exclusive4 ? how.createverf! : how.createboth!.verf; @@ -3231,7 +3245,8 @@ export class Nfs4Session { if (attrs !== undefined) { this.#settableOrRefuse(attrs); } - const created = await this.#createFile(path, (attrs?.values.mode ?? 0o666) & 0o7777); + const mode = (attrs?.values.mode ?? 0o666) & 0o7777; + const created = await this.#createFile(path, mode); if (!created) { const remembered = this.#exclusives.match(path, verifier); if (remembered === undefined) { @@ -3242,7 +3257,7 @@ export class Nfs4Session { attrset.push(...remembered.attrset); return undefined; } - await this.#claim(path, cursor.creds); + await this.#claim(path, cursor.creds, { parent, directory: false, mode }); const bits: number[] = []; let status = NFS4_OK; if (attrs !== undefined) { diff --git a/src/ownership.ts b/src/ownership.ts new file mode 100644 index 0000000..93782f5 --- /dev/null +++ b/src/ownership.ts @@ -0,0 +1,152 @@ +/** + * Who a newly created entry belongs to, and the set-group-ID rule that decides + * it. + * + * A driver creates everything as the server process, so every session hands a + * fresh entry to its caller afterwards (`#claim`). "Its caller" is the easy + * half. The group is not: POSIX declines to pick one, `mkdir(2)` saying the new + * directory's group "shall be set to the group ID of the parent directory or to + * the effective group ID of the process" and `open(2)` saying the same of a new + * file. Linux picks between those two arms with the parent's set-group-ID bit, + * and `inode(7)` states the choice as the meaning of the bit on a directory: + * "newly created files in the directory inherit the group of the directory, and + * newly created subdirectories inherit the set-group-ID bit". This is a + * transcription of `inode_init_owner()` (`fs/inode.c`), which is where the + * kernel makes that decision on behalf of every local filesystem — and which + * nothing makes on behalf of a userspace one. + * + * The rule lives here rather than in a session because it is one rule and there + * are four sessions. Two use it, and the other two say at their own `#claim` + * why their wire settles this before or without them: 9P's client computes the + * group and the bit itself (`v9fs_get_fsgid_for_create()`), and FUSE's does + * not, but also does not carry the supplementary groups the file half of the + * rule needs. + * + * **Not a rename.** Moving an entry into a set-group-ID directory does not + * change its group on Linux, and `rename(2)` gives no licence to: the rule is + * about creation, and `inode_init_owner()` is only called where an inode is + * made. + */ + +import { S_ISGID, S_IXGRP, type StatsLike } from "./types.ts"; + +/** + * Who a request says it is, as far as its transport can say. + * + * `undefined` is "did not say" — `AUTH_NONE` over RPC — and stays `-1`, the + * `node:fs` `chown` convention for "leave this one alone". `gids` is the + * supplementary set; only `AUTH_SYS` carries one (`RpcCredentials`). + */ +export interface CallerCredentials { + uid?: number | undefined; + gid?: number | undefined; + gids?: readonly number[] | undefined; +} + +/** A new entry, described as the create that just made it described it. */ +export interface NewEntry { + /** + * The parent directory's attributes as they were when it was created, or + * `undefined` when they could not be read — in which case nothing is + * inherited, which is the same answer a parent with the bit clear gives. + */ + parent: StatsLike | undefined; + /** Directories inherit the set-group-ID bit itself. Nothing else does. */ + directory: boolean; + /** The permission bits the create used (`& 0o7777`). */ + mode: number; +} + +/** What the new entry's owner, group and mode should be. */ +export interface NewEntryOwnership { + /** For `lchown`; `-1` means "leave it alone". */ + uid: number; + /** For `lchown`; `-1` means "leave it alone". */ + gid: number; + /** + * The mode the entry must be changed to, or `undefined` when the mode the + * create used is already right — which is the overwhelmingly common answer, + * and the one that costs no extra driver call. + */ + mode: number | undefined; +} + +/** + * `inode_init_owner()`'s decision, for one new entry. + * + * Two departures from the kernel, both because the information is not on every + * wire: a caller with no supplementary groups behaves as a caller in no + * supplementary group (which errs towards *clearing* set-group-ID, the safe + * direction), and `capable_wrt_inode_uidgid(dir, CAP_FSETID)` is read as uid 0, + * since a userspace server has no other view of a remote caller's capabilities. + */ +export function newEntryOwnership(caller: CallerCredentials, entry: NewEntry): NewEntryOwnership { + const uid = caller.uid ?? -1; + const parent = entry.parent; + if (parent === undefined || (parent.mode & S_ISGID) === 0) { + // "} else inode->i_gid = current_fsgid();" + return { uid, gid: caller.gid ?? -1, mode: undefined }; + } + // "if (dir && dir->i_mode & S_ISGID) { inode->i_gid = dir->i_gid;" + const gid = parent.gid; + if (entry.directory) { + // "Directories are special, and always inherit S_ISGID." + return { uid, gid, mode: (entry.mode & S_ISGID) === 0 ? entry.mode | S_ISGID : undefined }; + } + // "else if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) && + // !in_group_p(inode->i_gid) && !capable_wrt_inode_uidgid(dir, CAP_FSETID)) + // mode &= ~S_ISGID;" — a set-group-ID *executable* is a way to run as a + // group, so one may not be created for a group the creator is not in. + const setgidExecutable = (entry.mode & (S_ISGID | S_IXGRP)) === (S_ISGID | S_IXGRP); + const member = caller.gid === gid || caller.gids?.includes(gid) === true; + if (setgidExecutable && !member && uid !== 0) { + return { uid, gid, mode: entry.mode & ~S_ISGID }; + } + return { uid, gid, mode: undefined }; +} + +/** The two driver calls giving an entry away can take. */ +interface OwnershipDriver { + lchown(path: string, uid: number, gid: number): Promise; + chmod(path: string, mode: number): Promise; +} + +/** + * Give `path` to `owner`, quietly. + * + * Deliberately skipped when the answer is the state the driver already + * produced — the caller *is* the server process and nothing was inherited — + * because that is the common case and worth a round trip. Deliberately quiet + * when the driver has no `lchown`/`chmod` (`ENOSYS`) or is not privileged + * enough to hand ownership away (`EPERM`/`ENOTSUP`): a driver with no concept + * of ownership is not thereby broken, and failing the create it just completed + * would be the wrong answer to that. + * + * The `chmod` runs **after** the `lchown`, not before: `chown(2)` clears + * set-group-ID on an executable when an unprivileged caller changes ownership, + * so a driver modelled on it would undo the bit in the other order. + */ +export async function claimNewEntry( + driver: OwnershipDriver, + path: string, + owner: NewEntryOwnership, +): Promise { + const mine = owner.uid === (process.getuid?.() ?? -1) && owner.gid === (process.getgid?.() ?? -1); + if (!mine && (owner.uid !== -1 || owner.gid !== -1)) { + await quietly(driver.lchown(path, owner.uid, owner.gid)); + } + if (owner.mode !== undefined) { + await quietly(driver.chmod(path, owner.mode)); + } +} + +async function quietly(call: Promise): Promise { + try { + await call; + } catch (error) { + const code = (error as { code?: string }).code; + if (code !== "ENOSYS" && code !== "EPERM" && code !== "ENOTSUP") { + throw error; + } + } +} diff --git a/src/types.ts b/src/types.ts index cce14b1..d9cacbc 100644 --- a/src/types.ts +++ b/src/types.ts @@ -238,3 +238,9 @@ export const S_IFBLK = 0o060000; export const S_IFCHR = 0o020000; export const S_IFIFO = 0o010000; export const S_IFSOCK = 0o140000; + +// --- the mode bits that are not permission bits (`inode(7)`) --- +/** Set-group-ID. On a directory it also means "children inherit my group". */ +export const S_ISGID = 0o2000; +/** Group execute — the bit that makes set-group-ID mean something on a file. */ +export const S_IXGRP = 0o0010; diff --git a/test/index.test.ts b/test/index.test.ts index 0126029..a5719c4 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -18,7 +18,8 @@ import { resolvePath, splitPath, } from "../src/index.ts"; -import type { FsDriver } from "../src/index.ts"; +import type { ErrnoCode, FsDriver, StatsLike } from "../src/index.ts"; +import { claimNewEntry, newEntryOwnership } from "../src/ownership.ts"; describe("every entry point runs under node's type stripping", () => { /** @@ -344,6 +345,119 @@ describe("errors", () => { }); }); +/** + * `src/ownership.ts` on its own: the decision, with no session around it. + * + * The NFS session suites drive the same rule end to end; this is the table of + * cases `inode_init_owner()` distinguishes, including the two a wire is + * unlikely to produce (an unreadable parent, a caller who did not say who it + * is) and the one that needs a privileged caller. + */ +describe("set-gid inheritance", () => { + /** Only the two fields the rule reads. */ + const dir = (mode: number, gid: number): StatsLike => ({ mode, gid }) as StatsLike; + const caller = { uid: 1000, gid: 1000, gids: [] as number[] }; + + it("takes the caller's group when the parent has no set-gid bit", () => { + expect( + newEntryOwnership(caller, { parent: dir(0o40755, 4000), directory: false, mode: 0o644 }), + ).toEqual({ uid: 1000, gid: 1000, mode: undefined }); + }); + + it("inherits nothing from a parent that could not be read", () => { + expect(newEntryOwnership(caller, { parent: undefined, directory: true, mode: 0o755 })).toEqual({ + uid: 1000, + gid: 1000, + mode: undefined, + }); + }); + + it("leaves both alone for a caller that did not say who it is", () => { + // AUTH_NONE: `-1` is `chown`'s "leave this one alone", and there is still a + // group to inherit if the parent has the bit. + expect(newEntryOwnership({}, { parent: undefined, directory: false, mode: 0o644 })).toEqual({ + uid: -1, + gid: -1, + mode: undefined, + }); + expect( + newEntryOwnership({}, { parent: dir(0o42775, 4000), directory: false, mode: 0o644 }), + ).toEqual({ uid: -1, gid: 4000, mode: undefined }); + }); + + it("gives a new directory the bit, and does not ask for a chmod it already has", () => { + expect( + newEntryOwnership(caller, { parent: dir(0o42775, 4000), directory: true, mode: 0o755 }), + ).toEqual({ uid: 1000, gid: 4000, mode: 0o2755 }); + expect( + newEntryOwnership(caller, { parent: dir(0o42775, 4000), directory: true, mode: 0o2755 }), + ).toEqual({ uid: 1000, gid: 4000, mode: undefined }); + }); + + it("clears set-gid on an executable for a group the caller is not in", () => { + const entry = { parent: dir(0o42775, 4000), directory: false, mode: 0o2775 }; + expect(newEntryOwnership(caller, entry).mode).toBe(0o775); + // Membership counts whether it is the effective group or a supplementary + // one, and a privileged caller keeps the bit either way + // (`capable_wrt_inode_uidgid(dir, CAP_FSETID)`). + expect(newEntryOwnership({ uid: 1000, gid: 4000 }, entry).mode).toBeUndefined(); + expect(newEntryOwnership({ ...caller, gids: [4000] }, entry).mode).toBeUndefined(); + expect(newEntryOwnership({ ...caller, uid: 0 }, entry).mode).toBeUndefined(); + // Not group-executable: nothing to run as, so nothing to clear. + expect(newEntryOwnership(caller, { ...entry, mode: 0o2664 }).mode).toBeUndefined(); + }); + + describe("applying it", () => { + /** A driver that records its calls, and can be told to refuse them. */ + function recorder(refusal?: string) { + const calls: string[] = []; + const answer = async (what: string): Promise => { + calls.push(what); + if (refusal !== undefined) throw fsError(refusal as ErrnoCode); + }; + return { + calls, + lchown: (path: string, uid: number, gid: number) => answer(`lchown ${path} ${uid}:${gid}`), + chmod: (path: string, mode: number) => answer(`chmod ${path} ${mode.toString(8)}`), + }; + } + + it("chowns, then chmods — never the other way round", async () => { + const driver = recorder(); + await claimNewEntry(driver, "/f", { uid: 1000, gid: 4000, mode: 0o2755 }); + // `chown(2)` clears set-group-ID on an executable when an unprivileged + // caller changes ownership, so the bit has to go on afterwards. + expect(driver.calls).toEqual(["lchown /f 1000:4000", "chmod /f 2755"]); + }); + + it("does nothing when the driver already produced the answer", async () => { + const driver = recorder(); + await claimNewEntry(driver, "/f", { + uid: process.getuid?.() ?? -1, + gid: process.getgid?.() ?? -1, + mode: undefined, + }); + await claimNewEntry(driver, "/f", { uid: -1, gid: -1, mode: undefined }); + expect(driver.calls).toEqual([]); + }); + + it("is quiet for a driver with no concept of ownership, and only for that", async () => { + for (const code of ["ENOSYS", "EPERM", "ENOTSUP"]) { + const driver = recorder(code); + await claimNewEntry(driver, "/f", { uid: 1000, gid: 4000, mode: 0o2755 }); + // Quiet, and the refused `lchown` does not stop the `chmod` from being + // tried: they are two different things the driver may or may not have. + expect(driver.calls).toEqual(["lchown /f 1000:4000", "chmod /f 2755"]); + } + // Anything else is a real failure of the create that just happened. + const broken = recorder("EIO"); + await expect( + claimNewEntry(broken, "/f", { uid: 1000, gid: 4000, mode: undefined }), + ).rejects.toMatchObject({ code: "EIO" }); + }); + }); +}); + describe("harness", () => { const minimal: FsDriver = { stat: () => Promise.reject(new Error("unused")), diff --git a/test/nfs/v3/session.test.ts b/test/nfs/v3/session.test.ts index 0f9ffe3..3aa7e25 100644 --- a/test/nfs/v3/session.test.ts +++ b/test/nfs/v3/session.test.ts @@ -57,10 +57,17 @@ import { RPC_PROG_UNAVAIL, } from "../../../src/nfs/v3/constants.ts"; import { NFS_V4 } from "../../../src/nfs/v4/constants.ts"; -import { decodeReply, encodeCall, frameFragments } from "../../../src/nfs/rpc.ts"; +import { + AUTH_SYS, + decodeReply, + encodeAuthSys, + encodeCall, + frameFragments, + type OpaqueAuth, +} from "../../../src/nfs/rpc.ts"; import { encodeXdr } from "../../../src/nfs/xdr.ts"; import { createNfsServer, type NfsServer } from "../../../src/nfs/server.ts"; -import type { FsDriver } from "../../../src/types.ts"; +import type { FsDriver, FullFsDriver } from "../../../src/types.ts"; import { withoutExtensions } from "../../no-extensions.ts"; import { check, NfsClient, nfsDriver } from "./client.ts"; import { createLoopback, type Loopback } from "../../../src/harness.ts"; @@ -86,10 +93,13 @@ afterEach(async () => { } }); -async function serve(driver: FsDriver = createMemoryDriver()): Promise { - const server = createNfsServer(driver); +async function serve( + driver: FsDriver = createMemoryDriver(), + options: { cred?: OpaqueAuth; claimOwnership?: boolean } = {}, +): Promise { + const server = createNfsServer(driver, { claimOwnership: options.claimOwnership }); await server.listen(); - const client = await NfsClient.connect({ port: server.port }); + const client = await NfsClient.connect({ port: server.port, cred: options.cred }); const mounted = await client.mnt("/"); expect(mounted.status).toBe(MNT3_OK); const harness: Harness = { @@ -997,6 +1007,148 @@ describe("individual procedures", () => { }); }); +/** + * The rule in `src/ownership.ts`, driven through CREATE/MKDIR/SYMLINK/MKNOD. + * + * Everything here is about the *group*, which the caller's `AUTH_SYS` + * credential does not settle: a set-gid parent hands its own group down, and a + * new directory takes the bit with it. The tree is built through the driver + * rather than through the mount, so that the setup is not itself subject to the + * rule under test. + */ +describe("set-gid inheritance", () => { + const TEAM = 4000; + /** A caller in no group the fixture uses, so every group below is inherited. */ + const OUTSIDER = credential(4242, 4343); + /** The same caller, with the team in its supplementary list (`AUTH_SYS` gids). */ + const MEMBER = credential(4242, 4343, [7, TEAM]); + + function credential(uid: number, gid: number, gids: number[] = []): OpaqueAuth { + // `authSys()` always sends an empty `gids`, and the supplementary list is + // exactly what one half of the rule turns on. + return { + flavor: AUTH_SYS, + body: encodeAuthSys({ stamp: 0, machineName: "test", uid, gid, gids }), + }; + } + + /** `/team` is set-gid and owned by group 4000; `/plain` is an ordinary one. */ + async function fixture(): Promise { + const driver = createMemoryDriver(); + await driver.mkdir("/team"); + await driver.chown("/team", 500, TEAM); + await driver.chmod("/team", 0o2775); + await driver.mkdir("/plain"); + return driver; + } + + async function served(options: { cred?: OpaqueAuth; claimOwnership?: boolean } = {}): Promise<{ + client: NfsClient; + team: Uint8Array; + plain: Uint8Array; + }> { + const { client, root } = await serve(await fixture(), { cred: OUTSIDER, ...options }); + return { + client, + team: check(await client.lookup(root, "team"), "lookup").object!, + plain: check(await client.lookup(root, "plain"), "lookup").object!, + }; + } + + it("gives a new file the parent's group and the caller the ownership", async () => { + const { client, team } = await served(); + const created = check(await client.create(team, "f", CREATE_UNCHECKED), "create"); + expect(created.objAttributes!.uid).toBe(4242); + expect(created.objAttributes!.gid).toBe(TEAM); + // EXCLUSIVE carries no `sattr3` at all, and inherits just the same. + const exclusive = check( + await client.create(team, "x", CREATE_EXCLUSIVE, {}, VERIFIER), + "create", + ); + expect(exclusive.objAttributes!.gid).toBe(TEAM); + }); + + it("gives a new directory the group *and* the set-gid bit", async () => { + const { client, team } = await served(); + const made = check(await client.mkdir(team, "sub", { mode: 0o755 }), "mkdir"); + expect(made.objAttributes!.gid).toBe(TEAM); + // `inode(7)`: "newly created subdirectories inherit the set-group-ID bit", + // which is what makes the rule apply to the whole tree rather than one + // level of it. + expect(made.objAttributes!.mode).toBe(0o2755); + // And it really is inherited *again* one level down. + const sub = made.obj!; + expect(check(await client.mkdir(sub, "deeper"), "mkdir").objAttributes!.mode & 0o2000).toBe( + 0o2000, + ); + expect( + check(await client.create(sub, "f", CREATE_UNCHECKED), "create").objAttributes!.gid, + ).toBe(TEAM); + }); + + it("inherits through SYMLINK and MKNOD too", async () => { + const { client, team } = await served(); + const link = check(await client.symlink(team, "l", "./f"), "symlink"); + expect(link.objAttributes!.gid).toBe(TEAM); + // A symlink's own mode is 0o777 and stays it: nothing was chmod'ed through + // the link. + expect(link.objAttributes!.mode).toBe(0o777); + const fifo = check(await client.mknod(team, "fifo", NF3FIFO, { mode: 0o644 }), "mknod"); + expect(fifo.objAttributes!.gid).toBe(TEAM); + }); + + it("gives the caller's own group in an ordinary directory", async () => { + const { client, plain } = await served(); + const created = check(await client.create(plain, "f", CREATE_UNCHECKED), "create"); + expect(created.objAttributes!.uid).toBe(4242); + expect(created.objAttributes!.gid).toBe(4343); + const made = check(await client.mkdir(plain, "sub", { mode: 0o755 }), "mkdir"); + expect(made.objAttributes!.gid).toBe(4343); + // No parent bit, nothing to inherit: the mode is the one that was asked for. + expect(made.objAttributes!.mode).toBe(0o755); + }); + + it("clears set-gid on a new executable the creator has no claim to that group", async () => { + const { client, team } = await served(); + const created = check( + await client.create(team, "run", CREATE_UNCHECKED, { mode: 0o2775 }), + "create", + ); + // A set-gid executable is a way to *run as* a group, so one may not be made + // for a group its creator is not in — `inode_init_owner()`, and the reason + // the supplementary list is on the wire at all. + expect(created.objAttributes!.mode).toBe(0o775); + expect(created.objAttributes!.gid).toBe(TEAM); + }); + + it("keeps it when only the supplementary list makes the creator a member", async () => { + const { client, team } = await served({ cred: MEMBER }); + const created = check( + await client.create(team, "run", CREATE_UNCHECKED, { mode: 0o2775 }), + "create", + ); + expect(created.objAttributes!.mode).toBe(0o2775); + // Not group-executable, so there is nothing to run as and nothing to clear. + const plainMode = check( + await client.create(team, "data", CREATE_UNCHECKED, { mode: 0o2664 }), + "create", + ); + expect(plainMode.objAttributes!.mode).toBe(0o2664); + }); + + it("does not claim at all with claimOwnership off", async () => { + const { client, team } = await served({ claimOwnership: false }); + const created = check(await client.create(team, "f", CREATE_UNCHECKED), "create"); + // Whatever the driver did on its own: the server's own uid and gid, and no + // set-gid bit added to a new directory either. + expect(created.objAttributes!.uid).toBe(process.getuid?.() ?? 0); + expect(created.objAttributes!.gid).toBe(process.getgid?.() ?? 0); + const made = check(await client.mkdir(team, "sub", { mode: 0o755 }), "mkdir"); + expect(made.objAttributes!.mode).toBe(0o755); + expect(made.objAttributes!.gid).toBe(process.getgid?.() ?? 0); + }); +}); + describe("the socket", () => { it("reassembles a call split across RPC fragments", async () => { const { client, server } = await serve(); diff --git a/test/nfs/v4/session.test.ts b/test/nfs/v4/session.test.ts index 54a04e4..4225afd 100644 --- a/test/nfs/v4/session.test.ts +++ b/test/nfs/v4/session.test.ts @@ -26,10 +26,13 @@ import { afterEach, describe, expect, it } from "vitest"; import { createMemoryDriver } from "../../../src/drivers/memory.ts"; import { fsError } from "../../../src/errors.ts"; import { + AUTH_SYS, authSys, decodeReply, + encodeAuthSys, encodeCall, MSG_ACCEPTED, + type OpaqueAuth, RPC_PROC_UNAVAIL, RPC_SUCCESS, } from "../../../src/nfs/rpc.ts"; @@ -268,7 +271,13 @@ let nextXid = 1; */ function compoundCall( ops: Op[], - options: { tag?: string; minorversion?: number; xid?: number; count?: number } = {}, + options: { + tag?: string; + minorversion?: number; + xid?: number; + count?: number; + cred?: OpaqueAuth; + } = {}, ): { xid: number; bytes: Uint8Array } { const writer = new XdrWriter(512); writer.string(options.tag ?? ""); @@ -293,7 +302,7 @@ function compoundCall( program: NFS4_PROGRAM, version: NFS_V4, procedure: NFSPROC4_COMPOUND, - cred: authSys(1000, 1000), + cred: options.cred ?? authSys(1000, 1000), args: writer.bytes(), }), }; @@ -408,10 +417,18 @@ class Client { slotSeqid = 0; clientid = 0n; - constructor(readonly session: Nfs4Session) {} - - static async open(session: Nfs4Session, ownerid = "test-client"): Promise { - const client = new Client(session); + constructor( + readonly session: Nfs4Session, + /** The credential every compound this client sends carries. */ + readonly cred?: OpaqueAuth, + ) {} + + static async open( + session: Nfs4Session, + ownerid = "test-client", + cred?: OpaqueAuth, + ): Promise { + const client = new Client(session, cred); const exchange = await client.send([ { op: OP_EXCHANGE_ID, @@ -452,7 +469,7 @@ class Client { /** A raw compound, with no SEQUENCE prepended. */ async send(ops: Op[], options: Parameters[1] = {}): Promise { - const { bytes } = compoundCall(ops, options); + const { bytes } = compoundCall(ops, { cred: this.cred, ...options }); const reply = await this.session.handleCall(bytes); expect(reply).not.toBeNull(); return readReply(reply!); @@ -1965,8 +1982,12 @@ describe("the version router", () => { * server answers `NFS4ERR_GRACE` until it does — which is a case of its own * below. */ -async function ready(session: Nfs4Session, ownerid = "test-client"): Promise { - const client = await Client.open(session, ownerid); +async function ready( + session: Nfs4Session, + ownerid = "test-client", + cred?: OpaqueAuth, +): Promise { + const client = await Client.open(session, ownerid, cred); const done = await client.run([{ op: OP_RECLAIM_COMPLETE, args: { oneFs: false } }]); expect(done.compound.status).toBe(NFS4_OK); return client; @@ -4086,3 +4107,206 @@ describe("the v4 client over a real socket", () => { expect((await first.renew()).status).toBe(10_052); // NFS4ERR_BADSESSION }); }); + +/** + * The rule in `src/ownership.ts`, driven through CREATE and a creating OPEN. + * + * §18.4.3 has the server "derive the owner ... from the principal indicated in + * the RPC credentials of the call", and says nothing about the group, because + * the group is not the principal's to give: a set-gid parent hands its own + * down, and a new directory takes the bit with it. The fixture is built through + * the driver rather than through the server, so the setup is not itself subject + * to the rule under test. + */ +describe("set-gid inheritance", () => { + const TEAM = 4000; + /** A caller in no group the fixture uses, so every group below is inherited. */ + const OUTSIDER = credential(4242, 4343); + /** The same caller, with the team in its supplementary list (`AUTH_SYS` gids). */ + const MEMBER = credential(4242, 4343, [7, TEAM]); + + function credential(uid: number, gid: number, gids: number[] = []): OpaqueAuth { + // `authSys()` always sends an empty `gids`, and the supplementary list is + // exactly what one half of the rule turns on. + return { + flavor: AUTH_SYS, + body: encodeAuthSys({ stamp: 0, machineName: "test", uid, gid, gids }), + }; + } + + /** `/team` is set-gid and owned by group 4000; `/plain` is an ordinary one. */ + async function fixture(): Promise { + const driver = createMemoryDriver(); + await driver.mkdir("/team"); + await driver.chown("/team", 500, TEAM); + await driver.chmod("/team", 0o2775); + await driver.mkdir("/plain"); + return driver; + } + + async function serving( + options: { cred?: OpaqueAuth; claimOwnership?: boolean } = {}, + ): Promise<{ session: Nfs4Session; client: Client }> { + const session = new Nfs4Session(await fixture(), { + claimOwnership: options.claimOwnership, + }); + return { session, client: await ready(session, "test-client", options.cred ?? OUTSIDER) }; + } + + const TO_TEAM: Op[] = [{ op: OP_PUTROOTFH }, { op: OP_LOOKUP, args: { objname: "team" } }]; + const TO_PLAIN: Op[] = [{ op: OP_PUTROOTFH }, { op: OP_LOOKUP, args: { objname: "plain" } }]; + + it("gives a file created by OPEN the parent's group and the caller the ownership", async () => { + const { session, client } = await serving(); + const reply = await client.run([ + ...TO_TEAM, + OPEN({ + name: "f", + access: OPEN4_SHARE_ACCESS_BOTH, + openhow: createHow(UNCHECKED4, { bits: [FATTR4_MODE], values: { mode: 0o644 } }), + }), + GETATTR([FATTR4_OWNER, FATTR4_OWNER_GROUP]), + ]); + expect(reply.compound.status).toBe(NFS4_OK); + // §5.9's numeric form, since nothing is idmapped here. + expect(attrsFor(reply)).toMatchObject({ owner: "4242", ownerGroup: String(TEAM) }); + expect(await session.driver.lstat("/team/f")).toMatchObject({ uid: 4242, gid: TEAM }); + }); + + it("inherits on the exclusive create paths as well", async () => { + const { session, client } = await serving(); + for (const [mode, name] of [ + [EXCLUSIVE4, "one"], + [EXCLUSIVE4_1, "two"], + ] as const) { + const reply = await client.run([ + ...TO_TEAM, + OPEN({ + name, + access: OPEN4_SHARE_ACCESS_BOTH, + openhow: exclusiveHow(mode, VERIFIER), + }), + ]); + expect(reply.compound.status).toBe(NFS4_OK); + expect((await session.driver.lstat(`/team/${name}`)).gid).toBe(TEAM); + } + }); + + it("gives a new directory the group *and* the set-gid bit", async () => { + const { session, client } = await serving(); + const reply = await client.run([ + ...TO_TEAM, + { + op: OP_CREATE, + args: { + objtype: { type: NF4DIR }, + objname: "sub", + createattrs: { + attrmask: bitmapOf([FATTR4_MODE]), + values: { mode: 0o750 }, + unsupported: [], + }, + }, + }, + GETATTR([FATTR4_MODE, FATTR4_OWNER_GROUP]), + ]); + expect(reply.compound.status).toBe(NFS4_OK); + // `inode(7)`: "newly created subdirectories inherit the set-group-ID bit", + // which is what makes the rule apply to a whole tree rather than one level. + expect(attrsFor(reply)).toMatchObject({ mode: 0o2750, ownerGroup: String(TEAM) }); + // A symlink takes the group and keeps its own 0o777 — nothing was chmod'ed + // through the link. + const link = await client.run([ + ...TO_TEAM, + { + op: OP_CREATE, + args: { + objtype: { type: NF4LNK, linkdata: "./f" }, + objname: "l", + createattrs: { attrmask: [], values: {}, unsupported: [] }, + }, + }, + ]); + expect(link.compound.status).toBe(NFS4_OK); + expect(await session.driver.lstat("/team/l")).toMatchObject({ gid: TEAM, mode: 0o120777 }); + }); + + it("gives the caller's own group in an ordinary directory", async () => { + const { session, client } = await serving(); + const reply = await client.run([ + ...TO_PLAIN, + { + op: OP_CREATE, + args: { + objtype: { type: NF4DIR }, + objname: "sub", + createattrs: { + attrmask: bitmapOf([FATTR4_MODE]), + values: { mode: 0o750 }, + unsupported: [], + }, + }, + }, + GETATTR([FATTR4_MODE, FATTR4_OWNER_GROUP]), + ]); + expect(reply.compound.status).toBe(NFS4_OK); + // No parent bit, nothing to inherit: the mode is the one that was asked for. + expect(attrsFor(reply)).toMatchObject({ mode: 0o750, ownerGroup: "4343" }); + expect((await session.driver.lstat("/plain/sub")).gid).toBe(4343); + }); + + it("clears set-gid on a new executable the creator has no claim to that group", async () => { + const outsider = await serving(); + const reply = await outsider.client.run([ + ...TO_TEAM, + OPEN({ + name: "run", + access: OPEN4_SHARE_ACCESS_BOTH, + openhow: createHow(UNCHECKED4, { bits: [FATTR4_MODE], values: { mode: 0o2775 } }), + }), + GETATTR([FATTR4_MODE]), + ]); + expect(reply.compound.status).toBe(NFS4_OK); + // A set-gid executable is a way to *run as* a group, so one may not be made + // for a group its creator is not in — `inode_init_owner()`, and the reason + // the supplementary list is on the wire at all. + expect(attrsFor(reply).mode).toBe(0o775); + + const member = await serving({ cred: MEMBER }); + const kept = await member.client.run([ + ...TO_TEAM, + OPEN({ + name: "run", + access: OPEN4_SHARE_ACCESS_BOTH, + openhow: createHow(UNCHECKED4, { bits: [FATTR4_MODE], values: { mode: 0o2775 } }), + }), + GETATTR([FATTR4_MODE]), + ]); + expect(kept.compound.status).toBe(NFS4_OK); + expect(attrsFor(kept).mode).toBe(0o2775); + }); + + it("does not claim at all with claimOwnership off", async () => { + const { session, client } = await serving({ claimOwnership: false }); + const reply = await client.run([ + ...TO_TEAM, + { + op: OP_CREATE, + args: { + objtype: { type: NF4DIR }, + objname: "sub", + createattrs: { + attrmask: bitmapOf([FATTR4_MODE]), + values: { mode: 0o750 }, + unsupported: [], + }, + }, + }, + GETATTR([FATTR4_MODE]), + ]); + expect(reply.compound.status).toBe(NFS4_OK); + // Whatever the driver did on its own: no bit added, no group inherited. + expect(attrsFor(reply).mode).toBe(0o750); + expect((await session.driver.lstat("/team/sub")).gid).toBe(process.getgid?.() ?? 0); + }); +}); From 2f74fd0effba65625c9cdd89e541cffe43b0a7b4 Mon Sep 17 00:00:00 2001 From: Pooya Parsa Date: Fri, 31 Jul 2026 19:13:35 +0000 Subject: [PATCH 4/7] chore: record the three NFS items in the roadmap and the map MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The handle-table bound and exclusive create shipped, so their entries go — replaced by what is actually left of each: a default for `maxHandles` (which wants v4.1 open state pinned first, since that state is keyed by entry id) and `suppattr_exclcreat`, whose "the OPEN step's call" note predates the OPEN step making it. Co-Authored-By: Claude Opus 5 --- .agents/architecture.md | 34 +++++++++++++++++----------------- .agents/roadmap.md | 28 +++++++++++++++------------- 2 files changed, 32 insertions(+), 30 deletions(-) diff --git a/.agents/architecture.md b/.agents/architecture.md index ec39763..30c2f94 100644 --- a/.agents/architecture.md +++ b/.agents/architecture.md @@ -84,23 +84,23 @@ Linux only. No `server.ts` — it owns `/dev/fuse` directly. Two versions behind one router and one server. Linux and macOS — and on macOS **without root**, since `mount_nfs` is not setuid. -| File | What | -| ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `xdr.ts` | XDR (RFC 4506) — bounds-checked, big-endian, 64-bit as `bigint` | -| `rpc.ts` | ONC RPC v2 (RFC 5531) + TCP record marking + `AUTH_NONE`/`AUTH_SYS`. Version-neutral, so it lives here rather than in `v3/` | -| `handles.ts` | `FileHandleTable` (ids off a monotonic counter, so a dropped entry still answers `ESTALE`) and `DirectorySnapshots` | -| `util.ts` | the seam between `v3/` and `v4/` — version-neutral POSIX logic, `NfsSessionOptions`, `Nfs4IdMap`, and the `NfsSharedState` the router builds once | -| `session.ts` | `NfsSession` — peeks `prog`/`vers` and hands the **same raw bytes** on. Everything public is the versioned session's, reached through here | -| `server.ts` | one connection per socket, window of 64 (it counter-offers `ca_maxrequests` up to 64) | -| `mount.ts` | the only platform-aware transport: Linux and macOS, `version: "3"` or `"4.1"`, plus the macOS consent gate | -| `probe.ts` | root for Linux only; `v4` is reported orthogonally to `usable` | -| `v3/constants.ts`, `v3/protocol.ts` | RFC 1813, plus the MOUNT program — v3-only | -| `v3/session.ts` | answers both MOUNT and NFS programs | -| `v4/constants.ts` | RFC 8881 with RFC 5662 for the XDR it states only in prose. Complete tables even where the server answers `NFS4ERR_NOTSUPP`; no v4.2 | -| `v4/attr.ts` | the `bitmap4`/`fattr4` codec — an unsupported bit poisons every later offset, which is why `v4/session.ts` diffs the mask separately | -| `v4/protocol.ts` | COMPOUND framing and every operation. Optional ops this server will never implement are absent by design | -| `v4/state.ts` | client ids, sessions, slot replay caches, stateids, share reservations, locks, the lease clock. Pure and synchronous — **no grace period**, courteous revocation | -| `v4/session.ts` | COMPOUND dispatch over `state.ts`'s decisions; owns open-state → `FileHandleLike` and the current-stateid cursor | +| File | What | +| ----------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `xdr.ts` | XDR (RFC 4506) — bounds-checked, big-endian, 64-bit as `bigint` | +| `rpc.ts` | ONC RPC v2 (RFC 5531) + TCP record marking + `AUTH_NONE`/`AUTH_SYS`. Version-neutral, so it lives here rather than in `v3/` | +| `handles.ts` | `FileHandleTable` (ids off a monotonic counter, so a dropped entry still answers `ESTALE`; `maxHandles` evicts LRU past a cap, root exempt) and `DirectorySnapshots` | +| `util.ts` | the seam between `v3/` and `v4/` — version-neutral POSIX logic, `NfsSessionOptions`, `Nfs4IdMap`, `ExclusiveCreates` (the create verifiers a retransmission is recognised by), and the `NfsSharedState` the router builds once | +| `session.ts` | `NfsSession` — peeks `prog`/`vers` and hands the **same raw bytes** on. Everything public is the versioned session's, reached through here | +| `server.ts` | one connection per socket, window of 64 (it counter-offers `ca_maxrequests` up to 64) | +| `mount.ts` | the only platform-aware transport: Linux and macOS, `version: "3"` or `"4.1"`, plus the macOS consent gate | +| `probe.ts` | root for Linux only; `v4` is reported orthogonally to `usable` | +| `v3/constants.ts`, `v3/protocol.ts` | RFC 1813, plus the MOUNT program — v3-only | +| `v3/session.ts` | answers both MOUNT and NFS programs | +| `v4/constants.ts` | RFC 8881 with RFC 5662 for the XDR it states only in prose. Complete tables even where the server answers `NFS4ERR_NOTSUPP`; no v4.2 | +| `v4/attr.ts` | the `bitmap4`/`fattr4` codec — an unsupported bit poisons every later offset, which is why `v4/session.ts` diffs the mask separately | +| `v4/protocol.ts` | COMPOUND framing and every operation. Optional ops this server will never implement are absent by design | +| `v4/state.ts` | client ids, sessions, slot replay caches, stateids, share reservations, locks, the lease clock. Pure and synchronous — **no grace period**, courteous revocation | +| `v4/session.ts` | COMPOUND dispatch over `state.ts`'s decisions; owns open-state → `FileHandleLike` and the current-stateid cursor | Not on `mountx/nfs` yet: `v4/protocol.ts` and `v4/constants.ts`, reachable only through `NfsSession.v4`. diff --git a/.agents/roadmap.md b/.agents/roadmap.md index 72cd974..c83d3c0 100644 --- a/.agents/roadmap.md +++ b/.agents/roadmap.md @@ -100,8 +100,11 @@ rather than by accident. - **xattr and the rest of the `mountx.*` extension namespace** (byte-range locks, `fallocate`/`lseek`, cache-invalidation `notify`) — `ENOTSUP` until a real user needs one. -- **NFS `CREATE` with `EXCLUSIVE`** — currently `NFS3ERR_NOTSUPP`; the verifier - would have to be stored somewhere that survives a retry. +- **`suppattr_exclcreat` (75) is still unadvertised** in `src/nfs/v4/attr.ts`'s + `SUPPORTED_ATTRS`. Exclusive create shipped, and its verifier lives beside the + file rather than in an attribute, so the honest value is now the full settable + set — the attribute's "deliberately absent, it is the OPEN step's call" note + predates the OPEN step having made it. - **FUSE over io_uring** (Linux 6.14+) — would replace the read/reply loop with a shared ring and obsolete the threadpool discussion entirely; the transport layer should stay swappable so this is one file, not a rewrite. @@ -119,17 +122,16 @@ rather than by accident. mounting one rather than serving one; the natural follow-up to the S3 gateway transport. It would reuse `test/s3/client.ts`'s signing client instead of writing a second one. Not started. -- **Handle-table growth bound.** Neither NFS version has a `FORGET`, so nothing - tells the server a client is done with a handle. Half of this is closed: - `FileHandleTable.#detachPath` drops an entry once its last path is detached, - so create/delete churn under a mount no longer grows the table without bound. - What is left is genuinely bounded: **one entry per path a client currently has - a name for**, held for the life of the server however long ago the client - looked, plus the root entry, which is exempt from eviction because - `PUTROOTFH`/`PUTPUBFH`/`MNT` encode it from a field rather than from a lookup. - A generation-stamped LRU is the fix if a workload needs a bound on _that_ — it - is the only remaining growth, and the one an LRU can serve without breaking a - live client. +- **A default for `maxHandles`.** The bound itself shipped — the handle table + evicts least-recently-used past the cap, root exempt — but the cap is opt-in, + so out of the box the table is still one entry per path a client currently has + a name for, held for the life of the server. It is opt-in because an eviction + costs an NFSv4.1 client more than a re-`LOOKUP`: `src/nfs/v4/session.ts` keys + open and lock state by entry id, so evicting a handle a client has open costs + it an `OPEN` as well. Pinning entries with live v4 state — and only then + picking a default — is what is left. A directly constructed `Nfs3Session` / + `Nfs4Session` also builds its own table and ignores the option; every public + path goes through the router, so this is the tests and the CLI only. - **Set-gid inheritance on the FUSE session.** The rule itself now exists — `src/ownership.ts`, a transcription of `inode_init_owner()` — and both NFS sessions apply it: a new entry in a set-gid directory takes the parent's From b100c5d219f7016eede913f8dc63541b3ff6434c Mon Sep 17 00:00:00 2001 From: Pooya Parsa Date: Fri, 31 Jul 2026 19:52:54 +0000 Subject: [PATCH 5/7] fix(nfs): review findings on the three NFS changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A review of the branch reproduced four defects and two wrong doc claims. **The exclusive-create verifier was recorded too late.** Both sessions created the file, awaited more work, and only then stored the verifier — so a retransmission still in flight, which is the one case the feature exists for, found EEXIST and was answered EXIST. 17 of 17 concurrent duplicates failed. Both versions now record it through #createFile's onCreated hook, between the winning open and the close: the last point before an await where a duplicate cannot have overtaken. **v3 CREATE claimed a file it had not created.** #claim ran after open(O_WRONLY|O_CREAT), which succeeds on a file that already exists, so an outsider's UNCHECKED create over an existing file re-owned it — and, new on this branch, chmod'ed it, stripping set-group-ID off a file the caller did not make. v3 now learns what it created the way v4 already did, and claims only that. The plain open is still made on the not-created path, so the refusals this server has always given there (EISDIR, EACCES) do not quietly become NFS3_OK. **The set-gid chmod bypassed the server's umask**, re-applying the client's full requested mode: mkdir 0o775 came out 0o2775 under a set-gid parent where a plain parent gave 0o755 — the same call, one directory over, coming out wider. `claimNewEntry` now reads the entry back and flips that one bit on the mode the driver actually made. The extra lstat falls only on the set-gid path. **ExclusiveCreates.set used a bare .slice()** where invariant 13's documented rule is `copyBytes` — safe only for as long as every caller of the public handleCall hands over a plain Uint8Array rather than a Buffer. handles.ts told two half-truths about eviction: that it costs a v4.1 client an OPEN (it costs a *second* FileState, since v4 keys state by entry id, and a DENY_WRITE stops being enforced across clients), and that MNT hands out the root from a field (it goes through #attrOf, so a subdirectory export's mount root is an ordinary evictable entry). Both corrected, along with the missing floor: a cap below a READDIRPLUS page ships handles already stale. test/latency.ts is load-bearing for two of these: the memory driver answers inside a microtask, so concurrent requests run through it in lockstep and neither concurrency bug is observable against it. Co-Authored-By: Claude Opus 5 --- .agents/roadmap.md | 22 +++++-- .agents/testing.md | 7 +++ docs/2.transports/4.nfs.md | 3 + src/nfs/handles.ts | 93 +++++++++++++++++---------- src/nfs/rpc.ts | 18 ++++-- src/nfs/util.ts | 16 +++-- src/nfs/v3/session.ts | 91 ++++++++++++++++++++------- src/nfs/v4/session.ts | 46 ++++++++++---- src/ownership.ts | 69 +++++++++++++++----- test/index.test.ts | 121 ++++++++++++++++++++++++++++-------- test/latency.ts | 46 ++++++++++++++ test/nfs/session.test.ts | 9 ++- test/nfs/v3/session.test.ts | 113 +++++++++++++++++++++++++++++++++ test/nfs/v4/session.test.ts | 44 +++++++++++++ 14 files changed, 569 insertions(+), 129 deletions(-) create mode 100644 test/latency.ts diff --git a/.agents/roadmap.md b/.agents/roadmap.md index c83d3c0..d6d75b7 100644 --- a/.agents/roadmap.md +++ b/.agents/roadmap.md @@ -126,12 +126,22 @@ rather than by accident. evicts least-recently-used past the cap, root exempt — but the cap is opt-in, so out of the box the table is still one entry per path a client currently has a name for, held for the life of the server. It is opt-in because an eviction - costs an NFSv4.1 client more than a re-`LOOKUP`: `src/nfs/v4/session.ts` keys - open and lock state by entry id, so evicting a handle a client has open costs - it an `OPEN` as well. Pinning entries with live v4 state — and only then - picking a default — is what is left. A directly constructed `Nfs3Session` / - `Nfs4Session` also builds its own table and ignores the option; every public - path goes through the router, so this is the tests and the CLI only. + costs an NFSv4.1 client **correctness**, not a re-`LOOKUP`: + `src/nfs/v4/session.ts` keys open and lock state by entry id (`#fileKey`), so + a file that is evicted and looked up again acquires a _second_ `FileState` — + and the two do not see each other, which means one client's + `OPEN4_SHARE_DENY_WRITE` is silently bypassed by another client's open through + the fresh entry (reproduced). Byte-range locks split the same way. **Pinning + entries with live v4 state is the work**, and only then is a default worth + picking. Two smaller things belong with it: nothing enforces a floor relative + to a READDIRPLUS page (at `maxHandles: 8` one 40-entry page returned 40 + handles of which 33 were stale before the reply left — it converges, and it is + documented rather than clamped), and `MNT` binds the export root through an + ordinary lookup, so a _subdirectory_ export's mount root is evictable and a v3 + client has no name above it to recover with. A directly constructed + `Nfs3Session` / `Nfs4Session` also builds its own table and ignores the option; + every public path goes through the router, so this is the tests and the CLI + only. - **Set-gid inheritance on the FUSE session.** The rule itself now exists — `src/ownership.ts`, a transcription of `inode_init_owner()` — and both NFS sessions apply it: a new entry in a set-gid directory takes the parent's diff --git a/.agents/testing.md b/.agents/testing.md index 5276f8a..774fb3f 100644 --- a/.agents/testing.md +++ b/.agents/testing.md @@ -24,6 +24,13 @@ through its Tier-1 JS client, and FUSE contributes a real-mount column. - `test/no-extensions.ts` is the other side of the same seam: `withoutExtensions(driver)` drops the `mountx` key so the four sessions' no-extension paths keep being tested now that the memory driver has one. +- `test/latency.ts` is the other driver wrapper: `withLatency(driver)` pushes every + call to the next macrotask, because the memory driver answers inside a microtask + and two requests that arrive together therefore run through it in lockstep. That + hides every "the duplicate reached the table before the original wrote to it" + bug — the NFS exclusive-create verifier was recorded a `close()` too late and + every suite was green over it — so a case about _ordering under concurrency_ + belongs against this rather than against a bare memory driver. - A target declares `errors: "host"` when it forwards the host kernel's errors rather than carrying `src/errors.ts`'s table: on Linux that changes nothing, and on darwin it is what lets the suite hold the _code_ exact while allowing either number diff --git a/docs/2.transports/4.nfs.md b/docs/2.transports/4.nfs.md index 6148fea..822f325 100644 --- a/docs/2.transports/4.nfs.md +++ b/docs/2.transports/4.nfs.md @@ -286,6 +286,7 @@ session.v4; // the Nfs4Session underneath, for tests | `verifier` | random | boot verifier for file handles, so restarts invalidate them | | `rtmax` / `wtmax` | — | largest `READ` answered / `WRITE` accepted | | `snapshotCache` | `64` | directory snapshots kept for readdir cookies | +| `maxHandles` | no cap | most file handle table entries to keep, least recently used out first | | `claimOwnership` | `true` | give new entries to the request's `AUTH_SYS` caller | | `onError` | none | called for every request that ends in an error status | | `nfs4` | — | `Nfs4StateKnobs`: NFSv4.1-only knobs, ignored by the v3 session | @@ -294,6 +295,8 @@ session.v4; // the Nfs4Session underneath, for tests The group is the one POSIX gives, not simply the caller's. A **set-group-ID parent directory hands its own group down**, and a new directory inherits the bit as well, so a shared directory stays shared all the way down — the rule Linux applies in `inode_init_owner()` for a local filesystem, applied here because nothing else in the path will. A new group-executable file loses `S_ISGID` when its creator is in neither the effective nor the supplementary groups the `AUTH_SYS` credential carried. Turning `claimOwnership` off leaves every new entry exactly as the driver made it, group and mode included. +`maxHandles` bounds the file handle table, which is otherwise one entry per path a client currently has a name for, held for the life of the server — a client that walks a million-file tree leaves a million entries. It is off by default because an eviction is visible to the client that still holds the handle: an NFSv3 client pays one `ESTALE` and a re-`LOOKUP`, but an **NFSv4.1 client with the file open loses its share reservation**, since the re-lookup is a new entry id and so a second, unrelated open state. Two floors to respect if you set it anyway: keep it comfortably above the largest `READDIRPLUS` page your clients ask for (each name in a page binds a handle, so a smaller cap evicts what the same reply is handing out), and prefer a plain `/` export, because the mount root of a subdirectory export is an ordinary evictable entry with no name above it for a v3 client to look up again. + `nfs4` carries the lease length (90 s default), the id-mapping hooks (`idmap: Nfs4IdMap`, see [Owner strings](#owner-strings)), and the session/slot/operation ceilings a real client negotiates against (`maxSessions`, `maxForeSlots`, `maxOperations`, and the rest) — everything a v3 mount never asks for. ## The macOS consent gate diff --git a/src/nfs/handles.ts b/src/nfs/handles.ts index c13bcea..5043b06 100644 --- a/src/nfs/handles.ts +++ b/src/nfs/handles.ts @@ -52,18 +52,41 @@ * to a different file, and that handle answers `ESTALE` ("unknown file handle") * rather than quietly naming somebody else's. What it costs an NFSv3 client is * one round trip: `ESTALE` means "drop the dentry and look the name up again", - * which re-binds the path and mints a fresh handle. What it costs an NFSv4.1 - * client that had the file **open** is more — `v4/session.ts` keys open and - * lock state by entry id, so the re-lookup is a different file as far as share - * reservations are concerned and the client has to re-`OPEN`. That asymmetry is - * why the cap is off unless asked for, rather than defaulted to some number. + * which re-binds the path and mints a fresh handle. * - * The root is never a victim: `PUTROOTFH`, `PUTPUBFH` and `MNT` hand out its - * handle from the `root` field rather than from a lookup, so an id that stopped - * decoding would be a handle this server minted and immediately refused. - * Neither is the entry currently being bound — the call that overflowed the cap - * is about to answer with it — so the smallest table a cap can produce is those - * two, whatever number it is given. + * **What it costs an NFSv4.1 client that had the file open is correctness, not + * a round trip.** `v4/session.ts` keys open and lock state by *entry id* + * (`#fileKey`), and a re-`LOOKUP` after an eviction mints a **new** id for the + * same file — so the file acquires a second `FileState`, and the two do not see + * each other. A share reservation stops being enforced: one client's + * `OPEN4_SHARE_DENY_WRITE` is silently bypassed by another client's write open + * through the fresh entry, which is a denial the protocol promised and this + * server then did not make. Byte-range locks split the same way. This is why + * the cap is off unless asked for, and why "pin entries with live v4 state" is + * named as the work in `.agents/roadmap.md`'s "A default for `maxHandles`" + * entry rather than a default simply being picked. + * + * **Do not set a cap below a READDIRPLUS page.** One page binds a handle per + * name it returns — at `maxHandles: 8`, a 40-entry page returned 40 handles of + * which 33 were already evicted before the reply left the server. It converges + * (the client re-`LOOKUP`s and each `bind` re-mints), so it is self-correcting + * rather than broken, but a cap smaller than the largest page a client asks for + * is a cap that spends its whole life evicting what it just handed out. The + * floor worth honouring is "comfortably more than `dircount`/`maxcount` can + * produce", which for the defaults real clients use is in the hundreds. + * + * The root is never a victim, and one operation depends on that outright: + * `PUTROOTFH` and `PUTPUBFH` hand out the root's handle from the `root` field + * rather than from a lookup, so an id that stopped decoding would be a handle + * this server minted and immediately refused. `MNT` is **not** in that list — + * `#mnt` binds through `#attrOf(path)` like any other lookup — so the mount + * root of a *subdirectory* export (`127.0.0.1:/sub`) is an ordinary evictable + * entry, and its eviction is the one `ESTALE` a v3 client cannot recover from + * by re-`LOOKUP`, having no name above it to look up. A cap and a subdirectory + * export together want a table far larger than the working set, or a fresh + * `MNT`. Neither is the entry currently being bound a victim — the call that + * overflowed the cap is about to answer with it — so the smallest table a cap + * can produce is the root plus that one, whatever number it is given. */ import { randomFillSync } from "node:crypto"; @@ -126,9 +149,12 @@ export interface FileHandleTableOptions { * * The number counts everything {@link FileHandleTable.size} does, the root * included. The root and the entry being bound are never evicted, so a cap - * below two is honoured as far as it can be rather than refused. See the - * module docs for what an eviction costs a client still holding the handle — - * it is a real cost, which is why there is no default. + * below two is honoured as far as it can be rather than refused. Nothing + * enforces a floor, and there is one worth knowing: a cap smaller than the + * largest READDIRPLUS page a client asks for spends its life evicting the + * handles it just handed out, and an eviction costs an NFSv4.1 client with + * the file open a **share reservation**, not a round trip. Both are in the + * module docs, and both are why there is no default. */ maxHandles?: number; } @@ -142,10 +168,10 @@ export class FileHandleTable { readonly #useDriverIno: boolean; /** Entries to keep, or `0` for "no cap" — see {@link FileHandleTableOptions.maxHandles}. */ readonly #maxHandles: number; - /** Doubles as the LRU order: insertion order, youngest last. See {@link FileHandleTable.touch}. */ + /** Doubles as the LRU order: insertion order, youngest last. See `#touch`. */ readonly #byId = new Map(); readonly #byPath = new Map(); - /** Only holds entries with at least one path — see {@link FileHandleTable.detachPath}. */ + /** Only holds entries with at least one path — see `#detachPath`. */ readonly #byKey = new Map(); #nextId = ROOT_HANDLE_ID + 1n; /** @@ -367,10 +393,10 @@ export class FileHandleTable { * with no idea anything moved. * * The walk itself is `src/subtree.ts`'s — the same one `InodeTable` and - * `FidTable` do — driven by *this* table's {@link FileHandleTable.detachPath} - * and {@link FileHandleTable.attachPath}, so what a replaced destination - * means here is unchanged: the entry loses its identity key and its id, and - * the handle the client still holds answers `ESTALE`. It costs two full scans + * `FidTable` do — driven by *this* table's `#detachPath` and `#attachPath`, + * so what a replaced destination means here is unchanged: the entry loses its + * identity key and its id, and the handle the client still holds answers + * `ESTALE`. It costs two full scans * of the path map per rename, i.e. O(tracked paths) rather than O(subtree); * see that module for why a prefix tree is the fix and why it is a * benchmark-milestone concern, not a v1 one. @@ -439,8 +465,8 @@ export class FileHandleTable { // last sentence is the whole licence for evicting a *live* entry too: the // client is told `ESTALE`, never handed somebody else's file. // - // The root is the exception: `PUTROOTFH` and `MNT` hand out its handle from - // the `root` field rather than from a lookup, so an id that no longer + // The root is the exception: `PUTROOTFH` and `PUTPUBFH` hand out its handle + // from the `root` field rather than from a lookup, so an id that no longer // decodes would be a handle this server minted and immediately refuses. if (entry.id !== ROOT_HANDLE_ID) { this.#byId.delete(entry.id); @@ -462,7 +488,7 @@ export class FileHandleTable { * `decode` is not free. The root is a no-op too, and for the opposite reason: * it can never be a victim, so its place in the order means nothing, and * leaving it where it was inserted — the old end, for the life of the table — - * is what keeps {@link FileHandleTable.lruVictim}'s scan to a step or two. + * is what keeps `#lruVictim`'s scan to a step or two. */ #touch(entry: HandleEntry): void { if (this.#maxHandles <= 0 || entry.id === ROOT_HANDLE_ID) { @@ -509,18 +535,17 @@ export class FileHandleTable { /** * Drop an entry the client may still be naming. * - * Name by name through {@link FileHandleTable.detachPath}, so this is the - * same teardown as a file whose last link went away rather than a second one - * beside it — the maps cannot end up disagreeing about an entry that is gone, - * because only one piece of code decides what "gone" does. The trailing - * {@link FileHandleTable.dropEntry} covers the case the loop cannot reach: - * `pathOf` forgets a name the path map no longer agrees with without going - * through `#detachPath` at all, so an entry can be down to no names and still - * be here. It is idempotent, so paying for it unconditionally is cheaper than - * asking. + * Name by name through `#detachPath`, so this is the same teardown as a file + * whose last link went away rather than a second one beside it — the maps + * cannot end up disagreeing about an entry that is gone, because only one + * piece of code decides what "gone" does. The trailing `#dropEntry` covers + * the case the loop cannot reach: `pathOf` forgets a name the path map no + * longer agrees with without going through `#detachPath` at all, so an entry + * can be down to no names and still be here. It is idempotent, so paying for + * it unconditionally is cheaper than asking. * - * Deleting the name being visited is what {@link FileHandleTable.pruneToNlink} - * already does to the same set, and is well defined for a `Set` iterator. + * Deleting the name being visited is what `#pruneToNlink` already does to the + * same set, and is well defined for a `Set` iterator. */ #evict(entry: HandleEntry): void { for (const path of entry.paths) { diff --git a/src/nfs/rpc.ts b/src/nfs/rpc.ts index f12db28..b392ed8 100644 --- a/src/nfs/rpc.ts +++ b/src/nfs/rpc.ts @@ -461,9 +461,9 @@ export function frameFragments(message: Uint8Array, size: number): Uint8Array { /** * A genuine copy, whatever the input is. * - * Not `bytes.slice(start, end)`: the input here is always a socket `Buffer`, - * and `Buffer.prototype.slice` **is `subarray`** — it returns a view, the trap - * `xdr.ts` documents at length. + * Not `bytes.slice(start, end)`: the input here is very often a socket + * `Buffer`, and `Buffer.prototype.slice` **is `subarray`** — it returns a view, + * the trap `xdr.ts` documents at length. * * `xdr.ts` spells its copy `Uint8Array.prototype.slice.call`, which is equally * correct and costs the same — the two are within noise of each other at every @@ -473,8 +473,18 @@ export function frameFragments(message: Uint8Array, size: number): Uint8Array { * whose own `.slice()` is `subarray` again. A record exists to be owned * outright by whoever holds it; handing back a plain `Uint8Array` keeps that * from depending on what the socket happened to allocate. + * + * Which is why this is the copy anything **retaining** wire bytes past the call + * they arrived on uses — `RecordAssembler` below, and the verifiers + * `ExclusiveCreates` (`./util.ts`) keeps for two minutes. `handleCall` is + * public and takes any `Uint8Array`, so "the assembler happens to allocate + * plain ones" is not something a table outliving the request may rely on. */ -function copyBytes(bytes: Uint8Array, start: number, end: number): Uint8Array { +export function copyBytes( + bytes: Uint8Array, + start = 0, + end: number = bytes.byteLength, +): Uint8Array { const out = new Uint8Array(end - start); out.set(bytes.subarray(start, end)); return out; diff --git a/src/nfs/util.ts b/src/nfs/util.ts index c91632b..f9ab417 100644 --- a/src/nfs/util.ts +++ b/src/nfs/util.ts @@ -21,7 +21,7 @@ import type { PathLock } from "../lock.ts"; import type { StatsLike } from "../types.ts"; import { S_IFDIR, S_IFMT } from "../types.ts"; import { sameVerifier, type FileHandleTable } from "./handles.ts"; -import type { RpcCall, RpcCredentials } from "./rpc.ts"; +import { copyBytes, type RpcCall, type RpcCredentials } from "./rpc.ts"; /** * The largest offset either version can name. @@ -141,8 +141,10 @@ export interface NfsSessionOptions { * first past it. Default: no cap, which is what this server has always done. * * An eviction costs the client holding that handle an `ESTALE` and a - * re-lookup — and an NFSv4.1 client that had the file open rather more than - * that. See `./handles.ts` before setting it. + * re-lookup — and an NFSv4.1 client that had the file **open** its share + * reservation, since the re-lookup is a different entry id and so a different + * `FileState`. A cap below the largest READDIRPLUS page a client asks for is + * also self-defeating. Read `./handles.ts` before setting it. */ maxHandles?: number; /** Largest `READ` answered. */ @@ -288,9 +290,13 @@ export class ExclusiveCreates { /** Remember `verifier` as the one that created `path`. */ set(path: string, verifier: Uint8Array, attrset: readonly number[] = []): void { // Copied, both of them: this outlives the request they were decoded from, - // and `attrset` is the caller's own array, still being pushed to. + // and `attrset` is the caller's own array, still being pushed to. The copy + // is `copyBytes` and not `verifier.slice()` because `handleCall` is public + // and takes any `Uint8Array`: hand it a `Buffer` and `.slice()` is + // `subarray`, which would leave this table holding a view of somebody + // else's receive buffer for the next two minutes. const entry: ExclusiveCreate = { - verifier: verifier.slice(), + verifier: copyBytes(verifier), attrset: [...attrset], at: this.#now(), }; diff --git a/src/nfs/v3/session.ts b/src/nfs/v3/session.ts index 5df9c44..d3bfbe2 100644 --- a/src/nfs/v3/session.ts +++ b/src/nfs/v3/session.ts @@ -1112,9 +1112,16 @@ export class Nfs3Session { * rule and where it comes from). That is why `parent` is threaded in from the * `lstat` `#preOpStats` already did for the reply's `wcc_data` rather than * stat'ed here — every creating operation in this file reads the parent - * anyway, so the rule costs no extra round trip. It does cost a `chmod` when - * a new directory has to take the bit, which is the one case that cannot be - * folded into the create. + * anyway, so the rule costs no extra round trip. It does cost an `lstat` and + * a `chmod` when a new directory has to take the bit, which is the one case + * that cannot be folded into the create: the mode the create asked for is not + * the mode it got (`../../ownership.ts` on the umask), so the bit goes on top + * of what the driver actually made. + * + * Only ever for something **this call created** — an `UNCHECKED` CREATE that + * found the file already there must not re-own it, and must not re-mode it + * either, which would strip the set-group-ID bit off a file the caller had + * nothing to do with. */ async #claim(path: string, creds: RpcCredentials, entry: NewEntry): Promise { if (this.options.claimOwnership === false) { @@ -1139,14 +1146,25 @@ export class Nfs3Session { return; } const mode = (request.attributes?.mode ?? 0o666) & 0o7777; - const flags = - constants.O_WRONLY | - constants.O_CREAT | - (request.mode === CREATE_GUARDED ? constants.O_EXCL : 0); - const handle = await this.driver.open(path, flags, mode); - await handle.close(); + const created = await this.#createFile(path, mode); + if (!created && request.mode === CREATE_GUARDED) { + // "GUARDED ... the server checks for the presence of a duplicate + // object by name before performing the create." + throw new NfsStatusError(NFS3ERR_EXIST, "an object of that name already exists"); + } this.#invalidate(dir); - await this.#claim(path, creds, { parent, directory: false, mode }); + if (created) { + await this.#claim(path, creds, { parent, directory: false, mode }); + } else { + // `UNCHECKED` over an existing file is an `open(2)` of that file, and + // its refusals are this server's answer: `EISDIR` for a directory, + // `EACCES` for a file this caller may not write. Learning whether the + // create won had to be an `O_EXCL` attempt (a `stat` first would be a + // race, and would tell `#claim` to re-own and re-mode a file this call + // did not make), so the plain open is made here instead of skipped. + const handle = await this.driver.open(path, constants.O_WRONLY | constants.O_CREAT, mode); + await handle.close(); + } // `UNCHECKED` over an existing file still applies the attributes, which // is how a client asks for `open(…, O_CREAT|O_TRUNC)` in one round trip. await this.#applySattr(path, { ...request.attributes, mode: undefined }); @@ -1189,17 +1207,14 @@ export class Nfs3Session { parent: StatsLike | undefined, ): Promise { const verifier = verf ?? new Uint8Array(NFS3_CREATEVERFSIZE); - let handle: FileHandleLike; - try { - handle = await this.driver.open( - path, - constants.O_WRONLY | constants.O_CREAT | constants.O_EXCL, - 0o666, - ); - } catch (error) { - if ((error as { code?: string }).code !== "EEXIST") { - throw error; - } + // Recorded from inside the create, before it awaits anything else: the + // duplicate this mode exists for is a *retransmission*, sent because the + // reply was lost, and it therefore arrives while the original is still in + // flight. A verifier recorded after the `close()` — or after the `#claim` + // below — is one the duplicate looks for, does not find, and is answered + // `NFS3ERR_EXIST` for, by the very create that succeeded. + const created = await this.#createFile(path, 0o666, () => this.#exclusives.set(path, verifier)); + if (!created) { if (this.#exclusives.match(path, verifier) === undefined) { throw new NfsStatusError(NFS3ERR_EXIST, "a different file already has that name"); } @@ -1208,12 +1223,42 @@ export class Nfs3Session { // caller answers with the file this client already made. return; } - await handle.close(); - this.#exclusives.set(path, verifier); this.#invalidate(dir); await this.#claim(path, creds, { parent, directory: false, mode: 0o666 }); } + /** + * Create a file, answering whether it was **this call** that created it. + * + * `O_EXCL` rather than a `stat` first, for the same reason `v4/session.ts` + * learns it the same way: a `stat` would be a race, and every caller here + * needs the answer to be the truth about its own create — `#claim` may only + * give away a file this call made, and an `UNCHECKED` CREATE over a file + * somebody else owns must not re-own it or strip the set-group-ID bit off it. + * + * `onCreated` runs after the create wins and before the `close()`, i.e. + * before this function's next `await`. That is the only place a concurrent + * duplicate cannot already have overtaken — see `#createExclusive`. + */ + async #createFile(path: string, mode: number, onCreated?: () => void): Promise { + let handle: FileHandleLike; + try { + handle = await this.driver.open( + path, + constants.O_WRONLY | constants.O_CREAT | constants.O_EXCL, + mode, + ); + } catch (error) { + if ((error as { code?: string }).code === "EEXIST") { + return false; + } + throw error; + } + onCreated?.(); + await handle.close(); + return true; + } + async #mkdir(args: XdrReader, creds: RpcCredentials, writer: XdrWriter): Promise { const request = readMkdirArgs(args); args.end("MKDIR arguments"); diff --git a/src/nfs/v4/session.ts b/src/nfs/v4/session.ts index 58a206b..75971d9 100644 --- a/src/nfs/v4/session.ts +++ b/src/nfs/v4/session.ts @@ -2336,8 +2336,10 @@ export class Nfs4Session { * `parent` is therefore threaded in from the `stat` the caller already took — * CREATE reads the parent to check it is a directory, OPEN to build its * `change_info4` — so the rule adds no round trip of its own. A new directory - * taking the set-gid bit does cost a `chmod`, which is the one part of it - * that cannot be folded into the create. + * taking the set-gid bit does cost an `lstat` and a `chmod`, which is the one + * part of it that cannot be folded into the create: the mode the create asked + * for is not the mode it got (`../../ownership.ts` on the umask), so the bit + * goes on top of what the driver actually made. */ async #claim(path: string, creds: RpcCredentials, entry: NewEntry): Promise { if (this.options.claimOwnership === false) { @@ -3246,7 +3248,17 @@ export class Nfs4Session { this.#settableOrRefuse(attrs); } const mode = (attrs?.values.mode ?? 0o666) & 0o7777; - const created = await this.#createFile(path, mode); + // Recorded from inside the create, before it awaits anything else: the + // duplicate these two modes exist for is a *retransmission*, sent because + // the reply was lost, and it therefore arrives while the original is still + // in flight. A verifier recorded after the `#claim` and `#applyAttrs` + // below is one the duplicate looks for, does not find, and is answered + // `NFS4ERR_EXIST` for, by the very create that succeeded. It goes in with + // no `attrset`, and is rewritten with the real one further down — an + // under-reported `attrset` is the case §18.16.4 already tells client + // implementors to check for and follow with a SETATTR, whereas an `EXIST` + // for one's own file is not recoverable at all. + const created = await this.#createFile(path, mode, () => this.#exclusives.set(path, verifier)); if (!created) { const remembered = this.#exclusives.match(path, verifier); if (remembered === undefined) { @@ -3273,31 +3285,41 @@ export class Nfs4Session { bits.push(...applied.bits); status = applied.status; } - // Recorded even when an attribute failed to land: the file exists and this - // verifier is what made it, so a resend must still be recognised — and it - // is answered with the bits that *did* land, which is the partial `attrset` - // §18.16.4 tells client implementors to check for. + // Re-recorded, now that there are bits to record, and still recorded when + // an attribute failed to land: the file exists and this verifier is what + // made it, so a resend must be recognised either way — and it is answered + // with the bits that *did* land, which is the partial `attrset` §18.16.4 + // tells client implementors to check for. this.#exclusives.set(path, verifier, bits); attrset.push(...bits); return status === NFS4_OK ? undefined : status; } - /** Create a file, answering whether it was this call that created it. */ - async #createFile(path: string, mode: number): Promise { + /** + * Create a file, answering whether it was this call that created it. + * + * `onCreated` runs after the create wins and before the `close()`, i.e. + * before this function's next `await` — the only place a concurrent + * duplicate cannot already have overtaken. `#openCreateExclusive` records + * its verifier there and says why. + */ + async #createFile(path: string, mode: number, onCreated?: () => void): Promise { + let handle: FileHandleLike; try { - const handle = await this.driver.open( + handle = await this.driver.open( path, constants.O_CREAT | constants.O_EXCL | constants.O_WRONLY, mode, ); - await this.#close(handle); - return true; } catch (error) { if ((error as { code?: string }).code === "EEXIST") { return false; } throw error; } + onCreated?.(); + await this.#close(handle); + return true; } /** diff --git a/src/ownership.ts b/src/ownership.ts index 93782f5..9d522ae 100644 --- a/src/ownership.ts +++ b/src/ownership.ts @@ -57,18 +57,28 @@ export interface NewEntry { mode: number; } -/** What the new entry's owner, group and mode should be. */ +/** What the new entry's owner, group and set-group-ID bit should be. */ export interface NewEntryOwnership { /** For `lchown`; `-1` means "leave it alone". */ uid: number; /** For `lchown`; `-1` means "leave it alone". */ gid: number; /** - * The mode the entry must be changed to, or `undefined` when the mode the - * create used is already right — which is the overwhelmingly common answer, - * and the one that costs no extra driver call. + * The set-group-ID bit the entry has to end up with, or `undefined` when the + * create already settled it — which is the overwhelmingly common answer, and + * the one that costs no extra driver call. + * + * A **bit**, not a mode, and deliberately: the mode a create asks for is not + * the mode it gets. `open(2)` and `mkdir(2)` mask their mode argument with + * the umask of the process the driver runs in (and `vfs_mkdir` strips + * `S_ISGID` from it outright), so re-asserting the requested mode through + * `chmod` would hand the caller *wider* permissions than the same call gets + * one directory over — at umask 022, `mkdir 0775` coming out `2775` under a + * set-group-ID parent and `0755` under a plain one. {@link claimNewEntry} + * therefore reads back what the driver actually created and changes this one + * bit of it. */ - mode: number | undefined; + setgid: boolean | undefined; } /** @@ -85,13 +95,17 @@ export function newEntryOwnership(caller: CallerCredentials, entry: NewEntry): N const parent = entry.parent; if (parent === undefined || (parent.mode & S_ISGID) === 0) { // "} else inode->i_gid = current_fsgid();" - return { uid, gid: caller.gid ?? -1, mode: undefined }; + return { uid, gid: caller.gid ?? -1, setgid: undefined }; } // "if (dir && dir->i_mode & S_ISGID) { inode->i_gid = dir->i_gid;" const gid = parent.gid; if (entry.directory) { - // "Directories are special, and always inherit S_ISGID." - return { uid, gid, mode: (entry.mode & S_ISGID) === 0 ? entry.mode | S_ISGID : undefined }; + // "Directories are special, and always inherit S_ISGID." Asked for even + // when the create named the bit itself: `mkdir(2)` is not allowed to set + // it ("mode &= (S_IRWXUGO | S_ISVTX);" in `vfs_mkdir()`), so whether it is + // there is a question for the driver rather than for the request, and + // `claimNewEntry` skips the `chmod` when the answer is already yes. + return { uid, gid, setgid: true }; } // "else if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) && // !in_group_p(inode->i_gid) && !capable_wrt_inode_uidgid(dir, CAP_FSETID)) @@ -100,15 +114,16 @@ export function newEntryOwnership(caller: CallerCredentials, entry: NewEntry): N const setgidExecutable = (entry.mode & (S_ISGID | S_IXGRP)) === (S_ISGID | S_IXGRP); const member = caller.gid === gid || caller.gids?.includes(gid) === true; if (setgidExecutable && !member && uid !== 0) { - return { uid, gid, mode: entry.mode & ~S_ISGID }; + return { uid, gid, setgid: false }; } - return { uid, gid, mode: undefined }; + return { uid, gid, setgid: undefined }; } -/** The two driver calls giving an entry away can take. */ +/** The three driver calls giving an entry away can take. */ interface OwnershipDriver { lchown(path: string, uid: number, gid: number): Promise; chmod(path: string, mode: number): Promise; + lstat(path: string): Promise; } /** @@ -117,10 +132,10 @@ interface OwnershipDriver { * Deliberately skipped when the answer is the state the driver already * produced — the caller *is* the server process and nothing was inherited — * because that is the common case and worth a round trip. Deliberately quiet - * when the driver has no `lchown`/`chmod` (`ENOSYS`) or is not privileged - * enough to hand ownership away (`EPERM`/`ENOTSUP`): a driver with no concept - * of ownership is not thereby broken, and failing the create it just completed - * would be the wrong answer to that. + * when the driver has no `lchown`/`lstat`/`chmod` (`ENOSYS`) or is not + * privileged enough to hand ownership away (`EPERM`/`ENOTSUP`): a driver with + * no concept of ownership is not thereby broken, and failing the create it just + * completed would be the wrong answer to that. * * The `chmod` runs **after** the `lchown`, not before: `chown(2)` clears * set-group-ID on an executable when an unprivileged caller changes ownership, @@ -135,8 +150,28 @@ export async function claimNewEntry( if (!mine && (owner.uid !== -1 || owner.gid !== -1)) { await quietly(driver.lchown(path, owner.uid, owner.gid)); } - if (owner.mode !== undefined) { - await quietly(driver.chmod(path, owner.mode)); + if (owner.setgid !== undefined) { + await quietly(setgid(driver, path, owner.setgid)); + } +} + +/** + * Turn `S_ISGID` on (or off) on an entry that has just been created. + * + * The mode chmod'ed is the one the driver **made**, read back, not the one the + * create asked for: `open(2)`/`mkdir(2)` mask their mode argument with the + * umask of the process the driver runs in and a `chmod` is not masked at all, + * so re-asserting the requested mode here would silently widen the entry (see + * {@link NewEntryOwnership.setgid}). That `lstat` is paid **only under a + * set-group-ID parent** — every other create leaves `setgid` `undefined` and + * never reaches here — and it is the same call that lets a driver which already + * applied the rule itself be left alone. + */ +async function setgid(driver: OwnershipDriver, path: string, on: boolean): Promise { + const current = (await driver.lstat(path)).mode & 0o7777; + const wanted = on ? current | S_ISGID : current & ~S_ISGID; + if (wanted !== current) { + await driver.chmod(path, wanted); } } diff --git a/test/index.test.ts b/test/index.test.ts index a5719c4..f9f9a22 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -1,7 +1,10 @@ import { spawn } from "node:child_process"; -import { readFile, stat as nodeStat } from "node:fs/promises"; +import { mkdtemp, readFile, rm, stat as nodeStat } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { describe, expect, it } from "vitest"; import { createMemoryDriver } from "../src/drivers/memory.ts"; +import { createNodeFsDriver } from "../src/drivers/node-fs.ts"; import { basename, createLoopback, @@ -361,14 +364,14 @@ describe("set-gid inheritance", () => { it("takes the caller's group when the parent has no set-gid bit", () => { expect( newEntryOwnership(caller, { parent: dir(0o40755, 4000), directory: false, mode: 0o644 }), - ).toEqual({ uid: 1000, gid: 1000, mode: undefined }); + ).toEqual({ uid: 1000, gid: 1000, setgid: undefined }); }); it("inherits nothing from a parent that could not be read", () => { expect(newEntryOwnership(caller, { parent: undefined, directory: true, mode: 0o755 })).toEqual({ uid: 1000, gid: 1000, - mode: undefined, + setgid: undefined, }); }); @@ -378,38 +381,47 @@ describe("set-gid inheritance", () => { expect(newEntryOwnership({}, { parent: undefined, directory: false, mode: 0o644 })).toEqual({ uid: -1, gid: -1, - mode: undefined, + setgid: undefined, }); expect( newEntryOwnership({}, { parent: dir(0o42775, 4000), directory: false, mode: 0o644 }), - ).toEqual({ uid: -1, gid: 4000, mode: undefined }); + ).toEqual({ uid: -1, gid: 4000, setgid: undefined }); }); - it("gives a new directory the bit, and does not ask for a chmod it already has", () => { + it("asks for the bit on a new directory, whatever mode the create named", () => { expect( newEntryOwnership(caller, { parent: dir(0o42775, 4000), directory: true, mode: 0o755 }), - ).toEqual({ uid: 1000, gid: 4000, mode: 0o2755 }); + ).toEqual({ uid: 1000, gid: 4000, setgid: true }); + // Asked for even when the create named it: `mkdir(2)` is not allowed to set + // `S_ISGID` itself, so whether it is there is a question for the driver. + // `claimNewEntry` reads back and skips the `chmod` when it already is. expect( newEntryOwnership(caller, { parent: dir(0o42775, 4000), directory: true, mode: 0o2755 }), - ).toEqual({ uid: 1000, gid: 4000, mode: undefined }); + ).toEqual({ uid: 1000, gid: 4000, setgid: true }); }); it("clears set-gid on an executable for a group the caller is not in", () => { const entry = { parent: dir(0o42775, 4000), directory: false, mode: 0o2775 }; - expect(newEntryOwnership(caller, entry).mode).toBe(0o775); + expect(newEntryOwnership(caller, entry).setgid).toBe(false); // Membership counts whether it is the effective group or a supplementary // one, and a privileged caller keeps the bit either way // (`capable_wrt_inode_uidgid(dir, CAP_FSETID)`). - expect(newEntryOwnership({ uid: 1000, gid: 4000 }, entry).mode).toBeUndefined(); - expect(newEntryOwnership({ ...caller, gids: [4000] }, entry).mode).toBeUndefined(); - expect(newEntryOwnership({ ...caller, uid: 0 }, entry).mode).toBeUndefined(); + expect(newEntryOwnership({ uid: 1000, gid: 4000 }, entry).setgid).toBeUndefined(); + expect(newEntryOwnership({ ...caller, gids: [4000] }, entry).setgid).toBeUndefined(); + expect(newEntryOwnership({ ...caller, uid: 0 }, entry).setgid).toBeUndefined(); // Not group-executable: nothing to run as, so nothing to clear. - expect(newEntryOwnership(caller, { ...entry, mode: 0o2664 }).mode).toBeUndefined(); + expect(newEntryOwnership(caller, { ...entry, mode: 0o2664 }).setgid).toBeUndefined(); }); describe("applying it", () => { - /** A driver that records its calls, and can be told to refuse them. */ - function recorder(refusal?: string) { + /** + * A driver that records its calls, and can be told to refuse them. + * + * `lstat` answers with `mode`, which is what the entry is supposed to have + * come back from the create as — the whole point of the read-back being + * that it is not the mode the create *asked* for. + */ + function recorder(mode = 0o755, refusal?: string) { const calls: string[] = []; const answer = async (what: string): Promise => { calls.push(what); @@ -419,15 +431,34 @@ describe("set-gid inheritance", () => { calls, lchown: (path: string, uid: number, gid: number) => answer(`lchown ${path} ${uid}:${gid}`), chmod: (path: string, mode: number) => answer(`chmod ${path} ${mode.toString(8)}`), + lstat: async (path: string) => { + calls.push(`lstat ${path}`); + if (refusal !== undefined) throw fsError(refusal as ErrnoCode); + return { mode } as StatsLike; + }, }; } it("chowns, then chmods — never the other way round", async () => { - const driver = recorder(); - await claimNewEntry(driver, "/f", { uid: 1000, gid: 4000, mode: 0o2755 }); + const driver = recorder(0o755); + await claimNewEntry(driver, "/f", { uid: 1000, gid: 4000, setgid: true }); // `chown(2)` clears set-group-ID on an executable when an unprivileged // caller changes ownership, so the bit has to go on afterwards. - expect(driver.calls).toEqual(["lchown /f 1000:4000", "chmod /f 2755"]); + expect(driver.calls).toEqual(["lchown /f 1000:4000", "lstat /f", "chmod /f 2755"]); + }); + + it("chmods the mode the driver made, not the one the create asked for", async () => { + // The create asked for 0o775 and a umask of 022 made it 0o755. Asserting + // the requested mode here would hand the caller group-write it was denied + // one directory over, purely because this parent is set-gid. + const driver = recorder(0o755); + await claimNewEntry(driver, "/d", { uid: -1, gid: 4000, setgid: true }); + expect(driver.calls).toEqual(["lchown /d -1:4000", "lstat /d", "chmod /d 2755"]); + + // And the clearing direction reads back the same way. + const executable = recorder(0o2755); + await claimNewEntry(executable, "/x", { uid: -1, gid: 4000, setgid: false }); + expect(executable.calls).toEqual(["lchown /x -1:4000", "lstat /x", "chmod /x 755"]); }); it("does nothing when the driver already produced the answer", async () => { @@ -435,24 +466,60 @@ describe("set-gid inheritance", () => { await claimNewEntry(driver, "/f", { uid: process.getuid?.() ?? -1, gid: process.getgid?.() ?? -1, - mode: undefined, + setgid: undefined, }); - await claimNewEntry(driver, "/f", { uid: -1, gid: -1, mode: undefined }); + await claimNewEntry(driver, "/f", { uid: -1, gid: -1, setgid: undefined }); expect(driver.calls).toEqual([]); + + // ...including a driver that applied the rule itself: the bit is already + // there, so the read-back is the end of it. + const inherited = recorder(0o2755); + await claimNewEntry(inherited, "/d", { uid: -1, gid: -1, setgid: true }); + expect(inherited.calls).toEqual(["lstat /d"]); + }); + + /** + * The same thing against a driver with a real umask behind it. + * + * The recorder above proves what is chmod'ed; this proves the number it is + * computed from is the truth. `createNodeFsDriver` goes to `node:fs`, so + * `mkdir`'s mode is masked by the umask of *this* process and the `chmod` + * that follows is not — which is the whole bug: asserting the requested + * mode here widens a directory that a plain parent would have narrowed. + * The memory driver applies no umask unless asked, which is why none of the + * session suites could see it. + */ + it("reads back a real driver rather than widening what the umask narrowed", async () => { + const sandbox = await mkdtemp(join(tmpdir(), "mountx-setgid-")); + try { + const driver = createNodeFsDriver(sandbox); + await driver.mkdir!("/d", { mode: 0o775 }); + const created = (await driver.lstat!("/d")).mode & 0o7777; + await claimNewEntry(driver as never, "/d", { uid: -1, gid: -1, setgid: true }); + const claimed = (await driver.lstat!("/d")).mode & 0o7777; + // Exactly one bit different, whatever this host's umask turned 0o775 + // into — 0o755 at the usual 022, which the old rule published as + // 0o2775. + expect(claimed).toBe(created | 0o2000); + } finally { + await rm(sandbox, { recursive: true, force: true }); + } }); it("is quiet for a driver with no concept of ownership, and only for that", async () => { for (const code of ["ENOSYS", "EPERM", "ENOTSUP"]) { - const driver = recorder(code); - await claimNewEntry(driver, "/f", { uid: 1000, gid: 4000, mode: 0o2755 }); - // Quiet, and the refused `lchown` does not stop the `chmod` from being - // tried: they are two different things the driver may or may not have. - expect(driver.calls).toEqual(["lchown /f 1000:4000", "chmod /f 2755"]); + const driver = recorder(0o755, code); + await claimNewEntry(driver, "/f", { uid: 1000, gid: 4000, setgid: true }); + // Quiet, and the refused `lchown` does not stop the mode half from + // being tried: they are two different things the driver may or may not + // have. The `lstat` refusing is the end of that half, since there is + // nothing to put the bit on top of. + expect(driver.calls).toEqual(["lchown /f 1000:4000", "lstat /f"]); } // Anything else is a real failure of the create that just happened. - const broken = recorder("EIO"); + const broken = recorder(0o755, "EIO"); await expect( - claimNewEntry(broken, "/f", { uid: 1000, gid: 4000, mode: undefined }), + claimNewEntry(broken, "/f", { uid: 1000, gid: 4000, setgid: undefined }), ).rejects.toMatchObject({ code: "EIO" }); }); }); diff --git a/test/latency.ts b/test/latency.ts new file mode 100644 index 0000000..23555ef --- /dev/null +++ b/test/latency.ts @@ -0,0 +1,46 @@ +/** + * A driver that takes a real turn of the event loop to answer. + * + * The bundled memory driver answers every call inside one microtask, and that + * hides a whole class of ordering bug: two requests that arrive together run + * through it in lockstep, so a session's bookkeeping is always in place before + * the second request looks at it, however late the session writes it. No real + * driver behaves that way — `node:fs` is a threadpool round trip per call — and + * the difference is exactly the window a *retransmission* arrives in. + * + * So every method here, and every method of the handles `open` returns, is + * pushed to the next macrotask before it runs. Nothing else changes: the + * results, the errors and the order the driver itself sees the calls in are the + * driver's own. This is what makes "the duplicate reached the table before the + * original wrote to it" reproducible without a temp directory and a real + * filesystem. + * + * Not a `*.test.ts` file: it is imported by several. + */ + +import type { FileHandleLike, FsDriver } from "../src/types.ts"; + +/** The next macrotask, which is one full turn later than a microtask. */ +const turn = (): Promise => new Promise((resolve) => setImmediate(resolve)); + +/** Delay every call of `value`'s methods by one turn, `apply`ing on the target. */ +function delayed(value: T, onOpen?: (handle: unknown) => unknown): T { + return new Proxy(value, { + get(target, property, receiver) { + const member = Reflect.get(target, property, receiver) as unknown; + if (typeof member !== "function") { + return member; + } + return async (...args: unknown[]): Promise => { + await turn(); + const result = await (member as (...rest: unknown[]) => unknown).apply(target, args); + return property === "open" && onOpen !== undefined ? onOpen(result) : result; + }; + }, + }); +} + +/** The same driver, one event-loop turn slower on every call it answers. */ +export function withLatency(driver: T): FsDriver { + return delayed(driver, (handle) => delayed(handle as FileHandleLike)); +} diff --git a/test/nfs/session.test.ts b/test/nfs/session.test.ts index c388348..91c3fc6 100644 --- a/test/nfs/session.test.ts +++ b/test/nfs/session.test.ts @@ -23,6 +23,7 @@ * bottom is `ExclusiveCreates`' window and size cap. */ +import { Buffer } from "node:buffer"; import { describe, expect, it } from "vitest"; import { createMemoryDriver } from "../../src/drivers/memory.ts"; import { @@ -236,7 +237,11 @@ describe("ExclusiveCreates", () => { it("copies the verifier and the attrset it is handed", () => { const table = new ExclusiveCreates(); - const verifier = Uint8Array.from(VERIFIER); + // A `Buffer`, deliberately: `NfsSession.handleCall` is public and takes any + // `Uint8Array`, and `Buffer.prototype.slice` **is `subarray`** — so a + // `verifier.slice()` here passes against a plain `Uint8Array` and quietly + // stores a *view* of a socket's receive pool against a real one. + const verifier = Buffer.from(VERIFIER); const attrset = [33]; table.set("/f", verifier, attrset); // The request buffer is reused and the caller goes on pushing to its @@ -244,6 +249,8 @@ describe("ExclusiveCreates", () => { verifier.fill(0); attrset.push(4); expect(table.match("/f", VERIFIER)?.attrset).toEqual([33]); + // ...and the bytes that were reused are not the ones now being matched. + expect(table.match("/f", verifier)).toBeUndefined(); }); it("forgets an entry past the window, and a lookup does not extend it", () => { diff --git a/test/nfs/v3/session.test.ts b/test/nfs/v3/session.test.ts index 3aa7e25..e010ed8 100644 --- a/test/nfs/v3/session.test.ts +++ b/test/nfs/v3/session.test.ts @@ -47,6 +47,7 @@ import { NFS3ERR_TOOSMALL, NFS_PROGRAM, NFS_V3, + NFSPROC3_CREATE, NFSPROC3_LOOKUP, NFSPROC3_NULL, NFSPROC3_READ, @@ -66,8 +67,10 @@ import { type OpaqueAuth, } from "../../../src/nfs/rpc.ts"; import { encodeXdr } from "../../../src/nfs/xdr.ts"; +import { readCreateRes, writeCreateArgs } from "../../../src/nfs/v3/protocol.ts"; import { createNfsServer, type NfsServer } from "../../../src/nfs/server.ts"; import type { FsDriver, FullFsDriver } from "../../../src/types.ts"; +import { withLatency } from "../../latency.ts"; import { withoutExtensions } from "../../no-extensions.ts"; import { check, NfsClient, nfsDriver } from "./client.ts"; import { createLoopback, type Loopback } from "../../../src/harness.ts"; @@ -796,6 +799,52 @@ describe("individual procedures", () => { ); }); + /** + * The same promise, kept against the duplicate that actually happens. + * + * A retransmission is sent because the reply was *lost*, which means it goes + * out while the original is still in flight — not after it has been answered. + * A verifier recorded any later than "the create won" is therefore a verifier + * the duplicate looks for, does not find, and is told `NFS3ERR_EXIST` about + * by the very request that succeeded. + * + * Driven through {@link withLatency}, because the ordering is only observable + * against a driver that takes a real turn to answer: 17 of 17 duplicates were + * told `EXIST` before the verifier moved ahead of the `close()`. + */ + it("answers duplicates that arrive while the original is still in flight", async () => { + const { server, root } = await serve(withLatency(createMemoryDriver())); + // The same record 18 times — same xid and all, which is what an RPC + // retransmission *is* — handed to the session together rather than one + // after another, so none of them can see another's reply. The server + // dispatches every record it reads without awaiting it, so this is the + // arrival pattern a congested client produces, not a synthetic one. + const record = encodeCall({ + xid: 0x3c_3c_3c_3c, + program: NFS_PROGRAM, + version: NFS_V3, + procedure: NFSPROC3_CREATE, + args: encodeXdr((w) => + writeCreateArgs(w, { + where: { dir: root, name: "excl" }, + mode: CREATE_EXCLUSIVE, + attributes: {}, + verf: VERIFIER, + }), + ), + }); + const replies = await Promise.all( + Array.from({ length: 18 }, () => server.session.handleCall(record)), + ); + const results = replies.map((reply) => readCreateRes(decodeReply(reply!).results)); + const first = check(results[0]!, "create"); + for (const result of results) { + expect(result.status).toBe(NFS3_OK); + // Every one of them is the same file, which is the whole promise. + expect([...check(result, "create").obj!]).toEqual([...first.obj!]); + } + }); + it("forgets the verifier once the client commits with SETATTR", async () => { const { client, root } = await serve(); const created = check( @@ -1136,6 +1185,70 @@ describe("set-gid inheritance", () => { expect(plainMode.objAttributes!.mode).toBe(0o2664); }); + /** + * The bit goes on top of the mode the driver **made**, not the one the client + * asked for. + * + * `open`/`mkdir` mask their mode argument with the umask of the process the + * driver runs in and a `chmod` is not masked at all, so a rule that + * re-asserts the requested mode hands the caller *wider* permissions in a + * set-gid directory than the same call gets one directory over. The memory + * driver applies no umask by default, which is why every case above is blind + * to this; one that does is the smallest thing that can see it. + */ + it("puts the bit on the mode the driver made, not the one the client asked for", async () => { + const driver = createMemoryDriver({ umask: 0o022 }); + await driver.mkdir("/team"); + await driver.chown("/team", 500, TEAM); + await driver.chmod("/team", 0o2775); + await driver.mkdir("/plain"); + const { client, root } = await serve(driver, { cred: OUTSIDER }); + const team = check(await client.lookup(root, "team"), "lookup").object!; + const plain = check(await client.lookup(root, "plain"), "lookup").object!; + + // 0o775 asked for, 0o755 created, 0o2755 after the bit: the same mode the + // plain directory produces, plus the one bit that was inherited. + const inherited = check(await client.mkdir(team, "sub", { mode: 0o775 }), "mkdir"); + const control = check(await client.mkdir(plain, "sub", { mode: 0o775 }), "mkdir"); + expect(control.objAttributes!.mode).toBe(0o755); + expect(inherited.objAttributes!.mode).toBe(0o2755); + + // The clearing half reads back the same way: 0o2775 asked for, 0o2755 + // created, 0o755 once the bit this caller may not hand out comes off. + const executable = check( + await client.create(team, "run", CREATE_UNCHECKED, { mode: 0o2775 }), + "create", + ); + expect(executable.objAttributes!.mode).toBe(0o755); + }); + + /** + * `#claim` is about a *new* entry, and an `UNCHECKED` CREATE is not always + * one: §3.3.8's UNCHECKED "does not check for the existence of the file", so + * it succeeds over a file somebody else made and owns. Claiming that file + * would hand it to whoever named it last — and, once the rule started + * `chmod`ing as well, would strip the set-gid bit off it on the way. + */ + it("claims only the entry the call actually created", async () => { + const driver = await fixture(); + // Somebody else's shared executable, already in the team directory. + await (await driver.open("/team/run", "w")).close(); + await driver.chown("/team/run", 500, TEAM); + await driver.chmod("/team/run", 0o2775); + + const { client, root } = await serve(driver, { cred: OUTSIDER }); + const team = check(await client.lookup(root, "team"), "lookup").object!; + const over = check(await client.create(team, "run", CREATE_UNCHECKED), "create"); + expect(over.objAttributes!.uid).toBe(500); + expect(over.objAttributes!.gid).toBe(TEAM); + expect(over.objAttributes!.mode).toBe(0o2775); + + // ...and the file this call *did* create is claimed exactly as before. + const made = check(await client.create(team, "new", CREATE_UNCHECKED), "create"); + expect(made.objAttributes!.uid).toBe(4242); + expect(made.objAttributes!.gid).toBe(TEAM); + }); + it("does not claim at all with claimOwnership off", async () => { const { client, team } = await served({ claimOwnership: false }); const created = check(await client.create(team, "f", CREATE_UNCHECKED), "create"); diff --git a/test/nfs/v4/session.test.ts b/test/nfs/v4/session.test.ts index 4225afd..974162d 100644 --- a/test/nfs/v4/session.test.ts +++ b/test/nfs/v4/session.test.ts @@ -211,6 +211,7 @@ import { Nfs4Session } from "../../../src/nfs/v4/session.ts"; import { XdrWriter } from "../../../src/nfs/xdr.ts"; import { createLoopback } from "../../../src/harness.ts"; import { createNfsServer, type NfsServer } from "../../../src/nfs/server.ts"; +import { withLatency } from "../../latency.ts"; import { withoutExtensions } from "../../no-extensions.ts"; import { CREATE_EXCLUSIVE, NFS3ERR_EXIST } from "../../../src/nfs/v3/constants.ts"; import { check as check3, NfsClient as Nfs3Client, nfsDriver } from "../v3/client.ts"; @@ -2301,6 +2302,49 @@ describe("OPEN", () => { } }); + /** + * The same promise, kept against the duplicate that actually happens. + * + * A retransmission is sent because the reply was *lost*, so it goes out while + * the original is still in flight rather than after it has been answered. A + * verifier recorded any later than "the create won" — after the `#claim`, or + * after the `cva_attrs` are applied — is one the duplicate looks for, does + * not find, and is told `NFS4ERR_EXIST` about by the very OPEN that + * succeeded. {@link withLatency} is what makes that ordering observable: the + * memory driver answers inside a microtask and no duplicate can lose to it. + */ + it("answers duplicates that arrive while the original is still in flight", async () => { + for (const mode of [EXCLUSIVE4, EXCLUSIVE4_1]) { + const session = new Nfs4Session(withLatency(await populated())); + // Separate 4.1 sessions, because one slot table is one request at a time + // — which is the client-side reason a resend is a *new* connection's + // problem as often as it is a retransmission on this one. + const clients = await Promise.all( + Array.from({ length: 8 }, (_, index) => ready(session, `client-${index}`)), + ); + const replies = await Promise.all( + clients.map((client, index) => + client.run([ + ...TO_DIR, + OPEN({ + name: "exclusive", + owner: index + 1, + openhow: exclusiveHow(mode, VERIFIER), + }), + { op: OP_GETFH }, + ]), + ), + ); + const first = resFor(replies[0]!, OP_GETFH).object!; + for (const reply of replies) { + expect(reply.compound.status).toBe(NFS4_OK); + // Every one of them is the same file, which is the whole promise. + expect([...resFor(reply, OP_GETFH).object!]).toEqual([...first]); + } + expect(session.handles.resolve(first)).toBe("/dir/exclusive"); + } + }); + it("applies EXCLUSIVE4_1's cva_attrs and replays the same attrset", async () => { const session = new Nfs4Session(await populated()); const client = await ready(session); From 52bb6fb3978bac64e04c59bc90f68cfb3da4f947 Mon Sep 17 00:00:00 2001 From: Pooya Parsa Date: Fri, 31 Jul 2026 20:18:04 +0000 Subject: [PATCH 6/7] docs: keep finished work out of the roadmap entirely MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The file said "open work only" and then narrated what had shipped in half its entries — "the extension itself shipped", "half of this already ships", "the bound shipped and so did the correctness fix under it". A reader looking for something to pick up had to rule out a paragraph of history first. The header now says what the rule actually is: finished work is not mentioned at all, and an entry that needs a fact about what exists states it in the present tense, as the constraint the remaining work runs into. Every entry is rewritten to that, and the per-call-credentials entry moves to "Decisions still binding" — it is a decision, not work. Co-Authored-By: Claude Opus 5 --- .agents/roadmap.md | 175 +++++++++++++++++++++------------------------ 1 file changed, 82 insertions(+), 93 deletions(-) diff --git a/.agents/roadmap.md b/.agents/roadmap.md index 0b2f23d..fd01db4 100644 --- a/.agents/roadmap.md +++ b/.agents/roadmap.md @@ -7,6 +7,13 @@ map, `.agents/invariants.md` the rules, `.agents/conformance-matrix.md` / Nothing is removed from this file until it is done _and_ recorded in one of those. +**Then it is removed entirely.** Finished work is not mentioned here at all — +not as a checked box, not as "X shipped", not as "half of this is closed". An +entry that needs a fact about what exists states it in the present tense, as the +constraint the remaining work runs into; the entry is deleted the moment nothing +is left to run into it. A reader here is looking for what to pick up, and every +sentence about something already done is a sentence they have to rule out first. + ## Decisions still binding - **Layout:** single `mountx` package with subpath exports; the pnpm workspace @@ -28,6 +35,24 @@ those. - **The S3 gateway stays out of `mountx/auto`.** Auto's whole contract is a mountpoint and that transport never produces one. - **NLM byte-range locking is out of scope**, hence NFS mounts use `nolock`. +- **Per-call caller credentials are not going into `FsDriver`.** The shape would + be a `mountx.*` extension carrying the caller's uid/gid/groups into each + creating call, so ownership is the driver's from the start instead of a + post-hoc `lchown` in each session's `#claim`. Three things against it, and + they compound: `node:fs/promises` has no credential parameter and the POSIX + way to get one — `setfsuid(2)`/`setfsgid(2)` — is per-thread state Node does + not expose and the threadpool would scramble, so `node-fs` could never + implement it and `unstorage`/S3 have no ownership to implement it with; the + memory driver, the only one that could, enforces no permissions at all, so the + only observable effect of a credential inside it is the initial owner and + group, which `src/ownership.ts` decides; and invariant 5 means every session + keeps the `lchown` path anyway for drivers without the extension, so the whole + surface buys one driver's atomicity at the price of two permanent code paths. + What would reopen it is a driver that enforces permissions itself — there the + credential decides whether a call _succeeds_, not just who owns the result, + and `#claim` cannot express that at all. Where the **server** decides, the + credential is already first-class: `allowedAccess()` in `src/nfs/util.ts` + answers ACCESS from the effective and supplementary groups both. - **Subagent models:** implementation work SHOULD use `opus`; docs work SHOULD use `sonnet`. @@ -37,9 +62,7 @@ Nothing here blocks a release; each is a real gap worth picking up deliberately rather than by accident. - **FUSE reader concurrency: one mode worth building, two not.** Async - main-thread mode is all that ships. This bullet used to ask for a relay mode - and a sync-driver-in-workers mode; a design pass against - `.agents/benchmarks.md` cut it down, and what follows is the residue. + main-thread mode is all there is. **Relay buys no throughput.** The reader-count experiment already says pulling requests off `/dev/fuse` is not the bottleneck, so a mode whose whole content @@ -75,23 +98,18 @@ rather than by accident. and use the mount from another. `bench/drive.ts` does exactly that in reverse, which is why no benchmark column has ever wedged. -- **9P `trans=fd`, no longer waiting on a relay.** It was deferred twice, and - both premises are gone. `.agents/9p-plan.md` deferred it because Node - supposedly cannot create a socketpair without native code; that is **false**, - and verified on this host — libuv's `stdio: "pipe"` on POSIX _is_ - `socketpair(AF_UNIX, SOCK_STREAM)`, the child's inherited fd reads +- **9P `trans=fd`.** Reachable today with no relay and no native code, and an + ordinary option to be judged on its own merits: Node _can_ create a socketpair + without native code — verified on this host, libuv's `stdio: "pipe"` on POSIX + **is** `socketpair(AF_UNIX, SOCK_STREAM)`, the child's inherited fd reads `socket:[…]`, and the parent end is a full-duplex `net.Socket` that - round-trips bytes. This file deferred it again because it wanted a descriptor - only a relay would hold; with relay cut down to a worker that holds nothing of - the sort, that no longer follows either. So `trans=fd` is reachable today with - no relay and no native code, and it is now an ordinary option to be judged on - its own merits. Two facts need one root mount each first: whether `mount(8)` - keeps an inherited socket fd open across `mount(2)`, and whether v9fs's - `trans=fd` drives an `AF_UNIX` stream. -- **An NFSv4.1 bench column.** 9P has one now (`pnpm bench:9p`), which leaves - v4.1 as the last transport that is in the conformance matrix and not in the - benchmarks. -- **Whether `DEFAULT_MAX_IN_FLIGHT` should be higher.** The 9P column put numbers + round-trips bytes. (`.agents/9p-plan.md` says otherwise; it is wrong.) Two + facts need one root mount each first: whether `mount(8)` keeps an inherited + socket fd open across `mount(2)`, and whether v9fs's `trans=fd` drives an + `AF_UNIX` stream. +- **An NFSv4.1 bench column.** The last transport that is in the conformance + matrix and not in the benchmarks. +- **Whether `DEFAULT_MAX_IN_FLIGHT` should be higher.** The 9P column has numbers behind both of that transport's knobs, and they point opposite ways. The dispatch window pays: 64 against the shipped 16 is 1.34–1.35× on stats 64 deep, and closing it to 1 costs 0.87–0.94×, so the default sits below where it stops @@ -112,19 +130,17 @@ rather than by accident. covered at Tier 0/1 — two connections on one `createP9Server`, including the release when one drops — but two _real_ mounts of one server, a guest and its host or two guests, is not, and needs a host that can produce them. -- **A driver → transport "path X changed underneath you" channel.** Half of this - already ships, and it is the transport-facing half: `notifyInvalInode()` / - `notifyInvalEntry()` on the FUSE session (`src/fuse/session.ts`) and on the - mount (`src/fuse/mount.ts`), pure encoders in `src/fuse/notify.ts`. What is - missing is the driver end — nothing in `FsDriver` can say a path changed, so - the only caller today is code that already knows because it made the change - itself. It is **not** a `mountx.*` member: every extension there is a - path-shaped call the session makes, and this is an event the driver raises - unprompted, so it wants a `watch`-shaped surface on `FsDriver` rather than a - method in the namespace. The FUSE half of the translation is already in place - — `InodeTable.byPath` turns the path into the `ino` a notification needs, and - a path the kernel never looked up has nothing cached, which is the natural - no-op. +- **A driver → transport "path X changed underneath you" channel.** The driver + end is what is missing: nothing in `FsDriver` can say a path changed, so the + only caller of `notifyInvalInode()` / `notifyInvalEntry()` + (`src/fuse/session.ts`, `src/fuse/mount.ts`, encoders in `src/fuse/notify.ts`) + is code that already knows because it made the change itself. It is **not** a + `mountx.*` member: every extension there is a path-shaped call the session + makes, and this is an event the driver raises unprompted, so it wants a + `watch`-shaped surface on `FsDriver` rather than a method in the namespace. + The translation costs nothing — `InodeTable.byPath` turns the path into the + `ino` a notification needs, and a path the kernel never looked up has nothing + cached, which is the natural no-op. **FUSE is the only transport that could consume it today, and the only one that can without new protocol work.** 9P has no invalidation message at all (below). NFSv3 is request/response only: client caching is bounded by @@ -163,9 +179,8 @@ rather than by accident. unprivileged, zero-native-code path for macOS/Windows. Windows also has no `mount(8)`, so it stays out of the NFS transport's platform switch. - **A `mknod` polyfill for drivers that cannot store a special file - themselves.** The extension itself shipped (`mountx.mknod` + the memory - driver, which closed the whole pjdfstest gap). What is deferred is the - fallback: an overlay holding the node in memory only, writing nothing through + themselves.** A driver without `mountx.mknod` answers `ENOTSUP`; the fallback + would be an overlay holding the node in memory only, writing nothing through to the backing store — a small union filesystem, because it has to win on every creating call (`mkdir`/`symlink`/`link`/`open(O_CREAT)`/a `rename` destination must answer `EEXIST` against an overlaid name) and not merely on @@ -176,13 +191,13 @@ rather than by accident. every other one. - **xattr and the rest of the `mountx.*` extension namespace** (byte-range locks, `fallocate`/`lseek`) — `ENOTSUP` until a real user needs one. - Cache-invalidation `notify` used to be listed here and is not an extension at - all; it has its own entry above. -- **`suppattr_exclcreat` (75) is still unadvertised** in `src/nfs/v4/attr.ts`'s - `SUPPORTED_ATTRS`. Exclusive create shipped, and its verifier lives beside the - file rather than in an attribute, so the honest value is now the full settable - set — the attribute's "deliberately absent, it is the OPEN step's call" note - predates the OPEN step having made it. + Cache-invalidation `notify` is not an extension at all; it has its own entry + above. +- **`suppattr_exclcreat` (75) is unadvertised** in `src/nfs/v4/attr.ts`'s + `SUPPORTED_ATTRS`. The exclusive-create verifier lives beside the file rather + than in an attribute, so the honest value is the full settable set — the + attribute's "deliberately absent, it is the OPEN step's call" note predates + the OPEN step making that call. - **FUSE over io_uring** (Linux 6.14+) — would replace the read/reply loop with a shared ring and obsolete the threadpool discussion entirely; the transport layer should stay swappable so this is one file, not a rewrite. @@ -199,57 +214,31 @@ rather than by accident. - **`mountx/drivers/s3`** — an `FsDriver` over a real S3-compatible bucket, mounting one rather than serving one; the natural follow-up to the S3 gateway transport. It would reuse `test/s3/client.ts`'s signing client instead of - writing a second one. Not started. -- **A default for `maxHandles`.** The bound itself shipped — the handle table - evicts least-recently-used past the cap, root exempt — but the cap is opt-in, - so out of the box the table is still one entry per path a client currently has - a name for, held for the life of the server. It is opt-in because an eviction - costs an NFSv4.1 client **correctness**, not a re-`LOOKUP`: - `src/nfs/v4/session.ts` keys open and lock state by entry id (`#fileKey`), so - a file that is evicted and looked up again acquires a _second_ `FileState` — - and the two do not see each other, which means one client's - `OPEN4_SHARE_DENY_WRITE` is silently bypassed by another client's open through - the fresh entry (reproduced). Byte-range locks split the same way. **Pinning - entries with live v4 state is the work**, and only then is a default worth - picking. Two smaller things belong with it: nothing enforces a floor relative - to a READDIRPLUS page (at `maxHandles: 8` one 40-entry page returned 40 - handles of which 33 were stale before the reply left — it converges, and it is + writing a second one. +- **A default for `maxHandles`.** The cap is opt-in, and what makes picking a + default a judgement call rather than a number is that **the cap is soft**: an + entry with live NFSv4.1 state is pinned and never a victim, so when every + candidate is pinned the table exceeds `maxHandles` rather than breaking a + share reservation, and it does not evict its way back down until the opens + close. A default is therefore a hint about the working set, not a memory + ceiling, and the honest one depends on a workload nobody has measured here + yet. Two smaller things belong with it: nothing enforces a floor relative to a + READDIRPLUS page (at `maxHandles: 8` one 40-entry page returns 40 handles of + which 33 are stale before the reply leaves — it converges, and it is documented rather than clamped), and `MNT` binds the export root through an ordinary lookup, so a _subdirectory_ export's mount root is evictable and a v3 client has no name above it to recover with. A directly constructed - `Nfs3Session` / `Nfs4Session` also builds its own table and ignores the option; - every public path goes through the router, so this is the tests and the CLI - only. -- **Set-gid inheritance on the FUSE session.** The rule itself now exists — - `src/ownership.ts`, a transcription of `inode_init_owner()` — and both NFS - sessions apply it: a new entry in a set-gid directory takes the parent's - group, a new directory takes the bit, and a new group-executable loses - `S_ISGID` when its creator is in neither the effective nor the supplementary - group `AUTH_SYS` carried. 9P needs nothing: its client computes both halves - (`v9fs_get_fsgid_for_create()`) and they arrive on the wire. **FUSE is the - gap**, and `#claim` in `src/fuse/session.ts` says why it was not simply - mirrored: the NFS sessions read the parent anyway (for `wcc_data` and - `change_info4`) while nothing on the FUSE path does, so the rule would add an - `lstat` to every create for every caller — including the daemon-is-the-caller - case that currently costs no driver call at all — and the membership half - needs `FUSE_CREATE_SUPP_GROUP` (7.38), which `src/fuse/init.ts` does not ask - for. Both are decisions to take deliberately, not omissions. -- **Per-call caller credentials in `FsDriver`: considered, and not the - answer.** The shape was a `mountx.*` extension carrying the caller's - uid/gid/groups into each creating call, so ownership would be the driver's - from the start instead of a post-hoc `lchown` in `#claim`. Three things - against it, and they compound: `node:fs/promises` has no credential parameter - and the POSIX way to get one — `setfsuid(2)`/`setfsgid(2)` — is per-thread - state Node does not expose and the threadpool would scramble, so `node-fs` - could never implement it and `unstorage`/S3 have no ownership to implement it - with; the memory driver, the only one that could, enforces no permissions at - all, so the sole observable effect of a credential inside it is the initial - owner and group, which `src/ownership.ts` now gets right; and invariant 5 - means every session must keep the `lchown` path anyway for drivers without - the extension, so the whole surface buys one driver's atomicity at the price - of two permanent code paths. What would reopen it is a driver that enforces - permissions itself — at that point the credential decides whether a call - _succeeds_, not just who owns the result, and `#claim` cannot express that at - all. Note that the credential is already first-class where the **server** - decides: `allowedAccess()` in `src/nfs/util.ts` answers ACCESS from the - effective and supplementary groups both. + `Nfs3Session` / `Nfs4Session` also builds its own table and ignores the + option; every public path goes through the router, so this is the tests and + the CLI only. +- **Set-gid inheritance on the FUSE session.** `src/ownership.ts` holds the rule + (`inode_init_owner()`) and the two NFS sessions apply it from the parent + `stat` they take anyway for `wcc_data` / `change_info4`. 9P needs nothing — + its client computes both halves (`v9fs_get_fsgid_for_create()`) and they + arrive on the wire. FUSE is the gap, and `#claim` in `src/fuse/session.ts` + says why it is not simply mirrored: nothing on that path reads the parent, so + the rule would add an `lstat` to every create for every caller — including the + daemon-is-the-caller case that costs no driver call at all — and the + membership half needs `FUSE_CREATE_SUPP_GROUP` (7.38), which + `src/fuse/init.ts` does not ask for. Both are decisions to take deliberately, + not omissions. From 4f1d3484268b4a55be3df9ee442d9d162e2ec927 Mon Sep 17 00:00:00 2001 From: Pooya Parsa Date: Fri, 31 Jul 2026 20:21:55 +0000 Subject: [PATCH 7/7] fix(nfs): pin handle-table entries with live NFSv4.1 state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `FileState` — opens, share reservations, byte-range locks — is keyed by the handle-table entry id, so an LRU eviction of an entry a client had open handed the same file a *second* `FileState` on the next LOOKUP, and conflict detection stopped seeing the first. Reproduced with two v4.1 clients: A's OPEN4_SHARE_DENY_WRITE was silently bypassed by B after B walked enough files to push A's entry out. An entry now carries a pin refcount and is never a victim while it is held. The pins are driven from the one site that adds to `#files` and the one site that removes from it (`onFileRetained`/`onFileReleased`), so they balance by construction rather than by auditing CLOSE, lease revocation, sweep, DESTROY_CLIENTID and EXCHANGE_ID replacement one at a time; `destroy()` releases what is left, since the table is the router's and outlives the session. Two paths had to stop creating state they were about to discard: a refused LOCK built an empty `FileState`, and LOCKU created one to forget it. Pinning is only about eviction — a pinned entry whose last path goes away is still dropped, because a removed file's handles are meant to go stale. **The cap becomes soft**, and that is the trade: when every candidate is pinned the table exceeds `maxHandles` rather than breaking a reservation, and it does not evict its way back down until the opens close. That is documented where someone setting the option will read it, and it is why `maxHandles` still has no default. Co-Authored-By: Claude Opus 5 --- .agents/architecture.md | 2 +- docs/2.transports/4.nfs.md | 6 +- src/nfs/handles.ts | 150 +++++++++++++++++++++++++-------- src/nfs/util.ts | 11 ++- src/nfs/v4/session.ts | 81 +++++++++++++++++- src/nfs/v4/state.ts | 61 ++++++++++++-- test/nfs/handles.test.ts | 111 ++++++++++++++++++++++++ test/nfs/v4/session.test.ts | 164 ++++++++++++++++++++++++++++++++++++ 8 files changed, 537 insertions(+), 49 deletions(-) diff --git a/.agents/architecture.md b/.agents/architecture.md index da06629..5f15695 100644 --- a/.agents/architecture.md +++ b/.agents/architecture.md @@ -89,7 +89,7 @@ Two versions behind one router and one server. Linux and macOS — and on macOS | ----------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | | `xdr.ts` | XDR (RFC 4506) — bounds-checked, big-endian, 64-bit as `bigint` | | `rpc.ts` | ONC RPC v2 (RFC 5531) + TCP record marking + `AUTH_NONE`/`AUTH_SYS`. Version-neutral, so it lives here rather than in `v3/` | -| `handles.ts` | `FileHandleTable` (ids off a monotonic counter, so a dropped entry still answers `ESTALE`; `maxHandles` evicts LRU past a cap, root exempt) and `DirectorySnapshots` | +| `handles.ts` | `FileHandleTable` (ids off a monotonic counter, so a dropped entry still answers `ESTALE`; `maxHandles` evicts LRU past a cap, root and `pin`ned entries exempt — so the cap is soft) and `DirectorySnapshots` | | `util.ts` | the seam between `v3/` and `v4/` — version-neutral POSIX logic, `NfsSessionOptions`, `Nfs4IdMap`, `ExclusiveCreates` (the create verifiers a retransmission is recognised by), and the `NfsSharedState` the router builds once | | `session.ts` | `NfsSession` — peeks `prog`/`vers` and hands the **same raw bytes** on. Everything public is the versioned session's, reached through here | | `server.ts` | one connection per socket, window of 64 (it counter-offers `ca_maxrequests` up to 64) | diff --git a/docs/2.transports/4.nfs.md b/docs/2.transports/4.nfs.md index 822f325..58dd926 100644 --- a/docs/2.transports/4.nfs.md +++ b/docs/2.transports/4.nfs.md @@ -295,7 +295,11 @@ session.v4; // the Nfs4Session underneath, for tests The group is the one POSIX gives, not simply the caller's. A **set-group-ID parent directory hands its own group down**, and a new directory inherits the bit as well, so a shared directory stays shared all the way down — the rule Linux applies in `inode_init_owner()` for a local filesystem, applied here because nothing else in the path will. A new group-executable file loses `S_ISGID` when its creator is in neither the effective nor the supplementary groups the `AUTH_SYS` credential carried. Turning `claimOwnership` off leaves every new entry exactly as the driver made it, group and mode included. -`maxHandles` bounds the file handle table, which is otherwise one entry per path a client currently has a name for, held for the life of the server — a client that walks a million-file tree leaves a million entries. It is off by default because an eviction is visible to the client that still holds the handle: an NFSv3 client pays one `ESTALE` and a re-`LOOKUP`, but an **NFSv4.1 client with the file open loses its share reservation**, since the re-lookup is a new entry id and so a second, unrelated open state. Two floors to respect if you set it anyway: keep it comfortably above the largest `READDIRPLUS` page your clients ask for (each name in a page binds a handle, so a smaller cap evicts what the same reply is handing out), and prefer a plain `/` export, because the mount root of a subdirectory export is an ordinary evictable entry with no name above it for a v3 client to look up again. +`maxHandles` bounds the file handle table, which is otherwise one entry per path a client currently has a name for, held for the life of the server — a client that walks a million-file tree leaves a million entries. It is off by default because an eviction is visible to the client that still holds the handle: an NFSv3 client pays one `ESTALE` and a re-`LOOKUP`. + +It is a **soft cap**. An entry an NFSv4.1 client has open, or holds a byte-range lock on, is never evicted — its share reservations are keyed to that entry's id, and taking it would let another client's write open bypass a `DENY_WRITE` that is still in force — so a client holding more files open than the cap allows entries pushes the table past the number, and the table does not evict its way back down until those opens close. Sizing it is therefore a judgement about the working set, not a hard ceiling on memory. + +Two floors to respect: keep it comfortably above the largest `READDIRPLUS` page your clients ask for (each name in a page binds a handle, so a smaller cap evicts what the same reply is handing out), and prefer a plain `/` export, because the mount root of a subdirectory export is an ordinary evictable entry with no name above it for a v3 client to look up again. `nfs4` carries the lease length (90 s default), the id-mapping hooks (`idmap: Nfs4IdMap`, see [Owner strings](#owner-strings)), and the session/slot/operation ceilings a real client negotiates against (`maxSessions`, `maxForeSlots`, `maxOperations`, and the rest) — everything a v3 mount never asks for. diff --git a/src/nfs/handles.ts b/src/nfs/handles.ts index 5043b06..9bbe7bf 100644 --- a/src/nfs/handles.ts +++ b/src/nfs/handles.ts @@ -38,8 +38,10 @@ * Unset — the default — nothing is evicted, and the table grows to one entry * per path the client currently has a name for: a client that walks a * million-file tree leaves a million entries, and nothing on the wire will ever - * say they can go. Set, the table keeps at most that many entries (the root - * counted) and drops the least recently *used* one past the cap. Recency is + * say they can go. Set, the table aims at that many entries (the root counted) + * and drops the least recently *used* one past the cap — aims, because it will + * not take an entry with live NFSv4.1 state on it, which is "Pins" below. + * Recency is * stamped by every path that reaches an entry — `decode`, `at`, `pathOf`, and * every `bind`/`remap` attach — not only by the call that created it, so the * entries a client is working with are the last ones considered, and the one it @@ -54,18 +56,6 @@ * one round trip: `ESTALE` means "drop the dentry and look the name up again", * which re-binds the path and mints a fresh handle. * - * **What it costs an NFSv4.1 client that had the file open is correctness, not - * a round trip.** `v4/session.ts` keys open and lock state by *entry id* - * (`#fileKey`), and a re-`LOOKUP` after an eviction mints a **new** id for the - * same file — so the file acquires a second `FileState`, and the two do not see - * each other. A share reservation stops being enforced: one client's - * `OPEN4_SHARE_DENY_WRITE` is silently bypassed by another client's write open - * through the fresh entry, which is a denial the protocol promised and this - * server then did not make. Byte-range locks split the same way. This is why - * the cap is off unless asked for, and why "pin entries with live v4 state" is - * named as the work in `.agents/roadmap.md`'s "A default for `maxHandles`" - * entry rather than a default simply being picked. - * * **Do not set a cap below a READDIRPLUS page.** One page binds a handle per * name it returns — at `maxHandles: 8`, a 40-entry page returned 40 handles of * which 33 were already evicted before the reply left the server. It converges @@ -87,6 +77,38 @@ * `MNT`. Neither is the entry currently being bound a victim — the call that * overflowed the cap is about to answer with it — so the smallest table a cap * can produce is the root plus that one, whatever number it is given. + * + * ## Pins ({@link FileHandleTable.pin}), and why the cap is soft + * + * An NFSv4.1 client that had the file *open* could not have paid in round trips + * the way the v3 client above does. `v4/session.ts` keys open and lock state by + * entry id (`#fileKey`), so an evicted entry re-`LOOKUP`ed under a new id would + * give one file a second `FileState`, and the two would not see each other: one + * client's `OPEN4_SHARE_DENY_WRITE` would be silently bypassed by another + * client's write open through the fresh entry — a denial the protocol promised + * and this server then did not make — and byte-range locks would split the same + * way. + * + * So an entry with live v4.1 state is **pinned**, and `#lruVictim` walks past + * pinned entries. The v4 session pins a key for exactly as long as + * `v4/state.ts` holds a `FileState` for it (`onFileRetained`/`onFileReleased`, + * which bracket the one `Map` those states live in), and unpins whatever is + * left on `destroy()`. + * + * **The cap is therefore a soft one, deliberately.** When every candidate is + * pinned there is no victim to take, and `#enforceLimit` stops rather than + * breaking a share reservation: the table exceeds `maxHandles` and does *not* + * evict its way back down — it comes down as the opens close and ordinary + * binds find victims again. A client holding a thousand files open against + * `maxHandles: 100` gets a thousand-entry table, and that is the trade being + * made: the bound is advice, the reservation is a promise. + * + * A pin is about the LRU and nothing else. An entry whose **last path** goes + * away is still dropped outright, pinned or not: a removed file's handles are + * `ESTALE` whether or not somebody has it open (this server was never told a + * file was opened — see the third bullet at the top), and letting a pin keep a + * deleted file's entry alive would be a leak with a `rm` trigger rather than a + * reservation being kept. */ import { randomFillSync } from "node:crypto"; @@ -119,6 +141,12 @@ export interface HandleEntry { fileid: bigint; /** Insertion-ordered; the first is the primary path every driver call uses. */ readonly paths: Set; + /** + * Holds against LRU eviction while it is above zero — see + * {@link FileHandleTable.pin}. Bookkeeping, not state a caller sets: go + * through `pin`/`unpin`, which are the only things that can keep it balanced. + */ + pins: number; } function identityKey(stats: StatsLike): string | undefined { @@ -148,13 +176,18 @@ export interface FileHandleTableOptions { * option existed. * * The number counts everything {@link FileHandleTable.size} does, the root - * included. The root and the entry being bound are never evicted, so a cap - * below two is honoured as far as it can be rather than refused. Nothing - * enforces a floor, and there is one worth knowing: a cap smaller than the - * largest READDIRPLUS page a client asks for spends its life evicting the - * handles it just handed out, and an eviction costs an NFSv4.1 client with - * the file open a **share reservation**, not a round trip. Both are in the - * module docs, and both are why there is no default. + * included. **It is a soft cap.** The root, the entry being bound and every + * entry {@link FileHandleTable.pin}ned by live NFSv4.1 state are not + * evictable, so a table with nothing else to take simply grows past this + * number rather than breaking a share reservation, and it does not evict its + * way back down afterwards — a client holding more files open than the cap + * allows entries is a table the size of its opens. A cap below two is + * likewise honoured as far as it can be rather than refused. + * + * Nothing enforces a floor, and there is one worth knowing: a cap smaller + * than the largest READDIRPLUS page a client asks for spends its life + * evicting the handles it just handed out. That, and the soft cap above, are + * in the module docs, and they are why there is no default. */ maxHandles?: number; } @@ -197,6 +230,7 @@ export class FileHandleTable { key: undefined, fileid: ROOT_HANDLE_ID, paths: new Set(["/"]), + pins: 0, }; this.#byId.set(ROOT_HANDLE_ID, this.root); this.#byPath.set("/", this.root); @@ -204,9 +238,10 @@ export class FileHandleTable { /** * Entries currently known — one per file that still has at least one name, - * plus the root, and never more than - * {@link FileHandleTableOptions.maxHandles} when one was given. See the - * module docs for what is and is not dropped. + * plus the root. A cap holds this down to + * {@link FileHandleTableOptions.maxHandles} as far as it can: pinned entries + * are not evictable, so the number can sit above the cap. See the module + * docs for what is and is not dropped. */ get size(): number { return this.#byId.size; @@ -271,6 +306,40 @@ export class FileHandleTable { return this.#byId.get(id); } + /** + * Hold an entry against LRU eviction until the matching + * {@link FileHandleTable.unpin}. + * + * A refcount rather than a flag, because the caller's own reasons to hold an + * entry nest: `v4/state.ts` keeps one `FileState` per file for opens *and* + * byte-range locks, and a second holder must not be able to release the + * first one's claim. **Every path that takes a pin has to drop it, error + * paths included** — a leaked pin is an entry the table can never evict + * again, which is the failure this is worth testing for. + * + * Only the LRU is affected. A pinned entry whose last path is detached is + * still dropped (see `#detachPath`), and both calls are no-ops for an id the + * table no longer has — which is what makes them safe to leave balanced + * across a `REMOVE` that took the entry out from under the holder. + * + * The root is pinnable like anything else and gains nothing by it: it is + * never a victim. + */ + pin(id: bigint): void { + const entry = this.#byId.get(id); + if (entry !== undefined) { + entry.pins++; + } + } + + /** Release one {@link FileHandleTable.pin}. Never goes below zero. */ + unpin(id: bigint): void { + const entry = this.#byId.get(id); + if (entry !== undefined && entry.pins > 0) { + entry.pins--; + } + } + at(path: string): HandleEntry | undefined { const entry = this.#byPath.get(path); if (entry !== undefined) { @@ -323,7 +392,7 @@ export class FileHandleTable { if (entry === undefined) { const id = this.#nextId++; - entry = { id, key, fileid: fileidOf(stats, id), paths: new Set() }; + entry = { id, key, fileid: fileidOf(stats, id), paths: new Set(), pins: 0 }; this.#byId.set(entry.id, entry); } else { entry.fileid = fileidOf(stats, entry.id); @@ -405,11 +474,19 @@ export class FileHandleTable { remapSubtree(this.#byPath, from, to, this.#remapOps); } - /** Forget everything but the root. Teardown, and tests. */ + /** + * Forget everything but the root. Teardown, and tests. + * + * Pins go with the entries that carried them, the root's included: this is + * the whole table being thrown away, so there is no claim left to honour. A + * holder that unpins afterwards is a no-op — `unpin` neither resurrects an id + * nor goes below zero. + */ clear(): void { this.#byId.clear(); this.#byPath.clear(); this.#byKey.clear(); + this.root.pins = 0; this.root.paths.clear(); this.root.paths.add("/"); this.#byId.set(ROOT_HANDLE_ID, this.root); @@ -506,8 +583,10 @@ export class FileHandleTable { } while (this.#byId.size > this.#maxHandles) { const victim = this.#lruVictim(keep); - // Root plus the entry just bound, and a cap of one or two asking for - // less than that. Honour what can be honoured and stop. + // Nothing left that may go: the root, the entry just bound, and every + // entry pinned by live NFSv4.1 state. The cap is soft — honour what can + // be honoured and stop, rather than take an entry somebody's share + // reservation is keyed to. See the module docs. if (victim === undefined) { return; } @@ -516,16 +595,19 @@ export class FileHandleTable { } /** - * The oldest entry that may go — anything but the root and the entry the - * caller is in the middle of binding. + * The oldest entry that may go — anything but the root, the entry the caller + * is in the middle of binding, and the pinned. * - * The scan is not the linear cost it looks like: the root is inserted first - * and never touched, so it sits at the old end for the life of the table and - * this walks past it and one more entry at most. + * The scan is not the linear cost it looks like *for the first two*: the root + * is inserted first and never touched, so it sits at the old end for the life + * of the table and this walks past it and one more entry at most. Pins can + * lengthen it — a run of pinned entries at the old end is walked every time — + * and that is bounded by the opens a client holds, which is the same number + * `v4/state.ts` already caps per file. */ #lruVictim(keep: HandleEntry): HandleEntry | undefined { for (const entry of this.#byId.values()) { - if (entry.id !== ROOT_HANDLE_ID && entry !== keep) { + if (entry.id !== ROOT_HANDLE_ID && entry !== keep && entry.pins === 0) { return entry; } } diff --git a/src/nfs/util.ts b/src/nfs/util.ts index f9ab417..01df21b 100644 --- a/src/nfs/util.ts +++ b/src/nfs/util.ts @@ -141,10 +141,13 @@ export interface NfsSessionOptions { * first past it. Default: no cap, which is what this server has always done. * * An eviction costs the client holding that handle an `ESTALE` and a - * re-lookup — and an NFSv4.1 client that had the file **open** its share - * reservation, since the re-lookup is a different entry id and so a different - * `FileState`. A cap below the largest READDIRPLUS page a client asks for is - * also self-defeating. Read `./handles.ts` before setting it. + * re-lookup. An entry an NFSv4.1 client has **open** is never taken — its + * share reservations and byte-range locks are keyed to that entry's id — so + * this is a **soft cap**: a client holding more files open than the cap + * allows entries pushes the table past it, and the table will not evict its + * way back down until those opens close. A cap below the largest READDIRPLUS + * page a client asks for is also self-defeating. Read `./handles.ts` before + * setting it. */ maxHandles?: number; /** Largest `READ` answered. */ diff --git a/src/nfs/v4/session.ts b/src/nfs/v4/session.ts index 75971d9..f8d9386 100644 --- a/src/nfs/v4/session.ts +++ b/src/nfs/v4/session.ts @@ -37,6 +37,14 @@ * opens and closes its own handle per request, which is what `../v3/session.ts` * does for every request it answers. * + * That the `fileKey` is a handle-table entry id has one consequence running the + * other way: an entry the table evicted would come back under a *new* id, and + * the same file would then carry two `FileState`s that cannot see each other — + * a share reservation quietly stopping being enforced. So this file **pins** an + * entry for exactly as long as `./state.ts` holds state for its key + * (`onFileRetained`/`onFileReleased` → `#pinFile`/`#unpinFile`), and the cap in + * `../handles.ts` is soft as a result. + * * ## The current stateid * * A COMPOUND's cursor carries a stateid beside the two filehandles @@ -782,6 +790,17 @@ function stateKey(other: Uint8Array): string { return hexOf(other); } +/** + * The handle entry id a `fileKey` names — the inverse of `#fileKey`. + * + * `undefined` for anything that is not one of those keys, which is the reclaim + * arm's `""` and nothing else. Written as a test rather than a `try`, because + * `BigInt("0x")` throwing is not the reason this returns `undefined`. + */ +function entryIdOf(fileKey: string): bigint | undefined { + return /^[\da-f]+$/.test(fileKey) ? BigInt(`0x${fileKey}`) : undefined; +} + /** {@link allowedAccess}'s answer as `ACCESS4_*` bits (RFC 8881 §18.1.1). */ function accessBits4(rights: AccessRights): number { return ( @@ -914,6 +933,20 @@ export class Nfs4Session { * encoded. */ readonly #released: string[] = []; + /** + * The `fileKey`s whose handle entry this session is holding pinned. + * + * `../handles.ts` evicts least-recently-used entries under a cap, and this + * file keys every open and every byte-range lock by *entry id* — so an + * evicted entry would be looked up again under a new id and the same file + * would end up with two `FileState`s that cannot see each other, silently + * dropping a share reservation. `./state.ts` reports the exact span in which + * a key means something (`onFileRetained`/`onFileReleased`), and this set is + * the record of the pins taken for it: it makes the pins idempotent per key, + * and it is what `destroy()` hands back so a session teardown cannot leave an + * entry pinned for the rest of the table's life. + */ + readonly #pinnedFiles = new Set(); /** The verifiers of OPEN with `EXCLUSIVE4`/`EXCLUSIVE4_1` — see `../util.ts`. */ readonly #exclusives: ExclusiveCreates; #destroyed = false; @@ -943,6 +976,8 @@ export class Nfs4Session { this.state = new Nfs4State({ ...knobs, onOpenReleased: (other) => this.#released.push(stateKey(other)), + onFileRetained: (fileKey) => this.#pinFile(fileKey), + onFileReleased: (fileKey) => this.#unpinFile(fileKey), }); this.#maxOperations = Math.max(1, options.nfs4?.maxOperations ?? DEFAULT_MAX_OPERATIONS); } @@ -997,8 +1032,14 @@ export class Nfs4Session { } /** - * Drop every cached listing, close every driver handle still held open, and - * stop. Idempotent. File *handles* (the wire kind) are the router's. + * Drop every cached listing, release every pinned handle entry, close every + * driver handle still held open, and stop. Idempotent. File *handles* (the + * wire kind) are the router's. + * + * The pins have to go here precisely *because* the table is the router's: + * this session's state dies with it, but the entries it was holding against + * eviction do not, and a pin nobody can ever release again is an entry the + * cap can never reclaim. * * The driver handles are this session's own: an open state that is still * open when the server stops has nobody left to CLOSE it, and a driver whose @@ -1011,6 +1052,12 @@ export class Nfs4Session { this.#snapshots.clear(); this.#exclusives.clear(); this.#maxOpsBySession.clear(); + // Not through `#unpinFile`: the whole set goes at once here, so it is + // emptied in one step rather than deleted from under its own iterator. + for (const fileKey of this.#pinnedFiles) { + this.handles.unpin(entryIdOf(fileKey)!); + } + this.#pinnedFiles.clear(); const open = [...this.#openHandles.keys(), ...this.#released]; this.#released.length = 0; for (const key of open) { @@ -2825,6 +2872,36 @@ export class Nfs4Session { return entry.id.toString(16); } + /** + * Hold the handle entry a `fileKey` names against LRU eviction, for as long + * as `./state.ts` has state on that file. + * + * The key is hex of an entry id and nothing else, so it inverts — which is + * the only reason this can be driven by the state table, which knows nothing + * about handles. A key that is not one is ignored rather than refused: the + * reclaim arm of {@link Nfs4Session.#open} passes `""` deliberately, having + * no file to name, and a `FileState` is never created for it. + * + * Idempotent per key, and paired with {@link Nfs4Session.#unpinFile} through + * `#pinnedFiles`, so an unpin can only ever release a pin this session took. + */ + #pinFile(fileKey: string): void { + const id = entryIdOf(fileKey); + if (id === undefined || this.#pinnedFiles.has(fileKey)) { + return; + } + this.#pinnedFiles.add(fileKey); + this.handles.pin(id); + } + + /** Release {@link Nfs4Session.#pinFile}'s pin, if this session took one. */ + #unpinFile(fileKey: string): void { + if (!this.#pinnedFiles.delete(fileKey)) { + return; + } + this.handles.unpin(entryIdOf(fileKey)!); + } + /** The current filehandle as the pair a stateful operation needs. */ #target(cursor: Cursor): { path: string; fileKey: string; entry: HandleEntry } { const entry = this.handles.decode(this.#requireCurrent(cursor)); diff --git a/src/nfs/v4/state.ts b/src/nfs/v4/state.ts index 9e48394..03fe692 100644 --- a/src/nfs/v4/state.ts +++ b/src/nfs/v4/state.ts @@ -823,6 +823,27 @@ export interface Nfs4StateOptions { * middle of a table walk. A caller with asynchronous work to do queues it. */ onOpenReleased?: ((other: Uint8Array) => void) | undefined; + /** + * This file now has state on it, where a moment ago it had none. + * + * The pair with {@link Nfs4StateOptions.onFileReleased} brackets the one + * `Map` of `FileState`s — the thing that makes a `fileKey` mean anything to + * this table — so the two are balanced by construction: one call each, from + * the single site that adds the entry and the single site that removes it, + * never nested and never repeated for a key already held. + * + * It exists because the key is the *caller's*, and only the caller knows what + * keeping it alive costs. `../handles.ts` evicts least-recently-used entries + * under a cap, and an evicted entry would come back with a new id and so a + * second, unrelated `FileState` for the same file — a share reservation + * silently stopping being enforced. The session answers this by pinning the + * handle entry for exactly the span between these two calls. + * + * Synchronous and must not throw, for the same reason `onOpenReleased` is. + */ + onFileRetained?: ((fileKey: string) => void) | undefined; + /** The last open and the last granted range on this file are gone. */ + onFileReleased?: ((fileKey: string) => void) | undefined; } const DEFAULT_LEASE_SECONDS = 90; @@ -920,6 +941,8 @@ export class Nfs4State { readonly #maxLocksPerFile: number; readonly #requireReclaimComplete: boolean; readonly #onOpenReleased: ((other: Uint8Array) => void) | undefined; + readonly #onFileRetained: ((fileKey: string) => void) | undefined; + readonly #onFileReleased: ((fileKey: string) => void) | undefined; /** `co_ownerid` → the confirmed and unconfirmed records that ownerid has (§18.35.4 case 5). */ readonly #byOwner = new Map(); @@ -948,6 +971,8 @@ export class Nfs4State { this.#maxLocksPerFile = Math.max(1, options.maxLocksPerFile ?? 1024); this.#requireReclaimComplete = options.requireReclaimComplete ?? true; this.#onOpenReleased = options.onOpenReleased; + this.#onFileRetained = options.onFileRetained; + this.#onFileReleased = options.onFileReleased; } /** The lease length, in seconds — the `lease_time` attribute the session reports. */ @@ -1723,11 +1748,21 @@ export class Nfs4State { return open?.kind === "open" ? open : undefined; } + /** + * The `FileState` for a key, created if this is the file's first. + * + * The one place `#files` grows, which is what lets + * {@link Nfs4StateOptions.onFileRetained} be a balanced pair with the one + * place it shrinks. **Call it only once the state is certain to be + * recorded** — a refused LOCK that had already created an empty `FileState` + * would leave the caller holding a pin for a file with nothing on it. + */ #fileOf(fileKey: string): FileState { let file = this.#files.get(fileKey); if (file === undefined) { file = { opens: new Set(), locks: [] }; this.#files.set(fileKey, file); + this.#onFileRetained?.(fileKey); } return file; } @@ -1736,6 +1771,7 @@ export class Nfs4State { const file = this.#files.get(fileKey); if (file !== undefined && file.opens.size === 0 && file.locks.length === 0) { this.#files.delete(fileKey); + this.#onFileReleased?.(fileKey); } } @@ -2031,13 +2067,18 @@ export class Nfs4State { const ownerKey = lockState?.ownerKey ?? `${request.clientid}:${keyOf(request.lockOwner!)}`; this.#revokeExpiredHolders(request.fileKey, request.clientid); - const file = this.#fileOf(request.fileKey); - for (const held of file.locks) { - if (overlaps(held, range.start, range.end) && conflicts(held, ownerKey, type)) { - return { status: NFS4ERR_DENIED, denied: deniedOf(held) }; + // Read before the refusals and only created after them: `#fileOf` is what + // tells the caller this file now has state worth pinning a handle for, and + // a LOCK that ends in DENIED or DELAY has added none. (In practice the + // open the stateid above resolved through already made one — this is the + // rule holding rather than a case being handled.) + const held = this.#files.get(request.fileKey)?.locks ?? []; + for (const lock of held) { + if (overlaps(lock, range.start, range.end) && conflicts(lock, ownerKey, type)) { + return { status: NFS4ERR_DENIED, denied: deniedOf(lock) }; } } - if (file.locks.length >= this.#maxLocksPerFile) { + if (held.length >= this.#maxLocksPerFile) { return { status: NFS4ERR_DELAY }; } if (lockState === undefined) { @@ -2045,6 +2086,7 @@ export class Nfs4State { } else { lockState.seqid = bumpSeqid(lockState.seqid); } + const file = this.#fileOf(request.fileKey); file.locks = replaceRange(file.locks, { ownerKey, openKey: lockState.openKey, @@ -2115,8 +2157,13 @@ export class Nfs4State { return { status: resolved.status }; } const lockState = resolved.state!; - const file = this.#fileOf(request.fileKey); - file.locks = subtractRange(file.locks, lockState.ownerKey, range.start, range.end); + // A lock stateid outlives the ranges it stood for — LOCKU does not drop it + // — so a second LOCKU can arrive with the file's state already forgotten. + // Nothing to subtract from then, and nothing to create either. + const file = this.#files.get(request.fileKey); + if (file !== undefined) { + file.locks = subtractRange(file.locks, lockState.ownerKey, range.start, range.end); + } lockState.seqid = bumpSeqid(lockState.seqid); this.#forgetFileIfEmpty(request.fileKey); return { status: NFS4_OK, stateid: { seqid: lockState.seqid, other: copyOf(lockState.other) } }; diff --git a/test/nfs/handles.test.ts b/test/nfs/handles.test.ts index b4e5f82..a894e41 100644 --- a/test/nfs/handles.test.ts +++ b/test/nfs/handles.test.ts @@ -357,6 +357,117 @@ describe("the handle-table bound", () => { expect(table.pathCount).toBe(4); }); + it("never takes a pinned entry, however long it has been idle", () => { + const table = new FileHandleTable({ maxHandles: 3 }); + const pinned = table.bind("/open", stats(2)); + const held = table.encode(pinned); + table.pin(pinned.id); + // Never touched again: under the LRU alone this is the first thing to go, + // every single time. + for (let index = 0; index < 50; index++) { + table.bind(`/tmp${index}`, stats(100 + index)); + } + expect(table.get(pinned.id)).toBe(pinned); + expect(table.resolve(held)).toBe("/open"); + expect(table.size).toBe(3); + }); + + it("makes an unpinned entry the first victim again", () => { + const table = new FileHandleTable({ maxHandles: 3 }); + const entry = table.bind("/open", stats(2)); + table.pin(entry.id); + table.bind("/a", stats(3)); + table.bind("/b", stats(4)); + expect(table.get(entry.id)).toBe(entry); + + table.unpin(entry.id); + // Nothing re-evicts on unpin — the cap is only enforced by a `bind` — so + // the next one is what takes it, and it takes the oldest, which is this. + table.bind("/c", stats(5)); + expect(table.get(entry.id)).toBeUndefined(); + expect(table.size).toBe(3); + }); + + it("counts pins, so one holder cannot release another's", () => { + const table = new FileHandleTable({ maxHandles: 2 }); + const entry = table.bind("/open", stats(2)); + table.pin(entry.id); + table.pin(entry.id); + table.unpin(entry.id); + expect(entry.pins).toBe(1); + table.bind("/a", stats(3)); + expect(table.get(entry.id)).toBe(entry); + + table.unpin(entry.id); + expect(entry.pins).toBe(0); + table.bind("/b", stats(4)); + expect(table.get(entry.id)).toBeUndefined(); + }); + + it("never counts a pin below zero, so a lapsed holder cannot go negative", () => { + const table = new FileHandleTable({ maxHandles: 2 }); + const entry = table.bind("/open", stats(2)); + table.unpin(entry.id); + expect(entry.pins).toBe(0); + // And an id the table no longer has is neither pinnable nor a throw: a + // holder whose file was removed unpins into thin air. + table.unbind("/open"); + expect(table.get(entry.id)).toBeUndefined(); + table.pin(entry.id); + table.unpin(entry.id); + expect(table.size).toBe(1); + }); + + it("exceeds the cap rather than evicting a pinned entry", () => { + const table = new FileHandleTable({ maxHandles: 3 }); + const pinned: bigint[] = []; + for (let index = 0; index < 10; index++) { + const entry = table.bind(`/open${index}`, stats(100 + index)); + table.pin(entry.id); + pinned.push(entry.id); + } + // Root plus ten, at a cap of three: the soft cap, which is the accepted + // trade — a broken share reservation is worse than a table over its bound. + expect(table.size).toBe(11); + for (const id of pinned) { + expect(table.get(id)).toBeDefined(); + } + // And it does not evict its way back down when the pins go: it comes down + // as the next binds find victims again. + for (const id of pinned) { + table.unpin(id); + } + expect(table.size).toBe(11); + table.bind("/next", stats(200)); + expect(table.size).toBe(3); + }); + + it("still drops a pinned entry whose last path is gone", () => { + const table = new FileHandleTable({ maxHandles: 4 }); + const entry = table.bind("/doomed", stats(2)); + table.pin(entry.id); + // A pin is about the LRU and nothing else: a removed file's handles are + // ESTALE whether or not somebody has it open, and keeping the entry alive + // for the pin would be a leak with a `rm` trigger. + table.unbind("/doomed"); + expect(table.get(entry.id)).toBeUndefined(); + expect(table.size).toBe(1); + }); + + it("drops the pins with the entries on clear()", () => { + const table = new FileHandleTable({ maxHandles: 3 }); + const entry = table.bind("/open", stats(2)); + table.pin(entry.id); + table.pin(table.root.id); + table.clear(); + expect(table.root.pins).toBe(0); + expect(table.get(entry.id)).toBeUndefined(); + for (let index = 0; index < 10; index++) { + table.bind(`/f${index}`, stats(100 + index)); + } + expect(table.size).toBe(3); + }); + it("restamps a renamed subtree, and moves it whole", () => { const table = new FileHandleTable({ maxHandles: 5 }); const stale = table.bind("/old", stats(2)); diff --git a/test/nfs/v4/session.test.ts b/test/nfs/v4/session.test.ts index 974162d..64eb8f2 100644 --- a/test/nfs/v4/session.test.ts +++ b/test/nfs/v4/session.test.ts @@ -3071,6 +3071,170 @@ describe("byte-range locks", () => { }); }); +// --------------------------------------------------------------------------- +// a capped handle table under live v4.1 state +// --------------------------------------------------------------------------- + +/** + * What a handle eviction must **not** cost a client that has the file open. + * + * `../../src/nfs/v4/session.ts` keys open and lock state by handle table entry + * id, so an entry evicted under `maxHandles` and looked up again would come + * back with a *new* id — and one file would carry two `FileState`s that cannot + * see each other. That is not a round trip, it is a share reservation the + * server promised and then did not keep, and it was reproduced exactly as + * written here before the fix: the second OPEN was granted. + * + * The table is the router's, so these go through `NfsSession` — a directly + * constructed `Nfs4Session` builds its own table and ignores `maxHandles`. + */ +describe("a capped handle table under live v4.1 state", () => { + const TO_FILE: Op[] = [...TO_DIR, { op: OP_LOOKUP, args: { objname: "file" } }]; + + /** `/dir/file`, plus enough decoys to push anything unpinned out of a small cap. */ + async function crowded(): Promise { + const driver = await populated(); + for (let index = 0; index < 12; index++) { + const decoy = await driver.open(`/dir/decoy${index}`, "w"); + await decoy.close(); + } + return driver; + } + + /** Look each decoy up in turn: twelve binds into a table that holds four. */ + async function walk(client: Client): Promise { + for (let index = 0; index < 12; index++) { + const reply = await client.run([ + ...TO_DIR, + { op: OP_LOOKUP, args: { objname: `decoy${index}` } }, + ]); + expect(reply.compound.status).toBe(NFS4_OK); + } + } + + it("keeps a share reservation across the eviction pressure of another client", async () => { + const session = new NfsSession(await crowded(), { maxHandles: 4 }); + const holder = await ready(session.v4, "client-a"); + const rival = await ready(session.v4, "client-b"); + + const held = await holder.run([ + ...TO_DIR, + OPEN({ access: OPEN4_SHARE_ACCESS_READ, deny: OPEN4_SHARE_DENY_WRITE }), + ]); + expect(held.compound.status).toBe(NFS4_OK); + const entry = session.handles.at("/dir/file")!; + expect(entry.pins).toBe(1); + + await walk(rival); + // The pin held: same entry, same id, so the same `fileKey` and the same + // `FileState`. Twelve lookups into a table of four, and what is left is the + // root, `/dir`, the pinned file and the last decoy — the twelve decoys took + // each other's places instead. + expect(session.handles.at("/dir/file")).toBe(entry); + expect(session.handles.size).toBe(4); + + const clash = await rival.run([...TO_DIR, OPEN({ access: OPEN4_SHARE_ACCESS_WRITE })]); + expect(clash.compound.status).toBe(NFS4ERR_SHARE_DENIED); + }); + + it("keeps a byte-range lock across the same pressure", async () => { + const session = new NfsSession(await crowded(), { maxHandles: 4 }); + const holder = await ready(session.v4, "client-a"); + const rival = await ready(session.v4, "client-b"); + + const open = await opened(holder, { access: OPEN4_SHARE_ACCESS_BOTH }); + const locked = await holder.run([ + ...TO_FILE, + { + op: OP_LOCK, + args: { + locktype: WRITE_LT, + reclaim: false, + offset: 0n, + length: 10n, + locker: { + newLockOwner: true, + openOwner: { + openSeqid: 1, + openStateid: open, + lockSeqid: 1, + lockOwner: { clientid: 0n, owner: Uint8Array.from([5]) }, + }, + }, + }, + }, + ]); + expect(locked.compound.status).toBe(NFS4_OK); + + await walk(rival); + + // The granted range is `state.locksOf(fileKey)`, so it follows the entry id + // exactly as the reservation above does. + const test = await rival.run([ + ...TO_FILE, + { + op: OP_LOCKT, + args: { + locktype: WRITE_LT, + offset: 4n, + length: 2n, + owner: { clientid: 0n, owner: Uint8Array.from([6]) }, + }, + }, + ]); + expect(test.compound.status).toBe(NFS4ERR_DENIED); + expect(resFor(test, OP_LOCKT).denied!.owner.clientid).toBe(holder.clientid); + }); + + it("lets the entry go once the state does, and leaves no pin behind", async () => { + const session = new NfsSession(await crowded(), { maxHandles: 4 }); + const client = await ready(session.v4, "client-a"); + const stateid = await opened(client, { access: OPEN4_SHARE_ACCESS_READ }); + const entry = session.handles.at("/dir/file")!; + expect(entry.pins).toBe(1); + + const closed = await client.run([ + ...TO_FILE, + { op: OP_CLOSE, args: { seqid: 1, openStateid: stateid } }, + ]); + expect(closed.compound.status).toBe(NFS4_OK); + // A leaked pin is a permanently unevictable entry, so the count is asserted + // rather than inferred from the eviction that follows. + expect(entry.pins).toBe(0); + + await walk(client); + expect(session.handles.get(entry.id)).toBeUndefined(); + expect(session.handles.size).toBe(4); + }); + + it("releases the pins a teardown finds still held", async () => { + const session = new NfsSession(await crowded(), { maxHandles: 4 }); + const client = await ready(session.v4, "client-a"); + // Two files open at once, and no CLOSE for either: `destroy()` is the only + // thing left that can give the entries back. + await opened(client, { access: OPEN4_SHARE_ACCESS_READ }); + await opened(client, { name: "decoy0", owner: 2, access: OPEN4_SHARE_ACCESS_READ }); + const file = session.handles.at("/dir/file")!; + const decoy = session.handles.at("/dir/decoy0")!; + expect([file.pins, decoy.pins]).toEqual([1, 1]); + + // The v4 session alone, not the router: `NfsSession.destroy()` ends in + // `handles.clear()`, which would drop the entries and make any answer look + // right. What has to be proved is that the session gives its pins *back*, + // into a table that outlives it. + await session.v4.destroy(); + expect([file.pins, decoy.pins]).toEqual([0, 0]); + // The table outlives the session that pinned into it — it is the router's — + // so it must be evictable again for whatever serves next. + for (let index = 0; index < 12; index++) { + const path = `/dir/decoy${index}`; + session.handles.bind(path, await session.driver.lstat(path)); + } + expect(session.handles.size).toBe(4); + expect(session.handles.get(file.id)).toBeUndefined(); + }); +}); + describe("the current stateid", () => { const TO_FILE: Op[] = [...TO_DIR, { op: OP_LOOKUP, args: { objname: "file" } }];