kmod: stress test driver, few fixes and enhancements

This adds a new stress test driver for kmod: the kernel module loader. The new stress test driver, test_kmod, is only enabled as a module right now. It should be possible to load this as built-in and load tests early (refer to the force_init_test module parameter), however since a lot of test can get a system out of memory fast we leave this disabled for now.

Using a system with 1024 MiB of RAM can *easily* get your kernel OOM fast with this test driver.

The test_kmod driver exposes API knobs for us to fine tune simple request_module() and get_fs_type() calls. Since these API calls only allow each one parameter a test driver for these is rather simple. Other factors that can help out test driver though are the number of calls we issue and knowing current limitations of each. This exposes configuration as much as possible through userspace to be able to build tests directly from userspace.

Since it allows multiple misc devices its will eventually (once we add a knob to let us create new devices at will) also be possible to perform more tests in parallel, provided you have enough memory.

We only enable tests we know work as of right now.

Demo screenshots:

 # tools/testing/selftests/kmod/kmod.sh
kmod_test_0001_driver: OK! - loading kmod test
kmod_test_0001_driver: OK! - Return value: 256 (MODULE_NOT_FOUND), expected MODULE_NOT_FOUND
kmod_test_0001_fs: OK! - loading kmod test
kmod_test_0001_fs: OK! - Return value: -22 (-EINVAL), expected -EINVAL
kmod_test_0002_driver: OK! - loading kmod test
kmod_test_0002_driver: OK! - Return value: 256 (MODULE_NOT_FOUND), expected MODULE_NOT_FOUND
kmod_test_0002_fs: OK! - loading kmod test
kmod_test_0002_fs: OK! - Return value: -22 (-EINVAL), expected -EINVAL
kmod_test_0003: OK! - loading kmod test
kmod_test_0003: OK! - Return value: 0 (SUCCESS), expected SUCCESS
kmod_test_0004: OK! - loading kmod test
kmod_test_0004: OK! - Return value: 0 (SUCCESS), expected SUCCESS
kmod_test_0005: OK! - loading kmod test
kmod_test_0005: OK! - Return value: 0 (SUCCESS), expected SUCCESS
kmod_test_0006: OK! - loading kmod test
kmod_test_0006: OK! - Return value: 0 (SUCCESS), expected SUCCESS
kmod_test_0005: OK! - loading kmod test
kmod_test_0005: OK! - Return value: 0 (SUCCESS), expected SUCCESS
kmod_test_0006: OK! - loading kmod test
kmod_test_0006: OK! - Return value: 0 (SUCCESS), expected SUCCESS
Test completed

You can also request for specific tests:

 # tools/testing/selftests/kmod/kmod.sh -t 0001
kmod_test_0001_driver: OK! - loading kmod test
kmod_test_0001_driver: OK! - Return value: 256 (MODULE_NOT_FOUND), expected MODULE_NOT_FOUND
kmod_test_0001_fs: OK! - loading kmod test
kmod_test_0001_fs: OK! - Return value: -22 (-EINVAL), expected -EINVAL
Test completed

Lastly, the current available number of tests:

 # tools/testing/selftests/kmod/kmod.sh --help
Usage: tools/testing/selftests/kmod/kmod.sh [ -t <4-number-digit> ]
Valid tests: 0001-0009

0001 - Simple test - 1 thread  for empty string
0002 - Simple test - 1 thread  for modules/filesystems that do not exist
0003 - Simple test - 1 thread  for get_fs_type() only

0004 - Simple test - 2 threads for get_fs_type() only
0005 - multithreaded tests with default setup - request_module() only 0006 - multithreaded tests with default setup - get_fs_type() only
0007 - multithreaded tests with default setup test request_module() and get_fs_type() 0008 - multithreaded - push kmod_concurrent over max_modprobes for request_module() 0009 - multithreaded - push kmod_concurrent over max_modprobes for get_fs_type()

The following test cases currently fail, as such they are not currently enabled by default:

 # tools/testing/selftests/kmod/kmod.sh -t 0007
 # tools/testing/selftests/kmod/kmod.sh -t 0008
 # tools/testing/selftests/kmod/kmod.sh -t 0009
 # tools/testing/selftests/kmod/kmod.sh -t 0010
 # tools/testing/selftests/kmod/kmod.sh -t 0011

To be sure to run them as intended please unload both of the modules:

o test_module
o xfs

And ensure they are not loaded on your system prior to testing them. If you use these paritions for your rootfs you can change the default test driver used for get_fs_type() by exporting it into your environment. For example of other test defaults you can override refer to kmod.sh allow_user_defaults().

Behind the scenes this is how we fine tune at a test case prior to hitting a trigger to run it:

cat /sys/devices/virtual/misc/test_kmod0/config
echo -n "2" > /sys/devices/virtual/misc/test_kmod0/config_test_case echo -n "ext4" > /sys/devices/virtual/misc/test_kmod0/config_test_fs echo -n "80" > /sys/devices/virtual/misc/test_kmod0/config_num_threads cat /sys/devices/virtual/misc/test_kmod0/config
echo -n "1" > /sys/devices/virtual/misc/test_kmod0/config_num_threads

Finally to trigger:

echo -n "1" > /sys/devices/virtual/misc/test_kmod0/trigger_config

The kmod.sh script uses the above constructs to build differnt test cases.

A bit of interpretation of the current failures follows, first two premises:

a) When request_module() is used userspace figures out an optimized version of module order for us. Once it finds the modules it needs, as per depmod symbol dep map, it will finit_module() the respective modules which are needed for the original request_module() request.

b) We have an optimization in place whereby if a kernel uses request_module() on a module already loaded we never bother userspace as the module already is loaded. This is all handled by kernel/kmod.c.

A few things to consider to help identify root causes of issues:

0) kmod 19 has a broken heuristic for modules being assumed to be built-in to your kernel and will return 0 even though request_module() failed. Upgrade to a newer version of kmod.

1) A get_fs_type() call for "xfs" will request_module() for "fs-xfs", not for "xfs". The optimization in kernel described in b) fails to catch if we have a lot of consecutive get_fs_type() calls. The reason is the optimization in place does not look for aliases. This means two consecutive get_fs_type() calls will bump kmod_concurrent, whereas request_module() will not.

This one explanation why test case 0009 fails at least once for get_fs_type().

2) If a module fails to load --- for whatever reason (kmod_concurrent limit reached, file not yet present due to rootfs switch, out of memory) we have a period of time during which module request for the same name either with request_module() or get_fs_type() will *also* fail to load even if the file for the module is ready.

This explains why *multiple* NULLs are possible on test 0009.

3) finit_module() consumes quite a bit of memory.

4) Filesystems typically also have more dependent modules than other modules, its important to note though that even though a get_fs_type() call does not incur additional kmod_concurrent bumps, since userspace loads dependencies it finds it needs via finit_module_fd(), it *will* take much more memory to load a module with a lot of dependencies.

Because of 3) and 4) we will easily run into out of memory failures with certain tests. For instance test 0006 fails on qemu with 1024 MiB of RAM. It panics a box after reaping all userspace processes and still not having enough memory to reap.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 lib/Kconfig.debug                     |   25 +
 lib/Makefile                          |    1 +
 lib/test_kmod.c                       | 1248 +++++++++++++++++++++++++++++++++
 tools/testing/selftests/kmod/Makefile |   11 +
 tools/testing/selftests/kmod/config   |    7 +
 tools/testing/selftests/kmod/kmod.sh  |  449 ++++++++++++
 6 files changed, 1741 insertions(+)
 create mode 100644 lib/test_kmod.c
 create mode 100644 tools/testing/selftests/kmod/Makefile
 create mode 100644 tools/testing/selftests/kmod/config
 create mode 100755 tools/testing/selftests/kmod/kmod.sh

lib/Kconfig.debug +0 -0

index 7446097f72bd..6cad548e0682 100644
@@ -1994,6 +1994,31 @@ config BUG_ON_DATA_CORRUPTION
 

lib/Kconfig.debug +17 -0

+       help
+         Test the kernel's module loading mechanism: kmod. kmod implements
+         support to load modules using the Linux kernel's usermode helper.
+         This test provides a series of tests against kmod.
+
+         Although technically you can either build test_kmod as a module or
+         into the kernel we disallow building it into the kernel since
+         it stress tests request_module() and this will very likely cause
+         some issues by taking over precious threads available from other
+         module load requests, ultimately this could be fatal.
+
+         To run tests run:
+
+         tools/testing/selftests/kmod/kmod.sh --help
+
+         If unsure, say N.
+
 source "samples/Kconfig"
 
 source "lib/Kconfig.kgdb"

lib/Makefile +1 -0

index d15e235f72ea..3c5a14821e16 100644
@@ -55,6 +55,7 @@ obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
 obj-$(CONFIG_TEST_PRINTF) += test_printf.o
 obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o
 obj-$(CONFIG_TEST_UUID) += test_uuid.o
+obj-$(CONFIG_TEST_KMOD) += test_kmod.o
 
 ifeq ($(CONFIG_DEBUG_KOBJECT),y)
 CFLAGS_kobject.o += -DDEBUG

lib/test_kmod.c new file +31 -0

