livepatch,sched: Add livepatch task switching to cond_resched()

There have been reports [1][2] of live patches failing to complete within a reasonable amount of time due to CPU-bound kthreads.

Fix it by patching tasks in cond_resched().

There are four different flavors of cond_resched(), depending on the kernel configuration. Hook into all of them.

A more elegant solution might be to use a preempt notifier. However, non-ORC unwinders can't unwind a preempted task reliably.

[1] https://lore.kernel.org/lkml/[email protected]/
[2] https://lkml.kernel.org/lkml/[email protected]

kernel/livepatch/transition.c +2 -7

@@ -588,14 +641,10 @@ void klp_reverse_transition(void)
 		 klp_target_state == KLP_PATCHED ? "patching to unpatching" :
 						   "unpatching to patching");
 
-	klp_transition_patch->enabled = !klp_transition_patch->enabled;
-
-	klp_target_state = !klp_target_state;
-
 	/*
 	 * Clear all TIF_PATCH_PENDING flags to prevent races caused by
-	 * klp_update_patch_state() running in parallel with
-	 * klp_start_transition().
+	 * klp_update_patch_state() or __klp_sched_try_switch() running in
+	 * parallel with the reverse transition.
 	 */
 	read_lock(&tasklist_lock);
 	for_each_process_thread(g, task)

@@ -605,9 +654,16 @@ void klp_reverse_transition(void)
 	for_each_possible_cpu(cpu)
 		clear_tsk_thread_flag(idle_task(cpu), TIF_PATCH_PENDING);
 
-	/* Let any remaining calls to klp_update_patch_state() complete */

Review

Quoted 3 lines

(quoted diff) +3 -0

+	/* All patching has stopped, now start the reverse transition. */
+	klp_transition_patch->enabled = !klp_transition_patch->enabled;
+	klp_target_state = !klp_target_state;

I have double checked the synchronization and we need here:

	/*
	 * Make sure klp_update_patch_state() and __klp_sched_try_switch()
	 * see the updated klp_target_state before TIF_PATCH_PENDING
	 * is set again in klp_start_transition().
	 */
	smp_wmb();

The same is achieved by smp_wmb() in klp_init_transition().

Note that the extra barrier was missing here because klp_target_state was set before klp_synchronize_transition(). It was fine because klp_update_patch_state() was called on locations where a transition in any direction was always safe.

Just for record. We need to modify @klp_target_state after klp_synchronize_transition() now. The value is used by __klp_sched_try_switch() to decide when the transition is safe. It defines what functions must not be on the stack.

Quoted 20 lines · Petr Mladek

Review

Quoted 3 lines

(quoted diff) +3 -0

+	/* All patching has stopped, now start the reverse transition. */
+	klp_transition_patch->enabled = !klp_transition_patch->enabled;
+	klp_target_state = !klp_target_state;

I have double checked the synchronization and we need here:

	/*
	 * Make sure klp_update_patch_state() and __klp_sched_try_switch()
	 * see the updated klp_target_state before TIF_PATCH_PENDING
	 * is set again in klp_start_transition().
	 */
	smp_wmb();

The same is achieved by smp_wmb() in klp_init_transition().

Note that the extra barrier was missing here because klp_target_state was set before klp_synchronize_transition(). It was fine because klp_update_patch_state() was called on locations where a transition in any direction was always safe.

Quoted 4 lines

Just for record. We need to modify @klp_target_state after klp_synchronize_transition() now. The value is used by __klp_sched_try_switch() to decide when the transition is safe. It defines what functions must not be on the stack.

Yes, makes sense. And we need a corresponding smp_rmb() in __klp_sched_try_switch() before the call to klp_try_switch_task(), right?

Something like this on top? Also updated a few more comments.

kernel/livepatch/transition.c +2 -2

index b9e006632124..218ef4a5d575 100644
@@ -192,8 +192,8 @@ void klp_update_patch_state(struct task_struct *task)
 	 * barrier (smp_rmb) for two cases:
 	 *
 	 * 1) Enforce the order of the TIF_PATCH_PENDING read and the
