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

The following reply was made to PR kern/60504; it has been noted by GNATS.

From: Taylor R Campbell <[email protected]>
To: Shinichi Doyashiki <[email protected]>,
	"Konrad E. Schroeder" <[email protected]>
Cc: [email protected], [email protected]
Subject: Re: kern/60504: lfs_cleanerd can kernel crash on load in lfs_fcntl; lfs_sp == NULL
Date: Sat, 1 Aug 2026 04:58:04 +0000

 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 =3D lfs_sb_getoffset(fs);
    2028 		lfs_prelock(fs, 0);
    2029 		lfs_flush_dirops(fs);
    2030 		LFS_CLEANERINFO(cip, fs, bp);
    2031 		oclean =3D 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 !=3D NULL && ap->a_data !=3D NULL) {
    2036 			lws.direct =3D 0;
    2037 			lws.offset =3D lfs_btofsb(fs, fs->lfs_sp->bytes_written);
    2038 			*(struct lfs_write_stats *)ap->a_data =3D lws;
    2039 		}
    2040 		lfs_preunlock(fs);
    2041 		lfs_writer_leave(fs);
 
 https://nxr.netbsd.org/xref/src/sys/ufs/lfs/lfs_vnops.c?r=3D1.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 =3D fs->lfs_sp;
 ...
     807 	lfs_segunlock(fs);
     808=20
     809 	DLOG((DLOG_SEG, "  returning 0\n"));
     810 	return (0);
     811 }
 
 https://nxr.netbsd.org/xref/src/sys/ufs/lfs/lfs_segment.c?r=3D1.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 =3D=3D 1) {
 ...
     526 		if (!sync)
     527 			pool_put(&fs->lfs_segpool, sp);
     528 		fs->lfs_sp =3D NULL;
 ...
     569 		if (sync)
     570 			pool_put(&fs->lfs_segpool, sp);
 ...
     594 	} else {
     595 		--fs->lfs_seglock;
     596 		KASSERT(fs->lfs_seglock !=3D 0);
     597 	}
     598=20
     599 	lfs_preunlock(fs);
     600 }
 
 https://nxr.netbsd.org/xref/src/sys/ufs/lfs/lfs_subr.c?r=3D1.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 !=3D NULL) {
 +		if (ap !=3D NULL && ap->a_data !=3D NULL) {
  			lws.direct =3D 0;
  			lws.offset =3D lfs_btofsb(fs, fs->lfs_sp->bytes_written);
  			*(struct lfs_write_stats *)ap->a_data =3D 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 t=
 he
 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 ord=
 er
 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 =3D 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 =3D 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 |=3D SEGM_PROT;
 +		lfs_segwrite(ap->a_vp->v_mount, SEGM_CKP | SEGM_FORCE_CKP);
  		/* Copy out write stats */
  		if (ap !=3D NULL) {
  			lws.direct =3D 0;
  			lws.offset =3D lfs_btofsb(fs, fs->lfs_sp->bytes_written);
  			*(struct lfs_write_stats *)ap->a_data =3D 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.