Re: [PATCH] locking/rwsem: Fix logic error in rwsem_del_waiter()

[Waiman Long <longman@redhat.com>]

On 3/16/26 1:49 PM, Waiman Long wrote:
> On 3/16/26 1:34 PM, Waiman Long wrote:
>> On 3/14/26 2:26 PM, Andrei Vagin wrote:
>>> Commit 1ea4b473504b ("locking/rwsem: Remove the list_head from struct
>>> rw_semaphore") introduced a logic error in rwsem_del_waiter().
>>>
>>> The root cause of this issue is an inconsistency in the return 
>>> values of
>>> __rwsem_del_waiter() and rwsem_del_waiter(). Specifically,
>>> __rwsem_del_waiter() returns true when the wait list becomes empty,
>>> whereas rwsem_del_waiter() is supposed to return true if the wait list
>>> is NOT empty.
>>>
>>> This caused a null pointer dereference in rwsem_mark_wake() because it
>>> was being called when sem->first_waiter was NULL.
>>>
>>> Cc: Matthew Wilcox (Oracle)<willy@infradead.org>
>>> Reported-by:syzbot+3d2ff92c67127d337463@syzkaller.appspotmail.com
>>> Tested-by:syzbot+3d2ff92c67127d337463@syzkaller.appspotmail.com
>>> Fixes: 1ea4b473504b ("locking/rwsem: Remove the list_head from 
>>> struct rw_semaphore")
>>> Signed-off-by: Andrei Vagin<avagin@google.com>
>>> ---
>>>   kernel/locking/rwsem.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
>>> index ba4cb74de064..bf647097369c 100644
>>> --- a/kernel/locking/rwsem.c
>>> +++ b/kernel/locking/rwsem.c
>>> @@ -370,7 +370,7 @@ bool __rwsem_del_waiter(struct rw_semaphore 
>>> *sem, struct rwsem_waiter *waiter)
>>>   {
>>>       if (list_empty(&waiter->list)) {
>>>           sem->first_waiter = NULL;
>>> -        return true;
>>> +        return false;
>>>       }
>>>         if (sem->first_waiter == waiter) {
>>> @@ -379,7 +379,7 @@ bool __rwsem_del_waiter(struct rw_semaphore 
>>> *sem, struct rwsem_waiter *waiter)
>>>       }
>>>       list_del(&waiter->list);
>>>   -    return false;
>>> +    return true;
>>>   }
>>>     /*
>>
>> It will be better if we also document what does the return value of 
>> __rwsem_del_waiter() means as the we can't guess from the function 
>> name itself. Other that that,
>>
>> Reviewed-by: Waiman Long <longman@redhat.com>
>>
> Thinking a bit more about it. I think it will be better to not return 
> a value in __rwsem_del_waiter() at all. Something like
>
Sorry, my mailer screwed up the diff. It should be as follows.

Cheers,
Longman

=================================[ Cut here ]============================
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index ba4cb74de064..ce57ad3c1120 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -365,12 +365,11 @@ enum rwsem_wake_type {
  #define MAX_READERS_WAKEUP     0x100
  
  static inline
-bool __rwsem_del_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter)
+void __rwsem_del_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter)
         __must_hold(&sem->wait_lock)
  {
         if (list_empty(&waiter->list)) {
                 sem->first_waiter = NULL;
-               return true;
         }
  
         if (sem->first_waiter == waiter) {
@@ -378,8 +377,6 @@ bool __rwsem_del_waiter(struct rw_semaphore *sem, struct rw>
                                                      struct rwsem_waiter, list);
         }
         list_del(&waiter->list);
-
-       return false;
  }
  
  /*
@@ -394,7 +391,8 @@ static inline bool
  rwsem_del_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter)
  {
         lockdep_assert_held(&sem->wait_lock);
-       if (__rwsem_del_waiter(sem, waiter))
+       __rwsem_del_waiter(sem, waiter);
+       if (rwsem_is_contended(sem))
                 return true;
         atomic_long_andnot(RWSEM_FLAG_HANDOFF | RWSEM_FLAG_WAITERS, &sem->count>
         return false;