new file mode 100644
index 000000000000..63fded83b9b6
@@ -0,0 +1,1248 @@
+/*
+ * kmod stress test driver
+ *
+ * Copyright (C) 2016 Luis R. Rodriguez <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of copyleft-next (version 0.3.1 or later) as published
+ * at http://copyleft-next.org/.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+/*
+ * This driver provides an interface to trigger and test the kernel's
+ * module loader through a series of configurations and a few triggers.
+ * To test this driver use the following script as root:
+ *
+ * tools/testing/selftests/kmod/kmod.sh --help
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/kmod.h>
+#include <linux/printk.h>
+#include <linux/kthread.h>
+#include <linux/sched.h>
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/vmalloc.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+

lib/test_kmod.c new file +431 -0

+
+/*
+ * For device allocation / registration
+ */
+static DEFINE_MUTEX(reg_dev_mutex);
+static LIST_HEAD(reg_test_devs);
+
+/*
+ * num_test_devs actually represents the *next* ID of the next
+ * device we will allow to create.
+ */
+static int num_test_devs;
+
+/**
+ * enum kmod_test_case - linker table test case
+ *
+ * If you add a  test case, please be sure to review if you need to se
+ * @need_mod_put for your tests case.
+ *
+ * @TEST_KMOD_DRIVER: stress tests request_module()
+ * @TEST_KMOD_FS_TYPE: stress tests get_fs_type()
+ */
+enum kmod_test_case {
+       __TEST_KMOD_INVALID = 0,
+
+       TEST_KMOD_DRIVER,
+       TEST_KMOD_FS_TYPE,
+
+       __TEST_KMOD_MAX,
+};
+
+struct test_config {
+       char *test_driver;
+       char *test_fs;
+       unsigned int num_threads;
+       enum kmod_test_case test_case;
+       int test_result;
+};
+
+struct kmod_test_device;
+
+/**
+ * kmod_test_device_info - thread info
+ *
+ * @ret_sync: return value if request_module() is used, sync request for
+ *     @TEST_KMOD_DRIVER
+ * @fs_sync: return value of get_fs_type() for @TEST_KMOD_FS_TYPE
+ * @thread_idx: thread ID
+ * @test_dev: test device test is being performed under
+ * @need_mod_put: Some tests (get_fs_type() is one) requires putting the module
+ *     (module_put(fs_sync->owner)) when done, otherwise you will not be able
+ *     to unload the respective modules and re-test. We use this to keep
+ *     accounting of when we need this and to help out in case we need to
+ *     error out and deal with module_put() on error.
+ */
+struct kmod_test_device_info {
+       int ret_sync;
+       struct file_system_type *fs_sync;
+       struct task_struct *task_sync;
+       unsigned int thread_idx;
+       struct kmod_test_device *test_dev;
+       bool need_mod_put;
+};
+
+/**
+ * kmod_test_device - test device to help test kmod
+ *
+ * @dev_idx: unique ID for test device
+ * @config: configuration for the test
+ * @misc_dev: we use a misc device under the hood
+ * @dev: pointer to misc_dev's own struct device
+ * @config_mutex: protects configuration of test
+ * @trigger_mutex: the test trigger can only be fired once at a time
+ * @thread_lock: protects @done count, and the @info per each thread
+ * @done: number of threads which have completed or failed
+ * @test_is_oom: when we run out of memory, use this to halt moving forward
+ * @kthreads_done: completion used to signal when all work is done
+ * @list: needed to be part of the reg_test_devs
+ * @info: array of info for each thread
+ */
+struct kmod_test_device {
+       int dev_idx;
+       struct test_config config;
+       struct miscdevice misc_dev;
+       struct device *dev;
+       struct mutex config_mutex;
+       struct mutex trigger_mutex;
+       struct mutex thread_mutex;
+
+       unsigned int done;
+
+       bool test_is_oom;
+       struct completion kthreads_done;
+       struct list_head list;
+
+       struct kmod_test_device_info *info;
+};
+
+static const char *test_case_str(enum kmod_test_case test_case)
+{
+       switch (test_case) {
+       case TEST_KMOD_DRIVER:
+               return "TEST_KMOD_DRIVER";
+       case TEST_KMOD_FS_TYPE:
+               return "TEST_KMOD_FS_TYPE";
+       default:
+               return "invalid";
+       }
+}
+
+static struct miscdevice *dev_to_misc_dev(struct device *dev)
+{
+       return dev_get_drvdata(dev);
+}
+
+static struct kmod_test_device *misc_dev_to_test_dev(struct miscdevice *misc_dev)
+{
+       return container_of(misc_dev, struct kmod_test_device, misc_dev);
+}
+
+static struct kmod_test_device *dev_to_test_dev(struct device *dev)
+{
+       struct miscdevice *misc_dev;
+
+       misc_dev = dev_to_misc_dev(dev);
+
+       return misc_dev_to_test_dev(misc_dev);
+}
+
+/* Must run with thread_mutex held */
+static void kmod_test_done_check(struct kmod_test_device *test_dev,
+                                unsigned int idx)
+{
+       struct test_config *config = &test_dev->config;
+
+       test_dev->done++;
+       dev_dbg(test_dev->dev, "Done thread count: %u\n", test_dev->done);
+
+       if (test_dev->done == config->num_threads) {
+               dev_info(test_dev->dev, "Done: %u threads have all run now\n",
+                        test_dev->done);
+               dev_info(test_dev->dev, "Last thread to run: %u\n", idx);
+               complete(&test_dev->kthreads_done);
+       }
+}
+
+static void test_kmod_put_module(struct kmod_test_device_info *info)
+{
+       struct kmod_test_device *test_dev = info->test_dev;
+       struct test_config *config = &test_dev->config;
+
+       if (!info->need_mod_put)
+               return;
+
+       switch (config->test_case) {
+       case TEST_KMOD_DRIVER:
+               break;
+       case TEST_KMOD_FS_TYPE:
+               if (info && info->fs_sync && info->fs_sync->owner)
+                       module_put(info->fs_sync->owner);
+               break;
+       default:
+               BUG();
+       }
+
+       info->need_mod_put = true;
+}
+
+static int run_request(void *data)
+{
+       struct kmod_test_device_info *info = data;
+       struct kmod_test_device *test_dev = info->test_dev;
+       struct test_config *config = &test_dev->config;
+
+       switch (config->test_case) {
+       case TEST_KMOD_DRIVER:
+               info->ret_sync = request_module("%s", config->test_driver);
+               break;
+       case TEST_KMOD_FS_TYPE:
+               info->fs_sync = get_fs_type(config->test_fs);
+               info->need_mod_put = true;
+               break;
+       default:
+               /* __trigger_config_run() already checked for test sanity */
+               BUG();
+               return -EINVAL;
+       }
+
+       dev_dbg(test_dev->dev, "Ran thread %u\n", info->thread_idx);
+
+       test_kmod_put_module(info);
+
+       mutex_lock(&test_dev->thread_mutex);
+       info->task_sync = NULL;
+       kmod_test_done_check(test_dev, info->thread_idx);
+       mutex_unlock(&test_dev->thread_mutex);
+
+       return 0;
+}
+
+static int tally_work_test(struct kmod_test_device_info *info)
+{
+       struct kmod_test_device *test_dev = info->test_dev;
+       struct test_config *config = &test_dev->config;
+       int err_ret = 0;
+
+       switch (config->test_case) {
+       case TEST_KMOD_DRIVER:
+               /*
+                * Only capture errors, if one is found that's
+                * enough, for now.
+                */
+               if (info->ret_sync != 0)
+                       err_ret = info->ret_sync;
+               dev_info(test_dev->dev,
+                        "Sync thread %d return status: %d\n",
+                        info->thread_idx, info->ret_sync);
+               break;
+       case TEST_KMOD_FS_TYPE:
+               /* For now we make this simple */
+               if (!info->fs_sync)
+                       err_ret = -EINVAL;
+               dev_info(test_dev->dev, "Sync thread %u fs: %s\n",
+                        info->thread_idx, info->fs_sync ? config->test_fs :
+                        "NULL");
+               break;
+       default:
+               BUG();
+       }
+
+       return err_ret;
+}
+
+/*
+ * XXX: add result option to display if all errors did not match.
+ * For now we just keep any error code if one was found.
+ *
+ * If this ran it means *all* tasks were created fine and we
+ * are now just collecting results.
+ *
+ * Only propagate errors, do not override with a subsequent sucess case.
+ */
+static void tally_up_work(struct kmod_test_device *test_dev)
+{
+       struct test_config *config = &test_dev->config;
+       struct kmod_test_device_info *info;
+       unsigned int idx;
+       int err_ret = 0;
+       int ret = 0;
+
+       mutex_lock(&test_dev->thread_mutex);
+
+       dev_info(test_dev->dev, "Results:\n");
+
+       for (idx=0; idx < config->num_threads; idx++) {
+               info = &test_dev->info[idx];
+               ret = tally_work_test(info);
+               if (ret)
+                       err_ret = ret;
+       }
+
+       /*
+        * Note: request_module() returns 256 for a module not found even
+        * though modprobe itself returns 1.
+        */
+       config->test_result = err_ret;
+
+       mutex_unlock(&test_dev->thread_mutex);
+}
+
+static int try_one_request(struct kmod_test_device *test_dev, unsigned int idx)
+{
+       struct kmod_test_device_info *info = &test_dev->info[idx];
+       int fail_ret = -ENOMEM;
+
+       mutex_lock(&test_dev->thread_mutex);
+
+       info->thread_idx = idx;
+       info->test_dev = test_dev;
+       info->task_sync = kthread_run(run_request, info, "%s-%u",
+                                     KBUILD_MODNAME, idx);
+
+       if (!info->task_sync || IS_ERR(info->task_sync)) {
+               test_dev->test_is_oom = true;
+               dev_err(test_dev->dev, "Setting up thread %u failed\n", idx);
+               info->task_sync = NULL;
+               goto err_out;
+       } else
+               dev_dbg(test_dev->dev, "Kicked off thread %u\n", idx);
+
+       mutex_unlock(&test_dev->thread_mutex);
+
+       return 0;
+
+err_out:
+       info->ret_sync = fail_ret;
+       mutex_unlock(&test_dev->thread_mutex);
+
+       return fail_ret;
+}
+
+static void test_dev_kmod_stop_tests(struct kmod_test_device *test_dev)
+{
+       struct test_config *config = &test_dev->config;
+       struct kmod_test_device_info *info;
+       unsigned int i;
+
+       dev_info(test_dev->dev, "Ending request_module() tests\n");
+
+       mutex_lock(&test_dev->thread_mutex);
+
+       for (i=0; i < config->num_threads; i++) {
+               info = &test_dev->info[i];
+               if (info->task_sync && !IS_ERR(info->task_sync)) {
+                       dev_info(test_dev->dev,
+                                "Stopping still-running thread %i\n", i);
+                       kthread_stop(info->task_sync);
+               }
+
+               /*
+                * info->task_sync is well protected, it can only be
+                * NULL or a pointer to a struct. If its NULL we either
+                * never ran, or we did and we completed the work. Completed
+                * tasks *always* put the module for us. This is a sanity
+                * check -- just in case.
+                */
+               if (info->task_sync && info->need_mod_put)
+                       test_kmod_put_module(info);
+       }
+
+       mutex_unlock(&test_dev->thread_mutex);
+}
+
+/*
+ * Only wait *iff* we did not run into any errors during all of our thread
+ * set up. If run into any issues we stop threads and just bail out with
+ * an error to the trigger. This also means we don't need any tally work
+ * for any threads which fail.
+ */
+static int try_requests(struct kmod_test_device *test_dev)
+{
+       struct test_config *config = &test_dev->config;
+       unsigned int idx;
+       int ret;
+       bool any_error = false;
+
+       for (idx=0; idx < config->num_threads; idx++) {
+               if (test_dev->test_is_oom) {
+                       any_error = true;
+                       break;
+               }
+
+               ret = try_one_request(test_dev, idx);
+               if (ret) {
+                       any_error = true;
+                       break;
+               }
+       }
+
+       if (!any_error) {
+               test_dev->test_is_oom = false;
+               dev_info(test_dev->dev,
+                        "No errors were found while initializing threads\n");
+               wait_for_completion(&test_dev->kthreads_done);
+               tally_up_work(test_dev);
+       } else {
+               test_dev->test_is_oom = true;
+               dev_info(test_dev->dev,
+                        "At least one thread failed to start, stop all work\n");
+               test_dev_kmod_stop_tests(test_dev);
+               return -ENOMEM;
+       }
+
+       return 0;
+}
+
+static int run_test_driver(struct kmod_test_device *test_dev)
+{
+       struct test_config *config = &test_dev->config;
+
+       dev_info(test_dev->dev, "Test case: %s (%u)\n",
+                test_case_str(config->test_case),
+                config->test_case);
+       dev_info(test_dev->dev, "Test driver to load: %s\n",
+                config->test_driver);
+       dev_info(test_dev->dev, "Number of threads to run: %u\n",
+                config->num_threads);
+       dev_info(test_dev->dev, "Thread IDs will range from 0 - %u\n",
+                config->num_threads - 1);
+
+       return try_requests(test_dev);
+}
+
+static int run_test_fs_type(struct kmod_test_device *test_dev)
+{
+       struct test_config *config = &test_dev->config;
+
+       dev_info(test_dev->dev, "Test case: %s (%u)\n",
+                test_case_str(config->test_case),
+                config->test_case);
+       dev_info(test_dev->dev, "Test filesystem to load: %s\n",
+                config->test_fs);
+       dev_info(test_dev->dev, "Number of threads to run: %u\n",
+                config->num_threads);
+       dev_info(test_dev->dev, "Thread IDs will range from 0 - %u\n",
+                config->num_threads - 1);
+
+       return try_requests(test_dev);
+}
+
+static ssize_t config_show(struct device *dev,
+                          struct device_attribute *attr,
+                          char *buf)
+{
+       struct kmod_test_device *test_dev = dev_to_test_dev(dev);
+       struct test_config *config = &test_dev->config;
+       int len = 0;
+
+       mutex_lock(&test_dev->config_mutex);
+
+       len += sprintf(buf, "Custom trigger configuration for: %s\n",
+                      dev_name(dev));
+
+       len += sprintf(buf+len, "Number of threads:\t%u\n",
+                      config->num_threads);
+
+       len += sprintf(buf+len, "Test_case:\t%s (%u)\n",
+                      test_case_str(config->test_case),
+                      config->test_case);
+
+       if (config->test_driver)

lib/test_kmod.c new file +175 -0

+
+
+       mutex_unlock(&test_dev->config_mutex);
+
+       return len;
+}
+static DEVICE_ATTR_RO(config);
+
+/*
+ * This ensures we don't allow kicking threads through if our configuration
+ * is faulty.
+ */
+static int __trigger_config_run(struct kmod_test_device *test_dev)
+{
+       struct test_config *config = &test_dev->config;
+
+       test_dev->done = 0;
+
+       switch (config->test_case) {
+       case TEST_KMOD_DRIVER:
+               return run_test_driver(test_dev);
+       case TEST_KMOD_FS_TYPE:
+               return run_test_fs_type(test_dev);
+       default:
+               dev_warn(test_dev->dev,
+                        "Invalid test case requested: %u\n",
+                        config->test_case);
+               return -EINVAL;
+       }
+}
+
+static int trigger_config_run(struct kmod_test_device *test_dev)
+{
+       struct test_config *config = &test_dev->config;
+       int ret;
+
+       mutex_lock(&test_dev->trigger_mutex);
+       mutex_lock(&test_dev->config_mutex);
+
+       ret = __trigger_config_run(test_dev);
+       if (ret < 0)
+               goto out;
+       dev_info(test_dev->dev, "General test result: %d\n",
+                config->test_result);
+
+       /*
+        * We must return 0 after a trigger even unless something went
+        * wrong with the setup of the test. If the test setup went fine
+        * then userspace must just check the result of config->test_result.
+        * One issue with relying on the return from a call in the kernel
+        * is if the kernel returns a possitive value using this trigger
+        * will not return the value to userspace, it would be lost.
+        *
+        * By not relying on capturing the return value of tests we are using
+        * through the trigger it also us to run tests with set -e and only
+        * fail when something went wrong with the driver upon trigger
+        * requests.
+        */
+       ret = 0;
+
+out:
+       mutex_unlock(&test_dev->config_mutex);
+       mutex_unlock(&test_dev->trigger_mutex);
+
+       return ret;
+}
+
+static ssize_t
+trigger_config_store(struct device *dev,
+                    struct device_attribute *attr,
+                    const char *buf, size_t count)
+{
+       struct kmod_test_device *test_dev = dev_to_test_dev(dev);
+       int ret;
+
+       if (test_dev->test_is_oom)
+               return -ENOMEM;
+
+       /* For all intents and purposes we don't care what userspace
+        * sent this trigger, we care only that we were triggered.
+        * We treat the return value only for caputuring issues with
+        * the test setup. At this point all the test variables should
+        * have been allocated so typically this should never fail.
+        */
+       ret = trigger_config_run(test_dev);
+       if (unlikely(ret < 0))
+               goto out;
+
+       /*
+        * Note: any return > 0 will be treated as success
+        * and the error value will not be available to userspace.
+        * Do not rely on trying to send to userspace a test value
+        * return value as possitive return errors will be lost.
+        */
+       if (WARN_ON(ret > 0))
+               return -EINVAL;
+
+       ret = count;
+out:
+       return ret;
+}
+static DEVICE_ATTR_WO(trigger_config);
+
+/*
+ * XXX: move to kstrncpy() once merged.
+ *
+ * Users should use kfree_const() when freeing these.
+ */
+static int __kstrncpy(char **dst, const char *name, size_t count, gfp_t gfp)
+{
+       *dst = kstrndup(name, count, gfp);
+       if (!*dst)
+               return -ENOSPC;
+       return count;
+}
+
+static int config_copy_test_driver_name(struct test_config *config,
+                                   const char *name,
+                                   size_t count)
+{
+       return __kstrncpy(&config->test_driver, name, count, GFP_KERNEL);
+}
+
+
+static int config_copy_test_fs(struct test_config *config, const char *name,
+                              size_t count)
+{
+       return __kstrncpy(&config->test_fs, name, count, GFP_KERNEL);
+}
+
+static void __kmod_config_free(struct test_config *config)
+{
+       if (!config)
+               return;
+
+       kfree_const(config->test_driver);
+       config->test_driver = NULL;
+
+       kfree_const(config->test_fs);
+       config->test_driver = NULL;
+}
+
+static void kmod_config_free(struct kmod_test_device *test_dev)
+{
+       struct test_config *config;
+
+       if (!test_dev)
+               return;
+
+       config = &test_dev->config;
+
+       mutex_lock(&test_dev->config_mutex);
+       __kmod_config_free(config);
+       mutex_unlock(&test_dev->config_mutex);
+}
+
+static ssize_t config_test_driver_store(struct device *dev,
+                                       struct device_attribute *attr,
+                                       const char *buf, size_t count)
+{
+       struct kmod_test_device *test_dev = dev_to_test_dev(dev);
+       struct test_config *config = &test_dev->config;
+       int copied;
+
+       mutex_lock(&test_dev->config_mutex);
+
+       kfree_const(config->test_driver);
+       config->test_driver = NULL;
+
+       copied = config_copy_test_driver_name(config, buf, count);
+       mutex_unlock(&test_dev->config_mutex);
+
+       return copied;
+}
+

lib/test_kmod.c new file +27 -0

+       mutex_unlock(&test_dev->config_mutex);
+
+       return strlen(buf) + 1;
+}
+static DEVICE_ATTR(config_test_driver, 0644, config_test_driver_show,
+                  config_test_driver_store);
+
+static ssize_t config_test_fs_store(struct device *dev,
+                                   struct device_attribute *attr,
+                                   const char *buf, size_t count)
+{
+       struct kmod_test_device *test_dev = dev_to_test_dev(dev);
+       struct test_config *config = &test_dev->config;
+       int copied;
+
+       mutex_lock(&test_dev->config_mutex);
+
+       kfree_const(config->test_fs);
+       config->test_fs = NULL;
+
+       copied = config_copy_test_fs(config, buf, count);
+       mutex_unlock(&test_dev->config_mutex);
+
+       return copied;
+}
+
+static ssize_t config_test_fs_show(struct device *dev,

lib/test_kmod.c new file +14 -0

+
+       return strlen(buf) + 1;
+}
+static DEVICE_ATTR(config_test_fs, 0644, config_test_fs_show,
+                  config_test_fs_store);
+
+static int trigger_config_run_driver(struct kmod_test_device *test_dev,
+                                    const char *test_driver)
+{
+       int copied;
+       struct test_config *config = &test_dev->config;
+
+       mutex_lock(&test_dev->config_mutex);
+

lib/test_kmod.c new file +23 -0

+               test_dev->test_is_oom = true;
+               return -EINVAL;
+       }
+
+       test_dev->test_is_oom = false;
+
+       return trigger_config_run(test_dev);
+}
+
+static int trigger_config_run_fs(struct kmod_test_device *test_dev,
+                                const char *fs_type)
+{
+       int copied;
+       struct test_config *config = &test_dev->config;
+
+       mutex_lock(&test_dev->config_mutex);
+       config->test_case = TEST_KMOD_FS_TYPE;
+
+       kfree_const(config->test_fs);
+       config->test_driver = NULL;
+
+       copied = config_copy_test_fs(config, fs_type, strlen(fs_type));
+       mutex_unlock(&test_dev->config_mutex);

lib/test_kmod.c new file +135 -0

+
+static int kmod_config_sync_info(struct kmod_test_device *test_dev)
+{
+       struct test_config *config = &test_dev->config;
+
+       free_test_dev_info(test_dev);
+       test_dev->info = vzalloc(config->num_threads *
+                                sizeof(struct kmod_test_device_info));
+       if (!test_dev->info) {
+               dev_err(test_dev->dev, "Cannot alloc test_dev info\n");
+               return -ENOMEM;
+       }
+
+       return 0;
+}
+
+/*
+ * Old kernels may not have this, if you want to port this code to
+ * test it on older kernels.
+ */
+#ifdef get_kmod_umh_limit
+static unsigned int kmod_init_test_thread_limit(void)
+{
+       return get_kmod_umh_limit();
+}
+#else
+static unsigned int kmod_init_test_thread_limit(void)
+{
+       return TEST_START_NUM_THREADS;
+}
+#endif
+
+static int __kmod_config_init(struct kmod_test_device *test_dev)
+{
+       struct test_config *config = &test_dev->config;
+       int ret = -ENOMEM, copied;
+
+       __kmod_config_free(config);
+
+       copied = config_copy_test_driver_name(config, TEST_START_DRIVER,
+                                             strlen(TEST_START_DRIVER));
+       if (copied != strlen(TEST_START_DRIVER))
+               goto err_out;
+
+       copied = config_copy_test_fs(config, TEST_START_TEST_FS,
+                                    strlen(TEST_START_TEST_FS));
+       if (copied != strlen(TEST_START_TEST_FS))
+               goto err_out;
+
+       config->num_threads = kmod_init_test_thread_limit();
+       config->test_result = 0;
+       config->test_case = TEST_START_TEST_CASE;
+
+       ret = kmod_config_sync_info(test_dev);
+       if (ret)
+               goto err_out;
+
+       test_dev->test_is_oom = false;
+
+       return 0;
+
+err_out:
+       test_dev->test_is_oom = true;
+       WARN_ON(test_dev->test_is_oom);
+
+       __kmod_config_free(config);
+
+       return ret;
+}
+
+static ssize_t reset_store(struct device *dev,
+                          struct device_attribute *attr,
+                          const char *buf, size_t count)
+{
+       struct kmod_test_device *test_dev = dev_to_test_dev(dev);
+       int ret;
+
+       mutex_lock(&test_dev->trigger_mutex);
+       mutex_lock(&test_dev->config_mutex);
+
+       ret = __kmod_config_init(test_dev);
+       if (ret < 0) {
+               ret = -ENOMEM;
+               dev_err(dev, "could not alloc settings for config trigger: %d\n",
+                      ret);
+               goto out;
+       }
+
+       dev_info(dev, "reset\n");
+       ret = count;
+
+out:
+       mutex_unlock(&test_dev->config_mutex);
+       mutex_unlock(&test_dev->trigger_mutex);
+
+       return ret;
+}
+static DEVICE_ATTR_WO(reset);
+
+static int test_dev_config_update_uint_sync(struct kmod_test_device *test_dev,
+                                           const char *buf, size_t size,
+                                           unsigned int *config,
+                                           int (*test_sync)(struct kmod_test_device *test_dev))
+{
+       int ret;
+       char *end;
+       long new = simple_strtol(buf, &end, 0);
+       unsigned int old_val;
+       if (end == buf || new > UINT_MAX)
+               return -EINVAL;
+
+       mutex_lock(&test_dev->config_mutex);
+
+       old_val = *config;
+       *(unsigned int *)config = new;
+
+       ret = test_sync(test_dev);
+       if (ret) {
+               *(unsigned int *)config = old_val;
+
+               ret = test_sync(test_dev);
+               WARN_ON(ret);
+
+               mutex_unlock(&test_dev->config_mutex);
+               return -EINVAL;
+       }
+
+       mutex_unlock(&test_dev->config_mutex);
+       /* Always return full write size even if we didn't consume all */
+       return size;
+}
+
+static int test_dev_config_update_uint_range(struct kmod_test_device *test_dev,
+                                            const char *buf, size_t size,
+                                            unsigned int *config,

lib/test_kmod.c new file +6 -0

+       mutex_unlock(&test_dev->config_mutex);
+
+       /* Always return full write size even if we didn't consume all */
+       return size;
+}
+

lib/test_kmod.c new file +135 -0

+       mutex_unlock(&test_dev->config_mutex);
+       /* Always return full write size even if we didn't consume all */
+       return size;
+}
+
+static ssize_t test_dev_config_show_int(struct kmod_test_device *test_dev,
+                                       char *buf,
+                                       int config)
+{
+       int val;
+
+       mutex_lock(&test_dev->config_mutex);
+       val = config;
+       mutex_unlock(&test_dev->config_mutex);
+
+       return snprintf(buf, PAGE_SIZE, "%d\n", val);
+}
+
+static ssize_t test_dev_config_show_uint(struct kmod_test_device *test_dev,
+                                        char *buf,
+                                        unsigned int config)
+{
+       unsigned int val;
+
+       mutex_lock(&test_dev->config_mutex);
+       val = config;
+       mutex_unlock(&test_dev->config_mutex);
+
+       return snprintf(buf, PAGE_SIZE, "%u\n", val);
+}
+
+static ssize_t test_result_store(struct device *dev,
+                                struct device_attribute *attr,
+                                const char *buf, size_t count)
+{
+       struct kmod_test_device *test_dev = dev_to_test_dev(dev);
+       struct test_config *config = &test_dev->config;
+
+       return test_dev_config_update_int(test_dev, buf, count,
+                                         &config->test_result);
+}
+
+static ssize_t config_num_threads_store(struct device *dev,
+                                       struct device_attribute *attr,
+                                       const char *buf, size_t count)
+{
+       struct kmod_test_device *test_dev = dev_to_test_dev(dev);
+       struct test_config *config = &test_dev->config;
+
+       return test_dev_config_update_uint_sync(test_dev, buf, count,
+                                               &config->num_threads,
+                                               kmod_config_sync_info);
+}
+
+static ssize_t config_num_threads_show(struct device *dev,
+                                      struct device_attribute *attr,
+                                      char *buf)
+{
+       struct kmod_test_device *test_dev = dev_to_test_dev(dev);
+       struct test_config *config = &test_dev->config;
+
+       return test_dev_config_show_int(test_dev, buf, config->num_threads);
+}
+static DEVICE_ATTR(config_num_threads, 0644, config_num_threads_show,
+                  config_num_threads_store);
+
+static ssize_t config_test_case_store(struct device *dev,
+                                     struct device_attribute *attr,
+                                     const char *buf, size_t count)
+{
+       struct kmod_test_device *test_dev = dev_to_test_dev(dev);
+       struct test_config *config = &test_dev->config;
+
+       return test_dev_config_update_uint_range(test_dev, buf, count,
+                                                &config->test_case,
+                                                __TEST_KMOD_INVALID + 1,
+                                                __TEST_KMOD_MAX - 1);
+}
+
+static ssize_t config_test_case_show(struct device *dev,
+                                    struct device_attribute *attr,
+                                    char *buf)
+{
+       struct kmod_test_device *test_dev = dev_to_test_dev(dev);
+       struct test_config *config = &test_dev->config;
+
+       return test_dev_config_show_uint(test_dev, buf, config->test_case);
+}
+static DEVICE_ATTR(config_test_case, 0644, config_test_case_show,
+                  config_test_case_store);
+
+static ssize_t test_result_show(struct device *dev,
+                               struct device_attribute *attr,
+                               char *buf)
+{
+       struct kmod_test_device *test_dev = dev_to_test_dev(dev);
+       struct test_config *config = &test_dev->config;
+
+       return test_dev_config_show_int(test_dev, buf, config->test_result);
+}
+static DEVICE_ATTR(test_result, 0644, test_result_show, test_result_store);
+
+#define TEST_KMOD_DEV_ATTR(name)               &dev_attr_##name.attr
+
+static struct attribute *test_dev_attrs[] = {
+       TEST_KMOD_DEV_ATTR(trigger_config),
+       TEST_KMOD_DEV_ATTR(config),
+       TEST_KMOD_DEV_ATTR(reset),
+
+       TEST_KMOD_DEV_ATTR(config_test_driver),
+       TEST_KMOD_DEV_ATTR(config_test_fs),
+       TEST_KMOD_DEV_ATTR(config_num_threads),
+       TEST_KMOD_DEV_ATTR(config_test_case),
+       TEST_KMOD_DEV_ATTR(test_result),
+
+       NULL,
+};
+
+ATTRIBUTE_GROUPS(test_dev);
+
+static int kmod_config_init(struct kmod_test_device *test_dev)
+{
+       int ret;
+
+       mutex_lock(&test_dev->config_mutex);
+       ret = __kmod_config_init(test_dev);
+       mutex_unlock(&test_dev->config_mutex);
+
+       return ret;
+}
+
+/*
+ * XXX: this could perhaps be made generic already too, but a hunt
+ * for actual users would be needed first. It could be generic
+ * if other test drivers end up using a similar mechanism.

lib/test_kmod.c new file +161 -0

+
+static struct kmod_test_device *alloc_test_dev_kmod(int idx)
+{
+       int ret;
+       struct kmod_test_device *test_dev;
+       struct miscdevice *misc_dev;
+
+       test_dev = vzalloc(sizeof(struct kmod_test_device));
+       if (!test_dev) {
+               pr_err("Cannot alloc test_dev\n");
+               goto err_out;
+       }
+
+       mutex_init(&test_dev->config_mutex);
+       mutex_init(&test_dev->trigger_mutex);
+       mutex_init(&test_dev->thread_mutex);
+
+       init_completion(&test_dev->kthreads_done);
+
+       ret = kmod_config_init(test_dev);
+       if (ret < 0) {
+               pr_err("Cannot alloc kmod_config_init()\n");
+               goto err_out_free;
+       }
+
+       test_dev->dev_idx = idx;
+       misc_dev = &test_dev->misc_dev;
+
+       misc_dev->minor = MISC_DYNAMIC_MINOR;
+       misc_dev->name = test_dev_get_name("test_kmod", test_dev->dev_idx,
+                                          GFP_KERNEL);
+       if (!misc_dev->name) {
+               pr_err("Cannot alloc misc_dev->name\n");
+               goto err_out_free_config;
+       }
+       misc_dev->groups = test_dev_groups;
+
+       return test_dev;
+
+err_out_free_config:
+       free_test_dev_info(test_dev);
+       kmod_config_free(test_dev);
+err_out_free:
+       vfree(test_dev);
+       test_dev = NULL;
+err_out:
+       return NULL;
+}
+
+static void free_test_dev_kmod(struct kmod_test_device *test_dev)
+{
+       if (test_dev) {
+               kfree_const(test_dev->misc_dev.name);
+               test_dev->misc_dev.name = NULL;
+               free_test_dev_info(test_dev);
+               kmod_config_free(test_dev);
+               vfree(test_dev);
+               test_dev = NULL;
+       }
+}
+
+static struct kmod_test_device *register_test_dev_kmod(void)
+{
+       struct kmod_test_device *test_dev = NULL;
+       int ret;
+
+       mutex_unlock(&reg_dev_mutex);
+
+       /* int should suffice for number of devices, test for wrap */
+       if (unlikely(num_test_devs + 1) < 0) {
+               pr_err("reached limit of number of test devices\n");
+               goto out;
+       }
+
+       test_dev = alloc_test_dev_kmod(num_test_devs);
+       if (!test_dev)
+               goto out;
+
+       ret = misc_register(&test_dev->misc_dev);
+       if (ret) {
+               pr_err("could not register misc device: %d\n", ret);
+               free_test_dev_kmod(test_dev);
+               goto out;
+       }
+
+       test_dev->dev = test_dev->misc_dev.this_device;
+       list_add_tail(&test_dev->list, &reg_test_devs);
+       dev_info(test_dev->dev, "interface ready\n");
+
+       num_test_devs++;
+
+out:
+       mutex_unlock(&reg_dev_mutex);
+
+       return test_dev;
+
+}
+
+static int __init test_kmod_init(void)
+{
+       struct kmod_test_device *test_dev;
+       int ret;
+
+       test_dev = register_test_dev_kmod();
+       if (!test_dev) {
+               pr_err("Cannot add first test kmod device\n");
+               return -ENODEV;
+       }
+
+       /*
+        * With some work we might be able to gracefully enable
+        * testing with this driver built-in, for now this seems
+        * rather risky. For those willing to try have at it,
+        * and enable the below. Good luck! If that works, try
+        * lowering the init level for more fun.
+        */
+       if (force_init_test) {
+               ret = trigger_config_run_driver(test_dev, "tun");
+               if (WARN_ON(ret))
+                       return ret;
+               ret = trigger_config_run_fs(test_dev, "btrfs");
+               if (WARN_ON(ret))
+                       return ret;
+       }
+
+       return 0;
+}
+late_initcall(test_kmod_init);
+
+static
+void unregister_test_dev_kmod(struct kmod_test_device *test_dev)
+{
+       mutex_lock(&test_dev->trigger_mutex);
+       mutex_lock(&test_dev->config_mutex);
+
+       test_dev_kmod_stop_tests(test_dev);
+
+       dev_info(test_dev->dev, "removing interface\n");
+       misc_deregister(&test_dev->misc_dev);
+
+       mutex_unlock(&test_dev->config_mutex);
+       mutex_unlock(&test_dev->trigger_mutex);
+
+       free_test_dev_kmod(test_dev);
+}
+
+static void __exit test_kmod_exit(void)
+{
+       struct kmod_test_device *test_dev, *tmp;
+
+       mutex_lock(&reg_dev_mutex);
+       list_for_each_entry_safe(test_dev, tmp, &reg_test_devs, list) {
+               list_del(&test_dev->list);
+               unregister_test_dev_kmod(test_dev);
+       }
+       mutex_unlock(&reg_dev_mutex);
+}
+module_exit(test_kmod_exit);
+
+MODULE_AUTHOR("Luis R. Rodriguez <[email protected]>");
+MODULE_LICENSE("GPL");

