Re: [PATCH] kunit: Set the current KUnit context when cleaning up

[David Gow]

On Mon, 17 Apr 2023 at 18:43, Benjamin Berg <benjamin@xxxxxxxxxxxxxxxx> 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

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature