linux-kernel - Re: [PATCH v2 2/2] PM / sleep: don't suspend parent when async child suspend

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: <20161102050706.GA49402@google.com>
Date:   Tue, 1 Nov 2016 22:07:07 -0700
From:   Brian Norris <briannorris@...omium.org>
To:     "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Pavel Machek <pavel@....cz>, Len Brown <len.brown@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Doug Anderson <dianders@...omium.org>,
        Brian Norris <computersforpeace@...il.com>,
        Jeffy Chen <jeffy.chen@...k-chips.com>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        Chuansheng Liu <chuansheng.liu@...el.com>,
        Kevin Hilman <khilman@...nel.org>,
        Ulf Hansson <ulf.hansson@...aro.org>
Subject: Re: [PATCH v2 2/2] PM / sleep: don't suspend parent when async child
 suspend_{noirq,late} fails

+ more genpd folks

On Wed, Nov 02, 2016 at 04:51:08AM +0100, Rafael J. Wysocki wrote:
> On Tuesday, November 01, 2016 12:04:28 AM Dmitry Torokhov wrote:
> > On Mon, Oct 31, 2016 at 10:25 PM, Rafael J. Wysocki <rjw@...ysocki.net> wrote:
> > > On Thursday, October 27, 2016 09:05:34 AM Brian Norris wrote:
> > >> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > >> index c58563581345..eaf6b53463a5 100644
> > >> --- a/drivers/base/power/main.c
> > >> +++ b/drivers/base/power/main.c
> > >> @@ -1040,6 +1040,9 @@ static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool a
> > >>
> > >>       dpm_wait_for_children(dev, async);
> > >>
> > >> +     if (async_error)
> > >> +             goto Complete;
> > >> +
> > >
> > > This is a second chech for async_error in this routine and is the first one
> > > really needed after adding this?
> > 
> > There is really no point in waiting for children to be suspended if
> > error has already been signalled; that's what first check achieves.
> > The 2nd check ensures that we abort suspend if any of the children
> > failed to suspend.
> > 
> > I'd say both checks are needed (well, 1st is helpful, 2nd is essential).
> 
> OK, fair enough.

Sort of agreed, although I'm still not sure how helpful the 1st one is;
kinda serves to complicate things, for little real benefit IMO (you
don't save much time by "not waiting" -- either the child quickly
notices the same error and complete()'s quickly, or else you're going to
wait for that child in the end anyway).

I think it's also important to ask why we do this optimization in the
{late,noirq} cases, but we don't do this in __device_suspend(). As
demonstrated by the $subject bug, I think we would yield fewer bugs by
sharing code structure (if not the code itself) among the similar
phases.

I'm happy for you to take my current patch, of course, but I think some
further effort on making this consistent might be warranted. Either put
all of these short-circuit checks after the wait_for_children(), or else
add the same short-circuit for the missing case (__device_suspend()).
i.e., this (untested) patch:

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index e44944f4be77..2932a5bd892f 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1027,6 +1027,8 @@ static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool a
 	TRACE_DEVICE(dev);
 	TRACE_SUSPEND(0);

+	dpm_wait_for_children(dev, async);
+
 	if (async_error)
 		goto Complete;

@@ -1038,8 +1040,6 @@ static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool a
 	if (dev->power.syscore || dev->power.direct_complete)
 		goto Complete;

-	dpm_wait_for_children(dev, async);
-
 	if (dev->pm_domain) {
 		info = "noirq power domain ";
 		callback = pm_noirq_op(&dev->pm_domain->ops, state);
@@ -1174,6 +1174,8 @@ static int __device_suspend_late(struct device *dev, pm_message_t state, bool as

 	__pm_runtime_disable(dev, false);

+	dpm_wait_for_children(dev, async);
+
 	if (async_error)
 		goto Complete;

@@ -1185,8 +1187,6 @@ static int __device_suspend_late(struct device *dev, pm_message_t state, bool as
 	if (dev->power.syscore || dev->power.direct_complete)
 		goto Complete;

-	dpm_wait_for_children(dev, async);
-
 	if (dev->pm_domain) {
 		info = "late power domain ";
 		callback = pm_late_early_op(&dev->pm_domain->ops, state);

---

I can test this and send it in proper form if that looks preferable.

P.S. To get slightly off-topic here (but speaking of noirq bugs): I
noticed the genpd code has comments like this scattered all over:

 * This function is only called in "noirq" and "syscore" stages of system power
 * transitions, so it need not acquire locks (all of the "noirq" callbacks are
 * executed sequentially, so it is guaranteed that it will never run twice in
 * parallel).

Isn't that no longer true, now that noirq suspend can be asynchronous?
Maybe we should grep for the phrase "need not acquire locks" throughout
the kernel, in order to find low-hanging fruit for race conditions :)