[struct buf] Unlocked access to b_vflags?

[Konstantin Belousov]

On Tue, Apr 13, 2021 at 02:13:57PM +0200, Alexander Lochmann wrote:
> "The bug was found by analyzing the memory accesses to certain data types
> and lock operations while running a load. The whole workflow is called
> LockDoc."
> or
> "The bug was found by performing lock analysis using LockDoc."
> Is this enough, or shall I explain it in more detail?
> 
> Unfortunately, we do not have a project page yet.
> I, however, have the link to our paper:
> https://doi.org/10.1145/3302424.3303948
But it is beyond the paywall?

> 
> - Alex
> 
> On 13.04.21 13:55, Konstantin Belousov wrote:
> > On Tue, Apr 13, 2021 at 01:44:14PM +0200, Alexander Lochmann wrote:
> > > Thx!
> > > Do you mind adding "Found by LockDoc" to the Commit-Msg?
> > I do not, but I have no idea what LockDoc is.  Can you provide the complete
> > attribution sentence, perhaps including the URL to the relevant web page?
> > 
> > > 
> > > Cheers,
> > > Alex
> > > 
> > > On 13.04.21 12:25, Konstantin Belousov wrote:
> > > > 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) {
> > > > 
> > > 
> > > -- 
> > > Technische Universität Dortmund
> > > Alexander Lochmann                PGP key: 0xBC3EF6FD
> > > Otto-Hahn-Str. 16                 phone:  +49.231.7556141
> > > D-44227 Dortmund                  fax:    +49.231.7556116
> > > http://ess.cs.tu-dortmund.de/Staff/al
> > > 
> > 
> > 
> > 
> 
> -- 
> Technische Universität Dortmund
> Alexander Lochmann                PGP key: 0xBC3EF6FD
> Otto-Hahn-Str. 16                 phone:  +49.231.7556141
> D-44227 Dortmund                  fax:    +49.231.7556116
> http://ess.cs.tu-dortmund.de/Staff/al
>