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

[Andrei Vagin]

On Mon, Mar 16, 2026 at 12:10 PM Waiman Long <longman@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
> >>> Reported-by:syzbot+3d2ff92c67127d337463@xxxxxxxxxxxxxxxxxxxxxxxxx
> >>> Tested-by:syzbot+3d2ff92c67127d337463@xxxxxxxxxxxxxxxxxxxxxxxxx
> >>> Fixes: 1ea4b473504b ("locking/rwsem: Remove the list_head from
> >>> struct rw_semaphore")
> >>> Signed-off-by: Andrei Vagin<avagin@xxxxxxxxxx>
> >>> ---
> >>>   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@xxxxxxxxxx>
> >>
> > 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