[PATCH] futex/requeue: Revert "Prevent NULL pointer dereference in remove_waiter() on self-deadlock""

[PATCH] futex/requeue: Revert "Prevent NULL pointer dereference in remove_waiter() on self-deadlock""

[Sebastian Andrzej Siewior]

The commit cited below should not have been merged. It did not fix an
existing problem but it introduced new problems by keeping the pi_state
in state Q_REQUEUE_PI_IN_PROGRESS and leaking it.

Based on the commit description the intention was to handle the case
when task_blocks_on_rt_mutex() returns -EDEADLK and the following
remove_waiter() dereferences the NULL pointer in waiter->task.

That has been already handled by Davidlohr in commit 40a25d59e85b3
("locking/rtmutex: Skip remove_waiter() when waiter is not enqueued")
and requires no further acting.

Revert the commit breaking the "waiter == owner" case again.

Fixes: 74e144274af39 ("futex/requeue: Prevent NULL pointer dereference in remove_waiter() on self-deadlock")
Reported-by: Michael Bommarito <michael.bommarito@gmail.com>
Closes: https://lore.kernel.org/all/20260629020049.2082397-1-michael.bommarito@gmail.com
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/futex/requeue.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/kernel/futex/requeue.c b/kernel/futex/requeue.c
index 7384672916fb6..79823ad136830 100644
--- a/kernel/futex/requeue.c
+++ b/kernel/futex/requeue.c
@@ -645,12 +645,6 @@ int futex_requeue(u32 __user *uaddr1, unsigned int flags1,
 				continue;
 			}
 
-			/* Self-deadlock: non-top waiter already owns the PI futex. */
-			if (rt_mutex_owner(&pi_state->pi_mutex) == this->task) {
-				ret = -EDEADLK;
-				break;
-			}
-
 			ret = rt_mutex_start_proxy_lock(&pi_state->pi_mutex,
 							this->rt_waiter,
 							this->task);
-- 
2.53.0

Re: [PATCH] futex/requeue: Revert "Prevent NULL pointer dereference in remove_waiter() on self-deadlock""

[Thomas Gleixner]

On Wed, Jul 01 2026 at 15:11, Sebastian Andrzej Siewior wrote:
> The commit cited below should not have been merged. It did not fix an
> existing problem but it introduced new problems by keeping the pi_state
> in state Q_REQUEUE_PI_IN_PROGRESS and leaking it.
>
> Based on the commit description the intention was to handle the case
> when task_blocks_on_rt_mutex() returns -EDEADLK and the following
> remove_waiter() dereferences the NULL pointer in waiter->task.
>
> That has been already handled by Davidlohr in commit 40a25d59e85b3
> ("locking/rtmutex: Skip remove_waiter() when waiter is not enqueued")
> and requires no further acting.

No. If the self deadlock is obvious, then this should not even go into
the proxy lock code. I clearly failed to notice the leak problem, but
reverting it and relying on some magic down the road to handle it
correctly is not really solid.

The below should fix it properly.

---
--- a/kernel/futex/requeue.c
+++ b/kernel/futex/requeue.c
@@ -646,15 +646,13 @@ int futex_requeue(u32 __user *uaddr1, un
 			}
 
 			/* Self-deadlock: non-top waiter already owns the PI futex. */
