On Fri, 29 Jan 2016, Jessica Yu wrote:
> Detangle klp_module_notify() into two separate module notifiers
> klp_module_notify_{coming,going}() with the appropriate priorities,
> so that on module load, the ftrace module notifier will run *before*
> the livepatch coming module notifier but *after* the livepatch going
> module modifier.
>
> This fixes a notifier ordering issue in which the ftrace module notifier
> (and hence ftrace_module_enable()) for COMING modules was being called
> after klp_module_notify(), which caused livepatch modules to initialize
> incorrectly.
>
> Signed-off-by: Jessica Yu <j...@redhat.com>
Hi,
I don't know what the outcome of the discussion would be, but few comments
on the patch nevertheless.
> kernel/livepatch/core.c | 128
> +++++++++++++++++++++++++++---------------------
> 1 file changed, 73 insertions(+), 55 deletions(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index bc2c85c..23f4234 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -866,60 +866,73 @@ 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)
> +static int klp_module_notify_coming(struct notifier_block *nb,
> + unsigned long action, void *data)
> {
> - struct module *pmod = patch->mod;
> - struct module *mod = obj->mod;
> int ret;
> + struct module *mod = data;
> + 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 (patch->state == KLP_DISABLED)
> + if (action != MODULE_STATE_COMING)
> return 0;
>
> - pr_notice("applying patch '%s' to loading module '%s'\n",
> - pmod->name, mod->name);
> + mutex_lock(&klp_mutex);
>
> - 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;
> -}
> + /*
> + * Each module has to know that the notifier has been called.
> + * We never know what module will get patched by a new patch.
> + */
> + mod->klp_alive = true;
>
> -static void klp_module_notify_going(struct klp_patch *patch,
> - struct klp_object *obj)
> -{
> - struct module *pmod = patch->mod;
> - struct module *mod = obj->mod;
> + 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;
> +
> + obj->mod = mod;
>
> - if (patch->state == KLP_DISABLED)
> - goto disabled;
> + 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;
> + }
>
> - pr_notice("reverting patch '%s' on unloading module '%s'\n",
> - pmod->name, mod->name);
> + if (patch->state == KLP_DISABLED)
> + break;
>
> - klp_disable_object(obj);
> + pr_notice("applying patch '%s' to loading module
> '%s'\n",
> + patch->mod->name, obj->mod->name);
>
> -disabled:
> - klp_free_object_loaded(obj);
> + 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);
> +err:
> + if (ret) {
> + obj->mod = NULL;
> + pr_warn("patch '%s' is in an inconsistent
> state!\n",
> + patch->mod->name);
> + }
> +
> + break;
> + }
> + }
> +
> + mutex_unlock(&klp_mutex);
> +
> + return 0;
> }
>
> -static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> - void *data)
> +static int klp_module_notify_going(struct notifier_block *nb,
> + unsigned long action, void *data)
> {
> - int ret;
> struct module *mod = data;
> struct klp_patch *patch;
> struct klp_object *obj;
>
> - if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING)
> + if (action != MODULE_STATE_GOING)
> return 0;
>
> mutex_lock(&klp_mutex);
> @@ -928,27 +941,22 @@ static int klp_module_notify(struct notifier_block *nb,
> unsigned long action,
> * Each module has to know that the notifier has been called.
> * We never know what module will get patched by a new patch.
> */
> - if (action == MODULE_STATE_COMING)
> - mod->klp_alive = true;
> - else /* MODULE_STATE_GOING */
> - mod->klp_alive = false;
> + 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;
>
> - if (action == MODULE_STATE_COMING) {
> - obj->mod = mod;
> - 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);
> + if (patch->state == KLP_DISABLED)
> + goto disabled;
> +
> + pr_notice("reverting patch '%s' on unloading module
> '%s'\n",
> + patch->mod->name, obj->mod->name);
>
> + klp_disable_object(obj);
> +disabled:
> + klp_free_object_loaded(obj);
> break;
> }
> }
As for the correctness both notifiers look ok, but I must admit I don't
like the resulting code much. There is no need for 'disabled' label in the
last hunk. I think the same could be achieved with the condition only (and
it applies to the original klp_module_notify_going as well). I guess the
similar could be done to klp_module_notify_coming. However it is a matter
of taste.
> @@ -958,9 +966,14 @@ static int klp_module_notify(struct notifier_block *nb,
> unsigned long action,
> return 0;
> }
>
> -static struct notifier_block klp_module_nb = {
> - .notifier_call = klp_module_notify,
> - .priority = INT_MIN+1, /* called late but before ftrace notifier */
> +static struct notifier_block klp_module_nb_coming = {
> + .notifier_call = klp_module_notify_coming,
> + .priority = INT_MIN, /* called late but after ftrace (coming) notifier
> */
> +};
> +
> +static struct notifier_block klp_module_nb_going = {
> + .notifier_call = klp_module_notify_going,
> + .priority = INT_MIN+2, /* called late but before ftrace (going)
> notifier */
> };
>
> static int __init klp_init(void)
> @@ -973,7 +986,11 @@ static int __init klp_init(void)
> return -EINVAL;
> }
>
> - ret = register_module_notifier(&klp_module_nb);
> + ret = register_module_notifier(&klp_module_nb_coming);
> + if (ret)
> + return ret;
> +
> + ret = register_module_notifier(&klp_module_nb_going);
> if (ret)
> return ret;
There is klp_module_nb_coming already registered so in case of error we
need to unregister it. Maybe goto and two different error labels below?
Otherwise than that it looks good. I agree there are advantages to split
the notifiers. For example we can replace the coming one with the function
call somewhere in load_module() to improve error handling if the patching
fails while loading a module. This would be handy with a consistency model
in the future.
Cheers,
Miroslav
>
> @@ -986,7 +1003,8 @@ static int __init klp_init(void)
> return 0;
>
> unregister:
> - unregister_module_notifier(&klp_module_nb);
> + unregister_module_notifier(&klp_module_nb_coming);
> + unregister_module_notifier(&klp_module_nb_going);
> return ret;
> }
