Adapt the rseq.c/rseq.h code to follow GNU C library changes introduced by:

commit 2e456ccf0c34 ("Linux: Make __rseq_size useful for feature detection (bug 31965)")

Wihout this fix, rseq selftests for mm_cid fail:

./run_param_test.sh
Default parameters
Running test spinlock
Running compare-twice test spinlock
Running mm_cid test spinlock
Error: cpu id getter unavailable

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Shuah Khan <skhan@linuxfoundation.org>
CC: Carlos O'Donell <carlos@redhat.com>
CC: Florian Weimer <fweimer@redhat.com>
---
 tools/testing/selftests/rseq/rseq.c | 109 +++++++++++++++++++---------
 tools/testing/selftests/rseq/rseq.h |  10 +--
 2 files changed, 76 insertions(+), 43 deletions(-)

diff --git a/tools/testing/selftests/rseq/rseq.c b/tools/testing/selftests/rseq/rseq.c
index 96e812bdf8a4..3797bb0881da 100644
--- a/tools/testing/selftests/rseq/rseq.c
+++ b/tools/testing/selftests/rseq/rseq.c
@@ -60,12 +60,6 @@ unsigned int rseq_size = -1U;
 /* Flags used during rseq registration.  */
 unsigned int rseq_flags;
 
-/*
- * rseq feature size supported by the kernel. 0 if the registration was
- * unsuccessful.
- */
-unsigned int rseq_feature_size = -1U;
-
 static int rseq_ownership;
 static int rseq_reg_success;	/* At least one rseq registration has succeded. */
 
@@ -111,6 +105,43 @@ int rseq_available(void)
 	}
 }
 
+/* The rseq areas need to be at least 32 bytes. */
+static
+unsigned get_rseq_min_alloc_size(void)
+{
+	unsigned int alloc_size = rseq_size;
+
+	if (alloc_size < ORIG_RSEQ_ALLOC_SIZE)
+		alloc_size = ORIG_RSEQ_ALLOC_SIZE;
+	return alloc_size;
+}
+
+/*
+ * Return the feature size supported by the kernel.
+ *
+ * Depending on the value returned by getauxval(AT_RSEQ_FEATURE_SIZE):
+ *
+ * 0:   Return ORIG_RSEQ_FEATURE_SIZE (20)
+ * > 0: Return the value from getauxval(AT_RSEQ_FEATURE_SIZE).
+ *
+ * It should never return a value below ORIG_RSEQ_FEATURE_SIZE.
+ */
+static
+unsigned int get_rseq_kernel_feature_size(void)
+{
+	unsigned long auxv_rseq_feature_size, auxv_rseq_align;
+
+	auxv_rseq_align = getauxval(AT_RSEQ_ALIGN);
+	assert(!auxv_rseq_align || auxv_rseq_align <= RSEQ_THREAD_AREA_ALLOC_SIZE);
+
+	auxv_rseq_feature_size = getauxval(AT_RSEQ_FEATURE_SIZE);
+	assert(!auxv_rseq_feature_size || auxv_rseq_feature_size <= RSEQ_THREAD_AREA_ALLOC_SIZE);
+	if (auxv_rseq_feature_size)
+		return auxv_rseq_feature_size;
+	else
+		return ORIG_RSEQ_FEATURE_SIZE;
+}
+
 int rseq_register_current_thread(void)
 {
 	int rc;
@@ -119,7 +150,7 @@ int rseq_register_current_thread(void)
 		/* Treat libc's ownership as a successful registration. */
 		return 0;
 	}
