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

[Miroslav Benes <mbenes@suse.cz>]

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