Re: [RFC 01/10] kmod: add test driver to stress test the module loader

[Luis R. Rodriguez]

On Thu, Dec 08, 2016 at 12:24:35PM -0800, Kees Cook wrote:
> On Thu, Dec 8, 2016 at 10:47 AM, Luis R. Rodriguez <mcgrof@xxxxxxxxxx> wrote:
> > The kmod.sh script uses the above constructs to build differnt test cases.
> 
> Typo: different

Fixed.

> > 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 :)

> > 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.
> 
> Are the buffers not released until after all the dependent modules are
> loaded? I thought it would load one by one?

kmod.c allows up to kmod_concurrent concurrent requests out to userspace,
how it handles this is up to userspace, but note that prior to the knobs
exposed in this patch set userspace neither knew what kmod_concurrent
was nor how many concurrent threads are active at any point in time.

> > Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
> 
> This is a great selftest, thanks for working on it!

My pleasure.

> Notes below...
> 
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 7446097f72bd..6cad548e0682 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -1994,6 +1994,31 @@ config BUG_ON_DATA_CORRUPTION
> >
> >           If unsure, say N.
> >
> > +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?

Well yes and no, yes because its the defaults built-in. No, because as you note
we can alter the defaults in userspace. Without the alternatives being set the
driver will not really work at all though. Here is an example where Arnd's
kconfig "suggests" for kconfig could come in handy. Until we have that I think
I'd prefer to keep it this way.

> > diff --git a/lib/test_kmod.c b/lib/test_kmod.c
> > new file mode 100644
> > index 000000000000..63fded83b9b6
> > --- /dev/null
> > +++ b/lib/test_kmod.c
> > @@ -0,0 +1,1248 @@

> > +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

Fixed.

> > +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");
> > +
> > +       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...

Fixed. If the caller is sysfs_kf_seq_show() then max is PAGE_SIZE, will
use that as the limit to start with.

> > +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;
> > +
> > +       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.

Sure.

> > +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;
> > +
> > +       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).

Sure, I'm starting to think a lot of test boiler plate for setup and show of
config stuff could be shared. We can consider this more once we have a few more
test drivers like this. I have 3 total now in the pipeline.

> > +
> > +       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;
> > +
> > +       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.)

Sure, it can be:

if (copied <= 0 || copied != strlen(test_driver)) {

That way its both a negative check and also that something
non-empty was passed.

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

And come to think of it, these should return -ENOMEM;
> > +       }
> > +
> > +       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;
> > +       }
> > +
> > +       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...

They are now shared.

> > +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.

Fixed.

> > +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;
> > +       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?

Fixed.

> > +static int test_dev_config_update_int(struct kmod_test_device *test_dev,
> > +                                     const char *buf, size_t size,
> > +                                     int *config)
> > +{
> > +       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?

Fixed.

> > +/*
> > + * 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;
> > +       char *name;
> > +
> > +       if (!base)
> > +               return NULL;
> > +       if (strlen(base) > 30)
> > +               return NULL;
> 
> why?

It was an arbitrary limit, will use PAGE_SIZE. But I'll just remove the
entire routine (see below).
> 
> > +       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).

Heh, yeah, true, nuked.

> > diff --git a/tools/testing/selftests/kmod/config b/tools/testing/selftests/kmod/config
> > new file mode 100644
> > index 000000000000..259f4fd6b5e2
> > --- /dev/null
> > +++ b/tools/testing/selftests/kmod/config
> > @@ -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
> > diff --git a/tools/testing/selftests/kmod/kmod.sh b/tools/testing/selftests/kmod/kmod.sh
> > new file mode 100755
> > index 000000000000..9ea1864d8bae
> > --- /dev/null
> > +++ b/tools/testing/selftests/kmod/kmod.sh
> > @@ -0,0 +1,449 @@
> > +#!/bin/bash
> > +#

<-- snip -->

> > +# 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?

Whoops, yeah sorry 4 GiB only.

> > +# 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"
> > +}
> > +
> > +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...)

Addressed.

> > +function load_req_mod()
> > +{
> > +       if [ ! -d $DIR ]; then
> > +               # Alanis: "Oh isn't it ironic?"
> > +               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?

mcgrof@ergon ~/linux-next (git::(no branch, rebasing 20161213-kmod-test-driver))$ find tools/testing/selftests/ -name config
tools/testing/selftests/static_keys/config
tools/testing/selftests/cpu-hotplug/config
tools/testing/selftests/ipc/config
tools/testing/selftests/mount/config
tools/testing/selftests/zram/config
tools/testing/selftests/seccomp/config
tools/testing/selftests/memory-hotplug/config
tools/testing/selftests/vm/config
tools/testing/selftests/ftrace/config
tools/testing/selftests/pstore/config
tools/testing/selftests/firmware/config
tools/testing/selftests/net/config
tools/testing/selftests/bpf/config
tools/testing/selftests/user/config
tools/testing/selftests/kmod/config

Seems like a hipster trend.

> > +# 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
> > +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!

  Luis