Date: Sat, 18 May 2024 12:29:00 +0900
From: Masahiro Yamada <masahiroy@...nel.org>
To: Kees Cook <keescook@...omium.org>
Cc: Andy Lutomirski <luto@...capital.net>, Will Drewry <wad@...omium.org>, 
	linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Alexandre Belloni <alexandre.belloni@...tlin.com>, 
	Benjamin Tissoires <benjamin.tissoires@...hat.com>, 
	Christian Borntraeger <borntraeger@...ux.ibm.com>, Claudio Imbrenda <imbrenda@...ux.ibm.com>, 
	David Hildenbrand <david@...hat.com>, Janosch Frank <frankja@...ux.ibm.com>, Jiri Kosina <jikos@...nel.org>, 
	Shuah Khan <shuah@...nel.org>, bpf@...r.kernel.org, kvm@...r.kernel.org, 
	linux-input@...r.kernel.org, linux-rtc@...r.kernel.org
Subject: Re: [PATCH 1/2] selftests: harness: remove unneeded __constructor_order_last()

On Sat, May 18, 2024 at 8:26 AM Kees Cook <keescook@...omium.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@...nel.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

Powered by blists - more mailing lists