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

[Waiman Long <longman@redhat.com>]

On 3/17/26 5:23 PM, Andrei Vagin wrote:
> On Mon, Mar 16, 2026 at 12:10 PM Waiman Long <longman@redhat.com> wrote:
>> 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;
> We still need `return` here.
>
>>           }
>>
>>           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))
> I am ok with this approach too. Do you want to send this patch?

Sure. I will send out later tonight or tomorrow morning.

Cheers,
Longman