NetBSD Problem Report #57606

From www@netbsd.org  Wed Sep  6 12:27:59 2023
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id ECEF11A9239
	for <gnats-bugs@gnats.NetBSD.org>; Wed,  6 Sep 2023 12:27:58 +0000 (UTC)
Message-Id: <20230906122757.6ECB31A923A@mollari.NetBSD.org>
Date: Wed,  6 Sep 2023 12:27:57 +0000 (UTC)
From: merlin.scholz@tu-dortmund.de
Reply-To: merlin.scholz@tu-dortmund.de
To: gnats-bugs@NetBSD.org
Subject: Missing lock in FFS sync code
X-Send-Pr-Version: www-1.0

>Number:         57606
>Category:       kern
>Synopsis:       Missing lock in FFS sync code
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Sep 06 12:30:00 +0000 2023
>Closed-Date:    Thu Dec 07 14:30:38 +0000 2023
>Last-Modified:  Thu Dec 07 14:30:38 +0000 2023
>Originator:     Merlin Scholz
>Release:        10.99.5
>Organization:
TU Dortmund
>Environment:
NetBSD netbsd10 10.99.5 NetBSD 10.99.5 (GENERIC) #5: Mon Aug 21 01:01:48 CEST 2023  smmescho@mars:/home/lab/smmescho/ma/kernel/netbsd/obj/sys/arch/i386/compile/GENERIC i386
>Description:
According to the documentation provided in sys/sys/vnode.h, all vnode members, including v_numoutput, are supposed to be locked through a held v_interlock before being accessed.

This is not the case in /src/sys/ufs/ffs/ffs_vfsops.c#2024, where `ump->um_devvp->v_numoutput > 0` is being checked without prior locking of v_interlock.

The problem should be low-impact, as confirmed in #netbsd-code:
13:52 <@Riastradh> although I guess it's not actually a big issue, because either
13:53 <@Riastradh> (a) ffs_sync is being called concurrently with other file system activity, so new writes can be concurrently triggered anyway, so it doesn't really matter much; or
13:53 <@Riastradh> (b) ffs_sync is being called when the file system is quiesced, in which case it can't change anyway.

This issue has been found by performing lock analysis using LockDoc, see https://doi.org/10.1145/3302424.3303948.
>How-To-Repeat:

>Fix:
Adding the appropriate v_interlock actions in the specified code section should suffice in solving the issue. Since it is not possible to hold v_interlock across vn_lock, the if-statement would have to be split up.

>Release-Note:

