livepatch: Fix stacking of patches with respect to RCU

rcu_read_(un)lock(), list_*_rcu(), and synchronize_rcu() are used for a secure
access and manipulation of the list of patches that modify the same function.
In particular, it is the variable func_stack that is accessible from the ftrace
handler via struct ftrace_ops and klp_ops.

Of course, it synchronizes also some states of the patch on the top of the
stack, e.g. func->transition in klp_ftrace_handler.

At the same time, this mechanism guards also the manipulation of
task->patch_state. It is modified according to the state of the transition and
the state of the process.

Now, all this works well as long as RCU works well. Sadly livepatching might
get into some corner cases when this is not true. For example, RCU is not
watching when rcu_read_lock() is taken in idle threads.  It is because they
might sleep and prevent reaching the grace period for too long.

There are ways how to make RCU watching even in idle threads, see
rcu_irq_enter(). But there is a small location inside RCU infrastructure when
even this does not work.

This small problematic location can be detected either before calling
rcu_irq_enter() by rcu_irq_enter_disabled() or later by rcu_is_watching().
Sadly, there is no safe way how to handle it.  Once we detect that RCU was not
watching, we might see inconsistent state of the function stack and the related
variables in klp_ftrace_handler(). Then we could do a wrong decision, use an
incompatible implementation of the function and break the consistency of the
system. We could warn but we could not avoid the damage.

Fortunately, ftrace has similar problems and they seem to be solved well there.
It uses a heavy weight implementation of some RCU operations. In particular, it
replaces:

  + rcu_read_lock() with preempt_disable_notrace()
  + rcu_read_unlock() with preempt_enable_notrace()
  + synchronize_rcu() with schedule_on_each_cpu(sync_work)

My understanding is that this is RCU implementation from a stone age. It meets
the core RCU requirements but it is rather ineffective. Especially, it does not
allow to batch or speed up the synchronize calls.

On the other hand, it is very trivial. It allows to safely trace and/or
livepatch even the RCU core infrastructure.  And the effectiveness is a not a
big issue because using ftrace or livepatches on productive systems is a rare
operation.  The safety is much more important than a negligible extra load.

Note that the alternative implementation follows the RCU principles. Therefore,
     we could and actually must use list_*_rcu() variants when manipulating the
     func_stack.  These functions allow to access the pointers in the right
     order and with the right barriers. But they do not use any other
     information that would be set only by rcu_read_lock().

Also note that there are actually two problems solved in ftrace:

First, it cares about the consistency of RCU read sections.  It is being solved
the way as described and used in this patch.

Second, ftrace needs to make sure that nobody is inside the dynamic trampoline
when it is being freed. For this, it also calls synchronize_rcu_tasks() in
preemptive kernel in ftrace_shutdown().

Livepatch has similar problem but it is solved by ftrace for free.
klp_ftrace_handler() is a good guy and never sleeps. In addition, it is
registered with FTRACE_OPS_FL_DYNAMIC. It causes that
unregister_ftrace_function() calls:

	* schedule_on_each_cpu(ftrace_sync) - always
	* synchronize_rcu_tasks() - in preemptive kernel

The effect is that nobody is neither inside the dynamic trampoline nor inside
the ftrace handler after unregister_ftrace_function() returns.

[jkosina@suse.cz: reformat changelog, fix comment]
Signed-off-by: Petr Mladek <pmladek@suse.com>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>

diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index f8269036bf0b84f89faa2db3b5dc5fe4c259a019..52c4e907c14b0d4a7896d7ca9a6fa6fbc17451ba 100644 (file)
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -59,7 +59,11 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 
        ops = container_of(fops, struct klp_ops, fops);
 
-       rcu_read_lock();
+       /*
+        * A variant of synchronize_sched() is used to allow patching functions
+        * where RCU is not watching, see klp_synchronize_transition().
+        */
+       preempt_disable_notrace();
 
        func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
                                      stack_node);
@@ -115,7 +119,7 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 
        klp_arch_set_pc(regs, (unsigned long)func->new_func);
 unlock:
-       rcu_read_unlock();
+       preempt_enable_notrace();
 }
 
 /*

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index adc0cc64aa4b6ae199e5f4a5b2af0ad59c382175..b004a1fb603236f31cd5780962c098bfcf8fe743 100644 (file)
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -48,6 +48,28 @@ static void klp_transition_work_fn(struct work_struct *work)
 }
 static DECLARE_DELAYED_WORK(klp_transition_work, klp_transition_work_fn);
 
+/*
+ * This function is just a stub to implement a hard force
+ * of synchronize_sched(). This requires synchronizing
+ * tasks even in userspace and idle.
+ */
+static void klp_sync(struct work_struct *work)
+{
+}
+
+/*
+ * We allow to patch also functions where RCU is not watching,
+ * e.g. before user_exit(). We can not rely on the RCU infrastructure
+ * to do the synchronization. Instead hard force the sched synchronization.
+ *
+ * This approach allows to use RCU functions for manipulating func_stack
+ * safely.
+ */
+static void klp_synchronize_transition(void)
+{
+       schedule_on_each_cpu(klp_sync);
+}
+
 /*
  * The transition to the target patch state is complete.  Clean up the data
  * structures.
@@ -73,7 +95,7 @@ static void klp_complete_transition(void)
                 * func->transition gets cleared, the handler may choose a
                 * removed function.
                 */
-               synchronize_rcu();
+               klp_synchronize_transition();
        }
 
        if (klp_transition_patch->immediate)
@@ -92,7 +114,7 @@ static void klp_complete_transition(void)
 
        /* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
        if (klp_target_state == KLP_PATCHED)
-               synchronize_rcu();
+               klp_synchronize_transition();
 
        read_lock(&tasklist_lock);
        for_each_process_thread(g, task) {
@@ -136,7 +158,11 @@ void klp_cancel_transition(void)
  */
 void klp_update_patch_state(struct task_struct *task)
 {
-       rcu_read_lock();
+       /*
+        * A variant of synchronize_sched() is used to allow patching functions
+        * where RCU is not watching, see klp_synchronize_transition().
+        */
+       preempt_disable_notrace();
 
        /*
         * This test_and_clear_tsk_thread_flag() call also serves as a read
@@ -153,7 +179,7 @@ void klp_update_patch_state(struct task_struct *task)
        if (test_and_clear_tsk_thread_flag(task, TIF_PATCH_PENDING))
                task->patch_state = READ_ONCE(klp_target_state);
 
-       rcu_read_unlock();
+       preempt_enable_notrace();
 }
 
 /*
@@ -539,7 +565,7 @@ void klp_reverse_transition(void)
                clear_tsk_thread_flag(idle_task(cpu), TIF_PATCH_PENDING);
 
        /* Let any remaining calls to klp_update_patch_state() complete */
-       synchronize_rcu();
+       klp_synchronize_transition();
 
        klp_start_transition();
 }