Re: [PATCH 2/2] selftests/rseq: Adapt to glibc __rseq_size feature detection

[Shuah Khan]

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@xxxxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> CC: Boqun Feng <boqun.feng@xxxxxxxxx>
> CC: "Paul E. McKenney" <paulmck@xxxxxxxxxxxxxxxxxx>
> Cc: Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>
> CC: Carlos O'Donell <carlos@xxxxxxxxxx>
> CC: Florian Weimer <fweimer@xxxxxxxxxx>
> ---
>   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