ZLFS: concurrency-safe commit

Lift the documented single-threaded-write-path assumption.

Every reader and writer of the inode map now holds zm_lock -- the map
pointer is replaced when zlfs_imap_grow expands it, so an unlocked
reader could dereference freed memory: zlfs_read_dinode snapshots the
LBA under the lock, the commit's map-block copy loop and commit_node's
stores take it, the cleaner's pass 2 snapshots entry by entry, and the
remove/rmdir/rename zero-stores are covered (statfs and ialloc already
were).

The commit itself becomes all-or-nothing under held locks: dirty nodes
are snapshotted under one zm_lock hold (count and fill under the same
hold, with a headroom retry, so a node prepended to the list while
malloc slept cannot displace an older one -- say the fsync caller's --
out of the snapshot), each is referenced with an LK_NOWAIT vget (a
dying vnode is mid-reclaim and skipped), and then ALL their vnode
locks are taken with trylocks and held until the dirty flags clear.
Holding the locks across the segment write closes the overlay
use-after-free and the lost-update window (a write slipping between a
node's commit and the bulk flag clear), and keeps every checkpoint a
consistent all-or-nothing set -- no dangling dirents from a
half-committed create.  Blocking on a vnode lock while holding
zm_wlock would deadlock against an fsync that holds the vnode and
waits for the commit lock, so contention releases everything and
retries after a short sleep (ten attempts, then EBUSY); the holder
always finishes first, so progress is guaranteed.  The fsync caller's
own node recurses through its RWL_DUPOK rrwlock, whose exit
decrements the recursion rather than releasing the caller's hold.

The cleaner's pass 3 gets the same single-hold snapshot treatment: its
count-then-fill raced list growth, and a displaced unlinked-but-open
inode -- the very case pass 3 protects -- could have its zone reset.

doc/zlfs-design.md section 7 and the functional_testing.md gap table
updated; two long-standing latent-UAF rows close.

Author: xsub

doc/zlfs-design.md

@@ -144,8 +144,14 @@ flushed before the superblock was appended.
 - **Locks**: `zm_wlock` serialises commits; `zm_lock` guards the node
   list and inode-map swaps; per-inode `rrwlock` vnode locks follow the
   ufs contracts (documented per-VOP in `zlfs_vnops.c`).  The write
-  path is single-threaded under `KERNEL_LOCK` by documented
-  assumption; the concurrency-safe-commit phase lifts this.
+  path no longer assumes single-threading: every `zm_imap`
+  reader/writer holds `zm_lock`; a commit snapshots dirty nodes under
+  `zm_lock` with vnode references and commits each under its vnode
+  lock — all locks taken up front with trylocks and held until the
+  dirty flags clear, so the checkpoint is an all-or-nothing set and no
+  write slips between a node's commit and its flag clear; contention
+  releases everything and retries after the holder finishes (avoiding
+  the vnode->wlock inversion).
 
 ## 8. Why these choices
 

functional_testing.md

@@ -51,8 +51,8 @@ size 4096; superblock (SB) zones 0-1, 126 data zones.
 | `statfs` accuracy | IMPROVED (sbcap phase): `f_bfree` = allocatable now (free zones + log-head remainder), `f_files`/`f_ffree` from the real inode map; dead-but-unreclaimed zones still count as used until the cleaner runs (honest for an LFS) | closed enough for bring-up | per-zone byte accounting with the copying cleaner |
 | `zst_live_bytes` semantics | only tested against 0 in the reset loop; not a true byte count | none today; trap for future code | rename or fix when the copying cleaner needs real counts |
 | Inode ceiling (was: single-block map, 512) | format v2 raises the cap to `ZLFS_CKPT_NIMAP * epb` (~256000 at a 4 KB block: ~500 map-block LBAs fit in the checkpoint block) | wide trees fine now; the ~256k cap is structural until the map gets its own indirection | none planned; revisit only if a workload needs more |
-| Commit vs concurrent write on the same file | `zlfs_commit_file_blocks` adoption frees the overlay (`zlfs_dblk_free`) without the vnode lock while a concurrent `write` could be filling a buffer — the documented single-threaded-commit assumption now shields a potential use-after-free, not just a stale flag | none under KERNEL_LOCK today | serialise commit against vnode ops in the concurrency-safe-commit phase |
-| `zlfs_imap_grow` swaps `zm_imap` under `zm_lock` only | readers (`read_dinode`, commit, cleaner pass 2, remove/rmdir/rename) index the map without `zm_lock`; safe today because all accesses are single-expression under the kernel lock and grow has no sleep between copy and install — a latent use-after-free the moment these paths run MP-unlocked.  `statfs` now takes `zm_lock` for its scan (sbcap phase) | none under the current single-threaded assumption | take `zm_lock` (or `zm_wlock`) in the remaining map readers as part of the concurrency-safe-commit phase |
+| Commit vs concurrent write on the same file | CLOSED (concurrency phase): commit snapshots dirty nodes under `zm_lock` with vnode references (`LK_NOWAIT` vget skips dying vnodes) and takes ALL their `rrw` locks up front (trylock; contention = release all + retry, holder finishes first) and holds them until the dirty flags clear — closes the overlay UAF, the commit-vs-write lost-update window, and keeps the checkpoint an all-or-nothing set | closed | — |
+| `zlfs_imap_grow` swaps `zm_imap` under `zm_lock` only | readers (`read_dinode`, commit, cleaner pass 2, remove/rmdir/rename) index the map without `zm_lock`; CLOSED (concurrency phase): every reader/writer of `zm_imap` now holds `zm_lock` — `read_dinode`, the commit's map snapshot and `commit_node` stores, cleaner pass 2 (per-entry snapshot), remove/rmdir/rename zero-stores; `statfs`/`ialloc` already did | closed | — |
 | Conventional (non-write-pointer) superblock zones would break the write path | discovery supports them (forward scan), but `zlfs_zones_load` copies `DK_ZONE_WP_INVALID` (~0) write pointers verbatim, so `zm_sb_lba` starts at ~0 and the first commit's superblock write targets a nonsense LBA (pre-existing; flagged by the sbcap review) | RW mount of a device with conventional zones 0-1 fails at the first commit; the QEMU ZNS target has no conventional zones | validate the active SB zone's write pointer at mount; proper conventional-zone append tracking when such a target matters |
 
 ## 4. Later (roadmap order)

sys/zlfs/zlfs_alloc.c

@@ -294,11 +294,20 @@ zlfs_clean(struct zlfs_mount *zmp)
 	}
 pass1_done:
 