-			if (rt_mutex_owner(&pi_state->pi_mutex) == this->task) {
+			if (likely(rt_mutex_owner(&pi_state->pi_mutex) != this->task)) {
+				ret = rt_mutex_start_proxy_lock(&pi_state->pi_mutex,
+								this->rt_waiter, this->task);
+			} else {
 				ret = -EDEADLK;
-				break;
 			}
 
-			ret = rt_mutex_start_proxy_lock(&pi_state->pi_mutex,
-							this->rt_waiter,
-							this->task);
-
 			if (ret == 1) {
 				/*
 				 * We got the lock. We do neither drop the refcount

Re: [PATCH] futex/requeue: Revert "Prevent NULL pointer dereference in remove_waiter() on self-deadlock""

[Sebastian Andrzej Siewior]

On 2026-07-01 17:25:05 [+0200], Thomas Gleixner wrote:
> > That has been already handled by Davidlohr in commit 40a25d59e85b3
> > ("locking/rtmutex: Skip remove_waiter() when waiter is not enqueued")
> > and requires no further acting.
> 
> No. If the self deadlock is obvious, then this should not even go into
> the proxy lock code. I clearly failed to notice the leak problem, but
> reverting it and relying on some magic down the road to handle it
> correctly is not really solid.

Davidlohr fixed the issue that has been fixed again. We don't need two
changes for the same thing.
We have deadlock detection down the road, why special case it here?

> The below should fix it properly.
>
Sebastian

Re: [PATCH] futex/requeue: Revert "Prevent NULL pointer dereference in remove_waiter() on self-deadlock""

[Thomas Gleixner]

On Wed, Jul 01 2026 at 17:45, Sebastian Andrzej Siewior wrote:

> On 2026-07-01 17:25:05 [+0200], Thomas Gleixner wrote:
>> > That has been already handled by Davidlohr in commit 40a25d59e85b3
>> > ("locking/rtmutex: Skip remove_waiter() when waiter is not enqueued")
>> > and requires no further acting.
>> 
>> No. If the self deadlock is obvious, then this should not even go into
>> the proxy lock code. I clearly failed to notice the leak problem, but
>> reverting it and relying on some magic down the road to handle it
>> correctly is not really solid.
>
> Davidlohr fixed the issue that has been fixed again. We don't need two
> changes for the same thing.

He hardened the remove() function against that scenario.

> We have deadlock detection down the road, why special case it here?

It's not special, but there is no point to actucally call into complex
and as demonstrated fragile code when you can avoid it upfront, no?

Re: [PATCH] futex/requeue: Revert "Prevent NULL pointer dereference in remove_waiter() on self-deadlock""

[Sebastian Andrzej Siewior]

On 2026-07-01 21:13:21 [+0200], Thomas Gleixner wrote:
> On Wed, Jul 01 2026 at 17:45, Sebastian Andrzej Siewior wrote:
> 
> > On 2026-07-01 17:25:05 [+0200], Thomas Gleixner wrote:
> >> > That has been already handled by Davidlohr in commit 40a25d59e85b3
> >> > ("locking/rtmutex: Skip remove_waiter() when waiter is not enqueued")
> >> > and requires no further acting.
> >> 
> >> No. If the self deadlock is obvious, then this should not even go into
> >> the proxy lock code. I clearly failed to notice the leak problem, but
> >> reverting it and relying on some magic down the road to handle it
> >> correctly is not really solid.
> >
> > Davidlohr fixed the issue that has been fixed again. We don't need two
> > changes for the same thing.
> 
> He hardened the remove() function against that scenario.
> 
> > We have deadlock detection down the road, why special case it here?
> 
> It's not special, but there is no point to actucally call into complex
> and as demonstrated fragile code when you can avoid it upfront, no?

It has been like that for since forever. So it is a tested path.  If it
wouldn't be for commit 3bfdc63936dd4 ("rtmutex: Use waiter::task instead
of current in remove_waiter()") then none of these would be needed and
we would still the deadlock detection in one place.  It is just
unfortunate that had two fixes for the same thing "recently".

For the timeline:
| 8161239a8bcce ("rtmutex: Simplify PI algorithm and make highest prio task get lock")
| Date:   Fri Jan 14 17:09:41 2011 +0800
|
| 3bfdc63936dd4 ("rtmutex: Use waiter::task instead of current in remove_waiter()")
| Date:   Wed Apr 8 16:46:00 2026 +0800
|
| 40a25d59e85b3 ("locking/rtmutex: Skip remove_waiter() when waiter is not enqueued")
| Date:   Thu May 7 04:29:13 2026 -0700
|
| 74e144274af39 ("futex/requeue: Prevent NULL pointer dereference in remove_waiter() on self-deadlock")
| Date:   Tue Jun 2 09:12:04 2026 +0000

So July is the next futex fix patch for sure ;)

I will try to add a function test for these two paths but I'm already
behind on my other futex thingy so I can't promise anything.

Sebastian