> @@ -347,22 +356,36 @@ static int __klp_enable_patch(struct klp_patch *patch)
>
> pr_notice("enabling patch '%s'\n", patch->mod->name);
>
> + klp_init_transition(patch, KLP_PATCHED);
> +
> + /*
> + * Enforce the order of the func->transition writes in
> + * klp_init_transition() and the ops->func_stack writes in
> + * klp_patch_object(), so that klp_ftrace_handler() will see the
> + * func->transition updates before the handler is registered and the
> + * new funcs become visible to the handler.
> + */
> + smp_wmb();
> +
> klp_for_each_object(patch, obj) {
> if (!klp_is_object_loaded(obj))
> continue;
>
> ret = klp_patch_object(obj);
> - if (ret)
> - goto unregister;
> + if (ret) {
> + pr_warn("failed to enable patch '%s'\n",
> + patch->mod->name);
> +
> + klp_cancel_transition();
> + return ret;
> + }
[...]
> +static void klp_complete_transition(void)
> +{
> + struct klp_object *obj;
> + struct klp_func *func;
> + struct task_struct *g, *task;
> + unsigned int cpu;
> +
> + if (klp_target_state == KLP_UNPATCHED) {
> + /*
> + * All tasks have transitioned to KLP_UNPATCHED so we can now
> + * remove the new functions from the func_stack.
> + */
> + klp_unpatch_objects(klp_transition_patch);
> +
> + /*
> + * Make sure klp_ftrace_handler() can no longer see functions
> + * from this patch on the ops->func_stack. Otherwise, after
> + * func->transition gets cleared, the handler may choose a
> + * removed function.
> + */
> + synchronize_rcu();
> + }
> +
> + if (klp_transition_patch->immediate)
> + goto done;
> +
> + klp_for_each_object(klp_transition_patch, obj)
> + klp_for_each_func(obj, func)
> + func->transition = false;
> +
> + /* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
> + if (klp_target_state == KLP_PATCHED)
> + synchronize_rcu();
> +
> + read_lock(&tasklist_lock);
> + for_each_process_thread(g, task) {
> + WARN_ON_ONCE(test_tsk_thread_flag(task, TIF_PATCH_PENDING));
> + task->patch_state = KLP_UNDEFINED;
> + }
> + read_unlock(&tasklist_lock);
> +
> + for_each_possible_cpu(cpu) {
> + task = idle_task(cpu);
> + WARN_ON_ONCE(test_tsk_thread_flag(task, TIF_PATCH_PENDING));
> + task->patch_state = KLP_UNDEFINED;
> + }
> +
> +done:
> + klp_target_state = KLP_UNDEFINED;
> + klp_transition_patch = NULL;
> +}
> +
> +/*
> + * This is called in the error path, to cancel a transition before it has
> + * started, i.e. klp_init_transition() has been called but
> + * klp_start_transition() hasn't. If the transition *has* been started,
> + * klp_reverse_transition() should be used instead.
> + */
> +void klp_cancel_transition(void)
> +{
> + klp_target_state = !klp_target_state;
> + klp_complete_transition();
> +}
If we fail to enable patch in __klp_enable_patch(), we call
klp_cancel_transition() and get to klp_complete_transition(). If the patch
has immediate set to true, the module would not be allowed to go (the
changes are in the last patch unfortunately, but my remark is closely
related to klp_cancel_transition() and __klp_enable_patch()). This could
annoy a user if it was due to a trivial reason. So we could call
module_put() in this case. It should be safe as no task could be in a new
function thanks to klp_ftrace_handler() logic.
Pity I have not spotted this earlier.
Putting module_put(patch->mod) right after klp_cancel_transition() call in
__klp_enable_patch() would be the simplest fix (as a part of 15/15 patch).
Maybe with a comment that it is safe to do it there.
What do you think?
Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-s390" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
