[PATCH] kunit: Fix missing kerneldoc comment
Add a missing kerneldoc comment for the 'test' test context parameter, fixing the following warning: include/kunit/test.h:492: warning: Function parameter or struct member 'test' not described in 'kunit_kfree_const' Reported-by: Stephen Rothwell Closes: https://lore.kernel.org/lkml/20240827160631.67e12...@canb.auug.org.au/ Fixes: f2c6dbd22017 ("kunit: Device wrappers should also manage driver name") Signed-off-by: David Gow --- include/kunit/test.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/kunit/test.h b/include/kunit/test.h index 5ac237c949a0..34b71e42fb10 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -484,6 +484,7 @@ static inline void *kunit_kcalloc(struct kunit *test, size_t n, size_t size, gfp /** * kunit_kfree_const() - conditionally free test managed memory + * @test: The test context object. * @x: pointer to the memory * * Calls kunit_kfree() only if @x is not in .rodata section. -- 2.46.0.469.g59c65b2a67-goog
Re: [PATCH 2/3] list: test: Add a test for hlist_cut_number()
On Wed, 4 Sept 2024 at 21:43, 'Zhen Lei' via KUnit Development wrote: > > Test cases cover all possible situations: > 1. The cut number is invalid: zero or negative > 2. Partially cut. > 3. Cut all. > 4. The cut number is greater than the number of nodes in the old list. > 5. The old list is empty. > > Signed-off-by: Zhen Lei > --- Thanks very much for the detailed test. It's great to see these kept up-to-date! Reviewed-by: David Gow Cheers, -- David > lib/list-test.c | 51 + > 1 file changed, 51 insertions(+) > > diff --git a/lib/list-test.c b/lib/list-test.c > index 37cbc33e9fdb380..3c60a6458545452 100644 > --- a/lib/list-test.c > +++ b/lib/list-test.c > @@ -1172,6 +1172,56 @@ static void hlist_test_for_each_entry_safe(struct > kunit *test) > KUNIT_EXPECT_TRUE(test, hlist_empty(&list)); > } > > +static void hlist_test_cut_number(struct kunit *test) > +{ > + struct hlist_node a[4], *last; > + HLIST_HEAD(old); > + HLIST_HEAD(new); > + int cnt; > + > + hlist_add_head(&a[3], &old); > + hlist_add_head(&a[2], &old); > + hlist_add_head(&a[1], &old); > + hlist_add_head(&a[0], &old); > + > + /* The cut number is less than 0 or zero */ > + cnt = hlist_cut_number(&new, &old, -1, &last); > + KUNIT_EXPECT_EQ(test, cnt, 0); > + KUNIT_EXPECT_EQ(test, hlist_count_nodes(&old), 4); > + cnt = hlist_cut_number(&new, &old, 0, &last); > + KUNIT_EXPECT_EQ(test, cnt, 0); > + KUNIT_EXPECT_EQ(test, hlist_count_nodes(&old), 4); > + > + /* The cut number is less than the number of nodes in the old list. */ > + cnt = hlist_cut_number(&new, &old, 2, &last); > + KUNIT_EXPECT_EQ(test, cnt, 2); > + KUNIT_EXPECT_EQ(test, hlist_count_nodes(&old), 2); > + KUNIT_EXPECT_EQ(test, hlist_count_nodes(&new), 2); > + KUNIT_EXPECT_PTR_EQ(test, last, &a[1]); > + hlist_splice_init(&new, last, &old); > + > + /* The cut number is equal to the number of nodes in the old list. */ > + cnt = hlist_cut_number(&new, &old, 4, &last); > + KUNIT_EXPECT_EQ(test, cnt, 4); > + KUNIT_EXPECT_TRUE(test, hlist_empty(&old)); > + KUNIT_EXPECT_EQ(test, hlist_count_nodes(&new), 4); > + KUNIT_EXPECT_PTR_EQ(test, last, &a[3]); > + hlist_splice_init(&new, last, &old); > + > + /* The cut number is greater than the number of nodes in the old > list. */ > + cnt = hlist_cut_number(&new, &old, 5, &last); > + KUNIT_EXPECT_EQ(test, cnt, 4); > + KUNIT_EXPECT_TRUE(test, hlist_empty(&old)); > + KUNIT_EXPECT_EQ(test, hlist_count_nodes(&new), 4); > + KUNIT_EXPECT_PTR_EQ(test, last, &a[3]); > + > + /* The old list is empty. */ > + cnt = hlist_cut_number(&new, &old, 1, &last); > + KUNIT_EXPECT_EQ(test, cnt, 0); > + KUNIT_EXPECT_TRUE(test, hlist_empty(&old)); > + KUNIT_EXPECT_TRUE(test, hlist_empty(&new)); > +} > + > > static struct kunit_case hlist_test_cases[] = { > KUNIT_CASE(hlist_test_init), > @@ -1192,6 +1242,7 @@ static struct kunit_case hlist_test_cases[] = { > KUNIT_CASE(hlist_test_for_each_entry_continue), > KUNIT_CASE(hlist_test_for_each_entry_from), > KUNIT_CASE(hlist_test_for_each_entry_safe), > + KUNIT_CASE(hlist_test_cut_number), > {}, > }; > > -- > 2.34.1 > > -- > You received this message because you are subscribed to the Google Groups > "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to kunit-dev+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/kunit-dev/20240904134152.2141-3-thunder.leizhen%40huawei.com. smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH] kunit: tool: Build compile_commands.json
On Fri, 17 May 2024 at 03:38, Brendan Jackman wrote: > > compile_commands.json is used by clangd[1] to provide code navigation > and completion functionality to editors. See [2] for an example > configuration that includes this functionality for VSCode. > > It can currently be built manually when using kunit.py, by running: > > ./scripts/clang-tools/gen_compile_commands.py -d .kunit > > With this change however, it's built automatically so you don't need to > manually keep it up to date. > > Unlike the manual approach, having make build the compile_commands.json > means that it appears in the build output tree instead of at the root of > the source tree, so you'll need to add --compile-commands-dir= to your > clangd args for it to be found. > > [1] https://clangd.llvm.org/ > [2] https://github.com/FlorentRevest/linux-kernel-vscode > > Signed-off-by: Brendan Jackman > --- Sorry for missing this earlier. I'm happy with this. Having the compile_commands.json be in the .kunit directory is annoying, but it actually ends up being less annoying for the case where you have lots of out-of-tree builds, so I actually prefer it. Reviewed-by: David Gow Cheers, -- David > tools/testing/kunit/kunit_kernel.py | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/kunit/kunit_kernel.py > b/tools/testing/kunit/kunit_kernel.py > index 7254c110ff23..61931c4926fd 100644 > --- a/tools/testing/kunit/kunit_kernel.py > +++ b/tools/testing/kunit/kunit_kernel.py > @@ -72,7 +72,8 @@ class LinuxSourceTreeOperations: > raise ConfigError(e.output.decode()) > > def make(self, jobs: int, build_dir: str, make_options: > Optional[List[str]]) -> None: > - command = ['make', 'ARCH=' + self._linux_arch, 'O=' + > build_dir, '--jobs=' + str(jobs)] > + command = ['make', 'all', 'compile_commands.json', 'ARCH=' + > self._linux_arch, > + 'O=' + build_dir, '--jobs=' + str(jobs)] > if make_options: > command.extend(make_options) > if self._cross_compile: > > --- > base-commit: 3c999d1ae3c75991902a1a7dad0cb62c2a3008b4 > change-id: 20240516-kunit-compile-commands-d994074fc2be > > Best regards, > -- > Brendan Jackman > smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH] kunit: tool: make --kunitconfig accept dirs, add lib/kunit fragment
On Tue, Feb 23, 2021 at 6:52 AM Daniel Latypov wrote: > > TL;DR > $ ./tools/testing/kunit/kunit.py run --kunitconfig=lib/kunit > > Per suggestion from Ted [1], we can reduce the amount of typing by > assuming a convention that these files are named '.kunitconfig'. > > In the case of [1], we now have > $ ./tools/testing/kunit/kunit.py run --kunitconfig=fs/ext4 > > Also add in such a fragment for kunit itself so we can give that as an > example more close to home (and thus less likely to be accidentally > broken). > > [1] https://lore.kernel.org/linux-ext4/ycnf4yp1db97z...@mit.edu/ > > Signed-off-by: Daniel Latypov > --- Thanks! I really like this. I'd assumed we'd check if the path exists, and fall back to appending ".kunitconfig", but checking if it's a directory is better. I tried this out with all the different combinations I could think of, and it works well. Reviewed-by: David Gow Cheers, -- David > lib/kunit/.kunitconfig | 3 +++ > tools/testing/kunit/kunit.py | 4 +++- > tools/testing/kunit/kunit_kernel.py| 2 ++ > tools/testing/kunit/kunit_tool_test.py | 6 ++ > 4 files changed, 14 insertions(+), 1 deletion(-) > create mode 100644 lib/kunit/.kunitconfig > > diff --git a/lib/kunit/.kunitconfig b/lib/kunit/.kunitconfig > new file mode 100644 > index ..9235b7d42d38 > --- /dev/null > +++ b/lib/kunit/.kunitconfig > @@ -0,0 +1,3 @@ > +CONFIG_KUNIT=y > +CONFIG_KUNIT_TEST=y > +CONFIG_KUNIT_EXAMPLE_TEST=y > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py > index d5144fcb03ac..5da8fb3762f9 100755 > --- a/tools/testing/kunit/kunit.py > +++ b/tools/testing/kunit/kunit.py > @@ -184,7 +184,9 @@ def add_common_opts(parser) -> None: > help='Run all KUnit tests through allyesconfig', > action='store_true') > parser.add_argument('--kunitconfig', > -help='Path to Kconfig fragment that enables > KUnit tests', > +help='Path to Kconfig fragment that enables > KUnit tests.' > +' If given a directory, (e.g. lib/kunit), > "/.kunitconfig" ' > +'will get automatically appended.', > metavar='kunitconfig') > > def add_build_opts(parser) -> None: > diff --git a/tools/testing/kunit/kunit_kernel.py > b/tools/testing/kunit/kunit_kernel.py > index f309a33256cd..89a7d4024e87 100644 > --- a/tools/testing/kunit/kunit_kernel.py > +++ b/tools/testing/kunit/kunit_kernel.py > @@ -132,6 +132,8 @@ class LinuxSourceTree(object): > return > > if kunitconfig_path: > + if os.path.isdir(kunitconfig_path): > + kunitconfig_path = > os.path.join(kunitconfig_path, KUNITCONFIG_PATH) > if not os.path.exists(kunitconfig_path): > raise ConfigError(f'Specified kunitconfig > ({kunitconfig_path}) does not exist') > else: > diff --git a/tools/testing/kunit/kunit_tool_test.py > b/tools/testing/kunit/kunit_tool_test.py > index 1ad3049e9069..2e809dd956a7 100755 > --- a/tools/testing/kunit/kunit_tool_test.py > +++ b/tools/testing/kunit/kunit_tool_test.py > @@ -251,6 +251,12 @@ class LinuxSourceTreeTest(unittest.TestCase): > with tempfile.NamedTemporaryFile('wt') as kunitconfig: > tree = kunit_kernel.LinuxSourceTree('', > kunitconfig_path=kunitconfig.name) > > + def test_dir_kunitconfig(self): > + with tempfile.TemporaryDirectory('') as dir: > + with open(os.path.join(dir, '.kunitconfig'), 'w') as > f: > + pass > + tree = kunit_kernel.LinuxSourceTree('', > kunitconfig_path=dir) > + > # TODO: add more test cases. > > > > base-commit: b12b47249688915e987a9a2a393b522f86f6b7ab > -- > 2.30.0.617.g56c4b15f3c-goog >
[PATCH] kunit: tool: Fix a python tuple typing error
The first argument to namedtuple() should match the name of the type, which wasn't the case for KconfigEntryBase. Fixing this is enough to make mypy show no python typing errors again. Fixes 97752c39bd ("kunit: kunit_tool: Allow .kunitconfig to disable config items") Signed-off-by: David Gow --- tools/testing/kunit/kunit_config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py index 0b550cbd667d..1e2683dcc0e7 100644 --- a/tools/testing/kunit/kunit_config.py +++ b/tools/testing/kunit/kunit_config.py @@ -13,7 +13,7 @@ from typing import List, Set CONFIG_IS_NOT_SET_PATTERN = r'^# CONFIG_(\w+) is not set$' CONFIG_PATTERN = r'^CONFIG_(\w+)=(\S+|".*")$' -KconfigEntryBase = collections.namedtuple('KconfigEntry', ['name', 'value']) +KconfigEntryBase = collections.namedtuple('KconfigEntryBase', ['name', 'value']) class KconfigEntry(KconfigEntryBase): -- 2.30.0.617.g56c4b15f3c-goog
Re: [PATCH v3 1/2] kunit: support failure from dynamic analysis tools
On Wed, Feb 10, 2021 at 6:14 AM Daniel Latypov wrote: > > From: Uriel Guajardo > > Add a kunit_fail_current_test() function to fail the currently running > test, if any, with an error message. > > This is largely intended for dynamic analysis tools like UBSAN and for > fakes. > E.g. say I had a fake ops struct for testing and I wanted my `free` > function to complain if it was called with an invalid argument, or > caught a double-free. Most return void and have no normal means of > signalling failure (e.g. super_operations, iommu_ops, etc.). > > Key points: > * Always update current->kunit_test so anyone can use it. > * commit 83c4e7a0363b ("KUnit: KASAN Integration") only updated it for > CONFIG_KASAN=y > > * Create a new header so non-test code doesn't have > to include all of (e.g. lib/ubsan.c) > > * Forward the file and line number to make it easier to track down > failures > > * Declare the helper function for nice __printf() warnings about mismatched > format strings even when KUnit is not enabled. > > Example output from kunit_fail_current_test("message"): > [15:19:34] [FAILED] example_simple_test > [15:19:34] # example_simple_test: initializing > [15:19:34] # example_simple_test: lib/kunit/kunit-example-test.c:24: > message > [15:19:34] not ok 1 - example_simple_test > > Co-developed-by: Daniel Latypov > Signed-off-by: Uriel Guajardo > Signed-off-by: Daniel Latypov > --- > include/kunit/test-bug.h | 30 ++ > lib/kunit/test.c | 37 + > 2 files changed, 63 insertions(+), 4 deletions(-) > create mode 100644 include/kunit/test-bug.h > > diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h > new file mode 100644 > index ..18b1034ec43a > --- /dev/null > +++ b/include/kunit/test-bug.h > @@ -0,0 +1,30 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * KUnit API allowing dynamic analysis tools to interact with KUnit tests > + * > + * Copyright (C) 2020, Google LLC. > + * Author: Uriel Guajardo > + */ > + > +#ifndef _KUNIT_TEST_BUG_H > +#define _KUNIT_TEST_BUG_H > + > +#define kunit_fail_current_test(fmt, ...) \ > + __kunit_fail_current_test(__FILE__, __LINE__, fmt, ##__VA_ARGS__) > + > +#if IS_ENABLED(CONFIG_KUNIT) As the kernel test robot has pointed out on the second patch, this probably should be IS_BUILTIN(), otherwise this won't build if KUnit is a module, and the code calling it isn't. This does mean that things like UBSAN integration won't work if KUnit is a module, which is a shame. (It's worth noting that the KASAN integration worked around this by only calling inline functions, which would therefore be built-in even if the rest of KUnit was built as a module. I don't think it's quite as convenient to do that here, though.) > + > +extern __printf(3, 4) void __kunit_fail_current_test(const char *file, int > line, > + const char *fmt, ...); > + > +#else > + > +static __printf(3, 4) void __kunit_fail_current_test(const char *file, int > line, > + const char *fmt, ...) > +{ > +} > + > +#endif > + > + > +#endif /* _KUNIT_TEST_BUG_H */ > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index ec9494e914ef..5794059505cf 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -7,6 +7,7 @@ > */ > > #include > +#include > #include > #include > #include > @@ -16,6 +17,38 @@ > #include "string-stream.h" > #include "try-catch-impl.h" > > +/* > + * Fail the current test and print an error message to the log. > + */ > +void __kunit_fail_current_test(const char *file, int line, const char *fmt, > ...) > +{ > + va_list args; > + int len; > + char *buffer; > + > + if (!current->kunit_test) > + return; > + > + kunit_set_failure(current->kunit_test); > + > + /* kunit_err() only accepts literals, so evaluate the args first. */ > + va_start(args, fmt); > + len = vsnprintf(NULL, 0, fmt, args) + 1; > + va_end(args); > + > + buffer = kunit_kmalloc(current->kunit_test, len, GFP_KERNEL); > + if (!buffer) > + return; > + > + va_start(args, fmt); > + vsnprintf(buffer, len, fmt, args); > + va_end(args); > + > + kunit_err(current->kunit_test, "%s:%d: %s", file, line, buffer); > + kunit_kfree(current->kunit_test, buffer); > +} > +EXPORT_SYMBOL_GPL(__kunit_fail_current_test); > + > /* > * Append formatted message to log, size of which is limited to > * KUNIT_LOG_SIZE bytes (including null terminating byte). > @@ -273,9 +306,7 @@ static void kunit_try_run_case(void *data) > struct kunit_suite *suite = ctx->suite; > struct kunit_case *test_case = ctx->test_case; > > -#if (IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)) > current->kunit_test = test; > -#endif /* IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT) */ > >
Re: [PATCH] kunit: don't show `1 == 1` in failed assertion messages
On Fri, Jan 29, 2021 at 10:26 AM Daniel Latypov wrote: > > Currently, given something (fairly dystopian) like > > KUNIT_EXPECT_EQ(test, 2 + 2, 5) > > KUnit will prints a failure message like this. > > Expected 2 + 2 == 5, but > > 2 + 2 == 4 > > 5 == 5 > > With this patch, the output just becomes > > Expected 2 + 2 == 5, but > > 2 + 2 == 4 > > This patch is slightly hacky, but it's quite common* to compare an > expression to a literal integer value, so this can make KUnit less > chatty in many cases. (This patch also fixes variants like > KUNIT_EXPECT_GT, LE, et al.). > > It also allocates an additional string briefly, but given this only > happens on test failures, it doesn't seem too bad a tradeoff. > Also, in most cases it'll realize the lengths are unequal and bail out > before the allocation. > > We could save the result of the formatted string to avoid wasting this > extra work, but it felt cleaner to leave it as-is. > > Edge case: for something silly and unrealistic like > > KUNIT_EXPECT_EQ(test, 4, 5); > > It'll generate this message with a trailing "but" > > Expected 2 + 2 == 5, but > > I assume this is supposed to say "Expected 4 == 5" here. (I tested it to make sure, and that's what it did here.) Personally, I'd ideally like to get rid of the ", but", or even add a "but 4 != 5" style second line. Particularly in case the next line in the output might be confused for the rest of a sentence. That being said, this is a pretty silly edge case: I'd be worried if we ever saw that case in an actual submitted test. People might see it a bit while debugging, though: particularly if they're using KUNIT_EXPECT_EQ(test, 1, 2) as a way of forcing a test to fail. (I've done this while testing tooling, for instance.) > > It didn't feel worth adding a check up-front to see if both sides are > literals to handle this better. > > *A quick grep suggests 100+ comparisons to an integer literal as the > right hand side. > > Signed-off-by: Daniel Latypov > --- I tested this, and it works well: the results are definitely more human readable. I could see it making things slightly more complicated for people who wanted to automatically parse assertion errors, but no-one is doing that, and the extra complexity is pretty minimal anyway. One thing which might be worth doing is expanding this to KUNIT_EXPECT_STREQ() and/or KUNIT_EXPECT_PTR_EQ(). These have slightly more complicated formatting (quotes, leading 0s, etc), though. Comparing pointer literals is pretty unlikely to show up, though, so I don't think it's as important. (I thought that maybe the KASAN shadow memory tests might use them, but a quick look didn't reveal any.) For the record, this is what STREQ()/PTR_EQ()/ failures with literals look like: # example_simple_test: EXPECTATION FAILED at lib/kunit/kunit-example-test.c:31 Expected "abc" == "abd", but "abc" == abc "abd" == abd # example_simple_test: EXPECTATION FAILED at lib/kunit/kunit-example-test.c:33 Expected 0x124 == 0x1234, but 0x124 == 0124 0x1234 == 1234 Either way, though, this is: Tested-by: David Gow Cheers, -- David
Re: [PATCH] kunit: make kunit_tool accept optional path to .kunitconfig fragment
On Sat, Jan 23, 2021 at 8:17 AM Daniel Latypov wrote: > > Currently running tests via KUnit tool means tweaking a .kunitconfig > file, which you'd keep around locally and never commit. > This changes makes it so users can pass in a path to a kunitconfig. > > One of the imagined use cases is having kunitconfig fragments in-tree > to formalize interesting sets of tests for features/subsystems, e.g. > $ ./tools/testing/kunit/kunit.py run fs/ext4/kunitconfig > > For now, this hypothetical fs/ext4/kunitconfig would contain > CONFIG_KUNIT=y > CONFIG_EXT4_FS=y > CONFIG_EXT4_KUNIT_TESTS=y > > At the moment, it's not hard to manually whip up this file, but as more > and more tests get added, this will get tedious. > > It also opens the door to documenting how to run all the tests relevant > to a specific subsystem or feature as a simple one-liner. > > This can be seen as an analogue to tools/testing/selftests/*/config > But in the case of KUnit, the tests live in the same directory as the > code-under-test, so it feels more natural to allow the kunitconfig > fragments to live anywhere. (Though, people could create a separate > directory if wanted; this patch imposes no restrictions on the path). > > Signed-off-by: Daniel Latypov > --- Really glad this is finally happening. I tried it out, and it seemed to work pretty well. I was wondering whether a positional argument like this was best, or whether it'd be better to have an explicitly named argument (--kunitconfig=path). Thinking about it though, I'm quite happy with having this as-is: the only real other contender for a coveted positional argument spot would've been the name of a test or test suite (e.g., kunit.py run ext4_inode_test), and that's not really possible with the kunit_tool architecture as-is. One other comment below (should this work for kunit.py config?), otherwise it looks good. -- David > tools/testing/kunit/kunit.py | 9 ++--- > tools/testing/kunit/kunit_kernel.py| 12 > tools/testing/kunit/kunit_tool_test.py | 25 + > 3 files changed, 39 insertions(+), 7 deletions(-) > > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py > index e808a47c839b..3204a23bd16e 100755 > --- a/tools/testing/kunit/kunit.py > +++ b/tools/testing/kunit/kunit.py > @@ -188,6 +188,9 @@ def add_build_opts(parser) -> None: > help='As in the make command, "Specifies the > number of ' > 'jobs (commands) to run simultaneously."', > type=int, default=8, metavar='jobs') > + parser.add_argument('kunitconfig', > +help='Path to Kconfig fragment that enables > KUnit tests', > +type=str, nargs='?', metavar='kunitconfig') > Should this maybe be in add_common_opts()? I'd assume that we want kunit.py config to accept this custom kunitconfig path as well. > def add_exec_opts(parser) -> None: > parser.add_argument('--timeout', > @@ -256,7 +259,7 @@ def main(argv, linux=None): > os.mkdir(cli_args.build_dir) > > if not linux: > - linux = > kunit_kernel.LinuxSourceTree(cli_args.build_dir) > + linux = > kunit_kernel.LinuxSourceTree(cli_args.build_dir, > kunitconfig_path=cli_args.kunitconfig) > > request = KunitRequest(cli_args.raw_output, >cli_args.timeout, > @@ -274,7 +277,7 @@ def main(argv, linux=None): > os.mkdir(cli_args.build_dir) > > if not linux: > - linux = > kunit_kernel.LinuxSourceTree(cli_args.build_dir) > + linux = > kunit_kernel.LinuxSourceTree(cli_args.build_dir, > kunitconfig_path=cli_args.kunitconfig) > > request = KunitConfigRequest(cli_args.build_dir, > cli_args.make_options) > @@ -286,7 +289,7 @@ def main(argv, linux=None): > sys.exit(1) > elif cli_args.subcommand == 'build': > if not linux: > - linux = > kunit_kernel.LinuxSourceTree(cli_args.build_dir) > + linux = > kunit_kernel.LinuxSourceTree(cli_args.build_dir, > kunitconfig_path=cli_args.kunitconfig) > > request = KunitBuildRequest(cli_args.jobs, > cli_args.build_dir, > diff --git a/tools/testing/kunit/kunit_kernel.py > b/tools/testing/kunit/kunit_kernel.py > index 2076a5a2d060..0b461663e7d9 100644 > --- a/tools/testing/kunit/kunit_kernel.py > +++ b/tools/testing/kunit/kunit_kernel.py > @@ -123,7 +123,7 @@ def get_outfile_path(build_dir) -> str: > class LinuxSourceTree(object): > """Represents a Linux kernel source tree with KUnit tests.""" > > - def __init__(self, build_dir: str, load_config=True, > defconfig=DEFAULT_
[PATCH] kunit: tool: Disable PAGE_POISONING under --alltests
kunit_tool maintains a list of config options which are broken under UML, which we exclude from an otherwise 'make ARCH=um allyesconfig' build used to run all tests with the --alltests option. Something in UML allyesconfig is causing segfaults when page poisining is enabled (and is poisoning with a non-zero value). Previously, this didn't occur, as allyesconfig enabled the CONFIG_PAGE_POISONING_ZERO option, which worked around the problem by zeroing memory. This option has since been removed, and memory is now poisoned with 0xAA, which triggers segfaults in many different codepaths, preventing UML from booting. Note that we have to disable both CONFIG_PAGE_POISONING and CONFIG_DEBUG_PAGEALLOC, as the latter will 'select' the former on architectures (such as UML) which don't implement __kernel_map_pages(). Ideally, we'd fix this properly by tracking down the real root cause, but since this is breaking KUnit's --alltests feature, it's worth disabling there in the meantime so the kernel can boot to the point where tests can actually run. Fixes: f289041ed4 ("mm, page_poison: remove CONFIG_PAGE_POISONING_ZERO") Signed-off-by: David Gow --- As described above, 'make ARCH=um allyesconfig' is broken. KUnit has been maintaining a list of configs to force-disable for this in tools/testing/kunit/configs/broken_on_uml.config. The kernels we've built with this have broken since CONFIG_PAGE_POISONING_ZERO was removed, panic-ing on startup with: <0>[0.10][ T11] Kernel panic - not syncing: Segfault with no mm <4>[0.10][ T11] CPU: 0 PID: 11 Comm: kdevtmpfs Not tainted 5.11.0-rc7-3-g63381dc6f5f1-dirty #4 <4>[0.10][ T11] Stack: <4>[0.10][ T11] 677d3d40 677d3f10 000e 600c0bc0 <4>[0.10][ T11] 677d3d90 603c99be 677d3d90 62529b93 <4>[0.10][ T11] 603c9ac0 677d3f10 62529b00 603c98a0 <4>[0.10][ T11] Call Trace: <4>[0.10][ T11] [<600c0bc0>] ? set_signals+0x0/0x60 <4>[0.10][ T11] [<603c99be>] lookup_mnt+0x11e/0x220 <4>[0.10][ T11] [<62529b93>] ? down_write+0x93/0x180 <4>[0.10][ T11] [<603c9ac0>] ? lock_mount+0x0/0x160 <4>[0.10][ T11] [<62529b00>] ? down_write+0x0/0x180 <4>[0.10][ T11] [<603c98a0>] ? lookup_mnt+0x0/0x220 <4>[0.10][ T11] [<603c8160>] ? namespace_unlock+0x0/0x1a0 <4>[0.10][ T11] [<603c9b25>] lock_mount+0x65/0x160 <4>[0.10][ T11] [<6012f360>] ? up_write+0x0/0x40 <4>[0.10][ T11] [<603cbbd2>] do_new_mount_fc+0xd2/0x220 <4>[0.10][ T11] [<603eb560>] ? vfs_parse_fs_string+0x0/0xa0 <4>[0.10][ T11] [<603cbf04>] do_new_mount+0x1e4/0x260 <4>[0.10][ T11] [<603ccae9>] path_mount+0x1c9/0x6e0 <4>[0.10][ T11] [<603a9f4f>] ? getname_kernel+0xaf/0x1a0 <4>[0.10][ T11] [<603ab280>] ? kern_path+0x0/0x60 <4>[0.10][ T11] [<600238ee>] 0x600238ee <4>[0.10][ T11] [<62523baa>] devtmpfsd+0x52/0xb8 <4>[0.10][ T11] [<62523b58>] ? devtmpfsd+0x0/0xb8 <4>[0.10][ T11] [<600fffd8>] kthread+0x1d8/0x200 <4>[0.10][ T11] [<600a4ea6>] new_thread_handler+0x86/0xc0 Disabling PAGE_POISONING fixes this. The issue can't be repoduced with just PAGE_POISONING, there's clearly something (or several things) also enabled by allyesconfig which contribute. Ideally, we'd track these down and fix this at its root cause, but in the meantime it'd be nice to disable PAGE_POISONING so we can at least get the kernel to boot. One way would be to add a 'depends on !UML' or similar, but since PAGE_POISONING does seem to work in the non-allyesconfig case, adding it to our list of broken configs seemed the better choice. Thoughts? (Note that to reproduce this, you'll want to run ./tools/testing/kunit/kunit.py run --alltests --raw_output It also depends on a couple of other fixes which are not upstream yet: https://www.spinics.net/lists/linux-rtc/msg08294.html https://lore.kernel.org/linux-i3c/20210127040636.1535722-1-david...@google.com/ Cheers, -- David tools/testing/kunit/configs/broken_on_uml.config | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/testing/kunit/configs/broken_on_uml.config b/tools/testing/kunit/configs/broken_on_uml.config index a7f0603d33f6..690870043ac0 100644 --- a/tools/testing/kunit/configs/broken_on_uml.config +++ b/tools/testing/kunit/configs/broken_on_uml.config @@ -40,3 +40,5 @@ # CONFIG_RESET_BRCMSTB_RESCAL is not set # CONFIG_RESET_INTEL_GW is not set # CONFIG_ADI_AXI_ADC is not set +# CONFIG_DEBUG_PAGEALLOC is not set +# CONFIG_PAGE_POISONING is not set -- 2.30.0.478.g8a0d178c01-goog
[PATCH v9 1/5] Add KUnit Struct to Current Task
From: Patricia Alfonso In order to integrate debugging tools like KASAN into the KUnit framework, add KUnit struct to the current task to keep track of the current KUnit test. Signed-off-by: Patricia Alfonso Reviewed-by: Brendan Higgins Signed-off-by: David Gow --- include/linux/sched.h | 4 1 file changed, 4 insertions(+) diff --git a/include/linux/sched.h b/include/linux/sched.h index 683372943093..3a27399c98b1 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1197,6 +1197,10 @@ struct task_struct { struct kcsan_ctxkcsan_ctx; #endif +#if IS_ENABLED(CONFIG_KUNIT) + struct kunit*kunit_test; +#endif + #ifdef CONFIG_FUNCTION_GRAPH_TRACER /* Index of current stored address in ret_stack: */ int curr_ret_stack; -- 2.28.0.163.g6104cc2f0b6-goog
[PATCH v9 3/5] KASAN: Port KASAN Tests to KUnit
From: Patricia Alfonso Transfer all previous tests for KASAN to KUnit so they can be run more easily. Using kunit_tool, developers can run these tests with their other KUnit tests and see "pass" or "fail" with the appropriate KASAN report instead of needing to parse each KASAN report to test KASAN functionalities. All KASAN reports are still printed to dmesg. Stack tests do not work properly when KASAN_STACK is enabled so those tests use a check for "if IS_ENABLED(CONFIG_KASAN_STACK)" so they only run if stack instrumentation is enabled. If KASAN_STACK is not enabled, KUnit will print a statement to let the user know this test was not run with KASAN_STACK enabled. copy_user_test cannot be run in KUnit so there is a separate test file for those tests, which can be run as before as a module. Signed-off-by: Patricia Alfonso Signed-off-by: David Gow Reviewed-by: Brendan Higgins Reviewed-by: Andrey Konovalov Reviewed-by: Dmitry Vyukov --- lib/Kconfig.kasan | 22 +- lib/Makefile | 7 +- lib/test_kasan.c | 901 -- 3 files changed, 21 insertions(+), 909 deletions(-) delete mode 100644 lib/test_kasan.c diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan index 34b84bcbd3d9..a9f7451c541d 100644 --- a/lib/Kconfig.kasan +++ b/lib/Kconfig.kasan @@ -162,10 +162,22 @@ config KASAN_VMALLOC for KASAN to detect more sorts of errors (and to support vmapped stacks), but at the cost of higher memory usage. -config TEST_KASAN - tristate "Module for testing KASAN for bug detection" - depends on m && KASAN +config KASAN_KUNIT_TEST + tristate "KUnit-compatible tests of KASAN bug detection capabilities" if !KUNIT_ALL_TESTS + depends on KASAN && KUNIT + default KUNIT_ALL_TESTS help - This is a test module doing various nasty things like - out of bounds accesses, use after free. It is useful for testing + This is a KUnit test suite doing various nasty things like + out of bounds and use after free accesses. It is useful for testing kernel debugging features like KASAN. + + For more information on KUnit and unit tests in general, please refer + to the KUnit documentation in Documentation/dev-tools/kunit + +config TEST_KASAN_MODULE + tristate "KUnit-incompatible tests of KASAN bug detection capabilities" + depends on m && KASAN + help + This is a part of the KASAN test suite that is incompatible with + KUnit. Currently includes tests that do bad copy_from/to_user + accesses. diff --git a/lib/Makefile b/lib/Makefile index b1c42c10073b..24a5c3cc7262 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -64,9 +64,10 @@ CFLAGS_test_bitops.o += -Werror obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o obj-$(CONFIG_TEST_HASH) += test_hash.o test_siphash.o obj-$(CONFIG_TEST_IDA) += test_ida.o -obj-$(CONFIG_TEST_KASAN) += test_kasan.o -CFLAGS_test_kasan.o += -fno-builtin -CFLAGS_test_kasan.o += $(call cc-disable-warning, vla) +obj-$(CONFIG_KASAN_KUNIT_TEST) += kasan_kunit.o +CFLAGS_kasan_kunit.o += -fno-builtin +CFLAGS_kasan_kunit.o += $(call cc-disable-warning, vla) +obj-$(CONFIG_TEST_KASAN_MODULE) += test_kasan_module.o obj-$(CONFIG_TEST_UBSAN) += test_ubsan.o CFLAGS_test_ubsan.o += $(call cc-disable-warning, vla) UBSAN_SANITIZE_test_ubsan.o := y diff --git a/lib/test_kasan.c b/lib/test_kasan.c deleted file mode 100644 index 842adcd30943.. --- a/lib/test_kasan.c +++ /dev/null @@ -1,901 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -/* - * - * Copyright (c) 2014 Samsung Electronics Co., Ltd. - * Author: Andrey Ryabinin - */ - -#define pr_fmt(fmt) "kasan test: %s " fmt, __func__ - -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include - -#include - -#include - -/* - * We assign some test results to these globals to make sure the tests - * are not eliminated as dead code. - */ - -void *kasan_ptr_result; -int kasan_int_result; - -static struct kunit_resource resource; -static struct kunit_kasan_expectation fail_data; -static bool multishot; - -static int kasan_test_init(struct kunit *test) -{ - /* -* Temporarily enable multi-shot mode and set panic_on_warn=0. -* Otherwise, we'd only get a report for the first case. -*/ - multishot = kasan_save_enable_multi_shot(); - - return 0; -} - -static void kasan_test_exit(struct kunit *test) -{ - kasan_restore_multi_shot(multishot); -} - -/** - * KUNIT_EXPECT_KASAN_FAIL() - Causes a test failure when the expression does - * not cause a KASAN error. This uses a KUnit resource named "kasan_data." Do - * Do not use this name for a KUnit resource outside here. - * - */ -#define KUNIT_EXPECT_KASAN_FAIL(test, condition) do { \ - f
[PATCH v9 2/5] KUnit: KASAN Integration
From: Patricia Alfonso Integrate KASAN into KUnit testing framework. - Fail tests when KASAN reports an error that is not expected - Use KUNIT_EXPECT_KASAN_FAIL to expect a KASAN error in KASAN tests - Expected KASAN reports pass tests and are still printed when run without kunit_tool (kunit_tool still bypasses the report due to the test passing) - KUnit struct in current task used to keep track of the current test from KASAN code Make use of "[PATCH v3 kunit-next 1/2] kunit: generalize kunit_resource API beyond allocated resources" and "[PATCH v3 kunit-next 2/2] kunit: add support for named resources" from Alan Maguire [1] - A named resource is added to a test when a KASAN report is expected - This resource contains a struct for kasan_data containing booleans representing if a KASAN report is expected and if a KASAN report is found [1] (https://lore.kernel.org/linux-kselftest/1583251361-12748-1-git-send-email-alan.magu...@oracle.com/T/#t) Signed-off-by: Patricia Alfonso Signed-off-by: David Gow Reviewed-by: Andrey Konovalov Reviewed-by: Dmitry Vyukov Acked-by: Brendan Higgins --- include/kunit/test.h | 5 + include/linux/kasan.h | 6 ++ lib/kunit/test.c | 13 +++- lib/test_kasan.c | 47 +-- mm/kasan/report.c | 32 + 5 files changed, 96 insertions(+), 7 deletions(-) diff --git a/include/kunit/test.h b/include/kunit/test.h index 59f3144f009a..3391f38389f8 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -224,6 +224,11 @@ struct kunit { struct list_head resources; /* Protected by lock. */ }; +static inline void kunit_set_failure(struct kunit *test) +{ + WRITE_ONCE(test->success, false); +} + void kunit_init_test(struct kunit *test, const char *name, char *log); int kunit_run_tests(struct kunit_suite *suite); diff --git a/include/linux/kasan.h b/include/linux/kasan.h index 82522e996c76..3ccb7874a466 100644 --- a/include/linux/kasan.h +++ b/include/linux/kasan.h @@ -14,6 +14,12 @@ struct task_struct; #include #include +/* kasan_data struct is used in KUnit tests for KASAN expected failures */ +struct kunit_kasan_expectation { + bool report_expected; + bool report_found; +}; + extern unsigned char kasan_early_shadow_page[PAGE_SIZE]; extern pte_t kasan_early_shadow_pte[PTRS_PER_PTE]; extern pmd_t kasan_early_shadow_pmd[PTRS_PER_PMD]; diff --git a/lib/kunit/test.c b/lib/kunit/test.c index c36037200310..dcc35fd30d95 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -10,16 +10,12 @@ #include #include #include +#include #include "debugfs.h" #include "string-stream.h" #include "try-catch-impl.h" -static void kunit_set_failure(struct kunit *test) -{ - WRITE_ONCE(test->success, false); -} - static void kunit_print_tap_version(void) { static bool kunit_has_printed_tap_version; @@ -288,6 +284,10 @@ static void kunit_try_run_case(void *data) struct kunit_suite *suite = ctx->suite; struct kunit_case *test_case = ctx->test_case; +#if (IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)) + current->kunit_test = test; +#endif /* IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT) */ + /* * kunit_run_case_internal may encounter a fatal error; if it does, * abort will be called, this thread will exit, and finally the parent @@ -602,6 +602,9 @@ void kunit_cleanup(struct kunit *test) spin_unlock(&test->lock); kunit_remove_resource(test, res); } +#if (IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)) + current->kunit_test = NULL; +#endif /* IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)*/ } EXPORT_SYMBOL_GPL(kunit_cleanup); diff --git a/lib/test_kasan.c b/lib/test_kasan.c index dc2c6a51d11a..842adcd30943 100644 --- a/lib/test_kasan.c +++ b/lib/test_kasan.c @@ -23,19 +23,62 @@ #include +#include + /* * We assign some test results to these globals to make sure the tests * are not eliminated as dead code. */ -int kasan_int_result; void *kasan_ptr_result; +int kasan_int_result; + +static struct kunit_resource resource; +static struct kunit_kasan_expectation fail_data; +static bool multishot; + +static int kasan_test_init(struct kunit *test) +{ + /* +* Temporarily enable multi-shot mode and set panic_on_warn=0. +* Otherwise, we'd only get a report for the first case. +*/ + multishot = kasan_save_enable_multi_shot(); + + return 0; +} + +static void kasan_test_exit(struct kunit *test) +{ + kasan_restore_multi_shot(multishot); +} + +/** + * KUNIT_EXPECT_KASAN_FAIL() - Causes a test failure when the expression does + * not cause a KASAN err
[PATCH v9 0/5] KASAN-KUnit Integration
This patchset contains everything needed to integrate KASAN and KUnit. KUnit will be able to: (1) Fail tests when an unexpected KASAN error occurs (2) Pass tests when an expected KASAN error occurs Convert KASAN tests to KUnit with the exception of copy_user_test because KUnit is unable to test those. Add documentation on how to run the KASAN tests with KUnit and what to expect when running these tests. This patchset depends on: - "kunit: extend kunit resources API" [1] - This is already present in the kselftest/kunit branch I'd _really_ like to get this into 5.9 if possible: we also have some other changes which depend on some things here. Changes from v8: - Rebased on top of kselftest/kunit - (Which, with this patchset, should rebase cleanly on 5.8-rc7) - Renamed the KUnit test suite, config name to patch the proposed naming guidelines for KUnit tests[6] Changes from v7: - Rebased on top of kselftest/kunit - Rebased on top of v4 of the kunit resources API[1] - Rebased on top of v4 of the FORTIFY_SOURCE fix[2,3,4] - Updated the Kconfig entry to support KUNIT_ALL_TESTS Changes from v6: - Rebased on top of kselftest/kunit - Rebased on top of Daniel Axtens' fix for FORTIFY_SOURCE incompatibilites [2] - Removed a redundant report_enabled() check. - Fixed some places with out of date Kconfig names in the documentation. Changes from v5: - Split out the panic_on_warn changes to a separate patch. - Fix documentation to fewer to the new Kconfig names. - Fix some changes which were in the wrong patch. - Rebase on top of kselftest/kunit (currently identical to 5.7-rc1) Changes from v4: - KASAN no longer will panic on errors if both panic_on_warn and kasan_multishot are enabled. - As a result, the KASAN tests will no-longer disable panic_on_warn. - This also means panic_on_warn no-longer needs to be exported. - The use of temporary "kasan_data" variables has been cleaned up somewhat. - A potential refcount/resource leak should multiple KASAN errors appear during an assertion was fixed. - Some wording changes to the KASAN test Kconfig entries. Changes from v3: - KUNIT_SET_KASAN_DATA and KUNIT_DO_EXPECT_KASAN_FAIL have been combined and included in KUNIT_DO_EXPECT_KASAN_FAIL() instead. - Reordered logic in kasan_update_kunit_status() in report.c to be easier to read. - Added comment to not use the name "kasan_data" for any kunit tests outside of KUNIT_EXPECT_KASAN_FAIL(). Changes since v2: - Due to Alan's changes in [1], KUnit can be built as a module. - The name of the tests that could not be run with KUnit has been changed to be more generic: test_kasan_module. - Documentation on how to run the new KASAN tests and what to expect when running them has been added. - Some variables and functions are now static. - Now save/restore panic_on_warn in a similar way to kasan_multi_shot and renamed the init/exit functions to be more generic to accommodate. - Due to [4] in kasan_strings, kasan_memchr, and kasan_memcmp will fail if CONFIG_AMD_MEM_ENCRYPT is enabled so return early and print message explaining this circumstance. - Changed preprocessor checks to C checks where applicable. Changes since v1: - Make use of Alan Maguire's suggestion to use his patch that allows static resources for integration instead of adding a new attribute to the kunit struct - All KUNIT_EXPECT_KASAN_FAIL statements are local to each test - The definition of KUNIT_EXPECT_KASAN_FAIL is local to the test_kasan.c file since it seems this is the only place this will be used. - Integration relies on KUnit being builtin - copy_user_test has been separated into its own file since KUnit is unable to test these. This can be run as a module just as before, using CONFIG_TEST_KASAN_USER - The addition to the current task has been separated into its own patch as this is a significant enough change to be on its own. [1] https://lore.kernel.org/linux-kselftest/cafd5g46uu_5tg89uom0dj5cmq+11cwjbnsd-k_cvy6bqueu...@mail.gmail.com/T/#t [2] https://lore.kernel.org/linux-mm/20200424145521.8203-1-...@axtens.net/ [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=adb72ae1915db28f934e9e02c18bfcea2f3ed3b7 [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=47227d27e2fcb01a9e8f5958d8997cf47a820afc [5] https://bugzilla.kernel.org/show_bug.cgi?id=206337 [6] https://lore.kernel.org/linux-kselftest/20200620054944.167330-1-david...@google.com/ David Gow (1): mm: kasan: Do not panic if both panic_on_warn and kasan_multishot set Patricia Alfonso (4): Add KUnit Struct to Current Task KUnit: KASAN Integration KASAN: Port KASAN Tests to KUnit KASAN: Testing Documentation Documentation/dev-tools/kasan.rst | 70 +++ include/kunit/test.h | 5 + include/linux/kasan.h | 6 + include/linux/sched.h | 4 + lib/Kc
[PATCH v9 4/5] KASAN: Testing Documentation
From: Patricia Alfonso Include documentation on how to test KASAN using CONFIG_TEST_KASAN_KUNIT and CONFIG_TEST_KASAN_MODULE. Signed-off-by: Patricia Alfonso Signed-off-by: David Gow Reviewed-by: Andrey Konovalov Reviewed-by: Dmitry Vyukov Acked-by: Brendan Higgins --- Documentation/dev-tools/kasan.rst | 70 +++ 1 file changed, 70 insertions(+) diff --git a/Documentation/dev-tools/kasan.rst b/Documentation/dev-tools/kasan.rst index c652d740735d..1f9a75df0fc8 100644 --- a/Documentation/dev-tools/kasan.rst +++ b/Documentation/dev-tools/kasan.rst @@ -281,3 +281,73 @@ unmapped. This will require changes in arch-specific code. This allows ``VMAP_STACK`` support on x86, and can simplify support of architectures that do not have a fixed module region. + +CONFIG_KASAN_KUNIT_TEST & CONFIG_TEST_KASAN_MODULE +-- + +``CONFIG_KASAN_KUNIT_TEST`` utilizes the KUnit Test Framework for testing. +This means each test focuses on a small unit of functionality and +there are a few ways these tests can be run. + +Each test will print the KASAN report if an error is detected and then +print the number of the test and the status of the test: + +pass:: + +ok 28 - kmalloc_double_kzfree +or, if kmalloc failed:: + +# kmalloc_large_oob_right: ASSERTION FAILED at lib/test_kasan.c:163 +Expected ptr is not null, but is +not ok 4 - kmalloc_large_oob_right +or, if a KASAN report was expected, but not found:: + +# kmalloc_double_kzfree: EXPECTATION FAILED at lib/test_kasan.c:629 +Expected kasan_data->report_expected == kasan_data->report_found, but +kasan_data->report_expected == 1 +kasan_data->report_found == 0 +not ok 28 - kmalloc_double_kzfree + +All test statuses are tracked as they run and an overall status will +be printed at the end:: + +ok 1 - kasan_kunit_test + +or:: + +not ok 1 - kasan_kunit_test + +(1) Loadable Module + + +With ``CONFIG_KUNIT`` enabled, ``CONFIG_KASAN_KUNIT_TEST`` can be built as +a loadable module and run on any architecture that supports KASAN +using something like insmod or modprobe. + +(2) Built-In +~ + +With ``CONFIG_KUNIT`` built-in, ``CONFIG_KASAN_KUNIT_TEST`` can be built-in +on any architecure that supports KASAN. These and any other KUnit +tests enabled will run and print the results at boot as a late-init +call. + +(3) Using kunit_tool +~ + +With ``CONFIG_KUNIT`` and ``CONFIG_KASAN_KUNIT_TEST`` built-in, we can also +use kunit_tool to see the results of these along with other KUnit +tests in a more readable way. This will not print the KASAN reports +of tests that passed. Use `KUnit documentation <https://www.kernel.org/doc/html/latest/dev-tools/kunit/index.html>`_ for more up-to-date +information on kunit_tool. + +.. _KUnit: https://www.kernel.org/doc/html/latest/dev-tools/kunit/index.html + +``CONFIG_TEST_KASAN_MODULE`` is a set of KASAN tests that could not be +converted to KUnit. These tests can be run only as a module with +``CONFIG_TEST_KASAN_MODULE`` built as a loadable module and +``CONFIG_KASAN`` built-in. The type of error expected and the +function being run is printed before the expression expected to give +an error. Then the error is printed, if found, and that test +should be interpretted to pass only if the error was the one expected +by the test. -- 2.28.0.163.g6104cc2f0b6-goog
[PATCH v9 5/5] mm: kasan: Do not panic if both panic_on_warn and kasan_multishot set
KASAN errors will currently trigger a panic when panic_on_warn is set. This renders kasan_multishot useless, as further KASAN errors won't be reported if the kernel has already paniced. By making kasan_multishot disable this behaviour for KASAN errors, we can still have the benefits of panic_on_warn for non-KASAN warnings, yet be able to use kasan_multishot. This is particularly important when running KASAN tests, which need to trigger multiple KASAN errors: previously these would panic the system if panic_on_warn was set, now they can run (and will panic the system should non-KASAN warnings show up). Signed-off-by: David Gow Reviewed-by: Andrey Konovalov Reviewed-by: Brendan Higgins --- mm/kasan/report.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm/kasan/report.c b/mm/kasan/report.c index 90a1348c8b81..c83d6fde9ee4 100644 --- a/mm/kasan/report.c +++ b/mm/kasan/report.c @@ -95,7 +95,7 @@ static void end_report(unsigned long *flags) pr_err("==\n"); add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE); spin_unlock_irqrestore(&report_lock, *flags); - if (panic_on_warn) { + if (panic_on_warn && !test_bit(KASAN_BIT_MULTI_SHOT, &kasan_flags)) { /* * This thread may hit another WARN() in the panic path. * Resetting this prevents additional WARN() from panicking the -- 2.28.0.163.g6104cc2f0b6-goog
Re: [PATCH v1 1/2] kunit: tool: fix running kunit_tool from outside kernel tree
On Sat, Aug 8, 2020 at 4:51 PM Brendan Higgins wrote: > > On Fri, Aug 7, 2020 at 10:45 PM David Gow wrote: > > > > On Sat, Aug 8, 2020 at 9:17 AM Brendan Higgins > > wrote: > > > > > > Currently kunit_tool does not work correctly when executed from a path > > > outside of the kernel tree, so make sure that the current working > > > directory is correct and the kunit_dir is properly initialized before > > > running. > > > > > > Signed-off-by: Brendan Higgins > > > --- > > > tools/testing/kunit/kunit.py | 8 > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py > > > index 425ef40067e7..96344a11ff1f 100755 > > > --- a/tools/testing/kunit/kunit.py > > > +++ b/tools/testing/kunit/kunit.py > > > @@ -237,9 +237,14 @@ def main(argv, linux=None): > > > > > > cli_args = parser.parse_args(argv) > > > > > > + if get_kernel_root_path(): > > > + print('cd ' + get_kernel_root_path()) > > Do we want to print this, or is it a leftover debug statement? > > Whoops, I was supposed to delete that. That's embarrassing... ^_^; > > > > + os.chdir(get_kernel_root_path()) > > > + > > > if cli_args.subcommand == 'run': > > > if not os.path.exists(cli_args.build_dir): > > > os.mkdir(cli_args.build_dir) > > > + create_default_kunitconfig() > > Why are we adding this everywhere when it's already in config_tests, > > which should already be called in all of the places where a > > kunitconfig is required? > > Ah yes, .kunitconfig needs to be created before config_tests() can be > called because the LinuxSourceTree constructor needs .kunitconfig to > exist. I see. I guess the ultimate solution will be for LinuxSourceTree not require a .kunitconfig unless it's actually being used. > > Is the goal to always copy the default kunitconfig when creating a new > > build_dir? While I can sort-of see why we might want to do that, if > > the build dir doesn't exist, most of the subcommands will fail anyway > > (maybe we should only create the build-dir for 'config' and 'run'?) > > I just did it because we were getting a failure in a constructor so we > couldn't do much. Ideally we would check that the current state allows > for the command that the user intended to run, but I think that's > beyond the scope of this change. > > So I guess the real question is: Is it okay for it to crash in the > constructor with a cryptic error message for now, or do we want to let > it fail with a slightly less cryptic message later? > I personally am leaning towards allowing it to crash in the build, exec, etc. subcommands for now, and tidying up the error messages later, rather than silently creating a blank build dir, only for it then to fail later. In the meantime, yeah, we can add this for the config and run tasks, and maybe remove the whole "if cli_args.build_dir" / mkdir branch from the other subcommands. If we weren't going to fix the LinuxSourceTree constructor, it'd make sense to get rid of the redundant code to create it in config_tests(), too, but I'm not sure it's worthwhile. In any case, now I know what's happening, I'm okay with anything moderately sensible which gets the 'config' and 'run' subcommands working on an empty build dir, and the code and error messages can be fixed when tidying up the LinuxSourceTree() constructor in a separate patch. Cheers, -- David > > > if not linux: > > > linux = kunit_kernel.LinuxSourceTree() > > > @@ -257,6 +262,7 @@ def main(argv, linux=None): > > > if cli_args.build_dir: > > > if not os.path.exists(cli_args.build_dir): > > > os.mkdir(cli_args.build_dir) > > > + create_default_kunitconfig() > > > > > > if not linux: > > > linux = kunit_kernel.LinuxSourceTree() > > > @@ -273,6 +279,7 @@ def main(argv, linux=None): > > > if cli_args.build_dir: > > > if not os.path.exists(cli_args.build_dir): > > > os.mkdir(cli_args.build_dir) > > > + create_default_kunitconfig() > > > > > > if not linux: > > > linux = kunit_kernel.LinuxSourceTree() > > > @@ -291,6 +298,7 @@ def main(argv, linux=None): > > > if cli_args.build_dir: > > > if not os.path.exists(cli_args.build_dir): > > > os.mkdir(cli_args.build_dir) > > > + create_default_kunitconfig() > > > > > > if not linux: > > > linux = kunit_kernel.LinuxSourceTree() > > > > > > base-commit: 30185b69a2d533c4ba6ca926b8390ce7de495e29 > > > -- > > > 2.28.0.236.gb10cc79966-goog > > >
[PATCH v12 0/6] KASAN-KUnit Integration
nux.git/commit/?id=adb72ae1915db28f934e9e02c18bfcea2f3ed3b7 [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=47227d27e2fcb01a9e8f5958d8997cf47a820afc [5] https://bugzilla.kernel.org/show_bug.cgi?id=206337 [6] https://lore.kernel.org/linux-kselftest/20200620054944.167330-1-david...@google.com/ [7] https://lkml.org/lkml/2020/7/31/571 [8] https://lore.kernel.org/linux-kselftest/8d43e88e-1356-cd63-9152-209b81b16...@linuxfoundation.org/T/#u David Gow (2): kasan: test: Make KASAN KUnit test comply with naming guidelines mm: kasan: Do not panic if both panic_on_warn and kasan_multishot set Patricia Alfonso (4): Add KUnit Struct to Current Task KUnit: KASAN Integration KASAN: Port KASAN Tests to KUnit KASAN: Testing Documentation Documentation/dev-tools/kasan.rst | 70 +++ include/kunit/test.h | 5 + include/linux/kasan.h | 6 + include/linux/sched.h | 4 + lib/Kconfig.kasan | 22 +- lib/Makefile | 7 +- lib/kasan_kunit.c | 769 + lib/kunit/test.c | 13 +- lib/test_kasan.c | 903 -- lib/test_kasan_module.c | 111 mm/kasan/report.c | 34 +- 11 files changed, 1027 insertions(+), 917 deletions(-) create mode 100644 lib/kasan_kunit.c delete mode 100644 lib/test_kasan.c create mode 100644 lib/test_kasan_module.c -- 2.28.0.236.gb10cc79966-goog
[PATCH v12 1/6] Add KUnit Struct to Current Task
From: Patricia Alfonso In order to integrate debugging tools like KASAN into the KUnit framework, add KUnit struct to the current task to keep track of the current KUnit test. Signed-off-by: Patricia Alfonso Reviewed-by: Brendan Higgins Tested-by: Andrey Konovalov Signed-off-by: David Gow --- include/linux/sched.h | 4 1 file changed, 4 insertions(+) diff --git a/include/linux/sched.h b/include/linux/sched.h index f92d03087b5c..3db26aa88971 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1203,6 +1203,10 @@ struct task_struct { #endif #endif +#if IS_ENABLED(CONFIG_KUNIT) + struct kunit*kunit_test; +#endif + #ifdef CONFIG_FUNCTION_GRAPH_TRACER /* Index of current stored address in ret_stack: */ int curr_ret_stack; -- 2.28.0.236.gb10cc79966-goog
[PATCH v12 2/6] KUnit: KASAN Integration
From: Patricia Alfonso Integrate KASAN into KUnit testing framework. - Fail tests when KASAN reports an error that is not expected - Use KUNIT_EXPECT_KASAN_FAIL to expect a KASAN error in KASAN tests - Expected KASAN reports pass tests and are still printed when run without kunit_tool (kunit_tool still bypasses the report due to the test passing) - KUnit struct in current task used to keep track of the current test from KASAN code Make use of "[PATCH v3 kunit-next 1/2] kunit: generalize kunit_resource API beyond allocated resources" and "[PATCH v3 kunit-next 2/2] kunit: add support for named resources" from Alan Maguire [1] - A named resource is added to a test when a KASAN report is expected - This resource contains a struct for kasan_data containing booleans representing if a KASAN report is expected and if a KASAN report is found [1] (https://lore.kernel.org/linux-kselftest/1583251361-12748-1-git-send-email-alan.magu...@oracle.com/T/#t) Signed-off-by: Patricia Alfonso Signed-off-by: David Gow Reviewed-by: Andrey Konovalov Reviewed-by: Dmitry Vyukov Acked-by: Brendan Higgins Tested-by: Andrey Konovalov --- include/kunit/test.h | 5 + include/linux/kasan.h | 6 ++ lib/kunit/test.c | 13 +++- lib/test_kasan.c | 47 +-- mm/kasan/report.c | 32 + 5 files changed, 96 insertions(+), 7 deletions(-) diff --git a/include/kunit/test.h b/include/kunit/test.h index 59f3144f009a..3391f38389f8 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -224,6 +224,11 @@ struct kunit { struct list_head resources; /* Protected by lock. */ }; +static inline void kunit_set_failure(struct kunit *test) +{ + WRITE_ONCE(test->success, false); +} + void kunit_init_test(struct kunit *test, const char *name, char *log); int kunit_run_tests(struct kunit_suite *suite); diff --git a/include/linux/kasan.h b/include/linux/kasan.h index 087fba34b209..30d343b4a40a 100644 --- a/include/linux/kasan.h +++ b/include/linux/kasan.h @@ -14,6 +14,12 @@ struct task_struct; #include #include +/* kasan_data struct is used in KUnit tests for KASAN expected failures */ +struct kunit_kasan_expectation { + bool report_expected; + bool report_found; +}; + extern unsigned char kasan_early_shadow_page[PAGE_SIZE]; extern pte_t kasan_early_shadow_pte[PTRS_PER_PTE]; extern pmd_t kasan_early_shadow_pmd[PTRS_PER_PMD]; diff --git a/lib/kunit/test.c b/lib/kunit/test.c index c36037200310..dcc35fd30d95 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -10,16 +10,12 @@ #include #include #include +#include #include "debugfs.h" #include "string-stream.h" #include "try-catch-impl.h" -static void kunit_set_failure(struct kunit *test) -{ - WRITE_ONCE(test->success, false); -} - static void kunit_print_tap_version(void) { static bool kunit_has_printed_tap_version; @@ -288,6 +284,10 @@ static void kunit_try_run_case(void *data) struct kunit_suite *suite = ctx->suite; struct kunit_case *test_case = ctx->test_case; +#if (IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)) + current->kunit_test = test; +#endif /* IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT) */ + /* * kunit_run_case_internal may encounter a fatal error; if it does, * abort will be called, this thread will exit, and finally the parent @@ -602,6 +602,9 @@ void kunit_cleanup(struct kunit *test) spin_unlock(&test->lock); kunit_remove_resource(test, res); } +#if (IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)) + current->kunit_test = NULL; +#endif /* IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)*/ } EXPORT_SYMBOL_GPL(kunit_cleanup); diff --git a/lib/test_kasan.c b/lib/test_kasan.c index 53e953bb1d1d..58bffadd8367 100644 --- a/lib/test_kasan.c +++ b/lib/test_kasan.c @@ -23,6 +23,8 @@ #include +#include + #include "../mm/kasan/kasan.h" #define OOB_TAG_OFF (IS_ENABLED(CONFIG_KASAN_GENERIC) ? 0 : KASAN_SHADOW_SCALE_SIZE) @@ -32,14 +34,55 @@ * are not eliminated as dead code. */ -int kasan_int_result; void *kasan_ptr_result; +int kasan_int_result; + +static struct kunit_resource resource; +static struct kunit_kasan_expectation fail_data; +static bool multishot; + +static int kasan_test_init(struct kunit *test) +{ + /* +* Temporarily enable multi-shot mode and set panic_on_warn=0. +* Otherwise, we'd only get a report for the first case. +*/ + multishot = kasan_save_enable_multi_shot(); + + return 0; +} + +static void kasan_test_exit(struct kunit *test) +{ + kasan_restore_multi_shot(multishot); +} + +/** + *
[PATCH v12 3/6] KASAN: Port KASAN Tests to KUnit
From: Patricia Alfonso Transfer all previous tests for KASAN to KUnit so they can be run more easily. Using kunit_tool, developers can run these tests with their other KUnit tests and see "pass" or "fail" with the appropriate KASAN report instead of needing to parse each KASAN report to test KASAN functionalities. All KASAN reports are still printed to dmesg. Stack tests do not work properly when KASAN_STACK is enabled so those tests use a check for "if IS_ENABLED(CONFIG_KASAN_STACK)" so they only run if stack instrumentation is enabled. If KASAN_STACK is not enabled, KUnit will print a statement to let the user know this test was not run with KASAN_STACK enabled. copy_user_test and kasan_rcu_uaf cannot be run in KUnit so there is a separate test file for those tests, which can be run as before as a module. Signed-off-by: Patricia Alfonso Signed-off-by: David Gow Reviewed-by: Brendan Higgins Reviewed-by: Andrey Konovalov Reviewed-by: Dmitry Vyukov Tested-by: Andrey Konovalov --- lib/Kconfig.kasan | 22 +- lib/Makefile| 3 +- lib/test_kasan.c| 687 +++- lib/test_kasan_module.c | 111 +++ 4 files changed, 385 insertions(+), 438 deletions(-) create mode 100644 lib/test_kasan_module.c diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan index 047b53dbfd58..9a237887e52e 100644 --- a/lib/Kconfig.kasan +++ b/lib/Kconfig.kasan @@ -167,12 +167,24 @@ config KASAN_VMALLOC for KASAN to detect more sorts of errors (and to support vmapped stacks), but at the cost of higher memory usage. -config TEST_KASAN - tristate "Module for testing KASAN for bug detection" - depends on m +config KASAN_KUNIT_TEST + tristate "KUnit-compatible tests of KASAN bug detection capabilities" if !KUNIT_ALL_TESTS + depends on KASAN && KUNIT + default KUNIT_ALL_TESTS help - This is a test module doing various nasty things like - out of bounds accesses, use after free. It is useful for testing + This is a KUnit test suite doing various nasty things like + out of bounds and use after free accesses. It is useful for testing kernel debugging features like KASAN. + For more information on KUnit and unit tests in general, please refer + to the KUnit documentation in Documentation/dev-tools/kunit + +config TEST_KASAN_MODULE + tristate "KUnit-incompatible tests of KASAN bug detection capabilities" + depends on m && KASAN + help + This is a part of the KASAN test suite that is incompatible with + KUnit. Currently includes tests that do bad copy_from/to_user + accesses. + endif # KASAN diff --git a/lib/Makefile b/lib/Makefile index e290fc5707ea..6ade090a29ff 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -60,9 +60,10 @@ CFLAGS_test_bitops.o += -Werror obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o obj-$(CONFIG_TEST_HASH) += test_hash.o test_siphash.o obj-$(CONFIG_TEST_IDA) += test_ida.o -obj-$(CONFIG_TEST_KASAN) += test_kasan.o +obj-$(CONFIG_KASAN_KUNIT_TEST) += test_kasan.o CFLAGS_test_kasan.o += -fno-builtin CFLAGS_test_kasan.o += $(call cc-disable-warning, vla) +obj-$(CONFIG_TEST_KASAN_MODULE) += test_kasan_module.o obj-$(CONFIG_TEST_UBSAN) += test_ubsan.o CFLAGS_test_ubsan.o += $(call cc-disable-warning, vla) UBSAN_SANITIZE_test_ubsan.o := y diff --git a/lib/test_kasan.c b/lib/test_kasan.c index 58bffadd8367..63c26171a791 100644 --- a/lib/test_kasan.c +++ b/lib/test_kasan.c @@ -5,8 +5,6 @@ * Author: Andrey Ryabinin */ -#define pr_fmt(fmt) "kasan test: %s " fmt, __func__ - #include #include #include @@ -77,416 +75,327 @@ static void kasan_test_exit(struct kunit *test) fail_data.report_found); \ } while (0) - - -/* - * Note: test functions are marked noinline so that their names appear in - * reports. - */ -static noinline void __init kmalloc_oob_right(void) +static void kmalloc_oob_right(struct kunit *test) { char *ptr; size_t size = 123; - pr_info("out-of-bounds to right\n"); ptr = kmalloc(size, GFP_KERNEL); - if (!ptr) { - pr_err("Allocation failed\n"); - return; - } - - ptr[size + OOB_TAG_OFF] = 'x'; + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr); + KUNIT_EXPECT_KASAN_FAIL(test, ptr[size + OOB_TAG_OFF] = 'x'); kfree(ptr); } -static noinline void __init kmalloc_oob_left(void) +static void kmalloc_oob_left(struct kunit *test) { char *ptr; size_t size = 15; - pr_info("out-of-bounds to left\n"); ptr = kmalloc(size, GFP_KERNEL); - if (!ptr) { - pr_err("Allocation failed\n"); - return; - } + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr); - *ptr =
[PATCH v12 4/6] kasan: test: Make KASAN KUnit test comply with naming guidelines
The proposed KUnit test naming guidelines[1] suggest naming KUnit test modules [suite]_kunit (and hence test source files [suite]_kunit.c). Rename test_kunit.c to kasan_kunit.c to comply with this, and be consistent with other KUnit tests. [1]: https://lore.kernel.org/linux-kselftest/20200702071416.1780522-1-david...@google.com/ Signed-off-by: David Gow Tested-by: Andrey Konovalov --- lib/Makefile| 6 +++--- lib/{test_kasan.c => kasan_kunit.c} | 0 2 files changed, 3 insertions(+), 3 deletions(-) rename lib/{test_kasan.c => kasan_kunit.c} (100%) diff --git a/lib/Makefile b/lib/Makefile index 6ade090a29ff..ed26f56ceda2 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -60,9 +60,9 @@ CFLAGS_test_bitops.o += -Werror obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o obj-$(CONFIG_TEST_HASH) += test_hash.o test_siphash.o obj-$(CONFIG_TEST_IDA) += test_ida.o -obj-$(CONFIG_KASAN_KUNIT_TEST) += test_kasan.o -CFLAGS_test_kasan.o += -fno-builtin -CFLAGS_test_kasan.o += $(call cc-disable-warning, vla) +obj-$(CONFIG_KASAN_KUNIT_TEST) += kasan_kunit.o +CFLAGS_kasan_kunit.o += -fno-builtin +CFLAGS_kasan_kunit.o += $(call cc-disable-warning, vla) obj-$(CONFIG_TEST_KASAN_MODULE) += test_kasan_module.o obj-$(CONFIG_TEST_UBSAN) += test_ubsan.o CFLAGS_test_ubsan.o += $(call cc-disable-warning, vla) diff --git a/lib/test_kasan.c b/lib/kasan_kunit.c similarity index 100% rename from lib/test_kasan.c rename to lib/kasan_kunit.c -- 2.28.0.236.gb10cc79966-goog
[PATCH v12 6/6] mm: kasan: Do not panic if both panic_on_warn and kasan_multishot set
KASAN errors will currently trigger a panic when panic_on_warn is set. This renders kasan_multishot useless, as further KASAN errors won't be reported if the kernel has already paniced. By making kasan_multishot disable this behaviour for KASAN errors, we can still have the benefits of panic_on_warn for non-KASAN warnings, yet be able to use kasan_multishot. This is particularly important when running KASAN tests, which need to trigger multiple KASAN errors: previously these would panic the system if panic_on_warn was set, now they can run (and will panic the system should non-KASAN warnings show up). Signed-off-by: David Gow Reviewed-by: Andrey Konovalov Reviewed-by: Brendan Higgins Tested-by: Andrey Konovalov --- mm/kasan/report.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm/kasan/report.c b/mm/kasan/report.c index e2c14b10bc81..00a53f1355ae 100644 --- a/mm/kasan/report.c +++ b/mm/kasan/report.c @@ -95,7 +95,7 @@ static void end_report(unsigned long *flags) pr_err("==\n"); add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE); spin_unlock_irqrestore(&report_lock, *flags); - if (panic_on_warn) { + if (panic_on_warn && !test_bit(KASAN_BIT_MULTI_SHOT, &kasan_flags)) { /* * This thread may hit another WARN() in the panic path. * Resetting this prevents additional WARN() from panicking the -- 2.28.0.236.gb10cc79966-goog
[PATCH v12 5/6] KASAN: Testing Documentation
From: Patricia Alfonso Include documentation on how to test KASAN using CONFIG_TEST_KASAN_KUNIT and CONFIG_TEST_KASAN_MODULE. Signed-off-by: Patricia Alfonso Signed-off-by: David Gow Reviewed-by: Andrey Konovalov Reviewed-by: Dmitry Vyukov Acked-by: Brendan Higgins Tested-by: Andrey Konovalov --- Documentation/dev-tools/kasan.rst | 70 +++ 1 file changed, 70 insertions(+) diff --git a/Documentation/dev-tools/kasan.rst b/Documentation/dev-tools/kasan.rst index 38fd5681fade..42991e40cbe1 100644 --- a/Documentation/dev-tools/kasan.rst +++ b/Documentation/dev-tools/kasan.rst @@ -281,3 +281,73 @@ unmapped. This will require changes in arch-specific code. This allows ``VMAP_STACK`` support on x86, and can simplify support of architectures that do not have a fixed module region. + +CONFIG_KASAN_KUNIT_TEST & CONFIG_TEST_KASAN_MODULE +-- + +``CONFIG_KASAN_KUNIT_TEST`` utilizes the KUnit Test Framework for testing. +This means each test focuses on a small unit of functionality and +there are a few ways these tests can be run. + +Each test will print the KASAN report if an error is detected and then +print the number of the test and the status of the test: + +pass:: + +ok 28 - kmalloc_double_kzfree +or, if kmalloc failed:: + +# kmalloc_large_oob_right: ASSERTION FAILED at lib/test_kasan.c:163 +Expected ptr is not null, but is +not ok 4 - kmalloc_large_oob_right +or, if a KASAN report was expected, but not found:: + +# kmalloc_double_kzfree: EXPECTATION FAILED at lib/test_kasan.c:629 +Expected kasan_data->report_expected == kasan_data->report_found, but +kasan_data->report_expected == 1 +kasan_data->report_found == 0 +not ok 28 - kmalloc_double_kzfree + +All test statuses are tracked as they run and an overall status will +be printed at the end:: + +ok 1 - kasan_kunit_test + +or:: + +not ok 1 - kasan_kunit_test + +(1) Loadable Module + + +With ``CONFIG_KUNIT`` enabled, ``CONFIG_KASAN_KUNIT_TEST`` can be built as +a loadable module and run on any architecture that supports KASAN +using something like insmod or modprobe. + +(2) Built-In +~ + +With ``CONFIG_KUNIT`` built-in, ``CONFIG_KASAN_KUNIT_TEST`` can be built-in +on any architecure that supports KASAN. These and any other KUnit +tests enabled will run and print the results at boot as a late-init +call. + +(3) Using kunit_tool +~ + +With ``CONFIG_KUNIT`` and ``CONFIG_KASAN_KUNIT_TEST`` built-in, we can also +use kunit_tool to see the results of these along with other KUnit +tests in a more readable way. This will not print the KASAN reports +of tests that passed. Use `KUnit documentation <https://www.kernel.org/doc/html/latest/dev-tools/kunit/index.html>`_ for more up-to-date +information on kunit_tool. + +.. _KUnit: https://www.kernel.org/doc/html/latest/dev-tools/kunit/index.html + +``CONFIG_TEST_KASAN_MODULE`` is a set of KASAN tests that could not be +converted to KUnit. These tests can be run only as a module with +``CONFIG_TEST_KASAN_MODULE`` built as a loadable module and +``CONFIG_KASAN`` built-in. The type of error expected and the +function being run is printed before the expression expected to give +an error. Then the error is printed, if found, and that test +should be interpretted to pass only if the error was the one expected +by the test. -- 2.28.0.236.gb10cc79966-goog
[PATCH v10 0/5] KASAN-KUnit Integration
This patchset contains everything needed to integrate KASAN and KUnit. KUnit will be able to: (1) Fail tests when an unexpected KASAN error occurs (2) Pass tests when an expected KASAN error occurs Convert KASAN tests to KUnit with the exception of copy_user_test because KUnit is unable to test those. Add documentation on how to run the KASAN tests with KUnit and what to expect when running these tests. This patchset depends on: - "kunit: extend kunit resources API" [1] - This is already present in the kselftest/kunit branch I'd _really_ like to get this into 5.9 if possible: we also have some other changes which depend on some things here. Changes from v9: - Rebased on top of linux-next (20200731) + kselftest/kunit and [7] - Note that the kasan_rcu_uaf test has not been ported to KUnit, and remains in test_kasan_module. This is because: (a) KUnit's expect failure will not check if the RCU stacktraces show. (b) KUnit is unable to link the failure to the test, as it occurs in an RCU callback. Changes from v8: - Rebased on top of kselftest/kunit - (Which, with this patchset, should rebase cleanly on 5.8-rc7) - Renamed the KUnit test suite, config name to patch the proposed naming guidelines for KUnit tests[6] Changes from v7: - Rebased on top of kselftest/kunit - Rebased on top of v4 of the kunit resources API[1] - Rebased on top of v4 of the FORTIFY_SOURCE fix[2,3,4] - Updated the Kconfig entry to support KUNIT_ALL_TESTS Changes from v6: - Rebased on top of kselftest/kunit - Rebased on top of Daniel Axtens' fix for FORTIFY_SOURCE incompatibilites [2] - Removed a redundant report_enabled() check. - Fixed some places with out of date Kconfig names in the documentation. Changes from v5: - Split out the panic_on_warn changes to a separate patch. - Fix documentation to fewer to the new Kconfig names. - Fix some changes which were in the wrong patch. - Rebase on top of kselftest/kunit (currently identical to 5.7-rc1) Changes from v4: - KASAN no longer will panic on errors if both panic_on_warn and kasan_multishot are enabled. - As a result, the KASAN tests will no-longer disable panic_on_warn. - This also means panic_on_warn no-longer needs to be exported. - The use of temporary "kasan_data" variables has been cleaned up somewhat. - A potential refcount/resource leak should multiple KASAN errors appear during an assertion was fixed. - Some wording changes to the KASAN test Kconfig entries. Changes from v3: - KUNIT_SET_KASAN_DATA and KUNIT_DO_EXPECT_KASAN_FAIL have been combined and included in KUNIT_DO_EXPECT_KASAN_FAIL() instead. - Reordered logic in kasan_update_kunit_status() in report.c to be easier to read. - Added comment to not use the name "kasan_data" for any kunit tests outside of KUNIT_EXPECT_KASAN_FAIL(). Changes since v2: - Due to Alan's changes in [1], KUnit can be built as a module. - The name of the tests that could not be run with KUnit has been changed to be more generic: test_kasan_module. - Documentation on how to run the new KASAN tests and what to expect when running them has been added. - Some variables and functions are now static. - Now save/restore panic_on_warn in a similar way to kasan_multi_shot and renamed the init/exit functions to be more generic to accommodate. - Due to [4] in kasan_strings, kasan_memchr, and kasan_memcmp will fail if CONFIG_AMD_MEM_ENCRYPT is enabled so return early and print message explaining this circumstance. - Changed preprocessor checks to C checks where applicable. Changes since v1: - Make use of Alan Maguire's suggestion to use his patch that allows static resources for integration instead of adding a new attribute to the kunit struct - All KUNIT_EXPECT_KASAN_FAIL statements are local to each test - The definition of KUNIT_EXPECT_KASAN_FAIL is local to the test_kasan.c file since it seems this is the only place this will be used. - Integration relies on KUnit being builtin - copy_user_test has been separated into its own file since KUnit is unable to test these. This can be run as a module just as before, using CONFIG_TEST_KASAN_USER - The addition to the current task has been separated into its own patch as this is a significant enough change to be on its own. [1] https://lore.kernel.org/linux-kselftest/cafd5g46uu_5tg89uom0dj5cmq+11cwjbnsd-k_cvy6bqueu...@mail.gmail.com/T/#t [2] https://lore.kernel.org/linux-mm/20200424145521.8203-1-...@axtens.net/ [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=adb72ae1915db28f934e9e02c18bfcea2f3ed3b7 [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=47227d27e2fcb01a9e8f5958d8997cf47a820afc [5] https://bugzilla.kernel.org/show_bug.cgi?id=206337 [6] https://lore.kernel.org/linux-kselftest/20200620054944.167330-1-david...@google.com/ [7] https://lkml.org/lkml/2020/7/31
[PATCH v10 2/5] KUnit: KASAN Integration
From: Patricia Alfonso Integrate KASAN into KUnit testing framework. - Fail tests when KASAN reports an error that is not expected - Use KUNIT_EXPECT_KASAN_FAIL to expect a KASAN error in KASAN tests - Expected KASAN reports pass tests and are still printed when run without kunit_tool (kunit_tool still bypasses the report due to the test passing) - KUnit struct in current task used to keep track of the current test from KASAN code Make use of "[PATCH v3 kunit-next 1/2] kunit: generalize kunit_resource API beyond allocated resources" and "[PATCH v3 kunit-next 2/2] kunit: add support for named resources" from Alan Maguire [1] - A named resource is added to a test when a KASAN report is expected - This resource contains a struct for kasan_data containing booleans representing if a KASAN report is expected and if a KASAN report is found [1] (https://lore.kernel.org/linux-kselftest/1583251361-12748-1-git-send-email-alan.magu...@oracle.com/T/#t) Signed-off-by: Patricia Alfonso Signed-off-by: David Gow Reviewed-by: Andrey Konovalov Reviewed-by: Dmitry Vyukov Acked-by: Brendan Higgins --- include/kunit/test.h | 5 + include/linux/kasan.h | 6 ++ lib/kunit/test.c | 13 +++- lib/test_kasan.c | 47 +-- mm/kasan/report.c | 32 + 5 files changed, 96 insertions(+), 7 deletions(-) diff --git a/include/kunit/test.h b/include/kunit/test.h index 59f3144f009a..3391f38389f8 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -224,6 +224,11 @@ struct kunit { struct list_head resources; /* Protected by lock. */ }; +static inline void kunit_set_failure(struct kunit *test) +{ + WRITE_ONCE(test->success, false); +} + void kunit_init_test(struct kunit *test, const char *name, char *log); int kunit_run_tests(struct kunit_suite *suite); diff --git a/include/linux/kasan.h b/include/linux/kasan.h index 087fba34b209..30d343b4a40a 100644 --- a/include/linux/kasan.h +++ b/include/linux/kasan.h @@ -14,6 +14,12 @@ struct task_struct; #include #include +/* kasan_data struct is used in KUnit tests for KASAN expected failures */ +struct kunit_kasan_expectation { + bool report_expected; + bool report_found; +}; + extern unsigned char kasan_early_shadow_page[PAGE_SIZE]; extern pte_t kasan_early_shadow_pte[PTRS_PER_PTE]; extern pmd_t kasan_early_shadow_pmd[PTRS_PER_PMD]; diff --git a/lib/kunit/test.c b/lib/kunit/test.c index c36037200310..dcc35fd30d95 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -10,16 +10,12 @@ #include #include #include +#include #include "debugfs.h" #include "string-stream.h" #include "try-catch-impl.h" -static void kunit_set_failure(struct kunit *test) -{ - WRITE_ONCE(test->success, false); -} - static void kunit_print_tap_version(void) { static bool kunit_has_printed_tap_version; @@ -288,6 +284,10 @@ static void kunit_try_run_case(void *data) struct kunit_suite *suite = ctx->suite; struct kunit_case *test_case = ctx->test_case; +#if (IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)) + current->kunit_test = test; +#endif /* IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT) */ + /* * kunit_run_case_internal may encounter a fatal error; if it does, * abort will be called, this thread will exit, and finally the parent @@ -602,6 +602,9 @@ void kunit_cleanup(struct kunit *test) spin_unlock(&test->lock); kunit_remove_resource(test, res); } +#if (IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)) + current->kunit_test = NULL; +#endif /* IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)*/ } EXPORT_SYMBOL_GPL(kunit_cleanup); diff --git a/lib/test_kasan.c b/lib/test_kasan.c index 53e953bb1d1d..58bffadd8367 100644 --- a/lib/test_kasan.c +++ b/lib/test_kasan.c @@ -23,6 +23,8 @@ #include +#include + #include "../mm/kasan/kasan.h" #define OOB_TAG_OFF (IS_ENABLED(CONFIG_KASAN_GENERIC) ? 0 : KASAN_SHADOW_SCALE_SIZE) @@ -32,14 +34,55 @@ * are not eliminated as dead code. */ -int kasan_int_result; void *kasan_ptr_result; +int kasan_int_result; + +static struct kunit_resource resource; +static struct kunit_kasan_expectation fail_data; +static bool multishot; + +static int kasan_test_init(struct kunit *test) +{ + /* +* Temporarily enable multi-shot mode and set panic_on_warn=0. +* Otherwise, we'd only get a report for the first case. +*/ + multishot = kasan_save_enable_multi_shot(); + + return 0; +} + +static void kasan_test_exit(struct kunit *test) +{ + kasan_restore_multi_shot(multishot); +} + +/** + * KUNIT_EXPECT_KASAN_FAIL() - Cau
[PATCH v10 1/5] Add KUnit Struct to Current Task
From: Patricia Alfonso In order to integrate debugging tools like KASAN into the KUnit framework, add KUnit struct to the current task to keep track of the current KUnit test. Signed-off-by: Patricia Alfonso Reviewed-by: Brendan Higgins Signed-off-by: David Gow --- include/linux/sched.h | 4 1 file changed, 4 insertions(+) diff --git a/include/linux/sched.h b/include/linux/sched.h index 27882a08163f..f3f990b82bde 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1196,6 +1196,10 @@ struct task_struct { struct kcsan_ctxkcsan_ctx; #endif +#if IS_ENABLED(CONFIG_KUNIT) + struct kunit*kunit_test; +#endif + #ifdef CONFIG_FUNCTION_GRAPH_TRACER /* Index of current stored address in ret_stack: */ int curr_ret_stack; -- 2.28.0.163.g6104cc2f0b6-goog
[PATCH v10 3/5] KASAN: Port KASAN Tests to KUnit
From: Patricia Alfonso Transfer all previous tests for KASAN to KUnit so they can be run more easily. Using kunit_tool, developers can run these tests with their other KUnit tests and see "pass" or "fail" with the appropriate KASAN report instead of needing to parse each KASAN report to test KASAN functionalities. All KASAN reports are still printed to dmesg. Stack tests do not work properly when KASAN_STACK is enabled so those tests use a check for "if IS_ENABLED(CONFIG_KASAN_STACK)" so they only run if stack instrumentation is enabled. If KASAN_STACK is not enabled, KUnit will print a statement to let the user know this test was not run with KASAN_STACK enabled. copy_user_test and kasan_rcu_uaf cannot be run in KUnit so there is a separate test file for those tests, which can be run as before as a module. Signed-off-by: Patricia Alfonso Signed-off-by: David Gow Reviewed-by: Brendan Higgins Reviewed-by: Andrey Konovalov Reviewed-by: Dmitry Vyukov --- lib/Kconfig.kasan | 22 +- lib/Makefile| 7 +- lib/kasan_kunit.c | 770 lib/test_kasan.c| 946 lib/test_kasan_module.c | 111 + 5 files changed, 902 insertions(+), 954 deletions(-) create mode 100644 lib/kasan_kunit.c delete mode 100644 lib/test_kasan.c create mode 100644 lib/test_kasan_module.c diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan index 047b53dbfd58..9a237887e52e 100644 --- a/lib/Kconfig.kasan +++ b/lib/Kconfig.kasan @@ -167,12 +167,24 @@ config KASAN_VMALLOC for KASAN to detect more sorts of errors (and to support vmapped stacks), but at the cost of higher memory usage. -config TEST_KASAN - tristate "Module for testing KASAN for bug detection" - depends on m +config KASAN_KUNIT_TEST + tristate "KUnit-compatible tests of KASAN bug detection capabilities" if !KUNIT_ALL_TESTS + depends on KASAN && KUNIT + default KUNIT_ALL_TESTS help - This is a test module doing various nasty things like - out of bounds accesses, use after free. It is useful for testing + This is a KUnit test suite doing various nasty things like + out of bounds and use after free accesses. It is useful for testing kernel debugging features like KASAN. + For more information on KUnit and unit tests in general, please refer + to the KUnit documentation in Documentation/dev-tools/kunit + +config TEST_KASAN_MODULE + tristate "KUnit-incompatible tests of KASAN bug detection capabilities" + depends on m && KASAN + help + This is a part of the KASAN test suite that is incompatible with + KUnit. Currently includes tests that do bad copy_from/to_user + accesses. + endif # KASAN diff --git a/lib/Makefile b/lib/Makefile index 46278be53cda..8a530bf7078c 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -60,9 +60,10 @@ CFLAGS_test_bitops.o += -Werror obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o obj-$(CONFIG_TEST_HASH) += test_hash.o test_siphash.o obj-$(CONFIG_TEST_IDA) += test_ida.o -obj-$(CONFIG_TEST_KASAN) += test_kasan.o -CFLAGS_test_kasan.o += -fno-builtin -CFLAGS_test_kasan.o += $(call cc-disable-warning, vla) +obj-$(CONFIG_KASAN_KUNIT_TEST) += kasan_kunit.o +CFLAGS_kasan_kunit.o += -fno-builtin +CFLAGS_kasan_kunit.o += $(call cc-disable-warning, vla) +obj-$(CONFIG_TEST_KASAN_MODULE) += test_kasan_module.o obj-$(CONFIG_TEST_UBSAN) += test_ubsan.o CFLAGS_test_ubsan.o += $(call cc-disable-warning, vla) UBSAN_SANITIZE_test_ubsan.o := y diff --git a/lib/kasan_kunit.c b/lib/kasan_kunit.c new file mode 100644 index ..8a01009d91e4 --- /dev/null +++ b/lib/kasan_kunit.c @@ -0,0 +1,770 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * + * Copyright (c) 2014 Samsung Electronics Co., Ltd. + * Author: Andrey Ryabinin + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#include + +#include "../mm/kasan/kasan.h" + +#define OOB_TAG_OFF (IS_ENABLED(CONFIG_KASAN_GENERIC) ? 0 : KASAN_SHADOW_SCALE_SIZE) + +/* + * We assign some test results to these globals to make sure the tests + * are not eliminated as dead code. + */ + +void *kasan_ptr_result; +int kasan_int_result; + +static struct kunit_resource resource; +static struct kunit_kasan_expectation fail_data; +static bool multishot; + +static int kasan_test_init(struct kunit *test) +{ + /* +* Temporarily enable multi-shot mode and set panic_on_warn=0. +* Otherwise, we'd only get a report for the first case. +*/ + multishot = kasan_save_enable_multi_shot(); + + return 0; +} + +static void kasan_test_exit(struct kunit *test) +{ + kasan_restore_multi_shot(multishot); +} + +/** + * KUNIT_EXPECT_KASA
[PATCH v10 4/5] KASAN: Testing Documentation
From: Patricia Alfonso Include documentation on how to test KASAN using CONFIG_TEST_KASAN_KUNIT and CONFIG_TEST_KASAN_MODULE. Signed-off-by: Patricia Alfonso Signed-off-by: David Gow Reviewed-by: Andrey Konovalov Reviewed-by: Dmitry Vyukov Acked-by: Brendan Higgins --- Documentation/dev-tools/kasan.rst | 70 +++ 1 file changed, 70 insertions(+) diff --git a/Documentation/dev-tools/kasan.rst b/Documentation/dev-tools/kasan.rst index 38fd5681fade..42991e40cbe1 100644 --- a/Documentation/dev-tools/kasan.rst +++ b/Documentation/dev-tools/kasan.rst @@ -281,3 +281,73 @@ unmapped. This will require changes in arch-specific code. This allows ``VMAP_STACK`` support on x86, and can simplify support of architectures that do not have a fixed module region. + +CONFIG_KASAN_KUNIT_TEST & CONFIG_TEST_KASAN_MODULE +-- + +``CONFIG_KASAN_KUNIT_TEST`` utilizes the KUnit Test Framework for testing. +This means each test focuses on a small unit of functionality and +there are a few ways these tests can be run. + +Each test will print the KASAN report if an error is detected and then +print the number of the test and the status of the test: + +pass:: + +ok 28 - kmalloc_double_kzfree +or, if kmalloc failed:: + +# kmalloc_large_oob_right: ASSERTION FAILED at lib/test_kasan.c:163 +Expected ptr is not null, but is +not ok 4 - kmalloc_large_oob_right +or, if a KASAN report was expected, but not found:: + +# kmalloc_double_kzfree: EXPECTATION FAILED at lib/test_kasan.c:629 +Expected kasan_data->report_expected == kasan_data->report_found, but +kasan_data->report_expected == 1 +kasan_data->report_found == 0 +not ok 28 - kmalloc_double_kzfree + +All test statuses are tracked as they run and an overall status will +be printed at the end:: + +ok 1 - kasan_kunit_test + +or:: + +not ok 1 - kasan_kunit_test + +(1) Loadable Module + + +With ``CONFIG_KUNIT`` enabled, ``CONFIG_KASAN_KUNIT_TEST`` can be built as +a loadable module and run on any architecture that supports KASAN +using something like insmod or modprobe. + +(2) Built-In +~ + +With ``CONFIG_KUNIT`` built-in, ``CONFIG_KASAN_KUNIT_TEST`` can be built-in +on any architecure that supports KASAN. These and any other KUnit +tests enabled will run and print the results at boot as a late-init +call. + +(3) Using kunit_tool +~ + +With ``CONFIG_KUNIT`` and ``CONFIG_KASAN_KUNIT_TEST`` built-in, we can also +use kunit_tool to see the results of these along with other KUnit +tests in a more readable way. This will not print the KASAN reports +of tests that passed. Use `KUnit documentation <https://www.kernel.org/doc/html/latest/dev-tools/kunit/index.html>`_ for more up-to-date +information on kunit_tool. + +.. _KUnit: https://www.kernel.org/doc/html/latest/dev-tools/kunit/index.html + +``CONFIG_TEST_KASAN_MODULE`` is a set of KASAN tests that could not be +converted to KUnit. These tests can be run only as a module with +``CONFIG_TEST_KASAN_MODULE`` built as a loadable module and +``CONFIG_KASAN`` built-in. The type of error expected and the +function being run is printed before the expression expected to give +an error. Then the error is printed, if found, and that test +should be interpretted to pass only if the error was the one expected +by the test. -- 2.28.0.163.g6104cc2f0b6-goog
[PATCH v10 5/5] mm: kasan: Do not panic if both panic_on_warn and kasan_multishot set
KASAN errors will currently trigger a panic when panic_on_warn is set. This renders kasan_multishot useless, as further KASAN errors won't be reported if the kernel has already paniced. By making kasan_multishot disable this behaviour for KASAN errors, we can still have the benefits of panic_on_warn for non-KASAN warnings, yet be able to use kasan_multishot. This is particularly important when running KASAN tests, which need to trigger multiple KASAN errors: previously these would panic the system if panic_on_warn was set, now they can run (and will panic the system should non-KASAN warnings show up). Signed-off-by: David Gow Reviewed-by: Andrey Konovalov Reviewed-by: Brendan Higgins --- mm/kasan/report.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm/kasan/report.c b/mm/kasan/report.c index e2c14b10bc81..00a53f1355ae 100644 --- a/mm/kasan/report.c +++ b/mm/kasan/report.c @@ -95,7 +95,7 @@ static void end_report(unsigned long *flags) pr_err("==\n"); add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE); spin_unlock_irqrestore(&report_lock, *flags); - if (panic_on_warn) { + if (panic_on_warn && !test_bit(KASAN_BIT_MULTI_SHOT, &kasan_flags)) { /* * This thread may hit another WARN() in the panic path. * Resetting this prevents additional WARN() from panicking the -- 2.28.0.163.g6104cc2f0b6-goog
Re: [PATCH v9 0/5] KASAN-KUnit Integration
On Fri, Jul 31, 2020 at 9:25 PM 'Andrey Konovalov' via kasan-dev wrote: > > On Fri, Jul 31, 2020 at 6:43 AM David Gow wrote: > > > > This patchset contains everything needed to integrate KASAN and KUnit. > > > > KUnit will be able to: > > (1) Fail tests when an unexpected KASAN error occurs > > (2) Pass tests when an expected KASAN error occurs > > > > Convert KASAN tests to KUnit with the exception of copy_user_test > > because KUnit is unable to test those. > > > > Add documentation on how to run the KASAN tests with KUnit and what to > > expect when running these tests. > > > > This patchset depends on: > > - "kunit: extend kunit resources API" [1] > > - This is already present in the kselftest/kunit branch > > > > I'd _really_ like to get this into 5.9 if possible: we also have some > > other changes which depend on some things here. > > Hi David, > > You'll need to rebase this on top of the mm tree, which currently > contains Walter's patch titled "kasan: fix KASAN unit tests for > tag-based KASAN". > > There's also another patch that touches KASAN tests in the series I've > just mailed titled "kasan: support stack instrumentation for tag-based > mode". > > Thanks! > I've rebased this on top of a linux-next (with the pending KUnit patches from kselftest/kunit and the "kasan: support stack instrumentation for tag-based mode" patchset applied): https://lore.kernel.org/linux-kselftest/20200801070924.1786166-1-david...@google.com/T/#u Note that the RCU test doesn't seem to be compatible with KUnit's KASAN integration at present: I'm no expert on RCU, but it looks like the current test context might not be propagated to the callback, so expecting the failure doesn't work. Given that KUnit also doesn't look for the aux stacks (just that a failure occurred), it seemed best to avoid trying to port that one to KUnit, so I've left it in the test_kasan_module.c file. It may be possible to port it at a later date. Note that I don't have an arm64 setup here, so I haven't actually tested the tag-based KASAN stuff yet. Cheers, -- David
Re: [PATCH] lib: Convert test_hexdump.c to KUnit
On Thu, Aug 6, 2020 at 5:53 PM Andy Shevchenko wrote: > > On Thu, Aug 6, 2020 at 12:48 PM Arpitha Raghunandan <98.a...@gmail.com> wrote: > > > > Converts test lib/test_hexdump.c to KUnit. > > More information about KUnit can be found at > > https://www.kernel.org/doc/html/latest/dev-tools/kunit/index.html. > > KUnit provides a common framework for unit tests in the kernel. > > > -config TEST_HEXDUMP > > - tristate "Test functions located in the hexdump module at runtime" > > We have a nice collection of tests starting with TEST_ in the > configuration, now it's gone. > I'm strongly against this change. > Code itself okay, but without addressing above - NAK. > This change is to make the test naming compliant with the proposed KUnit test naming guidelines: - https://lore.kernel.org/linux-kselftest/20200702071416.1780522-1-david...@google.com/ The hope is that tests built on KUnit will all end up with the same [x]_KUNIT_TEST config options (which at least preserves the consistency of the test naming, even if they'll not all sort together), and should make it easier for people to know that the test results will be in a common format, and that the test can also be run using the KUnit tools. The naming guidelines haven't been upstreamed yet, though, so we'd definitely appreciate input on that thread if you've got comments more broadly than for this particular patch. Ultimately, I don't think it matters too much what we end up using, but having some consistency is the goal.
Re: [PATCH v11 0/6] KASAN-KUnit Integration
On Fri, Aug 7, 2020 at 9:15 PM 'Andrey Konovalov' via kasan-dev wrote: > > On Wed, Aug 5, 2020 at 6:29 AM David Gow wrote: > > > > This patchset contains everything needed to integrate KASAN and KUnit. > > > > KUnit will be able to: > > (1) Fail tests when an unexpected KASAN error occurs > > (2) Pass tests when an expected KASAN error occurs > > > > Convert KASAN tests to KUnit with the exception of copy_user_test > > because KUnit is unable to test those. > > > > Add documentation on how to run the KASAN tests with KUnit and what to > > expect when running these tests. > > > > This patchset depends on: > > - "kunit: extend kunit resources API" [1] > > - This is included in the KUnit 5.9-rci pull request[8] > > > > I'd _really_ like to get this into 5.9 if possible: we also have some > > other changes which depend on some things here. > > Found a small issue in patch #3, but otherwise: > > Tested-by: Andrey Konovalov > > for the series. Cheers! The issue in #3 looks to be a rebase issue: I'll send a fixed version out soon. > > The patches apply cleanly on top of the latest linux-next/akpm branch. > > There are some tests that fail for tag-based mode, but those are > unrelated to this series, and require KASAN improvements. > Do you think it's worth disabling these tests if tag-based mode is disabled? Personally, I'm leaning "no", but if the planned support for explicitly skipping tests existed, this could be a good case for it: a test which is expected to fail due to a feature not existing in the current config. Thanks, -- David > > > > Changes from v10: > > - Fixed some whitespace issues in patch 2. > > - Split out the renaming of the KUnit test suite into a separate patch. > > > > Changes from v9: > > - Rebased on top of linux-next (20200731) + kselftest/kunit and [7] > > - Note that the kasan_rcu_uaf test has not been ported to KUnit, and > >remains in test_kasan_module. This is because: > >(a) KUnit's expect failure will not check if the RCU stacktraces > >show. > >(b) KUnit is unable to link the failure to the test, as it occurs in > >an RCU callback. > > > > Changes from v8: > > - Rebased on top of kselftest/kunit > > - (Which, with this patchset, should rebase cleanly on 5.8-rc7) > > - Renamed the KUnit test suite, config name to patch the proposed > >naming guidelines for KUnit tests[6] > > > > Changes from v7: > > - Rebased on top of kselftest/kunit > > - Rebased on top of v4 of the kunit resources API[1] > > - Rebased on top of v4 of the FORTIFY_SOURCE fix[2,3,4] > > - Updated the Kconfig entry to support KUNIT_ALL_TESTS > > > > Changes from v6: > > - Rebased on top of kselftest/kunit > > - Rebased on top of Daniel Axtens' fix for FORTIFY_SOURCE > >incompatibilites [2] > > - Removed a redundant report_enabled() check. > > - Fixed some places with out of date Kconfig names in the > >documentation. > > > > Changes from v5: > > - Split out the panic_on_warn changes to a separate patch. > > - Fix documentation to fewer to the new Kconfig names. > > - Fix some changes which were in the wrong patch. > > - Rebase on top of kselftest/kunit (currently identical to 5.7-rc1) > > > > Changes from v4: > > - KASAN no longer will panic on errors if both panic_on_warn and > >kasan_multishot are enabled. > > - As a result, the KASAN tests will no-longer disable panic_on_warn. > > - This also means panic_on_warn no-longer needs to be exported. > > - The use of temporary "kasan_data" variables has been cleaned up > >somewhat. > > - A potential refcount/resource leak should multiple KASAN errors > >appear during an assertion was fixed. > > - Some wording changes to the KASAN test Kconfig entries. > > > > Changes from v3: > > - KUNIT_SET_KASAN_DATA and KUNIT_DO_EXPECT_KASAN_FAIL have been > > combined and included in KUNIT_DO_EXPECT_KASAN_FAIL() instead. > > - Reordered logic in kasan_update_kunit_status() in report.c to be > > easier to read. > > - Added comment to not use the name "kasan_data" for any kunit tests > > outside of KUNIT_EXPECT_KASAN_FAIL(). > > > > Changes since v2: > > - Due to Alan's changes in [1], KUnit can be built as a module. > > - The name of the tests that could not be run with KUnit has been > > changed to be more generic: test_kasan_module. > > - Documentation on how to run the new
Re: [PATCH v1 1/2] kunit: tool: fix running kunit_tool from outside kernel tree
On Sat, Aug 8, 2020 at 9:17 AM Brendan Higgins wrote: > > Currently kunit_tool does not work correctly when executed from a path > outside of the kernel tree, so make sure that the current working > directory is correct and the kunit_dir is properly initialized before > running. > > Signed-off-by: Brendan Higgins > --- > tools/testing/kunit/kunit.py | 8 > 1 file changed, 8 insertions(+) > > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py > index 425ef40067e7..96344a11ff1f 100755 > --- a/tools/testing/kunit/kunit.py > +++ b/tools/testing/kunit/kunit.py > @@ -237,9 +237,14 @@ def main(argv, linux=None): > > cli_args = parser.parse_args(argv) > > + if get_kernel_root_path(): > + print('cd ' + get_kernel_root_path()) Do we want to print this, or is it a leftover debug statement? > + os.chdir(get_kernel_root_path()) > + > if cli_args.subcommand == 'run': > if not os.path.exists(cli_args.build_dir): > os.mkdir(cli_args.build_dir) > + create_default_kunitconfig() Why are we adding this everywhere when it's already in config_tests, which should already be called in all of the places where a kunitconfig is required? Is the goal to always copy the default kunitconfig when creating a new build_dir? While I can sort-of see why we might want to do that, if the build dir doesn't exist, most of the subcommands will fail anyway (maybe we should only create the build-dir for 'config' and 'run'?) > > if not linux: > linux = kunit_kernel.LinuxSourceTree() > @@ -257,6 +262,7 @@ def main(argv, linux=None): > if cli_args.build_dir: > if not os.path.exists(cli_args.build_dir): > os.mkdir(cli_args.build_dir) > + create_default_kunitconfig() > > if not linux: > linux = kunit_kernel.LinuxSourceTree() > @@ -273,6 +279,7 @@ def main(argv, linux=None): > if cli_args.build_dir: > if not os.path.exists(cli_args.build_dir): > os.mkdir(cli_args.build_dir) > + create_default_kunitconfig() > > if not linux: > linux = kunit_kernel.LinuxSourceTree() > @@ -291,6 +298,7 @@ def main(argv, linux=None): > if cli_args.build_dir: > if not os.path.exists(cli_args.build_dir): > os.mkdir(cli_args.build_dir) > + create_default_kunitconfig() > > if not linux: > linux = kunit_kernel.LinuxSourceTree() > > base-commit: 30185b69a2d533c4ba6ca926b8390ce7de495e29 > -- > 2.28.0.236.gb10cc79966-goog >
[PATCH] kunit: Print test statistics on failure
When a number of tests fail, it can be useful to get higher-level statistics of how many tests are failing (or how many parameters are failing in parameterised tests), and in what cases or suites. This is already done by some non-KUnit tests, so add support for automatically generating these for KUnit tests. This change adds a 'kunit_stats_enabled' switch which has three values: - 0: No stats are printed (current behaviour) - 1: Stats are printed only for tests/suites with more than one subtests, and at least one failure (new default) - 2: Always print test statistics For parameterised tests, the summary line looks as follows: "# inode_test_xtimestamp_decoding: 0 / 16 test parameters failed" For test suites, it looks like this: "# ext4_inode_test: (0 / 1) tests failed (0 / 16 test parameters)" kunit_tool is also updated to correctly ignore diagnostic lines, so that these statistics do not prevent the result from parsing. Signed-off-by: David Gow --- This is largely a follow-up to the discussion here: https://lore.kernel.org/linux-kselftest/CABVgOSmy4n_LGwDS7yWfoLftcQzxv6S+iXx9Y=opcgg2gu0...@mail.gmail.com/T/#t Does this seem like a sensible addition? Cheers, -- David lib/kunit/test.c| 71 + tools/testing/kunit/kunit_parser.py | 2 +- 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/lib/kunit/test.c b/lib/kunit/test.c index ec9494e914ef..711e269366a7 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -16,6 +17,40 @@ #include "string-stream.h" #include "try-catch-impl.h" +/* + * KUnit statistic mode: + * 0 - disabled + * 1 - only when there is at least one failure, and more than one subtest + * 2 - enabled + */ +static int kunit_stats_enabled = 1; +core_param(kunit_stats_enabled, kunit_stats_enabled, int, 0644); + +static bool kunit_should_print_stats(int num_failures, int num_subtests) +{ + if (kunit_stats_enabled == 0) + return false; + + if (kunit_stats_enabled == 2) + return true; + + return (num_failures > 0 && num_subtests > 1); +} + +static void kunit_print_test_stats(struct kunit *test, + size_t num_failures, size_t num_subtests) +{ + if (!kunit_should_print_stats(num_failures, num_subtests)) + return; + + kunit_log(KERN_INFO, test, + KUNIT_SUBTEST_INDENT + "# %s: %lu / %lu test parameters failed", + test->name, + num_failures, + num_subtests); +} + /* * Append formatted message to log, size of which is limited to * KUNIT_LOG_SIZE bytes (including null terminating byte). @@ -346,15 +381,37 @@ static void kunit_run_case_catch_errors(struct kunit_suite *suite, test_case->success = test->success; } +static void kunit_print_suite_stats(struct kunit_suite *suite, + size_t num_failures, + size_t total_param_failures, + size_t total_params) +{ + size_t num_cases = kunit_suite_num_test_cases(suite); + + if (!kunit_should_print_stats(num_failures, num_cases)) + return; + + kunit_log(KERN_INFO, suite, + "# %s: (%lu / %lu) tests failed (%lu / %lu test parameters)", + suite->name, + num_failures, + num_cases, + total_param_failures, + total_params); +} + int kunit_run_tests(struct kunit_suite *suite) { char param_desc[KUNIT_PARAM_DESC_SIZE]; struct kunit_case *test_case; + size_t num_suite_failures = 0; + size_t total_param_failures = 0, total_params = 0; kunit_print_subtest_start(suite); kunit_suite_for_each_test_case(suite, test_case) { struct kunit test = { .param_value = NULL, .param_index = 0 }; + size_t num_params = 0, num_failures = 0; bool test_success = true; if (test_case->generate_params) { @@ -385,13 +442,27 @@ int kunit_run_tests(struct kunit_suite *suite) test.param_value = test_case->generate_params(test.param_value, param_desc); test.param_index++; } + + if (!test.success) + num_failures++; + num_params++; + } while (test.param_value); + kunit_print_test_stats(&test, num_failures, num_params); + kunit_print_ok_not_ok(&test, true, test_success, kunit_test_case_num(suite, test_case),
[PATCH] kunit: tool: Fix spelling of "diagnostic" in kunit_parser
Various helper functions were misspelling "diagnostic" in their names. It finally got annoying, so fix it. Signed-off-by: David Gow --- tools/testing/kunit/kunit_parser.py | 24 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py index 6614ec4d0898..1a1e1d13f1d3 100644 --- a/tools/testing/kunit/kunit_parser.py +++ b/tools/testing/kunit/kunit_parser.py @@ -97,11 +97,11 @@ def print_log(log): TAP_ENTRIES = re.compile(r'^(TAP|[\s]*ok|[\s]*not ok|[\s]*[0-9]+\.\.[0-9]+|[\s]*#).*$') -def consume_non_diagnositic(lines: List[str]) -> None: +def consume_non_diagnostic(lines: List[str]) -> None: while lines and not TAP_ENTRIES.match(lines[0]): lines.pop(0) -def save_non_diagnositic(lines: List[str], test_case: TestCase) -> None: +def save_non_diagnostic(lines: List[str], test_case: TestCase) -> None: while lines and not TAP_ENTRIES.match(lines[0]): test_case.log.append(lines[0]) lines.pop(0) @@ -113,7 +113,7 @@ OK_NOT_OK_SUBTEST = re.compile(r'^[\s]+(ok|not ok) [0-9]+ - (.*)$') OK_NOT_OK_MODULE = re.compile(r'^(ok|not ok) ([0-9]+) - (.*)$') def parse_ok_not_ok_test_case(lines: List[str], test_case: TestCase) -> bool: - save_non_diagnositic(lines, test_case) + save_non_diagnostic(lines, test_case) if not lines: test_case.status = TestStatus.TEST_CRASHED return True @@ -139,7 +139,7 @@ SUBTEST_DIAGNOSTIC = re.compile(r'^[\s]+# (.*)$') DIAGNOSTIC_CRASH_MESSAGE = re.compile(r'^[\s]+# .*?: kunit test case crashed!$') def parse_diagnostic(lines: List[str], test_case: TestCase) -> bool: - save_non_diagnositic(lines, test_case) + save_non_diagnostic(lines, test_case) if not lines: return False line = lines[0] @@ -155,7 +155,7 @@ def parse_diagnostic(lines: List[str], test_case: TestCase) -> bool: def parse_test_case(lines: List[str]) -> Optional[TestCase]: test_case = TestCase() - save_non_diagnositic(lines, test_case) + save_non_diagnostic(lines, test_case) while parse_diagnostic(lines, test_case): pass if parse_ok_not_ok_test_case(lines, test_case): @@ -166,7 +166,7 @@ def parse_test_case(lines: List[str]) -> Optional[TestCase]: SUBTEST_HEADER = re.compile(r'^[\s]+# Subtest: (.*)$') def parse_subtest_header(lines: List[str]) -> Optional[str]: - consume_non_diagnositic(lines) + consume_non_diagnostic(lines) if not lines: return None match = SUBTEST_HEADER.match(lines[0]) @@ -179,7 +179,7 @@ def parse_subtest_header(lines: List[str]) -> Optional[str]: SUBTEST_PLAN = re.compile(r'[\s]+[0-9]+\.\.([0-9]+)') def parse_subtest_plan(lines: List[str]) -> Optional[int]: - consume_non_diagnositic(lines) + consume_non_diagnostic(lines) match = SUBTEST_PLAN.match(lines[0]) if match: lines.pop(0) @@ -202,7 +202,7 @@ def max_status(left: TestStatus, right: TestStatus) -> TestStatus: def parse_ok_not_ok_test_suite(lines: List[str], test_suite: TestSuite, expected_suite_index: int) -> bool: - consume_non_diagnositic(lines) + consume_non_diagnostic(lines) if not lines: test_suite.status = TestStatus.TEST_CRASHED return False @@ -235,7 +235,7 @@ def bubble_up_test_case_errors(test_suite: TestSuite) -> TestStatus: def parse_test_suite(lines: List[str], expected_suite_index: int) -> Optional[TestSuite]: if not lines: return None - consume_non_diagnositic(lines) + consume_non_diagnostic(lines) test_suite = TestSuite() test_suite.status = TestStatus.SUCCESS name = parse_subtest_header(lines) @@ -264,7 +264,7 @@ def parse_test_suite(lines: List[str], expected_suite_index: int) -> Optional[Te TAP_HEADER = re.compile(r'^TAP version 14$') def parse_tap_header(lines: List[str]) -> bool: - consume_non_diagnositic(lines) + consume_non_diagnostic(lines) if TAP_HEADER.match(lines[0]): lines.pop(0) return True @@ -274,7 +274,7 @@ def parse_tap_header(lines: List[str]) -> bool: TEST_PLAN = re.compile(r'[0-9]+\.\.([0-9]+)') def parse_test_plan(lines: List[str]) -> Optional[int]: - consume_non_diagnositic(lines) + consume_non_diagnostic(lines) match = TEST_PLAN.match(lines[0]) if match: lines.pop(0) @@ -286,7 +286,7 @@ def bubble_up_suite_errors(test_suite_list: List[TestSuite]) -> TestStatus: return bubble_up_errors(lambda x: x.status, test_suite_list) def parse_tes
[PATCH] kunit: tool: Force the use of the 'tty' console for UML
kunit_tool relies on the UML console outputting printk() output to the tty in order to get results. Since the default console driver could change, pass 'console=tty' to the kernel. This is triggered by a change[1] to use ttynull as a fallback console driver which -- by chance or by design -- seems to have changed the default console output on UML, breaking kunit_tool. While this may be fixed, we should be less fragile to such changes in the default. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=757055ae8dedf5333af17b3b5b4b70ba9bc9da4e Signed-off-by: David Gow Fixes: 757055ae8ded ("init/console: Use ttynull as a fallback when there is no console") --- tools/testing/kunit/kunit_kernel.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index 57c1724b7e5d..698358c9c0d6 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -198,7 +198,7 @@ class LinuxSourceTree(object): return self.validate_config(build_dir) def run_kernel(self, args=[], build_dir='', timeout=None): - args.extend(['mem=1G']) + args.extend(['mem=1G', 'console=tty']) self._ops.linux_bin(args, timeout, build_dir) outfile = get_outfile_path(build_dir) subprocess.call(['stty', 'sane']) -- 2.29.2.729.g45daf8777d-goog
[PATCH] Documentation: kunit: Update kunit_tool page
The kunit_tool documentation page was pretty minimal, and a bit outdated. Update it and flesh it out a bit. In particular, - Mention that .kunitconfig is now in the build directory - Describe the use of --kunitconfig to specify a different config framgent - Mention the split functionality (i.e., commands other than 'run') - Describe --raw_output and kunit.py parse - Mention the globbing support - Provide a quick overview of other options, including --build_dir and --alltests Note that this does overlap a little with the new running_tips page. I don't think it's a problem having both: this page is supposed to be a bit more of a reference, rather than a list of useful tips, so the fact that they both describe the same features isn't a problem. Signed-off-by: David Gow --- Documentation/dev-tools/kunit/kunit-tool.rst | 132 ++- 1 file changed, 128 insertions(+), 4 deletions(-) diff --git a/Documentation/dev-tools/kunit/kunit-tool.rst b/Documentation/dev-tools/kunit/kunit-tool.rst index 29ae2fee8123..0b45affcd65c 100644 --- a/Documentation/dev-tools/kunit/kunit-tool.rst +++ b/Documentation/dev-tools/kunit/kunit-tool.rst @@ -22,14 +22,19 @@ not require any virtualization support: it is just a regular program. What is a .kunitconfig? === -It's just a defconfig that kunit_tool looks for in the base directory. +It's just a defconfig that kunit_tool looks for in the build directory. kunit_tool uses it to generate a .config as you might expect. In addition, it verifies that the generated .config contains the CONFIG options in the .kunitconfig; the reason it does this is so that it is easy to be sure that a CONFIG that enables a test actually ends up in the .config. -How do I use kunit_tool? - +It's also possible to pass a separate .kunitconfig fragment to kunit_tool, +which is useful if you have several different groups of tests you wish +to run independently, or if you want to use pre-defined test configs for +certain subsystems. + +Getting Started with kunit_tool +=== If a kunitconfig is present at the root directory, all you have to do is: @@ -48,10 +53,129 @@ However, you most likely want to use it with the following options: .. note:: This command will work even without a .kunitconfig file: if no -.kunitconfig is present, a default one will be used instead. + .kunitconfig is present, a default one will be used instead. + +If you wish to use a different .kunitconfig file (such as one provided for +testing a particular subsystem), you can pass it as an option. + +.. code-block:: bash + + ./tools/testing/kunit/kunit.py run --kunitconfig=fs/ext4/.kunitconfig For a list of all the flags supported by kunit_tool, you can run: .. code-block:: bash ./tools/testing/kunit/kunit.py run --help + +Configuring, Building, and Running Tests + + +It's also possible to run just parts of the KUnit build process independently, +which is useful if you want to make manual changes to part of the process. + +A .config can be generated from a .kunitconfig by using the ``config`` argument +when running kunit_tool: + +.. code-block:: bash + + ./tools/testing/kunit/kunit.py config + +Similarly, if you just want to build a KUnit kernel from the current .config, +you can use the ``build`` argument: + +.. code-block:: bash + + ./tools/testing/kunit/kunit.py build + +And, if you already have a built UML kernel with built-in KUnit tests, you can +run the kernel and display the test results with the ``exec`` argument: + +.. code-block:: bash + + ./tools/testing/kunit/kunit.py exec + +The ``run`` command which is discussed above is equivalent to running all three +of these in sequence. + +All of these commands accept a number of optional command-line arguments. The +``--help`` flag will give a complete list of these, or keep reading this page +for a guide to some of the more useful ones. + +Parsing Test Results + + +KUnit tests output their results in TAP (Test Anything Protocol) format. +kunit_tool will, when running tests, parse this output and print a summary +which is much more pleasant to read. If you wish to look at the raw test +results in TAP format, you can pass the ``--raw_output`` argument. + +.. code-block:: bash + + ./tools/testing/kunit/kunit.py run --raw_output + +.. note:: + The raw output from test runs may contain other, non-KUnit kernel log + lines. + +If you have KUnit results in their raw TAP format, you can parse them and print +the human-readable summary with the ``parse`` command for kunit_tool. This +accepts a filename for an argument, or will read from standard input. + +.. code-block:: bash + + # Reading from a file + ./tools/testing/kunit/kunit.py parse /var/log/dmesg + # Reading from stdin +
[PATCH v8] fat: Add KUnit tests for checksums and timestamps
Add some basic sanity-check tests for the fat_checksum() function and the fat_time_unix2fat() and fat_time_fat2unix() functions. These unit tests verify these functions return correct output for a number of test inputs. These tests were inspored by -- and serve a similar purpose to -- the timestamp parsing KUnit tests in ext4[1]. Note that, unlike fat_time_unix2fat, fat_time_fat2unix wasn't previously exported, so this patch exports it as well. This is required for the case where we're building the fat and fat_test as modules. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/inode-test.c Signed-off-by: David Gow Acked-by: OGAWA Hirofumi --- It's been a while, but this hopefully is a final version of the FAT KUnit patchset. It has a number of changes to keep it up-to-date with current KUnit standards, notably the use of parameterised tests and the addition of a '.kunitconfig' file to allow for easy testing. It also fixes an endianness tagging issue picked up by the kernel test robot under sparse on pa-risc. Cheers, -- David Changes since v7: https://lore.kernel.org/linux-kselftest/20201028064631.3774908-1-david...@google.com/ - Make the two timestamp tests parameterised: this means that the KUnit runtime and tooling are aware of the different testcases (and print a nice list of them to the TAP log when the test is run). - Fix some issues sparse picked up with __le32 tagged integers. - Add an fs/fat/.kunitconfig file which contains all the Kconfig entries needed to run the test. The test can now be run with: ./tools/testing/kunit/kunit.py run --kunitconfig fs/fat/.kunitconfig Changes since v6: https://lore.kernel.org/linux-kselftest/20201024060558.2556249-1-david...@google.com/ - Make CONFIG_FAT_DEFAULT_CODEPAGE depend on FAT_FS, rather than either VFAT_FS or MSDOS_FS. - This means that FAT_KUNIT_TEST can now also just depend of FAT_FS - Fix a few warnings that KUnit tool was eating: - KUnit's type checking needs a specific cast for the fat_checksum() expected results. - The time test cases shouldn't be 'const' - The fake superblock is now static, as otherwise it increased the stack size too much. Changes since v4/5: https://lore.kernel.org/linux-kselftest/20201024052047.2526780-1-david...@google.com/ - Fix a typo introduced in the Kconfig. It builds now. Changes since v3: https://lore.kernel.org/linux-kselftest/20201021061713.1545931-1-david...@google.com/ - Update the Kconfig entry to use "depends on" rather than "select", as discussed in [2]. - Depend on "MSDOS_FS || VFAT_FS", rather than "FAT_FS", as we need the CONFIG_FAT_DEFAULT_CODEPAGE symbol to be defined. Changes since v2: https://lore.kernel.org/linux-kselftest/20201020055856.1270482-1-david...@google.com/ - Comment that the export for fat_time_fat2unix() function is for KUnit tests. Changes since v1: https://lore.kernel.org/linux-kselftest/20201017064107.375174-1-david...@google.com/ - Now export fat_time_fat2unix() so that the test can access it when built as a module. [2]: https://lore.kernel.org/linux-ext4/52959e99-4105-3de9-730c-c46894b82...@infradead.org/T/#t fs/fat/.kunitconfig | 5 ++ fs/fat/Kconfig | 14 +++- fs/fat/Makefile | 2 + fs/fat/fat_test.c | 197 fs/fat/misc.c | 2 + 5 files changed, 219 insertions(+), 1 deletion(-) create mode 100644 fs/fat/.kunitconfig create mode 100644 fs/fat/fat_test.c diff --git a/fs/fat/.kunitconfig b/fs/fat/.kunitconfig new file mode 100644 index ..0a6971dbeccb --- /dev/null +++ b/fs/fat/.kunitconfig @@ -0,0 +1,5 @@ +CONFIG_KUNIT=y +CONFIG_FAT_FS=y +CONFIG_MSDOS_FS=y +CONFIG_VFAT_FS=y +CONFIG_FAT_KUNIT_TEST=y diff --git a/fs/fat/Kconfig b/fs/fat/Kconfig index 66532a71e8fd..238cc55f84c4 100644 --- a/fs/fat/Kconfig +++ b/fs/fat/Kconfig @@ -77,7 +77,7 @@ config VFAT_FS config FAT_DEFAULT_CODEPAGE int "Default codepage for FAT" - depends on MSDOS_FS || VFAT_FS + depends on FAT_FS default 437 help This option should be set to the codepage of your FAT filesystems. @@ -115,3 +115,15 @@ config FAT_DEFAULT_UTF8 Say Y if you use UTF-8 encoding for file names, N otherwise. See for more information. + +config FAT_KUNIT_TEST + tristate "Unit Tests for FAT filesystems" if !KUNIT_ALL_TESTS + depends on KUNIT && FAT_FS + default KUNIT_ALL_TESTS + help + This builds the FAT KUnit tests + + For more information on KUnit and unit tests in general, please refer + to the KUnit documentation in Documentation/dev-tools/kunit + + If unsure, say N diff --git a/fs/fat/Makefile b/fs/fat/Makefile index 70645ce2f7fc..2b034112690d 100644 --- a/fs/fat/Makefile +++ b/fs/fat/Makefile @@ -10,3 +10,5 @@ obj-$(CONFIG_M
[PATCH v2] Documentation: kunit: Update kunit_tool page
The kunit_tool documentation page was pretty minimal, and a bit outdated. Update it and flesh it out a bit. In particular, - Mention that .kunitconfig is now in the build directory - Describe the use of --kunitconfig to specify a different config framgent - Mention the split functionality (i.e., commands other than 'run') - Describe --raw_output and kunit.py parse - Mention the globbing support - Provide a quick overview of other options, including --build_dir and --alltests Note that this does overlap a little with the new running_tips page. I don't think it's a problem having both: this page is supposed to be a bit more of a reference, rather than a list of useful tips, so the fact that they both describe the same features isn't a problem. Signed-off-by: David Gow Reviewed-by: Daniel Latypov --- Adopted the changes from Daniel. Changes since v1: https://lore.kernel.org/linux-kselftest/20210416034036.797727-1-david...@google.com/ - Mention that the default build directory is '.kunit' when discussing '.kunitconfig' files. - Reword the discussion of 'CONFIG_KUNIT_ALL_TESTS' under '--alltests' Documentation/dev-tools/kunit/kunit-tool.rst | 140 +-- 1 file changed, 132 insertions(+), 8 deletions(-) diff --git a/Documentation/dev-tools/kunit/kunit-tool.rst b/Documentation/dev-tools/kunit/kunit-tool.rst index 29ae2fee8123..4247b7420e3b 100644 --- a/Documentation/dev-tools/kunit/kunit-tool.rst +++ b/Documentation/dev-tools/kunit/kunit-tool.rst @@ -22,14 +22,19 @@ not require any virtualization support: it is just a regular program. What is a .kunitconfig? === -It's just a defconfig that kunit_tool looks for in the base directory. -kunit_tool uses it to generate a .config as you might expect. In addition, it -verifies that the generated .config contains the CONFIG options in the -.kunitconfig; the reason it does this is so that it is easy to be sure that a -CONFIG that enables a test actually ends up in the .config. +It's just a defconfig that kunit_tool looks for in the build directory +(``.kunit`` by default). kunit_tool uses it to generate a .config as you might +expect. In addition, it verifies that the generated .config contains the CONFIG +options in the .kunitconfig; the reason it does this is so that it is easy to +be sure that a CONFIG that enables a test actually ends up in the .config. -How do I use kunit_tool? - +It's also possible to pass a separate .kunitconfig fragment to kunit_tool, +which is useful if you have several different groups of tests you wish +to run independently, or if you want to use pre-defined test configs for +certain subsystems. + +Getting Started with kunit_tool +=== If a kunitconfig is present at the root directory, all you have to do is: @@ -48,10 +53,129 @@ However, you most likely want to use it with the following options: .. note:: This command will work even without a .kunitconfig file: if no -.kunitconfig is present, a default one will be used instead. + .kunitconfig is present, a default one will be used instead. + +If you wish to use a different .kunitconfig file (such as one provided for +testing a particular subsystem), you can pass it as an option. + +.. code-block:: bash + + ./tools/testing/kunit/kunit.py run --kunitconfig=fs/ext4/.kunitconfig For a list of all the flags supported by kunit_tool, you can run: .. code-block:: bash ./tools/testing/kunit/kunit.py run --help + +Configuring, Building, and Running Tests + + +It's also possible to run just parts of the KUnit build process independently, +which is useful if you want to make manual changes to part of the process. + +A .config can be generated from a .kunitconfig by using the ``config`` argument +when running kunit_tool: + +.. code-block:: bash + + ./tools/testing/kunit/kunit.py config + +Similarly, if you just want to build a KUnit kernel from the current .config, +you can use the ``build`` argument: + +.. code-block:: bash + + ./tools/testing/kunit/kunit.py build + +And, if you already have a built UML kernel with built-in KUnit tests, you can +run the kernel and display the test results with the ``exec`` argument: + +.. code-block:: bash + + ./tools/testing/kunit/kunit.py exec + +The ``run`` command which is discussed above is equivalent to running all three +of these in sequence. + +All of these commands accept a number of optional command-line arguments. The +``--help`` flag will give a complete list of these, or keep reading this page +for a guide to some of the more useful ones. + +Parsing Test Results + + +KUnit tests output their results in TAP (Test Anything Protocol) format. +kunit_tool will, when running tests, parse this output and print a summary +which is much more
Re: [PATCH v6] lib: add basic KUnit test for lib/math
On Sat, Apr 17, 2021 at 2:04 AM Daniel Latypov wrote: > > Add basic test coverage for files that don't require any config options: > * part of math.h (what seem to be the most commonly used macros) > * gcd.c > * lcm.c > * int_sqrt.c > * reciprocal_div.c > (Ignored int_pow.c since it's a simple textbook algorithm.) > > These tests aren't particularly interesting, but they > * provide short and simple examples of parameterized tests > * provide a place to add tests for any new files in this dir > * are written so adding new test cases to cover edge cases should be easy > * looking at code coverage, we hit all the branches in the .c files > > Signed-off-by: Daniel Latypov > Reviewed-by: David Gow > --- Thanks: I've tested this version, and am happy with it. A part of me still kind-of would like there to be names for the parameters, but I definitely understand that it doesn't really work well for the lcm and gcd cases where we're doing both (a,b) and (b,a). So let's keep it as-is. Hopefully we can get these in for 5.13! Cheers, -- David
Re: [PATCH v2 5/6] kunit: mptcp: adhear to KUNIT formatting standard
Hi Matt, > Like patch 1/6, I can apply it in MPTCP tree and send it later to > net-next with other patches. > Except if you guys prefer to apply it in KUnit tree and send it to > linux-next? Given 1/6 is going to net-next, it makes sense to send this out that way too, then, IMHO. The only slight concern I have is that the m68k test config patch in the series will get split from the others, but that should resolve itself when they pick up the last patch. At the very least, this shouldn't cause any conflicts with anything we're doing in the KUnit tree. Cheers, -- David
Re: [PATCH] kunit: add unit test for filtering suites by names
On Tue, Apr 13, 2021 at 8:08 AM Daniel Latypov wrote: > > This adds unit tests for kunit_filter_subsuite() and > kunit_filter_suites(). > > Note: what the executor means by "subsuite" is the array of suites > corresponding to each test file. > > This patch lightly refactors executor.c to avoid the use of global > variables to make it testable. > It also includes a clever `kfree_at_end()` helper that makes this test > easier to write than it otherwise would have been. > > Tested by running just the new tests using itself > $ ./tools/testing/kunit/kunit.py run '*exec*' > > Signed-off-by: Daniel Latypov I really like this test, thanks. A few small notes below, including what I think is a missing kfree_at_end() call. Assuming that one issue is fixed (or I'm mistaken): Reviewed-by: David Gow -- David > --- > lib/kunit/executor.c | 26 > lib/kunit/executor_test.c | 132 ++ > 2 files changed, 147 insertions(+), 11 deletions(-) > create mode 100644 lib/kunit/executor_test.c > > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c > index 15832ed44668..96a4ae983786 100644 > --- a/lib/kunit/executor.c > +++ b/lib/kunit/executor.c > @@ -19,7 +19,7 @@ MODULE_PARM_DESC(filter_glob, > "Filter which KUnit test suites run at boot-time, e.g. > list*"); > > static struct kunit_suite * const * > -kunit_filter_subsuite(struct kunit_suite * const * const subsuite) > +kunit_filter_subsuite(struct kunit_suite * const * const subsuite, const > char *filter_glob) > { > int i, n = 0; > struct kunit_suite **filtered; > @@ -52,19 +52,14 @@ struct suite_set { > struct kunit_suite * const * const *end; > }; > > -static struct suite_set kunit_filter_suites(void) > +static struct suite_set kunit_filter_suites(const struct suite_set > *suite_set, > + const char *filter_glob) > { > int i; > struct kunit_suite * const **copy, * const *filtered_subsuite; > struct suite_set filtered; > > - const size_t max = __kunit_suites_end - __kunit_suites_start; > - > - if (!filter_glob) { > - filtered.start = __kunit_suites_start; > - filtered.end = __kunit_suites_end; > - return filtered; > - } > + const size_t max = suite_set->end - suite_set->start; > > copy = kmalloc_array(max, sizeof(*filtered.start), GFP_KERNEL); > filtered.start = copy; > @@ -74,7 +69,7 @@ static struct suite_set kunit_filter_suites(void) > } > > for (i = 0; i < max; ++i) { > - filtered_subsuite = > kunit_filter_subsuite(__kunit_suites_start[i]); > + filtered_subsuite = > kunit_filter_subsuite(suite_set->start[i], filter_glob); > if (filtered_subsuite) > *copy++ = filtered_subsuite; > } > @@ -98,8 +93,13 @@ static void kunit_print_tap_header(struct suite_set > *suite_set) > int kunit_run_all_tests(void) > { > struct kunit_suite * const * const *suites; > + struct suite_set suite_set = { > + .start = __kunit_suites_start, > + .end = __kunit_suites_end, > + }; > > - struct suite_set suite_set = kunit_filter_suites(); > + if (filter_glob) > + suite_set = kunit_filter_suites(&suite_set, filter_glob); > > kunit_print_tap_header(&suite_set); > > @@ -115,4 +115,8 @@ int kunit_run_all_tests(void) > return 0; > } > > +#if IS_BUILTIN(CONFIG_KUNIT_TEST) > +#include "executor_test.c" > +#endif > + > #endif /* IS_BUILTIN(CONFIG_KUNIT) */ > diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c > new file mode 100644 > index ..8e925395beeb > --- /dev/null > +++ b/lib/kunit/executor_test.c > @@ -0,0 +1,132 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * KUnit test for the KUnit executor. > + * > + * Copyright (C) 2021, Google LLC. > + * Author: Daniel Latypov > + */ > + > +#include > + > +static void kfree_at_end(struct kunit *test, const void *to_free); > +static struct kunit_suite *alloc_fake_suite(struct kunit *test, > + const char *suite_name); > + > +static void filter_subsuite_test(struct kunit *test) > +{ > + struct kunit_suite *subsuite[3] = {NULL, NULL, NULL}; > + struct kunit_suite * const *filtered; > + > + subsuite[0] = alloc_fake_suite(test, "suite1"); > + subsuite[1] = alloc_fake_suite(test, "suite2&
Re: [PATCH v5] lib: add basic KUnit test for lib/math
On Tue, Apr 13, 2021 at 3:07 AM Daniel Latypov wrote: > > Add basic test coverage for files that don't require any config options: > * part of math.h (what seem to be the most commonly used macros) > * gcd.c > * lcm.c > * int_sqrt.c > * reciprocal_div.c > (Ignored int_pow.c since it's a simple textbook algorithm.) > > These tests aren't particularly interesting, but they > * provide short and simple examples of parameterized tests > * provide a place to add tests for any new files in this dir > * are written so adding new test cases to cover edge cases should be easy > * looking at code coverage, we hit all the branches in the .c files > > Signed-off-by: Daniel Latypov This looks good to me. A few comments/observations below, but nothing that I think should actually block this. Reviewed-by: David Gow -- David > --- > Changes since v4: > * add in test cases for some math.h macros (abs, round_up/round_down, > div_round_down/closest) > * use parameterized testing less to keep things terser > > Changes since v3: > * fix `checkpatch.pl --strict` warnings > * add test cases for gcd(0,0) and lcm(0,0) > * minor: don't test both gcd(a,b) and gcd(b,a) when a == b > > Changes since v2: mv math_test.c => math_kunit.c > > Changes since v1: > * Rebase and rewrite to use the new parameterized testing support. > * misc: fix overflow in literal and inline int_sqrt format string. > * related: commit 1f0e943df68a ("Documentation: kunit: provide guidance > for testing many inputs") was merged explaining the patterns shown here. > * there's an in-flight patch to update it for parameterized testing. > > v1: https://lore.kernel.org/lkml/20201019224556.3536790-1-dlaty...@google.com/ > --- > lib/math/Kconfig | 5 + > lib/math/Makefile | 2 + > lib/math/math_kunit.c | 264 ++ > 3 files changed, 271 insertions(+) > create mode 100644 lib/math/math_kunit.c > > diff --git a/lib/math/Kconfig b/lib/math/Kconfig > index f19bc9734fa7..6ba8680439c1 100644 > --- a/lib/math/Kconfig > +++ b/lib/math/Kconfig > @@ -15,3 +15,8 @@ config PRIME_NUMBERS > > config RATIONAL > bool > + > +config MATH_KUNIT_TEST > + tristate "KUnit test for lib/math" if !KUNIT_ALL_TESTS > + default KUNIT_ALL_TESTS > + depends on KUNIT This could have a description of the test and KUnit here, as mentioned in the style guide doc: https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html#test-kconfig-entries (I think it's sufficiently self explanatory that it's not essential, but it could be nice to have a more detailed description of the things being tested than just "lib/math".) > diff --git a/lib/math/Makefile b/lib/math/Makefile > index be6909e943bd..30abb7a8d564 100644 > --- a/lib/math/Makefile > +++ b/lib/math/Makefile > @@ -4,3 +4,5 @@ obj-y += div64.o gcd.o lcm.o int_pow.o int_sqrt.o > reciprocal_div.o > obj-$(CONFIG_CORDIC) += cordic.o > obj-$(CONFIG_PRIME_NUMBERS)+= prime_numbers.o > obj-$(CONFIG_RATIONAL) += rational.o > + > +obj-$(CONFIG_MATH_KUNIT_TEST) += math_kunit.o > diff --git a/lib/math/math_kunit.c b/lib/math/math_kunit.c > new file mode 100644 > index ..80a087a32884 > --- /dev/null > +++ b/lib/math/math_kunit.c > @@ -0,0 +1,264 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Simple KUnit suite for math helper funcs that are always enabled. > + * > + * Copyright (C) 2020, Google LLC. > + * Author: Daniel Latypov > + */ > + > +#include > +#include > +#include > +#include > +#include > + > +static void abs_test(struct kunit *test) > +{ There's something weird about taking the absolute values of char literals. I'm not sure if it's better to case integer literals (like with 'short' below), or keep it as-is. > + KUNIT_EXPECT_EQ(test, abs('\0'), '\0'); > + KUNIT_EXPECT_EQ(test, abs('a'), 'a'); > + KUNIT_EXPECT_EQ(test, abs(-'a'), 'a'); > + > + /* The expression in the macro is actually promoted to an int. */ > + KUNIT_EXPECT_EQ(test, abs((short)0), 0); > + KUNIT_EXPECT_EQ(test, abs((short)42), 42); > + KUNIT_EXPECT_EQ(test, abs((short)-42), 42); > + > + KUNIT_EXPECT_EQ(test, abs(0), 0); > + KUNIT_EXPECT_EQ(test, abs(42), 42); > + KUNIT_EXPECT_EQ(test, abs(-42), 42); > + > + KUNIT_EXPECT_EQ(test, abs(0L), 0L); > + KUNIT_EXPECT_EQ(test, abs(42L), 42L); > + KUNIT_EXPECT_EQ(test, abs(-42L), 42L); > + > + KUNIT_EXPECT_EQ(test, abs(0LL), 0LL); >
Re: [PATCH v3] Documentation: kunit: add tips for running KUnit
On Wed, Apr 14, 2021 at 8:45 AM Daniel Latypov wrote: > > This is long overdue. > > There are several things that aren't nailed down (in-tree > .kunitconfig's), or partially broken (GCOV on UML), but having them > documented, warts and all, is better than having nothing. > > This covers a bunch of the more recent features > * kunit_filter_glob > * kunit.py run --kunitconfig > * slightly more detail on building tests as modules > * CONFIG_KUNIT_DEBUGFS > > By my count, the only headline features now not mentioned are the KASAN > integration and KernelCI json output support (kunit.py run --json). > > And then it also discusses how to get code coverage reports under UML > and non-UML since this is a question people have repeatedly asked. > > Non-UML coverage collection is no different from normal, but we should > probably explicitly call this out. > > As for UML, I was able to get it working again with two small hacks.* > E.g. with CONFIG_KUNIT=y && CONFIG_KUNIT_ALL_TESTS=y > Overall coverage rate: > lines..: 15.1% (18294 of 120776 lines) > functions..: 16.8% (1860 of 11050 functions) > > Note: this doesn't document --alltests since this is not stable yet. > Hopefully being run more frequently as part of KernelCI will help... > > *Using gcc/gcov-6 and not using uml_abort() in os_dump_core(). > I've documented these hacks in "Notes" but left TODOs for > brendanhigg...@google.com who tracked down the runtime issue in GCC. > To be clear: these are not issues specific to KUnit, but rather to UML. > > Signed-off-by: Daniel Latypov > --- I'm very happy with this now: all my issues with the previous versions are addressed. I'm particularly excited to have code coverage documented somewhere. Assuming Brendan's happy with the TODOs being there, I think this is ready to go. I also built this with Sphinx and gave it a quick look, and it all looks good there as well. Therefore, this is: Reviewed-by: David Gow Cheers, -- David
[PATCH v2] Documentation: dev-tools: Add Testing Overview
The kernel now has a number of testing and debugging tools, and we've seen a bit of confusion about what the differences between them are. Add a basic documentation outlining the testing tools, when to use each, and how they interact. This is a pretty quick overview rather than the idealised "kernel testing guide" that'd probably be optimal, but given the number of times questions like "When do you use KUnit and when do you use Kselftest?" are being asked, it seemed worth at least having something. Hopefully this can form the basis for more detailed documentation later. Signed-off-by: David Gow --- Thanks, everyone, for the comments on the doc. I've made a few of the suggested changes. Please let me know what you think! -- David Changes since v1: https://lore.kernel.org/linux-kselftest/20210410070529.4113432-1-david...@google.com/ - Note KUnit's speed and that one should provide selftests for syscalls - Mention lockdep as a Dynamic Analysis Tool - Refer to "Dynamic Analysis Tools" instead of "Sanitizers" - A number of minor formatting tweaks and rewordings for clarity. Not changed: - I haven't included an exhaustive list of differences, advantages, etc, between KUnit and kselftest: for now, the doc continues to focus on the difference between 'in-kernel' and 'userspace' testing here. - Similarly, I'm not linking out to docs defining and describing "Unit" tests versus "End-to-end" tests. None of the existing documentation elsewhere quite matches what we do in the kernel perfectly, so it seems less confusing to focus on the 'in-kernel'/'userspace' distinction, and leave other definitions as a passing mention for those who are already familiar with the concepts. - I haven't linked to any talk videos here: a few of them are linked on (e.g.) the KUnit webpage, but I wanted to keep the Kernel documentation more self-contained for now. No objection to adding them in a follow-up patch if people feel strongly about it, though. - The link from index.rst to this doc is unchanged. I personally think that the link is prominent enough there: it's the first link, and shows up a few times. One possibility if people disagreed would be to merge this page with the index, but given not all dev-tools are going to be testing-related, it seemed a bit arrogant. :-) Documentation/dev-tools/index.rst| 3 + Documentation/dev-tools/testing-overview.rst | 117 +++ 2 files changed, 120 insertions(+) create mode 100644 Documentation/dev-tools/testing-overview.rst diff --git a/Documentation/dev-tools/index.rst b/Documentation/dev-tools/index.rst index 1b1cf4f5c9d9..f590e5860794 100644 --- a/Documentation/dev-tools/index.rst +++ b/Documentation/dev-tools/index.rst @@ -7,6 +7,8 @@ be used to work on the kernel. For now, the documents have been pulled together without any significant effort to integrate them into a coherent whole; patches welcome! +A brief overview of testing-specific tools can be found in :doc:`testing-overview`. + .. class:: toc-title Table of contents @@ -14,6 +16,7 @@ whole; patches welcome! .. toctree:: :maxdepth: 2 + testing-overview coccinelle sparse kcov diff --git a/Documentation/dev-tools/testing-overview.rst b/Documentation/dev-tools/testing-overview.rst new file mode 100644 index ..ce36a8cdf6b5 --- /dev/null +++ b/Documentation/dev-tools/testing-overview.rst @@ -0,0 +1,117 @@ +.. SPDX-License-Identifier: GPL-2.0 + + +Kernel Testing Guide + + + +There are a number of different tools for testing the Linux kernel, so knowing +when to use each of them can be a challenge. This document provides a rough +overview of their differences, and how they fit together. + + +Writing and Running Tests += + +The bulk of kernel tests are written using either the kselftest or KUnit +frameworks. These both provide infrastructure to help make running tests and +groups of tests easier, as well as providing helpers to aid in writing new +tests. + +If you're looking to verify the behaviour of the Kernel — particularly specific +parts of the kernel — then you'll want to use KUnit or kselftest. + + +The Difference Between KUnit and kselftest +-- + +KUnit (Documentation/dev-tools/kunit/index.rst) is an entirely in-kernel system +for "white box" testing: because test code is part of the kernel, it can access +internal structures and functions which aren't exposed to userspace. + +KUnit tests therefore are best written against small, self-contained parts +of the kernel, which can be tested in isolation. This aligns well with the +concept of 'unit' testing. + +For example, a KUnit test might test an individual kernel function (or even a +single
Re: [PATCH v2] Documentation: dev-tools: Add Testing Overview
On Thu, Apr 15, 2021 at 12:30 AM Daniel Latypov wrote: > > On Wed, Apr 14, 2021 at 1:15 AM David Gow wrote: > > > > The kernel now has a number of testing and debugging tools, and we've > > seen a bit of confusion about what the differences between them are. > > > > Add a basic documentation outlining the testing tools, when to use each, > > and how they interact. > > > > This is a pretty quick overview rather than the idealised "kernel > > testing guide" that'd probably be optimal, but given the number of times > > questions like "When do you use KUnit and when do you use Kselftest?" > > are being asked, it seemed worth at least having something. Hopefully > > this can form the basis for more detailed documentation later. > > > > Signed-off-by: David Gow > > Reviewed-by: Daniel Latypov > > Looks good to me. Some minor typos and nits about wording here and there. > Thanks: I'll send out v3 with some fixes to your suggestions soon. Cheers, -- David > > --- > > Thanks, everyone, for the comments on the doc. I've made a few of the > > suggested changes. Please let me know what you think! > > > > -- David > > > > Changes since v1: > > https://lore.kernel.org/linux-kselftest/20210410070529.4113432-1-david...@google.com/ > > - Note KUnit's speed and that one should provide selftests for syscalls > > - Mention lockdep as a Dynamic Analysis Tool > > - Refer to "Dynamic Analysis Tools" instead of "Sanitizers" > > - A number of minor formatting tweaks and rewordings for clarity. > > > > Not changed: > > - I haven't included an exhaustive list of differences, advantages, etc, > > between KUnit and kselftest: for now, the doc continues to focus on > > the difference between 'in-kernel' and 'userspace' testing here. > > - Similarly, I'm not linking out to docs defining and describing "Unit" > > tests versus "End-to-end" tests. None of the existing documentation > > elsewhere quite matches what we do in the kernel perfectly, so it > > seems less confusing to focus on the 'in-kernel'/'userspace' > > distinction, and leave other definitions as a passing mention for > > those who are already familiar with the concepts. > > - I haven't linked to any talk videos here: a few of them are linked on > > (e.g.) the KUnit webpage, but I wanted to keep the Kernel documentation > > more self-contained for now. No objection to adding them in a follow-up > > patch if people feel strongly about it, though. > > - The link from index.rst to this doc is unchanged. I personally think > > that the link is prominent enough there: it's the first link, and > > shows up a few times. One possibility if people disagreed would be to > > merge this page with the index, but given not all dev-tools are going > > to be testing-related, it seemed a bit arrogant. :-) > > > > Documentation/dev-tools/index.rst| 3 + > > Documentation/dev-tools/testing-overview.rst | 117 +++ > > 2 files changed, 120 insertions(+) > > create mode 100644 Documentation/dev-tools/testing-overview.rst > > > > diff --git a/Documentation/dev-tools/index.rst > > b/Documentation/dev-tools/index.rst > > index 1b1cf4f5c9d9..f590e5860794 100644 > > --- a/Documentation/dev-tools/index.rst > > +++ b/Documentation/dev-tools/index.rst > > @@ -7,6 +7,8 @@ be used to work on the kernel. For now, the documents have > > been pulled > > together without any significant effort to integrate them into a coherent > > whole; patches welcome! > > > > +A brief overview of testing-specific tools can be found in > > :doc:`testing-overview`. > > + > > .. class:: toc-title > > > >Table of contents > > @@ -14,6 +16,7 @@ whole; patches welcome! > > .. toctree:: > > :maxdepth: 2 > > > > + testing-overview > > coccinelle > > sparse > > kcov > > diff --git a/Documentation/dev-tools/testing-overview.rst > > b/Documentation/dev-tools/testing-overview.rst > > new file mode 100644 > > index ..ce36a8cdf6b5 > > --- /dev/null > > +++ b/Documentation/dev-tools/testing-overview.rst > > @@ -0,0 +1,117 @@ > > +.. SPDX-License-Identifier: GPL-2.0 > > + > > + > > +Kernel Testing Guide > > + > > + > > + > > +There are a number of different tools for testing the Linux
[PATCH v3] Documentation: dev-tools: Add Testing Overview
The kernel now has a number of testing and debugging tools, and we've seen a bit of confusion about what the differences between them are. Add a basic documentation outlining the testing tools, when to use each, and how they interact. This is a pretty quick overview rather than the idealised "kernel testing guide" that'd probably be optimal, but given the number of times questions like "When do you use KUnit and when do you use Kselftest?" are being asked, it seemed worth at least having something. Hopefully this can form the basis for more detailed documentation later. Signed-off-by: David Gow Reviewed-by: Marco Elver Reviewed-by: Daniel Latypov --- Thanks again. Assuming no-one has any objections, I think this is good to go. -- David Changes since v2: https://lore.kernel.org/linux-kselftest/20210414081428.337494-1-david...@google.com/ - A few typo fixes (Thanks Daniel) - Reworded description of dynamic analysis tools. - Updated dev-tools index page to not use ':doc:' syntax, but to provide a path instead. - Added Marco and Daniel's Reviewed-by tags. Changes since v1: https://lore.kernel.org/linux-kselftest/20210410070529.4113432-1-david...@google.com/ - Note KUnit's speed and that one should provide selftests for syscalls - Mention lockdep as a Dynamic Analysis Tool - Refer to "Dynamic Analysis Tools" instead of "Sanitizers" - A number of minor formatting tweaks and rewordings for clarity Documentation/dev-tools/index.rst| 4 + Documentation/dev-tools/testing-overview.rst | 117 +++ 2 files changed, 121 insertions(+) create mode 100644 Documentation/dev-tools/testing-overview.rst diff --git a/Documentation/dev-tools/index.rst b/Documentation/dev-tools/index.rst index 1b1cf4f5c9d9..929d916ffd4c 100644 --- a/Documentation/dev-tools/index.rst +++ b/Documentation/dev-tools/index.rst @@ -7,6 +7,9 @@ be used to work on the kernel. For now, the documents have been pulled together without any significant effort to integrate them into a coherent whole; patches welcome! +A brief overview of testing-specific tools can be found in +Documentation/dev-tools/testing-overview.rst + .. class:: toc-title Table of contents @@ -14,6 +17,7 @@ whole; patches welcome! .. toctree:: :maxdepth: 2 + testing-overview coccinelle sparse kcov diff --git a/Documentation/dev-tools/testing-overview.rst b/Documentation/dev-tools/testing-overview.rst new file mode 100644 index ..b5b46709969c --- /dev/null +++ b/Documentation/dev-tools/testing-overview.rst @@ -0,0 +1,117 @@ +.. SPDX-License-Identifier: GPL-2.0 + + +Kernel Testing Guide + + + +There are a number of different tools for testing the Linux kernel, so knowing +when to use each of them can be a challenge. This document provides a rough +overview of their differences, and how they fit together. + + +Writing and Running Tests += + +The bulk of kernel tests are written using either the kselftest or KUnit +frameworks. These both provide infrastructure to help make running tests and +groups of tests easier, as well as providing helpers to aid in writing new +tests. + +If you're looking to verify the behaviour of the Kernel — particularly specific +parts of the kernel — then you'll want to use KUnit or kselftest. + + +The Difference Between KUnit and kselftest +-- + +KUnit (Documentation/dev-tools/kunit/index.rst) is an entirely in-kernel system +for "white box" testing: because test code is part of the kernel, it can access +internal structures and functions which aren't exposed to userspace. + +KUnit tests therefore are best written against small, self-contained parts +of the kernel, which can be tested in isolation. This aligns well with the +concept of 'unit' testing. + +For example, a KUnit test might test an individual kernel function (or even a +single codepath through a function, such as an error handling case), rather +than a feature as a whole. + +This also makes KUnit tests very fast to build and run, allowing them to be +run frequently as part of the development process. + +There is a KUnit test style guide which may give further pointers in +Documentation/dev-tools/kunit/style.rst + + +kselftest (Documentation/dev-tools/kselftest.rst), on the other hand, is +largely implemented in userspace, and tests are normal userspace scripts or +programs. + +This makes it easier to write more complicated tests, or tests which need to +manipulate the overall system state more (e.g., spawning processes, etc.). +However, it's not possible to call kernel functions directly from kselftest. +This means that only kernel functionality which is exposed to userspace somehow +(e.g. by a syscall, device, filesystem, etc.) can be tested with kselftest. To +work around this, some test
Re: [PATCH v2 5/6] kunit: mptcp: adhear to KUNIT formatting standard
Hi Nico, Matthieu, Thanks for going to the trouble of making these conform to the KUnit style guidelines. On Wed, Apr 14, 2021 at 5:25 PM Matthieu Baerts wrote: > > Hi Nico, > > On 14/04/2021 10:58, Nico Pache wrote: > > Drop 'S' from end of CONFIG_MPTCP_KUNIT_TESTS inorder to adhear to the > > KUNIT *_KUNIT_TEST config name format. [Super nitpicky comment for this series: It's 'adhere', not 'adhear'. It's not worth resending this out just to fix the spelling here, but if you do another version, it might be worth fiing.] > > For MPTCP, we have multiple KUnit tests: crypto and token. That's why we > wrote TESTS with a S. So (as this patch series attests), there are a few different config options which cover (or intend one day to cover) multiple suites, and hence end in KUNIT_TESTS. Personally, I'd still slightly prefer TEST here, just to have a common suffix for KUnit test options, and that's what I was going for when writing the style guide. That being said, it's also worth noting that there is an explicit exemption for existing tests, so if you (for example) have a bunch of automation which relies on this name and can't easily be changed, that's probably more important than a lone 'S'. > I'm fine without S if we need to stick with KUnit' standard. But maybe > the standard wants us to split the two tests and create > MPTCP_TOKEN_KUNIT_TEST and MPTCP_TOKEN_KUNIT_TEST config? > > In this case, we could eventually keep MPTCP_KUNIT_TESTS which will in > charge of selecting the two new ones. This is certainly an option if you want to do it, but personally I wouldn't bother unless it gives you some real advantage. One thing I would note, however, is that it's possible to have a per-subsystem '.kunitconfig' file, so if you want to run a particular group of tests (i.e., have a particular set of config options for testing), it'd be possible to have that rather than a meta-Kconfig entry. While there are some advantages to trying to have a 1:1 suite:config mapping, there's isn't actually anything that depends on it at the moment (or indeed, anything actively planned). So, in my view, there's no need for you to split the config entry unless you think there's a good reason you'd want to be able to build one set of tests but not the other. > Up to the KUnit maintainers to decide ;-) To summarise my view: personally, I'd prefer things the way this patch works: have everything end in _KUNIT_TEST, even if that enables a couple of suites. The extra 'S' on the end isn't a huge problem if you have a good reason to particularly want to keep it, though: as long as you don't have something like _K_UNIT_VERIFICATION or something equally silly that'd break grepping for '_KUNIT_TEST', it's fine be me. So, since it matches my prejudices, this patch is: Reviewed-by: David Gow Thanks, -- David
Re: [PATCH] Documentation: kunit: add tips for running KUnit
Thanks for writing this: it's good to have these things documented at last! There are definitely a few things this document points out which still need deciding, which does make this document lean a bit into "design discussion" territory in a few of the notes. This doesn't bother me -- it's an accurate description of the state of things -- but I wouldn't want this documentation held up too long because of these sorts of TODOs (and can definitely see how having too many of them might discourage KUnit use a bit). Particularly things like the ".kunitconfig" fragment file feature stuff: I feel that's something better discussed on patches adding/using the feature than in the documentation / reviews of the documentation, so I'd rather drop or simplify those '..note:'s than bokeshed about it here (something I'm a little guilty of below). Otherwise, a few minor comments and nitpicks: -- David On Sat, Apr 10, 2021 at 2:01 AM Daniel Latypov wrote: > > This is long overdue. > > There are several things that aren't nailed down (in-tree > .kunitconfig's), or partially broken (GCOV on UML), but having them > documented, warts and all, is better than having nothing. > > This covers a bunch of the more recent features > * kunit_filter_glob > * kunit.py run --kunitconfig > * kunit.py run --alltests > * slightly more detail on building tests as modules > * CONFIG_KUNIT_DEBUGFS > > By my count, the only headline features now not mentioned are the KASAN > integration and KernelCI json output support (kunit.py run --json). > > And then it also discusses how to get code coverage reports under UML > and non-UML since this is a question people have repeatedly asked. > > Non-UML coverage collection is no differnt from normal, but we should > probably explicitly call thsi out. Nit: typos in 'different' and 'this'. > > As for UML, I was able to get it working again with two small hacks.* > E.g. with CONFIG_KUNIT=y && CONFIG_KUNIT_ALL_TESTS=y > Overall coverage rate: > lines..: 15.1% (18294 of 120776 lines) > functions..: 16.8% (1860 of 11050 functions) > > *Switching to use gcc/gcov-6 and not using uml_abort(). > I've documented these hacks in "Notes" but left TODOs for > brendanhigg...@google.com who tracked down the runtime issue in GCC. > To be clear: these are not issues specific to KUnit, but rather to UML. (We should probably note where uml_abort() needs to be replaced if we're mentioning this, though doing so below in the more detailed section may be more useful.) > > Signed-off-by: Daniel Latypov > --- > Documentation/dev-tools/kunit/index.rst | 1 + > .../dev-tools/kunit/running_tips.rst | 278 ++ > Documentation/dev-tools/kunit/start.rst | 2 + > 3 files changed, 281 insertions(+) > create mode 100644 Documentation/dev-tools/kunit/running_tips.rst > > diff --git a/Documentation/dev-tools/kunit/index.rst > b/Documentation/dev-tools/kunit/index.rst > index 848478838347..7f7cf8d2ab20 100644 > --- a/Documentation/dev-tools/kunit/index.rst > +++ b/Documentation/dev-tools/kunit/index.rst > @@ -14,6 +14,7 @@ KUnit - Unit Testing for the Linux Kernel > style > faq > tips > + running_tips > > What is KUnit? > == > diff --git a/Documentation/dev-tools/kunit/running_tips.rst > b/Documentation/dev-tools/kunit/running_tips.rst > new file mode 100644 > index ..d38e665e530f > --- /dev/null > +++ b/Documentation/dev-tools/kunit/running_tips.rst > @@ -0,0 +1,278 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > + > +Tips For Running KUnit Tests > + > + > +Using ``kunit.py run`` ("kunit tool") > += > + > +Running from any directory > +-- > + > +It can be handy to create a bash function like: > + > +.. code-block:: bash > + > + function run_kunit() { > + ( cd "$(git rev-parse --show-toplevel)" && > ./tools/testing/kunit/kunit.py run $@ ) > + } > + > +.. note:: > + Early versions of ``kunit.py`` (before 5.6) didn't work unless run > from > + the kernel root, hence the use of a subshell and ``cd``. > + > +Running a subset of tests > +- > + > +``kunit.py run`` accepts an optional glob argument to filter tests. Currently > +this only matches against suite names, but this may change in the future. > + > +Say that we wanted to run the sysctl tests, we could do so via: > + > +.. code-block:: bash > + > + $ echo -e 'CONFIG_KUNIT=y\nCONFIG_KUNIT_ALL_TESTS=y' > > .kunit/.kunitconfig > + $ ./tools/testing/kunit/kunit.py run 'sysctl*' > + > +We're paying the cost of building more tests than we need this way, but it's > +easier than fiddling with ``.kunitconfig`` files or commenting out > +``kunit_suite``'s. > + > +However, if we wanted to define a set of tests in a less ad hoc way, the next > +tip is useful. > + > +Defining a set of tests > +---
[PATCH] Documentation: dev-tools: Add Testing Overview
The kernel now has a number of testing and debugging tools, and we've seen a bit of confusion about what the differences between them are. Add a basic documentation outlining the testing tools, when to use each, and how they interact. This is a pretty quick overview rather than the idealised "kernel testing guide" that'd probably be optimal, but given the number of times questions like "When do you use KUnit and when do you use Kselftest?" are being asked, it seemed worth at least having something. Hopefully this can form the basis for more detailed documentation later. Signed-off-by: David Gow --- Documentation/dev-tools/index.rst| 3 + Documentation/dev-tools/testing-overview.rst | 102 +++ 2 files changed, 105 insertions(+) create mode 100644 Documentation/dev-tools/testing-overview.rst diff --git a/Documentation/dev-tools/index.rst b/Documentation/dev-tools/index.rst index 1b1cf4f5c9d9..f590e5860794 100644 --- a/Documentation/dev-tools/index.rst +++ b/Documentation/dev-tools/index.rst @@ -7,6 +7,8 @@ be used to work on the kernel. For now, the documents have been pulled together without any significant effort to integrate them into a coherent whole; patches welcome! +A brief overview of testing-specific tools can be found in :doc:`testing-overview`. + .. class:: toc-title Table of contents @@ -14,6 +16,7 @@ whole; patches welcome! .. toctree:: :maxdepth: 2 + testing-overview coccinelle sparse kcov diff --git a/Documentation/dev-tools/testing-overview.rst b/Documentation/dev-tools/testing-overview.rst new file mode 100644 index ..8452adcb8608 --- /dev/null +++ b/Documentation/dev-tools/testing-overview.rst @@ -0,0 +1,102 @@ +.. SPDX-License-Identifier: GPL-2.0 + + +Kernel Testing Guide + + + +There are a number of different tools for testing the Linux kernel, so knowing +when to use each of them can be a challenge. This document provides a rough +overview of their differences, and how they fit together. + + +Writing and Running Tests += + +The bulk of kernel tests are written using either the :doc:`kselftest +` or :doc:`KUnit ` frameworks. These both provide +infrastructure to help make running tests and groups of tests easier, as well +as providing helpers to aid in writing new tests. + +If you're looking to verify the behaviour of the Kernel — particularly specific +parts of the kernel — then you'll want to use `KUnit` or `kselftest`. + + +The Difference Between KUnit and kselftest +-- + +:doc:`KUnit ` is an entirely in-kernel system for "white box" +testing: because test code is part of the kernel, it can access internal +structures and functions which aren't exposed to userspace. + +`KUnit` tests therefore are best written against small, self-contained parts +of the kernel, which can be tested in isolation. This aligns well with the +concept of Unit testing. + +For example, a KUnit test might test an individual kernel function (or even a +single codepath through a function, such as an error handling case), rather +than a feature as a whole. + +There is a KUnit test style guide which may give further pointers + + +:doc:`kselftest `, on the other hand, is largely implemented in +userspace, and tests are normal userspace scripts or programs. + +This makes it easier to write more complicated tests, or tests which need to +manipulate the overall system state more (e.g., spawning processes, etc.). +However, it's not possible to call kernel functions directly unless they're +exposed to userspace (by a syscall, device, filesystem, etc.) Some tests to +also provide a kernel module which is loaded by the test, though for tests +which run mostly or entirely within the kernel, `KUnit` may be the better tool. + +`kselftest` is therefore suited well to tests of whole features, as these will +expose an interface to userspace, which can be tested, but not implementation +details. This aligns well with 'system' or 'end-to-end' testing. + + +Code Coverage Tools +=== + +The Linux Kernel supports two different code coverage mesurement tools. These +can be used to verify that a test is executing particular functions or lines +of code. This is useful for determining how much of the kernel is being tested, +and for finding corner-cases which are not covered by the appropriate test. + +:doc:`kcov` is a feature which can be built in to the kernel to allow +capturing coverage on a per-task level. It's therefore useful for fuzzing and +other situations where information about code executed during, for example, a +single syscall is useful. + +:doc:`gcov` is GCC's coverage testing tool, which can be used with the kernel +to get global or per-module coverage. Unlike KCOV, it does not record per-task +coverage. Coverag
Re: [PATCH] kunit: tool: simplify kconfig is_subset_of() logic
On Wed, Dec 9, 2020 at 7:21 AM Daniel Latypov wrote: > > Don't use an O(nm) algorithm* and make it more readable by using a dict. > > *Most obviously, it does a nested for-loop over the entire other config. > A bit more subtle, it calls .entries(), which constructs a set from the > list for _every_ outer iteration. > > Signed-off-by: Daniel Latypov > --- Thanks! This works great here: I didn't time it to see how much faster it is, but it's clearly an improvement. Reviewed-by: David Gow Cheers, -- David
[PATCH] drivers: fpga: Specify HAS_IOMEM dependency for FPGA_DFL
Because dfl.c uses the 'devm_ioremap', 'devm_iounmap', 'devm_ioremap_resource', and 'devm_platform_ioremap_resource' functions, it should depend on HAS_IOMEM. This fixes make allyesconfig under UML (ARCH=um), which doesn't provide HAS_IOMEM. Signed-off-by: David Gow --- drivers/fpga/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index 7cd5a29fc437..5645226ca3ce 100644 --- a/drivers/fpga/Kconfig +++ b/drivers/fpga/Kconfig @@ -142,6 +142,7 @@ config FPGA_DFL tristate "FPGA Device Feature List (DFL) support" select FPGA_BRIDGE select FPGA_REGION + depends on HAS_IOMEM help Device Feature List (DFL) defines a feature list structure that creates a linked list of feature headers within the MMIO space -- 2.29.2.454.gaff20da3a2-goog
[PATCH] staging: hikey9xx: Specify HAS_IOMEM dependency for MFD_HI6421_SPMI
MFD_CORE is selected by MFD_HI6421_SPMI, and MFD_CORE depends on HAS_IOMEM. If HAS_IOMEM is not set, this can cause a conflict in Kconfig resolution, yielding the following error: WARNING: unmet direct dependencies detected for MFD_CORE Depends on [n]: HAS_IOMEM [=n] Selected by [y]: - MFD_HI6421_SPMI [=y] && STAGING [=y] && OF [=y] && SPMI [=y] By specifying HAS_IOMEM as a dependency for MFD_HI6421_SPMI (as SPMI_HISI3670 already dows), this issue is resolved, and no such warning appears when building on architectures without HAS_IOMEM. Signed-off-by: David Gow --- drivers/staging/hikey9xx/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/staging/hikey9xx/Kconfig b/drivers/staging/hikey9xx/Kconfig index b29f5d5df134..2e48ded92a7e 100644 --- a/drivers/staging/hikey9xx/Kconfig +++ b/drivers/staging/hikey9xx/Kconfig @@ -25,6 +25,7 @@ config SPMI_HISI3670 # to be placed at drivers/mfd config MFD_HI6421_SPMI tristate "HiSilicon Hi6421v600 SPMI PMU/Codec IC" + depends on HAS_IOMEM depends on OF depends on SPMI select MFD_CORE -- 2.29.2.454.gaff20da3a2-goog
[RFC PATCH] bpf: preload: Fix build error when O= is set
If BPF_PRELOAD is enabled, and an out-of-tree build is requested with make O=, compilation seems to fail with: tools/scripts/Makefile.include:4: *** O=.kunit does not exist. Stop. make[4]: *** [../kernel/bpf/preload/Makefile:8: kernel/bpf/preload/libbpf.a] Error 2 make[3]: *** [../scripts/Makefile.build:500: kernel/bpf/preload] Error 2 make[2]: *** [../scripts/Makefile.build:500: kernel/bpf] Error 2 make[2]: *** Waiting for unfinished jobs make[1]: *** [.../Makefile:1799: kernel] Error 2 make[1]: *** Waiting for unfinished jobs make: *** [Makefile:185: __sub-make] Error 2 By the looks of things, this is because the (relative path) O= passed on the command line is being passed to the libbpf Makefile, which then can't find the directory. Given OUTPUT= is being passed anyway, we can work around this by explicitly setting an empty O=, which will be ignored in favour of OUTPUT= in tools/scripts/Makefile.include. Signed-off-by: David Gow --- Hi all, I'm not 100% sure this is the correct fix here -- it seems to work for me, and makes some sense, but let me know if there's a better way. One other thing worth noting is that I've been hitting this with make allyesconfig on ARCH=um, but there's a comment in the Kconfig suggesting that, because BPF_PRELOAD depends on !COMPILE_TEST, that maybe it shouldn't be being built at all. I figured that it was worth trying to fix this anyway. Cheers, -- David kernel/bpf/preload/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/bpf/preload/Makefile b/kernel/bpf/preload/Makefile index 23ee310b6eb4..39848d296097 100644 --- a/kernel/bpf/preload/Makefile +++ b/kernel/bpf/preload/Makefile @@ -5,7 +5,7 @@ LIBBPF_A = $(obj)/libbpf.a LIBBPF_OUT = $(abspath $(obj)) $(LIBBPF_A): - $(Q)$(MAKE) -C $(LIBBPF_SRCS) OUTPUT=$(LIBBPF_OUT)/ $(LIBBPF_OUT)/libbpf.a + $(Q)$(MAKE) -C $(LIBBPF_SRCS) O= OUTPUT=$(LIBBPF_OUT)/ $(LIBBPF_OUT)/libbpf.a userccflags += -I $(srctree)/tools/include/ -I $(srctree)/tools/include/uapi \ -I $(srctree)/tools/lib/ -Wno-unused-result -- 2.29.2.454.gaff20da3a2-goog
Re: [PATCH] drivers: fpga: Specify HAS_IOMEM dependency for FPGA_DFL
On Fri, Nov 20, 2020 at 2:27 PM Moritz Fischer wrote: > > Hi David, > > On Thu, Nov 19, 2020 at 12:22:09AM -0800, David Gow wrote: > > Because dfl.c uses the 'devm_ioremap', 'devm_iounmap', > > 'devm_ioremap_resource', and 'devm_platform_ioremap_resource' > > functions, it should depend on HAS_IOMEM. > > > > This fixes make allyesconfig under UML (ARCH=um), which doesn't provide > > HAS_IOMEM. > > > > Signed-off-by: David Gow > > --- > > drivers/fpga/Kconfig | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig > > index 7cd5a29fc437..5645226ca3ce 100644 > > --- a/drivers/fpga/Kconfig > > +++ b/drivers/fpga/Kconfig > > @@ -142,6 +142,7 @@ config FPGA_DFL > > tristate "FPGA Device Feature List (DFL) support" > > select FPGA_BRIDGE > > select FPGA_REGION > > + depends on HAS_IOMEM > > help > > Device Feature List (DFL) defines a feature list structure that > > creates a linked list of feature headers within the MMIO space > > -- > > 2.29.2.454.gaff20da3a2-goog > > > Do you think we can add a Fixes: tag for this? Sure. I think it should be: Fixes: 543be3d ("fpga: add device feature list support") Let me know if you want me to re-send the patch with that included. Cheers, -- David
Re: [PATCH 1/5] kunit: tool: fix unit test cleanup handling
On Tue, Dec 1, 2020 at 7:32 AM Daniel Latypov wrote: > > * Stop leaking file objects. > * Use self.addCleanup() to ensure we call cleanup functions even if > setUp() fails. > * use mock.patch.stopall instead of more error-prone manual approach > > Signed-off-by: Daniel Latypov > --- I won't pretend to be an expert on Python, but this seems good to me. I tested it on my machine and it works fine. So, Reviewed-by: David Gow -- Davkd > tools/testing/kunit/kunit_tool_test.py | 14 ++ > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/tools/testing/kunit/kunit_tool_test.py > b/tools/testing/kunit/kunit_tool_test.py > index 497ab51bc170..3fbe1acd531a 100755 > --- a/tools/testing/kunit/kunit_tool_test.py > +++ b/tools/testing/kunit/kunit_tool_test.py > @@ -288,19 +288,17 @@ class StrContains(str): > class KUnitMainTest(unittest.TestCase): > def setUp(self): > path = > get_absolute_path('test_data/test_is_test_passed-all_passed.log') > - file = open(path) > - all_passed_log = file.readlines() > - self.print_patch = mock.patch('builtins.print') > - self.print_mock = self.print_patch.start() > + with open(path) as file: > + all_passed_log = file.readlines() > + > + self.print_mock = mock.patch('builtins.print').start() > + self.addCleanup(mock.patch.stopall) > + > self.linux_source_mock = mock.Mock() > self.linux_source_mock.build_reconfig = > mock.Mock(return_value=True) > self.linux_source_mock.build_um_kernel = > mock.Mock(return_value=True) > self.linux_source_mock.run_kernel = > mock.Mock(return_value=all_passed_log) > > - def tearDown(self): > - self.print_patch.stop() > - pass > - > def test_config_passes_args_pass(self): > kunit.main(['config', '--build_dir=.kunit'], > self.linux_source_mock) > assert self.linux_source_mock.build_reconfig.call_count == 1 > > base-commit: b65054597872ce3aefbc6a666385eabdf9e288da > -- > 2.29.2.454.gaff20da3a2-goog >
Re: [PATCH 4/5] kunit: tool: use `with open()` in unit test
On Tue, Dec 1, 2020 at 7:33 AM Daniel Latypov wrote: > > The use of manual open() and .close() calls seems to be an attempt to > keep the contents in scope. > But Python doesn't restrict variables like that, so we can introduce new > variables inside of a `with` and use them outside. > > Do so to make the code more Pythonic. > > Signed-off-by: Daniel Latypov > --- I'm fine with this, and it clearly works fine for me. Out of curiosity, though, is there any difference here other than it being more usual Python style? We've struggled a bit in the past toeing a line between trying to follow "normal" Python style versus adapting it a bit to be more "kernel-y". Experience thus far has actually been that going out on our own has caused more problems than it solves, so I'm all for this change, but I do admit that my brain does understand the older code a touch more easily. In any case, Reviewed-by: David Gow -- David
Re: [PATCH 2/5] kunit: tool: fix unit test so it can run from non-root dir
On Tue, Dec 1, 2020 at 7:33 AM Daniel Latypov wrote: > > get_absolute_path() makes an attempt to allow for this. > But that doesn't work as soon as os.chdir() gets called. Can we explain why this doesn't work? It's because the test_data/ files are accessed with relative paths, so chdir breaks access to them, right? > > So make it so that os.chdir() does nothing to avoid this. > > Note: mock.patch.object() doesn't seem to work in setUpModule(), hence > the introduction of a new base class instead. > > Fixes: 5578d008d9e0 ("kunit: tool: fix running kunit_tool from outside kernel > tree") > Signed-off-by: Daniel Latypov > --- I don't like this: disabling a real system call seems like overkill to work around a path issue like this. Wouldn't it be better to either: (a) stop kunit_tool from needing to chdir entirely; or (b) have get_absolute_path() / test_data_path() produce an absolute path. The latter really seems like the most sensible approach: have the script read its own path and read files relative to that. Cheers, -- David > tools/testing/kunit/kunit_tool_test.py | 15 +++ > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/tools/testing/kunit/kunit_tool_test.py > b/tools/testing/kunit/kunit_tool_test.py > index 3fbe1acd531a..9f1f1e1b772a 100755 > --- a/tools/testing/kunit/kunit_tool_test.py > +++ b/tools/testing/kunit/kunit_tool_test.py > @@ -32,7 +32,13 @@ def tearDownModule(): > def get_absolute_path(path): > return os.path.join(os.path.dirname(__file__), path) > > -class KconfigTest(unittest.TestCase): > +class KUnitTest(unittest.TestCase): > + """Contains common setup, like stopping main() from calling chdir.""" > + def setUp(self): > + mock.patch.object(os, 'chdir').start() > + self.addCleanup(mock.patch.stopall) > + > +class KconfigTest(KUnitTest): > > def test_is_subset_of(self): > kconfig0 = kunit_config.Kconfig() > @@ -88,7 +94,7 @@ class KconfigTest(unittest.TestCase): > self.assertEqual(actual_kconfig.entries(), > expected_kconfig.entries()) > > -class KUnitParserTest(unittest.TestCase): > +class KUnitParserTest(KUnitTest): > > def assertContains(self, needle, haystack): > for line in haystack: > @@ -250,7 +256,7 @@ class KUnitParserTest(unittest.TestCase): > result.status) > self.assertEqual('kunit-resource-test', > result.suites[0].name) > > -class KUnitJsonTest(unittest.TestCase): > +class KUnitJsonTest(KUnitTest): > > def _json_for(self, log_file): > with(open(get_absolute_path(log_file))) as file: > @@ -285,8 +291,9 @@ class StrContains(str): > def __eq__(self, other): > return self in other > > -class KUnitMainTest(unittest.TestCase): > +class KUnitMainTest(KUnitTest): > def setUp(self): > + super().setUp() > path = > get_absolute_path('test_data/test_is_test_passed-all_passed.log') > with open(path) as file: > all_passed_log = file.readlines() > -- > 2.29.2.454.gaff20da3a2-goog >
Re: [PATCH 5/5] minor: kunit: tool: s/get_absolute_path/test_data_path in unit test
On Tue, Dec 1, 2020 at 7:33 AM Daniel Latypov wrote: > > 1. the name is a lie. It gives relative paths, e.g. if I run from the > same dir as the test file, it gives './test_data/' > > 2. it's only used for generating paths to tools/testing/kunit/test_data/ > So we can tersen things by making it less general. > > Signed-off-by: Daniel Latypov > --- This is an excellent and overdue rename/replacement. My only note is re: the concerns I have in patch 2, where I think we probably ought to make this function actually return an absolute path. It seems from the code (and the function name) that that was the intent, so if we can fix it, that'd be ideal. Personally, though, I'd still prefer the new test_data_path(), just have it be an actually absolute path. -- David
Re: [PATCH 3/5] kunit: tool: stop using bare asserts in unit test
On Tue, Dec 1, 2020 at 7:33 AM Daniel Latypov wrote: > > Use self.assertEqual/assertNotEqual() instead. > Besides being more appropriate in a unit test, it'll also give a better > error message by show the unexpected values. > > Also > * Delete redundant check of exception types. self.assertRaises does this. > * s/kall/call. There's no reason to name it this way. > * This is probably a misunderstanding from the docs which uses it > since `mock.call` is in scope as `call`. > > Signed-off-by: Daniel Latypov > --- This works for me, and seems pretty sensible from my rudimentary python knowledge. Reviewed-by: David Gow Cheers, -- David
Re: [PATCH v9 1/2] kunit: Support for Parameterized Testing
On Mon, Nov 23, 2020 at 9:08 PM Marco Elver wrote: > > On Tue, 17 Nov 2020 at 08:21, David Gow wrote: > > On Mon, Nov 16, 2020 at 1:41 PM Arpitha Raghunandan <98.a...@gmail.com> > > wrote: > > > > > > Implementation of support for parameterized testing in KUnit. This > > > approach requires the creation of a test case using the > > > KUNIT_CASE_PARAM() macro that accepts a generator function as input. > > > > > > This generator function should return the next parameter given the > > > previous parameter in parameterized tests. It also provides a macro to > > > generate common-case generators based on arrays. Generators may also > > > optionally provide a human-readable description of parameters, which is > > > displayed where available. > > > > > > Note, currently the result of each parameter run is displayed in > > > diagnostic lines, and only the overall test case output summarizes > > > TAP-compliant success or failure of all parameter runs. In future, when > > > supported by kunit-tool, these can be turned into subsubtest outputs. > > > > > > Signed-off-by: Arpitha Raghunandan <98.a...@gmail.com> > > > Co-developed-by: Marco Elver > > > Signed-off-by: Marco Elver > > > --- > > [Resending this because my email client re-defaulted to HTML! Aarrgh!] > > > > This looks good to me! I tested it in UML and x86-64 w/ KASAN, and > > both worked fine. > > > > Reviewed-by: David Gow > > Tested-by: David Gow > > Thank you! > > > Thanks for sticking with this! > > Will these patches be landing in 5.11 or 5.12? > I can't think of any reason not to have these in 5.11. We haven't started staging things in the kselftest/kunit branch for 5.11 yet, though. Patch 2 will probably need to be acked by Ted for ext4 first. Brendan, Shuah: can you make sure this doesn't get lost in patchwork? Cheers, -- David > > -- David > > Thanks, > -- Marco
Re: [PATCH 3/3] kunit: tool: move kunitconfig parsing into __init__
On Fri, Dec 4, 2020 at 3:41 AM Daniel Latypov wrote: > > LinuxSourceTree will unceremoniously crash if the user doesn't call > read_kunitconfig() first in a number of functions. This patch seems to partly be reverting the changes here, right: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/tools/testing/kunit?h=kunit&id=fcdb0bc08ced274078f371e1e0fe6421a97fa9f2 (That patch moved the reading of kunitconfig out of __init__) My overall concern is that, really, there are some operations that shouldn't need a kunitconfig (even if they do at the moment), so we'd ideally want at least some of the operations currently under LinuxSourceTree to be able to be run without first reading a kunitconfig. Most notably, it'd be nice if kunit.py exec (and hence LinuxSourceTree::run_kernel()) didn't need a kunitconfig, as the kernel ought to already be built at this point. Now, this is all a little bit hypothetical, as we haven't bothered to make kunit.py exec work without a kunitconfig thus far, but I'm a touch hesitant to make it harder to bypass the kunitconfig reading anyway. > > Adn currently every place we create an instance, the caller also calls > create_kunitconfig() and read_kunitconfig(). > > Move these instead into __init__() so they can't be forgotten and to > reduce copy-paste. This seems to now be missing the create_kunitconfig() stuff (see below). > > The https://github.com/google/pytype type-checker complained that > _config wasn't initialized. With this, kunit_tool now type checks > under both pytype and mypy. > > Signed-off-by: Daniel Latypov > --- > tools/testing/kunit/kunit.py| 20 > tools/testing/kunit/kunit_kernel.py | 19 +++ > 2 files changed, 11 insertions(+), 28 deletions(-) > > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py > index 08951a114654..b58fb3733cfa 100755 > --- a/tools/testing/kunit/kunit.py > +++ b/tools/testing/kunit/kunit.py > @@ -256,10 +256,7 @@ def main(argv, linux=None): > os.mkdir(cli_args.build_dir) > > if not linux: > - linux = kunit_kernel.LinuxSourceTree() > - > - linux.create_kunitconfig(cli_args.build_dir) > - linux.read_kunitconfig(cli_args.build_dir) > + linux = > kunit_kernel.LinuxSourceTree(cli_args.build_dir) > > request = KunitRequest(cli_args.raw_output, >cli_args.timeout, > @@ -277,10 +274,7 @@ def main(argv, linux=None): > os.mkdir(cli_args.build_dir) > > if not linux: > - linux = kunit_kernel.LinuxSourceTree() > - > - linux.create_kunitconfig(cli_args.build_dir) > - linux.read_kunitconfig(cli_args.build_dir) > + linux = > kunit_kernel.LinuxSourceTree(cli_args.build_dir) > > request = KunitConfigRequest(cli_args.build_dir, > cli_args.make_options) > @@ -292,10 +286,7 @@ def main(argv, linux=None): > sys.exit(1) > elif cli_args.subcommand == 'build': > if not linux: > - linux = kunit_kernel.LinuxSourceTree() > - > - linux.create_kunitconfig(cli_args.build_dir) > - linux.read_kunitconfig(cli_args.build_dir) > + linux = > kunit_kernel.LinuxSourceTree(cli_args.build_dir) > > request = KunitBuildRequest(cli_args.jobs, > cli_args.build_dir, > @@ -309,10 +300,7 @@ def main(argv, linux=None): > sys.exit(1) > elif cli_args.subcommand == 'exec': > if not linux: > - linux = kunit_kernel.LinuxSourceTree() > - > - linux.create_kunitconfig(cli_args.build_dir) > - linux.read_kunitconfig(cli_args.build_dir) > + linux = > kunit_kernel.LinuxSourceTree(cli_args.build_dir) > > exec_request = KunitExecRequest(cli_args.timeout, > cli_args.build_dir, > diff --git a/tools/testing/kunit/kunit_kernel.py > b/tools/testing/kunit/kunit_kernel.py > index bda7c4fd4d3e..79793031d2c4 100644 > --- a/tools/testing/kunit/kunit_kernel.py > +++ b/tools/testing/kunit/kunit_kernel.py > @@ -129,10 +129,15 @@ def get_outfile_path(build_dir) -> str: > class LinuxSourceTree(object): > """Represents a Linux kernel source tree with KUnit tests.""" > > - def __init__(self) -> None: > - self._ops = LinuxSourceTreeOperations() > + def __init__(self, build_dir: str, > defconfig=DEFAULT_KUNITCONFIG_PATH) -> None: > signal.signal(signal.SIGINT, self.signal_handler) > > + self._ops = LinuxSourceTreeOperations() > + > + kunitconfig_pat
Re: [PATCH 2/3] kunit: tool: fix minor typing issue with None status
On Fri, Dec 4, 2020 at 3:41 AM Daniel Latypov wrote: > This seems good to me, but I have a few questions, particularly around the description. > The code to handle aggregating statuses didn't check that the status > actually got set. I don't understand what you mean here. Does this refer to Test{Case,Suite}::status potentially being None, and that not being supported properly in bubble_up_{suite_,test_case_,}errors(), or something else? Either way, I can't see any additional code to "check" that the status has been set. As far as I can tell everything except the default to SUCCESS is a no-op, or am I missing something? > Default the value to SUCCESS. I'm a little iffy about defaulting this to success, but I think it's okay for now: the skip test support will eventually change this to a SKIPPED value. > > This sorta follows the precedent in commit 3fc48259d525 ("kunit: Don't > fail test suites if one of them is empty"). > > Also slightly simplify the code and add type annotations. > > Signed-off-by: Daniel Latypov > --- Otherwise, the actual code changes all seem sensible, and it worked fine when I tested it, so: Reviewed-by: David Gow -- David
Re: [PATCH 1/3] kunit: tool: surface and address more typing issues
On Fri, Dec 4, 2020 at 3:41 AM Daniel Latypov wrote: > > The authors of this tool were more familiar with a different > type-checker, https://github.com/google/pytype. > > That's open source, but mypy seems more prevalent (and runs faster). > And unlike pytype, mypy doesn't try to infer types so it doesn't check > unanotated functions. > > So annotate ~all functions in kunit tool to increase type-checking > coverage. > Note: per https://www.python.org/dev/peps/pep-0484/, `__init__()` should > be annotated as `-> None`. > > Doing so makes mypy discover a number of new violations. > Exclude main() since we reuse `request` for the different types of > requests, which mypy isn't happy about. > > This commit fixes all but one error, where `TestSuite.status` might be > None. > > Signed-off-by: Daniel Latypov > --- This looks good to me: I gave it some quick testing, and reading through it, all of the changes seem sensible. I wasn't able to get pytype running here, but mypy worked fine. Reviewed-by: David Gow -- David
Re: [PATCH 3/3] kunit: tool: move kunitconfig parsing into __init__
On Sat, Dec 5, 2020 at 2:18 AM Daniel Latypov wrote: > > On Thu, Dec 3, 2020 at 7:57 PM David Gow wrote: > > > > On Fri, Dec 4, 2020 at 3:41 AM Daniel Latypov wrote: > > > > > > LinuxSourceTree will unceremoniously crash if the user doesn't call > > > read_kunitconfig() first in a number of functions. > > > > This patch seems to partly be reverting the changes here, right: > > https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/tools/testing/kunit?h=kunit&id=fcdb0bc08ced274078f371e1e0fe6421a97fa9f2 > > (That patch moved the reading of kunitconfig out of __init__) > > Yes. > > > > > My overall concern is that, really, there are some operations that > > shouldn't need a kunitconfig (even if they do at the moment), so we'd > > ideally want at least some of the operations currently under > > LinuxSourceTree to be able to be run without first reading a > > kunitconfig. Most notably, it'd be nice if kunit.py exec (and hence > > LinuxSourceTree::run_kernel()) didn't need a kunitconfig, as the > > kernel ought to already be built at this point. > > > > Now, this is all a little bit hypothetical, as we haven't bothered to > > make kunit.py exec work without a kunitconfig thus far, but I'm a > > touch hesitant to make it harder to bypass the kunitconfig reading > > anyway. > > Fair point. > > So one alternative to this to make type-checkers happy is to declare > _config instead of sneakily setting it in some random later method. > Then in all the places that rely on _config, we'd need to add in > checks that it's in fact set to give a better error message (so it's > clear to the user that it's an internal tool bug and has nothing to do > with them). This seems plausible, if a bit verbose. > > The copy-paste of create+read_kunitconfig() is annoying, which is why > I went with this. Personally, the duplication of calls to {create,read}_kunitconfig() doesn't bother me, but I definitely can see the advantage of having the type system pick up when we've missed one. > How about __init__ takes an optional argument that can disable this parsing? This would be okay: I'm starting to feel that really, the ultimate solution is either to split LinuxSourceTree up (and have separate things for configuring, building, and running the kernel), or to pass the kconfig stuff into just the functions that require it. But that is a much more serious refactor, which I haven't fully thought through, and I don't want to let the perfect be the enemy of the good here. > > E.g. > > def __init__(kconfig = None): >if kconfig is not None: > self._config = kconfig >else: > // create and read > What would the kconfig argument here be? Just an empty Kconfig()? I'm not a huge fan of passing a "None" kconfig object when we want to load a config, and a non-None one when we want an empty one: that seems confusingly backwards. Maybe it'd be possible to move the loading of the kunitconfig outside LinuxSourceTree, and pass that (or an empty one) as needed? > Or if we don't like the idea of requiring users who don't want a > kconfig to pass in a dummy, > > def __init__(load_kconfig=True): >if not load_kconfig: > self._config = None >... > I slightly prefer this, for the reasons above: True/False makes more sense than None/Kconfig(). > > > > > > > > Adn currently every place we create an instance, the caller also calls > > > create_kunitconfig() and read_kunitconfig(). > > > > > > Move these instead into __init__() so they can't be forgotten and to > > > reduce copy-paste. > > > > This seems to now be missing the create_kunitconfig() stuff (see below). > > Ah good catch. Completely unintentional. > I'm sure I had the create_kunitconfig() stuff in __init__() at some > point but must have inadvertently removed it somehow later on. > > > > > > > The https://github.com/google/pytype type-checker complained that > > > _config wasn't initialized. With this, kunit_tool now type checks > > > under both pytype and mypy. > > > > > > Signed-off-by: Daniel Latypov > > > --- Okay, so it looks like there are a few options with _kconfig: 1. Check for None everywhere (after explicitly setting it in the constructor). Pros: Nicer error messages, doesn't require other changes, Cons: verbose, still somewhat prone to error (could forget {create,read}_kunitconfig()) 2. Pass a Kconfig object into the constructor. Pros: a kconfig must exist, so less error prone, Cons: if we allow passing
Re: [PATCH 2/5] kunit: tool: fix unit test so it can run from non-root dir
On Wed, Dec 2, 2020 at 3:00 AM Daniel Latypov wrote: > > On Mon, Nov 30, 2020 at 11:33 PM David Gow wrote: > > > > On Tue, Dec 1, 2020 at 7:33 AM Daniel Latypov wrote: > > > > > > get_absolute_path() makes an attempt to allow for this. > > > But that doesn't work as soon as os.chdir() gets called. > > > > Can we explain why this doesn't work? It's because the test_data/ > > files are accessed with relative paths, so chdir breaks access to > > them, right? > > Correct. > Because it actually returns a relative path. > > (I forgot that I called out that get_absolute_path() gives relative > paths in the last patch, and not this one. I can update the commit > desc if we want a v2 of this) > > > > > > > > > So make it so that os.chdir() does nothing to avoid this. > > > > > > Note: mock.patch.object() doesn't seem to work in setUpModule(), hence > > > the introduction of a new base class instead. > > > > > > Fixes: 5578d008d9e0 ("kunit: tool: fix running kunit_tool from outside > > > kernel tree") > > > Signed-off-by: Daniel Latypov > > > --- > > > > I don't like this: disabling a real system call seems like overkill to > > work around a path issue like this. > > > > Wouldn't it be better to either: > > (a) stop kunit_tool from needing to chdir entirely; or > > a) is doable, but would require plumbing cwd=get_kernel_root_path() to > all the subprocess calls to make, etc. > I'm not sure fixing a internal test-only issue necessarily justifies > taking that path instead of the easier "just add a chdir" we opted for > in 5578d008d9e0 ("kunit: tool: fix running kunit_tool from outside > kernel tree"). > > > (b) have get_absolute_path() / test_data_path() produce an absolute path. > > > > The latter really seems like the most sensible approach: have the > > script read its own path and read files relative to that. > > b) is not that simple, sadly. > > Say I invoke > $ python3 kunit_tool_test.py > then __file__ = kunit_tool_test.py. > > So __file__ is a relative path, but the code assumed it was an > absolute one and any change of directory breaks things. > Working around that would require something like caching the result of > os.path.abspath(__file__) somewhere. So, to clarify, __file__ is a relative path based on the cwd when the script is initially run, right? In any case, caching the result of os.path.abspath(__file__) seems like the most sensible solution to me. There's global state anyway (the current directory), we might as well have it in an explicit variable, IMHO. > > I can see the point about not mocking out something like os.chdir(). > But on the other hand, do we have any other legitimate reason to call > it besides that one place in kunit.py? > If we do have something that relies on doing an os.chdir(), it should > ideally notice that it didn't work and manifest in a unit test failure > somehow. Certainly there doesn't seem to be any other need to chdir() in kunit_tool at the moment, but I could see us doing so when adding other features. More to the point, if both kunit.py and kunit_tool_test.py rely on or manipulate the current directory as part of their state, that seems like it's asking for some trouble down the line. If we use an absolute path for the test data, that's something that seems unlikely to ever need further changes or cause issues. > > Alternatively, we could make get_kernel_root_path() return ""/None to > avoid the chdir call. > kunit.py: if get_kernel_root_path(): > kunit.py: os.chdir(get_kernel_root_path()) > > There'd be no adverse affects atm for stubbing that out, and I don't forsee > any. > But if we want to be even safer, then perhaps we have > > def chdir_to_kernel_root(): >... > > and mock out just that func in the unit test? I'd be okay with this, even if I'd prefer us to use an absolute path for the test_data as well. Having something like this might even give us the opportunity to verify that we're actually trying to change to the kernel directory in cases where we need to, but that seems like it's out-of-scope for a simple fix. -- David
Re: [PATCH v3] lib: Convert test_hexdump.c to KUnit
On Wed, Dec 2, 2020 at 6:06 PM Andy Shevchenko wrote: > > On Wed, Dec 02, 2020 at 09:51:19AM +0530, Arpitha Raghunandan wrote: > > On 01/12/20 4:36 pm, Andy Shevchenko wrote: > > > On Tue, Dec 1, 2020 at 9:21 AM Arpitha Raghunandan <98.a...@gmail.com> > > > wrote: > > ... > > > >> I ran both the original and converted tests as is to produce the > > >> output for success of the test in the two cases. I also ran these > > >> tests with a small modification to show the difference in the output > > >> for failure of the test in both cases. The modification I made is: > > >> static const char * const test_data_4_le[] __initconst = { > > >> - "7bdb32be", "b293180a", "24c4ba70", "9b34837d", > > >> + "7bdb32be", "b293180a", "24c4ba70", "9b3483d", > > >> > > >> The difference in outputs can be seen here: > > >> https://gist.github.com/arpi-r/38f53a3c65692bf684a6bf3453884b6e > > > > > > Looks pretty much good, what I'm sad to see is the absence of the test > > > statistics. Any ideas if we can get it back? > > > > > > > I could again include variable total_tests as was in the original test. > > Would that be fine? > > What I;m talking about is the output. How it will be implemented (using the > same variable or differently) is up to you. So the point is I want to see the > statistics of success/total at the end. > > I think this should be done in KUNIT rather than in the individual test cases. I tend to agree here that this really is something for KUnit. At the moment, the tools/testing/kunit/kunit.py script will parse the kernel log and generate these sorts of statistics. I know that needing to run it through a script might seem like a step backwards, but there's no formal place for statistics in the KTAP specification[1] being worked on to standardise kselftest/kunit output formats. Note that there are other parsers for TAP-like formats which are being used with KUnit results, so systems like LAVA could also sum up these statistics. It's also possible, as Arpitha alluded to, to have the test dump them out as a comment. This won't actually work for this test as-is, though, as the KUnit version is running as a single giant test case (so KUnit believes that 1/1 tests have passed, rather than having any more-detailed statistics). It looks like there are a few ways to split it up a bit which would make it neater (a test each for the for() loops in test_hexdump_init() seems sensible to me), but at the moment, there's not really a way of programmatically generating test cases which KUnit then counts The "Parameterised Tests"[2] work Arpitha has been working on ought to go some way to helping here, though it won't solve this completely in this initial version. The problem there is that parameterised tests are not reported individually in a way the kunit.py parser can report cleanly, yet, so it'll still only be counted as one test until that's changed (though, at least, that shouldn't require any test-specific work). My suggestion for the ultimate state of the test would be: - Split up the test into separate KUnit tests for the different "categories" of tests: (e.g., test_hexdump_set, test_hexdump_overflow_set_ascii, etc) - Replace the for loops in test_hexdump_init() with parameters, so that KUnit is aware of the original runs. - Once KUnit and the tooling supports it, these will be reported as subtests. (In the meantime, the results will be listed individually, commented out) Of course, it'll take a while before all of those KUnit pieces are in place. I personally think that a good compromise would be to just do the first of these for now, which would make kunit_tool give at least a 4/4 rather than 1/1 result. Then, once the parameterised testing work is merged (and perhaps the tooling fixes are finished), the tests could be updated to take advantage of that. Cheres, -- David [1]: https://lore.kernel.org/linux-kselftest/cy4pr13mb1175b804e31e502221bc8163fd...@cy4pr13mb1175.namprd13.prod.outlook.com/T/ [2]: https://lore.kernel.org/linux-kselftest/20201116054035.211498-1-98.a...@gmail.com/
Re: [PATCH v3] lib: Convert test_hexdump.c to KUnit
On Wed, Dec 2, 2020 at 8:21 PM Andy Shevchenko wrote: > > On Wed, Dec 2, 2020 at 1:57 PM David Gow wrote: > > On Wed, Dec 2, 2020 at 6:06 PM Andy Shevchenko > > wrote: > > > On Wed, Dec 02, 2020 at 09:51:19AM +0530, Arpitha Raghunandan wrote: > > ... > > > > What I;m talking about is the output. How it will be implemented (using > > > the > > > same variable or differently) is up to you. So the point is I want to see > > > the > > > statistics of success/total at the end. > > > > > > I think this should be done in KUNIT rather than in the individual test > > > cases. > > > > I tend to agree here that this really is something for KUnit. At the > > moment, the tools/testing/kunit/kunit.py script will parse the kernel > > log and generate these sorts of statistics. I know that needing to run > > it through a script might seem like a step backwards, but there's no > > formal place for statistics in the KTAP specification[1] being worked > > on to standardise kselftest/kunit output formats. > > Then it sucks. Fix specification (in a long term) and does it have a > comment style of messages that we can have this statistics printed > (but maybe not parsed)? I should clarify: there's nothing in the spec which explicitly defines a place for such statistics (nor anything which requires them). There are "diagnostic" lines which are not parsed, and so it'd be possible to output statistics there. KUnit itself doesn't at present, but allows individual tests to log diagnostic lines which could be such statistics, particularly in cases like this where the full structure of the tests aren't quite exposed to the framework. > > Note that there are > > other parsers for TAP-like formats which are being used with KUnit > > results, so systems like LAVA could also sum up these statistics. It's > > also possible, as Arpitha alluded to, to have the test dump them out > > as a comment. > > Fine to me. > > > This won't actually work for this test as-is, though, as the KUnit > > version is running as a single giant test case (so KUnit believes that > > 1/1 tests have passed, rather than having any more-detailed > > statistics). It looks like there are a few ways to split it up a bit > > which would make it neater (a test each for the for() loops in > > test_hexdump_init() seems sensible to me), but at the moment, there's > > not really a way of programmatically generating test cases which KUnit > > then counts > > Fix it, please. We rely on this statistics pretty much. The hope is that the Parameterised Test feature will make this possible (though, as mentioned, there are a few other issues around then making those statistics available, but we should be able to work through those). It may be a silly question, but what is it that makes these statistics useful in this test? Maybe I'm misunderstanding something, but I'd've thought that the important things were whether or not _all_ tests had passed, and -- if not --- _which_ ones had failed. Is the count of failing cases within a test like this really that useful for debugging, or is it more for comparing against different versions? Either way, we'll try to make sure they're available. > > The "Parameterised Tests"[2] work Arpitha has been working on ought to > > go some way to helping here, though it won't solve this completely in > > this initial version. The problem there is that parameterised tests > > are not reported individually in a way the kunit.py parser can report > > cleanly, yet, so it'll still only be counted as one test until that's > > changed (though, at least, that shouldn't require any test-specific > > work). > > > > My suggestion for the ultimate state of the test would be: > > - Split up the test into separate KUnit tests for the different > > "categories" of tests: (e.g., test_hexdump_set, > > test_hexdump_overflow_set_ascii, etc) > > - Replace the for loops in test_hexdump_init() with parameters, so > > that KUnit is aware of the original runs. > > - Once KUnit and the tooling supports it, these will be reported as > > subtests. (In the meantime, the results will be listed individually, > > commented out) > > I'm fine as long as we have this information printed to the user. Okay -- Arpitha: does this seem like a sensible approach to you? If printing it to the kernel log is really essential, I'll have a look into how we can do that in KUnit. > > Of course, it'll take a while before all of those KUnit pieces are in > > place. I personally thi
Re: [PATCH v2 1/4] kunit: tool: fix unit test cleanup handling
On Thu, Dec 3, 2020 at 3:09 AM Daniel Latypov wrote: > > * Stop leaking file objects. > * Use self.addCleanup() to ensure we call cleanup functions even if > setUp() fails. > * use mock.patch.stopall instead of more error-prone manual approach > > Signed-off-by: Daniel Latypov > --- This patch hasn't changed since v1, right? It's still: Reviewed-by: David Gow Cheers, -- David
Re: [PATCH v2 3/4] kunit: tool: use `with open()` in unit test
On Thu, Dec 3, 2020 at 3:09 AM Daniel Latypov wrote: > > The use of manual open() and .close() calls seems to be an attempt to > keep the contents in scope. > But Python doesn't restrict variables like that, so we can introduce new > variables inside of a `with` and use them outside. > > Do so to make the code more Pythonic. > > Signed-off-by: Daniel Latypov > --- Reviewed-by: David Gow Cheers, -- David
Re: [PATCH v2 4/4] minor: kunit: tool: fix unit test so it can run from non-root dir
On Thu, Dec 3, 2020 at 3:09 AM Daniel Latypov wrote: > > Also take this time to rename get_absolute_path() to test_data_path(). > > 1. the name is currently a lie. It gives relative paths, e.g. if I run > from the same dir as the test file, it gives './test_data/' > > See https://docs.python.org/3/reference/import.html#__file__, which > doesn't stipulate that implementations provide absolute paths. > > 2. it's only used for generating paths to tools/testing/kunit/test_data/ > So we can tersen things by making it less general. > > Cache the absolute path to the test data files per suggestion from [1]. > Using relative paths, the tests break because of this code in kunit.py > if get_kernel_root_path(): > os.chdir(get_kernel_root_path()) > > [1] > https://lore.kernel.org/linux-kselftest/cabvgosnh0gz7z5jhrcgyg1wg0zddbtlosucob-gwmexlgvt...@mail.gmail.com/ > > Fixes: 5578d008d9e0 ("kunit: tool: fix running kunit_tool from outside kernel > tree") > Signed-off-by: Daniel Latypov > --- Thanks: I much prefer this to v1. Having it work the same way as test_tmpdir is a bonus. Reviewed-by: David Gow Cheers, -- David
Re: [PATCH v2 2/4] kunit: tool: stop using bare asserts in unit test
On Thu, Dec 3, 2020 at 3:09 AM Daniel Latypov wrote: > > Use self.assertEqual/assertNotEqual() instead. > Besides being more appropriate in a unit test, it'll also give a better > error message by show the unexpected values. > > Also > * Delete redundant check of exception types. self.assertRaises does this. > * s/kall/call. There's no reason to name it this way. > * This is probably a misunderstanding from the docs which uses it > since `mock.call` is in scope as `call`. > > Signed-off-by: Daniel Latypov > --- Looks good, thanks! Reviewed-by: David Gow -- David
Re: [PATCH 1/2] kcsan: Make test follow KUnit style recommendations
On Thu, Jan 14, 2021 at 12:06 AM Marco Elver wrote: > > Per recently added KUnit style recommendations at > Documentation/dev-tools/kunit/style.rst, make the following changes to > the KCSAN test: > > 1. Rename 'kcsan-test.c' to 'kcsan_test.c'. > > 2. Rename suite name 'kcsan-test' to 'kcsan'. > > 3. Rename CONFIG_KCSAN_TEST to CONFIG_KCSAN_KUNIT_TEST and >default to KUNIT_ALL_TESTS. > > Cc: David Gow > Signed-off-by: Marco Elver Thanks very much -- it's great to see the naming guidelines starting to be picked up. I also tested the KUNIT_ALL_TESTS config option w/ KCSAN enabled, and it worked a treat. My only note is that we've had some problems[1] with mm-related changes which rename files getting corrupted at some point before reaching Linus, so it's probably worth keeping a close eye on this change to make sure nothing goes wrong. Reviewed-by: David Gow -- David [1]: https://www.spinics.net/lists/linux-mm/msg239149.html
[PATCH] soc: litex: Properly depend on HAS_IOMEM
The LiteX SOC controller driver makes use of IOMEM functions like devm_platform_ioremap_resource(), which are only available if CONFIG_HAS_IOMEM is defined. This causes the driver not to be enable under make ARCH=um allyesconfig, even though it won't build. By adding a dependency on HAS_IOMEM, the driver will not be enabled on architectures which don't support it. Fixes: 22447a99c97e ("drivers/soc/litex: add LiteX SoC Controller driver") Signed-off-by: David Gow --- drivers/soc/litex/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig index 7c6b009b6f6c..7a7c38282e11 100644 --- a/drivers/soc/litex/Kconfig +++ b/drivers/soc/litex/Kconfig @@ -8,6 +8,7 @@ config LITEX config LITEX_SOC_CONTROLLER tristate "Enable LiteX SoC Controller driver" depends on OF || COMPILE_TEST + depends on HAS_IOMEM select LITEX help This option enables the SoC Controller Driver which verifies -- 2.30.0.280.ga3ce27912f-goog
[PATCH] i3c/master/mipi-i3c-hci: Specify HAS_IOMEM dependency
The MIPI i3c HCI driver makes use of IOMEM functions like devm_platform_ioremap_resource(), which are only available if CONFIG_HAS_IOMEM is defined. This causes the driver to be enabled under make ARCH=um allyesconfig, even though it won't build. By adding a dependency on HAS_IOMEM, the driver will not be enabled on architectures which don't support it. Fixes: 9ad9a52cce28 ("i3c/master: introduce the mipi-i3c-hci driver") Signed-off-by: David Gow --- drivers/i3c/master/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/i3c/master/Kconfig b/drivers/i3c/master/Kconfig index e68f15f4b4d0..afff0e2320f7 100644 --- a/drivers/i3c/master/Kconfig +++ b/drivers/i3c/master/Kconfig @@ -25,6 +25,7 @@ config DW_I3C_MASTER config MIPI_I3C_HCI tristate "MIPI I3C Host Controller Interface driver (EXPERIMENTAL)" depends on I3C + depends on HAS_IOMEM help Support for hardware following the MIPI Aliance's I3C Host Controller Interface specification. -- 2.30.0.280.ga3ce27912f-goog
[PATCH] drivers: rtc: Make xilinx zynqmp driver depend on HAS_IOMEM
The Xilinx zynqmp RTC driver makes use of IOMEM functions like devm_platform_ioremap_resource(), which are only available if CONFIG_HAS_IOMEM is defined. This causes the driver not to be enable under make ARCH=um allyesconfig, even though it won't build. By adding a dependency on HAS_IOMEM, the driver will not be enabled on architectures which don't support it. Fixes: 09ef18bcd5ac ("rtc: use devm_platform_ioremap_resource() to simplify code") Signed-off-by: David Gow --- drivers/rtc/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig index 6123f9f4fbc9..474d95184f20 100644 --- a/drivers/rtc/Kconfig +++ b/drivers/rtc/Kconfig @@ -1300,7 +1300,7 @@ config RTC_DRV_OPAL config RTC_DRV_ZYNQMP tristate "Xilinx Zynq Ultrascale+ MPSoC RTC" - depends on OF + depends on OF && HAS_IOMEM help If you say yes here you get support for the RTC controller found on Xilinx Zynq Ultrascale+ MPSoC. -- 2.30.0.280.ga3ce27912f-goog
Re: [PATCH 2/2] kcsan: Switch to KUNIT_CASE_PARAM for parameterized tests
On Thu, Jan 14, 2021 at 12:06 AM Marco Elver wrote: > > Since KUnit now support parameterized tests via KUNIT_CASE_PARAM, update > KCSAN's test to switch to it for parameterized tests. This simplifies > parameterized tests and gets rid of the "parameters in case name" > workaround (hack). > > At the same time, we can increase the maximum number of threads used, > because on systems with too few CPUs, KUnit allows us to now stop at the > maximum useful threads and not unnecessarily execute redundant test > cases with (the same) limited threads as had been the case before. > > Cc: David Gow > Signed-off-by: Marco Elver > --- Thanks! This looks great from the KUnit point of view: I'm particularly excited to see a use of the parameterised test generator that's not just reading from an array. I tested this as well, and it all seemed to work fine for me. Reviewed-by: David Gow Cheers, -- David
Re: [PATCH] soc: litex: Properly depend on HAS_IOMEM
On Wed, Jan 27, 2021 at 8:27 PM Stafford Horne wrote: > > On Tue, Jan 26, 2021 at 07:36:04PM -0800, David Gow wrote: > > The LiteX SOC controller driver makes use of IOMEM functions like > > devm_platform_ioremap_resource(), which are only available if > > CONFIG_HAS_IOMEM is defined. > > > > This causes the driver not to be enable under make ARCH=um allyesconfig, > > even though it won't build. > > Is this wording correct? I suspect it causes to driver TO BE enabled. > Whoops! Yes: that should be "causes the driver to be enabled" -- sorry about that. Let me know if you want me to re-send it with the fixed wording, or if you just want to fix that yourself. > > > > By adding a dependency on HAS_IOMEM, the driver will not be enabled on > > architectures which don't support it. > > > > Fixes: 22447a99c97e ("drivers/soc/litex: add LiteX SoC Controller driver") > > Signed-off-by: David Gow a > > This looks ok to me. I can queue it for 5.11 fixes, if you can help with the > wording above. Thanks: that'd be great! Cheers, -- David > > -Stafford > > > --- > > drivers/soc/litex/Kconfig | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig > > index 7c6b009b6f6c..7a7c38282e11 100644 > > --- a/drivers/soc/litex/Kconfig > > +++ b/drivers/soc/litex/Kconfig > > @@ -8,6 +8,7 @@ config LITEX > > config LITEX_SOC_CONTROLLER > > tristate "Enable LiteX SoC Controller driver" > > depends on OF || COMPILE_TEST > > + depends on HAS_IOMEM > > select LITEX > > help > > This option enables the SoC Controller Driver which verifies > > -- > > 2.30.0.280.ga3ce27912f-goog > >
Re: [PATCH] kunit: tool: Force the use of the 'tty' console for UML
On Wed, Jan 6, 2021 at 3:52 AM Shuah Khan wrote: > > On 1/5/21 11:57 AM, Andy Shevchenko wrote: > > On Tue, Jan 05, 2021 at 09:34:33AM -0700, Shuah Khan wrote: > >> On 1/5/21 9:21 AM, Petr Mladek wrote: > >>> On Mon 2021-01-04 09:23:57, Shuah Khan wrote: > >>>> On 12/22/20 4:11 AM, Andy Shevchenko wrote: > >>>>> On Mon, Dec 21, 2020 at 11:39:00PM -0800, David Gow wrote: > >>>>>> kunit_tool relies on the UML console outputting printk() output to the > >>>>>> tty in order to get results. Since the default console driver could > >>>>>> change, pass 'console=tty' to the kernel. > >>>>>> > >>>>>> This is triggered by a change[1] to use ttynull as a fallback console > >>>>>> driver which -- by chance or by design -- seems to have changed the > >>>>>> default console output on UML, breaking kunit_tool. While this may be > >>>>>> fixed, we should be less fragile to such changes in the default. > >>>>>> > >>>>>> [1]: > >>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=757055ae8dedf5333af17b3b5b4b70ba9bc9da4e > >>>>> > >>>>> Reported-by: Andy Shevchenko > >>>>> Tested-by: Andy Shevchenko > >>>>> > >>>> > >>>> Thank you all. Now in linux-kselftest kunit-fixes branch. > >>>> > >>>> Will send this up for rc3. > >>>> > >>>> Sorry for the delay - have been away from the keyboard for a > >>>> bit. > >>> > >>> JFYI, I am not sure that this is the right solution. I am > >>> looking into it, see > >>> https://lore.kernel.org/linux-kselftest/X%2FSRA1P8t+ONZFKb@alley/ > >>> for more details. > >>> > >> > >> Thanks Petr. I will hold off on sending the patch up to Linus and > >> let you find a the right solution. > > > > Please. leave it in Linux Next at least. Otherwise kunit will be broken for > > a > > long time which is not good. > > > > > > Yes. That is the plan. It will be in there until real fix comes in. > Thanks, Shuah. Personally, I think that this patch makes some sense to keep even if the underlying issue with ttynull is resolved. Given that kunit.py requires the console output, explicitly stating we want console=tty set is probably worth doing rather than relying on it being the default. That being said, I definitely agree that this patch doesn't fix the underlying issue with UML/ttynull: it just makes the kunit.py script less sensitive to such changes (which, while unlikely, could potentially occur legitimately down the track). Cheers, -- David
Re: [PATCH] Documentation: kunit: include example of a parameterized test
On Wed, Dec 16, 2020 at 8:23 AM Daniel Latypov wrote: > > Commit fadb08e7c750 ("kunit: Support for Parameterized Testing") > introduced support but lacks documentation for how to use it. > > This patch builds on commit 1f0e943df68a ("Documentation: kunit: provide > guidance for testing many inputs") to show a minimal example of the new > feature. > > Signed-off-by: Daniel Latypov > --- This looks good to me. Thanks! Reviewed-by: David Gow -- David smime.p7s Description: S/MIME Cryptographic Signature
Re: [RFC PATCH] bpf: preload: Fix build error when O= is set
On Wed, Dec 16, 2020 at 10:53 PM Quentin Monnet wrote: > > 2020-11-21 17:48 UTC+0800 ~ David Gow > > On Sat, Nov 21, 2020 at 3:38 PM Andrii Nakryiko > > wrote: > >> > >> On Thu, Nov 19, 2020 at 12:51 AM David Gow wrote: > >>> > >>> If BPF_PRELOAD is enabled, and an out-of-tree build is requested with > >>> make O=, compilation seems to fail with: > >>> > >>> tools/scripts/Makefile.include:4: *** O=.kunit does not exist. Stop. > >>> make[4]: *** [../kernel/bpf/preload/Makefile:8: > >>> kernel/bpf/preload/libbpf.a] Error 2 > >>> make[3]: *** [../scripts/Makefile.build:500: kernel/bpf/preload] Error 2 > >>> make[2]: *** [../scripts/Makefile.build:500: kernel/bpf] Error 2 > >>> make[2]: *** Waiting for unfinished jobs > >>> make[1]: *** [.../Makefile:1799: kernel] Error 2 > >>> make[1]: *** Waiting for unfinished jobs > >>> make: *** [Makefile:185: __sub-make] Error 2 > >>> > >>> By the looks of things, this is because the (relative path) O= passed on > >>> the command line is being passed to the libbpf Makefile, which then > >>> can't find the directory. Given OUTPUT= is being passed anyway, we can > >>> work around this by explicitly setting an empty O=, which will be > >>> ignored in favour of OUTPUT= in tools/scripts/Makefile.include. > >> > >> Strange, but I can't repro it. I use make O= all the > >> time with no issues. I just tried specifically with a make O=.build, > >> where .build is inside Linux repo, and it still worked fine. See also > >> be40920fbf10 ("tools: Let O= makes handle a relative path with -C > >> option") which was supposed to address such an issue. So I'm wondering > >> what exactly is causing this problem. > >> > > [+ linux-um list] > > > > Hmm... From a quick check, I can't reproduce this on x86, so it's > > possibly a UML-specific issue. > > > > The problem here seems to be that $PWD is, for whatever reason, equal > > to the srcdir on x86, but not on UML. In general, $PWD behaves pretty > > weirdly -- I don't fully understand it -- but if I add a tactical "PWD > > := $(shell pwd)" or use $(CURDIR) instead, the issue shows up on x86 > > as well. I guess this is because PWD only gets updated when set by a > > shell or something, and UML does this somewhere? > > > > Thoughts? > > > > Cheers, > > -- David > > Hi David, Andrii, > > David, did you use a different command for building for UML and x86? I'm > asking because I reproduce on x86, but only for some targets, in > particular when I tried bindeb-pkg. I just ran "make ARCH={x86,um} O=.bpftest", with defconfig + enabling BPF_PRELOAD and its dependencies. UML fails, x86 works. (Though I can reproduce the failure if I make bindeb-pkg on x86). (It also shows up when building UML with the allyesconfig-based KUnit alltests option by running "./tools/testing/kunit/kunit.py run --alltests", though this understandably takes a long time and is less obvious) > > With "make O=.build vmlinux", I have: > - $(O) for "dummy" check in tools/scripts/Makefile.include set to > /linux/.build > - $(PWD) for same check set to /linux/tools > - Since $(O) is an absolute path, the "dummy" check passes > > With "make O=.build bindeb-pkg", I have instead: > - $(O) set to .build (relative path) > - $(PWD) set to /linux/.build > - "dummy" check changes to /linux/.build and searches for .build in it, > which fails and aborts the build > > (tools/scripts/Makefile.include is included from libbpf's Makefile, > called from kernel/bpf/preload/Makefile.) > > I'm not sure how exactly the bindeb-pkg target ends up passing these values. Yeah: I haven't been able to find where uml is changing them either: I'm assuming there's something which changes directory and/or spawns a shell/recursive make to change $(PWD) or something. > For what it's worth, I have been solving this (before finding this > thread) with a fix close to yours, I pass "O=$(abspath .)" on the > command line for building libbpf in kernel/bpf/preload/Makefile. It > looked consistent to me with the "tools/:" target from the main > Makefile, where "O=$(abspath $(objtree))" is passed (and $(objtree) is "."). Given that there are several targets being broken here, it's probably worth having a fix like this which overrides O= rather than trying to hunt down every target which could change $(PWD). I don't particularly mind whether we use O= or O=$(abspath .), both are working in the UML usecase as well. Does anyone object to basically accepting either this patch as-is, or using O=$(abspath .)? Cheers, -- David
[PATCH v2] drivers: fpga: Specify HAS_IOMEM dependency for FPGA_DFL
Because dfl.c uses the 'devm_ioremap', 'devm_iounmap', 'devm_ioremap_resource', and 'devm_platform_ioremap_resource' functions, it should depend on HAS_IOMEM. This fixes make allyesconfig under UML (ARCH=um), which doesn't provide HAS_IOMEM. Fixes: 89eb35e810a8 ("fpga: dfl: map feature mmio resources in their own feature drivers") Signed-off-by: David Gow --- Changes since v1: ( https://lore.kernel.org/linux-fpga/20201119082209.3598354-1-david...@google.com/ ) - Add Fixes tag drivers/fpga/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index 7cd5a29fc437..5645226ca3ce 100644 --- a/drivers/fpga/Kconfig +++ b/drivers/fpga/Kconfig @@ -142,6 +142,7 @@ config FPGA_DFL tristate "FPGA Device Feature List (DFL) support" select FPGA_BRIDGE select FPGA_REGION + depends on HAS_IOMEM help Device Feature List (DFL) defines a feature list structure that creates a linked list of feature headers within the MMIO space -- 2.29.2.454.gaff20da3a2-goog
Re: [RFC PATCH] bpf: preload: Fix build error when O= is set
On Sat, Nov 21, 2020 at 3:38 PM Andrii Nakryiko wrote: > > On Thu, Nov 19, 2020 at 12:51 AM David Gow wrote: > > > > If BPF_PRELOAD is enabled, and an out-of-tree build is requested with > > make O=, compilation seems to fail with: > > > > tools/scripts/Makefile.include:4: *** O=.kunit does not exist. Stop. > > make[4]: *** [../kernel/bpf/preload/Makefile:8: > > kernel/bpf/preload/libbpf.a] Error 2 > > make[3]: *** [../scripts/Makefile.build:500: kernel/bpf/preload] Error 2 > > make[2]: *** [../scripts/Makefile.build:500: kernel/bpf] Error 2 > > make[2]: *** Waiting for unfinished jobs > > make[1]: *** [.../Makefile:1799: kernel] Error 2 > > make[1]: *** Waiting for unfinished jobs > > make: *** [Makefile:185: __sub-make] Error 2 > > > > By the looks of things, this is because the (relative path) O= passed on > > the command line is being passed to the libbpf Makefile, which then > > can't find the directory. Given OUTPUT= is being passed anyway, we can > > work around this by explicitly setting an empty O=, which will be > > ignored in favour of OUTPUT= in tools/scripts/Makefile.include. > > Strange, but I can't repro it. I use make O= all the > time with no issues. I just tried specifically with a make O=.build, > where .build is inside Linux repo, and it still worked fine. See also > be40920fbf10 ("tools: Let O= makes handle a relative path with -C > option") which was supposed to address such an issue. So I'm wondering > what exactly is causing this problem. > [+ linux-um list] Hmm... From a quick check, I can't reproduce this on x86, so it's possibly a UML-specific issue. The problem here seems to be that $PWD is, for whatever reason, equal to the srcdir on x86, but not on UML. In general, $PWD behaves pretty weirdly -- I don't fully understand it -- but if I add a tactical "PWD := $(shell pwd)" or use $(CURDIR) instead, the issue shows up on x86 as well. I guess this is because PWD only gets updated when set by a shell or something, and UML does this somewhere? Thoughts? Cheers, -- David > > > > Signed-off-by: David Gow > > --- > > > > Hi all, > > > > I'm not 100% sure this is the correct fix here -- it seems to work for > > me, and makes some sense, but let me know if there's a better way. > > > > One other thing worth noting is that I've been hitting this with > > make allyesconfig on ARCH=um, but there's a comment in the Kconfig > > suggesting that, because BPF_PRELOAD depends on !COMPILE_TEST, that > > maybe it shouldn't be being built at all. I figured that it was worth > > trying to fix this anyway. > > > > Cheers, > > -- David > > > > > > kernel/bpf/preload/Makefile | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/bpf/preload/Makefile b/kernel/bpf/preload/Makefile > > index 23ee310b6eb4..39848d296097 100644 > > --- a/kernel/bpf/preload/Makefile > > +++ b/kernel/bpf/preload/Makefile > > @@ -5,7 +5,7 @@ LIBBPF_A = $(obj)/libbpf.a > > LIBBPF_OUT = $(abspath $(obj)) > > > > $(LIBBPF_A): > > - $(Q)$(MAKE) -C $(LIBBPF_SRCS) OUTPUT=$(LIBBPF_OUT)/ > > $(LIBBPF_OUT)/libbpf.a > > + $(Q)$(MAKE) -C $(LIBBPF_SRCS) O= OUTPUT=$(LIBBPF_OUT)/ > > $(LIBBPF_OUT)/libbpf.a > > > > userccflags += -I $(srctree)/tools/include/ -I > > $(srctree)/tools/include/uapi \ > > -I $(srctree)/tools/lib/ -Wno-unused-result > > -- > > 2.29.2.454.gaff20da3a2-goog > >
[PATCH] lib/list-test: add a test for the 'list' doubly linked list
This change adds a KUnit test for the kernel doubly linked list implementation in include/linux/list.h Note that, at present, it only tests the list_ types (not the singly-linked hlist_), and does not yet test all of the list_for_each_entry* macros (and some related things like list_prepare_entry). This change depends on KUnit, so should be merged via the 'test' branch: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/log/?h=test Signed-off-by: David Gow --- lib/Kconfig.debug | 12 + lib/Makefile | 3 + lib/list-test.c | 711 ++ 3 files changed, 726 insertions(+) create mode 100644 lib/list-test.c diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index a3017a5dadcd..60691c0aac3e 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1961,6 +1961,18 @@ config SYSCTL_KUNIT_TEST If unsure, say N. +config LIST_TEST + bool "KUnit Test for Kernel Linked-list stuctures" + depends on KUNIT + help + This builds the linked list unit test, which runs on boot. + It tests that the API and basic functionality of the list_head type + and associated macros. + For more information on KUnit and unit tests in general please refer + to the KUnit documentation in Documentation/dev-tools/kunit/. + + If unsure, say N. + config TEST_UDELAY tristate "udelay test driver" help diff --git a/lib/Makefile b/lib/Makefile index bba1fd5485f7..309e174ee35d 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -292,3 +292,6 @@ obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o obj-$(CONFIG_OBJAGG) += objagg.o + +# KUnit tests +obj-$(CONFIG_LIST_TEST) += list-test.o diff --git a/lib/list-test.c b/lib/list-test.c new file mode 100644 index ..f333e8b0d9fe --- /dev/null +++ b/lib/list-test.c @@ -0,0 +1,711 @@ +// SPDX-License-Identifier: GPL-2.0 +#include + +#include + +struct list_test_struct { + int data; + struct list_head list; +}; + +static void list_init_test(struct kunit *test) +{ + /* Test the different ways of initialising a list. */ + struct list_head list1 = LIST_HEAD_INIT(list1); + struct list_head list2; + LIST_HEAD(list3); + + INIT_LIST_HEAD(&list2); + + /* list_empty_careful() checks both next and prev. */ + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list1)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list2)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list3)); +} + +static void list_add_test(struct kunit *test) +{ + struct list_head a, b; + LIST_HEAD(list); + + list_add(&a, &list); + list_add(&b, &list); + + /* should be [list] -> b -> a */ + KUNIT_EXPECT_PTR_EQ(test, list.next, &b); + KUNIT_EXPECT_PTR_EQ(test, b.prev, &list); + KUNIT_EXPECT_PTR_EQ(test, b.next, &a); +} + +static void list_add_tail_test(struct kunit *test) +{ + struct list_head a, b; + LIST_HEAD(list); + + list_add_tail(&a, &list); + list_add_tail(&b, &list); + + /* should be [list] -> a -> b */ + KUNIT_EXPECT_PTR_EQ(test, list.next, &a); + KUNIT_EXPECT_PTR_EQ(test, a.prev, &list); + KUNIT_EXPECT_PTR_EQ(test, a.next, &b); +} + +static void list_del_test(struct kunit *test) +{ + struct list_head a, b; + LIST_HEAD(list); + + list_add_tail(&a, &list); + list_add_tail(&b, &list); + + /* before: [list] -> a -> b */ + list_del(&a); + + /* now: [list] -> b */ + KUNIT_EXPECT_PTR_EQ(test, list.next, &b); + KUNIT_EXPECT_PTR_EQ(test, b.prev, &list); +} + +static void list_replace_test(struct kunit *test) +{ + struct list_head a_old, a_new, b; + LIST_HEAD(list); + + list_add_tail(&a_old, &list); + list_add_tail(&b, &list); + + /* before: [list] -> a_old -> b */ + list_replace(&a_old, &a_new); + + /* now: [list] -> a_new -> b */ + KUNIT_EXPECT_PTR_EQ(test, list.next, &a_new); + KUNIT_EXPECT_PTR_EQ(test, b.prev, &a_new); +} + +static void list_replace_init_test(struct kunit *test) +{ + struct list_head a_old, a_new, b; + LIST_HEAD(list); + + list_add_tail(&a_old, &list); + list_add_tail(&b, &list); + + /* before: [list] -> a_old -> b */ + list_replace_init(&a_old, &a_new); + + /* now: [list] -> a_new -> b */ + KUNIT_EXPECT_PTR_EQ(test, list.next, &a_new); + KUNIT_EXPECT_PTR_EQ(test, b.prev, &a_new); + + /* check a_old is empty (initialized) */ + KUNIT_EXPECT_TRUE(test, list_empty_careful(&a_old)); +} +
Re: [PATCH] lib/list-test: add a test for the 'list' doubly linked list
On Mon, Oct 7, 2019 at 6:03 PM Kees Cook wrote: (...) > > + > > +static void list_init_test(struct kunit *test) > > +{ > > + /* Test the different ways of initialising a list. */ > > + struct list_head list1 = LIST_HEAD_INIT(list1); > > + struct list_head list2; > > + LIST_HEAD(list3); > > + > > + INIT_LIST_HEAD(&list2); > > Can you also add different storage locations and initial contents tests? > For example: > > struct list_head *list4; > struct list_head *list5; > > list4 = kzalloc(sizeof(*list4)); > INIT_LIST_HEAD(list4); > > list5 = kmalloc(sizeof(*list5)); > memset(list4, 0xff, sizeof(*list5)); > INIT_LIST_HEAD(list5); > > > KUNIT_EXPECT_TRUE(test, list_empty_careful(&list4)); > KUNIT_EXPECT_TRUE(test, list_empty_careful(&list5)); > > kfree(list4); > kfree(list5); > > Then we know it's not an accident that INIT_LIST_HEAD() works when both > all-zeros and all-0xFF initial contents are there. :) Will do. > > +static void list_entry_test(struct kunit *test) > > +{ > > + struct list_test_struct test_struct; > > I guess this is not a missing initialization here because this is just > testing the container_of() wrapper macro? > Yeah: we shouldn't be doing any memory accesses here, just the pointer manipulation, so it shouldn't matter. I can add a comment pointing this out, or just initialise it anyway. > > + > > + KUNIT_EXPECT_PTR_EQ(test, &test_struct, > > list_entry(&(test_struct.list), struct list_test_struct, list)); > > +} > > + > > +static void list_first_entry_test(struct kunit *test) > > +{ > > + struct list_test_struct test_struct1, test_struct2; > > + static LIST_HEAD(list); > > This is this static? > Oops, this doesn't need to be static. I'll fix this (and the others) for the next version. > > +static void list_for_each_test(struct kunit *test) > > +{ > > + struct list_head entries[3], *cur; > > + LIST_HEAD(list); > > + int i = 0; > > + > > + list_add_tail(&entries[0], &list); > > + list_add_tail(&entries[1], &list); > > + list_add_tail(&entries[2], &list); > > + > > + list_for_each(cur, &list) { > > + KUNIT_EXPECT_PTR_EQ(test, cur, &entries[i]); > > + i++; > > + } > > + > > + KUNIT_EXPECT_EQ(test, i, 3); > > +} > > + > > +static void list_for_each_prev_test(struct kunit *test) > > +{ > > + struct list_head entries[3], *cur; > > + LIST_HEAD(list); > > + int i = 2; > > + > > + list_add_tail(&entries[0], &list); > > + list_add_tail(&entries[1], &list); > > + list_add_tail(&entries[2], &list); > > + > > + list_for_each_prev(cur, &list) { > > + KUNIT_EXPECT_PTR_EQ(test, cur, &entries[i]); > > + i--; > > + } > > + > > + KUNIT_EXPECT_EQ(test, i, -1); > > Both of these tests test that the list contained the correct number of > entries, not that anything about ordering was established. I would load > values into these with the list_test_struct and test that the counting > matches the expected ordering. > These tests do check the order of the entries (the order of the list should match the order of the entries array, and KUNIT_EXPECT_PTR_EQ() is verifying that the entries[i] is correct). It would be possible to actually use list_test_struct like with the list_for_each_entry tests, but since list_for_each just returns the list_head, it didn't seem useful, so long as the list_head pointers match. > > +} > > + > > +static void list_for_each_safe_test(struct kunit *test) > > +{ > > + struct list_head entries[3], *cur, *n; > > + LIST_HEAD(list); > > + int i = 0; > > + > > + > > + list_add_tail(&entries[0], &list); > > + list_add_tail(&entries[1], &list); > > + list_add_tail(&entries[2], &list); > > + > > + list_for_each_safe(cur, n, &list) { > > + KUNIT_EXPECT_PTR_EQ(test, cur, &entries[i]); > > + list_del(&entries[i]); > > + i++; > > + } > > + > > + KUNIT_EXPECT_EQ(test, i, 3); > > I would expect an is_empty() test here too. > Will do. > > +} > > + > > +static void list_for_each_prev_safe_test(struct kunit *test) > > +{ > > + struct list_head entries[3], *cur, *n; > > + LIST_HEAD(list); > > + int i = 2; > > + > > + list_add_tail(&entries[0], &list); > > + list_add_tail(&entries[1], &list); > > + list_add_tail(&entries[2], &list); > > + > > + list_for_each_prev_safe(cur, n, &list) { > > + KUNIT_EXPECT_PTR_EQ(test, cur, &entries[i]); > > + list_del(&entries[i]); > > + i--; > > + } > > Same thing here. > Will do. > > +static void list_for_each_entry_test(struct kunit *test) > > +{ > > + struct list_test_struct entries[5], *cur; > > + static LIST_HEAD(list); > > + int i = 0; > > + > > + for (i = 0; i < 5; ++i) { > > + entries[i].data = i; > > + list_add_tail(&entries[i].list, &list
Re: [PATCH] kunit: Kconfig: enable a KUNIT_RUN_ALL fragment
On Sat, May 2, 2020 at 4:31 AM Brendan Higgins wrote: > > On Fri, May 1, 2020 at 1:35 AM Anders Roxell wrote: > > > > Make it easier to enable all KUnit fragments. This is needed for kernel > > test-systems, so its easy to get all KUnit tests enabled and if new gets > > added they will be enabled as well. Fragments that has to be builtin > > will be missed if CONFIG_KUNIT_RUN_ALL is set as a module. > > > > Adding 'if !KUNIT_RUN_ALL' so individual test can be turned of if > > someone wants that even though KUNIT_RUN_ALL is enabled. > > I would LOVE IT, if you could make this work! I have been trying to > figure out the best way to run all KUnit tests for a long time now. > > That being said, I am a bit skeptical that this approach will be much > more successful than just using allyesconfig. Either way, there are > tests coming down the pipeline that are incompatible with each other > (the KASAN test and the KCSAN test will be incompatible). Even so, > tests like the apparmor test require a lot of non-default > configuration to compile. In the end, I am not sure how many tests we > will really be able to turn on this way. > > Thoughts? I think there's still some value in this which the allyesconfig option doesn't provide. As you point out, it's not possible to have a generic "run all tests" option due to potential conflicting dependencies, but this does provide a way to run all tests for things enabled in the current config. This could be really useful for downstream developers who want a way of running all tests relevant to their config without the overhead of running irrelevant tests (e.g., for drivers they don't build). Using allyesconfig doesn't make that distinction. Ultimately, we'll probably still want something which enables a broader set of tests for upstream development: whether that's based on this, allyesconfig, or something else entirely remains to be seen, I think. I suspect we're going to end up with something subsystem-specific (having a kunitconfig per subsystem, or a testing line in MAINTAINERS or similar are ideas which have been brought up in the past). This is a great looking tool to have in the toolbox, though. -- David
[PATCH kunit-next] kunit: kunit_tool: Separate out config/build/exec/parse
Add new subcommands to kunit.py to allow stages of the existing 'run' subcommand to be run independently: - 'config': Verifies that .config is a subset of .kunitconfig - 'build': Compiles a UML kernel for KUnit - 'exec': Runs the kernel, and outputs the test results. - 'parse': Parses test results from a file or stdin The 'run' command continues to behave as before. Signed-off-by: David Gow Reviewed-by: Brendan Higgins --- Changes since the RFC[1]: - Rebased on top of kselftest/kunit, adding support for --alltests and --make_options - Fixed an issue where the help text wouldn't print properly if an invalid subcommand were specified. - Changed 'parse' subcommand to make the filename optional (and default to stdin) as suggested by Brendan. [1]: https://lkml.org/lkml/2020/3/11/1225 tools/testing/kunit/kunit.py | 293 - tools/testing/kunit/kunit_tool_test.py | 63 +- 2 files changed, 296 insertions(+), 60 deletions(-) diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index 7dca74774dd2..b01838b6f5f9 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -20,8 +20,17 @@ import kunit_config import kunit_kernel import kunit_parser -KunitResult = namedtuple('KunitResult', ['status','result']) +KunitResult = namedtuple('KunitResult', ['status','result','elapsed_time']) +KunitConfigRequest = namedtuple('KunitConfigRequest', + ['build_dir', 'defconfig', 'make_options']) +KunitBuildRequest = namedtuple('KunitBuildRequest', + ['jobs', 'build_dir', 'alltests', + 'make_options']) +KunitExecRequest = namedtuple('KunitExecRequest', + ['timeout', 'build_dir', 'alltests']) +KunitParseRequest = namedtuple('KunitParseRequest', + ['raw_output', 'input_data']) KunitRequest = namedtuple('KunitRequest', ['raw_output','timeout', 'jobs', 'build_dir', 'defconfig', 'alltests', 'make_options']) @@ -46,14 +55,25 @@ def get_kernel_root_path(): sys.exit(1) return parts[0] -def run_tests(linux: kunit_kernel.LinuxSourceTree, - request: KunitRequest) -> KunitResult: +def config_tests(linux: kunit_kernel.LinuxSourceTree, +request: KunitConfigRequest) -> KunitResult: + kunit_parser.print_with_timestamp('Configuring KUnit Kernel ...') + config_start = time.time() + if request.defconfig: + create_default_kunitconfig() success = linux.build_reconfig(request.build_dir, request.make_options) config_end = time.time() if not success: - return KunitResult(KunitStatus.CONFIG_FAILURE, 'could not configure kernel') + return KunitResult(KunitStatus.CONFIG_FAILURE, + 'could not configure kernel', + config_end - config_start) + return KunitResult(KunitStatus.SUCCESS, + 'configured kernel successfully', + config_end - config_start) +def build_tests(linux: kunit_kernel.LinuxSourceTree, + request: KunitBuildRequest) -> KunitResult: kunit_parser.print_with_timestamp('Building KUnit Kernel ...') build_start = time.time() @@ -64,78 +84,167 @@ def run_tests(linux: kunit_kernel.LinuxSourceTree, build_end = time.time() if not success: return KunitResult(KunitStatus.BUILD_FAILURE, 'could not build kernel') + if not success: + return KunitResult(KunitStatus.BUILD_FAILURE, + 'could not build kernel', + build_end - build_start) + return KunitResult(KunitStatus.SUCCESS, + 'built kernel successfully', + build_end - build_start) +def exec_tests(linux: kunit_kernel.LinuxSourceTree, + request: KunitExecRequest) -> KunitResult: kunit_parser.print_with_timestamp('Starting KUnit Kernel ...') test_start = time.time() - kunit_output = linux.run_kernel( + result = linux.run_kernel( timeout=None if request.alltests else request.timeout, build_dir=request.build_dir) + + test_end = time.time() + + return KunitResu
[PATCH linux-kselftest/test v5] lib/list-test: add a test for the 'list' doubly linked list
Add a KUnit test for the kernel doubly linked list implementation in include/linux/list.h Each test case (list_test_x) is focused on testing the behaviour of the list function/macro 'x'. None of the tests pass invalid lists to these macros, and so should behave identically with DEBUG_LIST enabled and disabled. Note that, at present, it only tests the list_ types (not the singly-linked hlist_), and does not yet test all of the list_for_each_entry* macros (and some related things like list_prepare_entry). Signed-off-by: David Gow --- v5 replaces the use of KUNIT_ASSERT_NOT_ERR_OR_NULL() -- to check the return value from kzalloc() and kmalloc() -- with the __GFP_NOFAIL arugment. (Both in the list_test_list_init test.) Earlier versions of the test can be found: v4: https://lore.kernel.org/linux-kselftest/20191018215549.65000-1-david...@google.com/ v3: https://lore.kernel.org/linux-kselftest/20191016215707.95317-1-david...@google.com/ v2: https://lore.kernel.org/linux-kselftest/20191010185631.26541-1-david...@google.com/ v1: https://lore.kernel.org/linux-kselftest/20191007213633.92565-1-david...@google.com/ MAINTAINERS | 5 + lib/Kconfig.debug | 18 ++ lib/Makefile | 3 + lib/list-test.c | 738 ++ 4 files changed, 764 insertions(+) create mode 100644 lib/list-test.c diff --git a/MAINTAINERS b/MAINTAINERS index 7ef985e01457..7ced1b69a3d3 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9504,6 +9504,11 @@ F: Documentation/misc-devices/lis3lv02d.rst F: drivers/misc/lis3lv02d/ F: drivers/platform/x86/hp_accel.c +LIST KUNIT TEST +M: David Gow +S: Maintained +F: lib/list-test.c + LIVE PATCHING M: Josh Poimboeuf M: Jiri Kosina diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index a3017a5dadcd..7991b78eb1f3 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1961,6 +1961,24 @@ config SYSCTL_KUNIT_TEST If unsure, say N. +config LIST_KUNIT_TEST + bool "KUnit Test for Kernel Linked-list structures" + depends on KUNIT + help + This builds the linked list KUnit test suite. + It tests that the API and basic functionality of the list_head type + and associated macros. + + KUnit tests run during boot and output the results to the debug log + in TAP format (http://testanything.org/). Only useful for kernel devs + running the KUnit test harness, and not intended for inclusion into a + production build. + + For more information on KUnit and unit tests in general please refer + to the KUnit documentation in Documentation/dev-tools/kunit/. + + If unsure, say N. + config TEST_UDELAY tristate "udelay test driver" help diff --git a/lib/Makefile b/lib/Makefile index bba1fd5485f7..890e581d00c4 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -292,3 +292,6 @@ obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o obj-$(CONFIG_OBJAGG) += objagg.o + +# KUnit tests +obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o diff --git a/lib/list-test.c b/lib/list-test.c new file mode 100644 index ..a6d17647e309 --- /dev/null +++ b/lib/list-test.c @@ -0,0 +1,738 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * KUnit test for the Kernel Linked-list structures. + * + * Copyright (C) 2019, Google LLC. + * Author: David Gow + */ +#include + +#include + +struct list_test_struct { + int data; + struct list_head list; +}; + +static void list_test_list_init(struct kunit *test) +{ + /* Test the different ways of initialising a list. */ + struct list_head list1 = LIST_HEAD_INIT(list1); + struct list_head list2; + LIST_HEAD(list3); + struct list_head *list4; + struct list_head *list5; + + INIT_LIST_HEAD(&list2); + + list4 = kzalloc(sizeof(*list4), GFP_KERNEL | __GFP_NOFAIL); + INIT_LIST_HEAD(list4); + + list5 = kmalloc(sizeof(*list5), GFP_KERNEL | __GFP_NOFAIL); + memset(list5, 0xFF, sizeof(*list5)); + INIT_LIST_HEAD(list5); + + /* list_empty_careful() checks both next and prev. */ + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list1)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list2)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list3)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(list4)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(list5)); + + kfree(list4); + kfree(list5); +} + +static void list_test_list_add(struct kunit *test) +{ + struct list_head a, b; + LIST_HEAD(list); + + list_add(&a, &list); + list_add(&b, &list); + + /* should be [list] -> b -> a */ + KUNIT_EXPECT_PTR_EQ(test, list.next, &b); + KUNIT_EXPECT_PTR_EQ(test, b.prev,
Re: [PATCH linux-kselftest/test v4] lib/list-test: add a test for the 'list' doubly linked list
On Sat, Oct 19, 2019 at 1:27 AM Dan Carpenter wrote: > > On Fri, Oct 18, 2019 at 02:55:49PM -0700, David Gow wrote: > > + list4 = kzalloc(sizeof(*list4), GFP_KERNEL); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, list4); > > Why not just use GFP_KERNEL | GFP_NOFAIL and remove the check? I've sent a new version of the patch out (v5) which uses __GFP_NOFAIL instead. The idea had been to exercise KUnit's assertion functionality, in the hope that it'd allow the test to fail (but potentially allow other tests to still run) in the case of allocation failure. Given that we're only allocating enough to store ~4 pointers in total, though, that's probably of little use. > kzalloc() can't return error pointers. If this were an IS_ERR_OR_NULL() > check then it would generate a static checker warning, but static > checkers don't know about KUNIT_ASSERT_NOT_ERR_OR_NULL() yet so you're > safe. Alas, KUnit doesn't have a KUNIT_ASSERT_NOT_NULL() macro, and I'd assumed it was not dangerous (even if not ideal) to check for error pointers, even if kzalloc() can't return them. Perhaps it'd make sense to add a convenient way of checking the NULL-ness of pointers to KUnit (it's possible with the KUNIT_ASSERT_PTR_EQ(), but requires a bit of casting to make the type checker happy) in the future. Once KUnit is properly upstream, it may be worth teaching the static analysis tools about these functions to avoid having warnings in these sorts of tests. For now, though, (and for this test in particular), I agree with the suggestion of just using __GFP_NOFAIL. Thanks a lot for the comments, -- David
[PATCH linux-kselftest/test v2] lib/list-test: add a test for the 'list' doubly linked list
Add a KUnit test for the kernel doubly linked list implementation in include/linux/list.h Each test case (list_test_x) is focused on testing the behaviour of the list function/macro 'x'. None of the tests pass invalid lists to these macros, and so should behave identically with DEBUG_LIST enabled and disabled. Note that, at present, it only tests the list_ types (not the singly-linked hlist_), and does not yet test all of the list_for_each_entry* macros (and some related things like list_prepare_entry). Signed-off-by: David Gow --- Addressed the various comments on v1. The biggest change is the renaming of all of the testcases from the form x_test to list_test_x. I'm not super happy with how that looks (not the least because it ends up with 'list' twice), but it is more consistent with the proc/sysctl tests. MAINTAINERS | 5 + lib/Kconfig.debug | 12 + lib/Makefile | 3 + lib/list-test.c | 738 ++ 4 files changed, 758 insertions(+) create mode 100644 lib/list-test.c diff --git a/MAINTAINERS b/MAINTAINERS index 7ef985e01457..9ccabdb25a26 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9504,6 +9504,11 @@ F: Documentation/misc-devices/lis3lv02d.rst F: drivers/misc/lis3lv02d/ F: drivers/platform/x86/hp_accel.c +LIST UNIT TEST +M: David Gow +S: Maintained +F: lib/list-test.c + LIVE PATCHING M: Josh Poimboeuf M: Jiri Kosina diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index a3017a5dadcd..7b648141ff52 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1961,6 +1961,18 @@ config SYSCTL_KUNIT_TEST If unsure, say N. +config LIST_TEST + bool "KUnit Test for Kernel Linked-list structures" + depends on KUNIT + help + This builds the linked list unit test, which runs on boot. + It tests that the API and basic functionality of the list_head type + and associated macros. + For more information on KUnit and unit tests in general please refer + to the KUnit documentation in Documentation/dev-tools/kunit/. + + If unsure, say N. + config TEST_UDELAY tristate "udelay test driver" help diff --git a/lib/Makefile b/lib/Makefile index bba1fd5485f7..309e174ee35d 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -292,3 +292,6 @@ obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o obj-$(CONFIG_OBJAGG) += objagg.o + +# KUnit tests +obj-$(CONFIG_LIST_TEST) += list-test.o diff --git a/lib/list-test.c b/lib/list-test.c new file mode 100644 index ..52522ba83a68 --- /dev/null +++ b/lib/list-test.c @@ -0,0 +1,738 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * KUnit test for the Kernel Linked-list structures. + * + * Copyright (C) 2019, Google LLC. + * Author: David Gow + */ +#include + +#include + +struct list_test_struct { + int data; + struct list_head list; +}; + +static void list_test_list_init(struct kunit *test) +{ + /* Test the different ways of initialising a list. */ + struct list_head list1 = LIST_HEAD_INIT(list1); + struct list_head list2; + LIST_HEAD(list3); + struct list_head *list4; + struct list_head *list5; + + INIT_LIST_HEAD(&list2); + + list4 = kzalloc(sizeof(*list4), 0); + INIT_LIST_HEAD(list4); + + list5 = kmalloc(sizeof(*list5), 0); + memset(list5, 0xFF, sizeof(*list5)); + INIT_LIST_HEAD(list5); + + /* list_empty_careful() checks both next and prev. */ + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list1)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list2)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list3)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(list4)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(list5)); + + kfree(list4); + kfree(list5); +} + +static void list_test_list_add(struct kunit *test) +{ + struct list_head a, b; + LIST_HEAD(list); + + list_add(&a, &list); + list_add(&b, &list); + + /* should be [list] -> b -> a */ + KUNIT_EXPECT_PTR_EQ(test, list.next, &b); + KUNIT_EXPECT_PTR_EQ(test, b.prev, &list); + KUNIT_EXPECT_PTR_EQ(test, b.next, &a); +} + +static void list_test_list_add_tail(struct kunit *test) +{ + struct list_head a, b; + LIST_HEAD(list); + + list_add_tail(&a, &list); + list_add_tail(&b, &list); + + /* should be [list] -> a -> b */ + KUNIT_EXPECT_PTR_EQ(test, list.next, &a); + KUNIT_EXPECT_PTR_EQ(test, a.prev, &list); + KUNIT_EXPECT_PTR_EQ(test, a.next, &b); +} + +static void list_test_list_del(struct kunit *test) +{ + struct list_head a, b; + LIST_HEAD(list); + + list_add_tail(&a, &
Re: [PATCH linux-kselftest/test v2] lib/list-test: add a test for the 'list' doubly linked list
On Fri, Oct 11, 2019 at 2:05 PM Andrew Morton wrote: > > > > Given that everything runs at late_initcall time, shouldn't everything > be __init, __initdata etc so all the code and data doesn't hang around > for ever? > That's an interesting point. We haven't done this for KUnit tests to date, and there is certainly a possibility down the line that we may want to be able to run these tests in other circumstances. (There's some work being done to allow KUnit and KUnit tests to be built as modules here: https://lkml.org/lkml/2019/10/8/628 for example.) Maybe it'd be worth having macros which wrap __init/__initdata etc as a way of futureproofing tests against such a change? Either way, I suspect this is something that should probably be considered for KUnit as a whole, rather than on a test-by-test basis. On Fri, Oct 11, 2019 at 2:06 PM Andrew Morton wrote: > > On Thu, 10 Oct 2019 11:56:31 -0700 David Gow wrote: > > > Reply-To: 20191007213633.92565-1-david...@google.com > > That's a bit irksome. Deliberate? Whoops, this was supposed to be In-Reply-To. Please ignore it. On Fri, Oct 11, 2019 at 2:07 PM Andrew Morton wrote: > > On Thu, 10 Oct 2019 11:56:31 -0700 David Gow wrote: > > lib/list-test.c | 738 ++ > > Should this be lib/kunit/list-test.c? > The idea here was to have KUnit tests co-located with the code being tested, but with the linked-list code contained to a header, lib/ seemed the best place to put it, being alongside list_debug.c and similar. lib/kunit just contains the implementation for the KUnit framework itself (and tests which test that framework). Cheers, -- David smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH/RFC] clk: gate: Add some kunit test suites
On Tue, Apr 14, 2020 at 7:46 PM Vaittinen, Matti wrote: > > Hello Stephen & All, > > Prologue: > > I have been traumatized in the past - by unit tests :) Thus I am always > a bit jumpy when I see people adding UTs. I always see the inertia UTs > add to development - when people change anything they must also change > impacted UTs - and every unnecessary UT case is actually burden. I was > once buried under such burden.. Back at those times I quite often found > myself wondering why I spend two days fixing UT cases after I did a > necessary code change which took maybe 10 minutes. Please see my > comments knowing this history and do your decisions based on less > biased - brighter understanding :] > > Hey Matti, As someone who's definitely wasted a lot of time fixing poorly-thought-through tests, but who is nevertheless working enthusiastically on KUnit, I wanted to respond quickly here. Certainly, the goal is not to reduce development velocity, though it is redistributed a bit: hopefully the extra time spent updating tests will be more than paid back in identifying and fixing bugs earlier, as well as making testing one's own changes simpler. Only time will tell if we're right or not, but if they turn out to cause more problems than they solve we can always adjust or remove them. I remain optimistic, though. I do think that these tests are unlikely to increase the burden on developers much: they seem mostly to test interfaces and behaviour which is used quite a bit elsewhere in the kernel: changes that break them seem likely to be reasonably disruptive anyway, so these tests aren't going to add too much to that overall, and may ultimately make things a bit simpler by helping to document and verify the changes. A few other notes inline: > On Tue, 2020-04-07 at 20:56 -0700, Stephen Boyd wrote: > > Test various parts of the clk gate implementation with the kunit > > testing > > framework. > > > > Cc: Brendan Higgins > > Cc: > > Signed-off-by: Stephen Boyd > > --- > > > > This patch is on top of this series[1] that allows the clk > > framework to be selected by Kconfig language. > > > > [1] > > https://lore.kernel.org/r/20200405025123.154688-1-sb...@kernel.org > > > > drivers/clk/Kconfig | 8 + > > drivers/clk/Makefile| 1 + > > drivers/clk/clk-gate-test.c | 481 > > > > 3 files changed, 490 insertions(+) > > create mode 100644 drivers/clk/clk-gate-test.c > > > > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > > index 6ea0631e3956..66193673bcdf 100644 > > --- a/drivers/clk/Kconfig > > +++ b/drivers/clk/Kconfig > > @@ -377,4 +377,12 @@ source "drivers/clk/ti/Kconfig" > > source "drivers/clk/uniphier/Kconfig" > > source "drivers/clk/zynqmp/Kconfig" > > > > +# Kunit test cases > > +config CLK_GATE_TEST > > + tristate "Basic gate type Kunit test" > > + depends on KUNIT > > + default KUNIT > > + help > > + Kunit test for the basic clk gate type. > > + > > endif > > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > > index f4169cc2fd31..0785092880fd 100644 > > --- a/drivers/clk/Makefile > > +++ b/drivers/clk/Makefile > > @@ -7,6 +7,7 @@ obj-$(CONFIG_COMMON_CLK) += clk-divider.o > > obj-$(CONFIG_COMMON_CLK) += clk-fixed-factor.o > > obj-$(CONFIG_COMMON_CLK) += clk-fixed-rate.o > > obj-$(CONFIG_COMMON_CLK) += clk-gate.o > > +obj-$(CONFIG_CLK_GATE_TEST) += clk-gate-test.o > > obj-$(CONFIG_COMMON_CLK) += clk-multiplier.o > > obj-$(CONFIG_COMMON_CLK) += clk-mux.o > > obj-$(CONFIG_COMMON_CLK) += clk-composite.o > > diff --git a/drivers/clk/clk-gate-test.c b/drivers/clk/clk-gate- > > test.c > > new file mode 100644 > > index ..b1d6c21e9698 > > --- /dev/null > > +++ b/drivers/clk/clk-gate-test.c > > @@ -0,0 +1,481 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * KUnit test for clk gate basic type > > + */ > > +#include > > +#include > > +#include > > + > > +#include > > + > > +static void clk_gate_register_test_dev(struct kunit *test) > > +{ > > + struct clk_hw *ret; > > + struct platform_device *pdev; > > + > > + pdev = platform_device_register_simple("test_gate_device", -1, > > NULL, 0); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev); > > + > > + ret = clk_hw_register_gate(&pdev->dev, "test_gate", NULL, 0, > > NULL, > > +0, 0, NULL); > > + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, ret); > > + KUNIT_EXPECT_STREQ(test, "test_gate", clk_hw_get_name(ret)); > > + KUNIT_EXPECT_EQ(test, 0UL, clk_hw_get_flags(ret)); > > + > > + clk_hw_unregister_gate(ret); > > + platform_device_put(pdev); > > +} > > + > > +static void clk_gate_register_test_parent_names(struct kunit *test) > > +{ > > + struct clk_hw *parent; > > + struct clk_hw *ret; > > + > > + parent = clk_hw_register_fixed_rate(NULL, "test_parent", NULL, > > 0, > > + 100); > > + KUNI
Re: [PATCH linux-kselftest/test v2] lib/list-test: add a test for the 'list' doubly linked list
Hi all, Thanks, Andrew, for the review and for adding this to the -mm tree -- having some soak time in -next has been helpful and picked up at least one bug. Since KUnit is not yet in Linus' branch, though, it probably makes sense to put this test into the linux-kselftest/test branch, so that there aren't any chances of the list test getting in without the KUnit infrastructure. Ultimately, once KUnit is upstream, this shouldn't be an issue, but it is probably easier to consolidate things for now. Does that sound sensible? In any case, I plan to send a v3 patch out shortly which addresses some memory allocation warnings (caught by Dan Carpenter, thanks!). I could always do that as a separate bugfix patch if people preferred, though, but if this switches to the linux-kselftest/test branch, I feel we might as well get it right in the original patch. Cheers, -- David On Fri, Oct 11, 2019 at 2:55 PM Andrew Morton wrote: > > On Fri, 11 Oct 2019 14:37:25 -0700 David Gow wrote: > > > On Fri, Oct 11, 2019 at 2:05 PM Andrew Morton > > wrote: > > > > > > > > > > > > Given that everything runs at late_initcall time, shouldn't everything > > > be __init, __initdata etc so all the code and data doesn't hang around > > > for ever? > > > > > > > That's an interesting point. We haven't done this for KUnit tests to > > date, and there is certainly a possibility down the line that we may > > want to be able to run these tests in other circumstances. (There's > > some work being done to allow KUnit and KUnit tests to be built as > > modules here: https://lkml.org/lkml/2019/10/8/628 for example.) Maybe > > it'd be worth having macros which wrap __init/__initdata etc as a way > > of futureproofing tests against such a change? > > > > Either way, I suspect this is something that should probably be > > considered for KUnit as a whole, rather than on a test-by-test basis. > > Sure, a new set of macros for this makes sense. Can be retrofitted any > time. > > There might be a way of loading all of list_test.o into a discardable > section at link time instead of sprinkling annotation all over the .c > code. smime.p7s Description: S/MIME Cryptographic Signature
[PATCH linux-kselftest/test v3] lib/list-test: add a test for the 'list' doubly linked list
Add a KUnit test for the kernel doubly linked list implementation in include/linux/list.h Each test case (list_test_x) is focused on testing the behaviour of the list function/macro 'x'. None of the tests pass invalid lists to these macros, and so should behave identically with DEBUG_LIST enabled and disabled. Note that, at present, it only tests the list_ types (not the singly-linked hlist_), and does not yet test all of the list_for_each_entry* macros (and some related things like list_prepare_entry). Signed-off-by: David Gow --- Note: v2 can be found here: https://lkml.org/lkml/2019/10/10/1011 This revision fixes some issues with the memory allocation in the 'list_test_list_init' test. In particular, it uses a KUnit assert to verify that the lists allocated with kmalloc() and kzalloc() are non-NULL (and also properly passes GFP_KERNEL to them). Since this depends on KUnit, which is not yet upstream, it probably makes sense for it to go into the linux-kselftest/test branch alongside KUnit itself. v2 has ended up in the -mm tree, and is already in -next, though. I think it nevertheless makes sense to move this change to the linux-kselftest/test branch, as outlined here: https://lore.kernel.org/linux-kselftest/CABVgOS=W4cfFoE=jt4mbk1zkusreucrw_b81r2jwdcfpoco...@mail.gmail.com/T/#t Thanks, -- David MAINTAINERS | 5 + lib/Kconfig.debug | 12 + lib/Makefile | 3 + lib/list-test.c | 740 ++ 4 files changed, 760 insertions(+) create mode 100644 lib/list-test.c diff --git a/MAINTAINERS b/MAINTAINERS index 7ef985e01457..9ccabdb25a26 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9504,6 +9504,11 @@ F: Documentation/misc-devices/lis3lv02d.rst F: drivers/misc/lis3lv02d/ F: drivers/platform/x86/hp_accel.c +LIST UNIT TEST +M: David Gow +S: Maintained +F: lib/list-test.c + LIVE PATCHING M: Josh Poimboeuf M: Jiri Kosina diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index a3017a5dadcd..7b648141ff52 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1961,6 +1961,18 @@ config SYSCTL_KUNIT_TEST If unsure, say N. +config LIST_TEST + bool "KUnit Test for Kernel Linked-list structures" + depends on KUNIT + help + This builds the linked list unit test, which runs on boot. + It tests that the API and basic functionality of the list_head type + and associated macros. + For more information on KUnit and unit tests in general please refer + to the KUnit documentation in Documentation/dev-tools/kunit/. + + If unsure, say N. + config TEST_UDELAY tristate "udelay test driver" help diff --git a/lib/Makefile b/lib/Makefile index bba1fd5485f7..309e174ee35d 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -292,3 +292,6 @@ obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o obj-$(CONFIG_OBJAGG) += objagg.o + +# KUnit tests +obj-$(CONFIG_LIST_TEST) += list-test.o diff --git a/lib/list-test.c b/lib/list-test.c new file mode 100644 index ..de6de4ef32c9 --- /dev/null +++ b/lib/list-test.c @@ -0,0 +1,740 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * KUnit test for the Kernel Linked-list structures. + * + * Copyright (C) 2019, Google LLC. + * Author: David Gow + */ +#include + +#include + +struct list_test_struct { + int data; + struct list_head list; +}; + +static void list_test_list_init(struct kunit *test) +{ + /* Test the different ways of initialising a list. */ + struct list_head list1 = LIST_HEAD_INIT(list1); + struct list_head list2; + LIST_HEAD(list3); + struct list_head *list4; + struct list_head *list5; + + INIT_LIST_HEAD(&list2); + + list4 = kzalloc(sizeof(*list4), GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, list4); + INIT_LIST_HEAD(list4); + + list5 = kmalloc(sizeof(*list5), GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, list5); + memset(list5, 0xFF, sizeof(*list5)); + INIT_LIST_HEAD(list5); + + /* list_empty_careful() checks both next and prev. */ + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list1)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list2)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list3)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(list4)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(list5)); + + kfree(list4); + kfree(list5); +} + +static void list_test_list_add(struct kunit *test) +{ + struct list_head a, b; + LIST_HEAD(list); + + list_add(&a, &list); + list_add(&b, &list); + + /* should be [list] -> b -> a */ + KUNIT_EXPECT_PTR_EQ(test, list.next, &b); + KUNIT_EXPECT_PTR_EQ(test, b.prev, &list); + K
[PATCH linux-kselftest/test v4] lib/list-test: add a test for the 'list' doubly linked list
Add a KUnit test for the kernel doubly linked list implementation in include/linux/list.h Each test case (list_test_x) is focused on testing the behaviour of the list function/macro 'x'. None of the tests pass invalid lists to these macros, and so should behave identically with DEBUG_LIST enabled and disabled. Note that, at present, it only tests the list_ types (not the singly-linked hlist_), and does not yet test all of the list_for_each_entry* macros (and some related things like list_prepare_entry). Signed-off-by: David Gow --- The changes from v3 are mostly to do with naming: - The Kconfig entry has been renamed from LIST_TEST to LIST_KUNIT_TEST, which matches the SYSCTL_KUNIT_TEST entry, - The Kconfig description was updated to better match other KUnit tests, specifying that the test is not intended for use in a production kernel. A now-redundant mention of the test running a boot was removed. - The MAINTAINERS entry refers to a "KUNIT TEST" rather than a "UNIT TEST" - The module name has changed from "list-test" to "list-kunit-test". Earlier versions of the test can be found: v3: https://lore.kernel.org/linux-kselftest/20191016215707.95317-1-david...@google.com/ v2: https://lore.kernel.org/linux-kselftest/20191010185631.26541-1-david...@google.com/ v1: https://lore.kernel.org/linux-kselftest/20191007213633.92565-1-david...@google.com/ MAINTAINERS | 5 + lib/Kconfig.debug | 18 ++ lib/Makefile | 3 + lib/list-test.c | 740 ++ 4 files changed, 766 insertions(+) create mode 100644 lib/list-test.c diff --git a/MAINTAINERS b/MAINTAINERS index 7ef985e01457..7ced1b69a3d3 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9504,6 +9504,11 @@ F: Documentation/misc-devices/lis3lv02d.rst F: drivers/misc/lis3lv02d/ F: drivers/platform/x86/hp_accel.c +LIST KUNIT TEST +M: David Gow +S: Maintained +F: lib/list-test.c + LIVE PATCHING M: Josh Poimboeuf M: Jiri Kosina diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index a3017a5dadcd..7991b78eb1f3 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1961,6 +1961,24 @@ config SYSCTL_KUNIT_TEST If unsure, say N. +config LIST_KUNIT_TEST + bool "KUnit Test for Kernel Linked-list structures" + depends on KUNIT + help + This builds the linked list KUnit test suite. + It tests that the API and basic functionality of the list_head type + and associated macros. + + KUnit tests run during boot and output the results to the debug log + in TAP format (http://testanything.org/). Only useful for kernel devs + running the KUnit test harness, and not intended for inclusion into a + production build. + + For more information on KUnit and unit tests in general please refer + to the KUnit documentation in Documentation/dev-tools/kunit/. + + If unsure, say N. + config TEST_UDELAY tristate "udelay test driver" help diff --git a/lib/Makefile b/lib/Makefile index bba1fd5485f7..890e581d00c4 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -292,3 +292,6 @@ obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o obj-$(CONFIG_OBJAGG) += objagg.o + +# KUnit tests +obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o diff --git a/lib/list-test.c b/lib/list-test.c new file mode 100644 index ..75ba3ddac959 --- /dev/null +++ b/lib/list-test.c @@ -0,0 +1,740 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * KUnit test for the Kernel Linked-list structures. + * + * Copyright (C) 2019, Google LLC. + * Author: David Gow + */ +#include + +#include + +struct list_test_struct { + int data; + struct list_head list; +}; + +static void list_test_list_init(struct kunit *test) +{ + /* Test the different ways of initialising a list. */ + struct list_head list1 = LIST_HEAD_INIT(list1); + struct list_head list2; + LIST_HEAD(list3); + struct list_head *list4; + struct list_head *list5; + + INIT_LIST_HEAD(&list2); + + list4 = kzalloc(sizeof(*list4), GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, list4); + INIT_LIST_HEAD(list4); + + list5 = kmalloc(sizeof(*list5), GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, list5); + memset(list5, 0xFF, sizeof(*list5)); + INIT_LIST_HEAD(list5); + + /* list_empty_careful() checks both next and prev. */ + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list1)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list2)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list3)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(list4)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(list5)); + + kfree(list4); + kf
Re: [PATCH v7 0/5] KUnit-KASAN Integration
On Sat, May 23, 2020 at 6:30 AM shuah wrote: > > On 5/3/20 4:09 AM, Alan Maguire wrote: > > On Thu, 23 Apr 2020, David Gow wrote: > > > >> This patchset contains everything needed to integrate KASAN and KUnit. > >> > >> KUnit will be able to: > >> (1) Fail tests when an unexpected KASAN error occurs > >> (2) Pass tests when an expected KASAN error occurs > >> > >> Convert KASAN tests to KUnit with the exception of copy_user_test > >> because KUnit is unable to test those. > >> > >> Add documentation on how to run the KASAN tests with KUnit and what to > >> expect when running these tests. > >> > >> This patchset depends on: > >> - "[PATCH v3 kunit-next 0/2] kunit: extend kunit resources API" [1] > >> - "[PATCH v3 0/3] Fix some incompatibilites between KASAN and > >>FORTIFY_SOURCE" [2] > >> > >> Changes from v6: > >> - Rebased on top of kselftest/kunit > >> - Rebased on top of Daniel Axtens' fix for FORTIFY_SOURCE > >> incompatibilites [2] > >> - Removed a redundant report_enabled() check. > >> - Fixed some places with out of date Kconfig names in the > >> documentation. > >> > > > > Sorry for the delay in getting to this; I retested the > > series with the above patchsets pre-applied; all looks > > good now, thanks! Looks like Daniel's patchset has a v4 > > so I'm not sure if that will have implications for applying > > your changes on top of it (haven't tested it yet myself). > > > > For the series feel free to add > > > > Tested-by: Alan Maguire > > > > I'll try and take some time to review v7 shortly, but I wanted > > to confirm the issues I saw went away first in case you're > > blocked. The only remaining issue I see is that we'd need the > > named resource patchset to land first; it would be good > > to ensure the API it provides is solid so you won't need to > > respin. > > > > Thanks! > > > > Alan > > > >> Changes from v5: > >> - Split out the panic_on_warn changes to a separate patch. > >> - Fix documentation to fewer to the new Kconfig names. > >> - Fix some changes which were in the wrong patch. > >> - Rebase on top of kselftest/kunit (currently identical to 5.7-rc1) > >> > > > > Hi Brendan, > > Is this series ready to go inot Linux 5.8-rc1? Let me know. > Probably needs rebase on top of kselftest/kunit. I applied > patches from David and Vitor > > thanks, > -- Shuah > Hi Shuah, I think the only things holding this up are the missing dependencies: the "extend kunit resources API" patches[1] for KUnit (which look ready to me), and the "Fix some incompatibilities between KASAN and FORTIFY_SOURCE" changes[2] on the KASAN side (which also seem ready). This patchset may need a (likely rather trivial) rebase on top of whatever versions of those end up merged: I'm happy to do that if necessary. Cheers, -- David [1]: https://lore.kernel.org/linux-kselftest/1585313122-26441-1-git-send-email-alan.magu...@oracle.com/T/#t [2]: http://lkml.iu.edu/hypermail/linux/kernel/2004.3/00735.html
[PATCH] Documentation: kunit: Add naming guidelines
As discussed in [1], KUnit tests have hitherto not had a particularly consistent naming scheme. This adds documentation outlining how tests and test suites should be named, including how those names should be used in Kconfig entries and filenames. [1]: https://lore.kernel.org/linux-kselftest/202006141005.BA19A9D3@keescook/t/#u Signed-off-by: David Gow Reviewed-by: Kees Cook --- This is a follow-up v1 to the RFC patch here: https://lore.kernel.org/linux-kselftest/20200620054944.167330-1-david...@google.com/T/#u There weren't any fundamental objections to the naming guidelines themselves, so nothing's changed on that front. Otherwise, changes since the RFC: - Fixed a bit of space/tab confusion in the index (Thanks, Randy) - Added some more examples (and some test case examples). - Added some examples of what not to call subsystems and suites. - No longer explicitly require "If unsure, put N" in Kconfig entries. - Minor formatting changes. Cheers, -- David Documentation/dev-tools/kunit/index.rst | 1 + Documentation/dev-tools/kunit/style.rst | 181 2 files changed, 182 insertions(+) create mode 100644 Documentation/dev-tools/kunit/style.rst diff --git a/Documentation/dev-tools/kunit/index.rst b/Documentation/dev-tools/kunit/index.rst index e93606ecfb01..c234a3ab3c34 100644 --- a/Documentation/dev-tools/kunit/index.rst +++ b/Documentation/dev-tools/kunit/index.rst @@ -11,6 +11,7 @@ KUnit - Unit Testing for the Linux Kernel usage kunit-tool api/index + style faq What is KUnit? diff --git a/Documentation/dev-tools/kunit/style.rst b/Documentation/dev-tools/kunit/style.rst new file mode 100644 index ..8cad2627924c --- /dev/null +++ b/Documentation/dev-tools/kunit/style.rst @@ -0,0 +1,181 @@ +.. SPDX-License-Identifier: GPL-2.0 + +=== +Test Style and Nomenclature +=== + +Subsystems, Suites, and Tests += + +In order to make tests as easy to find as possible, they're grouped into suites +and subsystems. A test suite is a group of tests which test a related area of +the kernel, and a subsystem is a set of test suites which test different parts +of the same kernel subsystem or driver. + +Subsystems +-- + +Every test suite must belong to a subsystem. A subsystem is a collection of one +or more KUnit test suites which test the same driver or part of the kernel. A +rule of thumb is that a test subsystem should match a single kernel module. If +the code being tested can't be compiled as a module, in many cases the subsystem +should correspond to a directory in the source tree or an entry in the +MAINTAINERS file. If unsure, follow the conventions set by tests in similar +areas. + +Test subsystems should be named after the code being tested, either after the +module (wherever possible), or after the directory or files being tested. Test +subsystems should be named to avoid ambiguity where necessary. + +If a test subsystem name has multiple components, they should be separated by +underscores. *Do not* include "test" or "kunit" directly in the subsystem name +unless you are actually testing other tests or the kunit framework itself. + +Example subsystems could be: + +``ext4`` + Matches the module and filesystem name. +``apparmor`` + Matches the module name and LSM name. +``kasan`` + Common name for the tool, prominent part of the path ``mm/kasan`` +``snd_hda_codec_hdmi`` + Has several components (``snd``, ``hda``, ``codec``, ``hdmi``) separated by + underscores. Matches the module name. + +Avoid names like these: + +``linear-ranges`` + Names should use underscores, not dashes, to separate words. Prefer + ``linear_ranges``. +``qos-kunit-test`` + As well as using underscores, this name should not have "kunit-test" as a + suffix, and ``qos`` is ambiguous as a subsystem name. ``power_qos`` would be a + better name. +``pc_parallel_port`` + The corresponding module name is ``parport_pc``, so this subsystem should also + be named ``parport_pc``. + +.. note:: +The KUnit API and tools do not explicitly know about subsystems. They're +simply a way of categorising test suites and naming modules which +provides a simple, consistent way for humans to find and run tests. This +may change in the future, though. + +Suites +-- + +KUnit tests are grouped into test suites, which cover a specific area of +functionality being tested. Test suites can have shared initialisation and +shutdown code which is run for all tests in the suite. +Not all subsystems will need to be split into multiple test suites (e.g. simple drivers). + +Test suites are named after the subsystem they are part of. If a subsystem +contains several suites, the specific area under test should be appended to the +subsystem name, separated by an underscore. + +The full test suite name (
[PATCH] clk: Specify IOMEM dependency for HSDK pll driver
The HSDK pll driver uses the devm_ioremap_resource function, but does not specify a dependency on IOMEM in Kconfig. This causes a build failure on architectures without IOMEM, for example, UML (notably with make allyesconfig). Fix this by making CONFIG_CLK_HSDK depend on CONFIG_IOMEM. Signed-off-by: David Gow --- drivers/clk/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 69934c0c3dd8..326f91b2dda9 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -50,6 +50,7 @@ source "drivers/clk/versatile/Kconfig" config CLK_HSDK bool "PLL Driver for HSDK platform" depends on OF || COMPILE_TEST + depends on IOMEM help This driver supports the HSDK core, system, ddr, tunnel and hdmi PLLs control. -- 2.27.0.212.ge8ba1cc988-goog
[PATCH] clk: staging: Specify IOMEM dependency for Xilinx Clocking Wizard driver
The Xilinx Clocking Wizard driver uses the devm_ioremap_resource function, but does not specify a dependency on IOMEM in Kconfig. This causes a build failure on architectures without IOMEM, for example, UML (notably with make allyesconfig). Fix this by making CONFIG_COMMON_CLK_XLNX_CLKWZRD depend on CONFIG_IOMEM. Signed-off-by: David Gow --- drivers/staging/clocking-wizard/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/clocking-wizard/Kconfig b/drivers/staging/clocking-wizard/Kconfig index 04be22dca9b6..69cf51445f08 100644 --- a/drivers/staging/clocking-wizard/Kconfig +++ b/drivers/staging/clocking-wizard/Kconfig @@ -5,6 +5,6 @@ config COMMON_CLK_XLNX_CLKWZRD tristate "Xilinx Clocking Wizard" - depends on COMMON_CLK && OF + depends on COMMON_CLK && OF && IOMEM help Support for the Xilinx Clocking Wizard IP core clock generator. -- 2.27.0.212.ge8ba1cc988-goog
[PATCH] iio: adc: Specify IOMEM dependency for adi-axi-adc driver
The Analog Devices AXI ADC driver uses the devm_ioremap_resource function, but does not specify a dependency on IOMEM in Kconfig. This causes a build failure on architectures without IOMEM, for example, UML (notably with make allyesconfig). Fix this by making CONFIG_ADI_AXI_ADC depend on CONFIG_IOMEM. Signed-off-by: David Gow --- drivers/iio/adc/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index ff3569635ce0..f5009b61b80c 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -263,6 +263,7 @@ config AD9467 config ADI_AXI_ADC tristate "Analog Devices Generic AXI ADC IP core driver" + depends on IOMEM select IIO_BUFFER select IIO_BUFFER_HW_CONSUMER select IIO_BUFFER_DMAENGINE -- 2.27.0.212.ge8ba1cc988-goog