Re: [PATCH v5 13/15] livepatch: change to a per-task consistency model

On Thu, Feb 16, 2017 at 03:33:26PM +0100, Miroslav Benes wrote:

quoted

@@ -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;
+		}

[...]

quoted

+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);
 }
 
 /*