tools/testing/selftests/kmod/Makefile new file +11 -0

new file mode 100644
index 000000000000..fa2ccc5fb3de
@@ -0,0 +1,11 @@
+# Makefile for kmod loading selftests
+
+# No binaries, but make sure arg-less "make" doesn't trigger "run_tests"
+all:
+
+TEST_PROGS := kmod.sh
+
+include ../lib.mk
+
+# Nothing to clean up.
+clean:

tools/testing/selftests/kmod/config new file +7 -0

new file mode 100644
index 000000000000..259f4fd6b5e2
@@ -0,0 +1,7 @@
+CONFIG_TEST_KMOD=m
+CONFIG_TEST_LKM=m
+CONFIG_XFS_FS=m
+
+# For the module parameter force_init_test is used
+CONFIG_TUN=m
+CONFIG_BTRFS_FS=m

tools/testing/selftests/kmod/kmod.sh new file +18 -0

new file mode 100755
index 000000000000..9ea1864d8bae
@@ -0,0 +1,449 @@
+#!/bin/bash
+#
+# Copyright (C) 2016 Luis R. Rodriguez <[email protected]>
+#
+# This program is free software; you can redistribute it and/or modify it
+# under the terms of copyleft-next (version 0.3.1 or later) as published
+# at http://copyleft-next.org/.
+
+# This is a stress test script for kmod, the kernel module loader. It uses
+# test_kmod which exposes a series of knobs for the API for us so we can
+# tweak each test in userspace rather than in kernelspace.
+#
+# The way kmod works is it uses the kernel's usermode helper API to eventually
+# call /sbin/modprobe. It has a limit of the number of concurrent calls
+# possible. The kernel interface to load modules is request_module(), however
+# mount uses get_fs_type(). Both behave slightly differently, but the
+# differences are important enough to test each call separately. For this
+# reason test_kmod starts by providing tests for both calls.

tools/testing/selftests/kmod/kmod.sh new file +26 -0

+# without running out of memory on them. For other requirements refer
+# to test_reqs()
+
+set -e
+
+TEST_DRIVER="test_kmod"
+
+function allow_user_defaults()
+{
+       if [ -z $DEFAULT_KMOD_DRIVER ]; then
+               DEFAULT_KMOD_DRIVER="test_module"
+       fi
+
+       if [ -z $DEFAULT_KMOD_FS ]; then
+               DEFAULT_KMOD_FS="xfs"
+       fi
+
+       if [ -z $PROC_DIR ]; then
+               PROC_DIR="/proc/sys/kernel/"
+       fi
+
+       if [ -z $MODPROBE_LIMIT ]; then
+               MODPROBE_LIMIT=50
+       fi
+
+       if [ -z $DIR ]; then

tools/testing/selftests/kmod/kmod.sh new file +22 -0

+               exit 1
+       fi
+
+       if ! which kmod 2> /dev/null > /dev/null; then
+               echo "$0: You need kmod installed"
+               exit 1
+       fi
+
+       # kmod 19 has a bad bug where it returns 0 when modprobe
+       # gets called *even* if the module was not loaded due to
+       # some bad heuristics. For details see:
+       #
+       # A work around is possible in-kernel but its rather
+       # complex.
+       KMOD_VERSION=$(kmod --version | awk '{print $3}')
+       if [[ $KMOD_VERSION  -le 19 ]]; then
+               echo "$0: You need at least kmod 20"
+               echo "kmod <= 19 is buggy, for details see:"
+               echo "http://git.kernel.org/cgit/utils/kernel/kmod/kmod.git/commit/libkmod/libkmod-module.c?id=fd44a98ae2eb5eb32161088954ab21e58e19dfc4"
+               exit 1
+       fi
+}

tools/testing/selftests/kmod/kmod.sh new file +341 -0

+                       exit 1
+               fi
+       fi
+}
+
+test_finish()
+{
+       echo "Test completed"
+}
+
+errno_name_to_val()
+{
+       case "$1" in
+       # kmod calls modprobe and upon of a module not found
+       # modprobe returns just 1... However in the kernel we
+       # *sometimes* see 256...
+       MODULE_NOT_FOUND)
+               echo 256;;
+       SUCCESS)
+               echo 0;;
+       -EPERM)
+               echo -1;;
+       -ENOENT)
+               echo -2;;
+       -EINVAL)
+               echo -22;;
+       -ERR_ANY)
+               echo -123456;;
+       *)
+               echo invalid;;
+       esac
+}
+
+errno_val_to_name()
+       case "$1" in
+       256)
+               echo MODULE_NOT_FOUND;;
+       0)
+               echo SUCCESS;;
+       -1)
+               echo -EPERM;;
+       -2)
+               echo -ENOENT;;
+       -22)
+               echo -EINVAL;;
+       -123456)
+               echo -ERR_ANY;;
+       *)
+               echo invalid;;
+       esac
+
+config_set_test_case_driver()
+{
+       if ! echo -n 1 >$DIR/config_test_case; then
+               echo "$0: Unable to set to test case to driver" >&2
+               exit 1
+       fi
+}
+
+config_set_test_case_fs()
+{
+       if ! echo -n 2 >$DIR/config_test_case; then
+               echo "$0: Unable to set to test case to fs" >&2
+               exit 1
+       fi
+}
+
+config_num_threads()
+{
+       if ! echo -n $1 >$DIR/config_num_threads; then
+               echo "$0: Unable to set to number of threads" >&2
+               exit 1
+       fi
+}
+
+config_get_modprobe_limit()
+{
+       if [[ -f ${MODPROBE_LIMIT_FILE} ]] ; then
+               MODPROBE_LIMIT=$(cat $MODPROBE_LIMIT_FILE)
+       fi
+       echo $MODPROBE_LIMIT
+}
+
+config_num_thread_limit_extra()
+{
+       MODPROBE_LIMIT=$(config_get_modprobe_limit)
+       let EXTRA_LIMIT=$MODPROBE_LIMIT+$1
+       config_num_threads $EXTRA_LIMIT
+}
+
+# For special characters use printf directly,
+# refer to kmod_test_0001
+config_set_driver()
+{
+       if ! echo -n $1 >$DIR/config_test_driver; then
+               echo "$0: Unable to set driver" >&2
+               exit 1
+       fi
+}
+
+config_set_fs()
+{
+       if ! echo -n $1 >$DIR/config_test_fs; then
+               echo "$0: Unable to set driver" >&2
+               exit 1
+       fi
+}
+
+config_get_driver()
+{
+       cat $DIR/config_test_driver
+}
+
+config_get_test_result()
+{
+       cat $DIR/test_result
+}
+
+config_reset()
+{
+       if ! echo -n "1" >"$DIR"/reset; then
+               echo "$0: reset shuld have worked" >&2
+               exit 1
+       fi
+}
+
+config_show_config()
+{
+       echo "----------------------------------------------------"
+       cat "$DIR"/config
+       echo "----------------------------------------------------"
+}
+
+config_trigger()
+{
+       if ! echo -n "1" >"$DIR"/trigger_config 2>/dev/null; then
+               echo "$1: FAIL - loading should have worked"
+               config_show_config
+               exit 1
+       fi
+       echo "$1: OK! - loading kmod test"
+}
+
+config_trigger_want_fail()
+{
+       if echo "1" > $DIR/trigger_config 2>/dev/null; then
+               echo "$1: FAIL - test case was expected to fail"
+               config_show_config
+               exit 1
+       fi
+       echo "$1: OK! - kmod test case failed as expected"
+}
+
+config_expect_result()
+{
+       RC=$(config_get_test_result)
+       RC_NAME=$(errno_val_to_name $RC)
+
+       ERRNO_NAME=$2
+       ERRNO=$(errno_name_to_val $ERRNO_NAME)
+
+       if [[ $ERRNO_NAME = "-ERR_ANY" ]]; then
+               if [[ $RC -ge 0 ]]; then
+                       echo "$1: FAIL, test expects $ERRNO_NAME - got $RC_NAME ($RC)" >&2
+                       config_show_config
+                       exit 1
+               fi
+       elif [[ $RC != $ERRNO ]]; then
+               echo "$1: FAIL, test expects $ERRNO_NAME ($ERRNO) - got $RC_NAME ($RC)" >&2
+               config_show_config
+               exit 1
+       fi
+       echo "$1: OK! - Return value: $RC ($RC_NAME), expected $ERRNO_NAME"
+}
+
+kmod_defaults_driver()
+{
+       config_reset
+       modprobe -r $DEFAULT_KMOD_DRIVER
+       config_set_driver $DEFAULT_KMOD_DRIVER
+}
+
+kmod_defaults_fs()
+{
+       config_reset
+       modprobe -r $DEFAULT_KMOD_FS
+       config_set_fs $DEFAULT_KMOD_FS
+       config_set_test_case_fs
+}
+
+kmod_test_0001_driver()
+{
+       NAME='\000'
+
+       kmod_defaults_driver
+       config_num_threads 1
+       printf '\000' >"$DIR"/config_test_driver
+       config_trigger ${FUNCNAME[0]}
+       config_expect_result ${FUNCNAME[0]} MODULE_NOT_FOUND
+}
+
+kmod_test_0001_fs()
+{
+       NAME='\000'
+
+       kmod_defaults_fs
+       config_num_threads 1
+       printf '\000' >"$DIR"/config_test_fs
+       config_trigger ${FUNCNAME[0]}
+       config_expect_result ${FUNCNAME[0]} -EINVAL
+}
+
+kmod_test_0001()
+{
+       kmod_test_0001_driver
+       kmod_test_0001_fs
+}
+
+kmod_test_0002_driver()
+{
+       NAME="nope-$DEFAULT_KMOD_DRIVER"
+
+       kmod_defaults_driver
+       config_set_driver $NAME
+       config_num_threads 1
+       config_trigger ${FUNCNAME[0]}
+       config_expect_result ${FUNCNAME[0]} MODULE_NOT_FOUND
+}
+
+kmod_test_0002_fs()
+{
+       NAME="nope-$DEFAULT_KMOD_FS"
+
+       kmod_defaults_fs
+       config_set_fs $NAME
+       config_trigger ${FUNCNAME[0]}
+       config_expect_result ${FUNCNAME[0]} -EINVAL
+}
+
+kmod_test_0002()
+{
+       kmod_test_0002_driver
+       kmod_test_0002_fs
+}
+
+kmod_test_0003()
+{
+       kmod_defaults_fs
+       config_num_threads 1
+       config_trigger ${FUNCNAME[0]}
+       config_expect_result ${FUNCNAME[0]} SUCCESS
+}
+
+kmod_test_0004()
+{
+       kmod_defaults_fs
+       config_num_threads 2
+       config_trigger ${FUNCNAME[0]}
+       config_expect_result ${FUNCNAME[0]} SUCCESS
+}
+
+kmod_test_0005()
+{
+       kmod_defaults_driver
+       config_trigger ${FUNCNAME[0]}
+       config_expect_result ${FUNCNAME[0]} SUCCESS
+}
+
+kmod_test_0006()
+{
+       kmod_defaults_fs
+       config_trigger ${FUNCNAME[0]}
+       config_expect_result ${FUNCNAME[0]} SUCCESS
+}
+
+kmod_test_0007()
+{
+       kmod_test_0005
+       kmod_test_0006
+}
+
+kmod_test_0008()
+{
+       kmod_defaults_driver
+       MODPROBE_LIMIT=$(config_get_modprobe_limit)
+       let EXTRA=$MODPROBE_LIMIT/2
+       config_num_thread_limit_extra $EXTRA
+       config_trigger ${FUNCNAME[0]}
+       config_expect_result ${FUNCNAME[0]} -EINVAL
+}
+
+kmod_test_0009()
+{
+       kmod_defaults_fs
+       #MODPROBE_LIMIT=$(config_get_modprobe_limit)
+       #let EXTRA=$MODPROBE_LIMIT/3
+       config_num_thread_limit_extra 5
+       config_trigger ${FUNCNAME[0]}
+       config_expect_result ${FUNCNAME[0]} -EINVAL
+}
+
+trap "test_finish" EXIT
+test_reqs
+allow_user_defaults
+load_req_mod
+
+usage()
+{
+       echo "Usage: $0 [ -t <4-number-digit> ]"
+       echo "Valid tests: 0001-0011"
+       echo
+       echo "0001 - Simple test - 1 thread  for empty string"
+       echo "0002 - Simple test - 1 thread  for modules/filesystems that do not exist"
+       echo "0003 - Simple test - 1 thread  for get_fs_type() only"
+       echo "0004 - Simple test - 2 threads for get_fs_type() only"
+       echo "0005 - multithreaded tests with default setup - request_module() only"
+       echo "0006 - multithreaded tests with default setup - get_fs_type() only"
+       echo "0007 - multithreaded tests with default setup test request_module() and get_fs_type()"
+       echo "0008 - multithreaded - push kmod_concurrent over max_modprobes for request_module()"
+       echo "0009 - multithreaded - push kmod_concurrent over max_modprobes for get_fs_type()"
+       exit 1
+}
+
+# You can ask for a specific test:
+if [[ $# > 0 ]] ; then
+       if [[ $1 != "-t" ]]; then
+               usage
+       fi
+
+       re='^[0-9]+$'
+       if ! [[ $2 =~ $re ]]; then
+               usage
+       fi
+
+       RUN_TEST=kmod_test_$2
+       $RUN_TEST
+       exit 0
+fi
+
+# Once tese are enabled please leave them as-is. Write your own test,
+# we have tons of space.

tools/testing/selftests/kmod/kmod.sh new file +2 -1

+
+exit 0
--

2.10.1

Quoted 1 line · Luis R. Rodriguez

The kmod.sh script uses the above constructs to build differnt test cases.

Typo: different

Quoted 1 line

3) finit_module() consumes quite a bit of memory.

Is this due to reading the module into kernel memory or something else?

Quoted 6 lines

4) Filesystems typically also have more dependent modules than other modules, its important to note though that even though a get_fs_type() call does not incur additional kmod_concurrent bumps, since userspace loads dependencies it finds it needs via finit_module_fd(), it *will* take much more memory to load a module with a lot of dependencies.

