Re: Various unprotected accesses to buf and vnode

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
> https://thasos.cs.tu-dortmund.de/freebsd-lockdoc/lockdoc-v13.0-0.6/source/sys/kern/vfs_cluster.c#L802.
> 
> It was release in binsfree() and bq_insert():
> https://thasos.cs.tu-dortmund.de/freebsd-lockdoc/latest/source/sys/kern/vfs_bio.c#L1537
> https://thasos.cs.tu-dortmund.de/freebsd-lockdoc/latest/source/sys/kern/vfs_bio.c#L1977

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
into https://reviews.freebsd.org/D31780

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.