diff mbox series

[v3,7/7] kunit: Add tests for fault

Message ID 20240319104857.70783-8-mic@digikod.net
State Not Applicable
Headers show
Series Handle faults in KUnit tests | expand

Commit Message

Mickaël Salaün March 19, 2024, 10:48 a.m. UTC 
Add a test case to check NULL pointer dereference and make sure it would
result as a failed test.

The full kunit_fault test suite is marked as skipped when run on UML
because it would result to a kernel panic.

Tested with:
./tools/testing/kunit/kunit.py run --arch x86_64 kunit_fault
./tools/testing/kunit/kunit.py run --arch arm64 \
  --cross_compile=aarch64-linux-gnu- kunit_fault

Cc: Brendan Higgins <brendanhiggins@google.com>
Cc: Rae Moar <rmoar@google.com>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Reviewed-by: David Gow <davidgow@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20240319104857.70783-8-mic@digikod.net
---

Changes since v2:
* Add David's Reviewed-by.

Changes since v1:
* Remove the rodata and const test cases for now.
* Replace CONFIG_X86 check with !CONFIG_UML, and remove the "_x86"
  references.
---
 lib/kunit/kunit-test.c | 45 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

Comments

Guenter Roeck April 19, 2024, 10:33 p.m. UTC | #1 
Hi,

On Tue, Mar 19, 2024 at 11:48:57AM +0100, Mickaël Salaün wrote:
> Add a test case to check NULL pointer dereference and make sure it would
> result as a failed test.
> 
> The full kunit_fault test suite is marked as skipped when run on UML
> because it would result to a kernel panic.
> 
> Tested with:
> ./tools/testing/kunit/kunit.py run --arch x86_64 kunit_fault
> ./tools/testing/kunit/kunit.py run --arch arm64 \
>   --cross_compile=aarch64-linux-gnu- kunit_fault
> 

What is the rationale for adding those tests unconditionally whenever
CONFIG_KUNIT_TEST is enabled ? This completely messes up my test system
because it concludes that it is pointless to continue testing
after the "Unable to handle kernel NULL pointer dereference" backtrace.
At the same time, it is all or nothing, meaning I can not disable
it but still run other kunit tests.

Guenter
Guenter Roeck April 19, 2024, 11:38 p.m. UTC | #2 
On Fri, Apr 19, 2024 at 03:33:49PM -0700, Guenter Roeck wrote:
> Hi,
> 
> On Tue, Mar 19, 2024 at 11:48:57AM +0100, Mickaël Salaün wrote:
> > Add a test case to check NULL pointer dereference and make sure it would
> > result as a failed test.
> > 
> > The full kunit_fault test suite is marked as skipped when run on UML
> > because it would result to a kernel panic.
> > 
> > Tested with:
> > ./tools/testing/kunit/kunit.py run --arch x86_64 kunit_fault
> > ./tools/testing/kunit/kunit.py run --arch arm64 \
> >   --cross_compile=aarch64-linux-gnu- kunit_fault
> > 
> 
> What is the rationale for adding those tests unconditionally whenever
> CONFIG_KUNIT_TEST is enabled ? This completely messes up my test system
> because it concludes that it is pointless to continue testing
> after the "Unable to handle kernel NULL pointer dereference" backtrace.
> At the same time, it is all or nothing, meaning I can not disable
> it but still run other kunit tests.
> 

Oh, never mind. I just disabled CONFIG_KUNIT_TEST in my test bed
to "solve" the problem. I'll take that as one of those "unintended
consequences" items: Instead of more tests, there are fewer.

Guenter
Mickaël Salaün April 22, 2024, 1:08 p.m. UTC | #3 
On Fri, Apr 19, 2024 at 04:38:01PM -0700, Guenter Roeck wrote:
> On Fri, Apr 19, 2024 at 03:33:49PM -0700, Guenter Roeck wrote:
> > Hi,
> > 
> > On Tue, Mar 19, 2024 at 11:48:57AM +0100, Mickaël Salaün wrote:
> > > Add a test case to check NULL pointer dereference and make sure it would
> > > result as a failed test.
> > > 
> > > The full kunit_fault test suite is marked as skipped when run on UML
> > > because it would result to a kernel panic.
> > > 
> > > Tested with:
> > > ./tools/testing/kunit/kunit.py run --arch x86_64 kunit_fault
> > > ./tools/testing/kunit/kunit.py run --arch arm64 \
> > >   --cross_compile=aarch64-linux-gnu- kunit_fault
> > > 
> > 
> > What is the rationale for adding those tests unconditionally whenever
> > CONFIG_KUNIT_TEST is enabled ? This completely messes up my test system
> > because it concludes that it is pointless to continue testing
> > after the "Unable to handle kernel NULL pointer dereference" backtrace.
> > At the same time, it is all or nothing, meaning I can not disable
> > it but still run other kunit tests.
> > 

