Fix preemption errors in watchpoints

When enabling debug config options relating to preemption, several bugs appear in the kernel log. With this series applied, the breakpoint code no longer prints bugs when running the powerpc/ptrace selftests.

Benjamin Gray (3):
powerpc/watchpoints: Disable preemption in thread_change_pc() powerpc/watchpoint: Disable pagefaults when getting user instruction powerpc/watchpoints: Annotate atomic context in more places

 arch/powerpc/kernel/hw_breakpoint.c             | 16 +++++++++++++++-
 arch/powerpc/kernel/hw_breakpoint_constraints.c |  7 ++++++-
 2 files changed, 21 insertions(+), 2 deletions(-)

--
2.41.0

thread_change_pc() uses CPU local data, so must be protected from swapping CPUs while it is reading the breakpoint struct.

The error is more noticeable after 1e60f3564bad ("powerpc/watchpoints:
Track perf single step directly on the breakpoint"), which added an unconditional __this_cpu_read() call in thread_change_pc(). However the existing __this_cpu_read() that runs if a breakpoint does need to be re-inserted has the same issue.

---

There's probably a more idiomatic way to express this. We technically don't need to disable preemption for the entire function: we should only need to disable preemption within each loop iteration while handling the pointer we are working with. Each iteration itself is independent.

---
 arch/powerpc/kernel/hw_breakpoint.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

index b8513dc3e53a..2854376870cf 100644
@@ -230,13 +230,15 @@ void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs)
 	struct arch_hw_breakpoint *info;
 	int i;
 
+	preempt_disable();
+
 	for (i = 0; i < nr_wp_slots(); i++) {
 		struct perf_event *bp = __this_cpu_read(bp_per_reg[i]);
 
 		if (unlikely(bp && counter_arch_bp(bp)->perf_single_step))
 			goto reset;
 	}
-	return;
+	goto out;
 
 reset:
 	regs_set_return_msr(regs, regs->msr & ~MSR_SE);

@@ -245,6 +247,9 @@ void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs)
 		__set_breakpoint(i, info);
 		info->perf_single_step = false;
 	}
+
+out:
+	preempt_enable();
 }
 
 static bool is_larx_stcx_instr(int type)

2.41.0

This is called in an atomic context, so is not allowed to sleep if a user page needs to be faulted in and has nowhere it can be deferred to. The pagefault_disabled() function is documented as preventing user access methods from sleeping.

In practice the page will be mapped in nearly always because we are reading the instruction that just triggered the watchpoint trap.

---
 arch/powerpc/kernel/hw_breakpoint_constraints.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

index a74623025f3a..9e51801c4915 100644
@@ -131,8 +131,13 @@ void wp_get_instr_detail(struct pt_regs *regs, ppc_inst_t *instr,
 			 int *type, int *size, unsigned long *ea)
 {
 	struct instruction_op op;
+	int err;
 
-	if (__get_user_instr(*instr, (void __user *)regs->nip))
+	pagefault_disable();
+	err = __get_user_instr(*instr, (void __user *)regs->nip);
+	pagefault_enable();
+
+	if (err)
 		return;
 
 	analyse_instr(&op, regs, *instr);

2.41.0

It can be easy to miss that the notifier mechanism invokes the callbacks in an atomic context, so add some comments to that effect on the two handlers we register here.

---
 arch/powerpc/kernel/hw_breakpoint.c | 9 +++++++++
 1 file changed, 9 insertions(+)

index 2854376870cf..a1318ce18d0e 100644
@@ -368,6 +368,11 @@ static void handle_p10dd1_spurious_exception(struct perf_event **bp,
 	}
 }
 
