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

[Waiman Long]

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

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; } 
if (sem->first_waiter == waiter) { @@ -378,8 +377,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 +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)) return true; 
atomic_long_andnot(RWSEM_FLAG_HANDOFF | RWSEM_FLAG_WAITERS, 
&sem->count); return false;

Cheers, Longman