+++ Petr Mladek [07/06/16 11:36 +0200]:
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 <mbenes@xxxxxxx> --- 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.
Hi Petr, I'm a bit unclear on this, can you clarify what you mean by initialize patch->list? I don't think any extra initialization is needed here to be able use a patch->list node with an existing list (klp_patches), since this field is embedded and the klp_patches list header is already statically initialized with LIST_HEAD. Jessica -- To unsubscribe from this list: send the line "unsubscribe live-patching" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
