[struct buf] Unlocked access to b_vflags?

[Alexander Lochmann]

On 19.04.21 22:00, Konstantin Belousov wrote:
> 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.
Is this done by BUF_LOCK() in line 7233?


Is the interlocked behavior done automatically, or just because both 
LK_INTERLOCK and BO_LOCKPTR(bo) are specified?
>
> Or do you ask about something else?
No. I think that's it.

- Alex
> 
>>
>> - 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 at FreeBSD.org>
>>> Date:   Tue Apr 13 13:22:56 2021 +0300
>>>
>>>       b_vflags update requries bufobj lock
>>>       Reported by:    Alexander Lochmann <alexander.lochmann at 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
>>
> 
> 
> 

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freebsd.org/pipermail/freebsd-fs/attachments/20210419/cf99f4a5/attachment.sig>