----- On Apr 27, 2020, at 12:54 PM, Florian Weimer fw@xxxxxxxxxxxxx wrote:
> * Mathieu Desnoyers:
>
>>>> +#include <sys/syscall.h>
>>>> +#include <stdint.h>
>>>> +#include <kernel-features.h>
>>>> +#include <sys/rseq.h>
>>>> +
>>>> +__thread struct rseq __rseq_abi = {
>>>> + .cpu_id = RSEQ_CPU_ID_UNINITIALIZED,
>>>> +};
>>>
>>> { should go onto its own line.
>>
>> OK
>>
>>> I'd also add attribute_tls_model_ie,
>>> also it's implied by the declaration in the header.
>>
>> This contradicts feedback I received from Szabolcs Nagy in September 2019:
>>
>> https://public-inbox.org/libc-alpha/c58d4d6e-f22a-f5d9-e23a-5bd72cec1a86@xxxxxxx/
>>
>> "note that libpthread.so is built with -ftls-model=initial-exec
>>
>> (and if it wasn't then you'd want to put the attribute on the
>> declaration in the internal header file, not on the definition,
>> so the actual tls accesses generate the right code)"
>>
>> In the context of his feedback, __rseq_abi was defined within
>> nptl/pthread_create.c.
>> It is now defined in sysdeps/unix/sysv/linux/rseq-sym.c, which is built into the
>> csu which AFAIU ends up in libc.so. His comment still applies though, because
>> libc.so is also built with -ftls-model=initial-exec.
>>
>> So should I apply the "initial-exec" TLS model only to the __rseq_abi
>> declaration, or is it preferred to apply it to both the declaration
>> and the definition ?
>
> I do not have a strong preference here. Technically, the declaration
> in the header file should be enough.
OK, so I'll just keep the attribute on the declaration in the header.
>
>>>> diff --git a/sysdeps/unix/sysv/linux/sys/rseq.h
>>>> b/sysdeps/unix/sysv/linux/sys/rseq.h
>>>> new file mode 100644
>>>> index 0000000000..503dce4cac
>>>> --- /dev/null
>>>> +++ b/sysdeps/unix/sysv/linux/sys/rseq.h
>>>> @@ -0,0 +1,186 @@
>>>
>>> I think there is some value in making this header compatible with
>>> inclusion from the assembler (including constants for the relevant
>>> struct offsets), but that can be a later change.
>>
>> Agreed. By "later", do you mean before merging the patch, between
>> merge of the patch and next glibc release, or for a subsequent glibc
>> release ?
>
> It can happen some time after merging the patch, preferably for this
> release. But I don't think it's release-critical.
OK
>
>>>> +/* struct rseq is aligned on 4 * 8 bytes to ensure it is always
>>>> + contained within a single cache-line.
>>>> +
>>>> + A single struct rseq per thread is allowed. */
>>>> +struct rseq
>>>> + {
>>>> + /* Restartable sequences cpu_id_start field. Updated by the
>>>> + kernel. Read by user-space with single-copy atomicity
>>>> + semantics. This field should only be read by the thread which
>>>> + registered this data structure. Aligned on 32-bit. Always
>>>
>>> What does “Aligned on 32-bit” mean in this context? Do you mean to
>>> reference 32-*byte* alignment here?
>>
>> No. I really mean 32-bit (4-byte). Being aligned on 32-byte guarantees that
>> this field is aligned at least on 4-byte. This is required by single-copy
>> atomicity semantics.
>>
>> Should I update this comment to state "Aligned on 4-byte" instead ?
>
> I think this is implied by all Linux ABIs. And the explicit alignment
> specification for struct rseq makes the alignment 32 bytes.
Unless a structure ends up being packed, which is of course not the case
here.
I would prefer to keep the comment about 32-bit alignment requirement on
the specific fields, because the motivation for alignment requirement is
much more strict for fields (correctness) than the motivation for alignment
of the structure (performance).
>
>>>> + /* Restartable sequences rseq_cs field.
>>>> +
>>>> + Contains NULL when no critical section is active for the current
>>>> + thread, or holds a pointer to the currently active struct rseq_cs.
>>>> +
>>>> + Updated by user-space, which sets the address of the currently
>>>> + active rseq_cs at the beginning of assembly instruction sequence
>>>> + block, and set to NULL by the kernel when it restarts an assembly
>>>> + instruction sequence block, as well as when the kernel detects that
>>>> + it is preempting or delivering a signal outside of the range
>>>> + targeted by the rseq_cs. Also needs to be set to NULL by user-space
>>>> + before reclaiming memory that contains the targeted struct rseq_cs.
>>>> +
>>>> + Read and set by the kernel. Set by user-space with single-copy
>>>> + atomicity semantics. This field should only be updated by the
>>>> + thread which registered this data structure. Aligned on 64-bit. */
>>>> + union {
>>>> + uint64_t ptr64;
>>>> +#ifdef __LP64__
>>>> + uint64_t ptr;
>>>> +#else
>>>> + struct {
>>>> +#if (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) ||
>>>> defined(__BIG_ENDIAN)
>>>> + uint32_t padding; /* Initialized to zero. */
>>>> + uint32_t ptr32;
>>>> +#else /* LITTLE */
>>>> + uint32_t ptr32;
>>>> + uint32_t padding; /* Initialized to zero. */
>>>> +#endif /* ENDIAN */
>>>> + } ptr;
>>>> +#endif
>>>> + } rseq_cs;
>>>
>>> Are these conditionals correct for x32?
>>
>> Let's see. With x86 gcc:
>>
>> -m64: (__x86_64__ && __LP64__)
>> -m32: (__i386__)
>> -mx32: (__x86_64__ && __ILP32__)
>>
>> So with "#ifdef __LP64__" we specifically target 64-bit pointers. The rest
>> falls into the "else" case, which expects 32-bit pointers. Considering that
>> x32 has 32-bit pointers, I don't see any issue here.
>
> Does the kernel have a separate 32-bit entry point for rseq on x32?
> If not, it will expect the 64-bit struct layout.
No, there is a single entry point into rseq covering all of 32-bit, 64-bit and x32.
We achieve this by ensuring the layout of the linux/rseq.h structures
uses the union representation for pointers. Therefore, the kernel does not care
whether it reads a pointer from a 32-bit or 64-bit process. This is becoming the
preferred way to design Linux kernel ABIs nowadays.
>
>> We don't mind that user-space uses that pointer, but we never want the kernel
>> to touch that pointer rather than the 32/64-bit-aware fields. One possibility
>> would be to do:
>>
>> union
>> {
>> uint64_t ptr64;
>> #ifdef __LP64__
>> uint64_t ptr;
>> #else
>> struct
>> {
>> #if (defined (__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || defined
>> (__BIG_ENDIAN)
>> uint32_t padding; /* Initialized to zero. */
>> uint32_t ptr32;
>> #else /* LITTLE */
>> uint32_t ptr32;
>> uint32_t padding; /* Initialized to zero. */
>> #endif /* ENDIAN */
>> } ptr;
>> #endif
>>
>> #ifndef __KERNEL__
>> const struct rseq_cs *uptr;
>> #endif
>> } rseq_cs;
>>
>> in the union, so only user-space can see that field. Thoughts ?
>
> I think this depends on where the x32 question lands.
x32 should not be an issue as explained above, so I'm very open to
add this "uptr" for user-space only.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
