* [PATCH 0/2] selftests/harness: Run TEARDOWN for ASSERT failures @ 2022-03-24 23:19 Kees Cook 2022-03-24 23:19 ` [PATCH 1/2] " Kees Cook 2022-03-24 23:19 ` [PATCH 2/2] selftests/harness: Pass variant to teardown Kees Cook 0 siblings, 2 replies; 6+ messages in thread From: Kees Cook @ 2022-03-24 23:19 UTC (permalink / raw) To: shuah Cc: Kees Cook, Willem de Bruijn, Andy Lutomirski, Will Drewry, Jakub Kicinski, linux-kernel, linux-kselftest, linux-hardening Hi, I've needed TEARDOWN for ASSERT tests for a while now. It just took me a while to figure out how to do it. Also included is a patch from Willem de Bruijn that got lost, which makes sure that variants are passed to TEARDOWN, since they may be needed there too. Thanks! -Kees Kees Cook (1): selftests/harness: Run TEARDOWN for ASSERT failures Willem de Bruijn (1): selftests/harness: Pass variant to teardown tools/testing/selftests/kselftest_harness.h | 59 ++++++++++++++------- 1 file changed, 40 insertions(+), 19 deletions(-) -- 2.32.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] selftests/harness: Run TEARDOWN for ASSERT failures 2022-03-24 23:19 [PATCH 0/2] selftests/harness: Run TEARDOWN for ASSERT failures Kees Cook @ 2022-03-24 23:19 ` Kees Cook 2022-03-25 19:37 ` Shuah Khan 2022-03-24 23:19 ` [PATCH 2/2] selftests/harness: Pass variant to teardown Kees Cook 1 sibling, 1 reply; 6+ messages in thread From: Kees Cook @ 2022-03-24 23:19 UTC (permalink / raw) To: shuah Cc: Kees Cook, Andy Lutomirski, Will Drewry, linux-kselftest, Willem de Bruijn, Jakub Kicinski, linux-kernel, linux-hardening The kselftest test harness has traditionally not run the registered TEARDOWN handler when a test encountered an ASSERT. This creates unexpected situations and tests need to be very careful about using ASSERT, which seems a needless hurdle for test writers. Because of the harness's design for optional failure handlers, the original implementation of ASSERT used an abort() to immediately stop execution, but that meant the context for running teardown was lost. Instead, use setjmp/longjmp so that teardown can be done. Failed SETUP routines continue to not be followed by TEARDOWN, though. Cc: Andy Lutomirski <luto@amacapital.net> Cc: Will Drewry <wad@chromium.org> Cc: Shuah Khan <shuah@kernel.org> Cc: linux-kselftest@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- tools/testing/selftests/kselftest_harness.h | 49 ++++++++++++++------- 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index 471eaa7b3a3f..bef08f824eb5 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -64,6 +64,7 @@ #include <sys/types.h> #include <sys/wait.h> #include <unistd.h> +#include <setjmp.h> #include "kselftest.h" @@ -183,7 +184,10 @@ struct __test_metadata *_metadata, \ struct __fixture_variant_metadata *variant) \ { \ - test_name(_metadata); \ + _metadata->setup_completed = true; \ + if (setjmp(_metadata->env) == 0) \ + test_name(_metadata); \ + __test_check_assert(_metadata); \ } \ static struct __test_metadata _##test_name##_object = \ { .name = #test_name, \ @@ -356,10 +360,7 @@ * Defines a test that depends on a fixture (e.g., is part of a test case). * Very similar to TEST() except that *self* is the setup instance of fixture's * datatype exposed for use by the implementation. - * - * Warning: use of ASSERT_* here will skip TEARDOWN. */ -/* TODO(wad) register fixtures on dedicated test lists. */ #define TEST_F(fixture_name, test_name) \ __TEST_F_IMPL(fixture_name, test_name, -1, TEST_TIMEOUT_DEFAULT) @@ -381,12 +382,17 @@ /* fixture data is alloced, setup, and torn down per call. */ \ FIXTURE_DATA(fixture_name) self; \ memset(&self, 0, sizeof(FIXTURE_DATA(fixture_name))); \ - fixture_name##_setup(_metadata, &self, variant->data); \ - /* Let setup failure terminate early. */ \ - if (!_metadata->passed) \ - return; \ - fixture_name##_##test_name(_metadata, &self, variant->data); \ - fixture_name##_teardown(_metadata, &self); \ + if (setjmp(_metadata->env) == 0) { \ + fixture_name##_setup(_metadata, &self, variant->data); \ + /* Let setup failure terminate early. */ \ + if (!_metadata->passed) \ + return; \ + _metadata->setup_completed = true; \ + fixture_name##_##test_name(_metadata, &self, variant->data); \ + } \ + if (_metadata->setup_completed) \ + fixture_name##_teardown(_metadata, &self); \ + __test_check_assert(_metadata); \ } \ static struct __test_metadata \ _##fixture_name##_##test_name##_object = { \ @@ -683,7 +689,7 @@ */ #define OPTIONAL_HANDLER(_assert) \ for (; _metadata->trigger; _metadata->trigger = \ - __bail(_assert, _metadata->no_print, _metadata->step)) + __bail(_assert, _metadata)) #define __INC_STEP(_metadata) \ /* Keep "step" below 255 (which is used for "SKIP" reporting). */ \ @@ -830,6 +836,9 @@ struct __test_metadata { bool timed_out; /* did this test timeout instead of exiting? */ __u8 step; bool no_print; /* manual trigger when TH_LOG_STREAM is not available */ + bool aborted; /* stopped test due to failed ASSERT */ + bool setup_completed; /* did setup finish? */ + jmp_buf env; /* for exiting out of test early */ struct __test_results *results; struct __test_metadata *prev, *next; }; @@ -848,16 +857,26 @@ static inline void __register_test(struct __test_metadata *t) __LIST_APPEND(t->fixture->tests, t); } -static inline int __bail(int for_realz, bool no_print, __u8 step) +static inline int __bail(int for_realz, struct __test_metadata *t) { + /* if this is ASSERT, return immediately. */ if (for_realz) { - if (no_print) - _exit(step); - abort(); + t->aborted = true; + longjmp(t->env, 1); } + /* otherwise, end the for loop and continue. */ return 0; } +static inline void __test_check_assert(struct __test_metadata *t) +{ + if (t->aborted) { + if (t->no_print) + _exit(t->step); + abort(); + } +} + struct __test_metadata *__active_test; static void __timeout_handler(int sig, siginfo_t *info, void *ucontext) { -- 2.32.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] selftests/harness: Run TEARDOWN for ASSERT failures 2022-03-24 23:19 ` [PATCH 1/2] " Kees Cook @ 2022-03-25 19:37 ` Shuah Khan 2022-03-25 22:36 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: Shuah Khan @ 2022-03-25 19:37 UTC (permalink / raw) To: Kees Cook, shuah Cc: Andy Lutomirski, Will Drewry, linux-kselftest, Willem de Bruijn, Jakub Kicinski, linux-kernel, linux-hardening, Shuah Khan On 3/24/22 5:19 PM, Kees Cook wrote: > The kselftest test harness has traditionally not run the registered > TEARDOWN handler when a test encountered an ASSERT. This creates > unexpected situations and tests need to be very careful about using > ASSERT, which seems a needless hurdle for test writers. > > Because of the harness's design for optional failure handlers, the > original implementation of ASSERT used an abort() to immediately > stop execution, but that meant the context for running teardown was > lost. Instead, use setjmp/longjmp so that teardown can be done. > Thanks for the patch. The change look good to me. > Failed SETUP routines continue to not be followed by TEARDOWN, though. Does this mean failed setup() routines have to handle TEARDOWN? What are guidelines to follow for setup() failures? Can you add a bit more detail on what you meant by " Failed SETUP routines continue to not be followed by TEARDOWN, though". With that: Reviewed-by: Shuah Khan <skhan@linuxfoundation.org> thanks, -- Shuah ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] selftests/harness: Run TEARDOWN for ASSERT failures 2022-03-25 19:37 ` Shuah Khan @ 2022-03-25 22:36 ` Kees Cook 0 siblings, 0 replies; 6+ messages in thread From: Kees Cook @ 2022-03-25 22:36 UTC (permalink / raw) To: Shuah Khan Cc: shuah, Andy Lutomirski, Will Drewry, linux-kselftest, Willem de Bruijn, Jakub Kicinski, linux-kernel, linux-hardening On Fri, Mar 25, 2022 at 01:37:20PM -0600, Shuah Khan wrote: > On 3/24/22 5:19 PM, Kees Cook wrote: > > The kselftest test harness has traditionally not run the registered > > TEARDOWN handler when a test encountered an ASSERT. This creates > > unexpected situations and tests need to be very careful about using > > ASSERT, which seems a needless hurdle for test writers. > > > > Because of the harness's design for optional failure handlers, the > > original implementation of ASSERT used an abort() to immediately > > stop execution, but that meant the context for running teardown was > > lost. Instead, use setjmp/longjmp so that teardown can be done. > > > > Thanks for the patch. The change look good to me. > > > Failed SETUP routines continue to not be followed by TEARDOWN, though. > > Does this mean failed setup() routines have to handle TEARDOWN? What > are guidelines to follow for setup() failures? > > Can you add a bit more detail on what you meant by " Failed SETUP > routines continue to not be followed by TEARDOWN, though". Sure! It means that any failures in a SETUP need to be cleaned up by the SETUP, as TEARDOWN won't be run. (As in, this is unchanged from how things behaved prior to this patch.) > > With that: > > Reviewed-by: Shuah Khan <skhan@linuxfoundation.org> Thanks! -- Kees Cook ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] selftests/harness: Pass variant to teardown 2022-03-24 23:19 [PATCH 0/2] selftests/harness: Run TEARDOWN for ASSERT failures Kees Cook 2022-03-24 23:19 ` [PATCH 1/2] " Kees Cook @ 2022-03-24 23:19 ` Kees Cook 2022-03-25 19:38 ` Shuah Khan 1 sibling, 1 reply; 6+ messages in thread From: Kees Cook @ 2022-03-24 23:19 UTC (permalink / raw) To: shuah Cc: Kees Cook, Willem de Bruijn, Jakub Kicinski, Andy Lutomirski, Will Drewry, linux-kernel, linux-kselftest, linux-hardening From: Willem de Bruijn <willemb@google.com> FIXTURE_VARIANT data is passed to FIXTURE_SETUP and TEST_F as "variant". In some cases, the variant will change the setup, such that expectations also change on teardown. Also pass variant to FIXTURE_TEARDOWN. The new FIXTURE_TEARDOWN logic is identical to that in FIXTURE_SETUP, right above. Signed-off-by: Willem de Bruijn <willemb@google.com> Reviewed-by: Jakub Kicinski <kuba@kernel.org> Acked-by: Kees Cook <keescook@chromium.org> Signed-off-by: Kees Cook <keescook@chromium.org> Link: https://lore.kernel.org/r/20201210231010.420298-1-willemdebruijn.kernel@gmail.com --- tools/testing/selftests/kselftest_harness.h | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index bef08f824eb5..f5cb5fd1d974 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -291,7 +291,9 @@ #define FIXTURE_TEARDOWN(fixture_name) \ void fixture_name##_teardown( \ struct __test_metadata __attribute__((unused)) *_metadata, \ - FIXTURE_DATA(fixture_name) __attribute__((unused)) *self) + FIXTURE_DATA(fixture_name) __attribute__((unused)) *self, \ + const FIXTURE_VARIANT(fixture_name) \ + __attribute__((unused)) *variant) /** * FIXTURE_VARIANT() - Optionally called once per fixture @@ -306,9 +308,9 @@ * ... * }; * - * Defines type of constant parameters provided to FIXTURE_SETUP() and TEST_F() - * as *variant*. Variants allow the same tests to be run with different - * arguments. + * Defines type of constant parameters provided to FIXTURE_SETUP(), TEST_F() and + * FIXTURE_TEARDOWN as *variant*. Variants allow the same tests to be run with + * different arguments. */ #define FIXTURE_VARIANT(fixture_name) struct _fixture_variant_##fixture_name @@ -391,7 +393,7 @@ fixture_name##_##test_name(_metadata, &self, variant->data); \ } \ if (_metadata->setup_completed) \ - fixture_name##_teardown(_metadata, &self); \ + fixture_name##_teardown(_metadata, &self, variant->data); \ __test_check_assert(_metadata); \ } \ static struct __test_metadata \ -- 2.32.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] selftests/harness: Pass variant to teardown 2022-03-24 23:19 ` [PATCH 2/2] selftests/harness: Pass variant to teardown Kees Cook @ 2022-03-25 19:38 ` Shuah Khan 0 siblings, 0 replies; 6+ messages in thread From: Shuah Khan @ 2022-03-25 19:38 UTC (permalink / raw) To: Kees Cook, shuah Cc: Willem de Bruijn, Jakub Kicinski, Andy Lutomirski, Will Drewry, linux-kernel, linux-kselftest, linux-hardening, Shuah Khan On 3/24/22 5:19 PM, Kees Cook wrote: > From: Willem de Bruijn <willemb@google.com> > > FIXTURE_VARIANT data is passed to FIXTURE_SETUP and TEST_F as "variant". > > In some cases, the variant will change the setup, such that expectations > also change on teardown. Also pass variant to FIXTURE_TEARDOWN. > > The new FIXTURE_TEARDOWN logic is identical to that in FIXTURE_SETUP, > right above. > > Signed-off-by: Willem de Bruijn <willemb@google.com> > Reviewed-by: Jakub Kicinski <kuba@kernel.org> > Acked-by: Kees Cook <keescook@chromium.org> > Signed-off-by: Kees Cook <keescook@chromium.org> > Link: https://lore.kernel.org/r/20201210231010.420298-1-willemdebruijn.kernel@gmail.com > --- > tools/testing/selftests/kselftest_harness.h | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h > index bef08f824eb5..f5cb5fd1d974 100644 > --- a/tools/testing/selftests/kselftest_harness.h > +++ b/tools/testing/selftests/kselftest_harness.h > @@ -291,7 +291,9 @@ > #define FIXTURE_TEARDOWN(fixture_name) \ > void fixture_name##_teardown( \ > struct __test_metadata __attribute__((unused)) *_metadata, \ > - FIXTURE_DATA(fixture_name) __attribute__((unused)) *self) > + FIXTURE_DATA(fixture_name) __attribute__((unused)) *self, \ > + const FIXTURE_VARIANT(fixture_name) \ > + __attribute__((unused)) *variant) > > /** > * FIXTURE_VARIANT() - Optionally called once per fixture > @@ -306,9 +308,9 @@ > * ... > * }; > * > - * Defines type of constant parameters provided to FIXTURE_SETUP() and TEST_F() > - * as *variant*. Variants allow the same tests to be run with different > - * arguments. > + * Defines type of constant parameters provided to FIXTURE_SETUP(), TEST_F() and > + * FIXTURE_TEARDOWN as *variant*. Variants allow the same tests to be run with > + * different arguments. > */ > #define FIXTURE_VARIANT(fixture_name) struct _fixture_variant_##fixture_name > > @@ -391,7 +393,7 @@ > fixture_name##_##test_name(_metadata, &self, variant->data); \ > } \ > if (_metadata->setup_completed) \ > - fixture_name##_teardown(_metadata, &self); \ > + fixture_name##_teardown(_metadata, &self, variant->data); \ > __test_check_assert(_metadata); \ > } \ > static struct __test_metadata \ > Looks good to me. Reviewed-by: Shuah Khan <skhan@linuxfoundation.org> thanks, -- Shuah ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-25 22:36 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-24 23:19 [PATCH 0/2] selftests/harness: Run TEARDOWN for ASSERT failures Kees Cook 2022-03-24 23:19 ` [PATCH 1/2] " Kees Cook 2022-03-25 19:37 ` Shuah Khan 2022-03-25 22:36 ` Kees Cook 2022-03-24 23:19 ` [PATCH 2/2] selftests/harness: Pass variant to teardown Kees Cook 2022-03-25 19:38 ` Shuah Khan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.