>Audit-Trail:
From: Merlin Scholz <merlin.scholz@tu-dortmund.de>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/57606
Date: Thu, 7 Sep 2023 10:12:16 +0200

 The following patch should fix the issue. It has been verified through 
 ATF and LockDoc.


 RCS file: /cvsroot/src/sys/ufs/ffs/ffs_vfsops.c,v
 retrieving revision 1.381
 diff -u -r1.381 ffs_vfsops.c
 --- src/sys/ufs/ffs/ffs_vfsops.c	15 Jun 2023 09:15:54 -0000	1.381
 +++ src/sys/ufs/ffs/ffs_vfsops.c	7 Sep 2023 08:05:06 -0000
 @@ -2021,14 +2021,20 @@
   	/*
   	 * Force stale file system control information to be flushed.
   	 */
 -	if (waitfor != MNT_LAZY && (ump->um_devvp->v_numoutput > 0 ||
 -	    !LIST_EMPTY(&ump->um_devvp->v_dirtyblkhd))) {
 -		vn_lock(ump->um_devvp, LK_EXCLUSIVE | LK_RETRY);
 -		if ((error = VOP_FSYNC(ump->um_devvp, cred,
 -		    (waitfor == MNT_WAIT ? FSYNC_WAIT : 0) | FSYNC_NOLOG,
 -		    0, 0)) != 0)
 -			allerror = error;
 -		VOP_UNLOCK(ump->um_devvp);
 +	if (waitfor != MNT_LAZY) {
 +		bool need_devvp_fsync;
 +		mutex_enter(ump->um_devvp->v_interlock);
 +		need_devvp_fsync = (ump->um_devvp->v_numoutput > 0 ||
 +		    !LIST_EMPTY(&ump->um_devvp->v_dirtyblkhd));
 +		mutex_exit(ump->um_devvp->v_interlock);
 +		if (need_devvp_fsync) {
 +			vn_lock(ump->um_devvp, LK_EXCLUSIVE | LK_RETRY);
 +			if ((error = VOP_FSYNC(ump->um_devvp, cred,
 +			    (waitfor == MNT_WAIT ? FSYNC_WAIT : 0) | FSYNC_NOLOG,
 +			    0, 0)) != 0)
 +				allerror = error;
 +			VOP_UNLOCK(ump->um_devvp);
 +		}
   	}
   #if defined(QUOTA) || defined(QUOTA2)
   	qsync(mp);

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57606 CVS commit: src/sys/ufs/ffs
Date: Fri, 8 Sep 2023 23:21:55 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Fri Sep  8 23:21:55 UTC 2023

 Modified Files:
 	src/sys/ufs/ffs: ffs_vfsops.c

 Log Message:
 ffs_sync: Avoid unlocked access to v_numoutput/v_dirtyblkhd.

 Found by lockdoc.

 PR kern/57606


 To generate a diff of this commit:
 cvs rdiff -u -r1.381 -r1.382 src/sys/ufs/ffs/ffs_vfsops.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Sat, 09 Sep 2023 21:31:51 +0000
State-Changed-Why:
needs pullups if we want to pacify data race detectors on release branches
(tsan or whatever)


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57606 CVS commit: [netbsd-10] src/sys/ufs/ffs
Date: Wed, 18 Oct 2023 15:10:41 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Wed Oct 18 15:10:41 UTC 2023

 Modified Files:
 	src/sys/ufs/ffs [netbsd-10]: ffs_vfsops.c

 Log Message:
 Pull up following revision(s) (requested by riastradh in ticket #424):

 	sys/ufs/ffs/ffs_vfsops.c: revision 1.382

 ffs_sync: Avoid unlocked access to v_numoutput/v_dirtyblkhd.

 Found by lockdoc.
 PR kern/57606


 To generate a diff of this commit:
 cvs rdiff -u -r1.378.2.2 -r1.378.2.3 src/sys/ufs/ffs/ffs_vfsops.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: needs-pullups->pending-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Tue, 28 Nov 2023 02:50:12 +0000
State-Changed-Why:
pullup-10 #424
pullup-9 #1770
pullup-8 #1921


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57606 CVS commit: [netbsd-9] src/sys/ufs/ffs
Date: Tue, 28 Nov 2023 13:11:38 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Tue Nov 28 13:11:38 UTC 2023

 Modified Files:
 	src/sys/ufs/ffs [netbsd-9]: ffs_vfsops.c

 Log Message:
 Pull up following revision(s) (requested by riastradh in ticket #1770):

 	sys/ufs/ffs/ffs_vfsops.c: revision 1.382

 ffs_sync: Avoid unlocked access to v_numoutput/v_dirtyblkhd.

 Found by lockdoc.

 PR kern/57606


 To generate a diff of this commit:
 cvs rdiff -u -r1.362 -r1.362.2.1 src/sys/ufs/ffs/ffs_vfsops.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57606 CVS commit: [netbsd-8] src/sys/ufs/ffs
Date: Tue, 28 Nov 2023 13:13:29 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Tue Nov 28 13:13:29 UTC 2023

 Modified Files:
 	src/sys/ufs/ffs [netbsd-8]: ffs_vfsops.c

 Log Message:
 Pull up following revision(s) (requested by riastradh in ticket #1921):

 	sys/ufs/ffs/ffs_vfsops.c: revision 1.382

 ffs_sync: Avoid unlocked access to v_numoutput/v_dirtyblkhd.

 Found by lockdoc.

 PR kern/57606


 To generate a diff of this commit:
 cvs rdiff -u -r1.353.4.2 -r1.353.4.3 src/sys/ufs/ffs/ffs_vfsops.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: pending-pullups->closed
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Thu, 07 Dec 2023 14:30:38 +0000
State-Changed-Why:
fixed and pulled up to 10, 9, 8


>Unformatted:

Home	PR Database Search