Re: [PATCH 1/4] glibc: Perform rseq(2) registration at C startup and thread creation (v7)

[Carlos O'Donell <codonell@redhat.com>]

On 3/25/19 11:54 AM, Mathieu Desnoyers wrote:
> Hi Carlos,
> 
> ----- On Mar 22, 2019, at 4:09 PM, Carlos O'Donell codonell@redhat.com wrote:
> 
> [...]
> 
> I took care of all your comments for an upcoming round of patches, except the
> following that remain open (see answer inline). I'm adding Linux maintainers
> for ARM, aarch64, MIPS, powerpc, s390, x86 in CC to discuss the choice of
> code signature prior to the abort handler for each of those architectures.

Thank you for kicking off this conversation.

Every architecture should have a reasonable RSEQ_SIG that applies to their
ISA with a comment about why that instruction was chosen. It should be a
conscious choice, without a default.

> * Support for automatically registering threads with the Linux rseq(2)
>    system call has been added.  This system call is implemented starting
>    from Linux 4.18.  The Restartable Sequences ABI accelerates user-space
>    operations on per-cpu data.  It allows user-space to perform updates
>    on per-cpu data without requiring heavy-weight atomic operations. See
>    https://www.efficios.com/blog/2019/02/08/linux-restartable-sequences/
>    for further explanation.
> 
>    In order to be activated, it requires that glibc is built against
>    kernel headers that include this system call, and that glibc detects
>    availability of that system call at runtime.

Suggest:

* Support for automatically registering threads with the Linux rseq(2)
   system call has been added.  This system call is implemented starting
   from Linux 4.18.  The Restartable Sequences ABI accelerates user-space
   operations on per-cpu data.  It allows user-space to perform updates
   on per-cpu data without requiring heavy-weight atomic operations.
   Automatically registering threads allows all libraries, including libc,
   to make immediate use of the rseq(2) support by using the documented ABI.
   See 'man 2 rseq' for the details of the ABI shared between libc and the
   kernel.

> 
> For reference the assembly code I'm pointing at below can be found
> in the Linux selftests under:
> 
> tools/testing/selftests/rseq/rseq-*.h

OK.


>>> +++ b/sysdeps/unix/sysv/linux/arm/bits/rseq.h
> [...]
>>> +
>>> +/* Signature required before each abort handler code.  */
>>> +#define RSEQ_SIG 0x53053053
>>
>> Why isn't this an arm specific op code? Does the user have to mark this
>> up as part of a constant pool when placing it in front of the abort handler
>> to avoid disassembling the constant as code? This was at one point required
>> to get gdb to work properly.
>>
> 
> For arm, the abort is defined as:
> 
> #define __RSEQ_ASM_DEFINE_ABORT(table_label, label, teardown,           \
>                                  abort_label, version, flags,            \
>                                  start_ip, post_commit_offset, abort_ip) \
>                  ".balign 32\n\t"                                        \
>                  __rseq_str(table_label) ":\n\t"                         \
>                  ".word " __rseq_str(version) ", " __rseq_str(flags) "\n\t" \
>                  ".word " __rseq_str(start_ip) ", 0x0, " __rseq_str(post_commit_offset) ", 0x0, " __rseq_str(abort_ip) ", 0x0\n\t" \
>                  ".word " __rseq_str(RSEQ_SIG) "\n\t"                    \
>                  __rseq_str(label) ":\n\t"                               \
>                  teardown                                                \
>                  "b %l[" __rseq_str(abort_label) "]\n\t"
> 
> Which contains a copy of the struct rseq_cs for that critical section
> close to the actual critical section, within the code, followed by the
> signature. The reason why we have a copy of the struct rseq_cs there is
> to speed up entry into the critical section by using the "adr" instruction
> to compute the address to store into __rseq_cs->rseq_cs.
> 
> AFAIU, a literal pool on ARM is defined as something which is always
> jumped over (never executed), which is the case here. We always have
> an unconditional branch instruction ("b") skipping over each
> RSEQ_ASM_DEFINE_ABORT().
> 
> Therefore, given that the signature is part of a literal pool on ARM,
> it can be any value we choose and should not need to be an actual valid
> instruction.
> 
> aarch64 defines the abort as:
> 
> #define RSEQ_ASM_DEFINE_ABORT(label, abort_label)                               \
>          "       b       222f\n"                                                 \
>          "       .inst   "       __rseq_str(RSEQ_SIG) "\n"                       \
>          __rseq_str(label) ":\n"                                                 \
>          "       b       %l[" __rseq_str(abort_label) "]\n"                      \
>          "222:\n"
> 
> Where the signature actually maps to a valid instruction. Considering that
> aarch64 also have literal pools, and we branch over the signature, I wonder
> why it's so important to ensure the signature is a valid trap instruction.
> Perhaps Will Deacon can enlighten us ?

In the event that you accidentally jump to it then you trap?

However, you want an *uncommon* trap insn.

I think the order of preference is:

1.  An uncommon insn (with random immediate values), in a literal pool, that is
     not a useful ROP/JOP sequence (very uncommon)
2a. A uncommon TRAP hopefully with some immediate data encoded (maybe uncommon)
2b. A NOP to avoid affecting speculative execution (maybe uncommon)

With 2a/2b being roughly equivalent depending on speculative execution policy.

>>> +/* Signature required before each abort handler code.  */
>>> +#define RSEQ_SIG 0x53053053
>>
>> Why isn't this a mips-specific op code?
> 
> MIPS also has a literal pool just before the abort handler, and it
> jumps over it. My understanding is that we can use any signature value
> we want, and it does not need to be a valid instruction, similarly to ARM:
> 
> #define __RSEQ_ASM_DEFINE_ABORT(table_label, label, teardown, \
>                                  abort_label, version, flags, \
>                                  start_ip, post_commit_offset, abort_ip) \
>                  ".balign 32\n\t" \
>                  __rseq_str(table_label) ":\n\t" \
>                  ".word " __rseq_str(version) ", " __rseq_str(flags) "\n\t" \
>                  LONG " " U32_U64_PAD(__rseq_str(start_ip)) "\n\t" \
>                  LONG " " U32_U64_PAD(__rseq_str(post_commit_offset)) "\n\t" \
>                  LONG " " U32_U64_PAD(__rseq_str(abort_ip)) "\n\t" \
>                  ".word " __rseq_str(RSEQ_SIG) "\n\t" \
>                  __rseq_str(label) ":\n\t" \
>                  teardown \
>                  "b %l[" __rseq_str(abort_label) "]\n\t"
> 
> Perhaps Paul Burton can confirm this ?

Yes please.

You also want to avoid the value being a valid MIPS insn that's common.

Did you check that?

> [...]
>>> +++ b/sysdeps/unix/sysv/linux/powerpc/bits/rseq.h
> [...]
>>> +/* Signature required before each abort handler code.  */
>>> +#define RSEQ_SIG 0x53053053
>>
>> Why isn't this an opcode specific to power?
> 
> On powerpc 32/64, the abort is placed in a __rseq_failure executable section:
> 
> #define RSEQ_ASM_DEFINE_ABORT(label, abort_label)                               \
>                  ".pushsection __rseq_failure, \"ax\"\n\t"                       \
>                  ".long " __rseq_str(RSEQ_SIG) "\n\t"                            \
>                  __rseq_str(label) ":\n\t"                                       \
>                  "b %l[" __rseq_str(abort_label) "]\n\t"                         \
>                  ".popsection\n\t"
> 
> That section only contains snippets of those trampolines. Arguably, it would be
> good if disassemblers could find valid instructions there. Boqun Feng could perhaps
> shed some light on this signature choice ? Now would be a good time to decide
> once and for all whether a valid instruction would be a better choice.

This seems questionable too.

> [...]
>>> +++ b/sysdeps/unix/sysv/linux/s390/bits/rseq.h
> [...]
>>> +
>>> +/* Signature required before each abort handler code.  */
>>> +#define RSEQ_SIG 0x53053053
>>
>> Why not a s390 specific value here?
> 
> s390 also has the abort handler in a __rseq_failure section:
> 
> #define RSEQ_ASM_DEFINE_ABORT(label, teardown, abort_label)             \
>                  ".pushsection __rseq_failure, \"ax\"\n\t"               \
>                  ".long " __rseq_str(RSEQ_SIG) "\n\t"                    \
>                  __rseq_str(label) ":\n\t"                               \
>                  teardown                                                \
>                  "j %l[" __rseq_str(abort_label) "]\n\t"                 \
>                  ".popsection\n\t"
> 
> Same question applies as powerpc: since disassemblers will try to decode
> that instruction, would it be better to define it as a valid one ?

Yes, I think it needs to be a valid uncommon insn or nop.

> [...]
>>> +++ b/sysdeps/unix/sysv/linux/x86/bits/rseq.h
> [...]
>>> +/* Signature required before each abort handler code.  */
>>> +#define RSEQ_SIG 0x53053053
>>
>> Why not an x86-specific op code?
> 
> On x86, we use this 4-byte signature as operand to a "no-op" instruction
> taking 4-byte immediate operand:

That makes perfect sense. Thanks.

So what is left to audit?

In summary:

- Why does AArch64 choose a trap?

- Is the current choice of 0x53053053 OK for MIPS? Does it map to a valid insn?

- What better choice is there for power? Pick a real uncommon insn or nop?

- What better choice is there for s390? Pick a real uncommon insn or nop?
   - Todays choice could become something special in the future since it's unassigned.

-- 
Cheers,
Carlos.