Received-SPF: pass (google.com: domain of linux-kernel+bounces-26392-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 15 Jan 2024 19:33:38 +0100
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To: Jiri Slaby <jirislaby@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
	boqun.feng@gmail.com, bristot@redhat.com, bsegall@google.com,
	dietmar.eggemann@arm.com, jstultz@google.com, juri.lelli@redhat.com,
	longman@redhat.com, mgorman@suse.de, mingo@redhat.com,
	rostedt@goodmis.org, swood@redhat.com, vincent.guittot@linaro.org,
	vschneid@redhat.com, will@kernel.org
Subject: Re: [PATCH v3 7/7] locking/rtmutex: Acquire the hb lock via trylock
 after wait-proxylock.
Message-ID: <20240115183338.xW3GS3Ex@linutronix.de>
References: <20230908162254.999499-1-bigeasy@linutronix.de>
 <20230908162254.999499-8-bigeasy@linutronix.de>
 <20230911141135.GB9098@noisy.programming.kicks-ass.net>
 <87fs3f1tl0.ffs@tglx>
 <20230915151943.GD6743@noisy.programming.kicks-ass.net>
 <4611bcf2-44d0-4c34-9b84-17406f881003@kernel.org>
 <9f75eb59-9b7a-4b49-9081-e6a3cbb00187@kernel.org>
 <c737a604-d441-49c6-a5cd-ef01e9f2a454@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <c737a604-d441-49c6-a5cd-ef01e9f2a454@kernel.org>

On 2024-01-15 13:54:32 [+0100], Jiri Slaby wrote:
> A simplified reproducer attached (in particular, no APR anymore). Build with
> -pthread, obviously. If you see
> BADx rv=22
> 
> that's bad.

So task1 owns the futex, task2 does lock_pi_futex(). task2 setups the pi
state and everything for task1. Then task1 times out on the lock and
does not want the lock no more. And we break user state vs kernel via:


task1			task2

futex_unlock_pi();	futex_lock_pi();
			rt_mutex_wait_proxy_lock() // ETIMEDOUT
spin_lock(&hb->lock);
			rt_mutex_cleanup_proxy_lock()
			/* now the lock is still owned by task1, and the
			 * task2 removed itself as the waiter but its
			 * futex_q is still queued
			 */
			spin_lock(&hb->lock); /* block */

top_waiter = futex_top_waiter(hb, &key);
/* top_wait is task2's */
rt_waiter = rt_mutex_top_waiter(&pi_state->pi_mutex);
/* rt_waiter is NULL and the 
   futex is unlocked in userland via uaddr 
*/

and now

task 3			task4
locks in userland
			futex_lock_pi();
			futex_lock_pi_atomic();
			   -EINVAL = attach_to_pi_state()
			   /*
			    * becauase pi_state says owner
			    * is task1 but uaddr says task3.
			    */

\*/

This is due to the new lock ordering and the guarantees we no longer
have since the commit cited. The pi-state is cleaned/ removed by the last
one that wants the lock so in the unlock path there is either pi-state
with a waiter or nothing.

This duct tape at the end waits until the pi-state leaves or we get a
waiter. So this works but is not a fix.

diff --git a/kernel/futex/pi.c b/kernel/futex/pi.c
index 90e5197f4e56..f504ae864cc9 100644
--- a/kernel/futex/pi.c
+++ b/kernel/futex/pi.c
@@ -1182,6 +1182,9 @@ int futex_unlock_pi(u32 __user *uaddr, unsigned int flags)
 		rt_waiter = rt_mutex_top_waiter(&pi_state->pi_mutex);
 		if (!rt_waiter) {
 			raw_spin_unlock_irq(&pi_state->pi_mutex.wait_lock);
+			spin_unlock(&hb->lock);
+			cpu_relax();
+			goto retry;
 			goto do_uncontended;
 		}
 
-- 
2.43.0

> regards,

Sebastian