update

issues/erofs-vget-insmntque-failure-use-after-release.md

@@ -0,0 +1,176 @@
+# `erofs_vget()` lifecycle violation after `insmntque()` failure
+
+## Status
+
+- State: **FIXED / RUNTIME VALIDATION PENDING**
+- Priority: **P1**
+- Scope: pre-existing vnode lifecycle issue; not introduced by Pre10 Batch B
+- Affected feature: inode-to-vnode construction through `erofs_vget()`
+
+## Problem description
+
+`repo-pre-10/src/inode.c:431` defines `erofs_vget()`. Before this fix, after
+allocating a new vnode and attaching an `erofs_node`, the function called
+`insmntque()` and used this failure branch:
+
+```c
+vp->v_data = en;
+en->vnode = vp;
+en->nid = nid;
+lockmgr(vp->v_vnlock, LK_EXCLUSIVE, NULL);
+error = insmntque(vp, mp);
+if (error != 0) {
+	free(en, M_EROFS);
+	vp->v_data = NULL;
+	*vpp = NULL;
+	return (error);
+}
+```
+
+The relevant call and failure branch are at
+`repo-pre-10/src/inode.c:463-468`. On an insertion failure, the FreeBSD
+`insmntque()` API reclaims and releases the vnode before returning. The
+subsequent assignment to `vp->v_data` therefore accesses a vnode whose lifetime
+has ended from the caller's perspective.
+
+## Trigger condition
+
+The locally reviewed FreeBSD implementation rejects insertion while the target
+mount is being torn down. In
+`/work/dev-freebsd-releng/sys/kern/vfs_subr.c:2303-2306`, the failure condition
+checks `MNTK_UNMOUNT` and either forced unmount (`MNTK_UNMOUNTF`) or an empty
+mount vnode list, unless `VV_FORCEINSMQ` applies. The function then returns
+`EBUSY` at `vfs_subr.c:2315`.
+
+The issue can therefore be triggered when vnode creation in `erofs_vget()`
+races with mount teardown or forced unmount and `insmntque()` rejects the new
+vnode.
+
+## FreeBSD contract
+
+The local FreeBSD kernel source documents the ownership distinction explicitly
+at `/work/dev-freebsd-releng/sys/kern/vfs_subr.c:2328-2331`:
+
+- `insmntque()` reclaims the vnode when insertion fails.
+- `insmntque1()` leaves vnode cleanup to the caller.
+
+`insmntque()` calls `insmntque1_int(vp, mp, true)` at
+`vfs_subr.c:2333-2337`. In the relevant failure path,
+`insmntque1_int()` performs all of the following at
+`vfs_subr.c:2309-2313`:
+
+```c
+vp->v_data = NULL;
+vp->v_op = &dead_vnodeops;
+vgone(vp);
+vput(vp);
+```
+
+Consequently, an error return from `insmntque()` does not preserve caller
+ownership of a live `vp`. Reading or writing `vp`, including assigning
+`vp->v_data`, is unsafe after that return.
+
+## Impact
+
+The affected path is vnode construction for filesystem objects. Under the
+mount-teardown race described above, the pre-fix failure branch may write
+through a released vnode pointer. Likely consequences include a use-after-free,
+memory corruption, a kernel panic, or corruption of an unrelated vnode if the
+storage is recycled quickly. The exact manifestation has not been reproduced
+and must not be treated as confirmed beyond the statically established lifetime
+violation.
+
+The separately allocated `en` object is not released by the kernel failure
+path. That path clears `vp->v_data` and changes `v_op` to `dead_vnodeops` before
+calling `vgone()`. The dead vnode reclaim operation is `VOP_NULL`, so the
+filesystem reclaim callback does not run and cannot release `en`. Caller-side
+`free(en, M_EROFS)` is therefore required exactly once.
+
+## Implemented fix
+
+Pre10 now treats an error return from `insmntque()` as the end of caller
+ownership of `vp`:
+
+```c
+error = insmntque(vp, mp);
+if (error != 0) {
+	free(en, M_EROFS);
+	*vpp = NULL;
+	return (error);
+}
+```
+
+The caller frees only the independently allocated `en` object and does not
+read, write, reclaim, unlock, or release `vp` after `insmntque()` returns an
+error. The kernel failure path clears `vp->v_data` and installs
+`dead_vnodeops` before invoking `vgone()` and `vput()`. Dead vnode reclaim is a
+no-op, so no vnode cleanup callback owns `en`. This gives `en` exactly one
+release on the failure path.
+
+The successful insertion path, vnode lock state, hash insertion race handling,
+errno return, and later inode-read cleanup are unchanged. In particular,
+`vfs_hash_insert()` remains responsible for reclaiming and releasing the losing
+new vnode when another thread wins the hash race.
+
+## Current analysis and progress
+
+- Independently reviewed the Pre10 `erofs_vget()` failure path.
+- Confirmed the local FreeBSD `insmntque()` and `insmntque1()` ownership
+  contract against kernel source.
+- Confirmed the teardown/forced-unmount condition that produces `EBUSY`.
+- Confirmed that `repo-pre-10/src/inode.c` accesses `vp->v_data` after the
+  reclaiming API returns an error.
+- Compared the cleanup pattern with the local FreeBSD ext2fs, msdosfs, UDF,
+  pseudofs, and p9fs vnode construction paths. They likewise avoid touching the
+  vnode after `insmntque()` fails and separately dispose caller-owned node
+  state where required.
+- Implemented the minimal Pre10 source fix by removing the post-failure
+  `vp->v_data` assignment.
+- Completed static ownership, lock, hash-race, reclaim, and error-path review.
+- No QEMU run, runtime reproduction, dedicated race test, or feature test has
+  been attempted.
+
+The statically proven lifetime violation is fixed. Runtime validation remains
+open because the triggering teardown race is not covered by ordinary smoke
+testing.
+
+## Candidate fix boundaries
+
+The implemented correction preserves the existing choice of `insmntque()` and
+treats an error return as transferring vnode cleanup entirely to the kernel.
+The caller releases only independently owned `en`, clears `*vpp`, and returns
+without any further access to or release of `vp`.
+
+An alternative would be to deliberately switch to `insmntque1()` and implement
+the complete caller-owned failure cleanup required by that API. This is a wider
+lifecycle change and should not be selected merely to mirror naming or control
+flow.
+
+The wider `insmntque1()` alternative was rejected because it would require new
+caller-owned vnode cleanup without providing any benefit for this path.
+
+## Required validation
+
+Before marking runtime validation complete:
+
+1. Build the EROFS module with the supported FreeBSD source tree.
+2. Run a dedicated mount/unmount race test that repeatedly creates or looks up
+   previously uncached vnodes while normal and forced unmount are attempted.
+3. Use kernel diagnostics appropriate for detecting stale vnode access, memory
+   corruption, lock misuse, and double release.
+4. Re-run the ordinary mount, lookup, read, and unmount smoke coverage after the
+   race test passes.
+
+The dedicated unmount race test is required because ordinary static checks and
+single-threaded smoke tests do not exercise the failing `insmntque()` branch.
+
+## Relationship to Pre10 Batch B
+
+This issue predates Batch B. Batch B extracted `erofs_fill_vnode()` only for the
+successful post-`erofs_read_inode()` field setup and did not modify the
+`insmntque()` call or its failure branch. Independent review found no Batch B
+regression in this path.
+
+The minimal ownership fix is included in Pre10. The dedicated teardown race
+test remains deferred and cannot be replaced by the planned ordinary smoke
+test.