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

Andrei Vagin
2026-03-17T21:24:07Z

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?

Thanks,
Andrei