[PATCH] locking/rwsem: Fix improper return value of __rwsem_del_waiter()

[PATCH] locking/rwsem: Fix improper return value of __rwsem_del_waiter()

[Waiman Long]

Commit 1ea4b473504b ("locking/rwsem: Remove the list_head from struct
rw_semaphore") introduces a new __rwsem_del_waiter() which return
true if the wait list is going to be empty after deletion and false
otherwise. However this return value is the exact oppsite of the value
returned by rwsem_del_waiter() and __rwsem_del_waiter() is used as if
its return value matches that of rwsem_del_waiter().

This caused a null pointer dereference in rwsem_mark_wake() because it
was being called when sem->first_waiter was NULL.

Andrei sent a patch [1] to reverse the polarity of the return value to
match that of rwsem_del_waiter() which can fix this bug. To make it
better, I believe we should either put the same return value comment
at the top of __rwsem_del_waiter() to make this clear or don't return
a value at all.

This patch adopts the later approach by making __rwsem_del_waiter()
a void function and using rwsem_is_contended() to check if the wait
list is empty or not. This will make the code more readable.

Below is the size comparison of the gcc compiled rwsem.o object files.

             text    data     bss     dec     hex filename
Before       6791     696       0    7487    1d3f kernel/locking/rwsem.o
Andrei patch 6887     696       0    7583    1d9f kernel/locking/rwsem.o
This patch   6823     696       0    7519    1d5f kernel/locking/rwsem.o

So this patch isn't bad from the size perspective.

[1] https://lore.kernel.org/lkml/20260314182607.3343346-1-avagin@google.com/

Reported-by: syzbot+3d2ff92c67127d337463@syzkaller.appspotmail.com
Fixes: 1ea4b473504b ("locking/rwsem: Remove the list_head from struct rw_semaphore")
Signed-off-by: Waiman Long <longman@redhat.com>
---
 kernel/locking/rwsem.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index ba4cb74de064..e8fee8cc933f 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -365,12 +365,12 @@ 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;
+		return;
 	}
 
 	if (sem->first_waiter == waiter) {
@@ -378,8 +378,6 @@ bool __rwsem_del_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter)
 						     struct rwsem_waiter, list);
 	}
 	list_del(&waiter->list);
-
-	return false;
 }
 
 /*
@@ -394,7 +392,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;
-- 
2.53.0

Re: [PATCH] locking/rwsem: Fix improper return value of __rwsem_del_waiter()

[Andrei Vagin]

On Tue, Mar 17, 2026 at 8:25 PM Waiman Long <longman@redhat.com> wrote:
>
> Commit 1ea4b473504b ("locking/rwsem: Remove the list_head from struct
> rw_semaphore") introduces a new __rwsem_del_waiter() which return
> true if the wait list is going to be empty after deletion and false
> otherwise. However this return value is the exact oppsite of the value
> returned by rwsem_del_waiter() and __rwsem_del_waiter() is used as if
> its return value matches that of rwsem_del_waiter().
>
> This caused a null pointer dereference in rwsem_mark_wake() because it
> was being called when sem->first_waiter was NULL.
>
> Andrei sent a patch [1] to reverse the polarity of the return value to
> match that of rwsem_del_waiter() which can fix this bug. To make it
> better, I believe we should either put the same return value comment
> at the top of __rwsem_del_waiter() to make this clear or don't return
> a value at all.
>
> This patch adopts the later approach by making __rwsem_del_waiter()
> a void function and using rwsem_is_contended() to check if the wait
> list is empty or not. This will make the code more readable.
>
> Below is the size comparison of the gcc compiled rwsem.o object files.
>
>              text    data     bss     dec     hex filename
> Before       6791     696       0    7487    1d3f kernel/locking/rwsem.o
> Andrei patch 6887     696       0    7583    1d9f kernel/locking/rwsem.o
> This patch   6823     696       0    7519    1d5f kernel/locking/rwsem.o
>
> So this patch isn't bad from the size perspective.
>
> [1] https://lore.kernel.org/lkml/20260314182607.3343346-1-avagin@google.com/
>
> Reported-by: syzbot+3d2ff92c67127d337463@syzkaller.appspotmail.com
> Fixes: 1ea4b473504b ("locking/rwsem: Remove the list_head from struct rw_semaphore")
> Signed-off-by: Waiman Long <longman@redhat.com>

Acked-by: Andrei Vagin <avagin@google.com>

Thanks,
Andrei