linux-kernel - Re: [PATCH v1] cpuidle: Do not return from cpuidle_play

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: <CAJZ5v0jkPb-sgrMfk+KN19+R+ezucssHKUzfJZ74Qw1Ned6gaw@mail.gmail.com>
Date: Fri, 15 Nov 2024 14:25:23 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Peter Zijlstra <peterz@...radead.org>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>, Linux PM <linux-pm@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, x86 Maintainers <x86@...nel.org>, 
	Patryk Wlazlyn <patryk.wlazlyn@...ux.intel.com>, 
	"Gautham R. Shenoy" <gautham.shenoy@....com>, 
	Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>, 
	Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH v1] cpuidle: Do not return from cpuidle_play_dead() on
 callback failures

On Fri, Nov 15, 2024 at 1:55 PM Peter Zijlstra <peterz@...radead.org> wrote:
>
> On Fri, Nov 15, 2024 at 01:46:29PM +0100, Rafael J. Wysocki wrote:
> > On Fri, Nov 15, 2024 at 11:14 AM Peter Zijlstra <peterz@...radead.org> wrote:
> > >
> > > On Thu, Nov 14, 2024 at 06:46:20PM +0100, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > > >
> > > > If the :enter_dead() idle state callback fails for a certain state,
> > > > there may be still a shallower state for which it will work.
> > > >
> > > > Because the only caller of cpuidle_play_dead(), native_play_dead(),
> > > > falls back to hlt_play_dead() if it returns an error, it should
> > > > better try all of the idle states for which :enter_dead() is present
> > > > before failing, so change it accordingly.
> > > >
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > > > ---
> > > >  drivers/cpuidle/cpuidle.c |    7 ++++---
> > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > >
> > > > Index: linux-pm/drivers/cpuidle/cpuidle.c
> > > > ===================================================================
> > > > --- linux-pm.orig/drivers/cpuidle/cpuidle.c
> > > > +++ linux-pm/drivers/cpuidle/cpuidle.c
> > > > @@ -70,9 +70,10 @@ int cpuidle_play_dead(void)
> > > >               return -ENODEV;
> > > >
> > > >       /* Find lowest-power state that supports long-term idle */
> > > > -     for (i = drv->state_count - 1; i >= 0; i--)
> > > > -             if (drv->states[i].enter_dead)
> > > > -                     return drv->states[i].enter_dead(dev, i);
> > > > +     for (i = drv->state_count - 1; i >= 0; i--) {
> > > > +             if (drv->states[i].enter_dead && !drv->states[i].enter_dead(dev, i))
> > > > +                     return 0;
> > > > +     }
> > >
> > > Hmm, strictly speaking there is no 'success' return from play_dead(). On
> > > success, the CPU is dead :-)
> >
> > Well, would you prefer something like
> >
> > for (i = drv->state_count - 1; i >= 0; i--) {
> >         if (drv->states[i].enter_dead)
> >                 drv->states[i].enter_dead(dev, i);
> > }
> >
> > and adding a comment before the final return statement that
> > :enter_dead() only returns on failure?
>
> Yeah, but perhaps remove the return value entirely if we're going to
> ignore it anyway. And then assume that any return is a failure to die.
>
> I mean, something like:
>
>         if (drv->states[i].enter_dead && !drv->states[i].enter_dead(dev, i))
>                 panic("Dead CPU walking...");
>
> is 'fun' but not very useful.

The panic would be hard to debug if it ever triggers I'm afraid and
there is the fallback to HLT in the caller.

An error message could be printed here though:

    if (drv->states[i].enter_dead && !drv->states[i].enter_dead(dev, i))
            pr_err("CPU %d: Unexpectedly undead\n", dev->cpu);