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

[PATCH v4] 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>
---
v3:
- modify the code style
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 | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 284e269..d4603e7 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 patch '%s' for module '%s' (%d)\n",
+			pmod->name, mod->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,12 @@ 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 in an inconsistent state!\n",
+						patch->mod->name);
+				}
 			} else /* MODULE_STATE_GOING */
 				klp_module_notify_going(patch, obj);
 
-- 
2.2.2

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

[Josh Poimboeuf]

On Thu, May 14, 2015 at 09:51:07AM +0800, Minfei Huang wrote:
> 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>
> ---
> v3:
> - modify the code style
> 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 | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 284e269..d4603e7 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 patch '%s' for module '%s' (%d)\n",
> +			pmod->name, mod->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;

One more minor comment: the out label isn't needed.  Instead of "goto
out", they can just return directly.

Other than that, it looks good to me.

Thanks!

-- 
Josh

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

[Miroslav Benes]

Hi,

I have few nitpicks...

The subject is slightly misleading. We still apply the patch (or the patch 
is already applied to be precise). Only the coming module is not patched 
and won't be patched. So I propose something like

livepatch: prevent patch inconsistencies if the coming module notifier fails 

(or bugs, corruptions, whatever).

On Thu, 14 May 2015, Minfei Huang wrote:

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

", there will be something wrong with the ftrace."

I would improve that...

", ftrace will reject to register the handler because of wrong address or 
will register the handler for wrong address." But feel free to change it 
according to your view. Just be more specific than the changelog is right
now.

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

s/enable/enabled/

But I think it would be nice to describe different behaviours for the sake 
of the changelog. I don't have strong opinion about this though.

> 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>
> ---
> v3:
> - modify the code style
> 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 | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 284e269..d4603e7 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 patch '%s' for module '%s' (%d)\n",
> +			pmod->name, mod->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,12 @@ 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 in an inconsistent state!\n",
> +						patch->mod->name);
> +				}
>  			} else /* MODULE_STATE_GOING */
>  				klp_module_notify_going(patch, obj);
>  

Otherwise it looks ok. Please fix the minor issue Josh pointed out, 
consider fixing the things mentioned above and respin.

Thanks a lot
Miroslav

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

[Minfei Huang]

On 05/14/15 at 09:30am, Josh Poimboeuf wrote:
> On Thu, May 14, 2015 at 09:51:07AM +0800, Minfei Huang wrote:
> > @@ -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 patch '%s' for module '%s' (%d)\n",
> > +			pmod->name, mod->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;
> 
> One more minor comment: the out label isn't needed.  Instead of "goto
> out", they can just return directly.

Ok, I will remove the label "out" in the next version.

Thanks
Minfei

> 
> Other than that, it looks good to me.
> 
> Thanks!
> 
> -- 
> Josh

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

[Minfei Huang]

On 05/14/15 at 05:05pm, Miroslav Benes wrote:
> 
> Hi,
> 
> I have few nitpicks...
> 
> The subject is slightly misleading. We still apply the patch (or the patch 
> is already applied to be precise). Only the coming module is not patched 
> and won't be patched. So I propose something like
> 
> livepatch: prevent patch inconsistencies if the coming module notifier fails 
> 
> (or bugs, corruptions, whatever).

Will do.

> 
> On Thu, 14 May 2015, Minfei Huang wrote:
> 
> > 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.
> 
> ", there will be something wrong with the ftrace."
> 
> I would improve that...
> 
> ", ftrace will reject to register the handler because of wrong address or 
> will register the handler for wrong address." But feel free to change it 
> according to your view. Just be more specific than the changelog is right
> now.
> 

Thanks


> > 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.
> 
> s/enable/enabled/

Will correct it.
> 
> But I think it would be nice to describe different behaviours for the sake 
> of the changelog. I don't have strong opinion about this though.
> 

Thanks
Minfei


> > 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>
> > ---
> > v3:
> > - modify the code style
> > 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 | 30 +++++++++++++++++++-----------
> >  1 file changed, 19 insertions(+), 11 deletions(-)
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 284e269..d4603e7 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 patch '%s' for module '%s' (%d)\n",
> > +			pmod->name, mod->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,12 @@ 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 in an inconsistent state!\n",
> > +						patch->mod->name);
> > +				}
> >  			} else /* MODULE_STATE_GOING */
> >  				klp_module_notify_going(patch, obj);
> >  
> 
> Otherwise it looks ok. Please fix the minor issue Josh pointed out, 
> consider fixing the things mentioned above and respin.
> 
> Thanks a lot
> Miroslav