-	/* Pass 2: the in-core live set, through zm_imap. */
-	for (ino = 0; ino < zmp->zm_ninodes; ino++) {
-		if (zmp->zm_imap[ino] == 0)
+	/* Pass 2: the in-core live set, through zm_imap.  Snapshot each
+	 * entry under zm_lock (the map may be grown/replaced) and read
+	 * the inode outside it. */
+	for (ino = 0;; ino++) {
+		rw_enter_read(&zmp->zm_lock);
+		lba = (ino < zmp->zm_ninodes) ? zmp->zm_imap[ino] : 0;
+		if (ino >= zmp->zm_ninodes) {
+			rw_exit_read(&zmp->zm_lock);
+			break;
+		}
+		rw_exit_read(&zmp->zm_lock);
+		if (lba == 0)
 			continue;
-		if (zlfs_gc_mark_inode(zmp, zmp->zm_imap[ino]) != 0)
+		if (zlfs_gc_mark_inode(zmp, lba) != 0)
 			return;
 	}
 
@@ -307,11 +316,55 @@ zlfs_clean(struct zlfs_mount *zmp)
 	 * unlinked-but-open file is in neither map once its removal has
 	 * been committed, yet the open vnode still reads its old blocks
 	 * from disk, so mark everything each in-core inode points at.
+	 * The marking sleeps (bread), so snapshot the inodes under
+	 * zm_lock first -- the list may change while we sleep.
 	 */
-	LIST_FOREACH(znp, &zmp->zm_nodes, zn_entry) {
-		if (zlfs_gc_mark_blocks(zmp, &znp->zn_dinode) != 0)
-			return;
+	{
+		struct zlfs_inode *snap = NULL;
+		u_int64_t nsnap = 0, cap = 0, n, s;
+
+		/*
+		 * Count and fill under one zm_lock hold, with a headroom
+		 * retry when the list grew while we slept in malloc --
+		 * a node prepended during that sleep must not displace an
+		 * older one (the unlinked-but-open case pass 3 exists for)
+		 * out of a capped snapshot.
+		 */
+		for (;;) {
+			rw_enter_read(&zmp->zm_lock);
+			n = 0;
+			LIST_FOREACH(znp, &zmp->zm_nodes, zn_entry)
+				n++;
+			if (n == 0) {
+				rw_exit_read(&zmp->zm_lock);
+				free(snap, M_TEMP, cap * sizeof(*snap));
+				goto pass3_done;
+			}
+			if (snap == NULL || n > cap) {
+				rw_exit_read(&zmp->zm_lock);
+				free(snap, M_TEMP, cap * sizeof(*snap));
+				cap = n + 8;
+				snap = mallocarray(cap, sizeof(*snap),
+				    M_TEMP, M_WAITOK);
+				continue;
+			}
+			LIST_FOREACH(znp, &zmp->zm_nodes, zn_entry) {
+				if (nsnap >= cap)
+					break;
+				snap[nsnap++] = znp->zn_dinode;
+			}
+			rw_exit_read(&zmp->zm_lock);
+			break;
+		}
+		for (s = 0; s < nsnap; s++) {
+			if (zlfs_gc_mark_blocks(zmp, &snap[s]) != 0) {
+				free(snap, M_TEMP, cap * sizeof(*snap));
+				return;
+			}
+		}
+		free(snap, M_TEMP, cap * sizeof(*snap));
 	}
+pass3_done:
 
 	/*
 	 * Reset any written-but-dead data zone.  The log head is spared

sys/zlfs/zlfs_subr.c

@@ -499,9 +499,10 @@ zlfs_read_dinode(struct zlfs_mount *zmp, u_int64_t ino, struct zlfs_inode *zi)
 	u_int64_t lba;
 	int error;
 
-	if (ino >= zmp->zm_ninodes)
-		return ENOENT;
-	lba = zmp->zm_imap[ino];
+	/* zm_lock: zlfs_imap_grow may free and replace the map. */
+	rw_enter_read(&zmp->zm_lock);
+	lba = (ino < zmp->zm_ninodes) ? zmp->zm_imap[ino] : 0;
+	rw_exit_read(&zmp->zm_lock);
 	if (lba == 0)
 		return ENOENT;
 

sys/zlfs/zlfs_vnops.c

@@ -795,8 +795,11 @@ zlfs_remove(void *v)
 
 	if (np->zn_dinode.zi_nlink > 0)
 		np->zn_dinode.zi_nlink--;
-	if (np->zn_dinode.zi_nlink == 0)
+	if (np->zn_dinode.zi_nlink == 0) {
+		rw_enter_write(&zmp->zm_lock);
 		zmp->zm_imap[np->zn_ino] = 0;
+		rw_exit_write(&zmp->zm_lock);
+	}
 	zlfs_node_dirty(np);
 	return 0;
 }
@@ -837,7 +840,9 @@ zlfs_rmdir(void *v)
 	zlfs_node_dirty(dnp);
 	np->zn_dinode.zi_nlink = 0;
 	np->zn_datalen = 0;
+	rw_enter_write(&zmp->zm_lock);
 	zmp->zm_imap[np->zn_ino] = 0;
+	rw_exit_write(&zmp->zm_lock);
 	zlfs_node_dirty(np);
 
 out:
@@ -963,7 +968,9 @@ zlfs_rename(void *v)
 			tdnp->zn_dinode.zi_nlink--;	/* lost target's ".." */
 		tnp->zn_dinode.zi_nlink = 0;
 		tnp->zn_datalen = 0;
+		rw_enter_write(&zmp->zm_lock);
 		zmp->zm_imap[tnp->zn_ino] = 0;
+		rw_exit_write(&zmp->zm_lock);
 		zlfs_node_dirty(tnp);
 	}
 

