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