[PATCH 0/3] Fix preemption errors in watchpoints

[PATCH 0/3] Fix preemption errors in watchpoints

[Benjamin Gray]

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

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

[Benjamin Gray]

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.

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>

---

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(-)

diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index b8513dc3e53a..2854376870cf 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -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

[PATCH 2/3] powerpc/watchpoint: Disable pagefaults when getting user instruction

[Benjamin Gray]

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.

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
 arch/powerpc/kernel/hw_breakpoint_constraints.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/hw_breakpoint_constraints.c b/arch/powerpc/kernel/hw_breakpoint_constraints.c
index a74623025f3a..9e51801c4915 100644
--- a/arch/powerpc/kernel/hw_breakpoint_constraints.c
+++ b/arch/powerpc/kernel/hw_breakpoint_constraints.c
@@ -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

[PATCH 3/3] powerpc/watchpoints: Annotate atomic context in more places

[Benjamin Gray]

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.

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
 arch/powerpc/kernel/hw_breakpoint.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 2854376870cf..a1318ce18d0e 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -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

Re: [PATCH 0/3] Fix preemption errors in watchpoints

[Benjamin Gray]

On 29/8/23 4:34 pm, Benjamin Gray wrote:
> 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

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

Re: [PATCH 0/3] Fix preemption errors in watchpoints

[Michael Ellerman]

On Tue, 29 Aug 2023 16:34:54 +1000, Benjamin Gray wrote:
> 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
> 
> [...]

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