[PATCH v2] fs/proc: Expose RSEQ configuration

[PATCH v2] fs/proc: Expose RSEQ configuration

[Piotr Figiel]

For userspace checkpoint and restore (C/R) some way of getting process
state containing RSEQ configuration is needed.

There are two ways this information is going to be used:
 - to re-enable RSEQ for threads which had it enabled before C/R
 - to detect if a thread was in a critical section during C/R

Since C/R preserves TLS memory and addresses RSEQ ABI will be restored
using the address registered before C/R.

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.
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.

Signed-off-by: Piotr Figiel <figiel@google.com>

---

v2:
 - fixed string formatting for 32-bit architectures

v1:
 - https://lkml.kernel.org/r/20210113174127.2500051-1-figiel@google.com

---
 fs/proc/base.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index b3422cda2a91..7cc36a224b8b 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -662,6 +662,21 @@ static int proc_pid_syscall(struct seq_file *m, struct pid_namespace *ns,
 
 	return 0;
 }
+
+#ifdef CONFIG_RSEQ
+static int proc_pid_rseq(struct seq_file *m, struct pid_namespace *ns,
+				struct pid *pid, struct task_struct *task)
+{
+	int res = lock_trace(task);
+
+	if (res)
+		return res;
+	seq_printf(m, "%tx %08x\n", (ptrdiff_t)((uintptr_t)task->rseq),
+		   task->rseq_sig);
+	unlock_trace(task);
+	return 0;
+}
+#endif /* CONFIG_RSEQ */
 #endif /* CONFIG_HAVE_ARCH_TRACEHOOK */
 
 /************************************************************************/
@@ -3182,6 +3197,9 @@ static const struct pid_entry tgid_base_stuff[] = {
 	REG("comm",      S_IRUGO|S_IWUSR, proc_pid_set_comm_operations),
 #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
 	ONE("syscall",    S_IRUSR, proc_pid_syscall),
+#ifdef CONFIG_RSEQ
+	ONE("rseq",       S_IRUSR, proc_pid_rseq),
+#endif
 #endif
 	REG("cmdline",    S_IRUGO, proc_pid_cmdline_ops),
 	ONE("stat",       S_IRUGO, proc_tgid_stat),
@@ -3522,6 +3540,9 @@ static const struct pid_entry tid_base_stuff[] = {
 			 &proc_pid_set_comm_operations, {}),
 #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
 	ONE("syscall",   S_IRUSR, proc_pid_syscall),
+#ifdef CONFIG_RSEQ
+	ONE("rseq",      S_IRUSR, proc_pid_rseq),
+#endif
 #endif
 	REG("cmdline",   S_IRUGO, proc_pid_cmdline_ops),
 	ONE("stat",      S_IRUGO, proc_tid_stat),
-- 
2.30.0.284.gd98b1dd5eaa7-goog

Re: [PATCH v2] fs/proc: Expose RSEQ configuration

[Mathieu Desnoyers]

----- 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.

> For userspace checkpoint and restore (C/R) some way of getting process
> state containing RSEQ configuration is needed.
> 
> There are two ways this information is going to be used:
> - to re-enable RSEQ for threads which had it enabled before C/R
> - to detect if a thread was in a critical section during C/R
> 
> Since C/R preserves TLS memory and addresses RSEQ ABI will be restored
> using the address registered before C/R.

Indeed, if the process goes through a checkpoint/restore while within a
rseq c.s., that critical section should abort. Given that it's only the
restored process which resumes user-space execution, there should be some
way to ensure that the rseq tls pointer is restored before that thread goes
back to user-space, or some way to ensure the rseq TLS is registered
before that thread returns to the saved instruction pointer.

How do you plan to re-register the rseq TLS for each thread upon restore ?

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.

> 
> 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.

> 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.

> 
> Signed-off-by: Piotr Figiel <figiel@google.com>
> 
> ---
> 
> v2:
> - fixed string formatting for 32-bit architectures
> 
> v1:
> - https://lkml.kernel.org/r/20210113174127.2500051-1-figiel@google.com
> 
> ---
> fs/proc/base.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index b3422cda2a91..7cc36a224b8b 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -662,6 +662,21 @@ static int proc_pid_syscall(struct seq_file *m, struct
> pid_namespace *ns,
> 
> 	return 0;
> }
> +
> +#ifdef CONFIG_RSEQ
> +static int proc_pid_rseq(struct seq_file *m, struct pid_namespace *ns,
> +				struct pid *pid, struct task_struct *task)
> +{
> +	int res = lock_trace(task);

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 ?

> +
> +	if (res)
> +		return res;
> +	seq_printf(m, "%tx %08x\n", (ptrdiff_t)((uintptr_t)task->rseq),

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

  (ptrdiff_t)(uintptr_t)task->rseq

?

Thanks,

Mathieu

> +		   task->rseq_sig);
> +	unlock_trace(task);
> +	return 0;
> +}
> +#endif /* CONFIG_RSEQ */
> #endif /* CONFIG_HAVE_ARCH_TRACEHOOK */
> 
> /************************************************************************/
> @@ -3182,6 +3197,9 @@ static const struct pid_entry tgid_base_stuff[] = {
> 	REG("comm",      S_IRUGO|S_IWUSR, proc_pid_set_comm_operations),
> #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
> 	ONE("syscall",    S_IRUSR, proc_pid_syscall),
> +#ifdef CONFIG_RSEQ
> +	ONE("rseq",       S_IRUSR, proc_pid_rseq),
> +#endif
> #endif
> 	REG("cmdline",    S_IRUGO, proc_pid_cmdline_ops),
> 	ONE("stat",       S_IRUGO, proc_tgid_stat),
> @@ -3522,6 +3540,9 @@ static const struct pid_entry tid_base_stuff[] = {
> 			 &proc_pid_set_comm_operations, {}),
> #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
> 	ONE("syscall",   S_IRUSR, proc_pid_syscall),
> +#ifdef CONFIG_RSEQ
> +	ONE("rseq",      S_IRUSR, proc_pid_rseq),
> +#endif
> #endif
> 	REG("cmdline",   S_IRUGO, proc_pid_cmdline_ops),
> 	ONE("stat",      S_IRUGO, proc_tid_stat),
> --
> 2.30.0.284.gd98b1dd5eaa7-goog

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH v2] fs/proc: Expose RSEQ configuration

[Piotr Figiel]

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.