linux-kernel - Re: [PATCH] locking/rwsem: Remove unnessary check in rwsem_down_read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5086cd5b-a832-4250-9927-4b300d2f611e@shopee.com>
Date:   Thu, 9 Nov 2023 11:17:32 +0800
From:   Haifeng Xu <haifeng.xu@...pee.com>
To:     Waiman Long <longman@...hat.com>
Cc:     peterz@...radead.org, mingo@...hat.com, will@...nel.org,
        boqun.feng@...il.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] locking/rwsem: Remove unnessary check in
 rwsem_down_read_slowpath()



On 2023/11/8 22:04, Waiman Long wrote:
> On 11/8/23 05:56, Haifeng Xu wrote:
>> When the owner of rw_semaphore is reader, the count can't be
>> RWSEM_WRITER_LOCKED, so there is no need to check it.
>>
>> Signed-off-by: Haifeng Xu <haifeng.xu@...pee.com>
>> ---
>>   kernel/locking/rwsem.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
>> index 2340b6d90ec6..7a4d8a9ebd9c 100644
>> --- a/kernel/locking/rwsem.c
>> +++ b/kernel/locking/rwsem.c
>> @@ -1005,8 +1005,7 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, long count, unsigned int stat
>>        * waiter, don't attempt optimistic lock stealing if the lock is
>>        * currently owned by readers.
>>        */
>> -    if ((atomic_long_read(&sem->owner) & RWSEM_READER_OWNED) &&
>> -        (rcnt > 1) && !(count & RWSEM_WRITER_LOCKED))
>> +    if ((atomic_long_read(&sem->owner) & RWSEM_READER_OWNED) && (rcnt > 1))
>>           goto queue;
>>         /*
> 
> Unlike RWSEM_WRITER_LOCKED bit in count, the RWSEM_READER_OWNED bit in owner is just a hint, not an authoritative state of the rwsem. So it is possible that both the RWSEM_READER_OWNED bit can be set in owner and RWSEM_WRITER_LOCKED bit set in count in a transition period right after RWSEM_WRITER_LOCKED bit is set. 

reader		writer					reader

acquire		
release		
		rwsem_write_trylock
			set RWSEM_WRITER_LOCKED
							rwsem_down_read_slowpath
			set owner

If prev lock holder is a reader, when it releases the lock, the owner isn't cleared(CONFIG_DEBUG_RWSEMS isn't enabled).
A writer comes and can set the RWSEM_WRITER_LOCKED bit succsessfully, then a new reader run into slow path, before
the writer set the owner, the new reader will see that both the RWSEM_READER_OWNED bit and RWSEM_WRITER_LOCKED bit are
set.

So the above sequence could be the case, right?



So the RWSEM_WRITER_LOCKED check can still provide some value. We should probably update the comment to reflect that.
> 
> Cheers,
> Longman
>