On Fri, May 13, 2022 at 8:04 PM David Gow davidgow@google.com wrote:
Hmm... thinking about it, I think it might be worth not tainting if 0 suites run, but tainting if 0 tests run.
If we taint even if there are no suites present, that'll make things awkward for the "build KUnit in, but not any tests" case: the kernel would be tainted regardless. Given Android might be having the KUnit
Actually, this is what the code does right now. I was wrong. If there are 0 suites => not tainted. If there are 0 tests in the suites => tainted.
For kunit being built in, it first goes through this func 206 static void kunit_exec_run_tests(struct suite_set *suite_set) 207 { 208 struct kunit_suite * const * const *suites; 209 210 kunit_print_tap_header(suite_set); 211 212 for (suites = suite_set->start; suites < suite_set->end; suites++) 213 __kunit_test_suites_init(*suites); 214 }
So for the "build KUnit in, but not any tests" case, you'll never enter that for-loop. Thus you'll never call __kunit_test_suites_init() => kunit_run_tests().
For module-based tests, we have the same behavior. If there's 0 test suites, we never enter the second loop, so we never taint. But if there's >0 suites, then we will, regardless of the # of test cases.
570 int __kunit_test_suites_init(struct kunit_suite * const * const suites) 571 { 572 unsigned int i; 573 574 for (i = 0; suites[i] != NULL; i++) { 575 kunit_init_suite(suites[i]); 576 kunit_run_tests(suites[i]); 577 } 578 return 0; 579 }
So this change should already work as intended.
execution stuff built-in (but using modules for tests), it's probably worth not tainting there. (Though I think they have a separate way of disabling KUnit as well, so it's probably not a complete deal-breaker).
The case of having suites but no tests should still taint the kernel, as suite_init functions could still run.
Yes, suite_init functions are the concern. I agree we should taint if there are >0 suites but 0 test cases.
I don't think it's worth trying to be fancy and tainting iff there >0 test cases or a suite_init/exit function ran.
Assuming that seems sensible, I'll send out a v4 with that changed.
Just to be clear: that shouldn't be necessary.
I wasn't quite sure where this applied, but I manually applied the changes here. Without this patch, this command exits fine: $ ./tools/testing/kunit/kunit.py run --kernel_args=panic_on_taint=0x40000
With it, I get [12:03:31] Kernel panic - not syncing: panic_on_taint set ... [12:03:31] CPU: 0 PID: 1 Comm: swapper Tainted: G N
This is showing both 'G' and 'N' ('G' being the character for GPL --
I just somehow missed the fact there was an 'N' at the end there. Thanks, I thought I was going crazy. I guess I was just going blind.
Daniel