Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
MIME-Version: 1.0
References: <20230929174442.1635558-1-bgeffon@google.com> <CAJZ5v0g6160NYtRQun0D_qS-R6NfoXrX47muWt+sTN3wnNyHeA@mail.gmail.com>
In-Reply-To: <CAJZ5v0g6160NYtRQun0D_qS-R6NfoXrX47muWt+sTN3wnNyHeA@mail.gmail.com>
From:   Brian Geffon <bgeffon@google.com>
Date:   Tue, 17 Oct 2023 15:00:01 -0400
Message-ID: <CADyq12y3dy3G2mhwTq2NDozpCJ6MCe4jzH66M7PysPNmhZGLQw@mail.gmail.com>
Subject: Re: [PATCH] pid: Allow frozen userspace to reboot from non-init pid ns
To:     "Rafael J. Wysocki" <rafael@kernel.org>
Cc:     Christian Brauner <brauner@kernel.org>,
        Kees Cook <keescook@chromium.org>,
        Matthias Kaehlcke <mka@chromium.org>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Frederic Weisbecker <frederic@kernel.org>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Oct 12, 2023 at 5:48=E2=80=AFAM Rafael J. Wysocki <rafael@kernel.or=
g> wrote:
>
> On Fri, Sep 29, 2023 at 7:45=E2=80=AFPM Brian Geffon <bgeffon@google.com>=
 wrote:
> >
> > When the system has a frozen userspace, for example, during hibernation
> > the child reaper task will also be frozen. Attmepting to deliver a
> > signal to it to handle the reboot(2) will ultimately lead to the system
> > hanging unless userspace is thawed.
> >
> > This change checks if the current task is the suspending task and if so
> > it will allow it to proceed with a reboot from the non-init pid ns.
> >
> > Signed-off-by: Brian Geffon <bgeffon@google.com>
> > Reported-by: Matthias Kaehlcke <mka@chromium.org>
> > Tested-by: Matthias Kaehlcke <mka@chromium.org>
>
> If the report is public, which I think is the case, having a Link: tag
> pointing to it here would be nice.
>
> > ---
> >  kernel/pid_namespace.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> > index 0bf44afe04dd..4a93a5063eda 100644
> > --- a/kernel/pid_namespace.c
> > +++ b/kernel/pid_namespace.c
> > @@ -321,6 +321,15 @@ int reboot_pid_ns(struct pid_namespace *pid_ns, in=
t cmd)
> >         if (pid_ns =3D=3D &init_pid_ns)
> >                 return 0;
> >
> > +       if (current->flags & PF_SUSPEND_TASK) {
> > +               /*
> > +                * Attempting to signal the child_reaper won't work if =
it's
> > +                * frozen. In this case we shutdown the system as if we=
 were in
> > +                * the init_pid_ns.
> > +                */
>
> Is the system guaranteed to be in the right state for a shutdown at this =
point?
>
> There is a system-wide suspend-resume or hibernation in progress, so
> system_transition_mutex should be held and that should cause reboot()
> to block anyway.  Do you know why it doesn't block and why the suspend
> task has any reason to call it?
>

Sorry for the delay in responding to these questions, I'm going to do
another pass through this code and respond with a more detailed
explanation in the next few days.

> > +               return 0;
> > +       }
> > +
> >         switch (cmd) {
> >         case LINUX_REBOOT_CMD_RESTART2:
> >         case LINUX_REBOOT_CMD_RESTART:
> > --
> > 2.42.0.582.g8ccd20d70d-goog
> >