Quoted 4 lines

Because of 3) and 4) we will easily run into out of memory failures with certain tests. For instance test 0006 fails on qemu with 1024 MiB of RAM. It panics a box after reaping all userspace processes and still not having enough memory to reap.

Are the buffers not released until after all the dependent modules are loaded? I thought it would load one by one?

Quoted 1 line

Signed-off-by Luis R. Rodriguez <[email protected]>

This is a great selftest, thanks for working on it!

Notes below...

Quoted 8 lines

lib/Kconfig.debug +0 -0

index 7446097f72bd..6cad548e0682 100644
@@ -1994,6 +1994,31 @@ config BUG_ON_DATA_CORRUPTION
 
          If unsure, say N.
 

lib/Kconfig.debug config BUG_ON_DATA_CORRUPTION · line 2004

Quoted 8 lines

lib/Kconfig.debug +8 -0

+config TEST_KMOD
+       tristate "kmod stress tester"
+       default n
+       depends on m
+       select TEST_LKM
+       select XFS_FS
+       select TUN
+       select BTRFS_FS

Since the desired FS can be changed at runtime, maybe these selects aren't needed?

Quoted 6 lines

lib/test_kmod.c new file +0 -0

new file mode 100644
index 000000000000..63fded83b9b6
@@ -0,0 +1,1248 @@

lib/test_kmod.c line 41

Quoted 4 lines

lib/test_kmod.c new file +4 -0

+static bool force_init_test = false;
+module_param(force_init_test, bool_enable_only, 0644);
+MODULE_PARM_DESC(force_init_test,
+                "Force kicking a test immediatley after driver loads");

Typo: immediately

Quoted 25 lines

lib/test_kmod.c new file +25 -0