-	rc = sys_rseq(&__rseq_abi, rseq_size, 0, RSEQ_SIG);
+	rc = sys_rseq(&__rseq_abi, get_rseq_min_alloc_size(), 0, RSEQ_SIG);
 	if (rc) {
 		if (RSEQ_READ_ONCE(rseq_reg_success)) {
 			/* Incoherent success/failure within process. */
@@ -140,28 +171,12 @@ int rseq_unregister_current_thread(void)
 		/* Treat libc's ownership as a successful unregistration. */
 		return 0;
 	}
-	rc = sys_rseq(&__rseq_abi, rseq_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;
 }
 
-static
-unsigned int get_rseq_feature_size(void)
-{
-	unsigned long auxv_rseq_feature_size, auxv_rseq_align;
-
-	auxv_rseq_align = getauxval(AT_RSEQ_ALIGN);
-	assert(!auxv_rseq_align || auxv_rseq_align <= RSEQ_THREAD_AREA_ALLOC_SIZE);
-
-	auxv_rseq_feature_size = getauxval(AT_RSEQ_FEATURE_SIZE);
-	assert(!auxv_rseq_feature_size || auxv_rseq_feature_size <= RSEQ_THREAD_AREA_ALLOC_SIZE);
-	if (auxv_rseq_feature_size)
-		return auxv_rseq_feature_size;
-	else
-		return ORIG_RSEQ_FEATURE_SIZE;
-}
-
 static __attribute__((constructor))
 void rseq_init(void)
 {
@@ -178,28 +193,53 @@ void rseq_init(void)
 	}
 	if (libc_rseq_size_p && libc_rseq_offset_p && libc_rseq_flags_p &&
 			*libc_rseq_size_p != 0) {
+		unsigned int libc_rseq_size;
+
 		/* rseq registration owned by glibc */
 		rseq_offset = *libc_rseq_offset_p;
-		rseq_size = *libc_rseq_size_p;
+		libc_rseq_size = *libc_rseq_size_p;
 		rseq_flags = *libc_rseq_flags_p;
-		rseq_feature_size = get_rseq_feature_size();
-		if (rseq_feature_size > rseq_size)
-			rseq_feature_size = rseq_size;
+
+		/*
+		 * Previous versions of glibc expose the value
+		 * 32 even though the kernel only supported 20
+		 * bytes initially. Therefore treat 32 as a
+		 * special-case. glibc 2.40 exposes a 20 bytes
+		 * __rseq_size without using getauxval(3) to
+		 * query the supported size, while still allocating a 32
+		 * bytes area. Also treat 20 as a special-case.
+		 *
+		 * Special-cases are handled by using the following
+		 * value as active feature set size:
+		 *
+		 *   rseq_size = min(32, get_rseq_kernel_feature_size())
+		 */
+		switch (libc_rseq_size) {
+		case ORIG_RSEQ_FEATURE_SIZE:	/* Fallthrough. */
+		case ORIG_RSEQ_ALLOC_SIZE:
+		{
+			unsigned int rseq_kernel_feature_size = get_rseq_kernel_feature_size();
+
+			if (rseq_kernel_feature_size < ORIG_RSEQ_ALLOC_SIZE)
+				rseq_size = rseq_kernel_feature_size;
+			else
+				rseq_size = ORIG_RSEQ_ALLOC_SIZE;
+			break;
+		}
+		default:
+			/* Otherwise just use the __rseq_size from libc as rseq_size. */
+			rseq_size = libc_rseq_size;
+			break;
+		}
 		return;
 	}
 	rseq_ownership = 1;
 	if (!rseq_available()) {
 		rseq_size = 0;
-		rseq_feature_size = 0;
 		return;
 	}
 	rseq_offset = (void *)&__rseq_abi - rseq_thread_pointer();
 	rseq_flags = 0;
-	rseq_feature_size = get_rseq_feature_size();
-	if (rseq_feature_size == ORIG_RSEQ_FEATURE_SIZE)
-		rseq_size = ORIG_RSEQ_ALLOC_SIZE;
-	else
-		rseq_size = RSEQ_THREAD_AREA_ALLOC_SIZE;
 }
 
 static __attribute__((destructor))
@@ -209,7 +249,6 @@ void rseq_exit(void)
 		return;
 	rseq_offset = 0;
 	rseq_size = -1U;
-	rseq_feature_size = -1U;
 	rseq_ownership = 0;
 }
 
diff --git a/tools/testing/selftests/rseq/rseq.h b/tools/testing/selftests/rseq/rseq.h
index d7364ea4d201..4e217b620e0c 100644
--- a/tools/testing/selftests/rseq/rseq.h
+++ b/tools/testing/selftests/rseq/rseq.h
@@ -68,12 +68,6 @@ extern unsigned int rseq_size;
 /* Flags used during rseq registration. */
 extern unsigned int rseq_flags;
 
