Message-ID: <a39af0400abedb2e9b31d84c37551cecc3eed0e1.camel@sipsolutions.net>
Date:   Tue, 18 Apr 2023 11:24:18 +0200
From:   Benjamin Berg <benjamin@...solutions.net>
To:     David Gow <davidgow@...gle.com>
Cc:     Brendan Higgins <brendan.higgins@...ux.dev>,
        Rae Moar <rmoar@...gle.com>,
        Daniel Latypov <dlatypov@...gle.com>, maxime@...no.tech,
        Stephen Boyd <sboyd@...nel.org>,
        Shuah Khan <skhan@...uxfoundation.org>,
        kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kunit: Set the current KUnit context when cleaning up

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@...solutions.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

Powered by blists - more mailing lists