On Thu, Feb 16, 2017 at 03:33:26PM +0100, Miroslav Benes wrote:
>
> > @@ -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?
Good catch. I agree that 15/15 should have something like that.
Also, the module_put() will be needed for non-immediate patches which
have a func->immediate set.
What do you think about the following? I tried to put the logic in
klp_complete_transition(), so the module_put()'s would be in one place.
But it was too messy, so I put it in klp_cancel_transition() instead.
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index e96346e..bd1c1fd 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -121,8 +121,28 @@ static void klp_complete_transition(void)
*/
void klp_cancel_transition(void)
{
+ bool immediate_func = false;
+
klp_target_state = !klp_target_state;
klp_complete_transition();
+
+ if (klp_target_state == KLP_PATCHED)
+ return;
+
+ /*
+ * In the enable error path, even immediate patches can be safely
+ * removed because the transition hasn't been started yet.
+ *
+ * klp_complete_transition() doesn't have a module_put() for immediate
+ * patches, so do it here.
+ */
+ klp_for_each_object(klp_transition_patch, obj)
+ klp_for_each_func(obj, func)
+ if (func->immediate)
+ immediate_func = true;
+
+ if (klp_transition_patch->immediate || immediate_func)
+ module_put(klp_transition_patch->mod);
}
/*
--
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
