Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
MIME-Version: 1.0
References: <20230929174442.1635558-1-bgeffon@google.com> <202309291304.9AC4F5CFB@keescook>
In-Reply-To: <202309291304.9AC4F5CFB@keescook>
From:   Brian Geffon <bgeffon@google.com>
Date:   Fri, 29 Sep 2023 20:25:42 -0400
Message-ID: <CADyq12xRZfafeu1PVxB1G9zTgThB0W5nok2eXPFLC+T28-b-Bg@mail.gmail.com>
Subject: Re: [PATCH] pid: Allow frozen userspace to reboot from non-init pid ns
To:     Kees Cook <keescook@chromium.org>
Cc:     Christian Brauner <brauner@kernel.org>,
        "Rafael J . Wysocki" <rafael@kernel.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 Fri, Sep 29, 2023 at 4:09=E2=80=AFPM Kees Cook <keescook@chromium.org> w=
rote:
>
> On Fri, Sep 29, 2023 at 01:44:42PM -0400, Brian Geffon 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.
>
> I don't know the code flow too well here, but shouldn't init_pid_ns
> always be doing the reboot regardless of anything else?

I think the point of this is, normally the reaper is runnable and so
an appropriate signal will be delivered allowing them to also clean up
[2]. In our case, they won't be runnable and doing this wouldn't make
sense.

>
> Also how is this syscall running if current is frozen? This feels weird
> to me... shouldn't the frozen test be against pid_ns->child_reaper
> instead of current?

The task which froze the system won't be frozen to make sure this
happens it will have the flag PF_SUSPEND_TASK added, so we know if we
have this flag we're the only running user space task [1].

>
> -Kees

I hope my understanding is correct and it makes sense. Thanks for
taking the time to review.

Brian

1. https://elixir.bootlin.com/linux/latest/source/kernel/power/process.c#L1=
30
2. https://elixir.bootlin.com/linux/latest/source/kernel/pid_namespace.c#L3=
27


>
> --
> Kees Cook