lkml.org 
[lkml]   [2019]   [Apr]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/4] glibc: Perform rseq(2) registration at C startup and thread creation (v7)
From
Date
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.

\
 
 \ /
  Last update: 2019-04-04 22:51    [W:0.190 / U:1.796 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site