Re: [v3,v3,1/2] selftests/harness: order TEST_F and XFAIL_ADD constructors

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
selftests/harness: order TEST_F and XFAIL_ADD constructors

This commit fixes a constructor ordering race where XFAIL_ADD() could run
before TEST_F() and read a NULL _##fixture_##test##_object pointer. The
fix uses constructor priorities to ensure TEST_F runs first.

> TEST_F() allocates and registers its struct __test_metadata via mmap()
> inside its constructor, and only then assigns the
> _##fixture_##test##_object pointer.
>
> XFAIL_ADD() runs in a constructor too and reads
> _##fixture_##test##_object to initialize xfail->test. If XFAIL_ADD runs
> first, xfail->test can be NULL and the expected failure will be reported
> as FAIL.
>
> Use constructor priorities to ensure TEST_F registration runs before
> XFAIL_ADD, without adding extra state or runtime lookups.
>
> Fixes: 2709473c9386 ("selftests: kselftest_harness: support using xfail")

Should the Fixes: tag point to commit 24cf65a62266 instead?

The existing tag points to 2709473c9386 ("selftests: kselftest_harness:
support using xfail"), but that commit used static initialization
(.test = &object) which had no race condition.

The race was actually introduced by commit 24cf65a62266
("selftests/harness: Share _metadata between forked processes"), which
changed TEST_F to use constructor-based allocation and also changed
XFAIL_ADD to read the pointer value in its constructor. This created the
race where XFAIL_ADD could run before TEST_F and read NULL.

> Signed-off-by: Sun Jian <sun.jian.kdev@gmail.com>

> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index 16a119a4656c..4afaef01c22e 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -76,6 +76,9 @@ static inline void __kselftest_memset_safe(void *s, int c, size_t n)
>  		memset(s, c, n);
>  }
>
> +#define KSELFTEST_PRIO_TEST_F  20000
> +#define KSELFTEST_PRIO_XFAIL   20001
> +
>  #define TEST_TIMEOUT_DEFAULT 30
>
>  /* Utilities exposed to the test definitions */
> @@ -465,7 +468,7 @@ static inline void __kselftest_memset_safe(void *s, int c, size_t n)
>  			fixture_name##_teardown(_metadata, self, variant); \
>  	} \
>  	static struct __test_metadata *_##fixture_name##_##test_name##_object; \
> -	static void __attribute__((constructor)) \
> +	static void __attribute__((constructor(KSELFTEST_PRIO_TEST_F))) \
>  			_register_##fixture_name##_##test_name(void) \
>  	{ \
>  		struct __test_metadata *object = mmap(NULL, sizeof(*object), \
> @@ -880,7 +883,7 @@ struct __test_xfail {
>  		.fixture = &_##fixture_name##_fixture_object, \
>  		.variant = &_##fixture_name##_##variant_name##_object, \
>  	}; \
> -	static void __attribute__((constructor)) \
> +	static void __attribute__((constructor(KSELFTEST_PRIO_XFAIL))) \
>  		_register_##fixture_name##_##variant_name##_##test_name##_xfail(void) \
>  	{ \
>  		_##fixture_name##_##variant_name##_##test_name##_xfail.test = \