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

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.

quoted

quoted

+++ b/sysdeps/unix/sysv/linux/arm/bits/rseq.h

[...]

quoted

quoted

+
+/* 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.

quoted

quoted

+/* 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?

[...]

quoted

quoted

+++ b/sysdeps/unix/sysv/linux/powerpc/bits/rseq.h

[...]

quoted

quoted

+/* 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.

[...]

quoted

quoted

+++ b/sysdeps/unix/sysv/linux/s390/bits/rseq.h

[...]

quoted

quoted

+
+/* 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.

[...]

quoted

quoted

+++ b/sysdeps/unix/sysv/linux/x86/bits/rseq.h

[...]

quoted

quoted

+/* 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.