-/*
- * rseq feature size supported by the kernel. 0 if the registration was
- * unsuccessful.
- */
-extern unsigned int rseq_feature_size;
-
 enum rseq_mo {
 	RSEQ_MO_RELAXED = 0,
 	RSEQ_MO_CONSUME = 1,	/* Unused */
@@ -193,7 +187,7 @@ static inline uint32_t rseq_current_cpu(void)
 
 static inline bool rseq_node_id_available(void)
 {
-	return (int) rseq_feature_size >= rseq_offsetofend(struct rseq_abi, node_id);
+	return (int) rseq_size >= rseq_offsetofend(struct rseq_abi, node_id);
 }
 
 /*
@@ -207,7 +201,7 @@ static inline uint32_t rseq_current_node_id(void)
 
 static inline bool rseq_mm_cid_available(void)
 {
-	return (int) rseq_feature_size >= rseq_offsetofend(struct rseq_abi, mm_cid);
+	return (int) rseq_size >= rseq_offsetofend(struct rseq_abi, mm_cid);
 }
 
 static inline uint32_t rseq_current_mm_cid(void)
-- 
2.39.2

On 10/3/24 11:51, Mathieu Desnoyers wrote:
> Adapt the rseq.c/rseq.h code to follow GNU C library changes introduced by:
> 
> commit 2e456ccf0c34 ("Linux: Make __rseq_size useful for feature detection (bug 31965)")
> 
> Wihout this fix, rseq selftests for mm_cid fail:

Without

Can you change the short log to say "Fix mm_cid test failure"

This way it is clear that this fixes a test failure. You can
add "Adapt to glibc __rseq_size feature detection: in the
chabeg log for context.

> 
> ./run_param_test.sh
> Default parameters
> Running test spinlock
> Running compare-twice test spinlock
> Running mm_cid test spinlock
> Error: cpu id getter unavailable
> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> CC: Boqun Feng <boqun.feng@gmail.com>
> CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Cc: Shuah Khan <skhan@linuxfoundation.org>
> CC: Carlos O'Donell <carlos@redhat.com>
> CC: Florian Weimer <fweimer@redhat.com>
> ---
>   tools/testing/selftests/rseq/rseq.c | 109 +++++++++++++++++++---------
>   tools/testing/selftests/rseq/rseq.h |  10 +--
>   2 files changed, 76 insertions(+), 43 deletions(-)
> 
> diff --git a/tools/testing/selftests/rseq/rseq.c b/tools/testing/selftests/rseq/rseq.c
> index 96e812bdf8a4..3797bb0881da 100644
> --- a/tools/testing/selftests/rseq/rseq.c
> +++ b/tools/testing/selftests/rseq/rseq.c
> @@ -60,12 +60,6 @@ unsigned int rseq_size = -1U;
>   /* Flags used during rseq registration.  */
>   unsigned int rseq_flags;
>   
> -/*
> - * rseq feature size supported by the kernel. 0 if the registration was
> - * unsuccessful.
> - */
> -unsigned int rseq_feature_size = -1U;
> -
>   static int rseq_ownership;
>   static int rseq_reg_success;	/* At least one rseq registration has succeded. */
>   
> @@ -111,6 +105,43 @@ int rseq_available(void)
>   	}
>   }
>   
> +/* The rseq areas need to be at least 32 bytes. */
> +static
> +unsigned get_rseq_min_alloc_size(void)
> +{
> +	unsigned int alloc_size = rseq_size;
> +
> +	if (alloc_size < ORIG_RSEQ_ALLOC_SIZE)
> +		alloc_size = ORIG_RSEQ_ALLOC_SIZE;
> +	return alloc_size;
> +}
> +
> +/*
> + * Return the feature size supported by the kernel.
> + *
> + * Depending on the value returned by getauxval(AT_RSEQ_FEATURE_SIZE):
> + *
> + * 0:   Return ORIG_RSEQ_FEATURE_SIZE (20)
> + * > 0: Return the value from getauxval(AT_RSEQ_FEATURE_SIZE).
> + *
> + * It should never return a value below ORIG_RSEQ_FEATURE_SIZE.
> + */
> +static
> +unsigned int get_rseq_kernel_feature_size(void)
> +{
> +	unsigned long auxv_rseq_feature_size, auxv_rseq_align;
> +
> +	auxv_rseq_align = getauxval(AT_RSEQ_ALIGN);
> +	assert(!auxv_rseq_align || auxv_rseq_align <= RSEQ_THREAD_AREA_ALLOC_SIZE);
> +
> +	auxv_rseq_feature_size = getauxval(AT_RSEQ_FEATURE_SIZE);
> +	assert(!auxv_rseq_feature_size || auxv_rseq_feature_size <= RSEQ_THREAD_AREA_ALLOC_SIZE);
> +	if (auxv_rseq_feature_size)
> +		return auxv_rseq_feature_size;
> +	else
> +		return ORIG_RSEQ_FEATURE_SIZE;
> +}
> +
>   int rseq_register_current_thread(void)
>   {
>   	int rc;
> @@ -119,7 +150,7 @@ int rseq_register_current_thread(void)
>   		/* Treat libc's ownership as a successful registration. */
>   		return 0;
>   	}
> -	rc = sys_rseq(&__rseq_abi, rseq_size, 0, RSEQ_SIG);
> +	rc = sys_rseq(&__rseq_abi, get_rseq_min_alloc_size(), 0, RSEQ_SIG);
>   	if (rc) {
>   		if (RSEQ_READ_ONCE(rseq_reg_success)) {
>   			/* Incoherent success/failure within process. */
> @@ -140,28 +171,12 @@ int rseq_unregister_current_thread(void)
>   		/* Treat libc's ownership as a successful unregistration. */
>   		return 0;
>   	}
> -	rc = sys_rseq(&__rseq_abi, rseq_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;
>   }
>   
> -static
> -unsigned int get_rseq_feature_size(void)
> -{
> -	unsigned long auxv_rseq_feature_size, auxv_rseq_align;
> -
> -	auxv_rseq_align = getauxval(AT_RSEQ_ALIGN);
> -	assert(!auxv_rseq_align || auxv_rseq_align <= RSEQ_THREAD_AREA_ALLOC_SIZE);
> -
> -	auxv_rseq_feature_size = getauxval(AT_RSEQ_FEATURE_SIZE);
> -	assert(!auxv_rseq_feature_size || auxv_rseq_feature_size <= RSEQ_THREAD_AREA_ALLOC_SIZE);
> -	if (auxv_rseq_feature_size)
> -		return auxv_rseq_feature_size;
> -	else
> -		return ORIG_RSEQ_FEATURE_SIZE;
> -}
> -
>   static __attribute__((constructor))
>   void rseq_init(void)
>   {
> @@ -178,28 +193,53 @@ void rseq_init(void)
>   	}
>   	if (libc_rseq_size_p && libc_rseq_offset_p && libc_rseq_flags_p &&
>   			*libc_rseq_size_p != 0) {
> +		unsigned int libc_rseq_size;
> +
>   		/* rseq registration owned by glibc */
>   		rseq_offset = *libc_rseq_offset_p;
> -		rseq_size = *libc_rseq_size_p;
> +		libc_rseq_size = *libc_rseq_size_p;
>   		rseq_flags = *libc_rseq_flags_p;
> -		rseq_feature_size = get_rseq_feature_size();
> -		if (rseq_feature_size > rseq_size)
> -			rseq_feature_size = rseq_size;
> +
> +		/*
> +		 * Previous versions of glibc expose the value
> +		 * 32 even though the kernel only supported 20
> +		 * bytes initially. Therefore treat 32 as a
> +		 * special-case. glibc 2.40 exposes a 20 bytes
> +		 * __rseq_size without using getauxval(3) to
> +		 * query the supported size, while still allocating a 32
> +		 * bytes area. Also treat 20 as a special-case.
> +		 *
> +		 * Special-cases are handled by using the following
> +		 * value as active feature set size:
> +		 *
> +		 *   rseq_size = min(32, get_rseq_kernel_feature_size())
> +		 */
> +		switch (libc_rseq_size) {
> +		case ORIG_RSEQ_FEATURE_SIZE:	/* Fallthrough. */
> +		case ORIG_RSEQ_ALLOC_SIZE:
> +		{
> +			unsigned int rseq_kernel_feature_size = get_rseq_kernel_feature_size();
> +
> +			if (rseq_kernel_feature_size < ORIG_RSEQ_ALLOC_SIZE)
> +				rseq_size = rseq_kernel_feature_size;
> +			else
> +				rseq_size = ORIG_RSEQ_ALLOC_SIZE;
> +			break;
> +		}
> +		default:
> +			/* Otherwise just use the __rseq_size from libc as rseq_size. */
> +			rseq_size = libc_rseq_size;
> +			break;
> +		}
>   		return;
>   	}
>   	rseq_ownership = 1;
>   	if (!rseq_available()) {
>   		rseq_size = 0;
> -		rseq_feature_size = 0;
>   		return;
>   	}
>   	rseq_offset = (void *)&__rseq_abi - rseq_thread_pointer();
>   	rseq_flags = 0;
> -	rseq_feature_size = get_rseq_feature_size();
> -	if (rseq_feature_size == ORIG_RSEQ_FEATURE_SIZE)
> -		rseq_size = ORIG_RSEQ_ALLOC_SIZE;
> -	else
> -		rseq_size = RSEQ_THREAD_AREA_ALLOC_SIZE;
>   }
>   
>   static __attribute__((destructor))
> @@ -209,7 +249,6 @@ void rseq_exit(void)
>   		return;
>   	rseq_offset = 0;
>   	rseq_size = -1U;
> -	rseq_feature_size = -1U;
>   	rseq_ownership = 0;
>   }
>   
> diff --git a/tools/testing/selftests/rseq/rseq.h b/tools/testing/selftests/rseq/rseq.h
> index d7364ea4d201..4e217b620e0c 100644
> --- a/tools/testing/selftests/rseq/rseq.h
> +++ b/tools/testing/selftests/rseq/rseq.h
> @@ -68,12 +68,6 @@ extern unsigned int rseq_size;
>   /* Flags used during rseq registration. */
>   extern unsigned int rseq_flags;
>   
> -/*
> - * rseq feature size supported by the kernel. 0 if the registration was
> - * unsuccessful.
> - */
> -extern unsigned int rseq_feature_size;
> -
>   enum rseq_mo {
>   	RSEQ_MO_RELAXED = 0,
>   	RSEQ_MO_CONSUME = 1,	/* Unused */
> @@ -193,7 +187,7 @@ static inline uint32_t rseq_current_cpu(void)
>   
>   static inline bool rseq_node_id_available(void)
>   {
> -	return (int) rseq_feature_size >= rseq_offsetofend(struct rseq_abi, node_id);
> +	return (int) rseq_size >= rseq_offsetofend(struct rseq_abi, node_id);
>   }
>   
>   /*
> @@ -207,7 +201,7 @@ static inline uint32_t rseq_current_node_id(void)
>   
>   static inline bool rseq_mm_cid_available(void)
>   {
> -	return (int) rseq_feature_size >= rseq_offsetofend(struct rseq_abi, mm_cid);
> +	return (int) rseq_size >= rseq_offsetofend(struct rseq_abi, mm_cid);
>   }
>   
>   static inline uint32_t rseq_current_mm_cid(void)

thanks,
-- Shuah

On 2024-10-04 00:40, Shuah Khan wrote:
> On 10/3/24 11:51, Mathieu Desnoyers wrote:
>> Adapt the rseq.c/rseq.h code to follow GNU C library changes 
>> introduced by:
>>
>> commit 2e456ccf0c34 ("Linux: Make __rseq_size useful for feature 
>> detection (bug 31965)")
>>
>> Wihout this fix, rseq selftests for mm_cid fail:
> 
> Without
> 
> Can you change the short log to say "Fix mm_cid test failure"
> 
> This way it is clear that this fixes a test failure. You can
> add "Adapt to glibc __rseq_size feature detection: in the
> chabeg log for context.

Yes, I will update it and re-send the series.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com