Date: Mon, 19 Apr 2021 23:00:01 +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: <YH3hQcmVhFvQK81W@kib.kiev.ua> In-Reply-To: <4ade0f5d-d4f4-616a-b198-fc58f947070d@tu-dortmund.de> References: <792c8a3d-8ea6-073f-3fda-b3eb793ef2b9@tu-dortmund.de> <YHVxfMrU9lmw3sG9@kib.kiev.ua> <4ade0f5d-d4f4-616a-b198-fc58f947070d@tu-dortmund.de>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Apr 19, 2021 at 09:43:47PM +0200, Alexander Lochmann wrote:
> Out of curiosity: What's the path from function entry to the access in line
> 759?
> The lock is acquired upon function entry in line 7197, and never released
> afterwards (except for the lines 7212 and 7217).
The bufobj mutex is interlocked with the buffer locks. Basically, each
time LK_INTERLOCK is used, the bo_lock is dropped.
Or do you ask about something else?
>
> - 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
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?YH3hQcmVhFvQK81W>