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