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

[Petr Mladek <pmladek@suse.com>]

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.

Best Regards,
Petr