On Fri, Feb 17, 2017 at 09:51:29AM +0100, Miroslav Benes wrote:

quoted

On Thu, 16 Feb 2017, Josh Poimboeuf wrote:

quoted

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;

This is not needed, I think. We call klp_cancel_transition() in 
__klp_enable_patch() only. klp_target_state is set to KLP_PATCHED there 
(through klp_init_transition()) and negated here. We know it must be 
KLP_UNPATCHED.

Yeah, I was trying to hedge against the possibility of future code
calling this function in the disable path.  But that probably won't
happen and it would probably be cleaner to just add a warning if
klp_target_state isn't KLP_PATCHED.

quoted

Moreover, due to klp_complete_transition() klp_target_state is always 
KLP_UNDEFINED after it.

quoted

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

Almost correct. The only problem is that klp_transition_patch is NULL at 
this point. klp_complete_transition() does that and it should stay there 
in my opinion to keep it simple.

So we could either move all this to __klp_enable_patch(), where patch 
variable is defined, or we could store klp_transition_patch to a local 
variable here in klp_cancel_transition() before klp_complete_transition() 
is called. That should be safe. I like the latter more, because it keeps 
the code in klp_cancel_transition() where it belongs.

Good points.  Here's another try:

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index e96346e..a23c63c 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c

@@ -121,8 +121,31 @@ static void klp_complete_transition(void)
  */
 void klp_cancel_transition(void)
 {
-	klp_target_state = !klp_target_state;
+	struct klp_patch *patch = klp_transition_patch;
+	struct klp_object *obj;
+	struct klp_func *func;
+	bool immediate_func = false;
+
+	if (WARN_ON_ONCE(klp_target_state != KLP_PATCHED))
+		return;
+
+	klp_target_state = KLP_UNPATCHED;
 	klp_complete_transition();
+
+	/*
+	 * 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(patch, obj)
+		klp_for_each_func(obj, func)
+			if (func->immediate)
+				immediate_func = true;
+
+	if (patch->immediate || immediate_func)
+		module_put(patch->mod);
 }