[PATCH 0/2] selftests: harness: refactor __constructor_order

[PATCH 0/2] selftests: harness: refactor __constructor_order

[Masahiro Yamada]

This series refactors __constructor_order because
__constructor_order_last() is unneeded.

BTW, the comments in kselftest_harness.h was confusing to me.

As far as I tested, all arches executed constructors in the forward
order.

[test code]

  #include <stdio.h>

  static int x;

  static void __attribute__((constructor)) increment(void)
  {
           x += 1;
  }

  static void __attribute__((constructor)) multiply(void)
  {
          x *= 2;
  }

  int main(void)
  {
          printf("foo = %d\n", x);
          return 0;
  }

It should print 2 for forward order systems, 1 for reverse order systems.

I executed it on some archtes by using QEMU. I always got 2.



Masahiro Yamada (2):
  selftests: harness: remove unneeded __constructor_order_last()
  selftests: harness: rename __constructor_order for clarification

 .../drivers/s390x/uvdevice/test_uvdevice.c     |  6 ------
 tools/testing/selftests/hid/hid_bpf.c          |  6 ------
 tools/testing/selftests/kselftest_harness.h    | 18 ++++--------------
 tools/testing/selftests/rtc/rtctest.c          |  7 -------
 4 files changed, 4 insertions(+), 33 deletions(-)

-- 
2.40.1

[PATCH 1/2] selftests: harness: remove unneeded __constructor_order_last()

[Masahiro Yamada]

__constructor_order_last() is unneeded.

If __constructor_order_last() is not called on reverse-order systems,
__constructor_order will remain 0 instead of being set to
_CONSTRUCTOR_ORDER_BACKWARD (= -1).

__LIST_APPEND() will still take the 'else' branch, so there is no
difference in the behavior.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 .../selftests/drivers/s390x/uvdevice/test_uvdevice.c   |  6 ------
 tools/testing/selftests/hid/hid_bpf.c                  |  6 ------
 tools/testing/selftests/kselftest_harness.h            | 10 +---------
 tools/testing/selftests/rtc/rtctest.c                  |  7 -------
 4 files changed, 1 insertion(+), 28 deletions(-)

diff --git a/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c b/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c
index ea0cdc37b44f..7ee7492138c6 100644
--- a/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c
+++ b/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c
@@ -257,12 +257,6 @@ TEST_F(attest_fixture, att_inval_addr)
 	att_inval_addr_test(&self->uvio_attest.meas_addr, _metadata, self);
 }
 
