| Message ID | 20220430030019.803481-1-davidgow@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] kunit: Taint kernel if any tests run | expand |
On Sat, Apr 30, 2022 at 11:00:19AM +0800, David Gow wrote: > KUnit tests are not supposed to run on production systems: they may do > deliberately illegal things to trigger errors, and have security > implications (assertions will often deliberately leak kernel addresses). > > Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been > run. This will be printed as 'N' (for kuNit, as K, U and T were already > taken). > > This should discourage people from running KUnit tests on production > systems, and to make it easier to tell if tests have been run > accidentally (by loading the wrong configuration, etc.) > > Signed-off-by: David Gow <davidgow@google.com> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Sat, Apr 30, 2022 at 11:00:19AM +0800, David Gow wrote: > KUnit tests are not supposed to run on production systems: they may do > deliberately illegal things to trigger errors, and have security > implications (assertions will often deliberately leak kernel addresses). > > Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been > run. This will be printed as 'N' (for kuNit, as K, U and T were already > taken). > > This should discourage people from running KUnit tests on production > systems, and to make it easier to tell if tests have been run > accidentally (by loading the wrong configuration, etc.) > > Signed-off-by: David Gow <davidgow@google.com> There is no reason to distinguish kunit from selftests if the result is the same: really make the kernel try really insane stupid things which may crash it or put it into a bad state. So no, this should be renamed to "TEST_BREAK" as I think outside of selftest and kunit we may grow the kernel to do stupid things outside of that domain and this gives us the flexilibilty to use that in other places as well. It begs the question if we *should* allow userspace to volunterally say "hey, we are doing really insane things, brace yourself." Why ? Well because selftest has tons of modules. We either then define a macro that adds the taint for them and wrap the module declaration for it, or we expose a syctl to let userspace volunteer to opt-in to seggest we are about to try something stupid with the kernel including loading some dangeerous modules which may not have macros which taint the kernel. That would let selftest taint on *any* selftest. Because we can run all selftests or run one selftest. Then, if such sysctl is exposed, maybe we should then also use this for example for blktests, fstests, fio tests, etc. Luis
On Sun, May 01, 2022 at 11:22:38AM -0700, Luis Chamberlain wrote: > On Sat, Apr 30, 2022 at 11:00:19AM +0800, David Gow wrote: > > KUnit tests are not supposed to run on production systems: they may do > > deliberately illegal things to trigger errors, and have security > > implications (assertions will often deliberately leak kernel addresses). > > > > Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been > > run. This will be printed as 'N' (for kuNit, as K, U and T were already > > taken). > > > > This should discourage people from running KUnit tests on production > > systems, and to make it easier to tell if tests have been run > > accidentally (by loading the wrong configuration, etc.) > > > > Signed-off-by: David Gow <davidgow@google.com> > > There is no reason to distinguish kunit from selftests if the result is > the same: really make the kernel try really insane stupid things which > may crash it or put it into a bad state. > > So no, this should be renamed to "TEST_BREAK" as I think outside of > selftest and kunit we may grow the kernel to do stupid things outside > of that domain and this gives us the flexilibilty to use that in other > places as well. > > It begs the question if we *should* allow userspace to volunterally say > "hey, we are doing really insane things, brace yourself." Why ? Well > because selftest has tons of modules. We either then define a macro > that adds the taint for them and wrap the module declaration for it, > or we expose a syctl to let userspace volunteer to opt-in to seggest > we are about to try something stupid with the kernel including loading > some dangeerous modules which may not have macros which taint the kernel. > That would let selftest taint on *any* selftest. Because we can run all > selftests or run one selftest. > > Then, if such sysctl is exposed, maybe we should then also use this for > example for blktests, fstests, fio tests, etc. For got to expand to fsdevel and linux-block. Luis
On Mon, May 2, 2022 at 2:24 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Sun, May 01, 2022 at 11:22:38AM -0700, Luis Chamberlain wrote: > > On Sat, Apr 30, 2022 at 11:00:19AM +0800, David Gow wrote: > > > KUnit tests are not supposed to run on production systems: they may do > > > deliberately illegal things to trigger errors, and have security > > > implications (assertions will often deliberately leak kernel addresses). > > > > > > Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been > > > run. This will be printed as 'N' (for kuNit, as K, U and T were already > > > taken). > > > > > > This should discourage people from running KUnit tests on production > > > systems, and to make it easier to tell if tests have been run > > > accidentally (by loading the wrong configuration, etc.) > > > > > > Signed-off-by: David Gow <davidgow@google.com> > > > > There is no reason to distinguish kunit from selftests if the result is > > the same: really make the kernel try really insane stupid things which > > may crash it or put it into a bad state. > > My initial thought is that KUnit is explicitly in-kernel testing, whereas kselftest is (at least somewhat) user-space based. My personal feeling is that "doing weird stuff from userspace" is fundamentally different from "doing weird stuff in the kernel". That being said, in practice many kselftest tests load modules which do strange things, and those could be in scope for something like that. I'd still err on the side of only having those tests (or specifically those modules) add the taint, rather than all selftests, but could be conveniced. The other thing of note is that KUnit tests do often leak pointer addresses, which again is something that's a worry in the kernel, and harmless in userspace. > > So no, this should be renamed to "TEST_BREAK" as I think outside of > > selftest and kunit we may grow the kernel to do stupid things outside > > of that domain and this gives us the flexilibilty to use that in other > > places as well. > > > > It begs the question if we *should* allow userspace to volunterally say > > "hey, we are doing really insane things, brace yourself." Why ? Well > > because selftest has tons of modules. We either then define a macro > > that adds the taint for them and wrap the module declaration for it, > > or we expose a syctl to let userspace volunteer to opt-in to seggest > > we are about to try something stupid with the kernel including loading > > some dangeerous modules which may not have macros which taint the kernel. > > That would let selftest taint on *any* selftest. Because we can run all > > selftests or run one selftest. > > > > Then, if such sysctl is exposed, maybe we should then also use this for > > example for blktests, fstests, fio tests, etc. Is this what TAINT_USER is for? I think (though haven't actually tried) writing to /proc/sys/kernel/tainted will add whatever taint flags are needed from userspace. That being said, I'm not _against_ making this more general, or standardising on the existing TAINT_USER, or similar. Personally, though, I quite like having KUnit separate (but I am, admittedly, biased :-) ). Cheers, -- David
On Tue, May 03, 2022 at 02:49:58PM +0800, David Gow wrote: > On Mon, May 2, 2022 at 2:24 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > On Sun, May 01, 2022 at 11:22:38AM -0700, Luis Chamberlain wrote: > > > On Sat, Apr 30, 2022 at 11:00:19AM +0800, David Gow wrote: > > > > KUnit tests are not supposed to run on production systems: they may do > > > > deliberately illegal things to trigger errors, and have security > > > > implications (assertions will often deliberately leak kernel addresses). > > > > > > > > Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been > > > > run. This will be printed as 'N' (for kuNit, as K, U and T were already > > > > taken). > > > > > > > > This should discourage people from running KUnit tests on production > > > > systems, and to make it easier to tell if tests have been run > > > > accidentally (by loading the wrong configuration, etc.) > > > > > > > > Signed-off-by: David Gow <davidgow@google.com> > > > > > > There is no reason to distinguish kunit from selftests if the result is > > > the same: really make the kernel try really insane stupid things which > > > may crash it or put it into a bad state. > > > > My initial thought is that KUnit is explicitly in-kernel testing, > whereas kselftest is (at least somewhat) user-space based. selftests has modules, although I am not sure if there are selftests which do not load modules. Shuah? > My personal > feeling is that "doing weird stuff from userspace" is fundamentally > different from "doing weird stuff in the kernel". True. > That being said, in > practice many kselftest tests load modules which do strange things, > and those could be in scope for something like that. I'd still err on > the side of only having those tests (or specifically those modules) > add the taint, rather than all selftests, but could be conveniced. Yeah I think now that this can easily be added by having a special new module info, MODULE_TAINTS(taint_flag). Then in check_modinfo() you'd get_modinfo(info, "taints") to then add_taint_module() if set. We can ignore the userspace thing I mentioned earlier as I thought at first we could not add the taint to selftest modules easily but we can. Luis
On Wed, May 4, 2022 at 9:51 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > selftests has modules, although I am not sure if there are selftests > which do not load modules. Shuah? I'm not Shuah, but there are indeed selftests that don't load modules. I went looking for an example and found tools/testing/selftests/bpf/test_doc_build.sh, which runs entirely in userspace (basically just `make docs`).
On Wed, May 04, 2022 at 11:25:14AM -0500, Daniel Latypov wrote: > On Wed, May 4, 2022 at 9:51 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > > selftests has modules, although I am not sure if there are selftests > > which do not load modules. Shuah? > > I'm not Shuah, but there are indeed selftests that don't load modules. > > I went looking for an example and found > tools/testing/selftests/bpf/test_doc_build.sh, which runs entirely in > userspace (basically just `make docs`). OK so, we can just skip tainting considerations for selftests which don't use modules for now. There may be selftests which do wonky things in userspace but indeed I agree the userspace taint would be better for those but I don't think it may be worth bother worrying about those at this point in time. But my point in that sharing a taint between kunit / selftests modules does make sense and is easily possible. The unfortunate aspect is just that selftests don't have a centralized runner, because I can just run tools/testing/selftests/sysctl/sysctl.sh for example and that's it. So I think we have no other option but to just add the module info manually for selftests at this time. Luis
On Wed, May 4, 2022 at 1:46 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > OK so, we can just skip tainting considerations for selftests which > don't use modules for now. There may be selftests which do wonky > things in userspace but indeed I agree the userspace taint would > be better for those but I don't think it may be worth bother > worrying about those at this point in time. > > But my point in that sharing a taint between kunit / selftests modules > does make sense and is easily possible. The unfortunate aspect is just Yes, I 100% agree that we should share a taint for kernelspace testing from both kunit/kselftest. Someone running the system won't care what framework was used. > that selftests don't have a centralized runner, because I can just > run tools/testing/selftests/sysctl/sysctl.sh for example and that's it. > So I think we have no other option but to just add the module info > manually for selftests at this time. Somewhat tangential: there's a number of other test modules that aren't explicitly part of kselftest. Long-term, I think most of them should be converted to kselftest or kunit as appropriate, so they'll get taken care of eventually. A number of these modules depend on CONFIG_DEBUG_KERNEL=y, but we can't pre-emptively set this new taint flag by checking for it as it's too widely used :\ E.g. the debian-based distro I'm using right now has it set $ grep 'CONFIG_DEBUG_KERNEL=y' /boot/config-$(uname -r) CONFIG_DEBUG_KERNEL=y -Daniel
On Wed, May 04, 2022 at 02:19:59PM -0500, Daniel Latypov wrote: > On Wed, May 4, 2022 at 1:46 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > OK so, we can just skip tainting considerations for selftests which > > don't use modules for now. There may be selftests which do wonky > > things in userspace but indeed I agree the userspace taint would > > be better for those but I don't think it may be worth bother > > worrying about those at this point in time. > > > > But my point in that sharing a taint between kunit / selftests modules > > does make sense and is easily possible. The unfortunate aspect is just > > Yes, I 100% agree that we should share a taint for kernelspace testing > from both kunit/kselftest. > Someone running the system won't care what framework was used. OK do you mind doing the nasty work of manually adding the new MODULE_TAINT() to the selftests as part of your effort? *Alternatively*, if we *moved* all sefltests modules to a new lib/debug/selftests/ directory or something like that then t would seem modpost *could* add the taint flag automagically for us without having to edit or require it on new drivers. We have similar type of taint for staging, see add_staging_flag(). I would *highly* prefer this approach, event though it is more work, because I think this is a step we should take anyway. However, I just checked modules on lib/ and well, some of them are already in their own directory, like lib/math/test_div64.c. So not sure, maybe just move a few modules which are just in lib/*.c for now and then just sprinkle the MODULE_TAINT() to the others? > > that selftests don't have a centralized runner, because I can just > > run tools/testing/selftests/sysctl/sysctl.sh for example and that's it. > > So I think we have no other option but to just add the module info > > manually for selftests at this time. > > Somewhat tangential: there's a number of other test modules that > aren't explicitly part of kselftest. Oh interesting, like which one? > Long-term, I think most of them should be converted to kselftest or > kunit as appropriate, so they'll get taken care of eventually. Makes sense. Luis
On Wed, May 04, 2022 at 02:12:30PM -0700, Luis Chamberlain wrote: > On Wed, May 04, 2022 at 02:19:59PM -0500, Daniel Latypov wrote: > > On Wed, May 4, 2022 at 1:46 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > OK so, we can just skip tainting considerations for selftests which > > > don't use modules for now. There may be selftests which do wonky > > > things in userspace but indeed I agree the userspace taint would > > > be better for those but I don't think it may be worth bother > > > worrying about those at this point in time. > > > > > > But my point in that sharing a taint between kunit / selftests modules > > > does make sense and is easily possible. The unfortunate aspect is just > > > > Yes, I 100% agree that we should share a taint for kernelspace testing > > from both kunit/kselftest. > > Someone running the system won't care what framework was used. > > OK do you mind doing the nasty work of manually adding the new > MODULE_TAINT() to the selftests as part of your effort? > > *Alternatively*, if we *moved* all sefltests modules to a new > lib/debug/selftests/ directory or something like that then t would > seem modpost *could* add the taint flag automagically for us without > having to edit or require it on new drivers. We have similar type of > taint for staging, see add_staging_flag(). > > I would *highly* prefer this approach, event though it is more work, > because I think this is a step we should take anyway. > > However, I just checked modules on lib/ and well, some of them are > already in their own directory, like lib/math/test_div64.c. So not > sure, maybe just move a few modules which are just in lib/*.c for now > and then just sprinkle the MODULE_TAINT() to the others? I *think* we could just pull this off with a much easier approach, simply looking for the substrings in the module name in modpost.c: * "_test." || "-test." * ^"test_" || ^"test-" An issue with this of course is a vendor $FOO with an out of tree test driver may end up with the taint. Perhaps we don't care. That means moving selftests to its own directory is not needed at this point in time. Luis
On Thu, May 5, 2022 at 1:57 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Wed, May 04, 2022 at 02:12:30PM -0700, Luis Chamberlain wrote: > > On Wed, May 04, 2022 at 02:19:59PM -0500, Daniel Latypov wrote: > > > On Wed, May 4, 2022 at 1:46 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > OK so, we can just skip tainting considerations for selftests which > > > > don't use modules for now. There may be selftests which do wonky > > > > things in userspace but indeed I agree the userspace taint would > > > > be better for those but I don't think it may be worth bother > > > > worrying about those at this point in time. > > > > > > > > But my point in that sharing a taint between kunit / selftests modules > > > > does make sense and is easily possible. The unfortunate aspect is just > > > > > > Yes, I 100% agree that we should share a taint for kernelspace testing > > > from both kunit/kselftest. > > > Someone running the system won't care what framework was used. > > > > OK do you mind doing the nasty work of manually adding the new > > MODULE_TAINT() to the selftests as part of your effort? > > > > *Alternatively*, if we *moved* all sefltests modules to a new > > lib/debug/selftests/ directory or something like that then t would > > seem modpost *could* add the taint flag automagically for us without > > having to edit or require it on new drivers. We have similar type of > > taint for staging, see add_staging_flag(). > > > > I would *highly* prefer this approach, event though it is more work, > > because I think this is a step we should take anyway. > > > > However, I just checked modules on lib/ and well, some of them are > > already in their own directory, like lib/math/test_div64.c. So not > > sure, maybe just move a few modules which are just in lib/*.c for now > > and then just sprinkle the MODULE_TAINT() to the others? > > I *think* we could just pull this off with a much easier approach, > simply looking for the substrings in the module name in modpost.c: > > * "_test." || "-test." > * ^"test_" || ^"test-" > > An issue with this of course is a vendor $FOO with an out of tree > test driver may end up with the taint. Perhaps we don't care. > > That means moving selftests to its own directory is not needed at this > point in time. I can't say I'm thrilled with the idea of just doing name comparisons, particularly since not all of them match this pattern, for example: bpf_testmod.ko. (Though, frankly, more of them do than I'd've guessed.) Maybe adding a taint call to the selftest helper module framework in kselftest_module.h, though again, there are several tests which don't use it. I _suspect_ we'd be able to hit most of them by tainting in frameworks like the above, and patch the remaining modules manually. There's also definitely a grey area with things like netdevsim, which are used a lot as helper modules by selftests, but may have other uses as well. (The advantage of the KUnit tainting is that, due to KUnit's centralised executor, we can be sure all KUnit tests will correctly trigger the taint. But maybe it doesn't matter as much if one or two selftests miss out.) -- David
On Fri, May 06, 2022 at 03:01:34PM +0800, David Gow wrote: > On Thu, May 5, 2022 at 1:57 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > On Wed, May 04, 2022 at 02:12:30PM -0700, Luis Chamberlain wrote: > > > On Wed, May 04, 2022 at 02:19:59PM -0500, Daniel Latypov wrote: > > > > On Wed, May 4, 2022 at 1:46 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > > OK so, we can just skip tainting considerations for selftests which > > > > > don't use modules for now. There may be selftests which do wonky > > > > > things in userspace but indeed I agree the userspace taint would > > > > > be better for those but I don't think it may be worth bother > > > > > worrying about those at this point in time. > > > > > > > > > > But my point in that sharing a taint between kunit / selftests modules > > > > > does make sense and is easily possible. The unfortunate aspect is just > > > > > > > > Yes, I 100% agree that we should share a taint for kernelspace testing > > > > from both kunit/kselftest. > > > > Someone running the system won't care what framework was used. > > > > > > OK do you mind doing the nasty work of manually adding the new > > > MODULE_TAINT() to the selftests as part of your effort? > > > > > > *Alternatively*, if we *moved* all sefltests modules to a new > > > lib/debug/selftests/ directory or something like that then t would > > > seem modpost *could* add the taint flag automagically for us without > > > having to edit or require it on new drivers. We have similar type of > > > taint for staging, see add_staging_flag(). > > > > > > I would *highly* prefer this approach, event though it is more work, > > > because I think this is a step we should take anyway. > > > > > > However, I just checked modules on lib/ and well, some of them are > > > already in their own directory, like lib/math/test_div64.c. So not > > > sure, maybe just move a few modules which are just in lib/*.c for now > > > and then just sprinkle the MODULE_TAINT() to the others? > > > > I *think* we could just pull this off with a much easier approach, > > simply looking for the substrings in the module name in modpost.c: > > > > * "_test." || "-test." > > * ^"test_" || ^"test-" > > > > An issue with this of course is a vendor $FOO with an out of tree > > test driver may end up with the taint. Perhaps we don't care. > > > > That means moving selftests to its own directory is not needed at this > > point in time. > > I can't say I'm thrilled with the idea of just doing name comparisons, > particularly since not all of them match this pattern, for example: > bpf_testmod.ko. (Though, frankly, more of them do than I'd've > guessed.) > > Maybe adding a taint call to the selftest helper module framework in > kselftest_module.h, though again, there are several tests which don't > use it. Right, I can't think of a generic way to peg this. I think long term we do stand to gain to move all selftests under a lib/debug/selftests/ or something like that, but for now what I suggested is the only thing I can come up with. > I _suspect_ we'd be able to hit most of them by tainting in frameworks > like the above, and patch the remaining modules manually. Works with me. > There's also > definitely a grey area with things like netdevsim, which are used a > lot as helper modules by selftests, but may have other uses as well. They can peg the module if they want the taint. > (The advantage of the KUnit tainting is that, due to KUnit's > centralised executor, we can be sure all KUnit tests will correctly > trigger the taint. But maybe it doesn't matter as much if one or two > selftests miss out.) That is what I was thinking. I'm convinced we *should* move selftests to a one directory. The amount of stuff in lib/ is getting out of hand. Luis
diff --git a/Documentation/admin-guide/tainted-kernels.rst b/Documentation/admin-guide/tainted-kernels.rst index ceeed7b0798d..8f18fc4659d4 100644 --- a/Documentation/admin-guide/tainted-kernels.rst +++ b/Documentation/admin-guide/tainted-kernels.rst @@ -100,6 +100,7 @@ Bit Log Number Reason that got the kernel tainted 15 _/K 32768 kernel has been live patched 16 _/X 65536 auxiliary taint, defined for and used by distros 17 _/T 131072 kernel was built with the struct randomization plugin + 18 _/N 262144 a KUnit test has been run === === ====== ======================================================== Note: The character ``_`` is representing a blank in this table to make reading diff --git a/include/linux/panic.h b/include/linux/panic.h index f5844908a089..1d316c26bf27 100644 --- a/include/linux/panic.h +++ b/include/linux/panic.h @@ -74,7 +74,8 @@ static inline void set_arch_panic_timeout(int timeout, int arch_default_timeout) #define TAINT_LIVEPATCH 15 #define TAINT_AUX 16 #define TAINT_RANDSTRUCT 17 -#define TAINT_FLAGS_COUNT 18 +#define TAINT_KUNIT 18 +#define TAINT_FLAGS_COUNT 19 #define TAINT_FLAGS_MAX ((1UL << TAINT_FLAGS_COUNT) - 1) struct taint_flag { diff --git a/kernel/panic.c b/kernel/panic.c index eb4dfb932c85..9a026d98a00c 100644 --- a/kernel/panic.c +++ b/kernel/panic.c @@ -404,6 +404,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = { [ TAINT_LIVEPATCH ] = { 'K', ' ', true }, [ TAINT_AUX ] = { 'X', ' ', true }, [ TAINT_RANDSTRUCT ] = { 'T', ' ', true }, + [ TAINT_KUNIT ] = { 'N', ' ', true }, }; /** diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 0f66c13d126e..ea8e9162445d 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -11,6 +11,7 @@ #include <kunit/test-bug.h> #include <linux/kernel.h> #include <linux/moduleparam.h> +#include <linux/panic.h> #include <linux/sched/debug.h> #include <linux/sched.h> @@ -498,6 +499,9 @@ int kunit_run_tests(struct kunit_suite *suite) struct kunit_result_stats suite_stats = { 0 }; struct kunit_result_stats total_stats = { 0 }; + /* Taint the kernel so we know we've run tests. */ + add_taint(TAINT_KUNIT, LOCKDEP_STILL_OK); + kunit_print_subtest_start(suite); kunit_suite_for_each_test_case(suite, test_case) {
KUnit tests are not supposed to run on production systems: they may do deliberately illegal things to trigger errors, and have security implications (assertions will often deliberately leak kernel addresses). Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been run. This will be printed as 'N' (for kuNit, as K, U and T were already taken). This should discourage people from running KUnit tests on production systems, and to make it easier to tell if tests have been run accidentally (by loading the wrong configuration, etc.) Signed-off-by: David Gow <davidgow@google.com> --- Changes since v1: https://lore.kernel.org/linux-kselftest/20220429043913.626647-1-davidgow@google.com/ - Make the taint per-module, to handle the case when tests are in (longer lasting) modules. (Thanks Greg KH). Note that this still has checkpatch.pl warnings around bracket placement, which are intentional as part of matching the surrounding code. --- Documentation/admin-guide/tainted-kernels.rst | 1 + include/linux/panic.h | 3 ++- kernel/panic.c | 1 + lib/kunit/test.c | 4 ++++ 4 files changed, 8 insertions(+), 1 deletion(-)