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

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

[Josh Poimboeuf]

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 <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);
-
 	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);
+	}
 
 	/* Set up license info based on the info section */
 	set_license(mod, get_modinfo(info, "license"));
-- 
2.7.4

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

[Josh Poimboeuf]

On Wed, Aug 24, 2016 at 04:33:00PM -0500, 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

"the the" -> "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>

-- 
Josh

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

[Jiri Kosina]

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 <chuhu@redhat.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

I like this change.

Rusty, in case you're okay with it as well, could you please either 
provide your Ack so that I could take it through livepatching.git? 
Alternatively, if you prefer to merge this through your tree, please feel 
free to add

	Acked-by: Jiri Kosina <jkosina@suse.cz>

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Rusty Russell]

Jiri Kosina <jikos@kernel.org> writes:
> 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 <chuhu@redhat.com>
>> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>
> I like this change.
>
> Rusty, in case you're okay with it as well, could you please either 
> provide your Ack so that I could take it through livepatching.git? 
> Alternatively, if you prefer to merge this through your tree, please feel 
> free to add

Happy to leave it to you :)

Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Thanks,
Rusty.

Re: livepatch/module: make TAINT_LIVEPATCH module-specific

[Jessica Yu]

+++ Josh Poimboeuf [24/08/16 16:33 -0500]:
>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.

Did a quick sanity check and verified the module taint
shows up in crash and sysfs as expected, looks good.

>Reviewed-by: Chunyu Hu <chuhu@redhat.com>
>Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

Acked-by: Jessica Yu <jeyu@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);
>-
> 	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);
>+	}
>
> 	/* Set up license info based on the info section */
> 	set_license(mod, get_modinfo(info, "license"));
>-- 
>2.7.4
>

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

[Josh Poimboeuf]

On Wed, Aug 24, 2016 at 04:33:00PM -0500, 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 <chuhu@redhat.com>

Sorry, another typo.  Should be:

Reported-by: Chunyu Hu <chuhu@redhat.com>

-- 
Josh

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1691 bytes --]

Hi Josh,

[auto build test ERROR on jikos-livepatching/for-next]
[also build test ERROR on v4.8-rc3 next-20160824]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Josh-Poimboeuf/livepatch-module-make-TAINT_LIVEPATCH-module-specific/20160825-053847
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jikos/livepatching.git for-next
config: x86_64-kexec (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   kernel/module.c: In function 'check_modinfo':
>> kernel/module.c:2960:6: error: 'struct module' has no member named 'klp'; did you mean 'kp'?
      mod->klp = true;
         ^~

vim +2960 kernel/module.c

  2954		if (get_modinfo(info, "livepatch")) {
  2955			if (!IS_ENABLED(CONFIG_LIVEPATCH)) {
  2956				pr_err("%s: module is marked as livepatch module, but livepatch support is disabled\n",
  2957				       mod->name);
  2958				return -ENOEXEC;
  2959			}
> 2960			mod->klp = true;
  2961			pr_warn("%s: loading livepatch module.\n", mod->name);
  2962			add_taint_module(mod, TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
  2963		}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 22949 bytes --]

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

[Petr Mladek]

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

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

[Josh Poimboeuf]

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

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

[Miroslav Benes]

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 <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);
> -
>  	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?

Miroslav

>  
>  	/* Set up license info based on the info section */
>  	set_license(mod, get_modinfo(info, "license"));
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Josh Poimboeuf]

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 <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);
> > -
> >  	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/87mw3jxdea.fsf@rustcorp.com.au

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

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

[Miroslav Benes]

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 <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);
> > > -
> > >  	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/87mw3jxdea.fsf@rustcorp.com.au
> 
> 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