Re: [RFC PATCH v2 1/2] rseq: x86: implement abort-at-ip extension

Christian Brauner
2022-01-12T08:46:30Z

On Tue, Jan 11, 2022 at 12:43:05PM -0500, Mathieu Desnoyers wrote:
> ----- On Jan 11, 2022, at 6:05 AM, Christian Brauner brauner@kernel.org wrote:
>
> > On Mon, Jan 10, 2022 at 12:16:10PM -0500, Mathieu Desnoyers wrote:
> >> Allow rseq critical section abort handlers to optionally figure out at
> >> which instruction pointer the rseq critical section was aborted.
> >>
> >> This allows implementing rseq critical sections with loops containing
> >> the commit instruction, for which having the commit as last instruction
> >> of the sequence is not possible. This is useful to implement adaptative
> >> mutexes aware of preemption in user-space. (see [1])
> >>
> >> This also allows implementing rseq critical sections with multiple
> >> commit steps, and use the abort-at-ip information to figure out what
> >> needs to be undone in the abort handler.
> >>
> >> Introduce the RSEQ_FLAG_QUERY_ABORT_AT_IP rseq system call flag. This
> >> lets userspace query whether the kernel and architecture supports the
> >> abort-at-ip rseq extension.
> >>
> >> Only provide this information for rseq critical sections for which the
> >> rseq_cs descriptor has the RSEQ_CS_FLAG_ABORT_AT_IP flag set. Abort
> >> handlers for critical sections with this flag set need to readjust the
> >> stack pointer. The abort-at-ip pointer is populated by the kernel on
> >> the top of stack on abort. For x86-32, the abort handler of an
> >> abort-at-ip critical section needs to add 4 bytes to the stack pointer.
> >> For x86-64, the abort hanler needs to add 136 bytes to the stack
> >> pointer: 8 bytes to skip the abort-at-ip value, and 128 bytes to skip
> >> the x86-64 redzone for leaf functions.
> >>
> >> [1]
> >> https://github.com/compudj/rseq-test/blob/adapt-lock-abort-at-ip/test-rseq-adaptative-lock.c#L80
> >>
> >> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> >> ---
> >> Changes since v1:
> >> - Use top of stack to place abort-at-ip value rather than ecx/rcx
> >> register,
> >> - Skip redzone on x86-64.
> >> ---
> >> arch/x86/include/asm/ptrace.h | 5 +++++
> >> arch/x86/kernel/ptrace.c | 12 ++++++++++++
> >> include/uapi/linux/rseq.h | 4 ++++
> >> kernel/rseq.c | 28 ++++++++++++++++++++++++++++
> >> 4 files changed, 49 insertions(+)
> >>
> >> diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
> >> index 703663175a5a..c96eb2448110 100644
> >> --- a/arch/x86/include/asm/ptrace.h
> >> +++ b/arch/x86/include/asm/ptrace.h
> >> @@ -387,5 +387,10 @@ extern int do_set_thread_area(struct task_struct *p, int
> >> idx,
> >> # define do_set_thread_area_64(p, s, t) (0)
> >> #endif
> >>
> >> +#ifdef CONFIG_RSEQ
> >> +# define RSEQ_ARCH_HAS_ABORT_AT_IP
> >> +int rseq_abort_at_ip(struct pt_regs *regs, unsigned long ip);
> >> +#endif
> >> +
> >> #endif /* !__ASSEMBLY__ */
> >> #endif /* _ASM_X86_PTRACE_H */
> >> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> >> index 6d2244c94799..561ed98d12ba 100644
> >> --- a/arch/x86/kernel/ptrace.c
> >> +++ b/arch/x86/kernel/ptrace.c
> >> @@ -1368,3 +1368,15 @@ void user_single_step_report(struct pt_regs *regs)
> >> {
> >> send_sigtrap(regs, 0, TRAP_BRKPT);
> >> }
> >> +
> >> +int rseq_abort_at_ip(struct pt_regs *regs, unsigned long ip)
> >> +{
> >> + if (user_64bit_mode(regs)) {
> >> + /* Need to skip redzone for leaf functions. */
> >> + regs->sp -= sizeof(u64) + 128;
> >> + return put_user(ip, (u64 __user *)regs->sp);
> >> + } else {
> >> + regs->sp -= sizeof(u32);
> >> + return put_user(ip, (u32 __user *)regs->sp);
> >> + }
> >
> > I think it would be really helpful if you added the full explanation for
> > sizeof(u64) + 128 or -sizeof(u32) into this codepath or provide
> > constants. For folks not familiar with stuff like this it'll look like
> > magic numbers. :)
>
> Good point, here is the planned update:

That's great, thanks!

>
> int rseq_abort_at_ip(struct pt_regs *regs, unsigned long ip)
> {
> if (user_64bit_mode(regs)) {
> /*
> * rseq abort-at-ip x86-64 ABI: stack pointer is decremented to
> * skip the redzone (128 bytes on x86-64), and decremented of
> * the pointer size (8 bytes). The aborted address (abort-at-ip)
> * is stored at this updated stack pointer location (top of stack).
> *
> * Skipping the redzone is needed to make sure not to corrupt
> * stack data when the rseq critical section is within a leaf
> * function.
> */
> regs->sp -= sizeof(u64) + 128;
> return put_user(ip, (u64 __user *)regs->sp);
> } else {
> /*
> * rseq abort-at-ip x86-32 ABI: stack pointer is decremented of
> * the pointer size (4 bytes). The aborted address (abort-at-ip)
> * is stored at this updated stack pointer location (top of stack).
> */
> regs->sp -= sizeof(u32);
> return put_user(ip, (u32 __user *)regs->sp);
> }
> }
>
>
>
> >
> >> +}
> >> diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
> >> index 9a402fdb60e9..3130232e6d0c 100644
> >> --- a/include/uapi/linux/rseq.h
> >> +++ b/include/uapi/linux/rseq.h
> >> @@ -20,12 +20,14 @@ enum rseq_cpu_id_state {
> >>
> >> enum rseq_flags {
> >> RSEQ_FLAG_UNREGISTER = (1 << 0),
> >> + RSEQ_FLAG_QUERY_ABORT_AT_IP = (1 << 1),
> >> };
> >>
> >> enum rseq_cs_flags_bit {
> >> RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT_BIT = 0,
> >> RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL_BIT = 1,
> >> RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE_BIT = 2,
> >> + RSEQ_CS_FLAG_ABORT_AT_IP_BIT = 3,
> >> };
> >>
> >> enum rseq_cs_flags {
> >> @@ -35,6 +37,8 @@ enum rseq_cs_flags {
> >> (1U << RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL_BIT),
> >> RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE =
> >> (1U << RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE_BIT),
> >> + RSEQ_CS_FLAG_ABORT_AT_IP =
> >> + (1U << RSEQ_CS_FLAG_ABORT_AT_IP_BIT),
> >> };
> >>
> >> /*
> >> diff --git a/kernel/rseq.c b/kernel/rseq.c
> >> index 6d45ac3dae7f..fb52f2d11b56 100644
> >> --- a/kernel/rseq.c
> >> +++ b/kernel/rseq.c
> >> @@ -21,6 +21,13 @@
> >> #define RSEQ_CS_PREEMPT_MIGRATE_FLAGS (RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE | \
> >> RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT)
> >>
> >> +#ifdef RSEQ_ARCH_HAS_ABORT_AT_IP
> >> +static bool rseq_has_abort_at_ip(void) { return true; }
> >> +#else
> >> +static bool rseq_has_abort_at_ip(void) { return false; }
> >> +static int rseq_abort_at_ip(struct pt_regs *regs, unsigned long ip) { return 0;
> >> }
> >> +#endif
> >> +
> >> /*
> >> *
> >> * Restartable sequences are a lightweight interface that allows
> >> @@ -79,6 +86,16 @@
> >> *
> >> * [abort_ip]
> >> * F1. <failure>
> >> + *
> >> + * rseq critical sections defined with the RSEQ_CS_FLAG_ABORT_AT_IP flag
> >> + * have the following behavior on abort: when the stack grows down: the
> >> + * stack pointer is decremented to skip the redzone, and decremented of
> >> + * the pointer size. The aborted address (abort-at-ip) is stored at
> >> + * this stack pointer location. The user-space abort handler needs to
> >> + * pop the abort-at-ip address from the stack, and add the redzone size
> >> + * to the stack pointer.
> >> + *
> >> + * TODO: describe stack grows up.
> >
> > Is this intentional or did you forget? :)
>
> Since I did not implement abort-at-ip on stack-grows-up architectures, I felt
> it would be too early to describe the algorithm. I can simply remove the TODO
> altogether and we'll take care of it when we get there ? If I had to try to
> wordsmith it, it would look like e.g.:
>
> * [...] When the stack grows up: the
> * stack pointer is incremented to skip the redzone, and incremented of
> * the pointer size. The aborted address (abort-at-ip) is stored immediately
> * under this stack pointer location. The user-space abort handler needs to
> * pop the abort-at-ip address from the stack, and subtract the redzone size
> * from the stack pointer.
>
> [ Please let me know if I got somehow confused in my understanding of stack grows
> up architectures. ]
>
> I'm also unsure whether any of the stack grows up architecture have redzones ?

I don't think so? From when I last touched that piece of arch code when
massaging copy_thread() I only remember parisc as having an upwards
growing stack.

> From a quick grep for redzone in Linux arch/, only openrisc, powerpc64 and
> x86-64 appear to have redzones.