linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific
@ 2016-08-24 21:33 Josh Poimboeuf
  2016-08-24 21:43 ` Josh Poimboeuf
                   ` (6 more replies)
  0 siblings, 7 replies; 12+ messages in thread
From: Josh Poimboeuf @ 2016-08-24 21:33 UTC (permalink / raw)
  To: Jessica Yu, Jiri Kosina, Miroslav Benes
  Cc: Petr Mladek, linux-kernel, live-patching, Chunyu Hu, Rusty Russell

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific
  2016-08-24 21:33 [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific Josh Poimboeuf
@ 2016-08-24 21:43 ` Josh Poimboeuf
  2016-08-25  0:54 ` Jiri Kosina
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Josh Poimboeuf @ 2016-08-24 21:43 UTC (permalink / raw)
  To: Jessica Yu, Jiri Kosina, Miroslav Benes
  Cc: Petr Mladek, linux-kernel, live-patching, Chunyu Hu, Rusty Russell

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific
  2016-08-24 21:33 [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific Josh Poimboeuf
  2016-08-24 21:43 ` Josh Poimboeuf
@ 2016-08-25  0:54 ` Jiri Kosina
  2016-08-25  4:14   ` Rusty Russell
  2016-08-25  2:53 ` Jessica Yu
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 12+ messages in thread
From: Jiri Kosina @ 2016-08-25  0:54 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Jessica Yu, Miroslav Benes, Petr Mladek, linux-kernel,
	live-patching, Chunyu Hu, Rusty Russell

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: livepatch/module: make TAINT_LIVEPATCH module-specific
  2016-08-24 21:33 [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific Josh Poimboeuf
  2016-08-24 21:43 ` Josh Poimboeuf
  2016-08-25  0:54 ` Jiri Kosina
@ 2016-08-25  2:53 ` Jessica Yu
  2016-08-25  3:23 ` [PATCH] " Josh Poimboeuf
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Jessica Yu @ 2016-08-25  2:53 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Jiri Kosina, Miroslav Benes, Petr Mladek, linux-kernel,
	live-patching, Chunyu Hu, Rusty Russell

+++ 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
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific
  2016-08-24 21:33 [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific Josh Poimboeuf
                   ` (2 preceding siblings ...)
  2016-08-25  2:53 ` Jessica Yu
@ 2016-08-25  3:23 ` Josh Poimboeuf
  2016-08-25  6:40 ` kbuild test robot
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Josh Poimboeuf @ 2016-08-25  3:23 UTC (permalink / raw)
  To: Jessica Yu, Jiri Kosina, Miroslav Benes
  Cc: Petr Mladek, linux-kernel, live-patching, Chunyu Hu, Rusty Russell

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific
  2016-08-25  0:54 ` Jiri Kosina
@ 2016-08-25  4:14   ` Rusty Russell
  0 siblings, 0 replies; 12+ messages in thread
From: Rusty Russell @ 2016-08-25  4:14 UTC (permalink / raw)
  To: Jiri Kosina, Josh Poimboeuf
  Cc: Jessica Yu, Miroslav Benes, Petr Mladek, linux-kernel,
	live-patching, Chunyu Hu

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific
  2016-08-24 21:33 [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific Josh Poimboeuf
                   ` (3 preceding siblings ...)
  2016-08-25  3:23 ` [PATCH] " Josh Poimboeuf
@ 2016-08-25  6:40 ` kbuild test robot
  2016-08-25  7:46 ` Petr Mladek
  2016-08-25 14:25 ` Miroslav Benes
  6 siblings, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2016-08-25  6:40 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: kbuild-all, Jessica Yu, Jiri Kosina, Miroslav Benes, Petr Mladek,
	linux-kernel, live-patching, Chunyu Hu, Rusty Russell

[-- 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 --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific
  2016-08-24 21:33 [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific Josh Poimboeuf
                   ` (4 preceding siblings ...)
  2016-08-25  6:40 ` kbuild test robot
@ 2016-08-25  7:46 ` Petr Mladek
  2016-08-25 12:31   ` Josh Poimboeuf
  2016-08-25 14:25 ` Miroslav Benes
  6 siblings, 1 reply; 12+ messages in thread
From: Petr Mladek @ 2016-08-25  7:46 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Jessica Yu, Jiri Kosina, Miroslav Benes, linux-kernel,
	live-patching, Chunyu Hu, Rusty Russell

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific
  2016-08-25  7:46 ` Petr Mladek
@ 2016-08-25 12:31   ` Josh Poimboeuf
  0 siblings, 0 replies; 12+ messages in thread
From: Josh Poimboeuf @ 2016-08-25 12:31 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Jessica Yu, Jiri Kosina, Miroslav Benes, linux-kernel,
	live-patching, Chunyu Hu, Rusty Russell

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific
  2016-08-24 21:33 [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific Josh Poimboeuf
                   ` (5 preceding siblings ...)
  2016-08-25  7:46 ` Petr Mladek
@ 2016-08-25 14:25 ` Miroslav Benes
  2016-08-25 14:42   ` Josh Poimboeuf
  6 siblings, 1 reply; 12+ messages in thread
From: Miroslav Benes @ 2016-08-25 14:25 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Jessica Yu, Jiri Kosina, Petr Mladek, linux-kernel,
	live-patching, Chunyu Hu, Rusty Russell

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
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific
  2016-08-25 14:25 ` Miroslav Benes
@ 2016-08-25 14:42   ` Josh Poimboeuf
  2016-08-25 14:47     ` Miroslav Benes
  0 siblings, 1 reply; 12+ messages in thread
From: Josh Poimboeuf @ 2016-08-25 14:42 UTC (permalink / raw)
  To: Miroslav Benes
  Cc: Jessica Yu, Jiri Kosina, Petr Mladek, linux-kernel,
	live-patching, Chunyu Hu, Rusty Russell

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific
  2016-08-25 14:42   ` Josh Poimboeuf
@ 2016-08-25 14:47     ` Miroslav Benes
  0 siblings, 0 replies; 12+ messages in thread
From: Miroslav Benes @ 2016-08-25 14:47 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Jessica Yu, Jiri Kosina, Petr Mladek, linux-kernel,
	live-patching, Chunyu Hu, Rusty Russell

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-08-25 14:47 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-24 21:33 [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific Josh Poimboeuf
2016-08-24 21:43 ` Josh Poimboeuf
2016-08-25  0:54 ` Jiri Kosina
2016-08-25  4:14   ` Rusty Russell
2016-08-25  2:53 ` Jessica Yu
2016-08-25  3:23 ` [PATCH] " Josh Poimboeuf
2016-08-25  6:40 ` kbuild test robot
2016-08-25  7:46 ` Petr Mladek
2016-08-25 12:31   ` Josh Poimboeuf
2016-08-25 14:25 ` Miroslav Benes
2016-08-25 14:42   ` Josh Poimboeuf
2016-08-25 14:47     ` Miroslav Benes

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).