On Thu, Aug 25, 2016 at 04:25:15PM +0200, Miroslav Benes wrote: > On Wed, 24 Aug 2016, Josh Poimboeuf wrote: > > > There's no reliable way to determine which module tainted the kernel > > with CONFIG_LIVEPATCH. For example, /sys/module/<klp module>/taint > > doesn't report it. Neither does the "mod -t" command in the crash tool. > > > > Make it crystal clear who the guilty party is by converting > > CONFIG_LIVEPATCH to a module taint flag. > > > > This changes the behavior a bit: now the the flag gets set when the > > module is loaded, rather than when it's enabled. > > > > Reviewed-by: Chunyu Hu <[email protected]> > > Signed-off-by: Josh Poimboeuf <[email protected]> > > --- > > kernel/livepatch/core.c | 3 --- > > kernel/module.c | 35 ++++++++++++----------------------- > > 2 files changed, 12 insertions(+), 26 deletions(-) > > > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > > index 5fbabe0..af46438 100644 > > --- a/kernel/livepatch/core.c > > +++ b/kernel/livepatch/core.c > > @@ -545,9 +545,6 @@ static int __klp_enable_patch(struct klp_patch *patch) > > list_prev_entry(patch, list)->state == KLP_DISABLED) > > return -EBUSY; > > > > - pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n"); > > - add_taint(TAINT_LIVEPATCH, LOCKDEP_STILL_OK); > > - > > pr_notice("enabling patch '%s'\n", patch->mod->name); > > > > klp_for_each_object(patch, obj) { > > diff --git a/kernel/module.c b/kernel/module.c > > index 529efae..fd5f95b 100644 > > --- a/kernel/module.c > > +++ b/kernel/module.c > > @@ -1149,6 +1149,8 @@ static size_t module_flags_taint(struct module *mod, > > char *buf) > > buf[l++] = 'C'; > > if (mod->taints & (1 << TAINT_UNSIGNED_MODULE)) > > buf[l++] = 'E'; > > + if (mod->taints & (1 << TAINT_LIVEPATCH)) > > + buf[l++] = 'K'; > > /* > > * TAINT_FORCED_RMMOD: could be added. > > * TAINT_CPU_OUT_OF_SPEC, TAINT_MACHINE_CHECK, TAINT_BAD_PAGE don't > > @@ -2791,26 +2793,6 @@ static int copy_chunked_from_user(void *dst, const > > void __user *usrc, unsigned l > > return 0; > > } > > > > -#ifdef CONFIG_LIVEPATCH > > -static int find_livepatch_modinfo(struct module *mod, struct load_info > > *info) > > -{ > > - mod->klp = get_modinfo(info, "livepatch") ? true : false; > > - > > - return 0; > > -} > > -#else /* !CONFIG_LIVEPATCH */ > > -static int find_livepatch_modinfo(struct module *mod, struct load_info > > *info) > > -{ > > - if (get_modinfo(info, "livepatch")) { > > - pr_err("%s: module is marked as livepatch module, but livepatch > > support is disabled", > > - mod->name); > > - return -ENOEXEC; > > - } > > - > > - return 0; > > -} > > -#endif /* CONFIG_LIVEPATCH */ > > - > > /* Sets info->hdr and info->len. */ > > static int copy_module_from_user(const void __user *umod, unsigned long > > len, > > struct load_info *info) > > @@ -2969,9 +2951,16 @@ static int check_modinfo(struct module *mod, struct > > load_info *info, int flags) > > "is unknown, you have been warned.\n", mod->name); > > } > > > > - err = find_livepatch_modinfo(mod, info); > > - if (err) > > - return err; > > + if (get_modinfo(info, "livepatch")) { > > + if (!IS_ENABLED(CONFIG_LIVEPATCH)) { > > + pr_err("%s: module is marked as livepatch module, but > > livepatch support is disabled\n", > > + mod->name); > > + return -ENOEXEC; > > + } > > + mod->klp = true; > > + pr_warn("%s: loading livepatch module.\n", mod->name); > > + add_taint_module(mod, TAINT_LIVEPATCH, LOCKDEP_STILL_OK); > > + } > > The old code set mod->klp to false if get_modinfo(info, "livepatch")) > returned true. I think that we don't have to do it, because struct module > of a module is statically allocated (if I am not mistaken) and hence > mod->klp should be initialized to false. However maybe it'd better to do > it explicitly. What do you think?
Rusty confirmed before that the module struct is initialized to zero: https://lkml.kernel.org/r/[email protected] And I suspect a lot of module code relies on that fact. For example, see mod->async_probe_requested. So my preference would be to follow what seems to be the current convention in the code, and not explicitly initialize it to false. -- Josh
