Date: Tue, 13 Apr 2021 15:35:24 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Alexander Lochmann <alexander.lochmann@tu-dortmund.de> Cc: freebsd-fs@freebsd.org Subject: Re: [struct buf] Unlocked access to b_vflags? Message-ID: <YHWQDOVcQmr6Rj8C@kib.kiev.ua> In-Reply-To: <291727c7-df2e-3fa6-ebb0-597b8deff461@tu-dortmund.de> References: <792c8a3d-8ea6-073f-3fda-b3eb793ef2b9@tu-dortmund.de> <YHVxfMrU9lmw3sG9@kib.kiev.ua> <ad235e06-ec65-bd0f-e665-fde25dc35cf1@tu-dortmund.de> <YHWGnboDlRugVSsg@kib.kiev.ua> <291727c7-df2e-3fa6-ebb0-597b8deff461@tu-dortmund.de>
next in thread | previous in thread | raw e-mail | index | archive | help
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@FreeBSD.org> > > > > Date: Tue Apr 13 13:22:56 2021 +0300 > > > > > > > > b_vflags update requries bufobj lock > > > > Reported by: Alexander Lochmann <alexander.lochmann@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 >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?YHWQDOVcQmr6Rj8C>