+static ssize_t config_show(struct device *dev,
+                          struct device_attribute *attr,
+                          char *buf)
+{
+       struct kmod_test_device *test_dev = dev_to_test_dev(dev);
+       struct test_config *config = &test_dev->config;
+       int len = 0;
+
+       mutex_lock(&test_dev->config_mutex);
+
+       len += sprintf(buf, "Custom trigger configuration for: %s\n",
+                      dev_name(dev));
+
+       len += sprintf(buf+len, "Number of threads:\t%u\n",
+                      config->num_threads);
+
+       len += sprintf(buf+len, "Test_case:\t%s (%u)\n",
+                      test_case_str(config->test_case),
+                      config->test_case);
+
+       if (config->test_driver)
+               len += sprintf(buf+len, "driver:\t%s\n",
+                              config->test_driver);
+       else
+               len += sprintf(buf+len, "driver:\tEMTPY\n");

lib/test_kmod.c line 482

Quoted 6 lines

lib/test_kmod.c new file +6 -0

+
+       if (config->test_fs)
+               len += sprintf(buf+len, "fs:\t%s\n",
+                              config->test_fs);
+       else
+               len += sprintf(buf+len, "fs:\tEMTPY\n");

These should all use snprintf...

Quoted 7 lines

lib/test_kmod.c new file +7 -0

+static ssize_t config_test_driver_show(struct device *dev,
+                                       struct device_attribute *attr,
+                                       char *buf)
+{
+       struct kmod_test_device *test_dev = dev_to_test_dev(dev);
+       struct test_config *config = &test_dev->config;
+

lib/test_kmod.c line 667

Quoted 3 lines

lib/test_kmod.c new file +3 -0

+       mutex_lock(&test_dev->config_mutex);
+       strcpy(buf, config->test_driver);
+       strcat(buf, "\n");

IIUC, the show/store API uses a max size of PAGE_SIZE. If that's correct, it's possible that this show routine could write past the end of buf, due to the end newline, etc. Best to use snprintf like you do below for the other shows.

Quoted 7 lines

lib/test_kmod.c new file +7 -0

+static ssize_t config_test_fs_show(struct device *dev,
+                                  struct device_attribute *attr,
+                                  char *buf)
+{
+       struct kmod_test_device *test_dev = dev_to_test_dev(dev);
+       struct test_config *config = &test_dev->config;
+

lib/test_kmod.c line 704

Quoted 4 lines

lib/test_kmod.c new file +4 -0

+       mutex_lock(&test_dev->config_mutex);
+       strcpy(buf, config->test_fs);
+       strcat(buf, "\n");
+       mutex_unlock(&test_dev->config_mutex);

Same here... (which, btw, could likely use to be a helper function, the show and store functions here are identical except for test_driver vs test_fs).

Quoted 19 lines

lib/test_kmod.c new file +19 -0

+
+       return strlen(buf) + 1;
+}
+static DEVICE_ATTR(config_test_fs, 0644, config_test_fs_show,
+                  config_test_fs_store);
+
+static int trigger_config_run_driver(struct kmod_test_device *test_dev,
+                                    const char *test_driver)
+{
+       int copied;
+       struct test_config *config = &test_dev->config;
+
+       mutex_lock(&test_dev->config_mutex);
+
+       config->test_case = TEST_KMOD_DRIVER;
+
+       kfree_const(config->test_driver);
+       config->test_driver = NULL;
+

lib/test_kmod.c line 728

Quoted 5 lines

lib/test_kmod.c new file +5 -0

+       copied = config_copy_test_driver_name(config, test_driver,
+                                             strlen(test_driver));
+       mutex_unlock(&test_dev->config_mutex);
+
+       if (copied != strlen(test_driver)) {

Can't these copied tests just check < 0? (i.e. avoid the repeated strlen which can be fragile.)

lib/test_kmod.c line 82

Quoted 2 lines

lib/test_kmod.c new file +2 -0

+               test_dev->test_is_oom = true;
+               return -EINVAL;

Quoted 26 lines

lib/test_kmod.c new file +26 -0

+       }
+
+       test_dev->test_is_oom = false;
+
+       return trigger_config_run(test_dev);
+}
+
+static int trigger_config_run_fs(struct kmod_test_device *test_dev,
+                                const char *fs_type)
+{
+       int copied;
+       struct test_config *config = &test_dev->config;
+
+       mutex_lock(&test_dev->config_mutex);
+       config->test_case = TEST_KMOD_FS_TYPE;
+
+       kfree_const(config->test_fs);
+       config->test_driver = NULL;
+
+       copied = config_copy_test_fs(config, fs_type, strlen(fs_type));
+       mutex_unlock(&test_dev->config_mutex);
+
+       if (copied != strlen(fs_type)) {
+               test_dev->test_is_oom = true;
+               return -EINVAL;
+       }

lib/test_kmod.c line 736

Quoted 5 lines

lib/test_kmod.c new file +5 -0

+
+       test_dev->test_is_oom = false;
+
+       return trigger_config_run(test_dev);
+}

These two functions are almost identical too. Only test_case and the copy function change...

lib/test_kmod.c line 769

Quoted 7 lines

lib/test_kmod.c new file +7 -0

+static void free_test_dev_info(struct kmod_test_device *test_dev)
+{
+       if (test_dev->info) {
+               vfree(test_dev->info);
+               test_dev->info = NULL;
+       }
+}

vfree() already checks for NULL, you can drop the if.

Quoted 7 lines

lib/test_kmod.c new file +7 -0

+static int test_dev_config_update_uint_range(struct kmod_test_device *test_dev,
+                                            const char *buf, size_t size,
+                                            unsigned int *config,
+                                            unsigned int min,
+                                            unsigned int max)
+{
+       char *end;

lib/test_kmod.c line 914

Quoted 6 lines

lib/test_kmod.c new file +6 -0

+       long new = simple_strtol(buf, &end, 0);
+       if (end == buf || new < min || new >  max || new > UINT_MAX)
+               return -EINVAL;
+
+       mutex_lock(&test_dev->config_mutex);
+       *(unsigned int *)config = new;

config is already an unsigned int *, why cast?

Quoted 4 lines

lib/test_kmod.c new file +4 -0

+static int test_dev_config_update_int(struct kmod_test_device *test_dev,
+                                     const char *buf, size_t size,
+                                     int *config)
+{

lib/test_kmod.c line 930

Quoted 6 lines

lib/test_kmod.c new file +6 -0

+       char *end;
+       long new = simple_strtol(buf, &end, 0);
+       if (end == buf || new > INT_MAX || new < INT_MIN)
+               return -EINVAL;
+       mutex_lock(&test_dev->config_mutex);
+       *(int *)config = new;

config is already an int *, why cast?

Quoted 8 lines

lib/test_kmod.c new file +8 -0

+/*
+ * XXX: this could perhaps be made generic already too, but a hunt
+ * for actual users would be needed first. It could be generic
+ * if other test drivers end up using a similar mechanism.
+ */
+const char *test_dev_get_name(const char *base, int idx, gfp_t gfp)
+{
+       const char *name_const;

lib/test_kmod.c line 1075

Quoted 6 lines

lib/test_kmod.c new file +6 -0

+       char *name;
+
+       if (!base)
+               return NULL;
+       if (strlen(base) > 30)
+               return NULL;

why?

lib/test_kmod.c line 1087

Quoted 13 lines

lib/test_kmod.c new file +12 -0

+       name = kzalloc(1024, gfp);
+       if (!name)
+               return NULL;
+
+       strncat(name, base, strlen(base));
+       sprintf(name+(strlen(base)), "%d", idx);
+       name_const = kstrdup_const(name, gfp);
+
+       kfree(name);
+
+       return name_const;
+}

What is going on here? Why not just:

    return kasprintf(gfp, "%s%d", base, idx);

For all of that code? And kstrdup_const is pointless here since it'll always just do the dup (as the kmalloc source isn't in rodata).

Quoted 13 lines

tools/testing/selftests/kmod/config new file +7 -0

new file mode 100644
index 000000000000..259f4fd6b5e2
@@ -0,0 +1,7 @@
+CONFIG_TEST_KMOD=m
+CONFIG_TEST_LKM=m
+CONFIG_XFS_FS=m
+
+# For the module parameter force_init_test is used
+CONFIG_TUN=m
+CONFIG_BTRFS_FS=m

tools/testing/selftests/kmod/kmod.sh line 2

Quoted 8 lines

tools/testing/selftests/kmod/kmod.sh new file +2 -0

new file mode 100755
index 000000000000..9ea1864d8bae
@@ -0,0 +1,449 @@
+#!/bin/bash
+#

tools/testing/selftests/kmod/kmod.sh line 28

Quoted 1 line

tools/testing/selftests/kmod/kmod.sh new file +1 -0

+# You'll want at least 4096 GiB of RAM to expect to run these tests

4TiB of RAM? I assume this was meant to be 4 GiB not 4096?

Quoted 32 lines

tools/testing/selftests/kmod/kmod.sh new file +32 -0

+# without running out of memory on them. For other requirements refer
+# to test_reqs()
+
+set -e
+
+TEST_DRIVER="test_kmod"
+
+function allow_user_defaults()
+{
+       if [ -z $DEFAULT_KMOD_DRIVER ]; then
+               DEFAULT_KMOD_DRIVER="test_module"
+       fi
+
+       if [ -z $DEFAULT_KMOD_FS ]; then
+               DEFAULT_KMOD_FS="xfs"
+       fi
+
+       if [ -z $PROC_DIR ]; then
+               PROC_DIR="/proc/sys/kernel/"
+       fi
+
+       if [ -z $MODPROBE_LIMIT ]; then
+               MODPROBE_LIMIT=50
+       fi
+
+       if [ -z $DIR ]; then
+               DIR="/sys/devices/virtual/misc/${TEST_DRIVER}0/"
+       fi
+
+       MODPROBE_LIMIT_FILE="${PROC_DIR}/kmod-limit"
+}
+

tools/testing/selftests/kmod/kmod.sh line 64

Quoted 4 lines

tools/testing/selftests/kmod/kmod.sh new file +4 -0

+test_reqs()
+{
+       if ! which modprobe 2> /dev/null > /dev/null; then
+               echo "$0: You need modprobe installed"

While not a huge deal, I prefer that error messages end up on stderr, so adding >&2 to all the failure echos (or providing an err function) would be nice. (This happens in later places...)

Quoted 4 lines

tools/testing/selftests/kmod/kmod.sh new file +4 -0

+function load_req_mod()
+{
+       if [ ! -d $DIR ]; then
+               # Alanis: "Oh isn't it ironic?"

tools/testing/selftests/kmod/kmod.sh line 96

Quoted 5 lines

tools/testing/selftests/kmod/kmod.sh new file +5 -0

+               modprobe $TEST_DRIVER
+               if [ ! -d $DIR ]; then
+                       echo "$0: $DIR not present"
+                       echo "You must have the following enabled in your kernel:"
+                       cat $PWD/config

I like this (minimum config in the test directory). Are other tests doing this too?

Quoted 7 lines

tools/testing/selftests/kmod/kmod.sh new file +7 -0

+# Once tese are enabled please leave them as-is. Write your own test,
+# we have tons of space.
+kmod_test_0001
+kmod_test_0002
+kmod_test_0003
+kmod_test_0004
+kmod_test_0005

tools/testing/selftests/kmod/kmod.sh line 447

Quoted 5 lines

tools/testing/selftests/kmod/kmod.sh new file +5 -0

+kmod_test_0006
+kmod_test_0007
+
+#kmod_test_0008
+#kmod_test_0009

While it's documented in the commit log, I think a short note for each disabled test should be added here too.

Quoted 4 lines · Kees Cook

Quoted 1 line · Luis R. Rodriguez

3) finit_module() consumes quite a bit of memory.

Is this due to reading the module into kernel memory or something else?

Very likely yes, but to be honest I have not had chance to instrument too carefully, its TODO work :)

Quoted 7 lines

Quoted 7 lines

tools/testing/selftests/kmod/kmod.sh new file +7 -0

+# Once tese are enabled please leave them as-is. Write your own test,
+# we have tons of space.
+kmod_test_0001
+kmod_test_0002
+kmod_test_0003
+kmod_test_0004
+kmod_test_0005

Quoted 8 lines

tools/testing/selftests/kmod/kmod.sh line 447

Quoted 5 lines

tools/testing/selftests/kmod/kmod.sh new file +5 -0

+kmod_test_0006
+kmod_test_0007
+
+#kmod_test_0008
+#kmod_test_0009

While it's documented in the commit log, I think a short note for each disabled test should be added here too.

Will do, thanks so much for the review!

While looking for early possible module loading failures I was able to reproduce a memory leak possible with kmemleak. There are a few rare ways to trigger a failure:

o we've run into a failure while processing kernel parameters (parse_args() returns an error)
o mod_sysfs_setup() fails
o we're a live patch module and copy_module_elf() fails

Chances of running into this issue is really low.

kmemleak splat:

unreferenced object 0xffff9f2c4ada1b00 (size 32):
comm "kworker/u16:4", pid 82, jiffies 4294897636 (age 681.816s) hex dump (first 32 bytes):

    6d 65 6d 73 74 69 63 6b 30 00 00 00 00 00 00 00  memstick0.......
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff8c6cfeba>] kmemleak_alloc+0x4a/0xa0
    [<ffffffff8c200046>] __kmalloc_track_caller+0x126/0x230
    [<ffffffff8c1bc581>] kstrdup+0x31/0x60
    [<ffffffff8c1bc5d4>] kstrdup_const+0x24/0x30
    [<ffffffff8c3c23aa>] kvasprintf_const+0x7a/0x90
    [<ffffffff8c3b5481>] kobject_set_name_vargs+0x21/0x90
    [<ffffffff8c4fbdd7>] dev_set_name+0x47/0x50
    [<ffffffffc07819e5>] memstick_check+0x95/0x33c [memstick]
    [<ffffffff8c09c893>] process_one_work+0x1f3/0x4b0
    [<ffffffff8c09cb98>] worker_thread+0x48/0x4e0
    [<ffffffff8c0a2b79>] kthread+0xc9/0xe0
    [<ffffffff8c6dab5f>] ret_from_fork+0x1f/0x40
    [<ffffffffffffffff>] 0xffffffffffffffff

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/module.c | 1 +
 1 file changed, 1 insertion(+)

kernel/module.c +1 -1

index f7482db0f843..e420ed67e533 100644
@@ -3722,6 +3722,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
        mod_sysfs_teardown(mod);
  coming_cleanup:
        mod->state = MODULE_STATE_GOING;
+       destroy_params(mod->kp, mod->num_kp);
        blocking_notifier_call_chain(&module_notify_list,
                                     MODULE_STATE_GOING, mod);
        klp_module_going(mod);
--

2.10.1

Quoted 33 lines · Luis R. Rodriguez

While looking for early possible module loading failures I was able to reproduce a memory leak possible with kmemleak. There are a few rare ways to trigger a failure:

o we've run into a failure while processing kernel parameters (parse_args() returns an error)
o mod_sysfs_setup() fails
o we're a live patch module and copy_module_elf() fails

Chances of running into this issue is really low.

kmemleak splat:

unreferenced object 0xffff9f2c4ada1b00 (size 32):
comm "kworker/u16:4", pid 82, jiffies 4294897636 (age 681.816s) hex dump (first 32 bytes):

    6d 65 6d 73 74 69 63 6b 30 00 00 00 00 00 00 00  memstick0.......
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff8c6cfeba>] kmemleak_alloc+0x4a/0xa0
    [<ffffffff8c200046>] __kmalloc_track_caller+0x126/0x230
    [<ffffffff8c1bc581>] kstrdup+0x31/0x60
    [<ffffffff8c1bc5d4>] kstrdup_const+0x24/0x30
    [<ffffffff8c3c23aa>] kvasprintf_const+0x7a/0x90
    [<ffffffff8c3b5481>] kobject_set_name_vargs+0x21/0x90
    [<ffffffff8c4fbdd7>] dev_set_name+0x47/0x50
    [<ffffffffc07819e5>] memstick_check+0x95/0x33c [memstick]
    [<ffffffff8c09c893>] process_one_work+0x1f3/0x4b0
    [<ffffffff8c09cb98>] worker_thread+0x48/0x4e0
    [<ffffffff8c0a2b79>] kthread+0xc9/0xe0
    [<ffffffff8c6dab5f>] ret_from_fork+0x1f/0x40
    [<ffffffffffffffff>] 0xffffffffffffffff

Quoted 1 line

Signed-off-by Luis R. Rodriguez <[email protected]>

Acked-by Kees Cook <[email protected]>

Is this worth sending through -stable too?

Quoted 36 lines · Kees Cook

Quoted 34 lines · Luis R. Rodriguez

While looking for early possible module loading failures I was able to reproduce a memory leak possible with kmemleak. There are a few rare ways to trigger a failure:

o we've run into a failure while processing kernel parameters (parse_args() returns an error)
o mod_sysfs_setup() fails
o we're a live patch module and copy_module_elf() fails

Chances of running into this issue is really low.

kmemleak splat:

unreferenced object 0xffff9f2c4ada1b00 (size 32):
comm "kworker/u16:4", pid 82, jiffies 4294897636 (age 681.816s) hex dump (first 32 bytes):

    6d 65 6d 73 74 69 63 6b 30 00 00 00 00 00 00 00  memstick0.......
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff8c6cfeba>] kmemleak_alloc+0x4a/0xa0
    [<ffffffff8c200046>] __kmalloc_track_caller+0x126/0x230
    [<ffffffff8c1bc581>] kstrdup+0x31/0x60
    [<ffffffff8c1bc5d4>] kstrdup_const+0x24/0x30
    [<ffffffff8c3c23aa>] kvasprintf_const+0x7a/0x90
    [<ffffffff8c3b5481>] kobject_set_name_vargs+0x21/0x90
    [<ffffffff8c4fbdd7>] dev_set_name+0x47/0x50
    [<ffffffffc07819e5>] memstick_check+0x95/0x33c [memstick]
    [<ffffffff8c09c893>] process_one_work+0x1f3/0x4b0
    [<ffffffff8c09cb98>] worker_thread+0x48/0x4e0
    [<ffffffff8c0a2b79>] kthread+0xc9/0xe0
    [<ffffffff8c6dab5f>] ret_from_fork+0x1f/0x40
    [<ffffffffffffffff>] 0xffffffffffffffff

Signed-off-by Luis R. Rodriguez <[email protected]>

Quoted 3 lines

Acked-by Kees Cook <[email protected]>

Is this worth sending through -stable too?

Yes, for some reason git-send e-mail complained to me about [email protected] not being a valid local address, so I had to remove it, but indeed. I'll try to fix this e-mail issue later and add your tag.

While looking for early possible module loading failures I was able to reproduce a memory leak possible with kmemleak. There are a few rare ways to trigger a failure:

o we've run into a failure while processing kernel parameters (parse_args() returns an error)
o mod_sysfs_setup() fails
o we're a live patch module and copy_module_elf() fails

Chances of running into this issue is really low.

kmemleak splat:

unreferenced object 0xffff9f2c4ada1b00 (size 32):
comm "kworker/u16:4", pid 82, jiffies 4294897636 (age 681.816s) hex dump (first 32 bytes):

    6d 65 6d 73 74 69 63 6b 30 00 00 00 00 00 00 00  memstick0.......
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff8c6cfeba>] kmemleak_alloc+0x4a/0xa0
    [<ffffffff8c200046>] __kmalloc_track_caller+0x126/0x230
    [<ffffffff8c1bc581>] kstrdup+0x31/0x60
    [<ffffffff8c1bc5d4>] kstrdup_const+0x24/0x30
    [<ffffffff8c3c23aa>] kvasprintf_const+0x7a/0x90
    [<ffffffff8c3b5481>] kobject_set_name_vargs+0x21/0x90
    [<ffffffff8c4fbdd7>] dev_set_name+0x47/0x50
    [<ffffffffc07819e5>] memstick_check+0x95/0x33c [memstick]
    [<ffffffff8c09c893>] process_one_work+0x1f3/0x4b0
    [<ffffffff8c09cb98>] worker_thread+0x48/0x4e0
    [<ffffffff8c0a2b79>] kthread+0xc9/0xe0
    [<ffffffff8c6dab5f>] ret_from_fork+0x1f/0x40
    [<ffffffffffffffff>] 0xffffffffffffffff

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/module.c | 1 +
 1 file changed, 1 insertion(+)

