Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 18 Jan 2021 18:25:41 +0100
From:   Piotr Figiel <figiel@google.com>
To:     Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        paulmck <paulmck@kernel.org>, Boqun Feng <boqun.feng@gmail.com>,
        Alexey Dobriyan <adobriyan@gmail.com>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Kees Cook <keescook@chromium.org>,
        Alexey Gladkov <gladkov.alexey@gmail.com>,
        Christian Brauner <christian.brauner@ubuntu.com>,
        Michel Lespinasse <walken@google.com>,
        Bernd Edlinger <bernd.edlinger@hotmail.de>,
        Andrei Vagin <avagin@gmail.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Peter Oskolkov <posk@google.com>,
        Kamil Yurtsever <kyurtsever@google.com>,
        Chris Kennelly <ckennelly@google.com>,
        Paul Turner <pjt@google.com>
Subject: Re: [PATCH v2] fs/proc: Expose RSEQ configuration
Message-ID: <YAXElZrWTM3THlvK@google.com>
References: <20210114185445.996-1-figiel@google.com>
 <1530232798.13459.1610725460826.JavaMail.zimbra@efficios.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1530232798.13459.1610725460826.JavaMail.zimbra@efficios.com>
Precedence: bulk

Hi, thanks for review.

On Fri, Jan 15, 2021 at 10:44:20AM -0500, Mathieu Desnoyers wrote:
> ----- On Jan 14, 2021, at 1:54 PM, Piotr Figiel figiel@google.com wrote:
> Added PeterZ, Paul and Boqun to CC. They are also listed as maintainers of rseq.
> Please CC them in your next round of patches.

OK.

> > Since C/R preserves TLS memory and addresses RSEQ ABI will be
> > restored using the address registered before C/R.
> How do you plan to re-register the rseq TLS for each thread upon
> restore ?

In CRIU restorer there is a moment when the code runs on behalf of the
restored thread after the memory is already restored but before the
control is passed to the application code. I'm going to use rseq()
syscall there with the checkpointed values of ABI address and signatures
(obtained via the newly added procfs file).

> I suspect you move the return IP to the abort either at checkpoint or
> restore if you detect that the thread is running in a rseq critical
> section.

Actually in the prototype implementation I use PTRACE_SINGLESTEP during
checkpointing (with some safeguards) to force the kernel to jump out of
the critical section before registers values are fetched. This has the
drawback though that the first instruction of abort handler is executed
upon checkpointing.
I'll likely rework it to update instruction pointer by getting abort
address with PTRACE_PEEKTEXT (via RSEQ ABI).
I think an option is to have a kernel interface to trigger the abort on
userspace's request without need for some hacks. This could be a ptrace
extension.  Alternatively attach could trigger RSEQ logic, but this is
potentially a breaking change for debuggers.

> > Detection whether the thread is in a critical section during C/R is
> > needed to enforce behavior of RSEQ abort during C/R. Attaching with
> > ptrace() before registers are dumped itself doesn't cause RSEQ
> > abort.
> Right, because the RSEQ abort is only done when going back to
> user-space, and AFAIU the checkpointed process will cease to exist,
> and won't go back to user-space, therefore bypassing any RSEQ abort.

The checkpointed process doesn't have to cease to exist, actually it can
continue, and when it's unpaused kernel will schedule the process and
should call the abort handler for RSEQ CS. But this will happen on the
checkpointing side after process state was already checkpointed.
For C/R is important that the checkpoint (serialized process state) is
safe wrt RSEQ.

> > Restoring the instruction pointer within the critical section is
> > problematic because rseq_cs may get cleared before the control is
> > passed to the migrated application code leading to RSEQ invariants
> > not being preserved.
> The commit message should state that both the per-thread rseq TLS area
> address and the signature are dumped within this new proc file.

I'll include this in v3, thanks.

> AFAIU lock_trace prevents concurrent exec() from modifying the task's
> content.  What prevents a concurrent rseq register/unregister to be
> executed concurrently with proc_pid_rseq ?

Yes, in this shape only ptrace prevents, as it was the intended use
case. Do you think it would make sense to add a mutex on task_struct for
the purpose of casual reader (sys admin?) consistency? This would be
locked only here and in the syscall during setting.

(Alternatively SMP barrier could be used to enforce the order so that
the ABI address is always written first, and the signature wouldn't make
sense on ABI address = 0, but probably being simply consistent is
better).

> I wonder if all those parentheses are needed. Wouldn't it be enough to have:

Will remove thanks.

Best regards, Piotr.