Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Thu, Aug 25, 2016 at 09:46:06AM +0200, Petr Mladek wrote:
> Hi,
> 
> I have spent some time to understand the change. I hope that the
> comments below would help others.
> 
> On Wed 2016-08-24 16:33:00, 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.
> 
> The above paragraph is a bit confusing. The patch adds TAINT_LIVEPATCH into the
> list of module taint flags.
>  
> > 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 <chuhu@redhat.com>
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> >  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);
> 
> The first important thing is that add_taint() is replaced with
> add_taint_module(). The other function sets also mod->taints.
> 
> It is a module taint flag, so it really makes sense to call it
> when the module is loaded.
> 
> > -
> >  	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';
> 
> This is the second important part of the change. It shows the flag
> in /sys/module/<klp module>/taint.
> 
> The rest is just reshufling of the code. But it has problems
> as already reported by kbuild test robot.
> 
> The change looks good to me. We just need to fix the compilation
> problem by adding some #ifdefs.

This patch was a bit sloppy.  I was doing too many things at once. :-)

I'll fix it up and improve the description.

-- 
Josh