Re: [patch] rw_try_wlock does not set recursive bit when recursing

2009/8/26 Attilio Rao <attilio_at_freebsd.org>:
> 2009/8/27 Andrew Brampton <brampton+freebsd_at_gmail.com>:
>> Hi,
>> The following sequence of commands fails on line 4 with an assertion
>> that the lock is not currently held:
>>
>> 1: rw_wlock(&rw);
>> 2: if ( rw_try_wlock(&rw) )
>> 3:    rw_wunlock(&rw);
>> 4: rw_wunlock(&rw);
>>
>> This is because after line 3 is executed the rw lock is no longer
>> held. I tracked this bug down to _rw_try_wlock which correctly
>> increments rw_recurse, but does not set the RW_LOCK_RECURSED bit.
>> Without this bit the third line unlocks the lock and leaves it in a
>> unlocked state (when it should still be locked). Adding a line to set
>> this bit makes _rw_try_wlock match the code in _rw_wlock_hard.
>
> Sorry, but I really don't understand how that can be a bug.
> On STABLE_7, RW_LOCK_RECURSED is not used for checking if the lock is
> recursed or not.
> it just got set for improving debugging and eventually we decided to
> drop it for 8.0.
>
> However, for deciding if a lock is recursed or not in both STABLE_7
> and HEAD we used just checking against the recursion count which is
> correctly handled by the function.
>
> What you describe can't be the problem.
>
> Attilio
>

Ok, so I have had a better look at the code in CURRENT, and compared
it to the code in STABLE_7. I apologise but I think I mixed up my
sources somewhere and the problem does not appear in CURRENT. The
problem does however occur in STABLE_7, and I can explain that below.

1: rw_wlock(&rw);
2: if ( rw_try_wlock(&rw) )
3:    rw_wunlock(&rw);
4: rw_wunlock(&rw);

Line 1, _rw_wlock gets called which calls __rw_wlock. This in turn
calls _rw_write_lock which changes rw->rw_lock to tid
Line 2, _rw_try_wlock gets called, which check if the lock is already
held (which it is), if so it then rw->rw_recurse++
Line 3, _rw_wunlock gets called which calls __rw_wunlock, which then
calls _rw_write_unlock. Now _rw_write_unlock trys to unlock the lock
by checking if rw->rw_lock is tid, if so it sets the lock to
RW_UNLOCKED. This is where the problem occurs, there is no check on
rx->rw_recurse.

Now, if we used this code instead:
1: rw_wlock(&rw);
2: if ( rw_try_wlock(&rw) )
3:    rw_wunlock(&rw);
4: rw_wunlock(&rw);

The order goes:
Line 1, _rw_wlock gets called which calls __rw_wlock. This in turn
calls _rw_write_lock which changes rw->rw_lock to tid
Line 2, _rw_wlock gets called which calls __rw_wlock. __rw_wlock
checks if rw->rw_lock is RW_UNLOCKED, otherwise it ends up calling
_rw_wlock_hard. Inside _rw_wlock_hard it checks if the lock is already
held, if so it increments rw_recurse, and changes rw->rw_lock to
(rw->rw_lock | RW_LOCK_RECURSED) by calling atomic_set_ptr.
Line 3, _rw_wunlock gets called which calls __rw_wunlock, which then
calls _rw_write_unlock. Now _rw_write_unlock trys to unlock the lock
by checking if rw->rw_lock is tid. This is where things differ from
the previous code, rw->rw_lock is NOT tid, it is now (tid |
RW_LOCK_RECURSED), and thus the code drops into _rw_wunlock_hard which
decrements rx->rw_recurse and removes the RW_LOCK_RECURSED flag if
rw_recurse equals zero.

Hopefully you see how not setting the RW_LOCK_RECURSED flag causes a
problem in rw_wunlock. So this can be fixed by either back porting the
CURRENT code to STABLE_7, or using the one line fix in my patch. As I
already stated, I am using my patch and the sample code I have given
no longer panics.

Sorry for the confusion
Andrew