On Thu, 25 Aug 2016, Josh Poimboeuf wrote: > 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.
Ah, right. Ok then. Miroslav
