linux-kernel - Re: [PATCH v11 1/7] locking/mutex: Remove wakeups from under mutex::wait

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANDhNCoTnsUXU9iHj=gW9fhSZs4BZs8UVdQck_N=3-M-NHDdbg@mail.gmail.com>
Date: Fri, 12 Jul 2024 12:53:38 -0700
From: John Stultz <jstultz@...gle.com>
To: K Prateek Nayak <kprateek.nayak@....com>
Cc: LKML <linux-kernel@...r.kernel.org>, Peter Zijlstra <peterz@...radead.org>, 
	Joel Fernandes <joelaf@...gle.com>, Qais Yousef <qyousef@...alina.io>, Ingo Molnar <mingo@...hat.com>, 
	Juri Lelli <juri.lelli@...hat.com>, Vincent Guittot <vincent.guittot@...aro.org>, 
	Dietmar Eggemann <dietmar.eggemann@....com>, Valentin Schneider <vschneid@...hat.com>, 
	Steven Rostedt <rostedt@...dmis.org>, Ben Segall <bsegall@...gle.com>, 
	Zimuzo Ezeozue <zezeozue@...gle.com>, Youssef Esmat <youssefesmat@...gle.com>, 
	Mel Gorman <mgorman@...e.de>, Will Deacon <will@...nel.org>, Waiman Long <longman@...hat.com>, 
	Boqun Feng <boqun.feng@...il.com>, "Paul E. McKenney" <paulmck@...nel.org>, 
	Metin Kaya <Metin.Kaya@....com>, Xuewen Yan <xuewen.yan94@...il.com>, 
	Thomas Gleixner <tglx@...utronix.de>, Daniel Lezcano <daniel.lezcano@...aro.org>, kernel-team@...roid.com, 
	Davidlohr Bueso <dave@...olabs.net>
Subject: Re: [PATCH v11 1/7] locking/mutex: Remove wakeups from under mutex::wait_lock

On Wed, Jul 10, 2024 at 10:42 AM 'K Prateek Nayak' via kernel-team
<kernel-team@...roid.com> wrote:
> On 7/10/2024 2:01 AM, John Stultz wrote:
> > @@ -681,6 +682,11 @@ __mutex_lock_common(struct mutex *lock, unsigned int state, unsigned int subclas
> >               }
> >
> >               raw_spin_unlock(&lock->wait_lock);
> > +             /* Make sure we do wakeups before calling schedule */
> > +             if (!wake_q_empty(&wake_q)) {
>
> nit.
>
> This checks seems unnecessary (to my untrained eye). Any harm in
> skipping it and simply doing a wake_up_q() followed by wake_q_init()
> unconditionally?
>
> > +                     wake_up_q(&wake_q);
> > +                     wake_q_init(&wake_q);
> > +             }
> >               schedule_preempt_disabled();

Ah, thank you for the suggestion. I've reworked this in my tree!

> > @@ -1207,6 +1209,7 @@ static int __sched task_blocks_on_rt_mutex(struct rt_mutex_base *lock,
> >       struct rt_mutex_waiter *top_waiter = waiter;
> >       struct rt_mutex_base *next_lock;
> >       int chain_walk = 0, res;
> > +     DEFINE_WAKE_Q(wake_q);
> >
> >       lockdep_assert_held(&lock->wait_lock);
> >
> > @@ -1245,7 +1248,10 @@ static int __sched task_blocks_on_rt_mutex(struct rt_mutex_base *lock,
> >
> >               /* Check whether the waiter should back out immediately */
> >               rtm = container_of(lock, struct rt_mutex, rtmutex);
> > -             res = __ww_mutex_add_waiter(waiter, rtm, ww_ctx);
> > +             preempt_disable();
> > +             res = __ww_mutex_add_waiter(waiter, rtm, ww_ctx, &wake_q);
> > +             wake_up_q(&wake_q);
> > +             preempt_enable();
>
> I'm trying to understand this - we enter task_blocks_on_rt_mutex() with
> "wait_lock" held (I believe the lockdep_assert_held() in the previous
> hunk checks for the same). I walked down the call chain (although
> briefly) and could only spot "task->pi_lock" being locked and unlocked
> before this call to "wake_up_q()" but the "wait_lock" seems to be held
> throughout, only being unlocked and locked again for
> "rt_mutex_adjust_prio_chain()" later down.
>
> Did I miss something or is disabling preemption for this specific hunk
> enough to enable safe nesting?

Thank you for pointing this out! It looks like I need to pipe the
wake_q down through task_blocks_on_rt_mutex(), and
rtlock_slowlock_locked() and define one in rtlock_slowlock().

Really appreciate the review and feedback here!
-john