| Message ID | 20230415091401.681395-1-davidgow@google.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: Set the current KUnit context when cleaning up | expand |
Hi, On Sat, 2023-04-15 at 17:14 +0800, David Gow wrote: > KUnit tests run in a kthread, with the current->kunit_test pointer set > to the test's context. This allows the kunit_get_current_test() and > kunit_fail_current_test() macros to work. Normally, this pointer is > still valid during test shutdown (i.e., the suite->exit function, and > any resource cleanup). However, if the test has exited early (e.g., due > to a failed assertion), the cleanup is done in the parent KUnit thread, > which does not have an active context. My only question here is whether assertions (not expectations) are OK within the exit/cleanup handler. That said, it wouldn't be clear whether we should try to continue cleaning up after every failure, so probably it is reasonable to not do that. But, I did see that at least the clock tests currently use assertions inside the init function. And, in those tests, if the context allocation fails the exit handler will dereference the NULL pointer. So, nothing for this patch, but maybe it would make sense to mark tests as failed (or print a warning) if they use assertions inside init, exit or cleanup handlers? Benjamin > Fix this by setting the active KUnit context for the duration of the > test shutdown procedure. When the test exits normally, this does > nothing. When run from the KUits previous value (probably NULL) > afterwards. > > This should make it easier to get access to the KUnit context, > particularly from within resource cleanup functions, which may, for > example, need access to data in test->priv. > > Signed-off-by: David Gow <davidgow@google.com> > --- > > This becomes useful with the current kunit_add_action() implementation, > as actions do not get the KUnit context passed in by default: > https://lore.kernel.org/linux-kselftest/CABVgOSmjs0wLUa4=ErkB9tH8p6A1P6N33befv63whF+hECRExQ@mail.gmail.com/ > > I think it's probably correct anyway, though, so we should either do > this, or totally rule out using kunit_get_current_test() here at all, by > resetting current->kunit_test to NULL before running cleanup even in > the normal case. > > I've only given this the most cursory testing so far (I'm not sure how > much of the executor innards I want to expose to be able to actually > write a proper test for it), so more eyes and/or suggestions are > welcome. > > Cheers, > -- David > > --- > lib/kunit/test.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index e2910b261112..2d7cad249863 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -392,10 +392,21 @@ static void kunit_case_internal_cleanup(struct kunit *test) > static void kunit_run_case_cleanup(struct kunit *test, > struct kunit_suite *suite) > { > + /* > + * If we're no-longer running from within the test kthread() because it failed > + * or timed out, we still need the context to be okay when running exit and > + * cleanup functions. > + */ > + struct kunit *old_current = current->kunit_test; > + > + current->kunit_test = test; > if (suite->exit) > suite->exit(test); > > kunit_case_internal_cleanup(test); > + > + /* Restore the thread's previous test context (probably NULL or test). */ > + current->kunit_test = old_current; > } > > struct kunit_try_catch_context {
On Mon, 17 Apr 2023 at 18:43, Benjamin Berg <benjamin@sipsolutions.net> wrote: > > Hi, > > On Sat, 2023-04-15 at 17:14 +0800, David Gow wrote: > > KUnit tests run in a kthread, with the current->kunit_test pointer set > > to the test's context. This allows the kunit_get_current_test() and > > kunit_fail_current_test() macros to work. Normally, this pointer is > > still valid during test shutdown (i.e., the suite->exit function, and > > any resource cleanup). However, if the test has exited early (e.g., due > > to a failed assertion), the cleanup is done in the parent KUnit thread, > > which does not have an active context. > > My only question here is whether assertions (not expectations) are OK > within the exit/cleanup handler. That said, it wouldn't be clear > whether we should try to continue cleaning up after every failure, so > probably it is reasonable to not do that. Excellent point. In general: - It's okay to use expectations within exit and cleanup functions. - It's not okay for assertions to fail within an exit / cleanup handler. - It's not okay to access anything which was allocated on the stack from within a test in exit / cleanup handlers. - It's okay to access and free resources from within cleanup / exit handlers, though it's not permitted to create new resources from cleanup handlers (exit is okay). I do think we need to document this better, at the very least. What I think we'll end up doing is implementing a different system: - The test (and, if successful, cleanup) runs in a kthread. - If it aborts (e.g., due to an assertion), cleanup runs in another kthread. - If this second kthread aborts early, no further cleanup is run. This would protect the KUnit executor thread from misbehaving cleanup functions, and if an assertion happens in a cleanup function, we'll leak things (which is bad), but not dereference a bunch of invalid pointers (which is worse). I've got this mostly working here, and will send it out as a replacement to this patch (that second kthread will have current->kunit_test set, rendering this change redundant). > > But, I did see that at least the clock tests currently use assertions > inside the init function. And, in those tests, if the context > allocation fails the exit handler will dereference the NULL pointer. Hmm... which clock test is that? Regardless, it sounds like a bug in the test. I think that ultimately, the right solution for dealing with the context pointer issue is to use resources (or, when available, kunit_add_action()), which nicely enforces the ordering as well. In the meantime, though, I guess it just needs wrapping in a lot of "if (ctx)" or similar... > So, nothing for this patch, but maybe it would make sense to mark tests > as failed (or print a warning) if they use assertions inside init, exit > or cleanup handlers? > I think we'll still want to support assertions in init functions (albeit possibly with some documentation pointing out any pitfalls). If actions or resources are used, it's not a problem. Also, a lot of these issues also apply to kunit_skip(), which is used _heavily_ in init functions. Warnings for assertions in exit or cleanup make sense. I _could_ see a reason to allow assertions if we wanted to have, e.g., helpers which asserted that their inputs were valid -- it'd be a bit rough to deny their use if the inputs are known to be okay -- but I'm not aware of any such case in practice yet, so I suspect it's still worth failing tests (and seeing if anyone complains). Cheers, -- David
Hi David, On Tue, 2023-04-18 at 14:53 +0800, David Gow wrote: > On Mon, 17 Apr 2023 at 18:43, Benjamin Berg <benjamin@sipsolutions.net> wrote: > > > > Hi, > > > > On Sat, 2023-04-15 at 17:14 +0800, David Gow wrote: > > > KUnit tests run in a kthread, with the current->kunit_test pointer set > > > to the test's context. This allows the kunit_get_current_test() and > > > kunit_fail_current_test() macros to work. Normally, this pointer is > > > still valid during test shutdown (i.e., the suite->exit function, and > > > any resource cleanup). However, if the test has exited early (e.g., due > > > to a failed assertion), the cleanup is done in the parent KUnit thread, > > > which does not have an active context. > > > > My only question here is whether assertions (not expectations) are OK > > within the exit/cleanup handler. That said, it wouldn't be clear > > whether we should try to continue cleaning up after every failure, so > > probably it is reasonable to not do that. > > Excellent point. > In general: > - It's okay to use expectations within exit and cleanup functions. > - It's not okay for assertions to fail within an exit / cleanup handler. > - It's not okay to access anything which was allocated on the stack > from within a test in exit / cleanup handlers. > - It's okay to access and free resources from within cleanup / exit > handlers, though it's not permitted to create new resources from > cleanup handlers (exit is okay). The list makes sense to me. > I do think we need to document this better, at the very least. > > What I think we'll end up doing is implementing a different system: > - The test (and, if successful, cleanup) runs in a kthread. > - If it aborts (e.g., due to an assertion), cleanup runs in another kthread. > - If this second kthread aborts early, no further cleanup is run. > > This would protect the KUnit executor thread from misbehaving cleanup > functions, and if an assertion happens in a cleanup function, we'll > leak things (which is bad), but not dereference a bunch of invalid > pointers (which is worse). Sounds good. > I've got this mostly working here, and will send it out as a > replacement to this patch (that second kthread will have > current->kunit_test set, rendering this change redundant). Cool! > > But, I did see that at least the clock tests currently use assertions > > inside the init function. And, in those tests, if the context > > allocation fails the exit handler will dereference the NULL pointer. > > Hmm... which clock test is that? Regardless, it sounds like a bug in the test. > > I think that ultimately, the right solution for dealing with the > context pointer issue is to use resources (or, when available, > kunit_add_action()), which nicely enforces the ordering as well. In > the meantime, though, I guess it just needs wrapping in a lot of "if > (ctx)" or similar... I am talking about drivers/clk/clk-gate_test.c. The _init functions (and clk_gate_test_alloc_ctx) use assertions heavily. The _exit handles does not deal with the ctx being NULL though. But, the fix is trivial though: diff --git a/drivers/clk/clk-gate_test.c b/drivers/clk/clk-gate_test.c index e136aaad48bf..f1adafe725e4 100644 --- a/drivers/clk/clk-gate_test.c +++ b/drivers/clk/clk-gate_test.c @@ -225,6 +225,9 @@ static void clk_gate_test_exit(struct kunit *test) { struct clk_gate_test_context *ctx = test->priv; + if (!ctx) + return; + clk_hw_unregister_gate(ctx->hw); clk_hw_unregister_fixed_rate(ctx->parent); } > > So, nothing for this patch, but maybe it would make sense to mark tests > > as failed (or print a warning) if they use assertions inside init, exit > > or cleanup handlers? > > > > I think we'll still want to support assertions in init functions > (albeit possibly with some documentation pointing out any pitfalls). > If actions or resources are used, it's not a problem. Yeah, a short sentence in the "struct kunit_suite" documentation should be enough. Something along the lines of: "This call to @exit will always happen, even if @init returned an error or aborted due to an assertion.". > Also, a lot of these issues also apply to kunit_skip(), which is used > _heavily_ in init functions. > > Warnings for assertions in exit or cleanup make sense. I _could_ see a > reason to allow assertions if we wanted to have, e.g., helpers which > asserted that their inputs were valid -- it'd be a bit rough to deny > their use if the inputs are known to be okay -- but I'm not aware of > any such case in practice yet, so I suspect it's still worth failing > tests (and seeing if anyone complains). I am not going to insist on disallowing or warning about assertions. It is OK from my point of view, as long as the damage is contained to a kunit kthread and "only" affects cleanup. Benjamin
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index e2910b261112..2d7cad249863 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -392,10 +392,21 @@ static void kunit_case_internal_cleanup(struct kunit *test) static void kunit_run_case_cleanup(struct kunit *test, struct kunit_suite *suite) { + /* + * If we're no-longer running from within the test kthread() because it failed + * or timed out, we still need the context to be okay when running exit and + * cleanup functions. + */ + struct kunit *old_current = current->kunit_test; + + current->kunit_test = test; if (suite->exit) suite->exit(test); kunit_case_internal_cleanup(test); + + /* Restore the thread's previous test context (probably NULL or test). */ + current->kunit_test = old_current; } struct kunit_try_catch_context {
KUnit tests run in a kthread, with the current->kunit_test pointer set to the test's context. This allows the kunit_get_current_test() and kunit_fail_current_test() macros to work. Normally, this pointer is still valid during test shutdown (i.e., the suite->exit function, and any resource cleanup). However, if the test has exited early (e.g., due to a failed assertion), the cleanup is done in the parent KUnit thread, which does not have an active context. Fix this by setting the active KUnit context for the duration of the test shutdown procedure. When the test exits normally, this does nothing. When run from the KUits previous value (probably NULL) afterwards. This should make it easier to get access to the KUnit context, particularly from within resource cleanup functions, which may, for example, need access to data in test->priv. Signed-off-by: David Gow <davidgow@google.com> --- This becomes useful with the current kunit_add_action() implementation, as actions do not get the KUnit context passed in by default: https://lore.kernel.org/linux-kselftest/CABVgOSmjs0wLUa4=ErkB9tH8p6A1P6N33befv63whF+hECRExQ@mail.gmail.com/ I think it's probably correct anyway, though, so we should either do this, or totally rule out using kunit_get_current_test() here at all, by resetting current->kunit_test to NULL before running cleanup even in the normal case. I've only given this the most cursory testing so far (I'm not sure how much of the executor innards I want to expose to be able to actually write a proper test for it), so more eyes and/or suggestions are welcome. Cheers, -- David --- lib/kunit/test.c | 11 +++++++++++ 1 file changed, 11 insertions(+)