Currently, KUnit does not allow the use of tests as a module.
This prevents the implementation of tests that require userspace.
This patchset makes this possible by introducing the use of
the root filesystem in KUnit. And it allows the use of tests
that can be compiled as a module
Vitor Massaru Iha (3):
kunit: tool: Add support root filesystem in kunit-tool
lib: Allows to borrow mm in userspace on KUnit
lib: Convert test_user_copy to KUnit test
include/kunit/test.h | 1 +
lib/Kconfig.debug | 17 ++
lib/Makefile | 2 +-
lib/kunit/try-catch.c | 15 +-
lib/{test_user_copy.c => user_copy_kunit.c} | 196 +++++++++-----------
tools/testing/kunit/kunit.py | 37 +++-
tools/testing/kunit/kunit_kernel.py | 105 +++++++++--
7 files changed, 238 insertions(+), 135 deletions(-)
rename lib/{test_user_copy.c => user_copy_kunit.c} (55%)
base-commit: 725aca9585956676687c4cb803e88f770b0df2b2
prerequisite-patch-id: 582b6d9d28ce4b71628890ec832df6522ca68de0
--
2.26.2
This makes it possible to use KUnit tests as modules.
And with that the tests can run in userspace.
The filesystem was created using debootstrap:
sudo debootstrap buster buster_rootfs
And change the owner of the root filesystem files
for your user:
sudo chown -R $USER:$USER buster_rootfs
For the kunit-tool to correctly capture the test results,
uml_utilities must be installed on the host to halt uml.
Signed-off-by: Vitor Massaru Iha <[email protected]>
---
tools/testing/kunit/kunit.py | 37 ++++++++--
tools/testing/kunit/kunit_kernel.py | 105 ++++++++++++++++++++++++----
2 files changed, 121 insertions(+), 21 deletions(-)
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
index 787b6d4ad716..6d8ba0215b90 100755
--- a/tools/testing/kunit/kunit.py
+++ b/tools/testing/kunit/kunit.py
@@ -23,16 +23,16 @@ import kunit_parser
KunitResult = namedtuple('KunitResult', ['status','result','elapsed_time'])
KunitConfigRequest = namedtuple('KunitConfigRequest',
- ['build_dir', 'make_options'])
+ ['build_dir', 'uml_root_dir', 'make_options'])
KunitBuildRequest = namedtuple('KunitBuildRequest',
- ['jobs', 'build_dir', 'alltests',
+ ['jobs', 'build_dir', 'uml_root_dir', 'alltests',
'make_options'])
KunitExecRequest = namedtuple('KunitExecRequest',
- ['timeout', 'build_dir', 'alltests'])
+ ['timeout', 'build_dir', 'uml_root_dir', 'alltests'])
KunitParseRequest = namedtuple('KunitParseRequest',
['raw_output', 'input_data'])
KunitRequest = namedtuple('KunitRequest', ['raw_output','timeout', 'jobs',
- 'build_dir', 'alltests',
+ 'build_dir', 'uml_root_dir', 'alltests',
'make_options'])
KernelDirectoryPath = sys.argv[0].split('tools/testing/kunit/')[0]
@@ -47,7 +47,6 @@ def create_default_kunitconfig():
if not os.path.exists(kunit_kernel.kunitconfig_path):
shutil.copyfile('arch/um/configs/kunit_defconfig',
kunit_kernel.kunitconfig_path)
-
def get_kernel_root_path():
parts = sys.argv[0] if not __file__ else __file__
parts = os.path.realpath(parts).split('tools/testing/kunit')
@@ -58,10 +57,22 @@ def get_kernel_root_path():
def config_tests(linux: kunit_kernel.LinuxSourceTree,
request: KunitConfigRequest) -> KunitResult:
kunit_parser.print_with_timestamp('Configuring KUnit Kernel ...')
+ defconfig = False
config_start = time.time()
create_default_kunitconfig()
- success = linux.build_reconfig(request.build_dir, request.make_options)
+
+ if request.uml_root_dir != None:
+ if os.path.exists(request.uml_root_dir):
+ kunit_kernel.uml_root_path = os.path.abspath(request.uml_root_dir)
+ defconfig = True
+ else:
+ config_end = time.time()
+ return KunitResult(KunitStatus.CONFIG_FAILURE,
+ 'could not configure kernel',
+ config_end - config_start)
+
+ success = linux.build_reconfig(request.build_dir, defconfig, request.make_options)
config_end = time.time()
if not success:
return KunitResult(KunitStatus.CONFIG_FAILURE,
@@ -79,6 +90,7 @@ def build_tests(linux: kunit_kernel.LinuxSourceTree,
success = linux.build_um_kernel(request.alltests,
request.jobs,
request.build_dir,
+ request.uml_root_dir,
request.make_options)
build_end = time.time()
if not success:
@@ -97,7 +109,7 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree,
test_start = time.time()
result = linux.run_kernel(
timeout=None if request.alltests else request.timeout,
- build_dir=request.build_dir)
+ build_dir=request.build_dir, uml_root_dir=request.uml_root_dir)
test_end = time.time()
@@ -130,12 +142,14 @@ def run_tests(linux: kunit_kernel.LinuxSourceTree,
run_start = time.time()
config_request = KunitConfigRequest(request.build_dir,
+ request.uml_root_dir,
request.make_options)
config_result = config_tests(linux, config_request)
if config_result.status != KunitStatus.SUCCESS:
return config_result
build_request = KunitBuildRequest(request.jobs, request.build_dir,
+ request.uml_root_dir,
request.alltests,
request.make_options)
build_result = build_tests(linux, build_request)
@@ -143,6 +157,7 @@ def run_tests(linux: kunit_kernel.LinuxSourceTree,
return build_result
exec_request = KunitExecRequest(request.timeout, request.build_dir,
+ request.uml_root_dir,
request.alltests)
exec_result = exec_tests(linux, exec_request)
if exec_result.status != KunitStatus.SUCCESS:
@@ -168,6 +183,10 @@ def add_common_opts(parser):
help='As in the make command, it specifies the build '
'directory.',
type=str, default='.kunit', metavar='build_dir')
+ parser.add_argument('--uml_root_dir',
+ help='To load modules, a root filesystem is '
+ 'required to install and load these modules.',
+ type=str, default=None, metavar='uml_root_dir')
parser.add_argument('--make_options',
help='X=Y make option, can be repeated.',
action='append')
@@ -252,6 +271,7 @@ def main(argv, linux=None):
cli_args.timeout,
cli_args.jobs,
cli_args.build_dir,
+ cli_args.uml_root_dir,
cli_args.alltests,
cli_args.make_options)
result = run_tests(linux, request)
@@ -272,6 +292,7 @@ def main(argv, linux=None):
linux = kunit_kernel.LinuxSourceTree()
request = KunitConfigRequest(cli_args.build_dir,
+ cli_args.uml_root_dir,
cli_args.make_options)
result = config_tests(linux, request)
kunit_parser.print_with_timestamp((
@@ -295,6 +316,7 @@ def main(argv, linux=None):
request = KunitBuildRequest(cli_args.jobs,
cli_args.build_dir,
+ cli_args.uml_root_dir,
cli_args.alltests,
cli_args.make_options)
result = build_tests(linux, request)
@@ -319,6 +341,7 @@ def main(argv, linux=None):
exec_request = KunitExecRequest(cli_args.timeout,
cli_args.build_dir,
+ cli_args.uml_root_dir,
cli_args.alltests)
exec_result = exec_tests(linux, exec_request)
parse_request = KunitParseRequest(cli_args.raw_output,
diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
index 63dbda2d029f..d712f4619eaa 100644
--- a/tools/testing/kunit/kunit_kernel.py
+++ b/tools/testing/kunit/kunit_kernel.py
@@ -11,6 +11,7 @@ import logging
import subprocess
import os
import signal
+import time
from contextlib import ExitStack
@@ -19,7 +20,59 @@ import kunit_parser
KCONFIG_PATH = '.config'
kunitconfig_path = '.kunitconfig'
+X86_64_DEFCONFIG_PATH = 'arch/um/configs/x86_64_defconfig'
BROKEN_ALLCONFIG_PATH = 'tools/testing/kunit/configs/broken_on_uml.config'
+uml_root_path = None
+
+make_cmd = {
+ 'make': {
+ 'command': ['make', 'ARCH=um'],
+ 'msg_error': 'Could not call execute make: ',
+ },
+ 'make_modules': {
+ 'command': ['make', 'modules', 'ARCH=um'],
+ 'msg_error': 'Could not call execute make modules: ',
+ },
+ 'make_modules_install': {
+ 'command': ['make', 'modules_install', 'ARCH=um'],
+ 'msg_error': 'Could not call execute make modules_install: ',
+ }
+}
+
+def halt_uml():
+ try:
+ subprocess.call(['uml_mconsole', 'kunitid', 'halt'])
+ except OSError as e:
+ raise ConfigError('Could not call uml_mconsole ' + e)
+ except subprocess.CalledProcessError as e:
+ raise ConfigError(e.output)
+
+
+def enable_uml_modules_on_boot(output_command):
+ uml_modules_path = None
+ found_kernel_version = False
+ modules = []
+ for i in output_command.decode('utf-8').split():
+ if found_kernel_version:
+ kernel_version = i
+ uml_modules_path = os.path.join(uml_root_path,
+ 'lib/modules/', kernel_version, 'kernel/lib/')
+ break
+ if 'DEPMOD' in i:
+ found_kernel_version = True
+
+ try:
+ if os.path.exists(uml_modules_path):
+ modules = subprocess.check_output(['ls',
+ uml_modules_path]).decode('utf-8').split()
+ except OSError as e:
+ raise ConfigError('Could not list directory ' + e)
+ except subprocess.CalledProcessError as e:
+ raise ConfigError(e.output)
+
+ with open(os.path.join(uml_root_path, 'etc/modules'), 'w') as f:
+ for i in modules:
+ f.write(i.replace('.ko', ''))
class ConfigError(Exception):
"""Represents an error trying to configure the Linux kernel."""
@@ -70,20 +123,27 @@ class LinuxSourceTreeOperations(object):
kunit_parser.print_with_timestamp(
'Starting Kernel with all configs takes a few minutes...')
- def make(self, jobs, build_dir, make_options):
- command = ['make', 'ARCH=um', '--jobs=' + str(jobs)]
+ def make(self, cmd, jobs, build_dir, make_options):
+ command = make_cmd[cmd]['command'] + ['--jobs=' + str(jobs)]
+
if make_options:
command.extend(make_options)
if build_dir:
command += ['O=' + build_dir]
+
+ if cmd == 'make_modules_install':
+ command += ['INSTALL_MOD_PATH=' + uml_root_path]
+
try:
- subprocess.check_output(command)
+ output = subprocess.check_output(command)
+ if cmd == 'make_modules_install':
+ enable_uml_modules_on_boot(output)
except OSError as e:
- raise BuildError('Could not call execute make: ' + e)
+ raise BuildError(make_cmd[cmd][msg_error] + e)
except subprocess.CalledProcessError as e:
raise BuildError(e.output)
- def linux_bin(self, params, timeout, build_dir, outfile):
+ def linux_bin(self, params, timeout, build_dir, uml_root_dir, outfile):
"""Runs the Linux UML binary. Must be named 'linux'."""
linux_bin = './linux'
if build_dir:
@@ -92,7 +152,11 @@ class LinuxSourceTreeOperations(object):
process = subprocess.Popen([linux_bin] + params,
stdout=output,
stderr=subprocess.STDOUT)
- process.wait(timeout)
+ if uml_root_dir:
+ time.sleep(timeout)
+ halt_uml()
+ else:
+ process.wait(timeout)
def get_kconfig_path(build_dir):
@@ -132,11 +196,16 @@ class LinuxSourceTree(object):
return False
return True
- def build_config(self, build_dir, make_options):
+ def build_config(self, build_dir, defconfig, make_options):
kconfig_path = get_kconfig_path(build_dir)
if build_dir and not os.path.exists(build_dir):
os.mkdir(build_dir)
self._kconfig.write_to_file(kconfig_path)
+
+ if defconfig:
+ with open(kconfig_path, 'a') as fw:
+ with open(X86_64_DEFCONFIG_PATH, 'r') as fr:
+ fw.write(fr.read())
try:
self._ops.make_olddefconfig(build_dir, make_options)
except ConfigError as e:
@@ -144,7 +213,7 @@ class LinuxSourceTree(object):
return False
return self.validate_config(build_dir)
- def build_reconfig(self, build_dir, make_options):
+ def build_reconfig(self, build_dir, defconfig, make_options):
"""Creates a new .config if it is not a subset of the .kunitconfig."""
kconfig_path = get_kconfig_path(build_dir)
if os.path.exists(kconfig_path):
@@ -153,28 +222,36 @@ class LinuxSourceTree(object):
if not self._kconfig.is_subset_of(existing_kconfig):
print('Regenerating .config ...')
os.remove(kconfig_path)
- return self.build_config(build_dir, make_options)
+ return self.build_config(build_dir, defconfig, make_options)
else:
return True
else:
print('Generating .config ...')
- return self.build_config(build_dir, make_options)
+ return self.build_config(build_dir, defconfig, make_options)
- def build_um_kernel(self, alltests, jobs, build_dir, make_options):
+ def build_um_kernel(self, alltests, jobs, build_dir, uml_root_dir, make_options):
if alltests:
self._ops.make_allyesconfig()
try:
self._ops.make_olddefconfig(build_dir, make_options)
- self._ops.make(jobs, build_dir, make_options)
+ self._ops.make('make', jobs, build_dir, make_options)
+ if uml_root_dir:
+ self._ops.make('make_modules', jobs, build_dir,
+ make_options)
+ self._ops.make('make_modules_install', jobs,
+ build_dir, make_options)
except (ConfigError, BuildError) as e:
logging.error(e)
return False
return self.validate_config(build_dir)
- def run_kernel(self, args=[], build_dir='', timeout=None):
+ def run_kernel(self, args=[], build_dir='', uml_root_dir=None, timeout=None):
args.extend(['mem=1G'])
+ if uml_root_dir:
+ args.extend(['root=/dev/root', 'rootfstype=hostfs',
+ 'rootflags=' + uml_root_path, 'umid=kunitid'])
outfile = 'test.log'
- self._ops.linux_bin(args, timeout, build_dir, outfile)
+ self._ops.linux_bin(args, timeout, build_dir, uml_root_dir, outfile)
subprocess.call(['stty', 'sane'])
with open(outfile, 'r') as file:
for line in file:
--
2.26.2
Signed-off-by: Vitor Massaru Iha <[email protected]>
---
include/kunit/test.h | 1 +
lib/kunit/try-catch.c | 15 ++++++++++++++-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 59f3144f009a..49c38bdcb93e 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -222,6 +222,7 @@ struct kunit {
* protect it with some type of lock.
*/
struct list_head resources; /* Protected by lock. */
+ struct mm_struct *mm;
};
void kunit_init_test(struct kunit *test, const char *name, char *log);
diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
index 0dd434e40487..f677c2f2a51a 100644
--- a/lib/kunit/try-catch.c
+++ b/lib/kunit/try-catch.c
@@ -11,7 +11,8 @@
#include <linux/completion.h>
#include <linux/kernel.h>
#include <linux/kthread.h>
-
+#include <linux/sched/mm.h>
+#include <linux/sched/task.h>
#include "try-catch-impl.h"
void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch)
@@ -24,8 +25,17 @@ EXPORT_SYMBOL_GPL(kunit_try_catch_throw);
static int kunit_generic_run_threadfn_adapter(void *data)
{
struct kunit_try_catch *try_catch = data;
+ struct kunit *test = try_catch->test;
+
+ if (test->mm != NULL)
+ kthread_use_mm(try_catch->test->mm);
try_catch->try(try_catch->context);
+ if (try_catch->test->mm) {
+ if (test->mm != NULL)
+ kthread_unuse_mm(try_catch->test->mm);
+ try_catch->test->mm = NULL;
+ }
complete_and_exit(try_catch->try_completion, 0);
}
@@ -65,6 +75,9 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
try_catch->context = context;
try_catch->try_completion = &try_completion;
try_catch->try_result = 0;
+
+ test->mm = get_task_mm(current);
+
task_struct = kthread_run(kunit_generic_run_threadfn_adapter,
try_catch,
"kunit_try_catch_thread");
--
2.26.2
This adds the conversion of the runtime tests of test_user_copy fuctions,
from `lib/test_user_copy.c`to KUnit tests.
Signed-off-by: Vitor Massaru Iha <[email protected]>
---
lib/Kconfig.debug | 17 ++
lib/Makefile | 2 +-
lib/{test_user_copy.c => user_copy_kunit.c} | 196 +++++++++-----------
3 files changed, 102 insertions(+), 113 deletions(-)
rename lib/{test_user_copy.c => user_copy_kunit.c} (55%)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 9ad9210d70a1..29558674c011 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2154,6 +2154,23 @@ config SYSCTL_KUNIT_TEST
If unsure, say N.
+config USER_COPY_KUNIT
+ tristate "KUnit Test for user/kernel boundary protections"
+ depends on KUNIT
+ depends on m
+ help
+ This builds the "test_user_copy" module that runs sanity checks
+ on the copy_to/from_user infrastructure, making sure basic
+ user/kernel boundary testing is working. If it fails to load,
+ a regression has been detected in the user/kernel memory boundary
+ protections.
+
+ 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 LIST_KUNIT_TEST
tristate "KUnit Test for Kernel Linked-list structures" if !KUNIT_ALL_TESTS
depends on KUNIT
diff --git a/lib/Makefile b/lib/Makefile
index b1c42c10073b..8c145f85accc 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -78,7 +78,6 @@ obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.o
obj-$(CONFIG_TEST_OVERFLOW) += test_overflow.o
obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
obj-$(CONFIG_TEST_SORT) += test_sort.o
-obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
obj-$(CONFIG_TEST_PRINTF) += test_printf.o
@@ -318,3 +317,4 @@ obj-$(CONFIG_OBJAGG) += objagg.o
# KUnit tests
obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
+obj-$(CONFIG_USER_COPY_KUNIT) += user_copy_kunit.o
diff --git a/lib/test_user_copy.c b/lib/user_copy_kunit.c
similarity index 55%
rename from lib/test_user_copy.c
rename to lib/user_copy_kunit.c
index 5ff04d8fe971..c15bb1e997d6 100644
--- a/lib/test_user_copy.c
+++ b/lib/user_copy_kunit.c
@@ -16,6 +16,7 @@
#include <linux/slab.h>
#include <linux/uaccess.h>
#include <linux/vmalloc.h>
+#include <kunit/test.h>
/*
* Several 32-bit architectures support 64-bit {get,put}_user() calls.
@@ -31,26 +32,16 @@
# define TEST_U64
#endif
-#define test(condition, msg, ...) \
-({ \
- int cond = (condition); \
- if (cond) \
- pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__); \
- cond; \
-})
-
static bool is_zeroed(void *from, size_t size)
{
return memchr_inv(from, 0x0, size) == NULL;
}
-static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)
+static void test_check_nonzero_user(struct kunit *test, char *kmem, char __user *umem, size_t size)
{
- int ret = 0;
size_t start, end, i, zero_start, zero_end;
- if (test(size < 2 * PAGE_SIZE, "buffer too small"))
- return -EINVAL;
+ KUNIT_EXPECT_FALSE_MSG(test, size < 2 * PAGE_SIZE, "buffer too small");
/*
* We want to cross a page boundary to exercise the code more
@@ -84,7 +75,7 @@ static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)
for (i = zero_end; i < size; i += 2)
kmem[i] = 0xff;
- ret |= test(copy_to_user(umem, kmem, size),
+ KUNIT_EXPECT_FALSE_MSG(test, copy_to_user(umem, kmem, size),
"legitimate copy_to_user failed");
for (start = 0; start <= size; start++) {
@@ -93,35 +84,31 @@ static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)
int retval = check_zeroed_user(umem + start, len);
int expected = is_zeroed(kmem + start, len);
- ret |= test(retval != expected,
- "check_nonzero_user(=%d) != memchr_inv(=%d) mismatch (start=%zu, end=%zu)",
- retval, expected, start, end);
+ KUNIT_EXPECT_FALSE_MSG(test, retval != expected,
+ "check_nonzero_user(=%d) != memchr_inv(=%d) mismatch (start=%zu, end=%zu)",
+ retval, expected, start, end);
}
}
-
- return ret;
}
-static int test_copy_struct_from_user(char *kmem, char __user *umem,
+static void test_copy_struct_from_user(struct kunit *test, char *kmem, char __user *umem,
size_t size)
{
- int ret = 0;
char *umem_src = NULL, *expected = NULL;
size_t ksize, usize;
umem_src = kmalloc(size, GFP_KERNEL);
- ret = test(umem_src == NULL, "kmalloc failed");
- if (ret)
- goto out_free;
+ KUNIT_EXPECT_FALSE_MSG(test, umem_src == NULL, "kmalloc failed");
expected = kmalloc(size, GFP_KERNEL);
- ret = test(expected == NULL, "kmalloc failed");
- if (ret)
- goto out_free;
+
+ if (expected == NULL)
+ kfree(umem_src);
+ KUNIT_EXPECT_FALSE_MSG(test, expected == NULL, "kmalloc failed");
/* Fill umem with a fixed byte pattern. */
memset(umem_src, 0x3e, size);
- ret |= test(copy_to_user(umem, umem_src, size),
+ KUNIT_EXPECT_FALSE_MSG(test, copy_to_user(umem, umem_src, size),
"legitimate copy_to_user failed");
/* Check basic case -- (usize == ksize). */
@@ -131,9 +118,9 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
memcpy(expected, umem_src, ksize);
memset(kmem, 0x0, size);
- ret |= test(copy_struct_from_user(kmem, ksize, umem, usize),
+ KUNIT_EXPECT_FALSE_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize),
"copy_struct_from_user(usize == ksize) failed");
- ret |= test(memcmp(kmem, expected, ksize),
+ KUNIT_EXPECT_FALSE_MSG(test, memcmp(kmem, expected, ksize),
"copy_struct_from_user(usize == ksize) gives unexpected copy");
/* Old userspace case -- (usize < ksize). */
@@ -144,9 +131,9 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
memset(expected + usize, 0x0, ksize - usize);
memset(kmem, 0x0, size);
- ret |= test(copy_struct_from_user(kmem, ksize, umem, usize),
+ KUNIT_EXPECT_FALSE_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize),
"copy_struct_from_user(usize < ksize) failed");
- ret |= test(memcmp(kmem, expected, ksize),
+ KUNIT_EXPECT_FALSE_MSG(test, memcmp(kmem, expected, ksize),
"copy_struct_from_user(usize < ksize) gives unexpected copy");
/* New userspace (-E2BIG) case -- (usize > ksize). */
@@ -154,7 +141,7 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
usize = size;
memset(kmem, 0x0, size);
- ret |= test(copy_struct_from_user(kmem, ksize, umem, usize) != -E2BIG,
+ KUNIT_EXPECT_FALSE_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize) != -E2BIG,
"copy_struct_from_user(usize > ksize) didn't give E2BIG");
/* New userspace (success) case -- (usize > ksize). */
@@ -162,24 +149,18 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
usize = size;
memcpy(expected, umem_src, ksize);
- ret |= test(clear_user(umem + ksize, usize - ksize),
+ KUNIT_EXPECT_FALSE_MSG(test, clear_user(umem + ksize, usize - ksize),
"legitimate clear_user failed");
memset(kmem, 0x0, size);
- ret |= test(copy_struct_from_user(kmem, ksize, umem, usize),
+ KUNIT_EXPECT_FALSE_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize),
"copy_struct_from_user(usize > ksize) failed");
- ret |= test(memcmp(kmem, expected, ksize),
+ KUNIT_EXPECT_FALSE_MSG(test, memcmp(kmem, expected, ksize),
"copy_struct_from_user(usize > ksize) gives unexpected copy");
-
-out_free:
- kfree(expected);
- kfree(umem_src);
- return ret;
}
-static int __init test_user_copy_init(void)
+static void user_copy_test(struct kunit *test)
{
- int ret = 0;
char *kmem;
char __user *usermem;
char *bad_usermem;
@@ -192,16 +173,14 @@ static int __init test_user_copy_init(void)
#endif
kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
- if (!kmem)
- return -ENOMEM;
+ KUNIT_EXPECT_FALSE_MSG(test, kmem == NULL, "kmalloc failed");
user_addr = vm_mmap(NULL, 0, PAGE_SIZE * 2,
PROT_READ | PROT_WRITE | PROT_EXEC,
MAP_ANONYMOUS | MAP_PRIVATE, 0);
if (user_addr >= (unsigned long)(TASK_SIZE)) {
- pr_warn("Failed to allocate user memory\n");
kfree(kmem);
- return -ENOMEM;
+ KUNIT_FAIL(test, "Failed to allocate user memory");
}
usermem = (char __user *)user_addr;
@@ -211,29 +190,29 @@ static int __init test_user_copy_init(void)
* Legitimate usage: none of these copies should fail.
*/
memset(kmem, 0x3a, PAGE_SIZE * 2);
- ret |= test(copy_to_user(usermem, kmem, PAGE_SIZE),
- "legitimate copy_to_user failed");
+ KUNIT_EXPECT_FALSE_MSG(test, copy_to_user(usermem, kmem, PAGE_SIZE), "legitimate copy_to_user failed");
+
memset(kmem, 0x0, PAGE_SIZE);
- ret |= test(copy_from_user(kmem, usermem, PAGE_SIZE),
- "legitimate copy_from_user failed");
- ret |= test(memcmp(kmem, kmem + PAGE_SIZE, PAGE_SIZE),
- "legitimate usercopy failed to copy data");
-
-#define test_legit(size, check) \
- do { \
- val_##size = check; \
- ret |= test(put_user(val_##size, (size __user *)usermem), \
- "legitimate put_user (" #size ") failed"); \
- val_##size = 0; \
- ret |= test(get_user(val_##size, (size __user *)usermem), \
- "legitimate get_user (" #size ") failed"); \
- ret |= test(val_##size != check, \
- "legitimate get_user (" #size ") failed to do copy"); \
- if (val_##size != check) { \
- pr_info("0x%llx != 0x%llx\n", \
- (unsigned long long)val_##size, \
- (unsigned long long)check); \
- } \
+ KUNIT_EXPECT_FALSE_MSG(test, copy_from_user(kmem, usermem, PAGE_SIZE),
+ "legitimate copy_from_user failed");
+ KUNIT_EXPECT_FALSE_MSG(test, memcmp(kmem, kmem + PAGE_SIZE, PAGE_SIZE),
+ "legitimate usercopy failed to copy data");
+
+#define test_legit(size, check) \
+ do { \
+ val_##size = check; \
+ KUNIT_EXPECT_FALSE_MSG(test, put_user(val_##size, (size __user *)usermem), \
+ "legitimate put_user (" #size ") failed"); \
+ val_##size = 0; \
+ KUNIT_EXPECT_FALSE_MSG(test, get_user(val_##size, (size __user *)usermem), \
+ "legitimate get_user (" #size ") failed"); \
+ KUNIT_EXPECT_FALSE_MSG(test, val_##size != check, \
+ "legitimate get_user (" #size ") failed to do copy"); \
+ if (val_##size != check) { \
+ kunit_info(test, "0x%llx != 0x%llx\n", \
+ (unsigned long long)val_##size, \
+ (unsigned long long)check); \
+ } \
} while (0)
test_legit(u8, 0x5a);
@@ -245,9 +224,9 @@ static int __init test_user_copy_init(void)
#undef test_legit
/* Test usage of check_nonzero_user(). */
- ret |= test_check_nonzero_user(kmem, usermem, 2 * PAGE_SIZE);
+ test_check_nonzero_user(test, kmem, usermem, 2 * PAGE_SIZE);
/* Test usage of copy_struct_from_user(). */
- ret |= test_copy_struct_from_user(kmem, usermem, 2 * PAGE_SIZE);
+ test_copy_struct_from_user(test, kmem, usermem, 2 * PAGE_SIZE);
/*
* Invalid usage: none of these copies should succeed.
@@ -258,13 +237,13 @@ static int __init test_user_copy_init(void)
memset(kmem + PAGE_SIZE, 0, PAGE_SIZE);
/* Reject kernel-to-kernel copies through copy_from_user(). */
- ret |= test(!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
- PAGE_SIZE),
- "illegal all-kernel copy_from_user passed");
+ KUNIT_EXPECT_FALSE_MSG(test, !copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
+ PAGE_SIZE),
+ "illegal all-kernel copy_from_user passed");
/* Destination half of buffer should have been zeroed. */
- ret |= test(memcmp(kmem + PAGE_SIZE, kmem, PAGE_SIZE),
- "zeroing failure for illegal all-kernel copy_from_user");
+ KUNIT_EXPECT_FALSE_MSG(test, memcmp(kmem + PAGE_SIZE, kmem, PAGE_SIZE),
+ "zeroing failure for illegal all-kernel copy_from_user");
#if 0
/*
@@ -273,30 +252,28 @@ static int __init test_user_copy_init(void)
* to be tested in LKDTM instead, since this test module does not
* expect to explode.
*/
- ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem,
- PAGE_SIZE),
- "illegal reversed copy_from_user passed");
+ KUNIT_EXPECT_FALSE_MSG(test, !copy_from_user(bad_usermem, (char __user *)kmem,
+ PAGE_SIZE),
+ "illegal reversed copy_from_user passed");
#endif
- ret |= test(!copy_to_user((char __user *)kmem, kmem + PAGE_SIZE,
- PAGE_SIZE),
- "illegal all-kernel copy_to_user passed");
- ret |= test(!copy_to_user((char __user *)kmem, bad_usermem,
- PAGE_SIZE),
- "illegal reversed copy_to_user passed");
-
-#define test_illegal(size, check) \
- do { \
- val_##size = (check); \
- ret |= test(!get_user(val_##size, (size __user *)kmem), \
- "illegal get_user (" #size ") passed"); \
- ret |= test(val_##size != (size)0, \
- "zeroing failure for illegal get_user (" #size ")"); \
- if (val_##size != (size)0) { \
- pr_info("0x%llx != 0\n", \
- (unsigned long long)val_##size); \
- } \
- ret |= test(!put_user(val_##size, (size __user *)kmem), \
- "illegal put_user (" #size ") passed"); \
+ KUNIT_EXPECT_FALSE_MSG(test, !copy_to_user((char __user *)kmem, kmem + PAGE_SIZE, PAGE_SIZE),
+ "illegal all-kernel copy_to_user passed");
+ KUNIT_EXPECT_FALSE_MSG(test, !copy_to_user((char __user *)kmem, bad_usermem, PAGE_SIZE),
+ "illegal reversed copy_to_user passed");
+
+#define test_illegal(size, check) \
+ do { \
+ val_##size = (check); \
+ KUNIT_EXPECT_FALSE_MSG(test, !get_user(val_##size, (size __user *)kmem), \
+ "illegal get_user (" #size ") passed"); \
+ KUNIT_EXPECT_FALSE_MSG(test, val_##size != (size)0, \
+ "zeroing failure for illegal get_user (" #size ")"); \
+ if (val_##size != (size)0) { \
+ kunit_info(test, "0x%llx != 0\n", \
+ (unsigned long long)val_##size); \
+ } \
+ KUNIT_EXPECT_FALSE_MSG(test, !put_user(val_##size, (size __user *)kmem), \
+ "illegal put_user (" #size ") passed"); \
} while (0)
test_illegal(u8, 0x5a);
@@ -309,23 +286,18 @@ static int __init test_user_copy_init(void)
vm_munmap(user_addr, PAGE_SIZE * 2);
kfree(kmem);
-
- if (ret == 0) {
- pr_info("tests passed.\n");
- return 0;
- }
-
- return -EINVAL;
}
-module_init(test_user_copy_init);
-
-static void __exit test_user_copy_exit(void)
-{
- pr_info("unloaded.\n");
-}
+static struct kunit_case user_copy_test_cases[] = {
+ KUNIT_CASE(user_copy_test),
+ {}
+};
-module_exit(test_user_copy_exit);
+static struct kunit_suite user_copy_test_suite = {
+ .name = "user_copy",
+ .test_cases = user_copy_test_cases,
+};
+kunit_test_suites(&user_copy_test_suite);
MODULE_AUTHOR("Kees Cook <[email protected]>");
MODULE_LICENSE("GPL");
--
2.26.2
On Wed, Jul 15, 2020 at 11:11 AM Vitor Massaru Iha <[email protected]> wrote:
>
> Currently, KUnit does not allow the use of tests as a module.
> This prevents the implementation of tests that require userspace.
If this is what I think it is, thanks! I'll hopefully get a chance to
play with it over the next few days.
Can we clarify what this means: the current description is a little
misleading, as KUnit tests can already be built and run as modules,
and "tests that require userspace" is a bit broad.
As I understand it, this patchset does three things:
- Let kunit_tool install modules to a root filesystem and boot UML
with that filesystem.
- Have tests inherit the mm of the process that started them, which
(if the test is in a module), provides a user-space memory context so
that copy_{from,to}_user() works.
- Port the test_user_copy.c tests to KUnit, using this new feature.
A few comments from my quick glance over it:
- The rootfs support is useful: I'm curious how it'll interact with
non-UML architectures in [1]. It'd be nice for this to be extensible
and to not explicitly state UML where possible.
- The inheriting of the mm stuff still means that
copy_{from,to}_user() will only work if loaded as a module. This
really needs to be documented. (Ideally, we'd find a way of having
this work even for built-in tests, but I don't have any real ideas as
to how that could be done).
- It'd be nice to split the test_user_copy.c test port into a separate
commit. In fact, it may make sense to also split the kunit_tool
changes and the mm changes into separate series, as they're both quite
useful independently.
Cheers,
-- David
> This patchset makes this possible by introducing the use of
> the root filesystem in KUnit. And it allows the use of tests
> that can be compiled as a module
>
> Vitor Massaru Iha (3):
> kunit: tool: Add support root filesystem in kunit-tool
> lib: Allows to borrow mm in userspace on KUnit
> lib: Convert test_user_copy to KUnit test
>
> include/kunit/test.h | 1 +
> lib/Kconfig.debug | 17 ++
> lib/Makefile | 2 +-
> lib/kunit/try-catch.c | 15 +-
> lib/{test_user_copy.c => user_copy_kunit.c} | 196 +++++++++-----------
> tools/testing/kunit/kunit.py | 37 +++-
> tools/testing/kunit/kunit_kernel.py | 105 +++++++++--
> 7 files changed, 238 insertions(+), 135 deletions(-)
> rename lib/{test_user_copy.c => user_copy_kunit.c} (55%)
>
>
> base-commit: 725aca9585956676687c4cb803e88f770b0df2b2
> prerequisite-patch-id: 582b6d9d28ce4b71628890ec832df6522ca68de0
> --
> 2.26.2
>
On Tue, Jul 14, 2020 at 8:11 PM Vitor Massaru Iha <[email protected]> wrote:
>
> This makes it possible to use KUnit tests as modules.
> And with that the tests can run in userspace.
>
> The filesystem was created using debootstrap:
> sudo debootstrap buster buster_rootfs
>
> And change the owner of the root filesystem files
> for your user:
> sudo chown -R $USER:$USER buster_rootfs
Probably want to add this to the documentation for KUnit.
> For the kunit-tool to correctly capture the test results,
> uml_utilities must be installed on the host to halt uml.
>
> Signed-off-by: Vitor Massaru Iha <[email protected]>
Overall this looks really good! I am really excited to see this!
Reviewed-by: Brendan Higgins <[email protected]>
> ---
> tools/testing/kunit/kunit.py | 37 ++++++++--
> tools/testing/kunit/kunit_kernel.py | 105 ++++++++++++++++++++++++----
> 2 files changed, 121 insertions(+), 21 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> index 787b6d4ad716..6d8ba0215b90 100755
> --- a/tools/testing/kunit/kunit.py
> +++ b/tools/testing/kunit/kunit.py
> @@ -23,16 +23,16 @@ import kunit_parser
> KunitResult = namedtuple('KunitResult', ['status','result','elapsed_time'])
>
> KunitConfigRequest = namedtuple('KunitConfigRequest',
> - ['build_dir', 'make_options'])
> + ['build_dir', 'uml_root_dir', 'make_options'])
> KunitBuildRequest = namedtuple('KunitBuildRequest',
> - ['jobs', 'build_dir', 'alltests',
> + ['jobs', 'build_dir', 'uml_root_dir', 'alltests',
> 'make_options'])
> KunitExecRequest = namedtuple('KunitExecRequest',
> - ['timeout', 'build_dir', 'alltests'])
> + ['timeout', 'build_dir', 'uml_root_dir', 'alltests'])
> KunitParseRequest = namedtuple('KunitParseRequest',
> ['raw_output', 'input_data'])
> KunitRequest = namedtuple('KunitRequest', ['raw_output','timeout', 'jobs',
> - 'build_dir', 'alltests',
> + 'build_dir', 'uml_root_dir', 'alltests',
> 'make_options'])
>
> KernelDirectoryPath = sys.argv[0].split('tools/testing/kunit/')[0]
> @@ -47,7 +47,6 @@ def create_default_kunitconfig():
> if not os.path.exists(kunit_kernel.kunitconfig_path):
> shutil.copyfile('arch/um/configs/kunit_defconfig',
> kunit_kernel.kunitconfig_path)
> -
> def get_kernel_root_path():
> parts = sys.argv[0] if not __file__ else __file__
> parts = os.path.realpath(parts).split('tools/testing/kunit')
> @@ -58,10 +57,22 @@ def get_kernel_root_path():
> def config_tests(linux: kunit_kernel.LinuxSourceTree,
> request: KunitConfigRequest) -> KunitResult:
> kunit_parser.print_with_timestamp('Configuring KUnit Kernel ...')
> + defconfig = False
>
> config_start = time.time()
> create_default_kunitconfig()
> - success = linux.build_reconfig(request.build_dir, request.make_options)
> +
> + if request.uml_root_dir != None:
> + if os.path.exists(request.uml_root_dir):
> + kunit_kernel.uml_root_path = os.path.abspath(request.uml_root_dir)
> + defconfig = True
> + else:
> + config_end = time.time()
> + return KunitResult(KunitStatus.CONFIG_FAILURE,
> + 'could not configure kernel',
> + config_end - config_start)
> +
> + success = linux.build_reconfig(request.build_dir, defconfig, request.make_options)
> config_end = time.time()
> if not success:
> return KunitResult(KunitStatus.CONFIG_FAILURE,
> @@ -79,6 +90,7 @@ def build_tests(linux: kunit_kernel.LinuxSourceTree,
> success = linux.build_um_kernel(request.alltests,
> request.jobs,
> request.build_dir,
> + request.uml_root_dir,
> request.make_options)
> build_end = time.time()
> if not success:
> @@ -97,7 +109,7 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree,
> test_start = time.time()
> result = linux.run_kernel(
> timeout=None if request.alltests else request.timeout,
> - build_dir=request.build_dir)
> + build_dir=request.build_dir, uml_root_dir=request.uml_root_dir)
>
> test_end = time.time()
>
> @@ -130,12 +142,14 @@ def run_tests(linux: kunit_kernel.LinuxSourceTree,
> run_start = time.time()
>
> config_request = KunitConfigRequest(request.build_dir,
> + request.uml_root_dir,
> request.make_options)
> config_result = config_tests(linux, config_request)
> if config_result.status != KunitStatus.SUCCESS:
> return config_result
>
> build_request = KunitBuildRequest(request.jobs, request.build_dir,
> + request.uml_root_dir,
> request.alltests,
> request.make_options)
> build_result = build_tests(linux, build_request)
> @@ -143,6 +157,7 @@ def run_tests(linux: kunit_kernel.LinuxSourceTree,
> return build_result
>
> exec_request = KunitExecRequest(request.timeout, request.build_dir,
> + request.uml_root_dir,
> request.alltests)
> exec_result = exec_tests(linux, exec_request)
> if exec_result.status != KunitStatus.SUCCESS:
> @@ -168,6 +183,10 @@ def add_common_opts(parser):
> help='As in the make command, it specifies the build '
> 'directory.',
> type=str, default='.kunit', metavar='build_dir')
> + parser.add_argument('--uml_root_dir',
> + help='To load modules, a root filesystem is '
> + 'required to install and load these modules.',
> + type=str, default=None, metavar='uml_root_dir')
> parser.add_argument('--make_options',
> help='X=Y make option, can be repeated.',
> action='append')
> @@ -252,6 +271,7 @@ def main(argv, linux=None):
> cli_args.timeout,
> cli_args.jobs,
> cli_args.build_dir,
> + cli_args.uml_root_dir,
> cli_args.alltests,
> cli_args.make_options)
> result = run_tests(linux, request)
> @@ -272,6 +292,7 @@ def main(argv, linux=None):
> linux = kunit_kernel.LinuxSourceTree()
>
> request = KunitConfigRequest(cli_args.build_dir,
> + cli_args.uml_root_dir,
> cli_args.make_options)
> result = config_tests(linux, request)
> kunit_parser.print_with_timestamp((
> @@ -295,6 +316,7 @@ def main(argv, linux=None):
>
> request = KunitBuildRequest(cli_args.jobs,
> cli_args.build_dir,
> + cli_args.uml_root_dir,
> cli_args.alltests,
> cli_args.make_options)
> result = build_tests(linux, request)
> @@ -319,6 +341,7 @@ def main(argv, linux=None):
>
> exec_request = KunitExecRequest(cli_args.timeout,
> cli_args.build_dir,
> + cli_args.uml_root_dir,
> cli_args.alltests)
> exec_result = exec_tests(linux, exec_request)
> parse_request = KunitParseRequest(cli_args.raw_output,
> diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> index 63dbda2d029f..d712f4619eaa 100644
> --- a/tools/testing/kunit/kunit_kernel.py
> +++ b/tools/testing/kunit/kunit_kernel.py
> @@ -11,6 +11,7 @@ import logging
> import subprocess
> import os
> import signal
> +import time
>
> from contextlib import ExitStack
>
> @@ -19,7 +20,59 @@ import kunit_parser
>
> KCONFIG_PATH = '.config'
> kunitconfig_path = '.kunitconfig'
> +X86_64_DEFCONFIG_PATH = 'arch/um/configs/x86_64_defconfig'
> BROKEN_ALLCONFIG_PATH = 'tools/testing/kunit/configs/broken_on_uml.config'
> +uml_root_path = None
nit: I don't like global variables. Can we just pass this in or store
it in a data structure. The above are all constants.
> +
> +make_cmd = {
This looks like a constant. Please capitalize it.
> + 'make': {
> + 'command': ['make', 'ARCH=um'],
> + 'msg_error': 'Could not call execute make: ',
> + },
> + 'make_modules': {
> + 'command': ['make', 'modules', 'ARCH=um'],
> + 'msg_error': 'Could not call execute make modules: ',
> + },
> + 'make_modules_install': {
> + 'command': ['make', 'modules_install', 'ARCH=um'],
> + 'msg_error': 'Could not call execute make modules_install: ',
> + }
> +}
>
> +def halt_uml():
> + try:
> + subprocess.call(['uml_mconsole', 'kunitid', 'halt'])
> + except OSError as e:
> + raise ConfigError('Could not call uml_mconsole ' + e)
> + except subprocess.CalledProcessError as e:
> + raise ConfigError(e.output)
> +
> +
> +def enable_uml_modules_on_boot(output_command):
> + uml_modules_path = None
> + found_kernel_version = False
> + modules = []
> + for i in output_command.decode('utf-8').split():
> + if found_kernel_version:
> + kernel_version = i
> + uml_modules_path = os.path.join(uml_root_path,
> + 'lib/modules/', kernel_version, 'kernel/lib/')
> + break
> + if 'DEPMOD' in i:
> + found_kernel_version = True
> +
> + try:
> + if os.path.exists(uml_modules_path):
> + modules = subprocess.check_output(['ls',
> + uml_modules_path]).decode('utf-8').split()
> + except OSError as e:
> + raise ConfigError('Could not list directory ' + e)
> + except subprocess.CalledProcessError as e:
> + raise ConfigError(e.output)
> +
> + with open(os.path.join(uml_root_path, 'etc/modules'), 'w') as f:
> + for i in modules:
> + f.write(i.replace('.ko', ''))
>
> class ConfigError(Exception):
> """Represents an error trying to configure the Linux kernel."""
> @@ -70,20 +123,27 @@ class LinuxSourceTreeOperations(object):
> kunit_parser.print_with_timestamp(
> 'Starting Kernel with all configs takes a few minutes...')
>
> - def make(self, jobs, build_dir, make_options):
> - command = ['make', 'ARCH=um', '--jobs=' + str(jobs)]
> + def make(self, cmd, jobs, build_dir, make_options):
> + command = make_cmd[cmd]['command'] + ['--jobs=' + str(jobs)]
> +
> if make_options:
> command.extend(make_options)
> if build_dir:
> command += ['O=' + build_dir]
> +
> + if cmd == 'make_modules_install':
> + command += ['INSTALL_MOD_PATH=' + uml_root_path]
> +
> try:
> - subprocess.check_output(command)
> + output = subprocess.check_output(command)
> + if cmd == 'make_modules_install':
> + enable_uml_modules_on_boot(output)
> except OSError as e:
> - raise BuildError('Could not call execute make: ' + e)
> + raise BuildError(make_cmd[cmd][msg_error] + e)
> except subprocess.CalledProcessError as e:
> raise BuildError(e.output)
>
> - def linux_bin(self, params, timeout, build_dir, outfile):
> + def linux_bin(self, params, timeout, build_dir, uml_root_dir, outfile):
> """Runs the Linux UML binary. Must be named 'linux'."""
> linux_bin = './linux'
> if build_dir:
> @@ -92,7 +152,11 @@ class LinuxSourceTreeOperations(object):
> process = subprocess.Popen([linux_bin] + params,
> stdout=output,
> stderr=subprocess.STDOUT)
> - process.wait(timeout)
> + if uml_root_dir:
> + time.sleep(timeout)
> + halt_uml()
> + else:
> + process.wait(timeout)
>
>
> def get_kconfig_path(build_dir):
> @@ -132,11 +196,16 @@ class LinuxSourceTree(object):
> return False
> return True
>
> - def build_config(self, build_dir, make_options):
> + def build_config(self, build_dir, defconfig, make_options):
> kconfig_path = get_kconfig_path(build_dir)
> if build_dir and not os.path.exists(build_dir):
> os.mkdir(build_dir)
> self._kconfig.write_to_file(kconfig_path)
> +
> + if defconfig:
> + with open(kconfig_path, 'a') as fw:
> + with open(X86_64_DEFCONFIG_PATH, 'r') as fr:
> + fw.write(fr.read())
> try:
> self._ops.make_olddefconfig(build_dir, make_options)
> except ConfigError as e:
> @@ -144,7 +213,7 @@ class LinuxSourceTree(object):
> return False
> return self.validate_config(build_dir)
>
> - def build_reconfig(self, build_dir, make_options):
> + def build_reconfig(self, build_dir, defconfig, make_options):
> """Creates a new .config if it is not a subset of the .kunitconfig."""
> kconfig_path = get_kconfig_path(build_dir)
> if os.path.exists(kconfig_path):
> @@ -153,28 +222,36 @@ class LinuxSourceTree(object):
> if not self._kconfig.is_subset_of(existing_kconfig):
> print('Regenerating .config ...')
> os.remove(kconfig_path)
> - return self.build_config(build_dir, make_options)
> + return self.build_config(build_dir, defconfig, make_options)
> else:
> return True
> else:
> print('Generating .config ...')
> - return self.build_config(build_dir, make_options)
> + return self.build_config(build_dir, defconfig, make_options)
>
> - def build_um_kernel(self, alltests, jobs, build_dir, make_options):
> + def build_um_kernel(self, alltests, jobs, build_dir, uml_root_dir, make_options):
> if alltests:
> self._ops.make_allyesconfig()
> try:
> self._ops.make_olddefconfig(build_dir, make_options)
> - self._ops.make(jobs, build_dir, make_options)
> + self._ops.make('make', jobs, build_dir, make_options)
> + if uml_root_dir:
> + self._ops.make('make_modules', jobs, build_dir,
> + make_options)
> + self._ops.make('make_modules_install', jobs,
> + build_dir, make_options)
> except (ConfigError, BuildError) as e:
> logging.error(e)
> return False
> return self.validate_config(build_dir)
>
> - def run_kernel(self, args=[], build_dir='', timeout=None):
> + def run_kernel(self, args=[], build_dir='', uml_root_dir=None, timeout=None):
> args.extend(['mem=1G'])
> + if uml_root_dir:
> + args.extend(['root=/dev/root', 'rootfstype=hostfs',
> + 'rootflags=' + uml_root_path, 'umid=kunitid'])
> outfile = 'test.log'
> - self._ops.linux_bin(args, timeout, build_dir, outfile)
> + self._ops.linux_bin(args, timeout, build_dir, uml_root_dir, outfile)
> subprocess.call(['stty', 'sane'])
> with open(outfile, 'r') as file:
> for line in file:
> --
> 2.26.2
>
On Tue, Jul 14, 2020 at 8:11 PM Vitor Massaru Iha <[email protected]> wrote:
Probably want to add more of a description here as what you are doing
is not entirely trivial to someone not familiar with mm contexts.
>
> Signed-off-by: Vitor Massaru Iha <[email protected]>
> ---
> include/kunit/test.h | 1 +
> lib/kunit/try-catch.c | 15 ++++++++++++++-
> 2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 59f3144f009a..49c38bdcb93e 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -222,6 +222,7 @@ struct kunit {
> * protect it with some type of lock.
> */
> struct list_head resources; /* Protected by lock. */
> + struct mm_struct *mm;
Part of me thinks we should put a better name here, part of me thinks
it is fine because it matches the convention.
Either way, this DEFINITELY deserves a comment explaining what it is,
why it exists, and how it should/shouldn't be used.
> };
>
> void kunit_init_test(struct kunit *test, const char *name, char *log);
> diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> index 0dd434e40487..f677c2f2a51a 100644
> --- a/lib/kunit/try-catch.c
> +++ b/lib/kunit/try-catch.c
> @@ -11,7 +11,8 @@
> #include <linux/completion.h>
> #include <linux/kernel.h>
> #include <linux/kthread.h>
> -
> +#include <linux/sched/mm.h>
> +#include <linux/sched/task.h>
> #include "try-catch-impl.h"
>
> void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch)
> @@ -24,8 +25,17 @@ EXPORT_SYMBOL_GPL(kunit_try_catch_throw);
> static int kunit_generic_run_threadfn_adapter(void *data)
> {
> struct kunit_try_catch *try_catch = data;
> + struct kunit *test = try_catch->test;
> +
> + if (test->mm != NULL)
> + kthread_use_mm(try_catch->test->mm);
>
> try_catch->try(try_catch->context);
> + if (try_catch->test->mm) {
Here and below: You already have a pointer to test. You should use it.
> + if (test->mm != NULL)
> + kthread_unuse_mm(try_catch->test->mm);
> + try_catch->test->mm = NULL;
> + }
>
> complete_and_exit(try_catch->try_completion, 0);
> }
> @@ -65,6 +75,9 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
> try_catch->context = context;
> try_catch->try_completion = &try_completion;
> try_catch->try_result = 0;
> +
> + test->mm = get_task_mm(current);
> +
> task_struct = kthread_run(kunit_generic_run_threadfn_adapter,
> try_catch,
> "kunit_try_catch_thread");
> --
> 2.26.2
>
On Tue, Jul 14, 2020 at 8:11 PM Vitor Massaru Iha <[email protected]> wrote:
>
> This adds the conversion of the runtime tests of test_user_copy fuctions,
> from `lib/test_user_copy.c`to KUnit tests.
>
> Signed-off-by: Vitor Massaru Iha <[email protected]>
> ---
> lib/Kconfig.debug | 17 ++
> lib/Makefile | 2 +-
> lib/{test_user_copy.c => user_copy_kunit.c} | 196 +++++++++-----------
> 3 files changed, 102 insertions(+), 113 deletions(-)
> rename lib/{test_user_copy.c => user_copy_kunit.c} (55%)
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 9ad9210d70a1..29558674c011 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2154,6 +2154,23 @@ config SYSCTL_KUNIT_TEST
>
> If unsure, say N.
>
> +config USER_COPY_KUNIT
> + tristate "KUnit Test for user/kernel boundary protections"
> + depends on KUNIT
> + depends on m
> + help
> + This builds the "test_user_copy" module that runs sanity checks
> + on the copy_to/from_user infrastructure, making sure basic
> + user/kernel boundary testing is working. If it fails to load,
> + a regression has been detected in the user/kernel memory boundary
> + protections.
> +
> + 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.
Where do you delete the entry for CONFIG_TEST_USER_COPY? I don't see it here.
> config LIST_KUNIT_TEST
> tristate "KUnit Test for Kernel Linked-list structures" if !KUNIT_ALL_TESTS
> depends on KUNIT
> diff --git a/lib/Makefile b/lib/Makefile
> index b1c42c10073b..8c145f85accc 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -78,7 +78,6 @@ obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.o
> obj-$(CONFIG_TEST_OVERFLOW) += test_overflow.o
> obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
> obj-$(CONFIG_TEST_SORT) += test_sort.o
> -obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
> obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
> obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
> obj-$(CONFIG_TEST_PRINTF) += test_printf.o
> @@ -318,3 +317,4 @@ obj-$(CONFIG_OBJAGG) += objagg.o
> # KUnit tests
> obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
> obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
> +obj-$(CONFIG_USER_COPY_KUNIT) += user_copy_kunit.o
> diff --git a/lib/test_user_copy.c b/lib/user_copy_kunit.c
> similarity index 55%
> rename from lib/test_user_copy.c
> rename to lib/user_copy_kunit.c
> index 5ff04d8fe971..c15bb1e997d6 100644
> --- a/lib/test_user_copy.c
> +++ b/lib/user_copy_kunit.c
> @@ -16,6 +16,7 @@
> #include <linux/slab.h>
> #include <linux/uaccess.h>
> #include <linux/vmalloc.h>
> +#include <kunit/test.h>
>
> /*
> * Several 32-bit architectures support 64-bit {get,put}_user() calls.
> @@ -31,26 +32,16 @@
> # define TEST_U64
> #endif
>
> -#define test(condition, msg, ...) \
> -({ \
> - int cond = (condition); \
> - if (cond) \
> - pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__); \
> - cond; \
> -})
> -
> static bool is_zeroed(void *from, size_t size)
> {
> return memchr_inv(from, 0x0, size) == NULL;
> }
>
> -static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)
> +static void test_check_nonzero_user(struct kunit *test, char *kmem, char __user *umem, size_t size)
> {
> - int ret = 0;
> size_t start, end, i, zero_start, zero_end;
>
> - if (test(size < 2 * PAGE_SIZE, "buffer too small"))
> - return -EINVAL;
> + KUNIT_EXPECT_FALSE_MSG(test, size < 2 * PAGE_SIZE, "buffer too small");
>
> /*
> * We want to cross a page boundary to exercise the code more
> @@ -84,7 +75,7 @@ static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)
> for (i = zero_end; i < size; i += 2)
> kmem[i] = 0xff;
>
> - ret |= test(copy_to_user(umem, kmem, size),
> + KUNIT_EXPECT_FALSE_MSG(test, copy_to_user(umem, kmem, size),
> "legitimate copy_to_user failed");
>
> for (start = 0; start <= size; start++) {
> @@ -93,35 +84,31 @@ static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)
> int retval = check_zeroed_user(umem + start, len);
> int expected = is_zeroed(kmem + start, len);
>
> - ret |= test(retval != expected,
> - "check_nonzero_user(=%d) != memchr_inv(=%d) mismatch (start=%zu, end=%zu)",
> - retval, expected, start, end);
> + KUNIT_EXPECT_FALSE_MSG(test, retval != expected,
> + "check_nonzero_user(=%d) != memchr_inv(=%d) mismatch (start=%zu, end=%zu)",
> + retval, expected, start, end);
> }
> }
> -
> - return ret;
> }
>
> -static int test_copy_struct_from_user(char *kmem, char __user *umem,
> +static void test_copy_struct_from_user(struct kunit *test, char *kmem, char __user *umem,
> size_t size)
> {
> - int ret = 0;
> char *umem_src = NULL, *expected = NULL;
> size_t ksize, usize;
>
> umem_src = kmalloc(size, GFP_KERNEL);
> - ret = test(umem_src == NULL, "kmalloc failed");
> - if (ret)
> - goto out_free;
> + KUNIT_EXPECT_FALSE_MSG(test, umem_src == NULL, "kmalloc failed");
>
> expected = kmalloc(size, GFP_KERNEL);
> - ret = test(expected == NULL, "kmalloc failed");
> - if (ret)
> - goto out_free;
> +
> + if (expected == NULL)
> + kfree(umem_src);
> + KUNIT_EXPECT_FALSE_MSG(test, expected == NULL, "kmalloc failed");
>
> /* Fill umem with a fixed byte pattern. */
> memset(umem_src, 0x3e, size);
> - ret |= test(copy_to_user(umem, umem_src, size),
> + KUNIT_EXPECT_FALSE_MSG(test, copy_to_user(umem, umem_src, size),
> "legitimate copy_to_user failed");
>
> /* Check basic case -- (usize == ksize). */
> @@ -131,9 +118,9 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
> memcpy(expected, umem_src, ksize);
>
> memset(kmem, 0x0, size);
> - ret |= test(copy_struct_from_user(kmem, ksize, umem, usize),
> + KUNIT_EXPECT_FALSE_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize),
> "copy_struct_from_user(usize == ksize) failed");
> - ret |= test(memcmp(kmem, expected, ksize),
> + KUNIT_EXPECT_FALSE_MSG(test, memcmp(kmem, expected, ksize),
> "copy_struct_from_user(usize == ksize) gives unexpected copy");
>
> /* Old userspace case -- (usize < ksize). */
> @@ -144,9 +131,9 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
> memset(expected + usize, 0x0, ksize - usize);
>
> memset(kmem, 0x0, size);
> - ret |= test(copy_struct_from_user(kmem, ksize, umem, usize),
> + KUNIT_EXPECT_FALSE_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize),
> "copy_struct_from_user(usize < ksize) failed");
> - ret |= test(memcmp(kmem, expected, ksize),
> + KUNIT_EXPECT_FALSE_MSG(test, memcmp(kmem, expected, ksize),
> "copy_struct_from_user(usize < ksize) gives unexpected copy");
>
> /* New userspace (-E2BIG) case -- (usize > ksize). */
> @@ -154,7 +141,7 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
> usize = size;
>
> memset(kmem, 0x0, size);
> - ret |= test(copy_struct_from_user(kmem, ksize, umem, usize) != -E2BIG,
> + KUNIT_EXPECT_FALSE_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize) != -E2BIG,
> "copy_struct_from_user(usize > ksize) didn't give E2BIG");
>
> /* New userspace (success) case -- (usize > ksize). */
> @@ -162,24 +149,18 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
> usize = size;
>
> memcpy(expected, umem_src, ksize);
> - ret |= test(clear_user(umem + ksize, usize - ksize),
> + KUNIT_EXPECT_FALSE_MSG(test, clear_user(umem + ksize, usize - ksize),
> "legitimate clear_user failed");
>
> memset(kmem, 0x0, size);
> - ret |= test(copy_struct_from_user(kmem, ksize, umem, usize),
> + KUNIT_EXPECT_FALSE_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize),
> "copy_struct_from_user(usize > ksize) failed");
> - ret |= test(memcmp(kmem, expected, ksize),
> + KUNIT_EXPECT_FALSE_MSG(test, memcmp(kmem, expected, ksize),
> "copy_struct_from_user(usize > ksize) gives unexpected copy");
> -
> -out_free:
> - kfree(expected);
> - kfree(umem_src);
> - return ret;
> }
>
> -static int __init test_user_copy_init(void)
> +static void user_copy_test(struct kunit *test)
> {
> - int ret = 0;
> char *kmem;
> char __user *usermem;
> char *bad_usermem;
> @@ -192,16 +173,14 @@ static int __init test_user_copy_init(void)
> #endif
>
> kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
> - if (!kmem)
> - return -ENOMEM;
> + KUNIT_EXPECT_FALSE_MSG(test, kmem == NULL, "kmalloc failed");
>
> user_addr = vm_mmap(NULL, 0, PAGE_SIZE * 2,
> PROT_READ | PROT_WRITE | PROT_EXEC,
> MAP_ANONYMOUS | MAP_PRIVATE, 0);
> if (user_addr >= (unsigned long)(TASK_SIZE)) {
> - pr_warn("Failed to allocate user memory\n");
> kfree(kmem);
> - return -ENOMEM;
> + KUNIT_FAIL(test, "Failed to allocate user memory");
> }
>
> usermem = (char __user *)user_addr;
> @@ -211,29 +190,29 @@ static int __init test_user_copy_init(void)
> * Legitimate usage: none of these copies should fail.
> */
> memset(kmem, 0x3a, PAGE_SIZE * 2);
> - ret |= test(copy_to_user(usermem, kmem, PAGE_SIZE),
> - "legitimate copy_to_user failed");
> + KUNIT_EXPECT_FALSE_MSG(test, copy_to_user(usermem, kmem, PAGE_SIZE), "legitimate copy_to_user failed");
> +
> memset(kmem, 0x0, PAGE_SIZE);
> - ret |= test(copy_from_user(kmem, usermem, PAGE_SIZE),
> - "legitimate copy_from_user failed");
> - ret |= test(memcmp(kmem, kmem + PAGE_SIZE, PAGE_SIZE),
> - "legitimate usercopy failed to copy data");
> -
> -#define test_legit(size, check) \
> - do { \
> - val_##size = check; \
> - ret |= test(put_user(val_##size, (size __user *)usermem), \
> - "legitimate put_user (" #size ") failed"); \
> - val_##size = 0; \
> - ret |= test(get_user(val_##size, (size __user *)usermem), \
> - "legitimate get_user (" #size ") failed"); \
> - ret |= test(val_##size != check, \
> - "legitimate get_user (" #size ") failed to do copy"); \
> - if (val_##size != check) { \
> - pr_info("0x%llx != 0x%llx\n", \
> - (unsigned long long)val_##size, \
> - (unsigned long long)check); \
> - } \
> + KUNIT_EXPECT_FALSE_MSG(test, copy_from_user(kmem, usermem, PAGE_SIZE),
> + "legitimate copy_from_user failed");
> + KUNIT_EXPECT_FALSE_MSG(test, memcmp(kmem, kmem + PAGE_SIZE, PAGE_SIZE),
> + "legitimate usercopy failed to copy data");
> +
> +#define test_legit(size, check) \
> + do { \
> + val_##size = check; \
> + KUNIT_EXPECT_FALSE_MSG(test, put_user(val_##size, (size __user *)usermem), \
> + "legitimate put_user (" #size ") failed"); \
> + val_##size = 0; \
> + KUNIT_EXPECT_FALSE_MSG(test, get_user(val_##size, (size __user *)usermem), \
> + "legitimate get_user (" #size ") failed"); \
> + KUNIT_EXPECT_FALSE_MSG(test, val_##size != check, \
> + "legitimate get_user (" #size ") failed to do copy"); \
> + if (val_##size != check) { \
> + kunit_info(test, "0x%llx != 0x%llx\n", \
> + (unsigned long long)val_##size, \
> + (unsigned long long)check); \
> + } \
> } while (0)
>
> test_legit(u8, 0x5a);
> @@ -245,9 +224,9 @@ static int __init test_user_copy_init(void)
> #undef test_legit
>
> /* Test usage of check_nonzero_user(). */
> - ret |= test_check_nonzero_user(kmem, usermem, 2 * PAGE_SIZE);
> + test_check_nonzero_user(test, kmem, usermem, 2 * PAGE_SIZE);
> /* Test usage of copy_struct_from_user(). */
> - ret |= test_copy_struct_from_user(kmem, usermem, 2 * PAGE_SIZE);
> + test_copy_struct_from_user(test, kmem, usermem, 2 * PAGE_SIZE);
>
> /*
> * Invalid usage: none of these copies should succeed.
> @@ -258,13 +237,13 @@ static int __init test_user_copy_init(void)
> memset(kmem + PAGE_SIZE, 0, PAGE_SIZE);
>
> /* Reject kernel-to-kernel copies through copy_from_user(). */
> - ret |= test(!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
> - PAGE_SIZE),
> - "illegal all-kernel copy_from_user passed");
> + KUNIT_EXPECT_FALSE_MSG(test, !copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
> + PAGE_SIZE),
> + "illegal all-kernel copy_from_user passed");
>
> /* Destination half of buffer should have been zeroed. */
> - ret |= test(memcmp(kmem + PAGE_SIZE, kmem, PAGE_SIZE),
> - "zeroing failure for illegal all-kernel copy_from_user");
> + KUNIT_EXPECT_FALSE_MSG(test, memcmp(kmem + PAGE_SIZE, kmem, PAGE_SIZE),
> + "zeroing failure for illegal all-kernel copy_from_user");
>
> #if 0
> /*
> @@ -273,30 +252,28 @@ static int __init test_user_copy_init(void)
> * to be tested in LKDTM instead, since this test module does not
> * expect to explode.
> */
> - ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem,
> - PAGE_SIZE),
> - "illegal reversed copy_from_user passed");
> + KUNIT_EXPECT_FALSE_MSG(test, !copy_from_user(bad_usermem, (char __user *)kmem,
> + PAGE_SIZE),
> + "illegal reversed copy_from_user passed");
> #endif
> - ret |= test(!copy_to_user((char __user *)kmem, kmem + PAGE_SIZE,
> - PAGE_SIZE),
> - "illegal all-kernel copy_to_user passed");
> - ret |= test(!copy_to_user((char __user *)kmem, bad_usermem,
> - PAGE_SIZE),
> - "illegal reversed copy_to_user passed");
> -
> -#define test_illegal(size, check) \
> - do { \
> - val_##size = (check); \
> - ret |= test(!get_user(val_##size, (size __user *)kmem), \
> - "illegal get_user (" #size ") passed"); \
> - ret |= test(val_##size != (size)0, \
> - "zeroing failure for illegal get_user (" #size ")"); \
> - if (val_##size != (size)0) { \
> - pr_info("0x%llx != 0\n", \
> - (unsigned long long)val_##size); \
> - } \
> - ret |= test(!put_user(val_##size, (size __user *)kmem), \
> - "illegal put_user (" #size ") passed"); \
> + KUNIT_EXPECT_FALSE_MSG(test, !copy_to_user((char __user *)kmem, kmem + PAGE_SIZE, PAGE_SIZE),
> + "illegal all-kernel copy_to_user passed");
> + KUNIT_EXPECT_FALSE_MSG(test, !copy_to_user((char __user *)kmem, bad_usermem, PAGE_SIZE),
> + "illegal reversed copy_to_user passed");
> +
> +#define test_illegal(size, check) \
> + do { \
> + val_##size = (check); \
> + KUNIT_EXPECT_FALSE_MSG(test, !get_user(val_##size, (size __user *)kmem), \
> + "illegal get_user (" #size ") passed"); \
> + KUNIT_EXPECT_FALSE_MSG(test, val_##size != (size)0, \
> + "zeroing failure for illegal get_user (" #size ")"); \
> + if (val_##size != (size)0) { \
> + kunit_info(test, "0x%llx != 0\n", \
> + (unsigned long long)val_##size); \
> + } \
> + KUNIT_EXPECT_FALSE_MSG(test, !put_user(val_##size, (size __user *)kmem), \
> + "illegal put_user (" #size ") passed"); \
> } while (0)
>
> test_illegal(u8, 0x5a);
> @@ -309,23 +286,18 @@ static int __init test_user_copy_init(void)
>
> vm_munmap(user_addr, PAGE_SIZE * 2);
> kfree(kmem);
> -
> - if (ret == 0) {
> - pr_info("tests passed.\n");
> - return 0;
> - }
> -
> - return -EINVAL;
> }
>
> -module_init(test_user_copy_init);
> -
> -static void __exit test_user_copy_exit(void)
> -{
> - pr_info("unloaded.\n");
> -}
> +static struct kunit_case user_copy_test_cases[] = {
> + KUNIT_CASE(user_copy_test),
> + {}
> +};
>
> -module_exit(test_user_copy_exit);
> +static struct kunit_suite user_copy_test_suite = {
> + .name = "user_copy",
> + .test_cases = user_copy_test_cases,
> +};
>
> +kunit_test_suites(&user_copy_test_suite);
> MODULE_AUTHOR("Kees Cook <[email protected]>");
> MODULE_LICENSE("GPL");
> --
> 2.26.2
>
On Wed, Jul 15, 2020 at 12:11:20AM -0300, Vitor Massaru Iha wrote:
> This adds the conversion of the runtime tests of test_user_copy fuctions,
> from `lib/test_user_copy.c`to KUnit tests.
>
> Signed-off-by: Vitor Massaru Iha <[email protected]>
> [...]
> @@ -16,6 +16,7 @@
> #include <linux/slab.h>
> #include <linux/uaccess.h>
> #include <linux/vmalloc.h>
> +#include <kunit/test.h>
>
> /*
> * Several 32-bit architectures support 64-bit {get,put}_user() calls.
> @@ -31,26 +32,16 @@
> # define TEST_U64
> #endif
>
> -#define test(condition, msg, ...) \
> -({ \
> - int cond = (condition); \
> - if (cond) \
> - pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__); \
> - cond; \
> -})
> -
> static bool is_zeroed(void *from, size_t size)
> {
> return memchr_inv(from, 0x0, size) == NULL;
> }
>
> -static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)
> +static void test_check_nonzero_user(struct kunit *test, char *kmem, char __user *umem, size_t size)
> {
> - int ret = 0;
> size_t start, end, i, zero_start, zero_end;
>
> - if (test(size < 2 * PAGE_SIZE, "buffer too small"))
> - return -EINVAL;
> + KUNIT_EXPECT_FALSE_MSG(test, size < 2 * PAGE_SIZE, "buffer too small");
I think this could be a much smaller diff if you just replaced the
"test" macro:
#define test(condition, msg, ...) \
({ \
int cond = !!(condition); \
KUNIT_EXPECT_FALSE_MSG(kunit_context, cond, msg, ##__VA_ARGS__);\
cond; \
})
--
Kees Cook
On Wed, Jul 15, 2020 at 11:47:11AM +0800, David Gow wrote:
> - The inheriting of the mm stuff still means that
> copy_{from,to}_user() will only work if loaded as a module. This
> really needs to be documented. (Ideally, we'd find a way of having
> this work even for built-in tests, but I don't have any real ideas as
> to how that could be done).
I'd like to better understand this ... are there conditions where
vm_mmap() doesn't work? I thought this would either use current() (e.g.
how LKDTM uses it when getting triggered from debugfs), or use init_mm.
I'd really like to see the mm patch more well described/justified.
--
Kees Cook
On Wed, 2020-07-15 at 11:47 +0800, David Gow wrote:
> On Wed, Jul 15, 2020 at 11:11 AM Vitor Massaru Iha <[email protected]
> > wrote:
> > Currently, KUnit does not allow the use of tests as a module.
> > This prevents the implementation of tests that require userspace.
>
> If this is what I think it is, thanks! I'll hopefully get a chance to
> play with it over the next few days.
>
> Can we clarify what this means: the current description is a little
> misleading, as KUnit tests can already be built and run as modules,
> and "tests that require userspace" is a bit broad.
>
> As I understand it, this patchset does three things:
> - Let kunit_tool install modules to a root filesystem and boot UML
> with that filesystem.
> - Have tests inherit the mm of the process that started them, which
> (if the test is in a module), provides a user-space memory context so
> that copy_{from,to}_user() works.
> - Port the test_user_copy.c tests to KUnit, using this new feature.
>
> A few comments from my quick glance over it:
> - The rootfs support is useful: I'm curious how it'll interact with
> non-UML architectures in [1]. It'd be nice for this to be extensible
> and to not explicitly state UML where possible.
Hm, I didn't think about other architectures. Which ones are you
thinking ?
> - The inheriting of the mm stuff still means that
> copy_{from,to}_user() will only work if loaded as a module. This
> really needs to be documented. (Ideally, we'd find a way of having
> this work even for built-in tests, but I don't have any real ideas as
> to how that could be done).
Sure, I'll write the documentation.
> - It'd be nice to split the test_user_copy.c test port into a
> separate
> commit. In fact, it may make sense to also split the kunit_tool
> changes and the mm changes into separate series, as they're both
> quite
> useful independently.
>
I'll do it.
Thanks for the review.
On Wed, 2020-07-15 at 19:41 -0700, Kees Cook wrote:
> On Wed, Jul 15, 2020 at 11:47:11AM +0800, David Gow wrote:
> > - The inheriting of the mm stuff still means that
> > copy_{from,to}_user() will only work if loaded as a module. This
> > really needs to be documented. (Ideally, we'd find a way of having
> > this work even for built-in tests, but I don't have any real ideas
> > as
> > to how that could be done).
>
> I'd like to better understand this ... are there conditions where
> vm_mmap() doesn't work? I thought this would either use current()
> (e.g.
> how LKDTM uses it when getting triggered from debugfs), or use
> init_mm.
>
> I'd really like to see the mm patch more well described/justified.
>
Sure, I'll describe the patch better.
Thanks for the review.
On Wed, 2020-07-15 at 17:29 -0700, 'Brendan Higgins' via KUnit
Development wrote:
> On Tue, Jul 14, 2020 at 8:11 PM Vitor Massaru Iha <[email protected]>
> wrote:
> > This makes it possible to use KUnit tests as modules.
> > And with that the tests can run in userspace.
> >
> > The filesystem was created using debootstrap:
> > sudo debootstrap buster buster_rootfs
> >
> > And change the owner of the root filesystem files
> > for your user:
> > sudo chown -R $USER:$USER buster_rootfs
>
> Probably want to add this to the documentation for KUnit.
>
> > For the kunit-tool to correctly capture the test results,
> > uml_utilities must be installed on the host to halt uml.
> >
> > Signed-off-by: Vitor Massaru Iha <[email protected]>
>
> Overall this looks really good! I am really excited to see this!
>
> Reviewed-by: Brendan Higgins <[email protected]>
>
> > ---
> > tools/testing/kunit/kunit.py | 37 ++++++++--
> > tools/testing/kunit/kunit_kernel.py | 105
> > ++++++++++++++++++++++++----
> > 2 files changed, 121 insertions(+), 21 deletions(-)
> >
> > diff --git a/tools/testing/kunit/kunit.py
> > b/tools/testing/kunit/kunit.py
> > index 787b6d4ad716..6d8ba0215b90 100755
> > --- a/tools/testing/kunit/kunit.py
> > +++ b/tools/testing/kunit/kunit.py
> > @@ -23,16 +23,16 @@ import kunit_parser
> > KunitResult = namedtuple('KunitResult',
> > ['status','result','elapsed_time'])
> >
> > KunitConfigRequest = namedtuple('KunitConfigRequest',
> > - ['build_dir', 'make_options'])
> > + ['build_dir', 'uml_root_dir',
> > 'make_options'])
> > KunitBuildRequest = namedtuple('KunitBuildRequest',
> > - ['jobs', 'build_dir', 'alltests',
> > + ['jobs', 'build_dir',
> > 'uml_root_dir', 'alltests',
> > 'make_options'])
> > KunitExecRequest = namedtuple('KunitExecRequest',
> > - ['timeout', 'build_dir', 'alltests'])
> > + ['timeout', 'build_dir',
> > 'uml_root_dir', 'alltests'])
> > KunitParseRequest = namedtuple('KunitParseRequest',
> > ['raw_output', 'input_data'])
> > KunitRequest = namedtuple('KunitRequest', ['raw_output','timeout',
> > 'jobs',
> > - 'build_dir', 'alltests',
> > + 'build_dir',
> > 'uml_root_dir', 'alltests',
> > 'make_options'])
> >
> > KernelDirectoryPath = sys.argv[0].split('tools/testing/kunit/')[0]
> > @@ -47,7 +47,6 @@ def create_default_kunitconfig():
> > if not os.path.exists(kunit_kernel.kunitconfig_path):
> > shutil.copyfile('arch/um/configs/kunit_defconfig',
> > kunit_kernel.kunitconfig_path)
> > -
> > def get_kernel_root_path():
> > parts = sys.argv[0] if not __file__ else __file__
> > parts =
> > os.path.realpath(parts).split('tools/testing/kunit')
> > @@ -58,10 +57,22 @@ def get_kernel_root_path():
> > def config_tests(linux: kunit_kernel.LinuxSourceTree,
> > request: KunitConfigRequest) -> KunitResult:
> > kunit_parser.print_with_timestamp('Configuring KUnit Kernel
> > ...')
> > + defconfig = False
> >
> > config_start = time.time()
> > create_default_kunitconfig()
> > - success = linux.build_reconfig(request.build_dir,
> > request.make_options)
> > +
> > + if request.uml_root_dir != None:
> > + if os.path.exists(request.uml_root_dir):
> > + kunit_kernel.uml_root_path =
> > os.path.abspath(request.uml_root_dir)
> > + defconfig = True
> > + else:
> > + config_end = time.time()
> > + return
> > KunitResult(KunitStatus.CONFIG_FAILURE,
> > + 'could not configure
> > kernel',
> > + config_end - config_start)
> > +
> > + success = linux.build_reconfig(request.build_dir,
> > defconfig, request.make_options)
> > config_end = time.time()
> > if not success:
> > return KunitResult(KunitStatus.CONFIG_FAILURE,
> > @@ -79,6 +90,7 @@ def build_tests(linux:
> > kunit_kernel.LinuxSourceTree,
> > success = linux.build_um_kernel(request.alltests,
> > request.jobs,
> > request.build_dir,
> > + request.uml_root_dir,
> > request.make_options)
> > build_end = time.time()
> > if not success:
> > @@ -97,7 +109,7 @@ def exec_tests(linux:
> > kunit_kernel.LinuxSourceTree,
> > test_start = time.time()
> > result = linux.run_kernel(
> > timeout=None if request.alltests else
> > request.timeout,
> > - build_dir=request.build_dir)
> > + build_dir=request.build_dir,
> > uml_root_dir=request.uml_root_dir)
> >
> > test_end = time.time()
> >
> > @@ -130,12 +142,14 @@ def run_tests(linux:
> > kunit_kernel.LinuxSourceTree,
> > run_start = time.time()
> >
> > config_request = KunitConfigRequest(request.build_dir,
> > + request.uml_root_dir,
> > request.make_options)
> > config_result = config_tests(linux, config_request)
> > if config_result.status != KunitStatus.SUCCESS:
> > return config_result
> >
> > build_request = KunitBuildRequest(request.jobs,
> > request.build_dir,
> > + request.uml_root_dir,
> > request.alltests,
> > request.make_options)
> > build_result = build_tests(linux, build_request)
> > @@ -143,6 +157,7 @@ def run_tests(linux:
> > kunit_kernel.LinuxSourceTree,
> > return build_result
> >
> > exec_request = KunitExecRequest(request.timeout,
> > request.build_dir,
> > + request.uml_root_dir,
> > request.alltests)
> > exec_result = exec_tests(linux, exec_request)
> > if exec_result.status != KunitStatus.SUCCESS:
> > @@ -168,6 +183,10 @@ def add_common_opts(parser):
> > help='As in the make command, it
> > specifies the build '
> > 'directory.',
> > type=str, default='.kunit',
> > metavar='build_dir')
> > + parser.add_argument('--uml_root_dir',
> > + help='To load modules, a root
> > filesystem is '
> > + 'required to install and load these
> > modules.',
> > + type=str, default=None,
> > metavar='uml_root_dir')
> > parser.add_argument('--make_options',
> > help='X=Y make option, can be
> > repeated.',
> > action='append')
> > @@ -252,6 +271,7 @@ def main(argv, linux=None):
> > cli_args.timeout,
> > cli_args.jobs,
> > cli_args.build_dir,
> > + cli_args.uml_root_dir,
> > cli_args.alltests,
> > cli_args.make_options)
> > result = run_tests(linux, request)
> > @@ -272,6 +292,7 @@ def main(argv, linux=None):
> > linux = kunit_kernel.LinuxSourceTree()
> >
> > request = KunitConfigRequest(cli_args.build_dir,
> > + cli_args.uml_root_dir,
> > cli_args.make_options)
> > result = config_tests(linux, request)
> > kunit_parser.print_with_timestamp((
> > @@ -295,6 +316,7 @@ def main(argv, linux=None):
> >
> > request = KunitBuildRequest(cli_args.jobs,
> > cli_args.build_dir,
> > + cli_args.uml_root_dir,
> > cli_args.alltests,
> > cli_args.make_options)
> > result = build_tests(linux, request)
> > @@ -319,6 +341,7 @@ def main(argv, linux=None):
> >
> > exec_request = KunitExecRequest(cli_args.timeout,
> > cli_args.build_dir,
> > + cli_args.uml_root_d
> > ir,
> > cli_args.alltests)
> > exec_result = exec_tests(linux, exec_request)
> > parse_request =
> > KunitParseRequest(cli_args.raw_output,
> > diff --git a/tools/testing/kunit/kunit_kernel.py
> > b/tools/testing/kunit/kunit_kernel.py
> > index 63dbda2d029f..d712f4619eaa 100644
> > --- a/tools/testing/kunit/kunit_kernel.py
> > +++ b/tools/testing/kunit/kunit_kernel.py
> > @@ -11,6 +11,7 @@ import logging
> > import subprocess
> > import os
> > import signal
> > +import time
> >
> > from contextlib import ExitStack
> >
> > @@ -19,7 +20,59 @@ import kunit_parser
> >
> > KCONFIG_PATH = '.config'
> > kunitconfig_path = '.kunitconfig'
> > +X86_64_DEFCONFIG_PATH = 'arch/um/configs/x86_64_defconfig'
> > BROKEN_ALLCONFIG_PATH =
> > 'tools/testing/kunit/configs/broken_on_uml.config'
> > +uml_root_path = None
>
> nit: I don't like global variables. Can we just pass this in or store
> it in a data structure. The above are all constants.
>
> > +
> > +make_cmd = {
>
> This looks like a constant. Please capitalize it.
>
> > + 'make': {
> > + 'command': ['make', 'ARCH=um'],
> > + 'msg_error': 'Could not call execute make: ',
> > + },
> > + 'make_modules': {
> > + 'command': ['make', 'modules', 'ARCH=um'],
> > + 'msg_error': 'Could not call execute make modules:
> > ',
> > + },
> > + 'make_modules_install': {
> > + 'command': ['make', 'modules_install', 'ARCH=um'],
> > + 'msg_error': 'Could not call execute make
> > modules_install: ',
> > + }
> > +}
> >
> > +def halt_uml():
> > + try:
> > + subprocess.call(['uml_mconsole', 'kunitid',
> > 'halt'])
> > + except OSError as e:
> > + raise ConfigError('Could not call uml_mconsole ' +
> > e)
> > + except subprocess.CalledProcessError as e:
> > + raise ConfigError(e.output)
> > +
> > +
> > +def enable_uml_modules_on_boot(output_command):
> > + uml_modules_path = None
> > + found_kernel_version = False
> > + modules = []
> > + for i in output_command.decode('utf-8').split():
> > + if found_kernel_version:
> > + kernel_version = i
> > + uml_modules_path =
> > os.path.join(uml_root_path,
> > + 'lib/modules/', kernel_version,
> > 'kernel/lib/')
> > + break
> > + if 'DEPMOD' in i:
> > + found_kernel_version = True
> > +
> > + try:
> > + if os.path.exists(uml_modules_path):
> > + modules = subprocess.check_output(['ls',
> > + uml_modules_path]).deco
> > de('utf-8').split()
> > + except OSError as e:
> > + raise ConfigError('Could not list directory ' + e)
> > + except subprocess.CalledProcessError as e:
> > + raise ConfigError(e.output)
> > +
> > + with open(os.path.join(uml_root_path, 'etc/modules'), 'w')
> > as f:
> > + for i in modules:
> > + f.write(i.replace('.ko', ''))
> >
> > class ConfigError(Exception):
> > """Represents an error trying to configure the Linux
> > kernel."""
> > @@ -70,20 +123,27 @@ class LinuxSourceTreeOperations(object):
> > kunit_parser.print_with_timestamp(
> > 'Starting Kernel with all configs takes a
> > few minutes...')
> >
> > - def make(self, jobs, build_dir, make_options):
> > - command = ['make', 'ARCH=um', '--jobs=' +
> > str(jobs)]
> > + def make(self, cmd, jobs, build_dir, make_options):
> > + command = make_cmd[cmd]['command'] + ['--jobs=' +
> > str(jobs)]
> > +
> > if make_options:
> > command.extend(make_options)
> > if build_dir:
> > command += ['O=' + build_dir]
> > +
> > + if cmd == 'make_modules_install':
> > + command += ['INSTALL_MOD_PATH=' +
> > uml_root_path]
> > +
> > try:
> > - subprocess.check_output(command)
> > + output = subprocess.check_output(command)
> > + if cmd == 'make_modules_install':
> > + enable_uml_modules_on_boot(output)
> > except OSError as e:
> > - raise BuildError('Could not call execute
> > make: ' + e)
> > + raise BuildError(make_cmd[cmd][msg_error] +
> > e)
> > except subprocess.CalledProcessError as e:
> > raise BuildError(e.output)
> >
> > - def linux_bin(self, params, timeout, build_dir, outfile):
> > + def linux_bin(self, params, timeout, build_dir,
> > uml_root_dir, outfile):
> > """Runs the Linux UML binary. Must be named
> > 'linux'."""
> > linux_bin = './linux'
> > if build_dir:
> > @@ -92,7 +152,11 @@ class LinuxSourceTreeOperations(object):
> > process = subprocess.Popen([linux_bin] +
> > params,
> > stdout=output,
> > stderr=subproces
> > s.STDOUT)
> > - process.wait(timeout)
> > + if uml_root_dir:
> > + time.sleep(timeout)
> > + halt_uml()
> > + else:
> > + process.wait(timeout)
> >
> >
> > def get_kconfig_path(build_dir):
> > @@ -132,11 +196,16 @@ class LinuxSourceTree(object):
> > return False
> > return True
> >
> > - def build_config(self, build_dir, make_options):
> > + def build_config(self, build_dir, defconfig, make_options):
> > kconfig_path = get_kconfig_path(build_dir)
> > if build_dir and not os.path.exists(build_dir):
> > os.mkdir(build_dir)
> > self._kconfig.write_to_file(kconfig_path)
> > +
> > + if defconfig:
> > + with open(kconfig_path, 'a') as fw:
> > + with open(X86_64_DEFCONFIG_PATH,
> > 'r') as fr:
> > + fw.write(fr.read())
> > try:
> > self._ops.make_olddefconfig(build_dir,
> > make_options)
> > except ConfigError as e:
> > @@ -144,7 +213,7 @@ class LinuxSourceTree(object):
> > return False
> > return self.validate_config(build_dir)
> >
> > - def build_reconfig(self, build_dir, make_options):
> > + def build_reconfig(self, build_dir, defconfig,
> > make_options):
> > """Creates a new .config if it is not a subset of
> > the .kunitconfig."""
> > kconfig_path = get_kconfig_path(build_dir)
> > if os.path.exists(kconfig_path):
> > @@ -153,28 +222,36 @@ class LinuxSourceTree(object):
> > if not
> > self._kconfig.is_subset_of(existing_kconfig):
> > print('Regenerating .config ...')
> > os.remove(kconfig_path)
> > - return self.build_config(build_dir,
> > make_options)
> > + return self.build_config(build_dir,
> > defconfig, make_options)
> > else:
> > return True
> > else:
> > print('Generating .config ...')
> > - return self.build_config(build_dir,
> > make_options)
> > + return self.build_config(build_dir,
> > defconfig, make_options)
> >
> > - def build_um_kernel(self, alltests, jobs, build_dir,
> > make_options):
> > + def build_um_kernel(self, alltests, jobs, build_dir,
> > uml_root_dir, make_options):
> > if alltests:
> > self._ops.make_allyesconfig()
> > try:
> > self._ops.make_olddefconfig(build_dir,
> > make_options)
> > - self._ops.make(jobs, build_dir,
> > make_options)
> > + self._ops.make('make', jobs, build_dir,
> > make_options)
> > + if uml_root_dir:
> > + self._ops.make('make_modules',
> > jobs, build_dir,
> > + make_options)
> > + self._ops.make('make_modules_instal
> > l', jobs,
> > + build_dir,
> > make_options)
> > except (ConfigError, BuildError) as e:
> > logging.error(e)
> > return False
> > return self.validate_config(build_dir)
> >
> > - def run_kernel(self, args=[], build_dir='', timeout=None):
> > + def run_kernel(self, args=[], build_dir='',
> > uml_root_dir=None, timeout=None):
> > args.extend(['mem=1G'])
> > + if uml_root_dir:
> > + args.extend(['root=/dev/root',
> > 'rootfstype=hostfs',
> > + 'rootflags=' + uml_root_path,
> > 'umid=kunitid'])
> > outfile = 'test.log'
> > - self._ops.linux_bin(args, timeout, build_dir,
> > outfile)
> > + self._ops.linux_bin(args, timeout, build_dir,
> > uml_root_dir, outfile)
> > subprocess.call(['stty', 'sane'])
> > with open(outfile, 'r') as file:
> > for line in file:
> > --
> > 2.26.2
> >
I'll fix all comments.
Thanks for the review.
On Wed, 2020-07-15 at 17:37 -0700, Brendan Higgins wrote:
> On Tue, Jul 14, 2020 at 8:11 PM Vitor Massaru Iha <[email protected]>
> wrote:
>
> Probably want to add more of a description here as what you are doing
> is not entirely trivial to someone not familiar with mm contexts.
>
> > Signed-off-by: Vitor Massaru Iha <[email protected]>
> > ---
> > include/kunit/test.h | 1 +
> > lib/kunit/try-catch.c | 15 ++++++++++++++-
> > 2 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index 59f3144f009a..49c38bdcb93e 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -222,6 +222,7 @@ struct kunit {
> > * protect it with some type of lock.
> > */
> > struct list_head resources; /* Protected by lock. */
> > + struct mm_struct *mm;
>
> Part of me thinks we should put a better name here, part of me thinks
> it is fine because it matches the convention.
>
> Either way, this DEFINITELY deserves a comment explaining what it is,
> why it exists, and how it should/shouldn't be used.
>
> > };
> >
> > void kunit_init_test(struct kunit *test, const char *name, char
> > *log);
> > diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> > index 0dd434e40487..f677c2f2a51a 100644
> > --- a/lib/kunit/try-catch.c
> > +++ b/lib/kunit/try-catch.c
> > @@ -11,7 +11,8 @@
> > #include <linux/completion.h>
> > #include <linux/kernel.h>
> > #include <linux/kthread.h>
> > -
> > +#include <linux/sched/mm.h>
> > +#include <linux/sched/task.h>
> > #include "try-catch-impl.h"
> >
> > void __noreturn kunit_try_catch_throw(struct kunit_try_catch
> > *try_catch)
> > @@ -24,8 +25,17 @@ EXPORT_SYMBOL_GPL(kunit_try_catch_throw);
> > static int kunit_generic_run_threadfn_adapter(void *data)
> > {
> > struct kunit_try_catch *try_catch = data;
> > + struct kunit *test = try_catch->test;
> > +
> > + if (test->mm != NULL)
> > + kthread_use_mm(try_catch->test->mm);
> >
> > try_catch->try(try_catch->context);
> > + if (try_catch->test->mm) {
>
> Here and below: You already have a pointer to test. You should use
> it.
>
> > + if (test->mm != NULL)
> > + kthread_unuse_mm(try_catch->test->mm);
> > + try_catch->test->mm = NULL;
> > + }
> >
> > complete_and_exit(try_catch->try_completion, 0);
> > }
> > @@ -65,6 +75,9 @@ void kunit_try_catch_run(struct kunit_try_catch
> > *try_catch, void *context)
> > try_catch->context = context;
> > try_catch->try_completion = &try_completion;
> > try_catch->try_result = 0;
> > +
> > + test->mm = get_task_mm(current);
> > +
> > task_struct =
> > kthread_run(kunit_generic_run_threadfn_adapter,
> > try_catch,
> > "kunit_try_catch_thread");
> > --
> > 2.26.2
> >
On Wed, 2020-07-15 at 17:37 -0700, Brendan Higgins wrote:
> On Tue, Jul 14, 2020 at 8:11 PM Vitor Massaru Iha <[email protected]>
> wrote:
>
> Probably want to add more of a description here as what you are doing
> is not entirely trivial to someone not familiar with mm contexts.
>
> > Signed-off-by: Vitor Massaru Iha <[email protected]>
> > ---
> > include/kunit/test.h | 1 +
> > lib/kunit/try-catch.c | 15 ++++++++++++++-
> > 2 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index 59f3144f009a..49c38bdcb93e 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -222,6 +222,7 @@ struct kunit {
> > * protect it with some type of lock.
> > */
> > struct list_head resources; /* Protected by lock. */
> > + struct mm_struct *mm;
>
> Part of me thinks we should put a better name here, part of me thinks
> it is fine because it matches the convention.
>
> Either way, this DEFINITELY deserves a comment explaining what it is,
> why it exists, and how it should/shouldn't be used.
>
> > };
> >
> > void kunit_init_test(struct kunit *test, const char *name, char
> > *log);
> > diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> > index 0dd434e40487..f677c2f2a51a 100644
> > --- a/lib/kunit/try-catch.c
> > +++ b/lib/kunit/try-catch.c
> > @@ -11,7 +11,8 @@
> > #include <linux/completion.h>
> > #include <linux/kernel.h>
> > #include <linux/kthread.h>
> > -
> > +#include <linux/sched/mm.h>
> > +#include <linux/sched/task.h>
> > #include "try-catch-impl.h"
> >
> > void __noreturn kunit_try_catch_throw(struct kunit_try_catch
> > *try_catch)
> > @@ -24,8 +25,17 @@ EXPORT_SYMBOL_GPL(kunit_try_catch_throw);
> > static int kunit_generic_run_threadfn_adapter(void *data)
> > {
> > struct kunit_try_catch *try_catch = data;
> > + struct kunit *test = try_catch->test;
> > +
> > + if (test->mm != NULL)
> > + kthread_use_mm(try_catch->test->mm);
> >
> > try_catch->try(try_catch->context);
> > + if (try_catch->test->mm) {
>
> Here and below: You already have a pointer to test. You should use
> it.
Ops, thanks!
>
> > + if (test->mm != NULL)
> > + kthread_unuse_mm(try_catch->test->mm);
> > + try_catch->test->mm = NULL;
> > + }
> >
> > complete_and_exit(try_catch->try_completion, 0);
> > }
> > @@ -65,6 +75,9 @@ void kunit_try_catch_run(struct kunit_try_catch
> > *try_catch, void *context)
> > try_catch->context = context;
> > try_catch->try_completion = &try_completion;
> > try_catch->try_result = 0;
> > +
> > + test->mm = get_task_mm(current);
> > +
> > task_struct =
> > kthread_run(kunit_generic_run_threadfn_adapter,
> > try_catch,
> > "kunit_try_catch_thread");
> > --
> > 2.26.2
> >
On Wed, 2020-07-15 at 17:40 -0700, Brendan Higgins wrote:
> On Tue, Jul 14, 2020 at 8:11 PM Vitor Massaru Iha <[email protected]>
> wrote:
> > This adds the conversion of the runtime tests of test_user_copy
> > fuctions,
> > from `lib/test_user_copy.c`to KUnit tests.
> >
> > Signed-off-by: Vitor Massaru Iha <[email protected]>
> > ---
> > lib/Kconfig.debug | 17 ++
> > lib/Makefile | 2 +-
> > lib/{test_user_copy.c => user_copy_kunit.c} | 196 +++++++++-------
> > ----
> > 3 files changed, 102 insertions(+), 113 deletions(-)
> > rename lib/{test_user_copy.c => user_copy_kunit.c} (55%)
> >
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 9ad9210d70a1..29558674c011 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -2154,6 +2154,23 @@ config SYSCTL_KUNIT_TEST
> >
> > If unsure, say N.
> >
> > +config USER_COPY_KUNIT
> > + tristate "KUnit Test for user/kernel boundary protections"
> > + depends on KUNIT
> > + depends on m
> > + help
> > + This builds the "test_user_copy" module that runs sanity
> > checks
> > + on the copy_to/from_user infrastructure, making sure
> > basic
> > + user/kernel boundary testing is working. If it fails to
> > load,
> > + a regression has been detected in the user/kernel memory
> > boundary
> > + protections.
> > +
> > + 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.
>
> Where do you delete the entry for CONFIG_TEST_USER_COPY? I don't see
> it here.
I didn't. Thanks I'll delete it.
> > config LIST_KUNIT_TEST
> > tristate "KUnit Test for Kernel Linked-list structures" if
> > !KUNIT_ALL_TESTS
> > depends on KUNIT
> > diff --git a/lib/Makefile b/lib/Makefile
> > index b1c42c10073b..8c145f85accc 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -78,7 +78,6 @@ obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.o
> > obj-$(CONFIG_TEST_OVERFLOW) += test_overflow.o
> > obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
> > obj-$(CONFIG_TEST_SORT) += test_sort.o
> > -obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
> > obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
> > obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
> > obj-$(CONFIG_TEST_PRINTF) += test_printf.o
> > @@ -318,3 +317,4 @@ obj-$(CONFIG_OBJAGG) += objagg.o
> > # KUnit tests
> > obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
> > obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
> > +obj-$(CONFIG_USER_COPY_KUNIT) += user_copy_kunit.o
> > diff --git a/lib/test_user_copy.c b/lib/user_copy_kunit.c
> > similarity index 55%
> > rename from lib/test_user_copy.c
> > rename to lib/user_copy_kunit.c
> > index 5ff04d8fe971..c15bb1e997d6 100644
> > --- a/lib/test_user_copy.c
> > +++ b/lib/user_copy_kunit.c
> > @@ -16,6 +16,7 @@
> > #include <linux/slab.h>
> > #include <linux/uaccess.h>
> > #include <linux/vmalloc.h>
> > +#include <kunit/test.h>
> >
> > /*
> > * Several 32-bit architectures support 64-bit {get,put}_user()
> > calls.
> > @@ -31,26 +32,16 @@
> > # define TEST_U64
> > #endif
> >
> > -#define test(condition, msg,
> > ...) \
> > -({
> > \
> > - int cond =
> > (condition); \
> > - if
> > (cond) \
> > - pr_warn("[%d] " msg "\n", __LINE__,
> > ##__VA_ARGS__); \
> > - cond;
> > \
> > -})
> > -
> > static bool is_zeroed(void *from, size_t size)
> > {
> > return memchr_inv(from, 0x0, size) == NULL;
> > }
> >
> > -static int test_check_nonzero_user(char *kmem, char __user *umem,
> > size_t size)
> > +static void test_check_nonzero_user(struct kunit *test, char
> > *kmem, char __user *umem, size_t size)
> > {
> > - int ret = 0;
> > size_t start, end, i, zero_start, zero_end;
> >
> > - if (test(size < 2 * PAGE_SIZE, "buffer too small"))
> > - return -EINVAL;
> > + KUNIT_EXPECT_FALSE_MSG(test, size < 2 * PAGE_SIZE, "buffer
> > too small");
> >
> > /*
> > * We want to cross a page boundary to exercise the code
> > more
> > @@ -84,7 +75,7 @@ static int test_check_nonzero_user(char *kmem,
> > char __user *umem, size_t size)
> > for (i = zero_end; i < size; i += 2)
> > kmem[i] = 0xff;
> >
> > - ret |= test(copy_to_user(umem, kmem, size),
> > + KUNIT_EXPECT_FALSE_MSG(test, copy_to_user(umem, kmem,
> > size),
> > "legitimate copy_to_user failed");
> >
> > for (start = 0; start <= size; start++) {
> > @@ -93,35 +84,31 @@ static int test_check_nonzero_user(char *kmem,
> > char __user *umem, size_t size)
> > int retval = check_zeroed_user(umem +
> > start, len);
> > int expected = is_zeroed(kmem + start,
> > len);
> >
> > - ret |= test(retval != expected,
> > - "check_nonzero_user(=%d) !=
> > memchr_inv(=%d) mismatch (start=%zu, end=%zu)",
> > - retval, expected, start, end);
> > + KUNIT_EXPECT_FALSE_MSG(test, retval !=
> > expected,
> > + "check_nonzero_user(=%d) !=
> > memchr_inv(=%d) mismatch (start=%zu, end=%zu)",
> > + retval, expected, start, end);
> > }
> > }
> > -
> > - return ret;
> > }
> >
> > -static int test_copy_struct_from_user(char *kmem, char __user
> > *umem,
> > +static void test_copy_struct_from_user(struct kunit *test, char
> > *kmem, char __user *umem,
> > size_t size)
> > {
> > - int ret = 0;
> > char *umem_src = NULL, *expected = NULL;
> > size_t ksize, usize;
> >
> > umem_src = kmalloc(size, GFP_KERNEL);
> > - ret = test(umem_src == NULL, "kmalloc failed");
> > - if (ret)
> > - goto out_free;
> > + KUNIT_EXPECT_FALSE_MSG(test, umem_src == NULL, "kmalloc
> > failed");
> >
> > expected = kmalloc(size, GFP_KERNEL);
> > - ret = test(expected == NULL, "kmalloc failed");
> > - if (ret)
> > - goto out_free;
> > +
> > + if (expected == NULL)
> > + kfree(umem_src);
> > + KUNIT_EXPECT_FALSE_MSG(test, expected == NULL, "kmalloc
> > failed");
> >
> > /* Fill umem with a fixed byte pattern. */
> > memset(umem_src, 0x3e, size);
> > - ret |= test(copy_to_user(umem, umem_src, size),
> > + KUNIT_EXPECT_FALSE_MSG(test, copy_to_user(umem, umem_src,
> > size),
> > "legitimate copy_to_user failed");
> >
> > /* Check basic case -- (usize == ksize). */
> > @@ -131,9 +118,9 @@ static int test_copy_struct_from_user(char
> > *kmem, char __user *umem,
> > memcpy(expected, umem_src, ksize);
> >
> > memset(kmem, 0x0, size);
> > - ret |= test(copy_struct_from_user(kmem, ksize, umem,
> > usize),
> > + KUNIT_EXPECT_FALSE_MSG(test, copy_struct_from_user(kmem,
> > ksize, umem, usize),
> > "copy_struct_from_user(usize == ksize)
> > failed");
> > - ret |= test(memcmp(kmem, expected, ksize),
> > + KUNIT_EXPECT_FALSE_MSG(test, memcmp(kmem, expected, ksize),
> > "copy_struct_from_user(usize == ksize) gives
> > unexpected copy");
> >
> > /* Old userspace case -- (usize < ksize). */
> > @@ -144,9 +131,9 @@ static int test_copy_struct_from_user(char
> > *kmem, char __user *umem,
> > memset(expected + usize, 0x0, ksize - usize);
> >
> > memset(kmem, 0x0, size);
> > - ret |= test(copy_struct_from_user(kmem, ksize, umem,
> > usize),
> > + KUNIT_EXPECT_FALSE_MSG(test, copy_struct_from_user(kmem,
> > ksize, umem, usize),
> > "copy_struct_from_user(usize < ksize) failed");
> > - ret |= test(memcmp(kmem, expected, ksize),
> > + KUNIT_EXPECT_FALSE_MSG(test, memcmp(kmem, expected, ksize),
> > "copy_struct_from_user(usize < ksize) gives
> > unexpected copy");
> >
> > /* New userspace (-E2BIG) case -- (usize > ksize). */
> > @@ -154,7 +141,7 @@ static int test_copy_struct_from_user(char
> > *kmem, char __user *umem,
> > usize = size;
> >
> > memset(kmem, 0x0, size);
> > - ret |= test(copy_struct_from_user(kmem, ksize, umem, usize)
> > != -E2BIG,
> > + KUNIT_EXPECT_FALSE_MSG(test, copy_struct_from_user(kmem,
> > ksize, umem, usize) != -E2BIG,
> > "copy_struct_from_user(usize > ksize) didn't
> > give E2BIG");
> >
> > /* New userspace (success) case -- (usize > ksize). */
> > @@ -162,24 +149,18 @@ static int test_copy_struct_from_user(char
> > *kmem, char __user *umem,
> > usize = size;
> >
> > memcpy(expected, umem_src, ksize);
> > - ret |= test(clear_user(umem + ksize, usize - ksize),
> > + KUNIT_EXPECT_FALSE_MSG(test, clear_user(umem + ksize, usize
> > - ksize),
> > "legitimate clear_user failed");
> >
> > memset(kmem, 0x0, size);
> > - ret |= test(copy_struct_from_user(kmem, ksize, umem,
> > usize),
> > + KUNIT_EXPECT_FALSE_MSG(test, copy_struct_from_user(kmem,
> > ksize, umem, usize),
> > "copy_struct_from_user(usize > ksize) failed");
> > - ret |= test(memcmp(kmem, expected, ksize),
> > + KUNIT_EXPECT_FALSE_MSG(test, memcmp(kmem, expected, ksize),
> > "copy_struct_from_user(usize > ksize) gives
> > unexpected copy");
> > -
> > -out_free:
> > - kfree(expected);
> > - kfree(umem_src);
> > - return ret;
> > }
> >
> > -static int __init test_user_copy_init(void)
> > +static void user_copy_test(struct kunit *test)
> > {
> > - int ret = 0;
> > char *kmem;
> > char __user *usermem;
> > char *bad_usermem;
> > @@ -192,16 +173,14 @@ static int __init test_user_copy_init(void)
> > #endif
> >
> > kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
> > - if (!kmem)
> > - return -ENOMEM;
> > + KUNIT_EXPECT_FALSE_MSG(test, kmem == NULL, "kmalloc
> > failed");
> >
> > user_addr = vm_mmap(NULL, 0, PAGE_SIZE * 2,
> > PROT_READ | PROT_WRITE | PROT_EXEC,
> > MAP_ANONYMOUS | MAP_PRIVATE, 0);
> > if (user_addr >= (unsigned long)(TASK_SIZE)) {
> > - pr_warn("Failed to allocate user memory\n");
> > kfree(kmem);
> > - return -ENOMEM;
> > + KUNIT_FAIL(test, "Failed to allocate user memory");
> > }
> >
> > usermem = (char __user *)user_addr;
> > @@ -211,29 +190,29 @@ static int __init test_user_copy_init(void)
> > * Legitimate usage: none of these copies should fail.
> > */
> > memset(kmem, 0x3a, PAGE_SIZE * 2);
> > - ret |= test(copy_to_user(usermem, kmem, PAGE_SIZE),
> > - "legitimate copy_to_user failed");
> > + KUNIT_EXPECT_FALSE_MSG(test, copy_to_user(usermem, kmem,
> > PAGE_SIZE), "legitimate copy_to_user failed");
> > +
> > memset(kmem, 0x0, PAGE_SIZE);
> > - ret |= test(copy_from_user(kmem, usermem, PAGE_SIZE),
> > - "legitimate copy_from_user failed");
> > - ret |= test(memcmp(kmem, kmem + PAGE_SIZE, PAGE_SIZE),
> > - "legitimate usercopy failed to copy data");
> > -
> > -#define test_legit(size,
> > check) \
> > - do
> > { \
> > - val_##size =
> > check; \
> > - ret |= test(put_user(val_##size, (size __user
> > *)usermem), \
> > - "legitimate put_user (" #size ")
> > failed"); \
> > - val_##size =
> > 0; \
> > - ret |= test(get_user(val_##size, (size __user
> > *)usermem), \
> > - "legitimate get_user (" #size ")
> > failed"); \
> > - ret |= test(val_##size !=
> > check, \
> > - "legitimate get_user (" #size ") failed to do
> > copy"); \
> > - if (val_##size != check)
> > { \
> > - pr_info("0x%llx !=
> > 0x%llx\n", \
> > - (unsigned long
> > long)val_##size, \
> > - (unsigned long
> > long)check); \
> > - }
> > \
> > + KUNIT_EXPECT_FALSE_MSG(test, copy_from_user(kmem, usermem,
> > PAGE_SIZE),
> > + "legitimate copy_from_user failed");
> > + KUNIT_EXPECT_FALSE_MSG(test, memcmp(kmem, kmem + PAGE_SIZE,
> > PAGE_SIZE),
> > + "legitimate usercopy failed to copy
> > data");
> > +
> > +#define test_legit(size,
> > check)
> > \
> > + do
> > {
> > \
> > + val_##size =
> > check;
> > \
> > + KUNIT_EXPECT_FALSE_MSG(test, put_user(val_##size,
> > (size __user *)usermem), \
> > + "legitimate put_user (" #size ")
> > failed"); \
> > + val_##size =
> > 0;
> > \
> > + KUNIT_EXPECT_FALSE_MSG(test, get_user(val_##size,
> > (size __user *)usermem), \
> > + "legitimate get_user (" #size ")
> > failed"); \
> > + KUNIT_EXPECT_FALSE_MSG(test, val_##size !=
> > check, \
> > + "legitimate get_user (" #size ")
> > failed to do copy"); \
> > + if (val_##size != check)
> > { \
> > + kunit_info(test, "0x%llx !=
> > 0x%llx\n", \
> > + (unsigned long
> > long)val_##size, \
> > + (unsigned long
> > long)check); \
> > + }
> > \
> > } while (0)
> >
> > test_legit(u8, 0x5a);
> > @@ -245,9 +224,9 @@ static int __init test_user_copy_init(void)
> > #undef test_legit
> >
> > /* Test usage of check_nonzero_user(). */
> > - ret |= test_check_nonzero_user(kmem, usermem, 2 *
> > PAGE_SIZE);
> > + test_check_nonzero_user(test, kmem, usermem, 2 *
> > PAGE_SIZE);
> > /* Test usage of copy_struct_from_user(). */
> > - ret |= test_copy_struct_from_user(kmem, usermem, 2 *
> > PAGE_SIZE);
> > + test_copy_struct_from_user(test, kmem, usermem, 2 *
> > PAGE_SIZE);
> >
> > /*
> > * Invalid usage: none of these copies should succeed.
> > @@ -258,13 +237,13 @@ static int __init test_user_copy_init(void)
> > memset(kmem + PAGE_SIZE, 0, PAGE_SIZE);
> >
> > /* Reject kernel-to-kernel copies through copy_from_user().
> > */
> > - ret |= test(!copy_from_user(kmem, (char __user *)(kmem +
> > PAGE_SIZE),
> > - PAGE_SIZE),
> > - "illegal all-kernel copy_from_user passed");
> > + KUNIT_EXPECT_FALSE_MSG(test, !copy_from_user(kmem, (char
> > __user *)(kmem + PAGE_SIZE),
> > + PAGE_SIZE),
> > + "illegal all-kernel copy_from_user
> > passed");
> >
> > /* Destination half of buffer should have been zeroed. */
> > - ret |= test(memcmp(kmem + PAGE_SIZE, kmem, PAGE_SIZE),
> > - "zeroing failure for illegal all-kernel
> > copy_from_user");
> > + KUNIT_EXPECT_FALSE_MSG(test, memcmp(kmem + PAGE_SIZE, kmem,
> > PAGE_SIZE),
> > + "zeroing failure for illegal all-kernel
> > copy_from_user");
> >
> > #if 0
> > /*
> > @@ -273,30 +252,28 @@ static int __init test_user_copy_init(void)
> > * to be tested in LKDTM instead, since this test module
> > does not
> > * expect to explode.
> > */
> > - ret |= test(!copy_from_user(bad_usermem, (char __user
> > *)kmem,
> > - PAGE_SIZE),
> > - "illegal reversed copy_from_user passed");
> > + KUNIT_EXPECT_FALSE_MSG(test, !copy_from_user(bad_usermem,
> > (char __user *)kmem,
> > + PAGE_SIZE),
> > + "illegal reversed copy_from_user
> > passed");
> > #endif
> > - ret |= test(!copy_to_user((char __user *)kmem, kmem +
> > PAGE_SIZE,
> > - PAGE_SIZE),
> > - "illegal all-kernel copy_to_user passed");
> > - ret |= test(!copy_to_user((char __user *)kmem, bad_usermem,
> > - PAGE_SIZE),
> > - "illegal reversed copy_to_user passed");
> > -
> > -#define test_illegal(size,
> > check) \
> > - do
> > { \
> > - val_##size =
> > (check); \
> > - ret |= test(!get_user(val_##size, (size __user
> > *)kmem), \
> > - "illegal get_user (" #size ")
> > passed"); \
> > - ret |= test(val_##size !=
> > (size)0, \
> > - "zeroing failure for illegal get_user (" #size
> > ")"); \
> > - if (val_##size != (size)0)
> > { \
> > - pr_info("0x%llx !=
> > 0\n", \
> > - (unsigned long
> > long)val_##size); \
> > - }
> > \
> > - ret |= test(!put_user(val_##size, (size __user
> > *)kmem), \
> > - "illegal put_user (" #size ")
> > passed"); \
> > + KUNIT_EXPECT_FALSE_MSG(test, !copy_to_user((char __user
> > *)kmem, kmem + PAGE_SIZE, PAGE_SIZE),
> > + "illegal all-kernel copy_to_user passed");
> > + KUNIT_EXPECT_FALSE_MSG(test, !copy_to_user((char __user
> > *)kmem, bad_usermem, PAGE_SIZE),
> > + "illegal reversed copy_to_user passed");
> > +
> > +#define test_illegal(size,
> > check)
> > \
> > + do
> > {
> > \
> > + val_##size =
> > (check);
> > \
> > + KUNIT_EXPECT_FALSE_MSG(test, !get_user(val_##size,
> > (size __user *)kmem), \
> > + "illegal get_user (" #size ")
> > passed"); \
> > + KUNIT_EXPECT_FALSE_MSG(test, val_##size !=
> > (size)0, \
> > + "zeroing failure for illegal
> > get_user (" #size ")"); \
> > + if (val_##size != (size)0)
> > { \
> > + kunit_info(test, "0x%llx !=
> > 0\n", \
> > + (unsigned long
> > long)val_##size); \
> > + }
> > \
> > + KUNIT_EXPECT_FALSE_MSG(test, !put_user(val_##size,
> > (size __user *)kmem), \
> > + "illegal put_user (" #size ")
> > passed"); \
> > } while (0)
> >
> > test_illegal(u8, 0x5a);
> > @@ -309,23 +286,18 @@ static int __init test_user_copy_init(void)
> >
> > vm_munmap(user_addr, PAGE_SIZE * 2);
> > kfree(kmem);
> > -
> > - if (ret == 0) {
> > - pr_info("tests passed.\n");
> > - return 0;
> > - }
> > -
> > - return -EINVAL;
> > }
> >
> > -module_init(test_user_copy_init);
> > -
> > -static void __exit test_user_copy_exit(void)
> > -{
> > - pr_info("unloaded.\n");
> > -}
> > +static struct kunit_case user_copy_test_cases[] = {
> > + KUNIT_CASE(user_copy_test),
> > + {}
> > +};
> >
> > -module_exit(test_user_copy_exit);
> > +static struct kunit_suite user_copy_test_suite = {
> > + .name = "user_copy",
> > + .test_cases = user_copy_test_cases,
> > +};
> >
> > +kunit_test_suites(&user_copy_test_suite);
> > MODULE_AUTHOR("Kees Cook <[email protected]>");
> > MODULE_LICENSE("GPL");
> > --
> > 2.26.2
> >
On Wed, 2020-07-15 at 19:34 -0700, Kees Cook wrote:
> On Wed, Jul 15, 2020 at 12:11:20AM -0300, Vitor Massaru Iha wrote:
> > This adds the conversion of the runtime tests of test_user_copy
> > fuctions,
> > from `lib/test_user_copy.c`to KUnit tests.
> >
> > Signed-off-by: Vitor Massaru Iha <[email protected]>
> > [...]
> > @@ -16,6 +16,7 @@
> > #include <linux/slab.h>
> > #include <linux/uaccess.h>
> > #include <linux/vmalloc.h>
> > +#include <kunit/test.h>
> >
> > /*
> > * Several 32-bit architectures support 64-bit {get,put}_user()
> > calls.
> > @@ -31,26 +32,16 @@
> > # define TEST_U64
> > #endif
> >
> > -#define test(condition, msg, ...)
> > \
> > -({
> > \
> > - int cond = (condition);
> > \
> > - if (cond) \
> > - pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__); \
> > - cond;
> > \
> > -})
> > -
> > static bool is_zeroed(void *from, size_t size)
> > {
> > return memchr_inv(from, 0x0, size) == NULL;
> > }
> >
> > -static int test_check_nonzero_user(char *kmem, char __user *umem,
> > size_t size)
> > +static void test_check_nonzero_user(struct kunit *test, char
> > *kmem, char __user *umem, size_t size)
> > {
> > - int ret = 0;
> > size_t start, end, i, zero_start, zero_end;
> >
> > - if (test(size < 2 * PAGE_SIZE, "buffer too small"))
> > - return -EINVAL;
> > + KUNIT_EXPECT_FALSE_MSG(test, size < 2 * PAGE_SIZE, "buffer too
> > small");
>
> I think this could be a much smaller diff if you just replaced the
> "test" macro:
>
> #define test(condition, msg, ...)
> \
> ({
> \
> int cond = !!(condition); \
> KUNIT_EXPECT_FALSE_MSG(kunit_context, cond, msg,
> ##__VA_ARGS__);\
> cond;
> \
> })
>
Sure, I'll do it.
Thanks.