| Message ID | 20220702040959.3232874-2-davidgow@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v5,1/4] panic: Taint kernel if tests are run | expand |
On Sat, Jul 2, 2022 at 12:10 PM David Gow <davidgow@google.com> wrote: > > Taint the kernel with TAINT_TEST whenever a test module loads, by adding > a new "TEST" module property, and setting it for all modules in the > tools/testing directory. This property can also be set manually, for > tests which live outside the tools/testing directory with: > MODULE_INFO(test, "Y"); > > Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> > Signed-off-by: David Gow <davidgow@google.com> > --- I forgot the changelogs here. The only significant difference from v4 is the change from pr_warn() to pr_warn_once(). Changes since v4: https://lore.kernel.org/linux-kselftest/20220701084744.3002019-2-davidgow@google.com/ - Use pr_warn_once() to only log a warning the first time a module taints the kernel with TAINT_TEST - Loading lots of test modules is a common usecase, and this would otherwise spam the logs too much. - Thanks Luis. - Remove a superfluous newline (Thanks Greg) - Add Luis' Reviewed-by tag. This patch was new in v4 of the series.
On Sat 2022-07-02 12:09 +0800, David Gow wrote: > Taint the kernel with TAINT_TEST whenever a test module loads, by adding > a new "TEST" module property, and setting it for all modules in the > tools/testing directory. This property can also be set manually, for > tests which live outside the tools/testing directory with: > MODULE_INFO(test, "Y"); > > Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> > Signed-off-by: David Gow <davidgow@google.com> > --- > kernel/module/main.c | 7 +++++++ > scripts/mod/modpost.c | 3 +++ > 2 files changed, 10 insertions(+) > > diff --git a/kernel/module/main.c b/kernel/module/main.c > index fed58d30725d..730503561eb0 100644 > --- a/kernel/module/main.c > +++ b/kernel/module/main.c > @@ -1988,6 +1988,13 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags) > /* Set up license info based on the info section */ > set_license(mod, get_modinfo(info, "license")); > > + if (!get_modinfo(info, "test")) { > + if (!test_taint(TAINT_TEST)) > + pr_warn_once("%s: loading test module taints kernel.\n", > + mod->name); > + add_taint_module(mod, TAINT_TEST, LOCKDEP_STILL_OK); > + } > + > return 0; > } > > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > index 29d5a841e215..5937212b4433 100644 > --- a/scripts/mod/modpost.c > +++ b/scripts/mod/modpost.c > @@ -2191,6 +2191,9 @@ static void add_header(struct buffer *b, struct module *mod) > > if (strstarts(mod->name, "drivers/staging")) > buf_printf(b, "\nMODULE_INFO(staging, \"Y\");\n"); > + > + if (strstarts(mod->name, "tools/testing")) > + buf_printf(b, "\nMODULE_INFO(test, \"Y\");\n"); > } > > static void add_exported_symbols(struct buffer *buf, struct module *mod) > -- > 2.37.0.rc0.161.g10f37bed90-goog > Hi David, I think this looks good: Reviewed-by: Aaron Tomlin <atomlin@redhat.com> Kind regards,
On Sat, Jul 2, 2022 at 12:10 AM 'David Gow' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > Taint the kernel with TAINT_TEST whenever a test module loads, by adding > a new "TEST" module property, and setting it for all modules in the > tools/testing directory. This property can also be set manually, for > tests which live outside the tools/testing directory with: > MODULE_INFO(test, "Y"); > > Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> > Signed-off-by: David Gow <davidgow@google.com> Acked-by: Brendan Higgins <brendanhiggins@google.com>
On Sat, Jul 02, 2022 at 12:09:57PM +0800, David Gow wrote: > Taint the kernel with TAINT_TEST whenever a test module loads, by adding > a new "TEST" module property, and setting it for all modules in the > tools/testing directory. This property can also be set manually, for > tests which live outside the tools/testing directory with: > MODULE_INFO(test, "Y"); > > Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> > Signed-off-by: David Gow <davidgow@google.com> > --- > kernel/module/main.c | 7 +++++++ > scripts/mod/modpost.c | 3 +++ > 2 files changed, 10 insertions(+) > > diff --git a/kernel/module/main.c b/kernel/module/main.c > index fed58d30725d..730503561eb0 100644 > --- a/kernel/module/main.c > +++ b/kernel/module/main.c > @@ -1988,6 +1988,13 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags) > /* Set up license info based on the info section */ > set_license(mod, get_modinfo(info, "license")); > > + if (!get_modinfo(info, "test")) { > + if (!test_taint(TAINT_TEST)) > + pr_warn_once("%s: loading test module taints kernel.\n", > + mod->name); > + add_taint_module(mod, TAINT_TEST, LOCKDEP_STILL_OK); > + } > + > return 0; > } > > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > index 29d5a841e215..5937212b4433 100644 > --- a/scripts/mod/modpost.c > +++ b/scripts/mod/modpost.c > @@ -2191,6 +2191,9 @@ static void add_header(struct buffer *b, struct module *mod) > > if (strstarts(mod->name, "drivers/staging")) > buf_printf(b, "\nMODULE_INFO(staging, \"Y\");\n"); > + > + if (strstarts(mod->name, "tools/testing")) > + buf_printf(b, "\nMODULE_INFO(test, \"Y\");\n"); > } > > static void add_exported_symbols(struct buffer *buf, struct module *mod) > -- > 2.37.0.rc0.161.g10f37bed90-goog > > Hi David, This change has landed in linux-next as commit e20729ede7ed ("module: panic: taint the kernel when selftest modules load") and on all of my test machines, I see this new message printed, even though as far as I am aware, I am not loading any testing modules. For example, in QEMU, I see: [ 0.596978] serio: loading test module taints kernel. and on my Honeycomb LX2, I see: [ 5.400861] fuse: loading test module taints kernel. It seems like the get_modinfo() check might be wrong? The following diff resolves it for me, I can send a formal patch if necessary (although it appears to have gone in via -mm so I assume Andrew can squash this in). Cheers, Nathan diff --git a/kernel/module/main.c b/kernel/module/main.c index 730503561eb0..4f91e41b8bc9 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -1988,7 +1988,7 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags) /* Set up license info based on the info section */ set_license(mod, get_modinfo(info, "license")); - if (!get_modinfo(info, "test")) { + if (get_modinfo(info, "test")) { if (!test_taint(TAINT_TEST)) pr_warn_once("%s: loading test module taints kernel.\n", mod->name);
On Fri, Jul 8, 2022 at 8:40 AM Nathan Chancellor <nathan@kernel.org> wrote: > > On Sat, Jul 02, 2022 at 12:09:57PM +0800, David Gow wrote: > > Taint the kernel with TAINT_TEST whenever a test module loads, by adding > > a new "TEST" module property, and setting it for all modules in the > > tools/testing directory. This property can also be set manually, for > > tests which live outside the tools/testing directory with: > > MODULE_INFO(test, "Y"); > > > > Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> > > Signed-off-by: David Gow <davidgow@google.com> > > --- > > kernel/module/main.c | 7 +++++++ > > scripts/mod/modpost.c | 3 +++ > > 2 files changed, 10 insertions(+) > > > > diff --git a/kernel/module/main.c b/kernel/module/main.c > > index fed58d30725d..730503561eb0 100644 > > --- a/kernel/module/main.c > > +++ b/kernel/module/main.c > > @@ -1988,6 +1988,13 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags) > > /* Set up license info based on the info section */ > > set_license(mod, get_modinfo(info, "license")); > > > > + if (!get_modinfo(info, "test")) { > > + if (!test_taint(TAINT_TEST)) > > + pr_warn_once("%s: loading test module taints kernel.\n", > > + mod->name); > > + add_taint_module(mod, TAINT_TEST, LOCKDEP_STILL_OK); > > + } > > + > > return 0; > > } > > > > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > > index 29d5a841e215..5937212b4433 100644 > > --- a/scripts/mod/modpost.c > > +++ b/scripts/mod/modpost.c > > @@ -2191,6 +2191,9 @@ static void add_header(struct buffer *b, struct module *mod) > > > > if (strstarts(mod->name, "drivers/staging")) > > buf_printf(b, "\nMODULE_INFO(staging, \"Y\");\n"); > > + > > + if (strstarts(mod->name, "tools/testing")) > > + buf_printf(b, "\nMODULE_INFO(test, \"Y\");\n"); > > } > > > > static void add_exported_symbols(struct buffer *buf, struct module *mod) > > -- > > 2.37.0.rc0.161.g10f37bed90-goog > > > > > > Hi David, > > This change has landed in linux-next as commit e20729ede7ed ("module: > panic: taint the kernel when selftest modules load") and on all of my > test machines, I see this new message printed, even though as far as I > am aware, I am not loading any testing modules. For example, in QEMU, I > see: > > [ 0.596978] serio: loading test module taints kernel. > > and on my Honeycomb LX2, I see: > > [ 5.400861] fuse: loading test module taints kernel. > > It seems like the get_modinfo() check might be wrong? The following diff > resolves it for me, I can send a formal patch if necessary (although it > appears to have gone in via -mm so I assume Andrew can squash this in). > Whoops: this is definitely the wrong way round. Thanks very much for catching it! (I'd swapped it locally at some point to test, and must've accidentally committed it.) I've sent out v6 with this fixed (and a couple of other minor changes): https://lore.kernel.org/linux-kselftest/20220708044847.531566-2-davidgow@google.com/T/#u That being said, if just squashing your change below in is easier, I'm fine with that, too. (The other changes are minor enough that we could live without them and/or send them in separately.) Unless there are any objections, should Andrew update this patch in his tree, and we remove the series from the kunit tree? Cheers, -- David > > diff --git a/kernel/module/main.c b/kernel/module/main.c > index 730503561eb0..4f91e41b8bc9 100644 > --- a/kernel/module/main.c > +++ b/kernel/module/main.c > @@ -1988,7 +1988,7 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags) > /* Set up license info based on the info section */ > set_license(mod, get_modinfo(info, "license")); > > - if (!get_modinfo(info, "test")) { > + if (get_modinfo(info, "test")) { > if (!test_taint(TAINT_TEST)) > pr_warn_once("%s: loading test module taints kernel.\n", > mod->name);
diff --git a/kernel/module/main.c b/kernel/module/main.c index fed58d30725d..730503561eb0 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -1988,6 +1988,13 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags) /* Set up license info based on the info section */ set_license(mod, get_modinfo(info, "license")); + if (!get_modinfo(info, "test")) { + if (!test_taint(TAINT_TEST)) + pr_warn_once("%s: loading test module taints kernel.\n", + mod->name); + add_taint_module(mod, TAINT_TEST, LOCKDEP_STILL_OK); + } + return 0; } diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c index 29d5a841e215..5937212b4433 100644 --- a/scripts/mod/modpost.c +++ b/scripts/mod/modpost.c @@ -2191,6 +2191,9 @@ static void add_header(struct buffer *b, struct module *mod) if (strstarts(mod->name, "drivers/staging")) buf_printf(b, "\nMODULE_INFO(staging, \"Y\");\n"); + + if (strstarts(mod->name, "tools/testing")) + buf_printf(b, "\nMODULE_INFO(test, \"Y\");\n"); } static void add_exported_symbols(struct buffer *buf, struct module *mod)