diff mbox series

[v4,4/4] selftest: Taint kernel when test module loaded

Message ID 20220701084744.3002019-4-davidgow@google.com (mailing list archive)
State New, archived
Headers show
Series [v4,1/4] panic: Taint kernel if tests are run | expand

Commit Message

David Gow July 1, 2022, 8:47 a.m. UTC 
Make any kselftest test module (using the kselftest_module framework)
taint the kernel with TAINT_TEST on module load.

Note that several selftests use kernel modules which are not based on
the kselftest_module framework, and so will not automatically taint the
kernel.

This can be done in two ways:
- Moving the module to the tools/testing directory. All modules under
  this directory will taint the kernel.
- Adding the 'test' module property with:
  MODULE_INFO(test, "Y")

Similarly, selftests which do not load modules into the kernel generally
should not taint the kernel (or possibly should only do so on failure),
as it's assumed that testing from user-space should be safe. Regardless,
they can write to /proc/sys/kernel/tainted if required.

Signed-off-by: David Gow <davidgow@google.com>
---

This still only covers a subset of selftest modules, but combined with
the modpost check for the tools/testing path, it should catch many
future tests. Others can be moved, adapted to use this framework, or
have MODULE_INFO(test, "Y") added. (Alas, I don't have the time to hunt
down all of the tests which don't do this at the moment.

No changes since v3:
https://lore.kernel.org/lkml/20220513083212.3537869-3-davidgow@google.com/

---
 tools/testing/selftests/kselftest_module.h | 2 ++
 1 file changed, 2 insertions(+)

Comments

kernel test robot July 1, 2022, 5:16 p.m. UTC | #1 
Hi David,

I love your patch! Yet something to improve:

[auto build test ERROR on masahiroy-kbuild/for-next]
[also build test ERROR on shuah-kselftest/next linus/master v5.19-rc4 next-20220701]
[cannot apply to mcgrof/modules-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Gow/panic-Taint-kernel-if-tests-are-run/20220701-164843
base:   https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git for-next
config: arm-randconfig-r024-20220629 (https://download.01.org/0day-ci/archive/20220702/202207020131.L5kV3eDf-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project a9119143a2d1f4d0d0bc1fe0d819e5351b4e0deb)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/intel-lab-lkp/linux/commit/42b6461d6cca4baeeeed474b1400e203057c2b9b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review David-Gow/panic-Taint-kernel-if-tests-are-run/20220701-164843
        git checkout 42b6461d6cca4baeeeed474b1400e203057c2b9b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash lib/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   lib/test_printf.c:157:52: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
           test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
                                  ~~~~                       ^
                                  %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:157:55: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
           test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
                                       ~~~~                     ^
                                       %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:157:58: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
           test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
                                            ~~~~                   ^~~
                                            %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:157:63: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
           test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
                                                 ~~~~                   ^~~
                                                 %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:157:68: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
           test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
                                                      ~~~~                   ^~
                                                      %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:158:52: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
           test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
                                  ~~~~                       ^
                                  %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:158:55: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
           test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
                                       ~~~~                     ^
                                       %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:158:58: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
           test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
                                            ~~~~                   ^~~
                                            %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:158:63: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
           test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
                                                 ~~~~                   ^~~
                                                 %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:158:68: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
           test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
                                                      ~~~~                   ^~
                                                      %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:159:41: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
           test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
                                     ~~~          ^~~~
                                     %o
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:159:47: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
           test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
                                        ~~~             ^~~~
                                        %o
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:159:53: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
           test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
                                           ~~~~               ^~~~~~
                                           %#o
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
>> lib/test_printf.c:801:1: error: use of undeclared identifier 'TAINT_KUNIT'
   KSTM_MODULE_LOADERS(test_printf);
   ^
   lib/../tools/testing/selftests/kselftest_module.h:45:12: note: expanded from macro 'KSTM_MODULE_LOADERS'
           add_taint(TAINT_KUNIT, LOCKDEP_STILL_OK);       \
                     ^
   13 warnings and 1 error generated.
--
>> lib/test_scanf.c:811:1: error: use of undeclared identifier 'TAINT_KUNIT'
   KSTM_MODULE_LOADERS(test_scanf);
   ^
   lib/../tools/testing/selftests/kselftest_module.h:45:12: note: expanded from macro 'KSTM_MODULE_LOADERS'
           add_taint(TAINT_KUNIT, LOCKDEP_STILL_OK);       \
                     ^
   1 error generated.
--
>> lib/test_bitmap.c:889:1: error: use of undeclared identifier 'TAINT_KUNIT'
   KSTM_MODULE_LOADERS(test_bitmap);
   ^
   lib/../tools/testing/selftests/kselftest_module.h:45:12: note: expanded from macro 'KSTM_MODULE_LOADERS'
           add_taint(TAINT_KUNIT, LOCKDEP_STILL_OK);       \
                     ^
   1 error generated.


vim +/TAINT_KUNIT +801 lib/test_printf.c

707cc7280f452a1 Rasmus Villemoes 2015-11-06  800  
6b1a4d5b1a26ae8 Tobin C. Harding 2019-04-05 @801  KSTM_MODULE_LOADERS(test_printf);
kernel test robot July 1, 2022, 5:37 p.m. UTC | #2 
Hi David,

I love your patch! Yet something to improve:

[auto build test ERROR on masahiroy-kbuild/for-next]
[also build test ERROR on shuah-kselftest/next linus/master v5.19-rc4 next-20220701]
[cannot apply to mcgrof/modules-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Gow/panic-Taint-kernel-if-tests-are-run/20220701-164843
base:   https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git for-next
config: alpha-randconfig-r006-20220629 (https://download.01.org/0day-ci/archive/20220702/202207020132.SKDpQP9D-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/42b6461d6cca4baeeeed474b1400e203057c2b9b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review David-Gow/panic-Taint-kernel-if-tests-are-run/20220701-164843
        git checkout 42b6461d6cca4baeeeed474b1400e203057c2b9b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=alpha SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from lib/test_printf.c:27:
   lib/test_printf.c: In function 'test_printf_init':
>> lib/../tools/testing/selftests/kselftest_module.h:45:19: error: 'TAINT_KUNIT' undeclared (first use in this function)
      45 |         add_taint(TAINT_KUNIT, LOCKDEP_STILL_OK);       \
         |                   ^~~~~~~~~~~
   lib/test_printf.c:801:1: note: in expansion of macro 'KSTM_MODULE_LOADERS'
     801 | KSTM_MODULE_LOADERS(test_printf);
         | ^~~~~~~~~~~~~~~~~~~
   lib/../tools/testing/selftests/kselftest_module.h:45:19: note: each undeclared identifier is reported only once for each function it appears in
      45 |         add_taint(TAINT_KUNIT, LOCKDEP_STILL_OK);       \
         |                   ^~~~~~~~~~~
   lib/test_printf.c:801:1: note: in expansion of macro 'KSTM_MODULE_LOADERS'
     801 | KSTM_MODULE_LOADERS(test_printf);
         | ^~~~~~~~~~~~~~~~~~~


vim +/TAINT_KUNIT +45 lib/../tools/testing/selftests/kselftest_module.h

    40	
    41	#define KSTM_MODULE_LOADERS(__module)			\
    42	static int __init __module##_init(void)			\
    43	{							\
    44		pr_info("loaded.\n");				\
  > 45		add_taint(TAINT_KUNIT, LOCKDEP_STILL_OK);	\
    46		selftest();					\
    47		return kstm_report(total_tests, failed_tests, skipped_tests);	\
    48	}							\
    49	static void __exit __module##_exit(void)		\
    50	{							\
    51		pr_info("unloaded.\n");				\
    52	}							\
    53	module_init(__module##_init);				\
    54	module_exit(__module##_exit)
    55
Luis Chamberlain July 1, 2022, 10:33 p.m. UTC | #3 
On Fri, Jul 01, 2022 at 04:47:44PM +0800, David Gow wrote:
> Make any kselftest test module (using the kselftest_module framework)
> taint the kernel with TAINT_TEST on module load.
> 
> Note that several selftests use kernel modules which are not based on
> the kselftest_module framework, and so will not automatically taint the
> kernel.
> 
> This can be done in two ways:
> - Moving the module to the tools/testing directory. All modules under
>   this directory will taint the kernel.
> - Adding the 'test' module property with:
>   MODULE_INFO(test, "Y")

This just needs to be documented somewhere other than a commit log.
Otherwise I am not sure how we can be sure it will catch on.

> Similarly, selftests which do not load modules into the kernel generally
> should not taint the kernel (or possibly should only do so on failure),
> as it's assumed that testing from user-space should be safe. Regardless,
> they can write to /proc/sys/kernel/tainted if required.
> 
> Signed-off-by: David Gow <davidgow@google.com>

Looks good otherwise!

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

Do we want this to go through selftest / kunit / modules tree?
Happy for it to through any. I can't predict a conflict.

  Luis
David Gow July 2, 2022, 4:06 a.m. UTC | #4 
On Sat, Jul 2, 2022 at 6:33 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Fri, Jul 01, 2022 at 04:47:44PM +0800, David Gow wrote:
> > Make any kselftest test module (using the kselftest_module framework)
> > taint the kernel with TAINT_TEST on module load.
> >
> > Note that several selftests use kernel modules which are not based on
> > the kselftest_module framework, and so will not automatically taint the
> > kernel.
> >
> > This can be done in two ways:
> > - Moving the module to the tools/testing directory. All modules under
> >   this directory will taint the kernel.
> > - Adding the 'test' module property with:
> >   MODULE_INFO(test, "Y")
>
> This just needs to be documented somewhere other than a commit log.
> Otherwise I am not sure how we can be sure it will catch on.

I've updated the kselftest documentation for v5.

> > Similarly, selftests which do not load modules into the kernel generally
> > should not taint the kernel (or possibly should only do so on failure),
> > as it's assumed that testing from user-space should be safe. Regardless,
> > they can write to /proc/sys/kernel/tainted if required.
> >
> > Signed-off-by: David Gow <davidgow@google.com>
>
> Looks good otherwise!
>
> Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>
>
> Do we want this to go through selftest / kunit / modules tree?
> Happy for it to through any. I can't predict a conflict.

I don't mind which tree it goes through either -- I'm not aware of
anything which would depend on it. I do have it on the list of things
pending for the KUnit tree, but it's much less KUnit-specific now
compared to v1. Regardless, I'll leave in the KUnit to-do list, and
we'll pick it up if no-one else particularly wants to.

Cheers,
-- David
David Gow July 2, 2022, 5:15 a.m. UTC | #5 
On Sat, Jul 2, 2022 at 12:06 PM David Gow <davidgow@google.com> wrote:
>
> On Sat, Jul 2, 2022 at 6:33 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > On Fri, Jul 01, 2022 at 04:47:44PM +0800, David Gow wrote:
> > > Make any kselftest test module (using the kselftest_module framework)
> > > taint the kernel with TAINT_TEST on module load.
> > >
> > > Note that several selftests use kernel modules which are not based on
> > > the kselftest_module framework, and so will not automatically taint the
> > > kernel.
> > >
> > > This can be done in two ways:
> > > - Moving the module to the tools/testing directory. All modules under
> > >   this directory will taint the kernel.
> > > - Adding the 'test' module property with:
> > >   MODULE_INFO(test, "Y")
> >
> > This just needs to be documented somewhere other than a commit log.
> > Otherwise I am not sure how we can be sure it will catch on.
>
> I've updated the kselftest documentation for v5.
>
> > > Similarly, selftests which do not load modules into the kernel generally
> > > should not taint the kernel (or possibly should only do so on failure),
> > > as it's assumed that testing from user-space should be safe. Regardless,
> > > they can write to /proc/sys/kernel/tainted if required.
> > >
> > > Signed-off-by: David Gow <davidgow@google.com>
> >
> > Looks good otherwise!
> >
> > Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>
> >
> > Do we want this to go through selftest / kunit / modules tree?
> > Happy for it to through any. I can't predict a conflict.
>
> I don't mind which tree it goes through either -- I'm not aware of
> anything which would depend on it. I do have it on the list of things
> pending for the KUnit tree, but it's much less KUnit-specific now
> compared to v1. Regardless, I'll leave in the KUnit to-do list, and
> we'll pick it up if no-one else particularly wants to.
>

FYI: It looks like patches 1 & 3 are already in the kunit tree, so it
makes sense to take the rest of them, too:
https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/log/?h=kunit

Cheers,
-- David
diff mbox series

Patch

diff --git a/tools/testing/selftests/kselftest_module.h b/tools/testing/selftests/kselftest_module.h
index e2ea41de3f35..226e616b82e0 100644
--- a/tools/testing/selftests/kselftest_module.h
+++ b/tools/testing/selftests/kselftest_module.h
@@ -3,6 +3,7 @@ 
 #define __KSELFTEST_MODULE_H
 
 #include <linux/module.h>
+#include <linux/panic.h>
 
 /*
  * Test framework for writing test modules to be loaded by kselftest.
@@ -41,6 +42,7 @@  static inline int kstm_report(unsigned int total_tests, unsigned int failed_test
 static int __init __module##_init(void)			\
 {							\
 	pr_info("loaded.\n");				\
+	add_taint(TAINT_KUNIT, LOCKDEP_STILL_OK);	\
 	selftest();					\
 	return kstm_report(total_tests, failed_tests, skipped_tests);	\
 }							\