NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
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 <riastradh%NetBSD.org@localhost>
To: Shinichi Doyashiki <clare%csel.org@localhost>,
"Konrad E. Schroeder" <perseant%hhhh.org@localhost>
Cc: gnats-bugs%NetBSD.org@localhost, netbsd-bugs%NetBSD.org@localhost
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.
Home |
Main Index |
Thread Index |
Old Index