-static void __attribute__((constructor)) __constructor_order_last(void)
-{
-	if (!__constructor_order)
-		__constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;
-}
-
 int main(int argc, char **argv)
 {
 	int fd = open(UV_PATH, O_ACCMODE);
diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c
index 2cf96f818f25..f47feef2aced 100644
--- a/tools/testing/selftests/hid/hid_bpf.c
+++ b/tools/testing/selftests/hid/hid_bpf.c
@@ -853,12 +853,6 @@ static int libbpf_print_fn(enum libbpf_print_level level,
 	return 0;
 }
 
-static void __attribute__((constructor)) __constructor_order_last(void)
-{
-	if (!__constructor_order)
-		__constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;
-}
-
 int main(int argc, char **argv)
 {
 	/* Use libbpf 1.0 API mode */
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index ba3ddeda24bf..60c1cf5b0f0d 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -444,12 +444,6 @@
  * Use once to append a main() to the test file.
  */
 #define TEST_HARNESS_MAIN \
-	static void __attribute__((constructor)) \
-	__constructor_order_last(void) \
-	{ \
-		if (!__constructor_order) \
-			__constructor_order = _CONSTRUCTOR_ORDER_BACKWARD; \
-	} \
 	int main(int argc, char **argv) { \
 		return test_harness_run(argc, argv); \
 	}
@@ -846,7 +840,6 @@ static struct __fixture_metadata *__fixture_list = &_fixture_global;
 static int __constructor_order;
 
 #define _CONSTRUCTOR_ORDER_FORWARD   1
-#define _CONSTRUCTOR_ORDER_BACKWARD -1
 
 static inline void __register_fixture(struct __fixture_metadata *f)
 {
@@ -1272,8 +1265,7 @@ static int test_harness_run(int argc, char **argv)
 
 static void __attribute__((constructor)) __constructor_order_first(void)
 {
-	if (!__constructor_order)
-		__constructor_order = _CONSTRUCTOR_ORDER_FORWARD;
+	__constructor_order = _CONSTRUCTOR_ORDER_FORWARD;
 }
 
 #endif  /* __KSELFTEST_HARNESS_H */
diff --git a/tools/testing/selftests/rtc/rtctest.c b/tools/testing/selftests/rtc/rtctest.c
index 63ce02d1d5cc..9647b14b47c5 100644
--- a/tools/testing/selftests/rtc/rtctest.c
+++ b/tools/testing/selftests/rtc/rtctest.c
@@ -410,13 +410,6 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) {
 	ASSERT_EQ(new, secs);
 }
 
-static void __attribute__((constructor))
-__constructor_order_last(void)
-{
-	if (!__constructor_order)
-		__constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;
-}
-
 int main(int argc, char **argv)
 {
 	switch (argc) {
-- 
2.40.1

Re: [PATCH 1/2] selftests: harness: remove unneeded __constructor_order_last()

[Kees Cook]

On Fri, May 17, 2024 at 08:45:05PM +0900, Masahiro Yamada wrote:
> __constructor_order_last() is unneeded.
> 
> If __constructor_order_last() is not called on reverse-order systems,
> __constructor_order will remain 0 instead of being set to
> _CONSTRUCTOR_ORDER_BACKWARD (= -1).
> 
> __LIST_APPEND() will still take the 'else' branch, so there is no
> difference in the behavior.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
>  .../selftests/drivers/s390x/uvdevice/test_uvdevice.c   |  6 ------
>  tools/testing/selftests/hid/hid_bpf.c                  |  6 ------
>  tools/testing/selftests/kselftest_harness.h            | 10 +---------
>  tools/testing/selftests/rtc/rtctest.c                  |  7 -------
>  4 files changed, 1 insertion(+), 28 deletions(-)
> 
> diff --git a/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c b/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c
> index ea0cdc37b44f..7ee7492138c6 100644
> --- a/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c
> +++ b/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c
> @@ -257,12 +257,6 @@ TEST_F(attest_fixture, att_inval_addr)
>  	att_inval_addr_test(&self->uvio_attest.meas_addr, _metadata, self);
>  }
>  
> -static void __attribute__((constructor)) __constructor_order_last(void)
> -{
> -	if (!__constructor_order)
> -		__constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;
> -}
> -
>  int main(int argc, char **argv)
>  {
>  	int fd = open(UV_PATH, O_ACCMODE);
> diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c
> index 2cf96f818f25..f47feef2aced 100644
> --- a/tools/testing/selftests/hid/hid_bpf.c
> +++ b/tools/testing/selftests/hid/hid_bpf.c
> @@ -853,12 +853,6 @@ static int libbpf_print_fn(enum libbpf_print_level level,
>  	return 0;
>  }
>  
> -static void __attribute__((constructor)) __constructor_order_last(void)
> -{
> -	if (!__constructor_order)
> -		__constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;
> -}
> -
>  int main(int argc, char **argv)
>  {
>  	/* Use libbpf 1.0 API mode */
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index ba3ddeda24bf..60c1cf5b0f0d 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -444,12 +444,6 @@
>   * Use once to append a main() to the test file.
>   */
>  #define TEST_HARNESS_MAIN \
> -	static void __attribute__((constructor)) \
> -	__constructor_order_last(void) \
> -	{ \
> -		if (!__constructor_order) \
> -			__constructor_order = _CONSTRUCTOR_ORDER_BACKWARD; \
> -	} \
>  	int main(int argc, char **argv) { \
>  		return test_harness_run(argc, argv); \
>  	}

This won't work. All constructors are executed, so we have to figure
out which is run _first_. Switching this to a boolean means we gain no
information about ordering: it'll always be set to "true". We need to
detect which constructor sets it first so that we can walk the lists
(that are built via all the constructors in between) in the correct
order.

> @@ -846,7 +840,6 @@ static struct __fixture_metadata *__fixture_list = &_fixture_global;
>  static int __constructor_order;
>  
>  #define _CONSTRUCTOR_ORDER_FORWARD   1
> -#define _CONSTRUCTOR_ORDER_BACKWARD -1
>  
>  static inline void __register_fixture(struct __fixture_metadata *f)
>  {
> @@ -1272,8 +1265,7 @@ static int test_harness_run(int argc, char **argv)
>  
>  static void __attribute__((constructor)) __constructor_order_first(void)
>  {
> -	if (!__constructor_order)
> -		__constructor_order = _CONSTRUCTOR_ORDER_FORWARD;
> +	__constructor_order = _CONSTRUCTOR_ORDER_FORWARD;
>  }
>  
>  #endif  /* __KSELFTEST_HARNESS_H */
> diff --git a/tools/testing/selftests/rtc/rtctest.c b/tools/testing/selftests/rtc/rtctest.c
> index 63ce02d1d5cc..9647b14b47c5 100644
> --- a/tools/testing/selftests/rtc/rtctest.c
> +++ b/tools/testing/selftests/rtc/rtctest.c
> @@ -410,13 +410,6 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) {
>  	ASSERT_EQ(new, secs);
>  }
>  
> -static void __attribute__((constructor))
> -__constructor_order_last(void)
> -{
> -	if (!__constructor_order)
> -		__constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;
> -}
> -
>  int main(int argc, char **argv)
>  {
>  	switch (argc) {

A better question is why these tests are open-coding the execution of
"main"...

-- 
Kees Cook

Re: [PATCH 1/2] selftests: harness: remove unneeded __constructor_order_last()

[Masahiro Yamada]

On Sat, May 18, 2024 at 8:26 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, May 17, 2024 at 08:45:05PM +0900, Masahiro Yamada wrote:
> > __constructor_order_last() is unneeded.
> >
> > If __constructor_order_last() is not called on reverse-order systems,
> > __constructor_order will remain 0 instead of being set to
> > _CONSTRUCTOR_ORDER_BACKWARD (= -1).
> >
> > __LIST_APPEND() will still take the 'else' branch, so there is no
> > difference in the behavior.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  .../selftests/drivers/s390x/uvdevice/test_uvdevice.c   |  6 ------
> >  tools/testing/selftests/hid/hid_bpf.c                  |  6 ------
> >  tools/testing/selftests/kselftest_harness.h            | 10 +---------
> >  tools/testing/selftests/rtc/rtctest.c                  |  7 -------
> >  4 files changed, 1 insertion(+), 28 deletions(-)
> >
> > diff --git a/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c b/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c
> > index ea0cdc37b44f..7ee7492138c6 100644
> > --- a/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c
> > +++ b/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c
> > @@ -257,12 +257,6 @@ TEST_F(attest_fixture, att_inval_addr)
> >       att_inval_addr_test(&self->uvio_attest.meas_addr, _metadata, self);
> >  }
> >
> > -static void __attribute__((constructor)) __constructor_order_last(void)
> > -{
> > -     if (!__constructor_order)
> > -             __constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;
> > -}
> > -
> >  int main(int argc, char **argv)
> >  {
> >       int fd = open(UV_PATH, O_ACCMODE);
> > diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c
> > index 2cf96f818f25..f47feef2aced 100644
> > --- a/tools/testing/selftests/hid/hid_bpf.c
> > +++ b/tools/testing/selftests/hid/hid_bpf.c
> > @@ -853,12 +853,6 @@ static int libbpf_print_fn(enum libbpf_print_level level,
> >       return 0;
> >  }
> >
> > -static void __attribute__((constructor)) __constructor_order_last(void)
> > -{
> > -     if (!__constructor_order)
> > -             __constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;
> > -}
> > -
> >  int main(int argc, char **argv)
> >  {
> >       /* Use libbpf 1.0 API mode */
> > diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> > index ba3ddeda24bf..60c1cf5b0f0d 100644
> > --- a/tools/testing/selftests/kselftest_harness.h
> > +++ b/tools/testing/selftests/kselftest_harness.h
> > @@ -444,12 +444,6 @@
> >   * Use once to append a main() to the test file.
> >   */
> >  #define TEST_HARNESS_MAIN \
> > -     static void __attribute__((constructor)) \
> > -     __constructor_order_last(void) \
> > -     { \
> > -             if (!__constructor_order) \
> > -                     __constructor_order = _CONSTRUCTOR_ORDER_BACKWARD; \
> > -     } \
> >       int main(int argc, char **argv) { \
> >               return test_harness_run(argc, argv); \
> >       }
>
> This won't work. All constructors are executed, so we have to figure
> out which is run _first_. Switching this to a boolean means we gain no
> information about ordering: it'll always be set to "true".



It will be set to "true" eventually,
but __LIST_APPEND() still sees "false"
on backward-order systems.




Let's see how the following is expanded.


 #include "kselftest_harness.h"

 TEST(foo) { ... }

 TEST(bar) { ... }



You will get something as follows:



void _attribute__((constructor)) __constructor_order_first(void)
{
        __constructor_order_forward = true;
}

void __attribute__((constructor)) _register_foo(void)
{
      __register_test(&foo_object); // call __LIST_APPEND() for foo
}


void __attribute__((constructor)) _register_bar(void)
{
      __register_test(&bar_object); // call __LIST_APPEND() for bar
}




On forward-order systems, the constructors are executed in this order:


  __constructor_order_first() -> _register_foo() -> _register_bar()


So, __LIST_APPEND will see "true".




On backward-order systems, the constructors are executed in this order:


  _register_bar() -> _register_foo() -> __constructor_order_first()


So, __LIST_APPEND will see "false" since __construtor_order_first()
has not been called yet.



Correct me if I am wrong.




> We need to
> detect which constructor sets it first so that we can walk the lists
> (that are built via all the constructors in between)


You have a wrong assumption here.

TEST() macros may not be placed in-between.


   #include "kselftest_harness.h"

   TEST_HARNESS_MAIN
   TEST(foo) { ... }
   TEST(bar) { ... }


This is perfectly correct code, because there is no reason to force
"Please put TEST_HARNESS_MAIN at the end of the file".

It is just a "coding style".


If the test code were written in such style with
the current harness implementation, __constructor_order
would be zero instead of _CONSTRUCTOR_ORDER_BACKWARD
on backward-order systems.
__LIST_APPEND() still works correctly, though.









> >  #endif  /* __KSELFTEST_HARNESS_H */
> > diff --git a/tools/testing/selftests/rtc/rtctest.c b/tools/testing/selftests/rtc/rtctest.c
> > index 63ce02d1d5cc..9647b14b47c5 100644
> > --- a/tools/testing/selftests/rtc/rtctest.c
> > +++ b/tools/testing/selftests/rtc/rtctest.c
> > @@ -410,13 +410,6 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) {
> >       ASSERT_EQ(new, secs);
> >  }
> >
> > -static void __attribute__((constructor))
> > -__constructor_order_last(void)
> > -{
> > -     if (!__constructor_order)
> > -             __constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;
> > -}
> > -
> >  int main(int argc, char **argv)
> >  {
> >       switch (argc) {
>
> A better question is why these tests are open-coding the execution of
> "main"...



It is open __unnecessary__ coding.



If __constructor_order_last() had not existed in the first place,
such things would not have occured.








--
Best Regards
Masahiro Yamada

Re: [PATCH 1/2] selftests: harness: remove unneeded __constructor_order_last()

[Kees Cook]

On Sat, May 18, 2024 at 12:29:00PM +0900, Masahiro Yamada wrote:
> It will be set to "true" eventually,
> but __LIST_APPEND() still sees "false"
> on backward-order systems.

Ah, yes -- you are right. I looked through the commit history (I had
to go back to when the seccomp test, and the harness, was out of tree).
There was a time when the logic happened during the list walking, rather
than during list _creation_. I was remembering the former.

So, yes, let's make this change. As you say, it also solves for defining
TEST_HARNESS_MAIN before the tests. Thank you! I'd still like to replace
all the open-coded TEST_HARNESS_MAIN calls, though.

-- 
Kees Cook

Re: [PATCH 0/2] selftests: harness: refactor __constructor_order

[Kees Cook]

On Fri, May 17, 2024 at 08:45:04PM +0900, Masahiro Yamada wrote:
> 
> This series refactors __constructor_order because
> __constructor_order_last() is unneeded.
> 
> BTW, the comments in kselftest_harness.h was confusing to me.
> 
> As far as I tested, all arches executed constructors in the forward
> order.
> 
> [test code]
> 
>   #include <stdio.h>
> 
>   static int x;
> 
>   static void __attribute__((constructor)) increment(void)
>   {
>            x += 1;
>   }
> 
>   static void __attribute__((constructor)) multiply(void)
>   {
>           x *= 2;
>   }
> 
>   int main(void)
>   {
>           printf("foo = %d\n", x);
>           return 0;
>   }
> 
> It should print 2 for forward order systems, 1 for reverse order systems.
> 
> I executed it on some archtes by using QEMU. I always got 2.

IIRC, and it was a long time ago now, it was actually a difference
between libc implementations where I encountered the problem. Maybe
glibc vs Bionic?

-Kees

-- 
Kees Cook