Re: Various unprotected accesses to buf and vnode

From: Alexander Lochmann <alexander.lochmann_at_tu-dortmund.de>
Date: Sat, 28 Aug 2021 18:53:19 UTC
On 27.08.21 20:40, Konstantin Belousov wrote:
>> - 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?
> 
>>
If you don't mind, you can find them here: 
https://thasos.cs.tu-dortmund.de/bugs/ctx-buf-b_blkno-r-cex.html
(Pls ignore the line 'Hypothesis 1: ....'.)
>> - 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.
> 
Yeah, I see that check: ASSERT_VOP_LOCKED(vp, "vnode_pager_alloc");.
However, our data says otherwise: According to our trace, the shared 
lock is taken.
You may have a look at 
https://thasos.cs.tu-dortmund.de/bugs/ctx-vnode-v_bufobj.bo_object-w-cex.html. 
'EMBSAME(vnode.v_lock[r])' refers to the shared vnode lock.
A click on each lock description, e.g., EMBSAME(vnode.v_lock[r]), leads 
to the code where it was taken.
(Pls ignore the line 'Hypothesis 1: ....'.)
>>
>> - 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.
> 
I'm referring to getdirtybuf(): msleep(&bp->b_xflags, 
BO_LOCKPTR(bp->b_bufobj),PRIBIO | PDROP, "getbuf", 0);

-- 
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