Re: livepatch/module: remove livepatch module notifier

[Jessica Yu]

+++ Petr Mladek [14/03/16 16:06 +0100]:
> On Fri 2016-03-11 15:03:48, Jessica Yu wrote:
> > Remove the livepatch module notifier in favor of directly enabling and
> > disabling patches to modules in the module loader. Hard-coding the
> > function calls ensures that ftrace_module_enable() is run before
> > klp_module_coming() during module load, and that klp_module_going() is
> > run before ftrace_release_mod() during module unload. This way, ftrace
> > and livepatch code is run in the correct order during the module
> > load/unload sequence without dependence on the module notifier call chain.
> >
> > Signed-off-by: Jessica Yu <jeyu@xxxxxxxxxx>
> > Acked-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> > Acked-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
> > ---
> >  include/linux/livepatch.h |  13 ++++
> >  kernel/livepatch/core.c   | 147 ++++++++++++++++++++++------------------------
> >  kernel/module.c           |  10 ++++
> >  3 files changed, 94 insertions(+), 76 deletions(-)
> >
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 780f00c..4fafbae 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -866,103 +866,108 @@ int klp_register_patch(struct klp_patch *patch)
> >  }
> >  EXPORT_SYMBOL_GPL(klp_register_patch);
> >
> > -static int klp_module_notify_coming(struct klp_patch *patch,
> > -				     struct klp_object *obj)
> > +int klp_module_coming(struct module *mod)
> >  {
> > -	struct module *pmod = patch->mod;
> > -	struct module *mod = obj->mod;
> >  	int ret;
> > +	struct klp_patch *patch;
> > +	struct klp_object *obj;
> >
> > -	ret = klp_init_object_loaded(patch, obj);
> > -	if (ret) {
> > -		pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
> > -			pmod->name, mod->name, ret);
> > -		return ret;
> > -	}
> > +	if (WARN_ON(mod->state != MODULE_STATE_COMING))
> > +		return -EINVAL;
> >
> > -	if (patch->state == KLP_DISABLED)
> > -		return 0;
> > +	mutex_lock(&klp_mutex);
> > +	/*
> > +	 * Each module has to know that klp_module_coming()
> > +	 * has been called. We never know what module will
> > +	 * get patched by a new patch.
> > +	 */
> > +	mod->klp_alive = true;
> >
> > -	pr_notice("applying patch '%s' to loading module '%s'\n",
> > -		  pmod->name, mod->name);
> > +	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;
> >
> > -	ret = klp_enable_object(obj);
> > -	if (ret)
> > -		pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > -			pmod->name, mod->name, ret);
> > -	return ret;
> > -}
> > +			obj->mod = mod;
> >
> > -static void klp_module_notify_going(struct klp_patch *patch,
> > -				    struct klp_object *obj)
> > -{
> > -	struct module *pmod = patch->mod;
> > -	struct module *mod = obj->mod;
> > +			ret = klp_init_object_loaded(patch, obj);
> > +			if (ret) {
> > +				pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
> > +					patch->mod->name, obj->mod->name, ret);
> > +				goto err;
> > +			}
> >
> > -	if (patch->state == KLP_DISABLED)
> > -		goto disabled;
> > +			if (patch->state == KLP_DISABLED)
> > +				break;
> > +
> > +			pr_notice("applying patch '%s' to loading module '%s'\n",
> > +				  patch->mod->name, obj->mod->name);
> > +
> > +			ret = klp_enable_object(obj);
> > +			if (ret) {
> > +				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > +					patch->mod->name, obj->mod->name, ret);
> > +				goto err;
> > +			}
> > +
> > +			break;
> > +		}
> > +	}
> >
> > -	pr_notice("reverting patch '%s' on unloading module '%s'\n",
> > -		  pmod->name, mod->name);
> > +	mutex_unlock(&klp_mutex);
> >
> > -	klp_disable_object(obj);
> > +	return 0;
> >
> > -disabled:
> > +err:
> > +	/*
> > +	 * If a patch is unsuccessfully applied, return
> > +	 * error to the module loader.
> > +	 */
> > +	pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
> > +		patch->mod->name, obj->mod->name, obj->mod->name);
>
> One more thing. We should add here:
>
> 	mod->klp_alive = true;
>
> Otherwise, there is a small race window when a new patch will try to
> patch the module.

Ah, you are right. I think you meant false, though :-) Will fix that.

> >  	klp_free_object_loaded(obj);
> > +	mutex_unlock(&klp_mutex);
> > +
> > +	return ret;
> >  }
>
> With the above fix:
>
> Reviewed-by: Petr Mladek <pmladek@xxxxxxx>
>
>
> Best Regards,
> Petr