[struct buf] Unlocked access to b_vflags?

[Konstantin Belousov]

On Mon, Apr 12, 2021 at 11:19:05PM +0200, Alexander Lochmann wrote:
> Hi folks,
> 
> I'm was digging through our data set when I encountered a strange situation:
> According to the code in trunc_dependencies() in sys/ufs/ffs/ffs_softdep.c,
> the bo_lock should be held. At least that's how I read the code.
> However, we see several thousands of accesses to b_vflags without the
> bo_lock held.
> At least the own b_lock is acquired.
> The access happens in line 7549: bp->b_vflags |= BV_SCANNED; [1]
> Can you please shed some light on this situation?
> Is the b_lock sufficeint, and somehow overrules the bo_lock?
> Am I missing something?
I think you found a valid race.  There is one more place where BV_SCANNED
was manipulated without owning bufobj lock.  Patch below should fix both.

commit a678470b1307542c5a46b930c119b2358863e0d2
Author: Konstantin Belousov <kib at FreeBSD.org>
Date:   Tue Apr 13 13:22:56 2021 +0300

    b_vflags update requries bufobj lock
    
    Reported by:    Alexander Lochmann <alexander.lochmann at tu-dortmund.de> (trunc_dependencies())

diff --git a/sys/ufs/ffs/ffs_softdep.c b/sys/ufs/ffs/ffs_softdep.c
index 0091b5dcd3b8..23c0cf6e128b 100644
--- a/sys/ufs/ffs/ffs_softdep.c
+++ b/sys/ufs/ffs/ffs_softdep.c
@@ -7546,7 +7546,9 @@ trunc_dependencies(ip, freeblks, lastlbn, lastoff, flags)
 			BO_LOCK(bo);
 			goto cleanrestart;
 		}
+		BO_LOCK(bo);
 		bp->b_vflags |= BV_SCANNED;
+		BO_UNLOCK(bo);
 		bremfree(bp);
 		if (blkoff != 0) {
 			allocbuf(bp, blkoff);
diff --git a/sys/ufs/ffs/ffs_vnops.c b/sys/ufs/ffs/ffs_vnops.c
index dc638595eb7b..05eb19c0ee13 100644
--- a/sys/ufs/ffs/ffs_vnops.c
+++ b/sys/ufs/ffs/ffs_vnops.c
@@ -321,8 +321,9 @@ ffs_syncvnode(struct vnode *vp, int waitfor, int flags)
 			if (BUF_LOCK(bp,
 			    LK_EXCLUSIVE | LK_SLEEPFAIL | LK_INTERLOCK,
 			    BO_LOCKPTR(bo)) != 0) {
+				BO_LOCK(bo);
 				bp->b_vflags &= ~BV_SCANNED;
-				goto next;
+				goto next_locked;
 			}
 		} else
 			continue;
@@ -385,6 +386,7 @@ ffs_syncvnode(struct vnode *vp, int waitfor, int flags)
 		 * to start from a known point.
 		 */
 		BO_LOCK(bo);
+next_locked:
 		nbp = TAILQ_FIRST(&bo->bo_dirty.bv_hd);
 	}
 	if (waitfor != MNT_WAIT) {