Re: [RFC][PATCH v2 10/11] livepatch: Remove klp_synchronize_transition()

On Wed 2021-09-29 17:17:33, Peter Zijlstra wrote:

The whole premise is broken, any trace usage outside of RCU is a BUG
and should be fixed. Notable all code prior (and a fair bit after)
ct_user_exit() is noinstr and explicitly doesn't allow any tracing.

I see. Is the situation the same with idle threads? I see that some
functions, called inside rcu_idle_enter()/exit() are still visible
for ftrace. For example, arch_cpu_idle() or tick_check_broadcast_expired().

That said, I am not sure if anyone would ever want to livepatch
this code. And there is still a workaround by livepatching
the callers.

Also it should be easy to catch eventual problems if we check
rcu_is_watching() in klp_ftrace_handler(). Most affected
scheduler and idle code paths will likely be called
even during the simple boot test.

Use regular RCU, which has the benefit of actually respecting
NOHZ_FULL.

Good to know.

After all, the patch looks good to me. I would just add something like:

--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c

@@ -69,6 +69,12 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 	if (WARN_ON_ONCE(!func))
 		goto unlock;
 
+	if (!rcu_is_watching()) {
+		printk_deferred_once(KERN_WARNING
+				     "warning: called \"%s\" without RCU watching. The function is not guarded by the consistency model. Also beware when removing the livepatch module.\n",
+				     func->old_name);
+	}
+
 	/*
 	 * In the enable path, enforce the order of the ops->func_stack and
 	 * func->transition reads.  The corresponding write barrier is in

But we could do this in a separate patch later.

Feel free to use:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr