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 vitor@massaru.org 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 vitor@massaru.org

Overall this looks really good! I am really excited to see this!

Reviewed-by: Brendan Higgins brendanhiggins@google.com

...

---

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 Tue, Jul 14, 2020 at 8:11 PM Vitor Massaru Iha vitor@massaru.org 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 vitor@massaru.org

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 vitor@massaru.org 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 vitor@massaru.org

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 Tue, Jul 14, 2020 at 8:11 PM Vitor Massaru Iha vitor@massaru.org 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 vitor@massaru.org

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 keescook@chromium.org"); 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 vitor@massaru.org [...] @@ -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; \ })