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