[0/2] selftests: harness: refactor __constructor_order

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(-)

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

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

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.