Re: Various unprotected accesses to buf and vnode

From: Konstantin Belousov <>
Date: Thu, 02 Sep 2021 04:16:21 UTC
On Wed, Sep 01, 2021 at 03:10:53PM +0200, Alexander Lochmann wrote:
> On 31.08.21 16:44, Konstantin Belousov wrote:
> >> So in all of those call sequences the buffer lock is not acquired.
> >> However, I'd not rule out that our tooling could be broken as well.
> > Buffer is locked inside UFS_BALLOC(), which returns it to the ffs_write()
> > use.
> I took a deep dive into our data, and had a closer look at two samples.
> In both cases the b_lock is not acquired.
> Since the debug information seems to be damaged, I had to use 'objdump
> -S' to translate the pc of the unguarded memory access to a source code
> position.
> It seems to be vp->v_lasta = bp->b_blkno; in
> It was release in binsfree() and bq_insert():

Ah, it is bp->b_blkno access after the b*write() functions were called
to write out and release the buffer, right.  I put the patch to fix this

Please remind me what attributions to use for 'Reported by:' tagline.

> > Read e.g. sys/ufs/ufs/inode.h gerald comment above struct inode definition.
> > It provides more detailed exposure.
> Aaah. Thx. This is about the struct inode. So I assume it also applies
> for a vnode belonging to an inode. Am I right?> Vnode lock is a lock
> obtained with vn_lock().  It is up to filesystem
When needed, yes, it is a reasonable locking strategy.  But I am not
sure that we actually use for any of the struct vnode fields proper,
Something closer to it is for v_writecount, but formally it is under the
vnode interlock.  Although I do not think we ever modify it without holding
vnode lock, in some mode.

> > to implement VOP_LOCK() which locks the vnode.
> > 
> > Default VOP_LOCK() locks vp->v_vnlock, which again by default points
> > to &vp->v_lock.
> > 
> > There are several special cases.  For instance, for FFS and snapshot vnodes,
> > v_vnlock points to the snapdata->sn_lock (snapdata is unique per FFS mount).
> > For nullfs non-reclaimed vnodes, v_vnlock points to the lower vnode lock.
> > 
> Thx! Is this written down somewhere?
No, but it is rather clear from the code.