kernel/module.c +1 -0

index f7482db0f843..e420ed67e533 100644
@@ -3722,6 +3722,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	mod_sysfs_teardown(mod);
  coming_cleanup:
 	mod->state = MODULE_STATE_GOING;
+	destroy_params(mod->kp, mod->num_kp);
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
 	klp_module_going(mod);

Signature · 1 line

2.10.1

Reviewed-by Miroslav Benes <[email protected]>

What about

Fixes e180a6b7759a ("param: fix charp parameters set via sysfs")

?

While looking for early possible module loading failures I was able to reproduce a memory leak possible with kmemleak. There are a few rare ways to trigger a failure:

o we've run into a failure while processing kernel parameters (parse_args() returns an error)
o mod_sysfs_setup() fails
o we're a live patch module and copy_module_elf() fails

Chances of running into this issue is really low.

kmemleak splat:

unreferenced object 0xffff9f2c4ada1b00 (size 32):
comm "kworker/u16:4", pid 82, jiffies 4294897636 (age 681.816s) hex dump (first 32 bytes):

    6d 65 6d 73 74 69 63 6b 30 00 00 00 00 00 00 00  memstick0.......
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff8c6cfeba>] kmemleak_alloc+0x4a/0xa0
    [<ffffffff8c200046>] __kmalloc_track_caller+0x126/0x230
    [<ffffffff8c1bc581>] kstrdup+0x31/0x60
    [<ffffffff8c1bc5d4>] kstrdup_const+0x24/0x30
    [<ffffffff8c3c23aa>] kvasprintf_const+0x7a/0x90
    [<ffffffff8c3b5481>] kobject_set_name_vargs+0x21/0x90
    [<ffffffff8c4fbdd7>] dev_set_name+0x47/0x50
    [<ffffffffc07819e5>] memstick_check+0x95/0x33c [memstick]
    [<ffffffff8c09c893>] process_one_work+0x1f3/0x4b0
    [<ffffffff8c09cb98>] worker_thread+0x48/0x4e0
    [<ffffffff8c0a2b79>] kthread+0xc9/0xe0
    [<ffffffff8c6dab5f>] ret_from_fork+0x1f/0x40
    [<ffffffffffffffff>] 0xffffffffffffffff

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/module.c | 1 +
 1 file changed, 1 insertion(+)

We currently statically limit the number of modprobe threads which we allow to run concurrently to 50. As per Keith Owens, this was a completely arbitrary value, and it was set in the 2.3.38 days [0] over 16 years ago in year 2000.

Although we haven't yet hit our lower limits, experimentation [1] shows that when and if we hit this limit in the worst case, will be fatal -- consider get_fs_type() failures upon mount on a system which has many partitions, some of which might even be with the same filesystem. Its best to be prudent and increase and set this value to something more sensible which ensures we're far from hitting the limit and also allows default build/user run time override.

The worst case is fatal given that once a module fails to load there is a period of time during which subsequent request for the same module will fail, so in the case of partitions its not just one request that could fail, but whole series of partitions. This later issue of a module request failure domino effect can be addressed later, but increasing the limit to something more meaninful should at least give us enough cushion to avoid this for a while.

Set this value up with a bit more meaninful modern limits:

Bump this up to 64 max for small systems (CONFIG_BASE_SMALL) Bump this up to 128 max for larger systems (!CONFIG_BASE_SMALL)

Also allow the default max limit to be further fine tuned at compile time and at initialization at run time at boot up using the kernel
parameter: max_modprobes.

[0] https://git.kernel.org/cgit/linux/kernel/git/history/history.git/commit/?id=ab1c4ec7410f6ec64e1511d1a7d850fc99c09b44
[1] https://github.com/mcgrof/test_request_module

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 Documentation/admin-guide/kernel-parameters.txt |  7 ++++
 include/linux/kmod.h                            |  3 +-
 init/Kconfig                                    | 23 +++++++++++++
 init/main.c                                     |  1 +
 kernel/kmod.c                                   | 43 ++++++++++++++++---------
 5 files changed, 61 insertions(+), 16 deletions(-)

Documentation/admin-guide/kernel-parameters.txt +7 -0

index be2d6d0a03a4..92bcccc65ea4 100644
@@ -1700,6 +1700,13 @@
 
        keepinitrd      [HW,ARM]
 
+       kmod.max_modprobes [KNL]
+                       This lets you set the max allowed of concurrent
+                       modprobes threads possible on a system overriding the
+                       default heuristic of:
+
+                               min(max_threads/2, 2 << CONFIG_MAX_KMOD_CONCURRENT)
+
        kernelcore=     [KNL,X86,IA-64,PPC]
                        Format: nn[KMGTPE] | "mirror"
                        This parameter

include/linux/kmod.h +2 -1

index fcfd2bf14d3f..15783cd7f056 100644
@@ -38,13 +38,14 @@ int __request_module(bool wait, const char *name, ...);
 #define request_module_nowait(mod...) __request_module(false, mod)
 #define try_then_request_module(x, mod...) \
        ((x) ?: (__request_module(true, mod), (x)))
+void init_kmod_umh(void);
 #else
 static inline int request_module(const char *name, ...) { return -ENOSYS; }
 static inline int request_module_nowait(const char *name, ...) { return -ENOSYS; }
+static inline void init_kmod_umh(void) { }
 #define try_then_request_module(x, mod...) (x)
 #endif
 
-
 struct cred;
 struct file;
 

init/Kconfig +23 -0

index 271692a352f1..da2c25746937 100644
@@ -2111,6 +2111,29 @@ config TRIM_UNUSED_KSYMS
 
          If unsure, or if you need to build out-of-tree modules, say N.
 
+config MAX_KMOD_CONCURRENT
+       int "Max allowed concurrent request_module() calls (6=>64, 10=>1024)"
+       range 0 14
+       default 6 if !BASE_SMALL
+       default 7 if BASE_SMALL
+       help
+         The kernel restricts the number of possible concurrent calls to
+         request_module() to help avoid a recursive loop possible with
+         modules. The default maximum number of concurrent threads allowed
+         to run request_module() will be:
+
+           max_modprobes = min(max_threads/2, 2 << CONFIG_MAX_KMOD_CONCURRENT);
+
+         The value set in CONFIG_MAX_KMOD_CONCURRENT represents then the power
+         of 2 value used at boot time for the above computation. You can
+         override the default built value using the kernel parameter:
+
+               kmod.max_modprobes=4096
+
+         We set this to default to 64 (2^6) concurrent modprobe threads for
+         small systems, for larger systems this defaults to 128 (2^7)
+         concurrent modprobe threads.
+
 endif # MODULES
 
 config MODULES_TREE_LOOKUP

init/main.c +1 -0

index 8161208d4ece..1fa441aa32c6 100644
@@ -638,6 +638,7 @@ asmlinkage __visible void __init start_kernel(void)
        thread_stack_cache_init();
        cred_init();
        fork_init();
+       init_kmod_umh();
        proc_caches_init();
        buffer_init();
        key_init();

kernel/kmod.c +12 -15

index 0277d1216f80..cb6f7ca7b8a5 100644
@@ -44,6 +44,9 @@
 #include <trace/events/module.h>
 
 extern int max_threads;
+unsigned int max_modprobes;
+module_param(max_modprobes, uint, 0644);
+MODULE_PARM_DESC(max_modprobes, "Max number of allowed concurrent modprobes");
 
 #define CAP_BSET       (void *)1
 #define CAP_PI         (void *)2

@@ -125,10 +128,8 @@ int __request_module(bool wait, const char *fmt, ...)
 {
        va_list args;
        char module_name[MODULE_NAME_LEN];
-       unsigned int max_modprobes;
        int ret;
        static atomic_t kmod_concurrent = ATOMIC_INIT(0);
-#define MAX_KMOD_CONCURRENT 50 /* Completely arbitrary value - KAO */
        static int kmod_loop_msg;
 
        /*

@@ -152,19 +153,6 @@ int __request_module(bool wait, const char *fmt, ...)
        if (ret)
                return ret;
 
-       /* If modprobe needs a service that is in a module, we get a recursive
-        * loop.  Limit the number of running kmod threads to max_threads/2 or
-        * MAX_KMOD_CONCURRENT, whichever is the smaller.  A cleaner method
-        * would be to run the parents of this process, counting how many times
-        * kmod was invoked.  That would mean accessing the internals of the
-        * process tables to get the command line, proc_pid_cmdline is static
-        * and it is not worth changing the proc code just to handle this case.
-        * KAO.
-        *
-        * "trace the ppid" is simple, but will fail if someone's
-        * parent exits.  I think this is as good as it gets. --RR
-        */
-       max_modprobes = min(max_threads/2, MAX_KMOD_CONCURRENT);
        atomic_inc(&kmod_concurrent);
        if (atomic_read(&kmod_concurrent) > max_modprobes) {
                /* We may be blaming an innocent here, but unlikely */

@@ -186,6 +174,31 @@ int __request_module(bool wait, const char *fmt, ...)
        return ret;
 }
 EXPORT_SYMBOL(__request_module);
+
+/*
+ * If modprobe needs a service that is in a module, we get a recursive
+ * loop.  Limit the number of running kmod threads to max_threads/2 or
+ * CONFIG_MAX_KMOD_CONCURRENT, whichever is the smaller.  A cleaner method
+ * would be to run the parents of this process, counting how many times
+ * kmod was invoked.  That would mean accessing the internals of the
+ * process tables to get the command line, proc_pid_cmdline is static
+ * and it is not worth changing the proc code just to handle this case.

kernel/kmod.c +6 -1

+{
+       if (!max_modprobes)
+               max_modprobes = min(max_threads/2,
+                                   2 << CONFIG_MAX_KMOD_CONCURRENT);
+}
+
 #endif /* CONFIG_MODULES */
 
 static void call_usermodehelper_freeinfo(struct subprocess_info *info)
--

2.10.1

Quoted 22 lines · Luis R. Rodriguez

kernel/kmod.c +13 -0

index 0277d1216f80..cb6f7ca7b8a5 100644
@@ -44,6 +44,9 @@

@@ -186,6 +174,31 @@ int __request_module(bool wait, const char *fmt, ...)
        return ret;
 }
 EXPORT_SYMBOL(__request_module);
+
+/*
+ * If modprobe needs a service that is in a module, we get a recursive
+ * loop.  Limit the number of running kmod threads to max_threads/2 or
+ * CONFIG_MAX_KMOD_CONCURRENT, whichever is the smaller.  A cleaner method
+ * would be to run the parents of this process, counting how many times
+ * kmod was invoked.  That would mean accessing the internals of the
+ * process tables to get the command line, proc_pid_cmdline is static
+ * and it is not worth changing the proc code just to handle this case.
+ *
+ * "trace the ppid" is simple, but will fail if someone's
+ * parent exits.  I think this is as good as it gets.
+ *

kernel/kmod.c int __request_module(bool wait, const char *fmt, ...) · line 195

Quoted 6 lines

kernel/kmod.c +6 -0

+ * You can override with with a kernel parameter, for instance to allow
+ * 4096 concurrent modprobe instances:
+ *
+ *     kmod.max_modprobes=4096
+ */
+void __init init_kmod_umh(void)

What does umh mean?

We currently statically limit the number of modprobe threads which we allow to run concurrently to 50. As per Keith Owens, this was a completely arbitrary value, and it was set in the 2.3.38 days [0] over 16 years ago in year 2000.

Although we haven't yet hit our lower limits, experimentation [1] shows that when and if we hit this limit in the worst case, will be fatal -- consider get_fs_type() failures upon mount on a system which has many partitions, some of which might even be with the same filesystem. Its best to be prudent and increase and set this value to something more sensible which ensures we're far from hitting the limit and also allows default build/user run time override.

The worst case is fatal given that once a module fails to load there is a period of time during which subsequent request for the same module will fail, so in the case of partitions its not just one request that could fail, but whole series of partitions. This later issue of a module request failure domino effect can be addressed later, but increasing the limit to something more meaninful should at least give us enough cushion to avoid this for a while.

Set this value up with a bit more meaninful modern limits:

Bump this up to 64 max for small systems (CONFIG_BASE_SMALL) Bump this up to 128 max for larger systems (!CONFIG_BASE_SMALL)

init/Kconfig +0 -0

index 271692a352f1..da2c25746937 100644

init/main.c +1 -0

index 8161208d4ece..1fa441aa32c6 100644
@@ -638,6 +638,7 @@ asmlinkage __visible void __init start_kernel(void)
 	thread_stack_cache_init();
 	cred_init();
 	fork_init();
+	init_kmod_umh();
 	proc_caches_init();
 	buffer_init();
 	key_init();

kernel/kmod.c +13 -0

index 0277d1216f80..cb6f7ca7b8a5 100644
@@ -186,6 +174,31 @@ int __request_module(bool wait, const char *fmt, ...)
 	return ret;
 }
 EXPORT_SYMBOL(__request_module);
