Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1469744 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-24 23:40 +0200
    Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-24 23:50 +0200
    Re: livepatch/module: make TAINT_LIVEPATCH module-specific Jessica Yu <jeyu@redhat.com> - 2016-08-25 06:40 +0200
    Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific Jiri Kosina <jikos@kernel.org> - 2016-08-25 06:50 +0200
      Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific Rusty Russell <rusty@rustcorp.com.au> - 2016-08-25 13:40 +0200
    Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific kbuild test robot <lkp@intel.com> - 2016-08-25 08:50 +0200
    Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-25 09:00 +0200
    Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific Petr Mladek <pmladek@suse.com> - 2016-08-25 09:50 +0200
      Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-25 14:50 +0200
    Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific Miroslav Benes <mbenes@suse.cz> - 2016-08-25 16:50 +0200
      Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific Miroslav Benes <mbenes@suse.cz> - 2016-08-25 16:50 +0200
      Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-25 16:50 +0200

#1469744 — [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific

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

[toc] | [next] | [standalone]

#1469752

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

[toc] | [prev] | [next] | [standalone]

#1469806 — Re: livepatch/module: make TAINT_LIVEPATCH module-specific

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

[toc] | [prev] | [next] | [standalone]

#1469819

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

[toc] | [prev] | [next] | [standalone]

#1470079

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.

[toc] | [prev] | [next] | [standalone]

#1469872

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1469874

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

[toc] | [prev] | [next] | [standalone]

#1469925

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

[toc] | [prev] | [next] | [standalone]

#1470118

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

[toc] | [prev] | [next] | [standalone]

#1470186

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
> 

[toc] | [prev] | [next] | [standalone]

#1470190

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

[toc] | [prev] | [next] | [standalone]

#1470195

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web