Re: Various unprotected accesses to buf and vnode

On Fri, Aug 27, 2021 at 03:48:38PM +0200, Alexander Lochmann wrote:
> Hi folks,
> 
> I'm still analyzing our LockDoc (lock analysis) data for FreeBSD. I came
> across accesses that do not adhere to the locking documentation. I cannot
> tell whether these accesses are made deliberately without locks or not.
> I listed them below.
> 
> Can you please shed some light on those cases?
> 
> Thx and regards,
> Alex
> 
> - Write to buf.b_error without buf.b_lock:
> https://github.com/freebsd/freebsd-src/blob/main/sys/kern/vfs_vnops.c#L2846
I believe this is fine.  If the buffer is still on the delayed write queue,
then nobody can start the write, and buffer cannot leave the queue because
we locked the bo lock.  It might be slightly formally better to move
clearing of b_error after we obtained the buffer lock, but I do not want
to change this code just for formality.  It might be that the clearing can
be avoided at all, in fact.

> 
> - Read of buf.b_blkno in cluster_write():
> According to the documentation b_lock is needed.
> Is b_blkno maybe a read-only element of struct buf?
No, b_blkno is not read-only, it is the disk block number for the block,
as opposed to b_lbklno which is logical file block number.  The buffer
is instantiated with b_blkno == b_lblkno, and when the buffer is mapped
to the real disk block, b_blkno is updated.

Could you show me the backtrace of the situation where cluster_write()
is called with unlocked buffer?

> 
> - Read of buf.b_flags, buf.b_xflags and buf.b_vp:
> https://github.com/freebsd/freebsd-src/blob/main/sys/kern/vfs_subr.c#L2351
> Are those reads innocent races?
> According to our data, buf.b_lock is not acquired.
These are fine.  We check that nbp buffer is still on the clean/dirty
queue of our vnode, and we own the buffer object lock.  Since buffers
are moved from the queues under the bo lock, the operation is safe.

You may think about it in the following way:
- the update of the flags word require owning corresponding lock to not loose
  other updates
- but changes of the flags that are tested in the referenced lines are also
  protected by the buffer object lock

> 
> - Write to vnode.v_bufobj.bo_object:
> https://github.com/freebsd/freebsd-src/blob/main/sys/vm/vnode_pager.c#L291
> According to the documentation, '[...] the vnode lock which embeds the
> bufobj' is needed. However, interlock is taken in line 276.
> Is the interlock equivalent to the vnode lock?
> (I assume 'the vnode lock' refers to vnode.v_lock.)
vnode_pager_alloc() must be called with the vnode exclusively locked.
This is asserted at the beginning of the function.

> 
> - Is buf.b_bufobj a read-only element?
You should scope the question.

While buffer is owned by a vnode, b_bufobj is constant.  Since buffers are
type-stable, they migrate between vnodes as cache finds it required to
reclaim and reuse.  There, b_bufobj is changed.