[PATCH v3] livepatch: Prevent to apply the patch once coming module notifier fails

[PATCH v3] livepatch: Prevent to apply the patch once coming module notifier fails

[Minfei Huang]

The previous patches can be applied, once the corresponding module is
loaded. In general, the patch will do relocation (if necessary) and
obtain/verify function address before we start to enable patch.

There are three different situations in which the coming module notifier
can fail:

1) relocations are not applied for some reason. In this case kallsyms
for module symbol is not called at all. The patch is not applied to the
module. If the user disable and enable patch again, there is possible
bug in klp_enable_func. If the user specified func->old_addr for some
function in the module (and he shouldn't do that, but nevertheless) our
warning would not catch it, there will be something wrong with the
ftrace.

2) relocations are applied successfully, but kallsyms lookup fails. In
this case func->old_addr can be correct for all previous lookups, 0 for
current failed one, and "unspecified" for the rest. If we undergo the
same scenario as in 1, the behaviour differs for three cases, but the
patch is not enable anyway.

3) the object is initialized, but klp_enable_object fails in the
notifier due to possible ftrace error. Since it is improbable that
ftrace would heal itself in the future, we would get those errors
everytime the patch is enabled.

In order to fix above situations, we can make obj->mod to NULL, if the
coming modified notifier fails.

Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
---
v2:
- add the error message to make it more friendly
- modify the commit log, base on the mbenes@suse.cz suggesting
v1:
- modify the commit log, describe the issue more details
---
 kernel/livepatch/core.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 284e269..e60d7ab 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -883,7 +883,7 @@ int klp_register_patch(struct klp_patch *patch)
 }
 EXPORT_SYMBOL_GPL(klp_register_patch);
 
