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

[Catalin Marinas <catalin.marinas@arm.com>]

On Wed, Jul 31, 2024 at 02:36:01PM +0100, Will Deacon 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>

We need to keep an eye so that the patch is not picked up for 4.19 (the
fixed commit) as it doesn't have the arm_arch_timer fix, nor the batch
jump label support. LTS 5.4 is fine though, it has both. If we want it
in -stable, we can explicitly mention the version in the Cc line.

The patch looks good to me.

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>