CONFIG_KUNIT_TEST is to test KUnit itself.  Why does this messes up your
test system, and what is your test system?  Is it related to the kernel
warning and then the message you previously sent?
https://lore.kernel.org/r/fd604ae0-5630-4745-acf2-1e51c69cf0c0@roeck-us.net
It seems David has a solution to suppress such warning.

> 
> Oh, never mind. I just disabled CONFIG_KUNIT_TEST in my test bed
> to "solve" the problem. I'll take that as one of those "unintended
> consequences" items: Instead of more tests, there are fewer.
> 
> Guenter
>
Guenter Roeck April 22, 2024, 1:35 p.m. UTC | #4 
On 4/22/24 06:08, Mickaël Salaün wrote:
> On Fri, Apr 19, 2024 at 04:38:01PM -0700, Guenter Roeck wrote:
>> On Fri, Apr 19, 2024 at 03:33:49PM -0700, Guenter Roeck wrote:
>>> Hi,
>>>
>>> On Tue, Mar 19, 2024 at 11:48:57AM +0100, Mickaël Salaün wrote:
>>>> Add a test case to check NULL pointer dereference and make sure it would
>>>> result as a failed test.
>>>>
>>>> The full kunit_fault test suite is marked as skipped when run on UML
>>>> because it would result to a kernel panic.
>>>>
>>>> Tested with:
>>>> ./tools/testing/kunit/kunit.py run --arch x86_64 kunit_fault
>>>> ./tools/testing/kunit/kunit.py run --arch arm64 \
>>>>    --cross_compile=aarch64-linux-gnu- kunit_fault
>>>>
>>>
>>> What is the rationale for adding those tests unconditionally whenever
>>> CONFIG_KUNIT_TEST is enabled ? This completely messes up my test system
>>> because it concludes that it is pointless to continue testing
>>> after the "Unable to handle kernel NULL pointer dereference" backtrace.
>>> At the same time, it is all or nothing, meaning I can not disable
>>> it but still run other kunit tests.
>>>
> 
> CONFIG_KUNIT_TEST is to test KUnit itself.  Why does this messes up your
> test system, and what is your test system?  Is it related to the kernel
> warning and then the message you previously sent?

It is not a warning, it is a BUG which terminates the affected kernel thread.
NULL pointer dereferences are normally fatal, which is why I abort tests
if one is encountered. I am not going to start introducing code into my
scripts to ignore such warnings (or BUG messages) on a case by case basis;
this would be unmaintainable.

> https://lore.kernel.org/r/fd604ae0-5630-4745-acf2-1e51c69cf0c0@roeck-us.net
> It seems David has a solution to suppress such warning.
> 

I don't think so. My series tried to suppress warning backtraces, not BUG
messages. BUG messages can not easily be suppressed since the reaction is
architecture specific and typically fatal.

As I said below, never mind, I just disabled CONFIG_KUNIT_TEST in my testing.

Guenter