-static void klp_module_notify_coming(struct klp_patch *patch,
+static int klp_module_notify_coming(struct klp_patch *patch,
 				     struct klp_object *obj)
 {
 	struct module *pmod = patch->mod;
@@ -891,22 +891,24 @@ static void klp_module_notify_coming(struct klp_patch *patch,
 	int ret;
 
 	ret = klp_init_object_loaded(patch, obj);
-	if (ret)
-		goto err;
+	if (ret) {
+		pr_warn("failed to initialize the patch '%s' (%d)\n",
+				pmod->name, ret);
+		goto out;
+	}
 
 	if (patch->state == KLP_DISABLED)
-		return;
+		goto out;
 
 	pr_notice("applying patch '%s' to loading module '%s'\n",
 		  pmod->name, mod->name);
 
 	ret = klp_enable_object(obj);
-	if (!ret)
-		return;
-
-err:
-	pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
-		pmod->name, mod->name, ret);
+	if (ret)
+		pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
+				pmod->name, mod->name, ret);
+out:
+	return ret;
 }
 
 static void klp_module_notify_going(struct klp_patch *patch,
@@ -930,6 +932,7 @@ disabled:
 static int klp_module_notify(struct notifier_block *nb, unsigned long action,
 			     void *data)
 {
+	int ret;
 	struct module *mod = data;
 	struct klp_patch *patch;
 	struct klp_object *obj;
@@ -955,7 +958,13 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
 
 			if (action == MODULE_STATE_COMING) {
 				obj->mod = mod;
-				klp_module_notify_coming(patch, obj);
+				ret = klp_module_notify_coming(patch, obj);
+				if (ret) {
+					obj->mod = NULL;
+					pr_warn("patch '%s' is dead, remove it "
+						"or re-install the module '%s'\n",
+						patch->mod->name, obj->name);
+				}
 			} else /* MODULE_STATE_GOING */
 				klp_module_notify_going(patch, obj);
 
-- 
2.2.2

Re: [PATCH v3] livepatch: Prevent to apply the patch once coming module notifier fails

[Josh Poimboeuf]

On Tue, May 12, 2015 at 10:04:44PM +0800, Minfei Huang wrote:
> @@ -883,7 +883,7 @@ int klp_register_patch(struct klp_patch *patch)
>  }
>  EXPORT_SYMBOL_GPL(klp_register_patch);
>  
> -static void klp_module_notify_coming(struct klp_patch *patch,
> +static int klp_module_notify_coming(struct klp_patch *patch,
>  				     struct klp_object *obj)
>  {
>  	struct module *pmod = patch->mod;
> @@ -891,22 +891,24 @@ static void klp_module_notify_coming(struct klp_patch *patch,
>  	int ret;
>  
>  	ret = klp_init_object_loaded(patch, obj);
> -	if (ret)
> -		goto err;
> +	if (ret) {
> +		pr_warn("failed to initialize the patch '%s' (%d)\n",
> +				pmod->name, ret);
> +		goto out;
> +	}

Can you change it to:

"failed to initialize the patch '%s' for module '%s' (%d)\n" ?

That would make it more consistent with the other error message and
identify the failing module.

Also, the indentation should be fixed on the second pr_warn() line.

>  
>  	if (patch->state == KLP_DISABLED)
> -		return;
> +		goto out;
>  
>  	pr_notice("applying patch '%s' to loading module '%s'\n",
>  		  pmod->name, mod->name);
>  
>  	ret = klp_enable_object(obj);
> -	if (!ret)
> -		return;
> -
> -err:
> -	pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> -		pmod->name, mod->name, ret);
> +	if (ret)
> +		pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> +				pmod->name, mod->name, ret);

Bad indentation here too.

> @@ -930,6 +932,7 @@ disabled:
>  static int klp_module_notify(struct notifier_block *nb, unsigned long action,
>  			     void *data)
>  {
> +	int ret;
>  	struct module *mod = data;
>  	struct klp_patch *patch;
>  	struct klp_object *obj;
> @@ -955,7 +958,13 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
>  
>  			if (action == MODULE_STATE_COMING) {
>  				obj->mod = mod;
> -				klp_module_notify_coming(patch, obj);
> +				ret = klp_module_notify_coming(patch, obj);
> +				if (ret) {
> +					obj->mod = NULL;
> +					pr_warn("patch '%s' is dead, remove it "
> +						"or re-install the module '%s'\n",
> +						patch->mod->name, obj->name);
> +				}

The patch isn't necessarily dead, since it might also include previously
enabled changes for vmlinux or other modules.  It can actually be a
dangerous condition if there's a mismatch between old code in the module
and new code elsewhere.  How about something like:

"patch '%s' is in an inconsistent state!\n"

Also, there's no need to split up the string literal into two lines.
It's ok for a line to have more than 80 columns in that case.

-- 
Josh

Re: [PATCH v3] livepatch: Prevent to apply the patch once coming module notifier fails

[Minfei Huang]

On 05/13/15 at 09:14P, Josh Poimboeuf wrote:
> On Tue, May 12, 2015 at 10:04:44PM +0800, Minfei Huang wrote:
> > @@ -883,7 +883,7 @@ int klp_register_patch(struct klp_patch *patch)
> >  }
> >  EXPORT_SYMBOL_GPL(klp_register_patch);
> >  
> > -static void klp_module_notify_coming(struct klp_patch *patch,
> > +static int klp_module_notify_coming(struct klp_patch *patch,
> >  				     struct klp_object *obj)
> >  {
> >  	struct module *pmod = patch->mod;
> > @@ -891,22 +891,24 @@ static void klp_module_notify_coming(struct klp_patch *patch,
> >  	int ret;
> >  
> >  	ret = klp_init_object_loaded(patch, obj);
> > -	if (ret)
> > -		goto err;
> > +	if (ret) {
> > +		pr_warn("failed to initialize the patch '%s' (%d)\n",
> > +				pmod->name, ret);
> > +		goto out;
> > +	}
> 
> Can you change it to:
> 
> "failed to initialize the patch '%s' for module '%s' (%d)\n" ?
> 
> That would make it more consistent with the other error message and
> identify the failing module.
> 
> Also, the indentation should be fixed on the second pr_warn() line.
> 

Will modify.

> >  
> >  	if (patch->state == KLP_DISABLED)
> > -		return;
> > +		goto out;
> >  
> >  	pr_notice("applying patch '%s' to loading module '%s'\n",
> >  		  pmod->name, mod->name);
> >  
> >  	ret = klp_enable_object(obj);
> > -	if (!ret)
> > -		return;
> > -
> > -err:
> > -	pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > -		pmod->name, mod->name, ret);
> > +	if (ret)
> > +		pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > +				pmod->name, mod->name, ret);
> 
> Bad indentation here too.
> 
> > @@ -930,6 +932,7 @@ disabled:
> >  static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> >  			     void *data)
> >  {
> > +	int ret;
> >  	struct module *mod = data;
> >  	struct klp_patch *patch;
> >  	struct klp_object *obj;
> > @@ -955,7 +958,13 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> >  
> >  			if (action == MODULE_STATE_COMING) {
> >  				obj->mod = mod;
> > -				klp_module_notify_coming(patch, obj);
> > +				ret = klp_module_notify_coming(patch, obj);
> > +				if (ret) {
> > +					obj->mod = NULL;
> > +					pr_warn("patch '%s' is dead, remove it "
> > +						"or re-install the module '%s'\n",
> > +						patch->mod->name, obj->name);
> > +				}
> 
> The patch isn't necessarily dead, since it might also include previously
> enabled changes for vmlinux or other modules.  It can actually be a
> dangerous condition if there's a mismatch between old code in the module
> and new code elsewhere.  How about something like:
> 
> "patch '%s' is in an inconsistent state!\n"
> 
> Also, there's no need to split up the string literal into two lines.
> It's ok for a line to have more than 80 columns in that case.
> 

Thanks for your reviewing. Will modify the patch.

Thanks
Minfei

> -- 
> Josh

Re: [PATCH v3] livepatch: Prevent to apply the patch once coming module notifier fails

[Petr Mladek]

On Wed 2015-05-13 09:14:15, Josh Poimboeuf wrote:
> On Tue, May 12, 2015 at 10:04:44PM +0800, Minfei Huang wrote:
> > @@ -883,7 +883,7 @@ int klp_register_patch(struct klp_patch *patch)
> >  }
> >  EXPORT_SYMBOL_GPL(klp_register_patch);
> >  
> > -static void klp_module_notify_coming(struct klp_patch *patch,
> > +static int klp_module_notify_coming(struct klp_patch *patch,
> >  				     struct klp_object *obj)
> >  {
> >  	struct module *pmod = patch->mod;
> > @@ -891,22 +891,24 @@ static void klp_module_notify_coming(struct klp_patch *patch,
> >  	int ret;
> >  
> >  	ret = klp_init_object_loaded(patch, obj);
> > -	if (ret)
> > -		goto err;
> > +	if (ret) {
> > +		pr_warn("failed to initialize the patch '%s' (%d)\n",
> > +				pmod->name, ret);
> > +		goto out;
> > +	}
> 
> Can you change it to:
> 
> "failed to initialize the patch '%s' for module '%s' (%d)\n" ?
> 
> That would make it more consistent with the other error message and
> identify the failing module.
> 
> Also, the indentation should be fixed on the second pr_warn() line.
> 
> >  
> >  	if (patch->state == KLP_DISABLED)
> > -		return;
> > +		goto out;
> >  
> >  	pr_notice("applying patch '%s' to loading module '%s'\n",
> >  		  pmod->name, mod->name);
> >  
> >  	ret = klp_enable_object(obj);
> > -	if (!ret)
> > -		return;
> > -
> > -err:
> > -	pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > -		pmod->name, mod->name, ret);
> > +	if (ret)
> > +		pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > +				pmod->name, mod->name, ret);
> 
> Bad indentation here too.
> 
> > @@ -930,6 +932,7 @@ disabled:
> >  static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> >  			     void *data)
> >  {
> > +	int ret;
> >  	struct module *mod = data;
> >  	struct klp_patch *patch;
> >  	struct klp_object *obj;
> > @@ -955,7 +958,13 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> >  
> >  			if (action == MODULE_STATE_COMING) {
> >  				obj->mod = mod;
> > -				klp_module_notify_coming(patch, obj);
> > +				ret = klp_module_notify_coming(patch, obj);
> > +				if (ret) {
> > +					obj->mod = NULL;
> > +					pr_warn("patch '%s' is dead, remove it "
> > +						"or re-install the module '%s'\n",
> > +						patch->mod->name, obj->name);
> > +				}
> 
> The patch isn't necessarily dead, since it might also include previously
> enabled changes for vmlinux or other modules.  It can actually be a
> dangerous condition if there's a mismatch between old code in the module
> and new code elsewhere.  How about something like:
> 
> "patch '%s' is in an inconsistent state!\n"

It must not be dangerous, otherwise the patch could not get applied
immediately.

I would omit this message completely. It would just duplicate the
warning printed by klp_module_notify_coming().


> Also, there's no need to split up the string literal into two lines.
> It's ok for a line to have more than 80 columns in that case.

I suggest to run ./scripts/chechpatch.pl before you send any patch.
It would catch the indentation problems, split of the string, ...

Best Regards,
Petr

Re: [PATCH v3] livepatch: Prevent to apply the patch once coming module notifier fails

[Minfei Huang]

On 05/18/15 at 02:08pm, Petr Mladek wrote:
> On Wed 2015-05-13 09:14:15, Josh Poimboeuf wrote:
> > On Tue, May 12, 2015 at 10:04:44PM +0800, Minfei Huang wrote:
> > > @@ -883,7 +883,7 @@ int klp_register_patch(struct klp_patch *patch)
> > >  }
> > >  EXPORT_SYMBOL_GPL(klp_register_patch);
> > >  
> > > -static void klp_module_notify_coming(struct klp_patch *patch,
> > > +static int klp_module_notify_coming(struct klp_patch *patch,
> > >  				     struct klp_object *obj)
> > >  {
> > >  	struct module *pmod = patch->mod;
> > > @@ -891,22 +891,24 @@ static void klp_module_notify_coming(struct klp_patch *patch,
> > >  	int ret;
> > >  
> > >  	ret = klp_init_object_loaded(patch, obj);
> > > -	if (ret)
> > > -		goto err;
> > > +	if (ret) {
> > > +		pr_warn("failed to initialize the patch '%s' (%d)\n",
> > > +				pmod->name, ret);
> > > +		goto out;
> > > +	}
> > 
> > Can you change it to:
> > 
> > "failed to initialize the patch '%s' for module '%s' (%d)\n" ?
> > 
> > That would make it more consistent with the other error message and
> > identify the failing module.
> > 
> > Also, the indentation should be fixed on the second pr_warn() line.
> > 
> > >  
> > >  	if (patch->state == KLP_DISABLED)
> > > -		return;
> > > +		goto out;
> > >  
> > >  	pr_notice("applying patch '%s' to loading module '%s'\n",
> > >  		  pmod->name, mod->name);
> > >  
> > >  	ret = klp_enable_object(obj);
> > > -	if (!ret)
> > > -		return;
> > > -
> > > -err:
> > > -	pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > > -		pmod->name, mod->name, ret);
> > > +	if (ret)
> > > +		pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > > +				pmod->name, mod->name, ret);
> > 
> > Bad indentation here too.
> > 
> > > @@ -930,6 +932,7 @@ disabled:
> > >  static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > >  			     void *data)
> > >  {
> > > +	int ret;
> > >  	struct module *mod = data;
> > >  	struct klp_patch *patch;
> > >  	struct klp_object *obj;
> > > @@ -955,7 +958,13 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > >  
> > >  			if (action == MODULE_STATE_COMING) {
> > >  				obj->mod = mod;
> > > -				klp_module_notify_coming(patch, obj);
> > > +				ret = klp_module_notify_coming(patch, obj);
> > > +				if (ret) {
> > > +					obj->mod = NULL;
> > > +					pr_warn("patch '%s' is dead, remove it "
> > > +						"or re-install the module '%s'\n",
> > > +						patch->mod->name, obj->name);
> > > +				}
> > 
> > The patch isn't necessarily dead, since it might also include previously
> > enabled changes for vmlinux or other modules.  It can actually be a
> > dangerous condition if there's a mismatch between old code in the module
> > and new code elsewhere.  How about something like:
> > 
> > "patch '%s' is in an inconsistent state!\n"
> 
> It must not be dangerous, otherwise the patch could not get applied
> immediately.

But kernel is in dangerous situation that the patch may corrupt it
later. So it is appropriate to notify the user.

> 
> I would omit this message completely. It would just duplicate the
> warning printed by klp_module_notify_coming().
> 

This error message aims to tell this fact that this patch is in an
inconsistent state. If someone do not notify this error, it is fine,
because the inconsistent patch does not have change to be applied to the
kernel.

> 
> > Also, there's no need to split up the string literal into two lines.
> > It's ok for a line to have more than 80 columns in that case.
> 
> I suggest to run ./scripts/chechpatch.pl before you send any patch.
> It would catch the indentation problems, split of the string, ...

I have used the script checkpatch.pl to verify the patch. And it passed
for the checking. About the indentation problems, it may be caused by
the sepcified vimrc file.

Thanks
Minfei

> 
> Best Regards,
> Petr

Re: [PATCH v3] livepatch: Prevent to apply the patch once coming module notifier fails

[Petr Mladek]

On Mon 2015-05-18 21:00:57, Minfei Huang wrote:
> On 05/18/15 at 02:08pm, Petr Mladek wrote:
> > On Wed 2015-05-13 09:14:15, Josh Poimboeuf wrote:
> > > On Tue, May 12, 2015 at 10:04:44PM +0800, Minfei Huang wrote:
> > > > @@ -883,7 +883,7 @@ int klp_register_patch(struct klp_patch *patch)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(klp_register_patch);
> > > >  
> > > > -static void klp_module_notify_coming(struct klp_patch *patch,
> > > > +static int klp_module_notify_coming(struct klp_patch *patch,
> > > >  				     struct klp_object *obj)
> > > >  {
> > > >  	struct module *pmod = patch->mod;
> > > > @@ -891,22 +891,24 @@ static void klp_module_notify_coming(struct klp_patch *patch,
> > > >  	int ret;
> > > >  
> > > >  	ret = klp_init_object_loaded(patch, obj);
> > > > -	if (ret)
> > > > -		goto err;
> > > > +	if (ret) {
> > > > +		pr_warn("failed to initialize the patch '%s' (%d)\n",
> > > > +				pmod->name, ret);
> > > > +		goto out;
> > > > +	}
> > > 
> > > Can you change it to:
> > > 
> > > "failed to initialize the patch '%s' for module '%s' (%d)\n" ?
> > > 
> > > That would make it more consistent with the other error message and
> > > identify the failing module.
> > > 
> > > Also, the indentation should be fixed on the second pr_warn() line.
> > > 
> > > >  
> > > >  	if (patch->state == KLP_DISABLED)
> > > > -		return;
> > > > +		goto out;
> > > >  
> > > >  	pr_notice("applying patch '%s' to loading module '%s'\n",
> > > >  		  pmod->name, mod->name);
> > > >  
> > > >  	ret = klp_enable_object(obj);
> > > > -	if (!ret)
> > > > -		return;
> > > > -
> > > > -err:
> > > > -	pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > > > -		pmod->name, mod->name, ret);
> > > > +	if (ret)
> > > > +		pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > > > +				pmod->name, mod->name, ret);
> > > 
> > > Bad indentation here too.
> > > 
> > > > @@ -930,6 +932,7 @@ disabled:
> > > >  static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > > >  			     void *data)
> > > >  {
> > > > +	int ret;
> > > >  	struct module *mod = data;
> > > >  	struct klp_patch *patch;
> > > >  	struct klp_object *obj;
> > > > @@ -955,7 +958,13 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > > >  
> > > >  			if (action == MODULE_STATE_COMING) {
> > > >  				obj->mod = mod;
> > > > -				klp_module_notify_coming(patch, obj);
> > > > +				ret = klp_module_notify_coming(patch, obj);
> > > > +				if (ret) {
> > > > +					obj->mod = NULL;
> > > > +					pr_warn("patch '%s' is dead, remove it "
> > > > +						"or re-install the module '%s'\n",
> > > > +						patch->mod->name, obj->name);
> > > > +				}
> > > 
> > > The patch isn't necessarily dead, since it might also include previously
> > > enabled changes for vmlinux or other modules.  It can actually be a
> > > dangerous condition if there's a mismatch between old code in the module
> > > and new code elsewhere.  How about something like:
> > > 
> > > "patch '%s' is in an inconsistent state!\n"
> > 
> > It must not be dangerous, otherwise the patch could not get applied
> > immediately.
> 
> But kernel is in dangerous situation that the patch may corrupt it
> later. So it is appropriate to notify the user.

How exactly could the patch corrupt the system? Could you please give
an example?

> > 
> > I would omit this message completely. It would just duplicate the
> > warning printed by klp_module_notify_coming().
> > 
> 
> This error message aims to tell this fact that this patch is in an
> inconsistent state. If someone do not notify this error, it is fine,
> because the inconsistent patch does not have change to be applied to the
> kernel.

But there is already printed a warning from
klp_module_notify_coming(). I wonder why we need one more here.

> > 
> > > Also, there's no need to split up the string literal into two lines.
> > > It's ok for a line to have more than 80 columns in that case.
> > 
> > I suggest to run ./scripts/chechpatch.pl before you send any patch.
> > It would catch the indentation problems, split of the string, ...
> 
> I have used the script checkpatch.pl to verify the patch. And it passed
> for the checking. About the indentation problems, it may be caused by
> the sepcified vimrc file.

I get the following warning on your patch:

--- cut ---
$> ./scripts/checkpatch.pl /prace/klp-module-notify-error-v3-iter1.patch
WARNING: quoted string split across lines
#181: FILE: kernel/livepatch/core.c:965:
+                                       pr_warn("patch '%s' is dead, remove it "
+                                               "or re-install the module '%s'\n",

total: 0 errors, 1 warnings, 62 lines checked

/prace/klp-module-notify-error-v3-iter1.patch has style problems, please review.

If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
--- cut ---

BTW: I do not see the problems with indentation.

Best Regards,
Petr

Re: [PATCH v3] livepatch: Prevent to apply the patch once coming module notifier fails

[Minfei Huang]

On 05/18/15 at 05:35pm, Petr Mladek wrote:
> On Mon 2015-05-18 21:00:57, Minfei Huang wrote:
> > On 05/18/15 at 02:08pm, Petr Mladek wrote:
> > > On Wed 2015-05-13 09:14:15, Josh Poimboeuf wrote:
...[snip]...
> > > > The patch isn't necessarily dead, since it might also include previously
> > > > enabled changes for vmlinux or other modules.  It can actually be a
> > > > dangerous condition if there's a mismatch between old code in the module
> > > > and new code elsewhere.  How about something like:
> > > > 
> > > > "patch '%s' is in an inconsistent state!\n"
> > > 
> > > It must not be dangerous, otherwise the patch could not get applied
> > > immediately.
> > 
> > But kernel is in dangerous situation that the patch may corrupt it
> > later. So it is appropriate to notify the user.
> 
> How exactly could the patch corrupt the system? Could you please give
> an example?
> 

Please review the commit log where I show the step by step in thread.

> > > 
> > > I would omit this message completely. It would just duplicate the
> > > warning printed by klp_module_notify_coming().
> > > 
> > 
> > This error message aims to tell this fact that this patch is in an
> > inconsistent state. If someone do not notify this error, it is fine,
> > because the inconsistent patch does not have change to be applied to the
> > kernel.
> 
> But there is already printed a warning from
> klp_module_notify_coming(). I wonder why we need one more here.
> 

Since the error messages are different for the different called function
in klp_module_notify_coming. It only shows that there is something wrong
to call the function.

It is prefer that livepatch can notify the user how the patch is, if
klp_module_notify_coming fails. 

> > > 
> > > > Also, there's no need to split up the string literal into two lines.
> > > > It's ok for a line to have more than 80 columns in that case.
> > > 
> > > I suggest to run ./scripts/chechpatch.pl before you send any patch.
> > > It would catch the indentation problems, split of the string, ...
> > 
> > I have used the script checkpatch.pl to verify the patch. And it passed
> > for the checking. About the indentation problems, it may be caused by
> > the sepcified vimrc file.
> 
> I get the following warning on your patch:
> 
> --- cut ---
> $> ./scripts/checkpatch.pl /prace/klp-module-notify-error-v3-iter1.patch
> WARNING: quoted string split across lines
> #181: FILE: kernel/livepatch/core.c:965:
> +                                       pr_warn("patch '%s' is dead, remove it "
> +                                               "or re-install the module '%s'\n",
> 
> total: 0 errors, 1 warnings, 62 lines checked
> 
> /prace/klp-module-notify-error-v3-iter1.patch has style problems, please review.
> 
> If any of these errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
> --- cut ---

Yes. The error message is too long, then it across the lines.

> 
> BTW: I do not see the problems with indentation.

Someone likes to use 4 blanks to instead of a tab. So the code may show
differently between them.

Thanks
Minfei

> 
> Best Regards,
> Petr

Re: [PATCH v3] livepatch: Prevent to apply the patch once coming module notifier fails

[Josh Poimboeuf]

On Mon, May 18, 2015 at 02:08:06PM +0200, Petr Mladek wrote:
> On Wed 2015-05-13 09:14:15, Josh Poimboeuf wrote:
> > On Tue, May 12, 2015 at 10:04:44PM +0800, Minfei Huang wrote:
> > > @@ -930,6 +932,7 @@ disabled:
> > >  static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > >  			     void *data)
> > >  {
> > > +	int ret;
> > >  	struct module *mod = data;
> > >  	struct klp_patch *patch;
> > >  	struct klp_object *obj;
> > > @@ -955,7 +958,13 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > >  
> > >  			if (action == MODULE_STATE_COMING) {
> > >  				obj->mod = mod;
> > > -				klp_module_notify_coming(patch, obj);
> > > +				ret = klp_module_notify_coming(patch, obj);
> > > +				if (ret) {
> > > +					obj->mod = NULL;
> > > +					pr_warn("patch '%s' is dead, remove it "
> > > +						"or re-install the module '%s'\n",
> > > +						patch->mod->name, obj->name);
> > > +				}
> > 
> > The patch isn't necessarily dead, since it might also include previously
> > enabled changes for vmlinux or other modules.  It can actually be a
> > dangerous condition if there's a mismatch between old code in the module
> > and new code elsewhere.  How about something like:
> > 
> > "patch '%s' is in an inconsistent state!\n"
> 
> It must not be dangerous, otherwise the patch could not get applied
> immediately.
>
> I would omit this message completely. It would just duplicate the
> warning printed by klp_module_notify_coming().

This error path doesn't mean that the entire patch isn't applied.  It
only affects the subset of the patch which applies to the coming module.
So you can have a dangerous mismatch in the case of a patch which
patches multiple objects.

-- 
Josh

Re: [PATCH v3] livepatch: Prevent to apply the patch once coming module notifier fails

[Petr Mladek]

On Mon 2015-05-18 10:22:21, Josh Poimboeuf wrote:
> On Mon, May 18, 2015 at 02:08:06PM +0200, Petr Mladek wrote:
> > On Wed 2015-05-13 09:14:15, Josh Poimboeuf wrote:
> > > On Tue, May 12, 2015 at 10:04:44PM +0800, Minfei Huang wrote:
> > > > @@ -930,6 +932,7 @@ disabled:
> > > >  static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > > >  			     void *data)
> > > >  {
> > > > +	int ret;
> > > >  	struct module *mod = data;
> > > >  	struct klp_patch *patch;
> > > >  	struct klp_object *obj;
> > > > @@ -955,7 +958,13 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > > >  
> > > >  			if (action == MODULE_STATE_COMING) {
> > > >  				obj->mod = mod;
> > > > -				klp_module_notify_coming(patch, obj);
> > > > +				ret = klp_module_notify_coming(patch, obj);
> > > > +				if (ret) {
> > > > +					obj->mod = NULL;
> > > > +					pr_warn("patch '%s' is dead, remove it "
> > > > +						"or re-install the module '%s'\n",
> > > > +						patch->mod->name, obj->name);
> > > > +				}
> > > 
> > > The patch isn't necessarily dead, since it might also include previously
> > > enabled changes for vmlinux or other modules.  It can actually be a
> > > dangerous condition if there's a mismatch between old code in the module
> > > and new code elsewhere.  How about something like:
> > > 
> > > "patch '%s' is in an inconsistent state!\n"
> > 
> > It must not be dangerous, otherwise the patch could not get applied
> > immediately.
> >
> > I would omit this message completely. It would just duplicate the
> > warning printed by klp_module_notify_coming().
> 
> This error path doesn't mean that the entire patch isn't applied.  It
> only affects the subset of the patch which applies to the coming module.
> So you can have a dangerous mismatch in the case of a patch which
> patches multiple objects.

We apply the patch immediately. This simple consistency model allows
to call patched function from an upatched one and vice versa. It means
that there must _not_ be any dependency between patched functions.
And it means that it must be safe to keep the module unpatched.

The situation will change after we introduce a more complex
consistency model. Then we will need to patch the module
directly in load_module() and refuse loading in case of error.
By other words, we will not and must not allow any dangerous state.

Does it make sense? Or did I miss anything, please?

Best Regards,
Petr

Re: [PATCH v3] livepatch: Prevent to apply the patch once coming module notifier fails

[Josh Poimboeuf]

On Mon, May 18, 2015 at 05:50:52PM +0200, Petr Mladek wrote:
> On Mon 2015-05-18 10:22:21, Josh Poimboeuf wrote:
> > On Mon, May 18, 2015 at 02:08:06PM +0200, Petr Mladek wrote:
> > > On Wed 2015-05-13 09:14:15, Josh Poimboeuf wrote:
> > > > On Tue, May 12, 2015 at 10:04:44PM +0800, Minfei Huang wrote:
> > > > > @@ -930,6 +932,7 @@ disabled:
> > > > >  static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > > > >  			     void *data)
> > > > >  {
> > > > > +	int ret;
> > > > >  	struct module *mod = data;
> > > > >  	struct klp_patch *patch;
> > > > >  	struct klp_object *obj;
> > > > > @@ -955,7 +958,13 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > > > >  
> > > > >  			if (action == MODULE_STATE_COMING) {
> > > > >  				obj->mod = mod;
> > > > > -				klp_module_notify_coming(patch, obj);
> > > > > +				ret = klp_module_notify_coming(patch, obj);
> > > > > +				if (ret) {
> > > > > +					obj->mod = NULL;
> > > > > +					pr_warn("patch '%s' is dead, remove it "
> > > > > +						"or re-install the module '%s'\n",
> > > > > +						patch->mod->name, obj->name);
> > > > > +				}
> > > > 
> > > > The patch isn't necessarily dead, since it might also include previously
> > > > enabled changes for vmlinux or other modules.  It can actually be a
> > > > dangerous condition if there's a mismatch between old code in the module
> > > > and new code elsewhere.  How about something like:
> > > > 
> > > > "patch '%s' is in an inconsistent state!\n"
> > > 
> > > It must not be dangerous, otherwise the patch could not get applied
> > > immediately.
> > >
> > > I would omit this message completely. It would just duplicate the
> > > warning printed by klp_module_notify_coming().
> > 
> > This error path doesn't mean that the entire patch isn't applied.  It
> > only affects the subset of the patch which applies to the coming module.
> > So you can have a dangerous mismatch in the case of a patch which
> > patches multiple objects.
> 
> We apply the patch immediately. This simple consistency model allows
> to call patched function from an upatched one and vice versa. It means
> that there must _not_ be any dependency between patched functions.
> And it means that it must be safe to keep the module unpatched.
> 
> The situation will change after we introduce a more complex
> consistency model. Then we will need to patch the module
> directly in load_module() and refuse loading in case of error.
> By other words, we will not and must not allow any dangerous state.
> 
> Does it make sense? Or did I miss anything, please?

Yeah, ok, that makes sense.  Given the simple consistency model, it's
not dangerous.  It's still inconsistent, and something the user should
know about, but perhaps a single warning in klp_module_notify_coming()
is enough.

I also agree that, once we have a better consistency model, failing to
load the module would be a better way to handle this error.

-- 
Josh