Re: [RFC PATCH] arm64: jump_label: Ensure patched jump_labels are visible to all CPUs

[Marc Zyngier <maz@kernel.org>]

On Wed, 31 Jul 2024 14:36:01 +0100,
Will Deacon <will@kernel.org> wrote:
> 
> Although the Arm architecture permits concurrent modification and
> execution of NOP and branch instructions, it still requires some
> synchronisation to ensure that other CPUs consistently execute the newly
> written instruction:
> 
>  >  When the modified instructions are observable, each PE that is
>  >  executing the modified instructions must execute an ISB or perform a
>  >  context synchronizing event to ensure execution of the modified
>  >  instructions
> 
> Prior to commit f6cc0c501649 ("arm64: Avoid calling stop_machine() when
> patching jump labels"), the arm64 jump_label patching machinery
> performed synchronisation using stop_machine() after each modification,
> however this was problematic when flipping static keys from atomic
> contexts (namely, the arm_arch_timer CPU hotplug startup notifier) and
> so we switched to the _nosync() patching routines to avoid "scheduling
> while atomic" BUG()s during boot.
> 
> In hindsight, the analysis of the issue in f6cc0c501649 isn't quite
> right: it cites the use of IPIs in the default patching routines as the
> cause of the lockup, whereas stop_machine() does not rely on IPIs and
> the I-cache invalidation is performed using __flush_icache_range(),
> which elides the call to kick_all_cpus_sync(). In fact, the blocking
> wait for other CPUs is what triggers the BUG() and the problem remains
> even after f6cc0c501649, for example because we could block on the
> jump_label_mutex. Eventually, the arm_arch_timer driver was fixed to
> avoid the static key entirely in commit a862fc2254bd
> ("clocksource/arm_arch_timer: Remove use of workaround static key").
> 
> This all leaves the jump_label patching code in a funny situation on
> arm64 as we do not synchronise with other CPUs to reduce the likelihood
> of a bug which no longer exists. Consequently, toggling a static key on
> one CPU cannot be assumed to take effect on other CPUs, leading to
> potential issues, for example with missing preempt notifiers.
> 
> Rather than revert f6cc0c501649 and go back to stop_machine() for each
> patch site, implement arch_jump_label_transform_apply() and kick all
> the other CPUs with an IPI at the end of patching.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Fixes: f6cc0c501649 ("arm64: Avoid calling stop_machine() when patching jump labels")
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/include/asm/jump_label.h |  1 +
>  arch/arm64/kernel/jump_label.c      | 11 +++++++++--
>  2 files changed, 10 insertions(+), 2 deletions(-)

Thanks for the detailed archaeology! This looks pretty reasonable to
me.

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.