Re: kern/60504: lfs_cleanerd can kernel crash on load in lfs_fcntl; lfs_sp == NULL

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

I'm a little puzzled by how this could possibly ever work.

I don't see anything taking the seglock in this _except_ lfs_segwrite
-- I don't think lfs_writer_enter, lfs_prelock, or lfs_flush_dirops
will do that:

   2021 	case LFCNRECLAIM:
   2022 		/*
   2023 		 * Flush dirops and write Ifile, allowing empty segments
   2024 		 * to be immediately reclaimed.
   2025 		 */
   2026 		lfs_writer_enter(fs, "pndirop");
   2027 		off = lfs_sb_getoffset(fs);
   2028 		lfs_prelock(fs, 0);
   2029 		lfs_flush_dirops(fs);
   2030 		LFS_CLEANERINFO(cip, fs, bp);
   2031 		oclean = lfs_ci_getclean(fs, cip);
   2032 		LFS_SYNC_CLEANERINFO(cip, fs, bp, 1);
   2033 		lfs_segwrite(ap->a_vp->v_mount, SEGM_CKP | SEGM_FORCE_CKP);
   2034 		/* Copy out write stats */
   2035 		if (ap != NULL && ap->a_data != NULL) {
   2036 			lws.direct = 0;
   2037 			lws.offset = lfs_btofsb(fs, fs->lfs_sp->bytes_written);
   2038 			*(struct lfs_write_stats *)ap->a_data = lws;
   2039 		}
   2040 		lfs_preunlock(fs);
   2041 		lfs_writer_leave(fs);

https://nxr.netbsd.org/xref/src/sys/ufs/lfs/lfs_vnops.c?r=1.353#2021

Inside lfs_segwrite, there is only one path out if the file system
isn't read-only -- these are the only two returns:

    542 int
    543 lfs_segwrite(struct mount *mp, int flags)
    544 {
...
    563 	if (fs->lfs_ronly)
    564 		return EROFS;
...
    588 	lfs_seglock(fs, flags | (do_ckp ? SEGM_CKP : 0));
    589 	sp = fs->lfs_sp;
...
    807 	lfs_segunlock(fs);
    808 
    809 	DLOG((DLOG_SEG, "  returning 0\n"));
    810 	return (0);
    811 }

https://nxr.netbsd.org/xref/src/sys/ufs/lfs/lfs_segment.c?r=1.308#539

So I think when we enter lfs_seglock from fcntl(LFCNRECLAIM), we are
guaranteed _not_ to hold the seglock, and thus the seglock depth from
line 588 to line 807 should be 1.  And in that case, lfs_segunlock
will free fs->lfs_sp and null it out:

    490 void
    491 lfs_segunlock(struct lfs *fs)
    492 {
...
    503 	if (fs->lfs_seglock == 1) {
...
    526 		if (!sync)
    527 			pool_put(&fs->lfs_segpool, sp);
    528 		fs->lfs_sp = NULL;
...
    569 		if (sync)
    570 			pool_put(&fs->lfs_segpool, sp);
...
    594 	} else {
    595 		--fs->lfs_seglock;
    596 		KASSERT(fs->lfs_seglock != 0);
    597 	}
    598 
    599 	lfs_preunlock(fs);
    600 }

https://nxr.netbsd.org/xref/src/sys/ufs/lfs/lfs_subr.c?r=1.110#486

So by the time lfs_segwrite returns, fs->lfs_sp should be null!

Now it looks like the LFCNRECLAIM code is racy: it's possible that
_another thread_ will set fs->lfs_sp to nonnull under the seglock, and
then LFCNRECLAIM might get lucky and read out a nonnull fs->lfs_sp by
chance.  But I expect that to be the exception, not the rule.

It looks like perseant@ recently added a null test here, in January --
although it is sensible to add this null test for other reasons, I
wonder whether it was prompted by the same crash you're describing?

Module Name:    src
Committed By:   perseant
Date:           Fri Jan 30 15:54:29 UTC 2026

Modified Files:
        src/sys/ufs/lfs: lfs_vnops.c

Log Message:
Check whether ap->a_data is NULL before using it.  Prevents a crash when
calling LCFNRECLAIM with a null stats pointer.


To generate a diff of this commit:
cvs rdiff -u -r1.352 -r1.353 src/sys/ufs/lfs/lfs_vnops.c

--- a/sys/ufs/lfs/lfs_vnops.c
+++ b/sys/ufs/lfs/lfs_vnops.c
...
@@ -2032,7 +2032,7 @@ lfs_fcntl(void *v)
 		LFS_SYNC_CLEANERINFO(cip, fs, bp, 1);
 		lfs_segwrite(ap->a_vp->v_mount, SEGM_CKP | SEGM_FORCE_CKP);
 		/* Copy out write stats */
-		if (ap != NULL) {
+		if (ap != NULL && ap->a_data != NULL) {
 			lws.direct = 0;
 			lws.offset = lfs_btofsb(fs, fs->lfs_sp->bytes_written);
 			*(struct lfs_write_stats *)ap->a_data = lws;

https://mail-index.netbsd.org/source-changes/2026/01/30/msg160457.html

Going back slightly further in January, it looks like LFCNRECLAIM
_used to_ hold the seglock across the logic where this crash happened:

Module Name:    src
Committed By:   perseant
Date:           Tue Jan 20 15:30:15 UTC 2026

Modified Files:
        src/sys/ufs/lfs: lfs_segment.c

Log Message:
Avoid a deadlock between vnode reclamation and lfs_writevnodes().  A vnode
being reclaimed will be in state VS_RECLAIMING, while it attemmpts to get the
segment lock.  lfs_writevnodes() holds the segment lock while traversing the
list of vnodes; so it must skip vnodes in the process of reclamation in order
to avoid a deadlock.


To generate a diff of this commit:
cvs rdiff -u -r1.306 -r1.307 src/sys/ufs/lfs/lfs_segment.c

--- a/sys/ufs/lfs/lfs_vnops.c
+++ b/sys/ufs/lfs/lfs_vnops.c
...
@@ -2025,20 +2025,19 @@ lfs_fcntl(void *v)
 		 */
 		lfs_writer_enter(fs, "pndirop");
 		off = lfs_sb_getoffset(fs);
-		lfs_seglock(fs, SEGM_FORCE_CKP | SEGM_CKP);
+		lfs_prelock(fs, 0);
 		lfs_flush_dirops(fs);
 		LFS_CLEANERINFO(cip, fs, bp);
 		oclean = lfs_ci_getclean(fs, cip);
 		LFS_SYNC_CLEANERINFO(cip, fs, bp, 1);
-		lfs_segwrite(ap->a_vp->v_mount, SEGM_FORCE_CKP);
-		fs->lfs_sp->seg_flags |= SEGM_PROT;
+		lfs_segwrite(ap->a_vp->v_mount, SEGM_CKP | SEGM_FORCE_CKP);
 		/* Copy out write stats */
 		if (ap != NULL) {
 			lws.direct = 0;
 			lws.offset = lfs_btofsb(fs, fs->lfs_sp->bytes_written);
 			*(struct lfs_write_stats *)ap->a_data = lws;
 		}
-		lfs_segunlock(fs);
+		lfs_preunlock(fs);
 		lfs_writer_leave(fs);

https://mail-index.netbsd.org/source-changes/2026/01/20/msg160304.html

So it looks like that change introduced the bug.