On Fri, May 13, 2022 at 8:04 PM David Gow <davidgow@xxxxxxxxxx> 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
