linux-kernel - Re: [PATCH v2] PM: runtime: avoid priority inversion on PREEMPT

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] [day] [month] [year] [list]

Message-ID: <20211117192520.6b085a1e.john@metanate.com>
Date:   Wed, 17 Nov 2021 19:25:20 +0000
From:   John Keeping <john@...anate.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     Linux PM <linux-pm@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...nel.org>, Pavel Machek <pavel@....cz>,
        Len Brown <len.brown@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] PM: runtime: avoid priority inversion on PREEMPT_RT

On Wed, 17 Nov 2021 19:53:47 +0100
"Rafael J. Wysocki" <rafael@...nel.org> wrote:

> On Wed, Nov 17, 2021 at 7:37 PM John Keeping <john@...anate.com> wrote:
> >
> > With PREEMPT_RT the cpu_relax() loops in rpm_suspend and rpm_resume can
> > cause unbounded latency if they preempt an asynchronous suspend.  The
> > main scenario where this can happen is when a realtime thread resumes a
> > device while it is asynchronously suspending on a worker thread.
> >
> > I'm not convinced this can actually happen in the rpm_suspend case, or
> > at least it's a lot less likely for a synchronous suspend to run at the
> > same time as an asynchronous suspend, but both functions are updated
> > here for symmetry.
> >
> > For devices setting power.irq_safe, it is possible that RPM functions
> > will be called with a spinlock held (for example in
> > pl330_issue_pending()).  This means a normal call to schedule() can't be
> > used, but to avoid the priority inversion it is necessary to wait and
> > schedule.  schedule_rtlock() is only available when CONFIG_PREEMPT_RT is
> > defined, so even though the logic is correct without any preprocessor
> > guards around schedule_rtlock(), they are necessary for compilation.
> >
> > Cc: Thomas Gleixner <tglx@...utronix.de>
> > Cc: Peter Zijlstra (Intel) <peterz@...radead.org>
> > Cc: Ingo Molnar <mingo@...nel.org>
> > Signed-off-by: John Keeping <john@...anate.com>
> > ---
> > Changes since v1:
> > - Use schedule_rtlock() instead of schedule() for PREEMPT_RT & irq_safe
> > - Rewritten commit description
> >
> >  drivers/base/power/runtime.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index f3de7bfc7f5b..fdf461bfae8c 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -596,7 +596,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
> >                         goto out;
> >                 }
> >
> > -               if (dev->power.irq_safe) {
> > +               if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {  
> 
> Please add a helper to avoid code duplication related to this (even
> though there is a small amount of it).

Ack.  I'd like some feedback on the schedule_rtlock() approach from the
scheduler & RT people, so I'll wait a bit before sending a v3.