sys/zlfs/zlfs_write.c

@@ -17,6 +17,7 @@
 #include <sys/systm.h>
 #include <sys/buf.h>
 #include <sys/endian.h>
+#include <sys/lock.h>
 #include <sys/malloc.h>
 #include <sys/mount.h>
 #include <sys/pool.h>
@@ -518,7 +519,9 @@ zlfs_commit_node(struct zlfs_mount *zmp, struct zlfs_node *znp)
 	 * written; its blocks become garbage for a later cleaner.
 	 */
 	if (znp->zn_dinode.zi_nlink == 0) {
+		rw_enter_write(&zmp->zm_lock);
 		zmp->zm_imap[znp->zn_ino] = 0;
+		rw_exit_write(&zmp->zm_lock);
 		return 0;
 	}
 
@@ -609,9 +612,11 @@ zlfs_commit_node(struct zlfs_mount *zmp, struct zlfs_node *znp)
 	if (error != 0)
 		return error;
 
+	rw_enter_write(&zmp->zm_lock);
 	zmp->zm_imap[znp->zn_ino] = lba;
 	if (znp->zn_ino + 1 > zmp->zm_ninodes)
 		zmp->zm_ninodes = znp->zn_ino + 1;
+	rw_exit_write(&zmp->zm_lock);
 	return 0;
 }
 