>>
>> Oh, never mind. I just disabled CONFIG_KUNIT_TEST in my test bed
>> to "solve" the problem. I'll take that as one of those "unintended
>> consequences" items: Instead of more tests, there are fewer.
>>
>> Guenter
>>
David Gow April 23, 2024, 9:22 a.m. UTC | #5 
On Mon, 22 Apr 2024 at 21:36, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 4/22/24 06:08, Mickaël Salaün wrote:
> > On Fri, Apr 19, 2024 at 04:38:01PM -0700, Guenter Roeck wrote:
> >> On Fri, Apr 19, 2024 at 03:33:49PM -0700, Guenter Roeck wrote:
> >>> Hi,
> >>>
> >>> On Tue, Mar 19, 2024 at 11:48:57AM +0100, Mickaël Salaün wrote:
> >>>> Add a test case to check NULL pointer dereference and make sure it would
> >>>> result as a failed test.
> >>>>
> >>>> The full kunit_fault test suite is marked as skipped when run on UML
> >>>> because it would result to a kernel panic.
> >>>>
> >>>> Tested with:
> >>>> ./tools/testing/kunit/kunit.py run --arch x86_64 kunit_fault
> >>>> ./tools/testing/kunit/kunit.py run --arch arm64 \
> >>>>    --cross_compile=aarch64-linux-gnu- kunit_fault
> >>>>
> >>>
> >>> What is the rationale for adding those tests unconditionally whenever
> >>> CONFIG_KUNIT_TEST is enabled ? This completely messes up my test system
> >>> because it concludes that it is pointless to continue testing
> >>> after the "Unable to handle kernel NULL pointer dereference" backtrace.
> >>> At the same time, it is all or nothing, meaning I can not disable
> >>> it but still run other kunit tests.
> >>>
> >
> > CONFIG_KUNIT_TEST is to test KUnit itself.  Why does this messes up your
> > test system, and what is your test system?  Is it related to the kernel
> > warning and then the message you previously sent?
>
> It is not a warning, it is a BUG which terminates the affected kernel thread.
> NULL pointer dereferences are normally fatal, which is why I abort tests
> if one is encountered. I am not going to start introducing code into my
> scripts to ignore such warnings (or BUG messages) on a case by case basis;
> this would be unmaintainable.
>
> > https://lore.kernel.org/r/fd604ae0-5630-4745-acf2-1e51c69cf0c0@roeck-us.net
> > It seems David has a solution to suppress such warning.
> >
>
> I don't think so. My series tried to suppress warning backtraces, not BUG
> messages. BUG messages can not easily be suppressed since the reaction is
> architecture specific and typically fatal.
>
> As I said below, never mind, I just disabled CONFIG_KUNIT_TEST in my testing.
>
> Guenter
>

I think it probably makes sense to permit disabling the fault tests
independently, at least until we have a way of suppressing the
warnings.

I've sent out a patch to add a CONFIG_KUNIT_FAULT_TEST option to
disable these tests. Would that help?
https://lore.kernel.org/linux-kselftest/20240423090808.242389-1-davidgow@google.com/

(The other option is to split the tests out into a totally separate
file / module. I think that's an option (and would make the config
option more consistent with other test options) but since they're
otherwise part of the KUnit tests, I think I prefer to keep them
together.)

Cheers,
-- David
diff mbox series

Patch

diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
index f7980ef236a3..0fdca5fffaec 100644
--- a/lib/kunit/kunit-test.c
+++ b/lib/kunit/kunit-test.c
@@ -109,6 +109,48 @@  static struct kunit_suite kunit_try_catch_test_suite = {
 	.test_cases = kunit_try_catch_test_cases,
 };
 
+#ifndef CONFIG_UML
+
+static void kunit_test_null_dereference(void *data)
+{
+	struct kunit *test = data;
+	int *null = NULL;
+
+	*null = 0;
+
+	KUNIT_FAIL(test, "This line should never be reached\n");
+}
+
+static void kunit_test_fault_null_dereference(struct kunit *test)
+{
+	struct kunit_try_catch_test_context *ctx = test->priv;
+	struct kunit_try_catch *try_catch = ctx->try_catch;
+
+	kunit_try_catch_init(try_catch,
+			     test,
+			     kunit_test_null_dereference,
+			     kunit_test_catch);
+	kunit_try_catch_run(try_catch, test);
+
+	KUNIT_EXPECT_EQ(test, try_catch->try_result, -EINTR);
+	KUNIT_EXPECT_TRUE(test, ctx->function_called);
+}
+
+#endif /* !CONFIG_UML */
+
+static struct kunit_case kunit_fault_test_cases[] = {
+#ifndef CONFIG_UML
+	KUNIT_CASE(kunit_test_fault_null_dereference),
+#endif /* !CONFIG_UML */
+	{}
+};
+
+static struct kunit_suite kunit_fault_test_suite = {
+	.name = "kunit_fault",
+	.init = kunit_try_catch_test_init,
+	.test_cases = kunit_fault_test_cases,
+};
+
 /*
  * Context for testing test managed resources
  * is_resource_initialized is used to test arbitrary resources
@@ -826,6 +868,7 @@  static struct kunit_suite kunit_current_test_suite = {
 
 kunit_test_suites(&kunit_try_catch_test_suite, &kunit_resource_test_suite,
 		  &kunit_log_test_suite, &kunit_status_test_suite,
-		  &kunit_current_test_suite, &kunit_device_test_suite);
+		  &kunit_current_test_suite, &kunit_device_test_suite,
+		  &kunit_fault_test_suite);
 
 MODULE_LICENSE("GPL v2");