Date:   Thu, 2 Mar 2023 16:32:53 -0500
From:   Steven Rostedt <rostedt@goodmis.org>
To:     John Stultz <jstultz@google.com>
Cc:     LKML <linux-kernel@vger.kernel.org>, Wei Wang <wvw@google.com>,
        Midas Chien <midaschieh@google.com>,
        Kees Cook <keescook@chromium.org>,
        Anton Vorontsov <anton@enomsg.org>,
        "Guilherme G. Piccoli" <gpiccoli@igalia.com>,
        Tony Luck <tony.luck@intel.com>, kernel-team@android.com,
        Thomas Gleixner <tglx@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Subject: Re: [PATCH] pstore: Revert pmsg_lock back to a normal mutex
Message-ID: <20230302163253.541ac3a8@gandalf.local.home>
In-Reply-To: <20230302152103.2618f1b7@gandalf.local.home>
References: <20230302062741.483079-1-jstultz@google.com>
        <20230302082414.77613351@gandalf.local.home>
        <CANDhNCo4ruC4pP+iDe49b3e1nAcWtYQj4bx82+oZhyLFYkdFJQ@mail.gmail.com>
        <20230302152103.2618f1b7@gandalf.local.home>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 2 Mar 2023 15:21:03 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> I could possibly add a patch, and see if that also works.

Can you try this patch to see if it improves the situation.

A few of things about this patch. It is lightly tested. It can be optimized
to cache the top waiter and not need to grab the spin lock and disable
interrupts for every loop, but right now I want to see if this improves the
situation. As when PREEMPT_RT becomes more mainline, we may need this.

Another thing I noticed is I think there's a bug in the existing code.


   CPU1					CPU2
   ----					----
rt_mutex_slowlock_block() {
  raw_spin_lock_irq(wait_lock);
  owner = rt_mutex_owner();
  raw_spin_unlock_irq(wait_lock);

  rtmutex_spin_on_owner(owner) {
    owner = rt_mutex_owner();

    [ task preempted! (could also be a long interrupt) ]

				   owner releases lock and exits
				   owner is freed

    [ task resumes ]

    if (!owner_on_cpu(owner)

      READ_ONCE(owner->on_cpu)
     *** BOOM invalid pointer dereference ***

I think we need a get_task_struct() somewhere there.

Anyway, that's another issue. Could you try this patch? I even added a
trace_printk() in there to see if it gets hit.

Thanks!

-- Steve

diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 010cf4e6d0b8..6c602775bb23 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1399,6 +1399,7 @@ static bool rtmutex_spin_on_owner(struct rt_mutex_base *lock,
 				  struct rt_mutex_waiter *waiter,
 				  struct task_struct *owner)
 {
+	struct rt_mutex_waiter *top_waiter;
 	bool res = true;
 
 	rcu_read_lock();
@@ -1421,11 +1422,25 @@ static bool rtmutex_spin_on_owner(struct rt_mutex_base *lock,
 		 *    for CONFIG_PREEMPT_RCU=y)
 		 *  - the VCPU on which owner runs is preempted
 		 */
-		if (!owner_on_cpu(owner) || need_resched() ||
-		    !rt_mutex_waiter_is_top_waiter(lock, waiter)) {
+		if (!owner_on_cpu(owner) || need_resched()) {
 			res = false;
 			break;
 		}
+		top_waiter = rt_mutex_top_waiter(lock);
+		if (top_waiter != waiter) {
+			raw_spin_lock_irq(&lock->wait_lock);
+			top_waiter = rt_mutex_top_waiter(lock);
+			if (top_waiter && top_waiter != waiter) {
+				trace_printk("spin on waiter! %s:%d\n",
+					     top_waiter->task->comm,
+					     top_waiter->task->pid);
+				if (!owner_on_cpu(top_waiter->task))
+					res = false;
+			}
+			raw_spin_unlock_irq(&lock->wait_lock);
+			if (!res)
+				break;
+		}
 		cpu_relax();
 	}
 	rcu_read_unlock();