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