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

[Waiman Long]

On 3/17/26 5:23 PM, Andrei Vagin wrote:
> 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?

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

Cheers,
Longman