[PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails

[PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails

[Joe Lawrence]

When an incoming module is considered for livepatching by
klp_module_coming(), it iterates over multiple patches and multiple
kernel objects in this order:

	list_for_each_entry(patch, &klp_patches, list) {
		klp_for_each_object(patch, obj) {

which means that if one of the kernel objects fails to patch,
klp_module_coming()'s error path needs to unpatch and cleanup any kernel
objects that were already patched by a previous patch.

Reported-by: Miroslav Benes <mbenes@suse.cz>
Suggested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
v2:

 - cleanup comment describing the new function
 - s/klp_cleanup_module_objects_limited/klp_cleanup_module_patches_limited
 - added a suggested-by tag for Petr since he suggested both code and
   commentary :)

 kernel/livepatch/core.c | 60 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 23 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index b9628e43c78f..bf8c8fd72589 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -830,6 +830,41 @@ int klp_register_patch(struct klp_patch *patch)
 }
 EXPORT_SYMBOL_GPL(klp_register_patch);
 
+/*
+ * Remove parts of patches that touch a given kernel module. The list of
+ * patches processed might be limited. When limit is NULL, all patches
+ * will be handled.
+ */
+static void klp_cleanup_module_patches_limited(struct module *mod,
+					       struct klp_patch *limit)
+{
+	struct klp_patch *patch;
+	struct klp_object *obj;
+
+	list_for_each_entry(patch, &klp_patches, list) {
+		if (patch == limit)
+			break;
+
+		klp_for_each_object(patch, obj) {
+			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
+				continue;
+
+			/*
+			 * Only unpatch the module if the patch is enabled or
+			 * is in transition.
+			 */
+			if (patch->enabled || patch == klp_transition_patch) {
+				pr_notice("reverting patch '%s' on unloading module '%s'\n",
+					  patch->mod->name, obj->mod->name);
+				klp_unpatch_object(obj);
+			}
+
+			klp_free_object_loaded(obj);
+			break;
+		}
+	}
+}
+
 int klp_module_coming(struct module *mod)
 {
 	int ret;
@@ -894,7 +929,7 @@ int klp_module_coming(struct module *mod)
 	pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
 		patch->mod->name, obj->mod->name, obj->mod->name);
 	mod->klp_alive = false;
-	klp_free_object_loaded(obj);
+	klp_cleanup_module_patches_limited(mod, patch);
 	mutex_unlock(&klp_mutex);
 
 	return ret;
@@ -902,9 +937,6 @@ int klp_module_coming(struct module *mod)
 
 void klp_module_going(struct module *mod)
 {
-	struct klp_patch *patch;
-	struct klp_object *obj;
-
 	if (WARN_ON(mod->state != MODULE_STATE_GOING &&
 		    mod->state != MODULE_STATE_COMING))
 		return;
@@ -917,25 +949,7 @@ void klp_module_going(struct module *mod)
 	 */
 	mod->klp_alive = false;
 
-	list_for_each_entry(patch, &klp_patches, list) {
-		klp_for_each_object(patch, obj) {
-			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
-				continue;
-
-			/*
-			 * Only unpatch the module if the patch is enabled or
-			 * is in transition.
-			 */
-			if (patch->enabled || patch == klp_transition_patch) {
-				pr_notice("reverting patch '%s' on unloading module '%s'\n",
-					  patch->mod->name, obj->mod->name);
-				klp_unpatch_object(obj);
-			}
-
-			klp_free_object_loaded(obj);
-			break;
-		}
-	}
+	klp_cleanup_module_patches_limited(mod, NULL);
 
 	mutex_unlock(&klp_mutex);
 }
-- 
1.8.3.1

Re: [PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails

[Josh Poimboeuf]

On Mon, Oct 02, 2017 at 11:56:48AM -0400, Joe Lawrence wrote:
> When an incoming module is considered for livepatching by
> klp_module_coming(), it iterates over multiple patches and multiple
> kernel objects in this order:
> 
> 	list_for_each_entry(patch, &klp_patches, list) {
> 		klp_for_each_object(patch, obj) {
> 
> which means that if one of the kernel objects fails to patch,
> klp_module_coming()'s error path needs to unpatch and cleanup any kernel
> objects that were already patched by a previous patch.
> 
> Reported-by: Miroslav Benes <mbenes@suse.cz>
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
> v2:
> 
>  - cleanup comment describing the new function
>  - s/klp_cleanup_module_objects_limited/klp_cleanup_module_patches_limited
>  - added a suggested-by tag for Petr since he suggested both code and
>    commentary :)
> 
>  kernel/livepatch/core.c | 60 ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 37 insertions(+), 23 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e43c78f..bf8c8fd72589 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -830,6 +830,41 @@ int klp_register_patch(struct klp_patch *patch)
>  }
>  EXPORT_SYMBOL_GPL(klp_register_patch);
>  
> +/*
> + * Remove parts of patches that touch a given kernel module. The list of
> + * patches processed might be limited. When limit is NULL, all patches
> + * will be handled.
> + */
> +static void klp_cleanup_module_patches_limited(struct module *mod,
> +					       struct klp_patch *limit)

One nit, I think the function name is too verbose.  It already has a
'limit' argument, so I think putting 'limited' in the name is too much
detail.  I would just call it klp_cleanup_module_patches().

Otherwise the patch looks great.  Thanks for fixing it!

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

Re: [PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails

[Josh Poimboeuf]

On Mon, Oct 02, 2017 at 11:56:48AM -0400, Joe Lawrence wrote:
> When an incoming module is considered for livepatching by
> klp_module_coming(), it iterates over multiple patches and multiple
> kernel objects in this order:
> 
> 	list_for_each_entry(patch, &klp_patches, list) {
> 		klp_for_each_object(patch, obj) {
> 
> which means that if one of the kernel objects fails to patch,
> klp_module_coming()'s error path needs to unpatch and cleanup any kernel
> objects that were already patched by a previous patch.
> 
> Reported-by: Miroslav Benes <mbenes@suse.cz>
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
> v2:
> 
>  - cleanup comment describing the new function
>  - s/klp_cleanup_module_objects_limited/klp_cleanup_module_patches_limited
>  - added a suggested-by tag for Petr since he suggested both code and
>    commentary :)

Any more comments on this patch?  This is holding up the callbacks patch
so it would be great if we could get this merged soon.

-- 
Josh

Re: [PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails

[Josh Poimboeuf]

On Thu, Oct 05, 2017 at 04:55:43PM -0500, Josh Poimboeuf wrote:
> On Mon, Oct 02, 2017 at 11:56:48AM -0400, Joe Lawrence wrote:
> > When an incoming module is considered for livepatching by
> > klp_module_coming(), it iterates over multiple patches and multiple
> > kernel objects in this order:
> > 
> > 	list_for_each_entry(patch, &klp_patches, list) {
> > 		klp_for_each_object(patch, obj) {
> > 
> > which means that if one of the kernel objects fails to patch,
> > klp_module_coming()'s error path needs to unpatch and cleanup any kernel
> > objects that were already patched by a previous patch.
> > 
> > Reported-by: Miroslav Benes <mbenes@suse.cz>
> > Suggested-by: Petr Mladek <pmladek@suse.com>
> > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> > ---
> > v2:
> > 
> >  - cleanup comment describing the new function
> >  - s/klp_cleanup_module_objects_limited/klp_cleanup_module_patches_limited
> >  - added a suggested-by tag for Petr since he suggested both code and
> >    commentary :)
> > 
> >  kernel/livepatch/core.c | 60 ++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 37 insertions(+), 23 deletions(-)
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index b9628e43c78f..bf8c8fd72589 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -830,6 +830,41 @@ int klp_register_patch(struct klp_patch *patch)
> >  }
> >  EXPORT_SYMBOL_GPL(klp_register_patch);
> >  
> > +/*
> > + * Remove parts of patches that touch a given kernel module. The list of
> > + * patches processed might be limited. When limit is NULL, all patches
> > + * will be handled.
> > + */
> > +static void klp_cleanup_module_patches_limited(struct module *mod,
> > +					       struct klp_patch *limit)
> 
> One nit, I think the function name is too verbose.  It already has a
> 'limit' argument, so I think putting 'limited' in the name is too much
> detail.  I would just call it klp_cleanup_module_patches().

I rescind my nit.  I forgot that the 'limited' suffix is already a
convention used throughout the livepatch code.  So my ack is
unconditional now.

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

Re: [PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails

[Miroslav Benes]

On Mon, 2 Oct 2017, Joe Lawrence wrote:

> When an incoming module is considered for livepatching by
> klp_module_coming(), it iterates over multiple patches and multiple
> kernel objects in this order:
> 
> 	list_for_each_entry(patch, &klp_patches, list) {
> 		klp_for_each_object(patch, obj) {
> 
> which means that if one of the kernel objects fails to patch,
> klp_module_coming()'s error path needs to unpatch and cleanup any kernel
> objects that were already patched by a previous patch.
> 
> Reported-by: Miroslav Benes <mbenes@suse.cz>
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
> v2:
> 
>  - cleanup comment describing the new function
>  - s/klp_cleanup_module_objects_limited/klp_cleanup_module_patches_limited
>  - added a suggested-by tag for Petr since he suggested both code and
>    commentary :)
> 
>  kernel/livepatch/core.c | 60 ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 37 insertions(+), 23 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e43c78f..bf8c8fd72589 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -830,6 +830,41 @@ int klp_register_patch(struct klp_patch *patch)
>  }
>  EXPORT_SYMBOL_GPL(klp_register_patch);
>  
> +/*
> + * Remove parts of patches that touch a given kernel module. The list of
> + * patches processed might be limited. When limit is NULL, all patches
> + * will be handled.
> + */
> +static void klp_cleanup_module_patches_limited(struct module *mod,
> +					       struct klp_patch *limit)
> +{
> +	struct klp_patch *patch;
> +	struct klp_object *obj;
> +
> +	list_for_each_entry(patch, &klp_patches, list) {
> +		if (patch == limit)
> +			break;
> +
> +		klp_for_each_object(patch, obj) {
> +			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
> +				continue;
> +
> +			/*
> +			 * Only unpatch the module if the patch is enabled or
> +			 * is in transition.
> +			 */
> +			if (patch->enabled || patch == klp_transition_patch) {
> +				pr_notice("reverting patch '%s' on unloading module '%s'\n",
> +					  patch->mod->name, obj->mod->name);
> +				klp_unpatch_object(obj);
> +			}
> +
> +			klp_free_object_loaded(obj);
> +			break;
> +		}
> +	}
> +}
> +
>  int klp_module_coming(struct module *mod)
>  {
>  	int ret;
> @@ -894,7 +929,7 @@ int klp_module_coming(struct module *mod)
>  	pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
>  		patch->mod->name, obj->mod->name, obj->mod->name);
>  	mod->klp_alive = false;
> -	klp_free_object_loaded(obj);
> +	klp_cleanup_module_patches_limited(mod, patch);
>  	mutex_unlock(&klp_mutex);
>  
>  	return ret;
> @@ -902,9 +937,6 @@ int klp_module_coming(struct module *mod)
>  
>  void klp_module_going(struct module *mod)
>  {
> -	struct klp_patch *patch;
> -	struct klp_object *obj;
> -
>  	if (WARN_ON(mod->state != MODULE_STATE_GOING &&
>  		    mod->state != MODULE_STATE_COMING))
>  		return;
> @@ -917,25 +949,7 @@ void klp_module_going(struct module *mod)
>  	 */
>  	mod->klp_alive = false;
>  
> -	list_for_each_entry(patch, &klp_patches, list) {
> -		klp_for_each_object(patch, obj) {
> -			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
> -				continue;
> -
> -			/*
> -			 * Only unpatch the module if the patch is enabled or
> -			 * is in transition.
> -			 */
> -			if (patch->enabled || patch == klp_transition_patch) {
> -				pr_notice("reverting patch '%s' on unloading module '%s'\n",
> -					  patch->mod->name, obj->mod->name);
> -				klp_unpatch_object(obj);
> -			}
> -
> -			klp_free_object_loaded(obj);
> -			break;
> -		}
> -	}
> +	klp_cleanup_module_patches_limited(mod, NULL);
>  
>  	mutex_unlock(&klp_mutex);
>  }

Well, I don't know. I like the idea of reusing the code a lot, but it 
feels odd not to use list_for_each_entry_{continue,from}_reverse() 
iterator. And I'm not talking about _reverse there (more on that later). 
That continue part gives us limited functionality for free. We cannot do 
the same in klp_free_funcs_limited(), because klp_funcs form an array. It 
is not a list.

On the other hand, the code would be slightly more complicated, because 
only the inner part of the loop could be reused.

Now about _reverse. I don't know about that either. The module's code is 
not used yet when klp_module_coming() is called (or in 
klp_module_going()). So it is probable that the order does not matter at 
all. But it would be the correct way to do it.

To sum it up, I'm able to live with the proposed approach if that's the 
consensus, because I haven't managed to convince myself that my proposal 
would be better.

Thanks,
Miroslav

Re: [PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails

[Josh Poimboeuf]

On Mon, Oct 09, 2017 at 04:49:29PM +0200, Miroslav Benes wrote:
> On Mon, 2 Oct 2017, Joe Lawrence wrote:
> 
> > When an incoming module is considered for livepatching by
> > klp_module_coming(), it iterates over multiple patches and multiple
> > kernel objects in this order:
> > 
> > 	list_for_each_entry(patch, &klp_patches, list) {
> > 		klp_for_each_object(patch, obj) {
> > 
> > which means that if one of the kernel objects fails to patch,
> > klp_module_coming()'s error path needs to unpatch and cleanup any kernel
> > objects that were already patched by a previous patch.
> > 
> > Reported-by: Miroslav Benes <mbenes@suse.cz>
> > Suggested-by: Petr Mladek <pmladek@suse.com>
> > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> > ---
> > v2:
> > 
> >  - cleanup comment describing the new function
> >  - s/klp_cleanup_module_objects_limited/klp_cleanup_module_patches_limited
> >  - added a suggested-by tag for Petr since he suggested both code and
> >    commentary :)
> > 
> >  kernel/livepatch/core.c | 60 ++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 37 insertions(+), 23 deletions(-)
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index b9628e43c78f..bf8c8fd72589 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -830,6 +830,41 @@ int klp_register_patch(struct klp_patch *patch)
> >  }
> >  EXPORT_SYMBOL_GPL(klp_register_patch);
> >  
> > +/*
> > + * Remove parts of patches that touch a given kernel module. The list of
> > + * patches processed might be limited. When limit is NULL, all patches
> > + * will be handled.
> > + */
> > +static void klp_cleanup_module_patches_limited(struct module *mod,
> > +					       struct klp_patch *limit)
> > +{
> > +	struct klp_patch *patch;
> > +	struct klp_object *obj;
> > +
> > +	list_for_each_entry(patch, &klp_patches, list) {
> > +		if (patch == limit)
> > +			break;
> > +
> > +		klp_for_each_object(patch, obj) {
> > +			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
> > +				continue;
> > +
> > +			/*
> > +			 * Only unpatch the module if the patch is enabled or
> > +			 * is in transition.
> > +			 */
> > +			if (patch->enabled || patch == klp_transition_patch) {
> > +				pr_notice("reverting patch '%s' on unloading module '%s'\n",
> > +					  patch->mod->name, obj->mod->name);
> > +				klp_unpatch_object(obj);
> > +			}
> > +
> > +			klp_free_object_loaded(obj);
> > +			break;
> > +		}
> > +	}
> > +}
> > +
> >  int klp_module_coming(struct module *mod)
> >  {
> >  	int ret;
> > @@ -894,7 +929,7 @@ int klp_module_coming(struct module *mod)
> >  	pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
> >  		patch->mod->name, obj->mod->name, obj->mod->name);
> >  	mod->klp_alive = false;
> > -	klp_free_object_loaded(obj);
> > +	klp_cleanup_module_patches_limited(mod, patch);
> >  	mutex_unlock(&klp_mutex);
> >  
> >  	return ret;
> > @@ -902,9 +937,6 @@ int klp_module_coming(struct module *mod)
> >  
> >  void klp_module_going(struct module *mod)
> >  {
> > -	struct klp_patch *patch;
> > -	struct klp_object *obj;
> > -
> >  	if (WARN_ON(mod->state != MODULE_STATE_GOING &&
> >  		    mod->state != MODULE_STATE_COMING))
> >  		return;
> > @@ -917,25 +949,7 @@ void klp_module_going(struct module *mod)
> >  	 */
> >  	mod->klp_alive = false;
> >  
> > -	list_for_each_entry(patch, &klp_patches, list) {
> > -		klp_for_each_object(patch, obj) {
> > -			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
> > -				continue;
> > -
> > -			/*
> > -			 * Only unpatch the module if the patch is enabled or
> > -			 * is in transition.
> > -			 */
> > -			if (patch->enabled || patch == klp_transition_patch) {
> > -				pr_notice("reverting patch '%s' on unloading module '%s'\n",
> > -					  patch->mod->name, obj->mod->name);
> > -				klp_unpatch_object(obj);
> > -			}
> > -
> > -			klp_free_object_loaded(obj);
> > -			break;
> > -		}
> > -	}
> > +	klp_cleanup_module_patches_limited(mod, NULL);
> >  
> >  	mutex_unlock(&klp_mutex);
> >  }
> 
> Well, I don't know. I like the idea of reusing the code a lot, but it 
> feels odd not to use list_for_each_entry_{continue,from}_reverse() 
> iterator. And I'm not talking about _reverse there (more on that later). 
> That continue part gives us limited functionality for free. We cannot do 
> the same in klp_free_funcs_limited(), because klp_funcs form an array. It 
> is not a list.
> 
> On the other hand, the code would be slightly more complicated, because 
> only the inner part of the loop could be reused.
> 
> Now about _reverse. I don't know about that either. The module's code is 
> not used yet when klp_module_coming() is called (or in 
> klp_module_going()). So it is probable that the order does not matter at 
> all. But it would be the correct way to do it.
> 
> To sum it up, I'm able to live with the proposed approach if that's the 
> consensus, because I haven't managed to convince myself that my proposal 
> would be better.

As you said, the order doesn't matter because the code isn't runnable.
And doing it this way allows us to share more code.  So I like the patch
as it is.

-- 
Josh

Re: [PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails

[Petr Mladek]

On Mon 2017-10-02 11:56:48, Joe Lawrence wrote:
> When an incoming module is considered for livepatching by
> klp_module_coming(), it iterates over multiple patches and multiple
> kernel objects in this order:
> 
> 	list_for_each_entry(patch, &klp_patches, list) {
> 		klp_for_each_object(patch, obj) {
> 
> which means that if one of the kernel objects fails to patch,
> klp_module_coming()'s error path needs to unpatch and cleanup any kernel
> objects that were already patched by a previous patch.
> 
> Reported-by: Miroslav Benes <mbenes@suse.cz>
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>

Looks good to me.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails

[Jiri Kosina]

On Mon, 2 Oct 2017, Joe Lawrence wrote:

> When an incoming module is considered for livepatching by
> klp_module_coming(), it iterates over multiple patches and multiple
> kernel objects in this order:
> 
> 	list_for_each_entry(patch, &klp_patches, list) {
> 		klp_for_each_object(patch, obj) {
> 
> which means that if one of the kernel objects fails to patch,
> klp_module_coming()'s error path needs to unpatch and cleanup any kernel
> objects that were already patched by a previous patch.
> 
> Reported-by: Miroslav Benes <mbenes@suse.cz>
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
> v2:
> 
>  - cleanup comment describing the new function
>  - s/klp_cleanup_module_objects_limited/klp_cleanup_module_patches_limited
>  - added a suggested-by tag for Petr since he suggested both code and
>    commentary :)

Appplied to for-4.14/upstream-fixes. Thanks,

-- 
Jiri Kosina
SUSE Labs