On Wed 2016-06-01 10:31:59, Miroslav Benes wrote:
> Currently we do not allow patch module to unload since there is no
> method to determine if a task is still running in the patched code.
>
> The consistency model gives us the way because when the unpatching
> finishes we know that all tasks were marked as safe to call an original
> function. Thus every new call to the function calls the original code
> and at the same time no task can be somewhere in the patched code,
> because it had to leave that code to be marked as safe.
>
> We can safely let the patch module go after that.
>
> Completion is used for synchronization between module removal and sysfs
> infrastructure in a similar way to commit 942e443127e9 ("module: Fix
> mod->mkobj.kobj potentially freed too early").
>
> Note that we still do not allow the removal for immediate model, that is
> no consistency model. The module refcount may increase in this case if
> somebody disables and enables the patch several times. This should not
> cause any harm.
>
> With this change a call to try_module_get() is moved to
> __klp_enable_patch from klp_register_patch to make module reference
> counting symmetric (module_put() is in a patch disable path) and to
> allow to take a new reference to a disabled module when being enabled.
>
> Also all kobject_put(&patch->kobj) calls are moved outside of klp_mutex
> lock protection to prevent a deadlock situation when
> klp_unregister_patch is called and sysfs directories are removed. There
> is no need to do the same for other kobject_put() callsites as we
> currently do not have their sysfs counterparts.
>
> Signed-off-by: Miroslav Benes <mbe...@suse.cz>
> ---
> Based on Josh's v2 of the consistency model.
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index d55701222b49..a649186fb4af 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -459,6 +472,15 @@ static ssize_t enabled_store(struct kobject *kobj,
> struct kobj_attribute *attr,
>
> mutex_lock(&klp_mutex);
>
> + if (!klp_is_patch_registered(patch)) {
> + /*
> + * Module with the patch could either disappear meanwhile or is
> + * not properly initialized yet.
> + */
> + ret = -EINVAL;
> + goto err;
> + }
> +
> if (patch->enabled == val) {
> /* already in requested state */
> ret = -EINVAL;
> @@ -700,11 +721,14 @@ static int klp_init_patch(struct klp_patch *patch)
> mutex_lock(&klp_mutex);
>
> patch->enabled = false;
> + init_completion(&patch->finish);
>
> ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
> klp_root_kobj, "%s", patch->mod->name);
> - if (ret)
> - goto unlock;
> + if (ret) {
> + mutex_unlock(&klp_mutex);
> + return ret;
> + }
>
> klp_for_each_object(patch, obj) {
> ret = klp_init_object(patch, obj);
> @@ -720,9 +744,12 @@ static int klp_init_patch(struct klp_patch *patch)
>
> free:
> klp_free_objects_limited(patch, obj);
> - kobject_put(&patch->kobj);
> -unlock:
> +
> mutex_unlock(&klp_mutex);
> +
Just for record. The sysfs entry exists but patch->list is not
initialized in this error path. Therefore we could write into
/sys/.../livepatch_foo/enable
and call enabled_store(). It is safe because enabled_store() calls
klp_is_patch_registered() and it does not need patch->list for this
check. Kudos for the klp_is_patch_registered() implementation.
I write this because it is not obvious and it took me some time
to verify that we are on the safe side.
Well, I would feel more comfortable if we initialize patch->list
above.
> + kobject_put(&patch->kobj);
> + wait_for_completion(&patch->finish);
> +
> return ret;
> }
>
> diff --git a/samples/livepatch/livepatch-sample.c
> b/samples/livepatch/livepatch-sample.c
> index e34f871e69b1..a84676aa7c62 100644
> --- a/samples/livepatch/livepatch-sample.c
> +++ b/samples/livepatch/livepatch-sample.c
> @@ -82,7 +82,6 @@ static int livepatch_init(void)
>
> static void livepatch_exit(void)
> {
> - WARN_ON(klp_disable_patch(&patch));
> WARN_ON(klp_unregister_patch(&patch));
Just for record. I was curious if the following race:
CPU0 CPU1
rmmod livepatch_foo
livepatch_exit()
echo 1> /sys/.../livepatch_foo/enable
__klp_enable_patch()
lock(&klp_mutex)
...
patch->enabled = true;
...
unlock(&klp_mutex)
klp_unregister_patch()
lock(&klp_mutex);
if (patch->enabled)
ret = -EBUSY;
unlock(&klp_mutex);
Fortunately, this is not possible. livepatch_exit() is called when
the module is in MODULE_STATE_GOING. It means that try_module_get()
will fail and therefore __klp_enable_patch() will fail.
All in all, the patch looks good to me. I am fine with the completion.
In fact, I like it.
Best Regards,
Petr
