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

[Josh Poimboeuf]

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