@@ -693,10 +698,14 @@ zlfs_commit_super(struct zlfs_mount *zmp, u_int8_t *blk, u_int64_t generation,
  * Commit all dirty inodes as a new log segment and checkpoint.
  *
  * Serialised against other writers by zm_wlock.  The node list is
- * walked without zm_lock (taking it here would invert the vnode-lock
- * ordering used by zlfs_vget); this bring-up therefore assumes commits
- * do not run concurrently with vnode creation or reclaim.  Proper
- * concurrent-safe commit is future work.
+ * snapshotted under one zm_lock hold with vnode references (LK_NOWAIT
+ * vget skips dying vnodes), then ALL their vnode locks are taken up
+ * front with trylocks and held until the dirty flags clear -- so no
+ * write can race an overlay adoption or slip between a node's commit
+ * and its flag clear, and the checkpoint always covers a consistent
+ * all-or-nothing set.  Contention releases everything and retries,
+ * letting the holder (e.g. an fsync waiting for zm_wlock) finish.  The
+ * fsync caller's own node recurses through its rrwlock (RWL_DUPOK).
  */
 int
 zlfs_commit(struct zlfs_mount *zmp)
@@ -707,20 +716,78 @@ zlfs_commit(struct zlfs_mount *zmp)
 	u_int64_t imap_lba, ckpt_lba, newgen, i, j, n, epb, nblocks, ninodes;
 	u_int64_t *imap_lbas = NULL;
 	u_int8_t *blk = NULL;
-	int error = 0, ndirty = 0;
+	struct zlfs_node **dnodes = NULL;
+	int error = 0, ndirty = 0, nd = 0, di2, nl, tries;
 
 	if (zmp->zm_rdonly)
 		return EROFS;
 
-	rw_enter_write(&zmp->zm_wlock);
+	for (tries = 0;; tries++) {
+		rw_enter_write(&zmp->zm_wlock);
+		/*
+		 * Snapshot the dirty nodes and take their vnode locks, all
+		 * or nothing.  Count and fill run under one zm_lock hold,
+		 * so a node inserted at the list head while we slept in
+		 * malloc cannot displace an older dirty node (say, the
+		 * fsync caller's) out of the snapshot.
+		 */
+		rw_enter_read(&zmp->zm_lock);
+		nd = 0;
+		LIST_FOREACH(znp, &zmp->zm_nodes, zn_entry) {
+			if (znp->zn_dirty)
+				nd++;
+		}
+		if (nd == 0) {
+			rw_exit_read(&zmp->zm_lock);
+			rw_exit_write(&zmp->zm_wlock);
+			free(dnodes, M_TEMP, ndirty * sizeof(*dnodes));
+			return 0;
+		}
+		if (dnodes == NULL || nd > ndirty) {
+			rw_exit_read(&zmp->zm_lock);
+			rw_exit_write(&zmp->zm_wlock);
+			free(dnodes, M_TEMP, ndirty * sizeof(*dnodes));
+			ndirty = nd + 8;	/* headroom for new dirtiers */
+			dnodes = mallocarray(ndirty, sizeof(*dnodes),
+			    M_TEMP, M_WAITOK);
+			continue;
+		}
+		nd = 0;
+		LIST_FOREACH(znp, &zmp->zm_nodes, zn_entry) {
+			if (!znp->zn_dirty || nd >= ndirty)
+				continue;
+			/* A dying vnode is mid-reclaim; its in-core state
+			 * is lost either way (dirty-reclaim caveat). */
+			if (vget(znp->zn_vnode, LK_NOWAIT) != 0)
+				continue;
+			dnodes[nd++] = znp;
+		}
+		rw_exit_read(&zmp->zm_lock);
 
-	LIST_FOREACH(znp, &zmp->zm_nodes, zn_entry) {
-		if (znp->zn_dirty)
-			ndirty++;
-	}
-	if (ndirty == 0) {
+		/*
+		 * Trylocks only: blocking on a vnode lock while holding
+		 * zm_wlock would deadlock against an fsync that holds the
+		 * vnode and waits for zm_wlock.  On contention release
+		 * everything and retry shortly; the holder finishes first.
+		 */
+		for (nl = 0; nl < nd; nl++) {
+			if (rrw_enter(&dnodes[nl]->zn_lock,
+			    RW_WRITE | RW_NOSLEEP) != 0)
+				break;
+		}
+		if (nl == nd)
+			break;		/* all locked; the commit proceeds */
+		while (nl-- > 0)
+			rrw_exit(&dnodes[nl]->zn_lock);
 		rw_exit_write(&zmp->zm_wlock);
-		return 0;
+		for (nl = 0; nl < nd; nl++)
+			vrele(dnodes[nl]->zn_vnode);
+		if (tries >= 9) {
+			free(dnodes, M_TEMP, ndirty * sizeof(*dnodes));
+			return EBUSY;
+		}
+		tsleep_nsec(&zmp->zm_wlock, PPAUSE, "zlfsretry",
+		    10 * 1000 * 1000ULL);
 	}
 
 	/*
@@ -740,11 +807,10 @@ zlfs_commit(struct zlfs_mount *zmp)
 		/* Checkpoint plus however many inode-map blocks. */
 		need = 1 + howmany(zmp->zm_ninodes,
 		    bsize / sizeof(u_int64_t));
-		LIST_FOREACH(znp, &zmp->zm_nodes, zn_entry) {
+		for (di2 = 0; di2 < nd; di2++) {
 			u_int32_t b;
 
-			if (!znp->zn_dirty)
-				continue;
+			znp = dnodes[di2];
 			if (znp->zn_data != NULL) {
 				/* Directory: rewritten whole. */
 				need += howmany(znp->zn_datalen, bsize) + 2;
@@ -767,11 +833,12 @@ zlfs_commit(struct zlfs_mount *zmp)
 			zlfs_clean(zmp);
 	}
 
-	/* 1. Data and inode blocks for every dirty inode. */
-	LIST_FOREACH(znp, &zmp->zm_nodes, zn_entry) {
-		if (!znp->zn_dirty)
-			continue;
-		error = zlfs_commit_node(zmp, znp);
+	/*
+	 * 1. Data and inode blocks for every dirty inode, each under its
+	 * vnode lock so writes cannot race the overlay adoption.
+	 */
+	for (di2 = 0; di2 < nd; di2++) {
+		error = zlfs_commit_node(zmp, dnodes[di2]);
 		if (error != 0)
 			goto out;
 	}
@@ -801,9 +868,12 @@ zlfs_commit(struct zlfs_mount *zmp)
 			goto out_blk;
 		memset(blk, 0, bsize);
 		n = MIN(epb, ninodes - j * epb);
+		/* zm_lock: a concurrent create may grow (replace) the map. */
+		rw_enter_read(&zmp->zm_lock);
 		for (i = 0; i < n; i++)
 			((u_int64_t *)blk)[i] =
 			    htole64(zmp->zm_imap[j * epb + i]);
+		rw_exit_read(&zmp->zm_lock);
 		error = zlfs_write_block(zmp, imap_lba, blk);
 		if (error != 0)
 			goto out_blk;
@@ -853,13 +923,20 @@ zlfs_commit(struct zlfs_mount *zmp)
 	 */
 	zmp->zm_super.zs_generation = newgen;
 	zmp->zm_super.zs_checkpoint_lba = ckpt_lba;
-	LIST_FOREACH(znp, &zmp->zm_nodes, zn_entry)
-		znp->zn_dirty = 0;
+	for (di2 = 0; di2 < nd; di2++)
+		dnodes[di2]->zn_dirty = 0;
 
 out_blk:
 	free(imap_lbas, M_ZLFS, bsize - sizeof(struct zlfs_checkpoint));
 	free(blk, M_ZLFS, bsize);
 out:
+	/* Dirty flags cleared (or preserved on failure) under the node
+	 * locks; release them, then the commit lock, then the refs. */
+	for (di2 = 0; di2 < nd; di2++)
+		rrw_exit(&dnodes[di2]->zn_lock);
 	rw_exit_write(&zmp->zm_wlock);
+	for (di2 = 0; di2 < nd; di2++)
+		vrele(dnodes[di2]->zn_vnode);
+	free(dnodes, M_TEMP, ndirty * sizeof(*dnodes));
 	return error;
 }