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

[Josh Poimboeuf <jpoimboe@redhat.com>]

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