| Message ID | 20210114185445.996-1-figiel@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] fs/proc: Expose RSEQ configuration | expand |
----- 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
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.
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),
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(+)