Adding the aligned(1024) attribute to the definition of __rseq_abi did not increase its size to 1024, for this attribute to impact the size of __rseq_abi it would need to be added to the declaration of 'struct rseq_abi'. We only want to increase the size of the TLS allocation to ensure registration will succeed with future extended ABI. Use a union with a dummy member to ensure we allocate 1024 bytes.
Signed-off-by: Michael Jeanson mjeanson@efficios.com Reviewed-by: Mathieu Desnoyers mathieu.desnoyers@efficios.com --- tools/testing/selftests/rseq/rseq.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/rseq/rseq.c b/tools/testing/selftests/rseq/rseq.c index f6156790c3b4..aa9ae866bc1a 100644 --- a/tools/testing/selftests/rseq/rseq.c +++ b/tools/testing/selftests/rseq/rseq.c @@ -71,9 +71,20 @@ static int rseq_ownership; /* Original struct rseq allocation size is 32 bytes. */ #define ORIG_RSEQ_ALLOC_SIZE 32
+/* + * Use a union to ensure we allocate a TLS area of 1024 bytes to accomodate an + * rseq registration that is larger than the current rseq ABI. + */ +union rseq { + struct rseq_abi abi; + char dummy[RSEQ_THREAD_AREA_ALLOC_SIZE]; +}; + static -__thread struct rseq_abi __rseq_abi __attribute__((tls_model("initial-exec"), aligned(RSEQ_THREAD_AREA_ALLOC_SIZE))) = { - .cpu_id = RSEQ_ABI_CPU_ID_UNINITIALIZED, +__thread union rseq __rseq __attribute__((tls_model("initial-exec"))) = { + .abi = { + .cpu_id = RSEQ_ABI_CPU_ID_UNINITIALIZED, + }, };
static int sys_rseq(struct rseq_abi *rseq_abi, uint32_t rseq_len, @@ -149,7 +160,7 @@ int rseq_register_current_thread(void) /* Treat libc's ownership as a successful registration. */ return 0; } - rc = sys_rseq(&__rseq_abi, get_rseq_min_alloc_size(), 0, RSEQ_SIG); + rc = sys_rseq(&__rseq.abi, get_rseq_min_alloc_size(), 0, RSEQ_SIG); if (rc) { /* * After at least one thread has registered successfully @@ -183,7 +194,7 @@ int rseq_unregister_current_thread(void) /* Treat libc's ownership as a successful unregistration. */ return 0; } - rc = sys_rseq(&__rseq_abi, get_rseq_min_alloc_size(), RSEQ_ABI_FLAG_UNREGISTER, RSEQ_SIG); + rc = sys_rseq(&__rseq.abi, get_rseq_min_alloc_size(), RSEQ_ABI_FLAG_UNREGISTER, RSEQ_SIG); if (rc) return -1; return 0; @@ -249,7 +260,7 @@ void rseq_init(void) rseq_ownership = 1;
/* Calculate the offset of the rseq area from the thread pointer. */ - rseq_offset = (void *)&__rseq_abi - rseq_thread_pointer(); + rseq_offset = (void *)&__rseq.abi - rseq_thread_pointer();
/* rseq flags are deprecated, always set to 0. */ rseq_flags = 0;
On Tue, Mar 11, 2025 at 03:21:45PM -0400, Michael Jeanson wrote:
Adding the aligned(1024) attribute to the definition of __rseq_abi did not increase its size to 1024, for this attribute to impact the size of __rseq_abi it would need to be added to the declaration of 'struct rseq_abi'. We only want to increase the size of the TLS allocation to ensure registration will succeed with future extended ABI. Use a union with a dummy member to ensure we allocate 1024 bytes.
This is in today's -next and breaks the build of the KVM selftests:
In file included from rseq_test.c:24: /home/broonie/git/bisect/usr/include/linux/rseq.h:62:1: error: use of 'rseq' with tag type that does not match previous declaration 62 | struct rseq { | ^ ./../rseq/rseq.c:78:7: note: previous use is here 78 | union rseq { | ^ In file included from rseq_test.c:24: /home/broonie/git/bisect/usr/include/linux/rseq.h:62:8: error: redefinition of 'rseq' 62 | struct rseq { | ^ ./../rseq/rseq.c:78:7: note: previous definition is here 78 | union rseq { | ^
since unlike the rseq tests the KVM rseq test includes the UAPI header for rseq which the padded union conflicts with.
On 2025-03-18 10:01, Mark Brown wrote:
On Tue, Mar 11, 2025 at 03:21:45PM -0400, Michael Jeanson wrote:
Adding the aligned(1024) attribute to the definition of __rseq_abi did not increase its size to 1024, for this attribute to impact the size of __rseq_abi it would need to be added to the declaration of 'struct rseq_abi'. We only want to increase the size of the TLS allocation to ensure registration will succeed with future extended ABI. Use a union with a dummy member to ensure we allocate 1024 bytes.
This is in today's -next and breaks the build of the KVM selftests:
In file included from rseq_test.c:24: /home/broonie/git/bisect/usr/include/linux/rseq.h:62:1: error: use of 'rseq' with tag type that does not match previous declaration 62 | struct rseq { | ^ ./../rseq/rseq.c:78:7: note: previous use is here 78 | union rseq { | ^ In file included from rseq_test.c:24: /home/broonie/git/bisect/usr/include/linux/rseq.h:62:8: error: redefinition of 'rseq' 62 | struct rseq { | ^ ./../rseq/rseq.c:78:7: note: previous definition is here 78 | union rseq { | ^
since unlike the rseq tests the KVM rseq test includes the UAPI header for rseq which the padded union conflicts with.
Oh, I missed that, we need a more unique name for the union.
I'm unfamiliar with the workflow of linux-next, should I send a V2 of the current patch, or a new one that applies on top?
Thanks,
Michael
On Tue, Mar 18, 2025 at 10:50:58AM -0400, Michael Jeanson wrote:
On 2025-03-18 10:01, Mark Brown wrote:
On Tue, Mar 11, 2025 at 03:21:45PM -0400, Michael Jeanson wrote:
Adding the aligned(1024) attribute to the definition of __rseq_abi did not increase its size to 1024, for this attribute to impact the size of __rseq_abi it would need to be added to the declaration of 'struct rseq_abi'. We only want to increase the size of the TLS allocation to ensure registration will succeed with future extended ABI. Use a union with a dummy member to ensure we allocate 1024 bytes.
This is in today's -next and breaks the build of the KVM selftests:
...
since unlike the rseq tests the KVM rseq test includes the UAPI header for rseq which the padded union conflicts with.
Oh, I missed that, we need a more unique name for the union.
I'm unfamiliar with the workflow of linux-next, should I send a V2 of the current patch, or a new one that applies on top?
It depends on the tree that the patch was applied to - -next merges the current stat of the maintainer trees daily rather than applying anything itself. In this case that's -tip, I think incremental is good for them but ICBW?
linux-kselftest-mirror@lists.linaro.org
-
Mark Brown -
Michael Jeanson