+
+/*
+ * If modprobe needs a service that is in a module, we get a recursive
+ * loop.  Limit the number of running kmod threads to max_threads/2 or
+ * CONFIG_MAX_KMOD_CONCURRENT, whichever is the smaller.  A cleaner method
+ * would be to run the parents of this process, counting how many times
+ * kmod was invoked.  That would mean accessing the internals of the
+ * process tables to get the command line, proc_pid_cmdline is static
+ * and it is not worth changing the proc code just to handle this case.
+ *
+ * "trace the ppid" is simple, but will fail if someone's
+ * parent exits.  I think this is as good as it gets.
+ *

Quoted 6 lines · Luis R. Rodriguez

init/Kconfig +0 -0

index 271692a352f1..da2c25746937 100644
@@ -2111,6 +2111,29 @@ config TRIM_UNUSED_KSYMS
 

init/Kconfig config TRIM_UNUSED_KSYMS · line 2116

Quoted 5 lines

init/Kconfig +3 -0

 	  If unsure, or if you need to build out-of-tree modules, say N.
 
+config MAX_KMOD_CONCURRENT
+	int "Max allowed concurrent request_module() calls (6=>64, 10=>1024)"
+	range 0 14

Would not too small range break loading module dependencies?

I am not sure how it is implemented but it might require having some more module loads in progress.

I would give 6 as minimum. Nobody has troubles with the current limit.

init/Kconfig config TRIM_UNUSED_KSYMS · line 2118

Quoted 2 lines

init/Kconfig +2 -0

+	default 6 if !BASE_SMALL
+	default 7 if BASE_SMALL

Aren't the conditions inversed?

init/Kconfig config TRIM_UNUSED_KSYMS · line 2123

Quoted 5 lines

init/Kconfig +5 -0

+void __init init_kmod_umh(void)
+{
+	if (!max_modprobes)
+		max_modprobes = min(max_threads/2,
+				    2 << CONFIG_MAX_KMOD_CONCURRENT);

This should be

	1 << CONFIG_MAX_KMOD_CONCURRENT);

1 << 1 = 2;

Note that this calculation is mentioned also some comments and documentation.

Quoted 7 lines · Petr Mladek

Quoted 6 lines · Luis R. Rodriguez

init/Kconfig +0 -0

index 271692a352f1..da2c25746937 100644
@@ -2111,6 +2111,29 @@ config TRIM_UNUSED_KSYMS
 

Quoted 7 lines

init/Kconfig config TRIM_UNUSED_KSYMS · line 2116

Quoted 5 lines

init/Kconfig +3 -0

 	  If unsure, or if you need to build out-of-tree modules, say N.
 
+config MAX_KMOD_CONCURRENT
+	int "Max allowed concurrent request_module() calls (6=>64, 10=>1024)"
+	range 0 14

Would not too small range break loading module dependencies?

No, dependencies are resolved by depmod, so userspace looks at the list and just finit_module() the depenencies, skipping kmod. So the limit is really only for kernel acting like a boss.

Quoted 2 lines

I am not sure how it is implemented but it might require having some more module loads in progress.

Dependencies should be OK, a more serious concern with dependencies is the aggregate memory it takes to load all dep modules for one required module since finit_module() ends up allocating the struct module to copy over data from userspace.

From some initial tests it would seem this doesn't help much but it does

kmod_concurrent is used as an atomic counter for enabling the allowed limit of modprobe calls, provide wrappers for it to enable this to be expanded on more easily. This will be done later.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/kmod.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

kernel/kmod.c +6 -0

index cb6f7ca7b8a5..049d7eabda38 100644
@@ -44,6 +44,9 @@
 #include <trace/events/module.h>
 
 extern int max_threads;
+
+static atomic_t kmod_concurrent = ATOMIC_INIT(0);
+
 unsigned int max_modprobes;
 module_param(max_modprobes, uint, 0644);
 MODULE_PARM_DESC(max_modprobes, "Max number of allowed concurrent modprobes");

@@ -108,6 +111,20 @@ static int call_modprobe(char *module_name, int wait)
        return -ENOMEM;
 }
 
+static int kmod_umh_threads_get(void)
+{
+       atomic_inc(&kmod_concurrent);

kernel/kmod.c +5 -7

+
 /**
  * __request_module - try to load a kernel module
  * @wait: wait (or not) for the operation to complete

@@ -129,7 +146,6 @@ int __request_module(bool wait, const char *fmt, ...)
        va_list args;
        char module_name[MODULE_NAME_LEN];
        int ret;
-       static atomic_t kmod_concurrent = ATOMIC_INIT(0);
        static int kmod_loop_msg;
 
        /*

@@ -153,8 +169,8 @@ int __request_module(bool wait, const char *fmt, ...)
        if (ret)
                return ret;
 
-       atomic_inc(&kmod_concurrent);
-       if (atomic_read(&kmod_concurrent) > max_modprobes) {
+       ret = kmod_umh_threads_get();
+       if (ret) {
                /* We may be blaming an innocent here, but unlikely */
                if (kmod_loop_msg < 5) {
                        printk(KERN_ERR

@@ -162,15 +178,14 @@ int __request_module(bool wait, const char *fmt, ...)
                               module_name);
                        kmod_loop_msg++;
                }
-               atomic_dec(&kmod_concurrent);
-               return -ENOMEM;
+               return ret;
        }
 
        trace_module_request(module_name, wait, _RET_IP_);
 
        ret = call_modprobe(module_name, wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC);
 
-       atomic_dec(&kmod_concurrent);
+       kmod_umh_threads_put();
        return ret;
 }
 EXPORT_SYMBOL(__request_module);
--

2.10.1

Quoted 28 lines · Luis R. Rodriguez

kmod_concurrent is used as an atomic counter for enabling the allowed limit of modprobe calls, provide wrappers for it to enable this to be expanded on more easily. This will be done later.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/kmod.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

kernel/kmod.c +3 -0

index cb6f7ca7b8a5..049d7eabda38 100644
@@ -44,6 +44,9 @@
 #include <trace/events/module.h>
 
 extern int max_threads;
+
+static atomic_t kmod_concurrent = ATOMIC_INIT(0);
+
 unsigned int max_modprobes;
 module_param(max_modprobes, uint, 0644);
 MODULE_PARM_DESC(max_modprobes, "Max number of allowed concurrent modprobes");

@@ -108,6 +111,20 @@ static int call_modprobe(char *module_name, int wait)
        return -ENOMEM;
 }
 

kernel/kmod.c static int call_modprobe(char *module_name, int wait) · line 126

Quoted 13 lines

kernel/kmod.c +13 -0

+static int kmod_umh_threads_get(void)
+{
+       atomic_inc(&kmod_concurrent);
+       if (atomic_read(&kmod_concurrent) < max_modprobes)
+               return 0;
+       atomic_dec(&kmod_concurrent);
+       return -ENOMEM;
+}
+
+static void kmod_umh_threads_put(void)
+{
+       atomic_dec(&kmod_concurrent);
+}

Can you use a kref here instead? We're trying to kill raw use of atomic_t for reference counting...

Quoted 19 lines · Kees Cook

Quoted 18 lines · Luis R. Rodriguez

kmod_concurrent is used as an atomic counter for enabling the allowed limit of modprobe calls, provide wrappers for it to enable this to be expanded on more easily. This will be done later.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/kmod.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

kernel/kmod.c +0 -0

index cb6f7ca7b8a5..049d7eabda38 100644
@@ -108,6 +111,20 @@ static int call_modprobe(char *module_name, int wait)
        return -ENOMEM;
 }
 

Quoted 3 lines

kernel/kmod.c static int call_modprobe(char *module_name, int wait) · line 116

Quoted 3 lines

kernel/kmod.c +3 -0

+static int kmod_umh_threads_get(void)
+{
+       atomic_inc(&kmod_concurrent);

Quoted 5 lines

Quoted 5 lines

kernel/kmod.c +5 -0

+       if (atomic_read(&kmod_concurrent) < max_modprobes)
+               return 0;
+       atomic_dec(&kmod_concurrent);
+       return -ENOMEM;
+}

Quoted 8 lines

kernel/kmod.c static int call_modprobe(char *module_name, int wait) · line 126

Quoted 5 lines

kernel/kmod.c +5 -0

+
+static void kmod_umh_threads_put(void)
+{
+       atomic_dec(&kmod_concurrent);
+}

Can you use a kref here instead? We're trying to kill raw use of atomic_t for reference counting...

That's a much broader functional change than I was looking for, but I am up for it. Can you describe the benefit of using kref you expect or why this is an ongoing crusade? Since its a larger functional change how about doing this change later, and we can test impact with the tress test driver. In theory if there are benefits can't we add a test case to prove the gains?

Quoted 20 lines · Luis R. Rodriguez

Quoted 19 lines · Kees Cook

Quoted 18 lines · Luis R. Rodriguez

kmod_concurrent is used as an atomic counter for enabling the allowed limit of modprobe calls, provide wrappers for it to enable this to be expanded on more easily. This will be done later.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/kmod.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

kernel/kmod.c +0 -0

index cb6f7ca7b8a5..049d7eabda38 100644
@@ -108,6 +111,20 @@ static int call_modprobe(char *module_name, int wait)
        return -ENOMEM;
 }
 

Quoted 3 lines

Quoted 3 lines

kernel/kmod.c static int call_modprobe(char *module_name, int wait) · line 116

Quoted 3 lines

kernel/kmod.c +3 -0

+static int kmod_umh_threads_get(void)
+{
+       atomic_inc(&kmod_concurrent);

This approach might actually cause false failures. If we are on the limit and more processes do this increment in parallel, it makes the number bigger that it should be.

Quoted 14 lines

Quoted 5 lines

Quoted 5 lines

kernel/kmod.c +5 -0

+       if (atomic_read(&kmod_concurrent) < max_modprobes)
+               return 0;
+       atomic_dec(&kmod_concurrent);
+       return -ENOMEM;
+}

Quoted 8 lines

kernel/kmod.c static int call_modprobe(char *module_name, int wait) · line 126

Quoted 5 lines

kernel/kmod.c +5 -0

+
+static void kmod_umh_threads_put(void)
+{
+       atomic_dec(&kmod_concurrent);
+}

Can you use a kref here instead? We're trying to kill raw use of atomic_t for reference counting...

Quoted 5 lines

That's a much broader functional change than I was looking for, but I am up for it. Can you describe the benefit of using kref you expect or why this is an ongoing crusade? Since its a larger functional change how about doing this change later, and we can test impact with the tress test driver. In theory if there are benefits can't we add a test case to prove the gains?

Kees probably refers to the kref improvements that Peter Zijlstra is working on, see
https://lkml.kernel.org/r/[email protected]

The advantage is that the new refcount API handles over and underflow.

Another advantage is that it increments/decrements the value only when it is safe. It uses cmpxchg to make sure that the checks are valid.

Quoted 21 lines · Luis R. Rodriguez

Quoted 20 lines · Kees Cook

Quoted 19 lines · Luis R. Rodriguez

Quoted 18 lines

kmod_concurrent is used as an atomic counter for enabling the allowed limit of modprobe calls, provide wrappers for it to enable this to be expanded on more easily. This will be done later.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/kmod.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

kernel/kmod.c +0 -0

index cb6f7ca7b8a5..049d7eabda38 100644
@@ -108,6 +111,20 @@ static int call_modprobe(char *module_name, int wait)
        return -ENOMEM;
 }
 

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

kernel/kmod.c static int call_modprobe(char *module_name, int wait) · line 116

Quoted 3 lines

kernel/kmod.c +3 -0

+static int kmod_umh_threads_get(void)
+{
+       atomic_inc(&kmod_concurrent);

This approach might actually cause false failures. If we are on the limit and more processes do this increment in parallel, it makes the number bigger that it should be.

This approach is *exactly* what the existing code does :P I just provided wrappers. I agree with the old approach though, reason is it acts as a lock in for the bump.

+++ Luis R. Rodriguez [16/12/16 09:05 +0100]:

Quoted 30 lines · Petr Mladek

Quoted 21 lines · Luis R. Rodriguez

Quoted 20 lines · Kees Cook

Quoted 19 lines · Luis R. Rodriguez

Quoted 18 lines

kmod_concurrent is used as an atomic counter for enabling the allowed limit of modprobe calls, provide wrappers for it to enable this to be expanded on more easily. This will be done later.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/kmod.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

kernel/kmod.c +0 -0

index cb6f7ca7b8a5..049d7eabda38 100644
@@ -108,6 +111,20 @@ static int call_modprobe(char *module_name, int wait)
        return -ENOMEM;
 }
 

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

kernel/kmod.c static int call_modprobe(char *module_name, int wait) · line 116

Quoted 3 lines

kernel/kmod.c +3 -0

+static int kmod_umh_threads_get(void)
+{
+       atomic_inc(&kmod_concurrent);

This approach might actually cause false failures. If we are on the limit and more processes do this increment in parallel, it makes the number bigger that it should be.

Quoted 3 lines

This approach is *exactly* what the existing code does :P I just provided wrappers. I agree with the old approach though, reason is it acts as a lock in for the bump.

I think what Petr meant was that we could run into false failures when multiple atomic increments happen between the first increment and the subsequent atomic_read.

Say max_modprobes is 64 -

atomic_inc(&kmod_concurrent); // thread 1: kmod_concurrent is 63
atomic_inc(&kmod_concurrent); // thread 2: kmod_concurrent is 64
atomic_inc(&kmod_concurrent); // thread 3: kmod_concurrent is 65
if (atomic_read(&kmod_concurrent) < max_modprobes) // if all threads read 65 here, then all will error out
return 0; // when the first two should have succeeded (false failures)
atomic_dec(&kmod_concurrent);
return -ENOMEM;

But yeah, I think this issue was already in the existing kmod code..