-	 *    klp_target_state read.  The corresponding write barrier is in
-	 *    klp_init_transition().
+	 *    klp_target_state read.  The corresponding write barriers are in
+	 *    klp_init_transition() and klp_reverse_transition().
 	 *
 	 * 2) Enforce the order of the TIF_PATCH_PENDING read and a future read
 	 *    of func->transition, if klp_ftrace_handler() is called later on

kernel/livepatch/transition.c +4 -2

+
 	klp_try_switch_task(current);
 
 out:

@@ -604,8 +612,9 @@ void klp_init_transition(struct klp_patch *patch, int state)
 	 * see a func in transition with a task->patch_state of KLP_UNDEFINED.
 	 *
 	 * Also enforce the order of the klp_target_state write and future
-	 * TIF_PATCH_PENDING writes to ensure klp_update_patch_state() doesn't
-	 * set a task->patch_state to KLP_UNDEFINED.
+	 * TIF_PATCH_PENDING writes to ensure klp_update_patch_state() and
+	 * __klp_sched_try_switch() don't set a task->patch_state to
+	 * KLP_UNDEFINED.
 	 */
 	smp_wmb();
 

kernel/livepatch/transition.c void __klp_sched_try_switch(void) · line 390

Quoted 11 lines

kernel/livepatch/transition.c +7 -0

@@ -381,6 +381,14 @@ void __klp_sched_try_switch(void)
 	if (unlikely(!klp_patch_pending(current)))
 		goto out;
 
+	/*
+	 * Enforce the order of the TIF_PATCH_PENDING read above and the
+	 * klp_target_state read in klp_try_switch_task().  The corresponding
+	 * write barriers are in klp_init_transition() and
+	 * klp_reverse_transition().
+	 */
+	smp_rmb();

This barrier has basically the same purpose as the implicit read barrier in klp_update_patch_state().

The comment in klp_update_patch_state() says that the read barrier actually has two purposes. The 1st one is easy. It is the one described above.

It took me quite some time to understand the 2nd purpose again. The original comment was:

* 2) Enforce the order of the TIF_PATCH_PENDING read and a future read
* of func->transition, if klp_ftrace_handler() is called later on
* the same CPU. See __klp_disable_patch().

I think that a better description would be:

* 2) Make sure that this CPU sees func->transition enabled when
* it sees the TIF_PATCH_PENDING enabled. This is important when
* the current task is transitioning itself and then calls
* klp_ftrace_handler() later. It ensures that the ftrace handler
* would check the state change that we did here.
* The corresponding write barrier is in __klp_enable_patch()
* and __klp_disable_patch().

Note that the previous comment wasn't correct. IMHO, the related write barrier is needed in both __klp_enable_patch() and __klp_disable_patch().

kernel/livepatch/transition.c void klp_reverse_transition(void) · line 674

Quoted 7 lines

kernel/livepatch/transition.c +2 -1

@@ -661,9 +670,19 @@ void klp_reverse_transition(void)
 	 */
 	klp_synchronize_transition();
 
-	/* All patching has stopped, now start the reverse transition. */
+	/* All patching has stopped, now start the reverse transition: */
+

Is the extra empty line intended?

Quoted 9 lines

kernel/livepatch/transition.c +7 -0

 	klp_transition_patch->enabled = !klp_transition_patch->enabled;
 	klp_target_state = !klp_target_state;
+
+	/*
+	 * Enforce the order of the klp_target_state write and the
+	 * TIF_PATCH_PENDING writes in klp_start_transition() to ensure
+	 * klp_update_patch_state() and __klp_sched_try_switch() don't set
+	 * task->patch_state to the wrong value.
+	 */

kernel/livepatch/transition.c void klp_reverse_transition(void) · line 687

Quoted 4 lines

kernel/livepatch/transition.c +2 -0

+	smp_wmb();
+
 	klp_start_transition();
 }

This made me to revisit all the barriers in the livepatch code. The good thing is that it seems that all the barriers are correct, including the new ones proposed in this patchset.

But some comments are a bit misleading. I would like to update them a bit. I have started working on it but it goes slowly. I often get lost...

I am not sure about the ordering. I do not want to block this patchset by the clean up of the comments. The currently proposed ones are good enough. Feel free to send v3.

Or would you prefer to wait for my clean up of the comments?