Skip to contentSkip to conversation

arm64: jump_label: Ensure patched jump_labels are visible to all CPUs

RFC PATCH Applied Will Deacon 3 participants 4 messages Jul 31, 2024 – Aug 2, 2024

Subsystems (2)
arm64 Static branch/call
lore
Share & tools
lore

Will Deacon

author maintainer
#0 lore raw

RFC PATCH 1/1 arm64: jump_label: Ensure patched jump_labels are visible to all CPUs

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:

Quoted 4 lines

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 <[email protected]>
Cc Alexander Potapenko <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Marc Zyngier <[email protected]>
Fixes f6cc0c501649 ("arm64: Avoid calling stop_machine() when patching jump labels")
Signed-off-by Will Deacon <[email protected]>
---
 arch/arm64/include/asm/jump_label.h |  1 +
 arch/arm64/kernel/jump_label.c      | 11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)
arch/arm64/include/asm/jump_label.h +1 -0
index 4e753908b801..a0a5bbae7229 100644
@@ -13,6 +13,7 @@
 #include <linux/types.h>
 #include <asm/insn.h>
 
+#define HAVE_JUMP_LABEL_BATCH
 #define JUMP_LABEL_NOP_SIZE		AARCH64_INSN_SIZE
 
 #define JUMP_TABLE_ENTRY(key, label)			\
arch/arm64/kernel/jump_label.c +9 -2
index faf88ec9c48e..f63ea915d6ad 100644
@@ -7,11 +7,12 @@
  */
 #include <linux/kernel.h>
 #include <linux/jump_label.h>
+#include <linux/smp.h>
 #include <asm/insn.h>
 #include <asm/patching.h>
 
-void arch_jump_label_transform(struct jump_entry *entry,
-			       enum jump_label_type type)
+bool arch_jump_label_transform_queue(struct jump_entry *entry,
+				     enum jump_label_type type)
 {
 	void *addr = (void *)jump_entry_code(entry);
 	u32 insn;

@@ -25,4 +26,10 @@ void arch_jump_label_transform(struct jump_entry *entry,
 	}
 
 	aarch64_insn_patch_text_nosync(addr, insn);
+	return true;
+}
+
+void arch_jump_label_transform_apply(void)
+{
+	kick_all_cpus_sync();
 }
Signature · 1 line
2.46.0.rc1.232.g9752f9e123-goog
maintainer Reviewed-by on v1 1/1
#1 lore raw
Quoted 39 lines · Will Deacon

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:

Quoted 4 lines

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.

Quoted 6 lines
Cc Catalin Marinas <[email protected]>
Cc Alexander Potapenko <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Marc Zyngier <[email protected]>
Fixes f6cc0c501649 ("arm64: Avoid calling stop_machine() when patching jump labels")
Signed-off-by Will Deacon <[email protected]>

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 <[email protected]>
Reviewed-by on v1 1/1
#2 lore raw
In reply to Will Deacon
Quoted 46 lines · Will Deacon

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:

Quoted 4 lines

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 <[email protected]>
Cc Alexander Potapenko <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Marc Zyngier <[email protected]>
Fixes f6cc0c501649 ("arm64: Avoid calling stop_machine() when patching jump labels")
Signed-off-by Will Deacon <[email protected]>
Quoted 4 lines
---
 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 <[email protected]>
	M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
maintainer Applied
#3 lore raw
In reply to Will Deacon
Quoted 11 lines · Will Deacon

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:

Quoted 4 lines

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

[...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64: jump_label: Ensure patched jump_labels are visible to all CPUs

      https://git.kernel.org/arm64/c/cfb00a357864
Signature · 1 line
Catalin