+/*
+ * Handle a DABR or DAWR exception.
+ *
+ * Called in atomic context.
+ */
 int hw_breakpoint_handler(struct die_args *args)
 {
 	bool err = false;

@@ -495,6 +500,8 @@ NOKPROBE_SYMBOL(hw_breakpoint_handler);
 
 /*
  * Handle single-step exceptions following a DABR hit.
+ *
+ * Called in atomic context.
  */
 static int single_step_dabr_instruction(struct die_args *args)
 {

@@ -546,6 +553,8 @@ NOKPROBE_SYMBOL(single_step_dabr_instruction);
 
 /*
  * Handle debug exception notifications.
+ *
+ * Called in atomic context.
  */
 int hw_breakpoint_exceptions_notify(
 		struct notifier_block *unused, unsigned long val, void *data)

2.41.0

The particular config is below, used by appending to a ppc64le_guest_defconfig. Not all options are relevant. Tested on a Power8 and Power10 machine (1 and 2 watchpoints).

CONFIG_LOCALVERSION="-watchpoint-sleep"
CONFIG_GENERIC_IRQ_DEBUGFS=y
CONFIG_PREEMPT=y
CONFIG_PRINTK_INDEX=y
CONFIG_CGROUP_DEBUG=y
CONFIG_PPC_KUAP_DEBUG=y
CONFIG_SCOM_DEBUGFS=y
CONFIG_UDBG_RTAS_CONSOLE=y
CONFIG_RELOCATABLE_TEST=y
CONFIG_PM_DEBUG=y
CONFIG_PM_ADVANCED_DEBUG=y
CONFIG_STATIC_KEYS_SELFTEST=y
CONFIG_MODULE_DEBUG=y
CONFIG_MODULE_STATS=y
CONFIG_MODULE_DEBUG_AUTOLOAD_DUPS_TRACE=y
CONFIG_CMA_DEBUGFS=y
CONFIG_CMA_SYSFS=y
CONFIG_PERCPU_STATS=y
CONFIG_USERFAULTFD=y
CONFIG_VALIDATE_FS_PARSER=y
CONFIG_EXT4_DEBUG=y
CONFIG_HARDENED_USERCOPY=y
CONFIG_FORTIFY_SOURCE=y
CONFIG_DYNAMIC_DEBUG=y
CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT=y
CONFIG_DEBUG_INFO_SPLIT=y
CONFIG_GDB_SCRIPTS=y
CONFIG_READABLE_ASM=y
CONFIG_NET_DEV_REFCNT_TRACKER=y
CONFIG_NET_NS_REFCNT_TRACKER=y
CONFIG_DEBUG_NET=y
CONFIG_DEBUG_PAGEALLOC=y
CONFIG_SLUB_DEBUG_ON=y
CONFIG_PTDUMP_DEBUGFS=y
CONFIG_DEBUG_KMEMLEAK=y
CONFIG_PER_VMA_LOCK_STATS=y
CONFIG_DEBUG_OBJECTS=y
CONFIG_DEBUG_OBJECTS_SELFTEST=y
CONFIG_DEBUG_OBJECTS_FREE=y
CONFIG_DEBUG_OBJECTS_TIMERS=y
CONFIG_DEBUG_OBJECTS_WORK=y
CONFIG_DEBUG_OBJECTS_RCU_HEAD=y
CONFIG_DEBUG_OBJECTS_PERCPU_COUNTER=y
CONFIG_SHRINKER_DEBUG=y
CONFIG_SCHED_STACK_END_CHECK=y
CONFIG_DEBUG_VM=y
CONFIG_DEBUG_VM_MAPLE_TREE=y
CONFIG_DEBUG_VM_RB=y
CONFIG_DEBUG_VM_PGFLAGS=y
CONFIG_DEBUG_VM_PGTABLE=y
CONFIG_DEBUG_VIRTUAL=y
CONFIG_DEBUG_PER_CPU_MAPS=y
CONFIG_DEBUG_SHIRQ=y
CONFIG_WQ_WATCHDOG=y
CONFIG_DEBUG_TIMEKEEPING=y
CONFIG_DEBUG_PREEMPT=y
CONFIG_PROVE_LOCKING=y
CONFIG_LOCK_STAT=y
CONFIG_DEBUG_ATOMIC_SLEEP=y
CONFIG_DEBUG_LOCKING_API_SELFTESTS=y
CONFIG_CSD_LOCK_WAIT_DEBUG=y
CONFIG_DEBUG_IRQFLAGS=y
CONFIG_WARN_ALL_UNSEEDED_RANDOM=y
CONFIG_DEBUG_NOTIFIERS=y
CONFIG_DEBUG_CREDENTIALS=y
CONFIG_RCU_CPU_STALL_CPUTIME=y
CONFIG_RCU_EQS_DEBUG=y
CONFIG_DEBUG_WQ_FORCE_RR_CPU=y
CONFIG_CPU_HOTPLUG_STATE_CONTROL=y
CONFIG_LATENCYTOP=y
CONFIG_PPC_IRQ_SOFT_MASK_DEBUG=y
CONFIG_PPC_RFI_SRR_DEBUG=y

Applied to powerpc/fixes.

[1/3] powerpc/watchpoints: Disable preemption in thread_change_pc()

      https://git.kernel.org/powerpc/c/cc879ab3ce39bc39f9b1d238b283f43a5f6f957d
[2/3] powerpc/watchpoint: Disable pagefaults when getting user instruction
      https://git.kernel.org/powerpc/c/3241f260eb830d27d09cc604690ec24533fdb433
[3/3] powerpc/watchpoints: Annotate atomic context in more places
      https://git.kernel.org/powerpc/c/27646b2e02b096a6936b3e3b6ba334ae20763eab

cheers